-
Notifications
You must be signed in to change notification settings - Fork 142
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
Fixed adding a Gitlab app via UI #1146
Conversation
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.
Nice find!
Edit: Nit: can we add some test coverage? Like maybe we should validate that the expiration is what we expect it to be
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 catch! Minor nit on the fix.
+1 on adding a test(s)
@@ -633,7 +633,7 @@ func (s *applicationServer) AuthorizeGitlab(ctx context.Context, msg *pb.Authori | |||
return nil, fmt.Errorf("could not exchange code: %w", err) | |||
} | |||
|
|||
token, err := s.jwtClient.GenerateJWT(time.Duration(tokenState.ExpiresIn), gitproviders.GitProviderGitLab, tokenState.AccessToken) | |||
token, err := s.jwtClient.GenerateJWT(time.Duration(tokenState.ExpiresIn)*time.Second, gitproviders.GitProviderGitLab, tokenState.AccessToken) |
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 but multiplying by seconds here makes the code a bit obscure. From looking at the code it appears that only GitLab (today) uses the ExpiresIn field. However, others might. The code is more readable (to me) if we add a function on tokenState to return the expires value in seconds - or another field ExpiresInSeconds. Then the individual providers can provide the conversion - if necessary.
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.
You don't need the type assertion time.Duration
either, because multiplying an integer by time.Duration will get you a time.Duration.
Closes:
What changed?
Interpret integer as seconds
Why?
It was interpreting the integer as milliseconds instead of seconds, causing unauthorized errors as the token only had a few seconds of validity.
How did you test it?
I manually added an app via UI, getting the next message afterwards:
Release notes
Documentation Changes