Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Auth: Support google OIDC and group fetching #70140

Merged
merged 6 commits into from
Jun 26, 2023
Merged

Auth: Support google OIDC and group fetching #70140

merged 6 commits into from
Jun 26, 2023

Conversation

Jguer
Copy link
Contributor

@Jguer Jguer commented Jun 15, 2023 •
edited
Loading

What is this feature?

  • Add group fetching and teamsync support to auth.google
  • Support google OIDC endpoints

Closes #69520
Closes #69521
Implements #70081

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@Jguer Jguer added this to the 10.1.x milestone Jun 15, 2023
@Jguer Jguer self-assigned this Jun 15, 2023
@Jguer Jguer requested review from torkelo, a team and chri2547 as code owners June 15, 2023 10:23
@grafanabot grafanabot added type/docs Flags the technical writing team for documentation support; auto adds to org-wide docs project area/backend labels Jun 15, 2023
arukiidou and others added 2 commits June 15, 2023 12:30
@arukiidou @Jguer
Auth: Update Google OAuth default configuration based on /.well-known…
892e197 
…/openid-configuration #69520

Signed-off-by: junya koyama <arukiidou@yahoo.co.jp>
@Jguer
add id_token parsing
918ecce 
add legacy API distinction

use google auth oidc connectors

add group fetching support and tests
@Jguer Jguer force-pushed the jguer/actualize-gauth-oidc branch from 5772b9e to 918ecce Compare June 15, 2023 10:30
jmatosgrafana
jmatosgrafana reviewed Jun 15, 2023
View reviewed changes
pkg/login/social/google_oauth.go

groups := []string{}

url := fmt.Sprintf("%s?query=member_key_id=='%s'", googleIAMGroupsEndpoint, userData.Email)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because Email has been provided by Google I don't believe it could be manipulated to inject unexpected characters in the url

jmatosgrafana
jmatosgrafana approved these changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@jmatosgrafana jmatosgrafana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from a security point of view

chri2547
chri2547 reviewed Jun 15, 2023
View reviewed changes
docs/sources/setup-grafana/configure-security/configure-authentication/google/index.md Outdated

2. Add the `https://www.googleapis.com/auth/cloud-identity.groups.readonly` scope to your Grafana `[auth.google]` configuration:

Example:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent

chri2547
chri2547 reviewed Jun 15, 2023
View reviewed changes
docs/sources/setup-grafana/configure-security/configure-authentication/google/index.md Outdated
Comment on lines 115 to 119
```ini
[auth.google]
# ..
scopes = openid email profile https://www.googleapis.com/auth/cloud-identity.groups.readonly
```
Copy link
Collaborator

@chri2547 chri2547 Jun 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent so that this content is nested under the step.

chri2547
chri2547 requested changes Jun 15, 2023
View reviewed changes
Copy link
Collaborator

@chri2547 chri2547 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggestions. Thank you for the contribution!

IevaVasiljeva
IevaVasiljeva reviewed Jun 16, 2023
View reviewed changes
Copy link
Contributor

@IevaVasiljeva IevaVasiljeva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good, but I've left some comments for edge cases that might need to be covered.

pkg/login/social/google_oauth.go Outdated Show resolved Hide resolved
pkg/login/social/google_oauth.go Outdated Show resolved Hide resolved
pkg/login/social/google_oauth.go Outdated Show resolved Hide resolved
pkg/login/social/google_oauth.go Outdated Show resolved Hide resolved
pkg/login/social/google_oauth.go Show resolved Hide resolved
pkg/login/social/google_oauth.go Outdated Show resolved Hide resolved
pkg/login/social/google_oauth_test.go Outdated Show resolved Hide resolved
pkg/login/social/google_oauth_test.go Show resolved Hide resolved
Jguer and others added 3 commits June 16, 2023 15:11
@Jguer @chri2547 @IevaVasiljeva
Apply suggestions from code review
7627f3e 
Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>
@Jguer
implement review feedback
6468369
@Jguer
indent docs
146a357
chri2547
chri2547 reviewed Jun 16, 2023
View reviewed changes
docs/sources/setup-grafana/configure-security/configure-authentication/google/index.md

### Configure team sync for Google OAuth

> Available in Grafana v10.1.0 and later versions.
Copy link
Collaborator

@chri2547 chri2547 Jun 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we version our docs, we don't need to add this kind of note. You can remove it.

Make sure you add this to the v10.1 what's new document!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @chri2547 I'm ok with removing but we end up getting PRs to re-add as users are used to using the latest version of the documentation. Could you provide further guidance?

Example: #69419

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Jguer , I checked with the team because you made a good point that I hadn't considered. Our guidance is to keep these kinds of notes in, and when they get stale (for example, 9.5 docs refer to 7.x), remove them as we come across them.

chri2547
chri2547 reviewed Jun 16, 2023
View reviewed changes
docs/sources/setup-grafana/configure-security/configure-authentication/google/index.md

### Configure team sync for Google OAuth

> Available in Grafana v10.1.0 and later versions.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
> Available in Grafana v10.1.0 and later versions.

chri2547
chri2547 approved these changes Jun 16, 2023
View reviewed changes
Copy link
Collaborator

@chri2547 chri2547 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One additional suggestion and approved. Thank you for the contribution!

@Jguer Jguer requested a review from chri2547 June 19, 2023 07:37
@Jguer Jguer merged commit 11d196e into main Jun 26, 2023
@Jguer Jguer deleted the jguer/actualize-gauth-oidc branch June 26, 2023 07:44
@mgyongyosi mgyongyosi mentioned this pull request Jun 26, 2023
Closed
LudoVio pushed a commit that referenced this pull request Jun 26, 2023
@Jguer @arukiidou
@chri2547 @IevaVasiljeva @LudoVio
Auth: Support google OIDC and group fetching (#70140)
3156be2 
* Auth: Update Google OAuth default configuration based on /.well-known/openid-configuration #69520

Signed-off-by: junya koyama <arukiidou@yahoo.co.jp>

* add id_token parsing

add legacy API distinction

use google auth oidc connectors

add group fetching support and tests

* Apply suggestions from code review

Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>

* implement review feedback

* indent docs

---------

Signed-off-by: junya koyama <arukiidou@yahoo.co.jp>
Co-authored-by: junya koyama <arukiidou@yahoo.co.jp>
Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>
harisrozajac pushed a commit that referenced this pull request Jun 29, 2023
@Jguer @arukiidou
@chri2547 @IevaVasiljeva @harisrozajac
Auth: Support google OIDC and group fetching (#70140)
1794503 
* Auth: Update Google OAuth default configuration based on /.well-known/openid-configuration #69520

Signed-off-by: junya koyama <arukiidou@yahoo.co.jp>

* add id_token parsing

add legacy API distinction

use google auth oidc connectors

add group fetching support and tests

* Apply suggestions from code review

Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>

* implement review feedback

* indent docs

---------

Signed-off-by: junya koyama <arukiidou@yahoo.co.jp>
Co-authored-by: junya koyama <arukiidou@yahoo.co.jp>
Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>
@Jguer Jguer mentioned this pull request Jul 20, 2023
Merged
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
@chenrui333 chenrui333 mentioned this pull request Aug 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmatosgrafana jmatosgrafana jmatosgrafana approved these changes

@chri2547 chri2547 chri2547 approved these changes

@mgyongyosi mgyongyosi mgyongyosi approved these changes

@torkelo torkelo Awaiting requested review from torkelo torkelo is a code owner

@IevaVasiljeva IevaVasiljeva Awaiting requested review from IevaVasiljeva

Assignees

@Jguer Jguer

Labels
add to changelog add to what's new area/backend no-backport Skip backport of PR type/docs Flags the technical writing team for documentation support; auto adds to org-wide docs project
Projects
None yet
Milestone
10.1.0
Development

Successfully merging this pull request may close these issues.

Auth: Google OAuth default configuration are different from .well-known/openid-configuration
8 participants
@Jguer @mgyongyosi @IevaVasiljeva @chri2547 @jmatosgrafana @ricky-undeadcoders @grafanabot @arukiidou