feat: add static token option in client #187
Conversation
There was a problem hiding this comment.
hey @TakumiHaruta
I was finally able to have a look at your PR. Thanks for your patience and the PR itself 😃
Please have a look at the comments. And i have an additional question:
In your issue you describe that you intend to use pass access token from the header of your frontend. How do you handle this if you use a static token? Don't you need to create a new client for every request?
| //There returned token will be used for authorization in all calls | ||
| //if expired, the token will be automatically refreshed | ||
| func NewAuthenticator(issuer string, jwtProfileTokenSource JWTProfileTokenSource, scopes ...string) (*AuthInterceptor, error) { | ||
| func NewJWTProfileAuthenticator(issuer string, jwtProfileTokenSource JWTProfileTokenSource, scopes ...string) (*AuthInterceptor, error) { |
There was a problem hiding this comment.
although it's probably not used directly in other projects, renaming it would be a breaking change... but it could be solved with a wrapper method:
...
// Deprecated: use NewJWTProfileAuthenticator instead
func NewAuthenticator(issuer string, jwtProfileTokenSource JWTProfileTokenSource, scopes ...string) (*AuthInterceptor, error) {
return NewJWTProfileAuthenticator(issuer, jwtProfileTokenSource, scopes...)
}
func NewJWTProfileAuthenticator(issuer string, jwtProfileTokenSource JWTProfileTokenSource, scopes ...string) (*AuthInterceptor, error) {
...
| func (a StaticTokenSource) RequireTransportSecurity() bool { | ||
| return false | ||
| } |
There was a problem hiding this comment.
IMO it should be possible to configure if TLS is used or not and i would prefer to have it enabled by default and that you would need to disable it explicitly
| if c.staticTokenSource == "" { | ||
| err := c.setInterceptors(c.issuer, c.orgID, c.scopes, c.jwtProfileTokenSource) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| dialOptions = append(dialOptions, | ||
| grpc.WithChainUnaryInterceptor( | ||
| c.unaryInterceptors..., | ||
| ), | ||
| grpc.WithChainStreamInterceptor( | ||
| c.streamInterceptors..., | ||
| ), | ||
| ) | ||
| } else { | ||
| c.setCredentials(c.staticTokenSource) | ||
| dialOptions = append(dialOptions, | ||
| grpc.WithPerRPCCredentials( | ||
| c.perRPCCredentials, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
the setInterceptors method also sets an interceptor for the x-zitadel-orgid header (allows to specify another organisation if needed)
IMO the static token version might need that as well
|
Hey @TakumiHaruta Just wanted to check the state on this PR. Maybe you missed my review. |
|
@livio-a |
no worries, just wanted to check. i will be on vacation but might check afterwards if i find a solution |
hifabienne
added
the
os-contribution
|
This PR is more than a year old, and the go lib has been refactored since then. |
Description
Close #186
NewAuthenticatortoNewJWTProfileAuthenticatorsince this function is JWT profile specific.StaticTokenSourcetype forcredentials.PerRPCCredentialsinterfaceNewConnectionand switch authentication method byc.staticTokenSourceis empty or notNote
The function
NewConnectionitself is a bit tightly-coupled to JWT profile method only, so I think the condition to switch auth methods insideNewConnectionis not quite clean. Please let me know if you have an idea how to handle this in a better way