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: Add support for role mapping and allowed groups in Google OIDC #76266

Merged
merged 7 commits into from
Oct 10, 2023
Merged

Auth: Add support for role mapping and allowed groups in Google OIDC #76266

merged 7 commits into from
Oct 10, 2023

Conversation

Jguer
Copy link
Contributor

@Jguer Jguer commented Oct 10, 2023 •
edited
Loading

What is this feature?

Add support for the following oauth config fields

[auth.google]
allowed_groups = ...
role_attribute_strict = ...
role_attribute_path = ...

Closes #72797
Closes #72824
Closes #72869
Closes #72870
Closes #72871

Why do we need this feature?

[Add a description of the problem the feature is trying to solve.]

Who is this feature for?

[Add information on what kind of user the feature is for.]

Which issue(s) does this PR fix?:

Fixes #

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.2.x milestone Oct 10, 2023
@Jguer Jguer self-assigned this Oct 10, 2023
@Jguer Jguer requested a review from a team as a code owner October 10, 2023 13:28
@Jguer Jguer requested review from gamab and kalleep and removed request for a team October 10, 2023 13:28
kalleep
kalleep approved these changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@kalleep kalleep left a comment

Choose a reason for hiding this comment

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

Looks good, left one comment but it is not a blocker

pkg/login/social/gitlab_oauth.go Show resolved Hide resolved
@Jguer Jguer requested review from torkelo, a team and chri2547 as code owners October 10, 2023 13:47
@Jguer Jguer requested review from papagian, zserge and suntala and removed request for a team October 10, 2023 13:47
IevaVasiljeva
IevaVasiljeva approved these changes Oct 10, 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.

Looks good (although I didn't test it myself). Thanks for getting it in so fast!

gamab
gamab reviewed Oct 10, 2023
View reviewed changes
pkg/setting/setting.go Outdated
// FIXME: for now we skip org role sync for google auth
// as we do not sync organization roles from Google
cfg.GoogleSkipOrgRoleSync = true
cfg.GoogleSkipOrgRoleSync = sec.Key("skip_org_role_sync").MustBool(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

This contradicts with the defaults.ini:
skip_org_role_sync = true

gamab
gamab approved these changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@gamab gamab left a comment

Choose a reason for hiding this comment

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

Very good job 🎉
I left two suggestions for the docs 👍

Jguer
Jguer commented Oct 10, 2023
View reviewed changes
pkg/setting/setting.go Outdated Show resolved Hide resolved
@Jguer @gamab
Apply suggestions from code review
72e0dbb 
Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>
@Jguer Jguer merged commit cada1f0 into main Oct 10, 2023
16 checks passed
@Jguer Jguer deleted the jguer/google-oauth-parity branch October 10, 2023 16:07
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
@dnhn dnhn mentioned this pull request Oct 24, 2023
Closed
@BrewTestBot BrewTestBot mentioned this pull request Oct 25, 2023
Closed
@mgyongyosi mgyongyosi mentioned this pull request Dec 14, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gamab gamab gamab approved these changes

@IevaVasiljeva IevaVasiljeva IevaVasiljeva approved these changes

@kalleep kalleep kalleep approved these changes

@chri2547 chri2547 chri2547 approved these changes

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

@papagian papagian Awaiting requested review from papagian papagian was automatically assigned from grafana/backend-platform

@zserge zserge Awaiting requested review from zserge zserge was automatically assigned from grafana/backend-platform

@suntala suntala Awaiting requested review from suntala suntala was automatically assigned from grafana/backend-platform

Assignees

@Jguer Jguer

Labels
add to changelog add to what's new area/backend no-backport Skip backport of PR type/docs
Projects
None yet
Milestone
10.2.0
6 participants
@Jguer @gamab @IevaVasiljeva @kalleep @chri2547 @zerok