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

SAML: Add option to skip org role sync #55230

Merged
merged 5 commits into from Sep 15, 2022
Merged

SAML: Add option to skip org role sync #55230

merged 5 commits into from Sep 15, 2022

Conversation

gamab
Copy link
Contributor

@gamab gamab commented Sep 15, 2022 •
edited

What this PR does / why we need it:

Adding an option for users that don't want user organizations and roles to be synchronized with the IdP. They can use the skip_org_role_sync configuration option.

Example configuration:

[auth.saml]
skip_org_role_sync = true

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Enterprise part of this: https://github.com/grafana/grafana-enterprise/pull/3848

@gamab gamab added add to changelog no-backport Skip backport of PR labels Sep 15, 2022
@gamab gamab added this to the 9.3.0 milestone Sep 15, 2022
@gamab gamab assigned gamab and Jguer Sep 15, 2022
@gamab gamab changed the base branch from main to jguer-gamab/respect-skip-oauth September 15, 2022 11:51
@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/33574

@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/33580

@gamab gamab marked this pull request as ready for review September 15, 2022 14:30
@gamab gamab requested review from a team as code owners September 15, 2022 14:30
@gamab gamab requested a review from a team September 15, 2022 14:30
@gamab gamab requested review from a team as code owners September 15, 2022 14:30
@gamab gamab requested review from joshhunt, ashharrison90 and jackw and removed request for a team September 15, 2022 14:30
@gamab gamab requested review from Jguer and Clarity-89 and removed request for ying-jeanne, jdbaldry and alyssawada September 15, 2022 14:35
@gamab gamab modified the milestones: 9.3.0, 9.2.0 Sep 15, 2022
gamab and others added 3 commits September 15, 2022 16:47
@gamab @Jguer
Remove update from config option name
4c34c91 
Co-authored-by: Jguer <joao.guerreiro@grafana.com>
@gamab @Jguer
Remove update from config option name
ede6c39 
Co-authored-by: Jguer <joao.guerreiro@grafana.com>
@gamab
Fix typo
4e46927
@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/33623

@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/33626

@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/33633

@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/33634

@gamab gamab merged commit 1b62507 into jguer-gamab/respect-skip-oauth Sep 15, 2022
@gamab gamab deleted the gamab-jguer/saml-skip-sync branch September 15, 2022 15:33
Jguer added a commit that referenced this pull request Sep 15, 2022
@Jguer @gamab
@Clarity-89 @joshhunt
Auth: Allow admins to manually change oauth user role if `oauth_skip_…
3e2e9f9 
…org_role_update_sync` is enabled (#55182)

* Auth: Allow admins to change oauth user info it it's not synced.

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>

* Update public/app/features/admin/UserAdminPage.tsx

Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>

* Add missing import

* Simplify init

Co-authored-by: Josh Hunt <joshhunt@users.noreply.github.com>
Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>

* SAML: Add option to skip org role sync (#55230)

* SAML: Add option to skip org role sync

* Modify frontend accordingly

* Remove update from config option name

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Remove update from config option name

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Fix typo

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>
Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>
Co-authored-by: gamab <gabi.mabs@gmail.com>
Co-authored-by: Josh Hunt <joshhunt@users.noreply.github.com>
@lpskdl
Copy link
Contributor

lpskdl commented Sep 16, 2022

Hi @gamab it seems the enterprise-test is broken with this PR. Would it be ok if you can check it.

I've seen in my PR that this commit breaks it https://drone.grafana.net/grafana/grafana-enterprise/33692/2/7 but im not entirely sure.

Jguer added a commit that referenced this pull request Sep 16, 2022
@Jguer
Auth: Allow admins to manually change oauth user role if `oauth_skip_…
4ed7af9 
…org_role_update_sync` is enabled (#55182)

* Auth: Allow admins to change oauth user info it it's not synced.

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>

* Update public/app/features/admin/UserAdminPage.tsx

Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>

* Add missing import

* Simplify init

Co-authored-by: Josh Hunt <joshhunt@users.noreply.github.com>
Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>

* SAML: Add option to skip org role sync (#55230)

* SAML: Add option to skip org role sync

* Modify frontend accordingly

* Remove update from config option name

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Remove update from config option name

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Fix typo

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>
Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>
Co-authored-by: gamab <gabi.mabs@gmail.com>
Co-authored-by: Josh Hunt <joshhunt@users.noreply.github.com>
(cherry picked from commit 3e2e9f9)
@gamab
Copy link
Contributor Author

gamab commented Sep 16, 2022

Hey @lpskdl, I don't think this is related. This PR has nothing to do with reports and the impacted file seem to be public/app/extensions/reports/CSVExportPage.tsx.

@Clarity-89
Copy link
Contributor

@lpskdl that issue is indeed in CSVExportPage.tsx as @gamab suggests, and you'd update your enterprise branch to the latest main to fix that build failure.

@lpskdl
Copy link
Contributor

lpskdl commented Sep 16, 2022

Hi @gamab @Clarity-89 , Apologies for the false alarm. It's weird because my changes are in the main grafana repository and not in the enterprise repo 🤔.

@Clarity-89
Copy link
Contributor

Hi @gamab @Clarity-89 , Apologies for the false alarm. It's weird because my changes are in the main grafana repository and not in the enterprise repo 🤔.

No worries! In that case I think your branch was run against an older version of the Enterprise main (before the Enterprise main was fixed). Try updating your branch to the latest main and see if that helps.

Jguer added a commit that referenced this pull request Sep 19, 2022
@Jguer
Backport: Revert changes to oauth behavior to v9.1.x (#55302)
5f51a14 
* OAuth: Allow assigning Server Admin (#54780)

* extract errors to errors file

* implement oauth server admin assignment

* add server admin tests

* deduplicate autoAssignOrgRole

* deduplicate strict setting

* deduplicate strict setting

* add support for generic oauth

* add role attribute strict support for generic oauth

* add support for github/gitlab

* assignGrafanaAdmin option is here to stay

* unify similar errors

* add config option

* add okta server admin mapping

* remove never used Company attribute

* unify generic oauth role extract with other methods

* case insensitive role match as in azure

* add ini settings

* add server admin to devenv

* remove duplicate fields

* add documentation to oauth

* fix titlecase test

* implement doc feedback

(cherry picked from commit ef24587)

* Auth: Restore legacy behavior and add deprecation notice for empty org role in oauth (#55118)

* Auth: Add deprecation notice for empty org role

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>

* fix recasts

* fix azure tests missing logger

* Adding test to gitlab oauth

* Covering more cases

* Cover more options

* Add role attributestrict check fail

* Adding one more edge case test

* Using legacy for gitlab

* Yet another edge case YAEC

* Reverting github oauth to legacy

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Not using token

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Nit.

* Adding warning in docs

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* add warning to generic oauth

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Be more precise

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Adding warning to github oauth

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Adding warning to gitlab oauth

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Adding warning to okta oauth

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Add docs about mapping to AzureAD

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Clarify oauth_skip_org_role_update_sync

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Nit.

* Nit on Azure AD

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Reorder docs index

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Fix typo

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>
Co-authored-by: gamab <gabi.mabs@gmail.com>
(cherry picked from commit 00e7324)

* Auth: Allow admins to manually change oauth user role if `oauth_skip_org_role_update_sync` is enabled  (#55182)

* Auth: Allow admins to change oauth user info it it's not synced.

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>

* Update public/app/features/admin/UserAdminPage.tsx

Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>

* Add missing import

* Simplify init

Co-authored-by: Josh Hunt <joshhunt@users.noreply.github.com>
Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>

* SAML: Add option to skip org role sync (#55230)

* SAML: Add option to skip org role sync

* Modify frontend accordingly

* Remove update from config option name

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Remove update from config option name

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Fix typo

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>
Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>
Co-authored-by: gamab <gabi.mabs@gmail.com>
Co-authored-by: Josh Hunt <joshhunt@users.noreply.github.com>
(cherry picked from commit 3e2e9f9)

* Update gitlab_oauth_test.go

* Update gitlab_oauth_test.go
@dsotirakis dsotirakis modified the milestones: 9.2.0, 9.2.0-beta1 Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jguer Jguer Jguer approved these changes

@Clarity-89 Clarity-89 Clarity-89 approved these changes

@joshhunt joshhunt Awaiting requested review from joshhunt joshhunt was automatically assigned from grafana/grafana-frontend-platform

Assignees

@gamab gamab

@Jguer Jguer

Labels
add to changelog area/backend area/frontend enterprise-failed no-backport Skip backport of PR type/docs
Projects
None yet
Milestone
9.2.0-beta1
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@gamab @grafanabot @lpskdl @Clarity-89 @Jguer @dsotirakis