-
Notifications
You must be signed in to change notification settings - Fork 49
feat: oauth feature #267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: oauth feature #267
Conversation
"github.com/twilio/twilio-go/oauth" | ||
|
||
"github.com/pkg/errors" | ||
"github.com/twilio/twilio-go/client/form" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: import ordering.
func NewClientCredentials(grantType, clientId, clientSecret string, handler RequestHandler) *ClientCredentials { | ||
return &ClientCredentials{GrantType: grantType, ClientId: clientId, ClientSecret: clientSecret, RequestHandler: handler} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not much readable, can we try running a linter (preferably go-fumpt)
@@ -0,0 +1 @@ | |||
package twilio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we please include tests and try to have the patch coverage.
tokenManager: tokenManager, | ||
} | ||
} | ||
func NewTokenAuth(grantType, clientId, clientSecret, code, audience, refreshToken, scope string) *TokenAuth { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, this is not being used anywhere.
tokenManager *TokenManager | ||
} | ||
|
||
func NewTokenAuthInitializer(token string, tokenManager *TokenManager) *TokenAuth { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can rename NewTokenAuthInitializer
to NewTokenAuth
. Any func prefixed with New
is already considered as a constructor func
} | ||
} | ||
|
||
func (t *TokenAuth) FetchToken(c client.RequestHandler) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to have func level comment to get quick glimpse of what func does and also helps when func is referenced in other packages.
return true | ||
} | ||
|
||
if claims, ok := token.Claims.(jwt.MapClaims); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to catch err here if token.Claims do not assert to jwt.MapClaims
? in what case that can happen, need more info here
Fixes
Oauth feature
Checklist
If you have questions, please file a support ticket, or create a GitHub Issue in this repository.