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
OAuth: Refactor OAuth parameters handling to support obtaining refresh tokens for Google OAuth #58782
Conversation
* Extract access token validity check to a function
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/42156 |
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/42284 |
3ae1493
to
8a93e18
Compare
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/42322 |
1bc556f
to
645bc1f
Compare
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/42324 |
645bc1f
to
70b88a7
Compare
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/42333 |
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/42364 |
// FIXME: access_type is a Google OAuth2 specific thing, consider refactoring this and moving to google_oauth.go | ||
opts := []oauth2.AuthCodeOption{oauth2.AccessTypeOffline} | ||
|
||
var opts []oauth2.AuthCodeOption |
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.
With these last changes, the extra AuthCodeOption
s that were added to gitlab,github and azure will be removed regardless of the feature flag.
From our discussion I think that's fine since it's not used by these providers but it's a small increase in risk compared to the previous version that we should be aware of
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/42400 |
690387f
to
92540f7
Compare
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.
LGTM
…h tokens for Google OAuth (#58782) * Add ApprovalForce to AuthCodeOptions * Extract access token validity check to a function * Refactor * Oauth: set options internally instead of exposing new function * Align tests * Remove unused function Co-authored-by: Karl Persson <kalle.persson@grafana.com> (cherry picked from commit 9c98314)
…ng refresh tokens for Google OAuth (#58940) OAuth: Refactor OAuth parameters handling to support obtaining refresh tokens for Google OAuth (#58782) * Add ApprovalForce to AuthCodeOptions * Extract access token validity check to a function * Refactor * Oauth: set options internally instead of exposing new function * Align tests * Remove unused function Co-authored-by: Karl Persson <kalle.persson@grafana.com> (cherry picked from commit 9c98314) Co-authored-by: Misi <mgyongyosi@users.noreply.github.com>
…ng refresh tokens for Google OAuth (grafana#58940) OAuth: Refactor OAuth parameters handling to support obtaining refresh tokens for Google OAuth (grafana#58782) * Add ApprovalForce to AuthCodeOptions * Extract access token validity check to a function * Refactor * Oauth: set options internally instead of exposing new function * Align tests * Remove unused function Co-authored-by: Karl Persson <kalle.persson@grafana.com> (cherry picked from commit 9c98314) Co-authored-by: Misi <mgyongyosi@users.noreply.github.com>
What is this feature?
These changes make sure that Grafana does get a refresh token from Google OAuth each time the user logs in to Grafana using Google as an OAuth IdP. To make this happen the responsibility to specify custom query string parameters (that are sent to the authorize endpoint) have moved to the separate OAuth connectors.
This functionality is only available when the
accessTokenExpirationCheck
feature toggle is enabled.Why do we need this feature?
We discovered that without the
prompt=consent
parameter the refresh token is only returned when the user first logs into Grafana through Google OAuth.Who is this feature for?
Special notes for your reviewer: