Auth: Enforce role sync except if skip org role sync is enabled #70766

Jguer · 2023-06-27T16:00:27Z

What is this feature?

If the information provided by the identity provider is empty, make the idP set the default org role.

Use skip_org_role_sync to leave it empty
Use role_attribute_strict to ensure it gets set by the idP

Release notice breaking change

This change impacts GitHub OAuth, Gitlab OAuth, Okta OAuth and Generic OAuth

Currently if no organization role mapping is found for a user when connecting via OAuth, Grafana doesn’t update the user’s organization role.

With Grafana 10.1, on every login, if the role_attribute_path property does not return a role, then the user is assigned the role specified by the auto_assign_org_role option.

To avoid overriding manually set roles, enable the skip_org_role_sync option in the Grafana configuration for your OAuth provider.

pkg/login/social/errors.go

IevaVasiljeva

Mostly looks good, but I added some questions about:

the behaviour when roleAttributeStrict is set for generic OAuth;
the role that we should default to if default org role is not set in the config;
the use of errutil.

I think we should also update the description of auto_assign_org_role in the config and docs, as it's currently not very accurate ("# Default role new users will be automatically assigned (if auto_assign_org above is set to true)"). I'm happy to handle docs as part of the docs work, but I think we should update the description in config files now.

IevaVasiljeva · 2023-06-28T15:23:47Z

pkg/login/social/errors.go

+	errRoleAttributePathNotSet = errutil.NewBase(errutil.StatusBadRequest,
+		"oauth.role_attribute_path_not_set",
+		errutil.WithPublicMessage("Instance role_attribute_path misconfigured, please contact your administrator"))


I'm not sure the errors will be parsed correctly, as we seem to use errutil error where this error is returned: https://github.com/grafana/grafana/blob/main/pkg/services/authn/clients/oauth.go#L125

Maybe it's fine, but worth testing (I can give it a go if you want).

IevaVasiljeva · 2023-06-28T15:35:30Z

pkg/login/social/social.go

+		if role != "" {
+			return "", false, errInvalidRole.Errorf("invalid role: %s", role)


Note: looks like we already check that the role is valid when we read the settings: https://github.com/grafana/grafana/blob/main/pkg/setting/setting.go#L1641 But I guess it doesn't hurt to do it twice.

pkg/login/social/social.go

IevaVasiljeva · 2023-06-28T15:46:47Z

pkg/login/social/generic_oauth.go

-					}
-				}
+		if (userInfo.Role == "" || userInfo.Role == s.defaultRole()) && !s.skipOrgRoleSync {
+			role, grafanaAdmin, err := s.extractRoleAndAdmin(data.rawJSON, []string{}, true)


This will fail if we fail to get the role from the token and roleAttributeStrict, right? I think we should allow for the role to be extracted from the API.

…nforce-sync

linoman

LGTM

…nforce-sync

IevaVasiljeva · 2023-07-12T10:15:46Z

pkg/login/social/social.go

+	} else if role != "" {
+		return "", false, errInvalidRole.Errorf("invalid role: %s", role)


question: should we fail if an invalid role is found? Or just if the role is invalid + roleAttributeStrict is set?

we'll only log and fallthrough to the next method.

After the 2 methods are evaluated we'll set to default role if the role is still "".

IevaVasiljeva

Sorry to block this again, but I think that the logic still isn't correct for generic OAuth.

IevaVasiljeva · 2023-07-12T10:24:56Z

pkg/login/social/social.go

+
+func (s *SocialBase) extractRoleAndAdmin(rawJSON []byte, groups []string) (org.RoleType, bool, error) {
+	role, gAdmin, err := s.extractRoleAndAdminOptional(rawJSON, groups)
+	if role == "" {


question: should we still set the role to default even if an error is returned?

fixed so it's only handled after the 2 methods

IevaVasiljeva · 2023-07-12T10:25:26Z

pkg/login/social/generic_oauth.go

+			role, grafanaAdmin, err := s.extractRoleAndAdminOptional(data.rawJSON, []string{})
+			if err != nil {


I still don't think we can return an error here - this would fail if roleAttributeStrict is set to true, but the role is being sent through the user info endpoint (it would fail, as the role won't be successfully parsed from the token).

fixed so role attribute strict is only evaluated after the 2 methods

IevaVasiljeva

Looks good to me now! Fingers crossed that we haven't missed anything 🤞

gamab

LGTM, great job! 🙂
I like how tidy the extractRoleAndAdmin code is.

Food for thought: Would it be possible to use extractRoleAndAdmin for AzureAD as well with a default role_attribute_path correctly set? To have all providers falling back on the same function.

gamab · 2023-07-13T07:40:32Z

pkg/setting/setting.go

@@ -1645,7 +1646,11 @@ func readUserSettings(iniFile *ini.File, cfg *Cfg) error {
 	AllowUserOrgCreate = users.Key("allow_org_create").MustBool(true)
 	cfg.AutoAssignOrg = users.Key("auto_assign_org").MustBool(true)
 	cfg.AutoAssignOrgId = users.Key("auto_assign_org_id").MustInt(1)
-	cfg.AutoAssignOrgRole = users.Key("auto_assign_org_role").In("Editor", []string{"Editor", "Admin", "Viewer"})
+	cfg.AutoAssignOrgRole = users.Key("auto_assign_org_role").In(
+		string(roletype.RoleViewer), []string{


Seems more logic to be in line with defaults.ini 😄

pkg/login/social/generic_oauth.go

pkg/login/social/gitlab_oauth_test.go

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

…nforce-sync

* enforce role sync except if skip org role sync is enabled * move errors to errors file and set codes * fix docs and defaults * remove legacy parameter * support fall through token-api in generic oauth * fix error handling for generic_oauth * Update pkg/login/social/generic_oauth.go Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com> * Update pkg/login/social/gitlab_oauth_test.go Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com> * Update pkg/login/social/gitlab_oauth_test.go Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com> --------- Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>

enforce role sync except if skip org role sync is enabled

0971749

Jguer added add to changelog no-backport Skip backport of PR add to what's new labels Jun 27, 2023

Jguer added this to the 10.1.x milestone Jun 27, 2023

Jguer self-assigned this Jun 27, 2023

Jguer requested a review from a team as a code owner June 27, 2023 16:00

Jguer requested review from linoman and IevaVasiljeva and removed request for a team June 27, 2023 16:00

grafanabot added the area/backend label Jun 27, 2023

move errors to errors file and set codes

9f275aa

Jguer requested a review from mgyongyosi June 28, 2023 09:06

mgyongyosi reviewed Jun 28, 2023

View reviewed changes

pkg/login/social/errors.go Outdated Show resolved Hide resolved

mgyongyosi reviewed Jun 28, 2023

View reviewed changes

pkg/login/social/errors.go Outdated Show resolved Hide resolved

IevaVasiljeva reviewed Jun 28, 2023

View reviewed changes

Jguer added 4 commits July 6, 2023 14:17

fix docs and defaults

db0cde7

Merge remote-tracking branch 'origin/main' into jguer/remove-legacy-e…

3c35aa9

…nforce-sync

remove legacy parameter

d0293eb

support fall through token-api in generic oauth

b9f8f84

Jguer requested review from torkelo and a team as code owners July 6, 2023 14:02

Jguer requested review from papagian, zserge, suntala, IevaVasiljeva and mgyongyosi and removed request for a team July 6, 2023 14:02

linoman approved these changes Jul 7, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into jguer/remove-legacy-e…

6abb770

…nforce-sync

IevaVasiljeva reviewed Jul 12, 2023

View reviewed changes

Jguer added the breaking change Relevant for changelog generation label Jul 12, 2023

fix error handling for generic_oauth

effc40b

Jguer requested a review from IevaVasiljeva July 12, 2023 15:58

Jguer added breaking change Relevant for changelog generation and removed breaking change Relevant for changelog generation labels Jul 12, 2023

IevaVasiljeva approved these changes Jul 12, 2023

View reviewed changes

gamab approved these changes Jul 13, 2023

View reviewed changes

Jguer and others added 5 commits July 13, 2023 13:26

Update pkg/login/social/generic_oauth.go

0a2c32c

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

Update pkg/login/social/gitlab_oauth_test.go

cbf0f91

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

Update pkg/login/social/gitlab_oauth_test.go

331aff3

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

Merge remote-tracking branch 'origin/main' into jguer/remove-legacy-e…

adc9cfb

…nforce-sync

Merge remote-tracking branch 'origin/main' into jguer/remove-legacy-e…

c134495

…nforce-sync

Jguer merged commit 0ffd359 into main Jul 17, 2023
12 checks passed

Jguer deleted the jguer/remove-legacy-enforce-sync branch July 17, 2023 13:58

Jguer mentioned this pull request Jul 20, 2023

docs: What’s New & Upgrade Guide 10.1 AuthNZ #72000

Merged

ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023

chenrui333 mentioned this pull request Aug 24, 2023

grafana 10.1.0 Homebrew/homebrew-core#140321

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auth: Enforce role sync except if skip org role sync is enabled #70766

Auth: Enforce role sync except if skip org role sync is enabled #70766

Jguer commented Jun 27, 2023 •

edited

Loading

IevaVasiljeva left a comment

IevaVasiljeva Jun 28, 2023

IevaVasiljeva Jun 28, 2023

IevaVasiljeva Jun 28, 2023

linoman left a comment

IevaVasiljeva Jul 12, 2023

Jguer Jul 12, 2023

IevaVasiljeva left a comment

IevaVasiljeva Jul 12, 2023

Jguer Jul 12, 2023

IevaVasiljeva Jul 12, 2023

Jguer Jul 12, 2023

IevaVasiljeva left a comment

gamab left a comment

gamab Jul 13, 2023

		if role != "" {
		return "", false, errInvalidRole.Errorf("invalid role: %s", role)

		} else if role != "" {
		return "", false, errInvalidRole.Errorf("invalid role: %s", role)

		role, grafanaAdmin, err := s.extractRoleAndAdminOptional(data.rawJSON, []string{})
		if err != nil {

Auth: Enforce role sync except if skip org role sync is enabled #70766

Auth: Enforce role sync except if skip org role sync is enabled #70766

Conversation

Jguer commented Jun 27, 2023 • edited Loading

Release notice breaking change

IevaVasiljeva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linoman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IevaVasiljeva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IevaVasiljeva left a comment

Choose a reason for hiding this comment

gamab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jguer commented Jun 27, 2023 •

edited

Loading