From 0ffd3598018e728a3b5fc249dd7212ce11d166da Mon Sep 17 00:00:00 2001 From: Jo Date: Mon, 17 Jul 2023 15:58:16 +0200 Subject: [PATCH] Auth: Enforce role sync except if skip org role sync is enabled (#70766) * 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 * Update pkg/login/social/gitlab_oauth_test.go Co-authored-by: Gabriel MABILLE * Update pkg/login/social/gitlab_oauth_test.go Co-authored-by: Gabriel MABILLE --------- Co-authored-by: Gabriel MABILLE --- conf/defaults.ini | 2 +- conf/sample.ini | 2 +- pkg/login/social/azuread_oauth.go | 29 +- pkg/login/social/azuread_oauth_test.go | 2 +- pkg/login/social/errors.go | 33 +- pkg/login/social/generic_oauth.go | 32 +- pkg/login/social/generic_oauth_test.go | 484 +++++++++++++------------ pkg/login/social/github_oauth.go | 7 +- pkg/login/social/github_oauth_test.go | 8 +- pkg/login/social/gitlab_oauth.go | 12 +- pkg/login/social/gitlab_oauth_test.go | 24 +- pkg/login/social/okta_oauth.go | 7 +- pkg/login/social/social.go | 50 ++- pkg/setting/setting.go | 7 +- 14 files changed, 365 insertions(+), 334 deletions(-) diff --git a/conf/defaults.ini b/conf/defaults.ini index ae8a7b0293d..d3782c4d17d 100644 --- a/conf/defaults.ini +++ b/conf/defaults.ini @@ -454,7 +454,7 @@ auto_assign_org = true # Set this value to automatically add new users to the provided organization (if auto_assign_org above is set to true) auto_assign_org_id = 1 -# Default role new users will be automatically assigned (if auto_assign_org above is set to true) +# Default role new users will be automatically assigned auto_assign_org_role = Viewer # Require email validation before sign up completes diff --git a/conf/sample.ini b/conf/sample.ini index 05c361a5f4d..2f8a754b29a 100644 --- a/conf/sample.ini +++ b/conf/sample.ini @@ -439,7 +439,7 @@ # Set this value to automatically add new users to the provided organization (if auto_assign_org above is set to true) ;auto_assign_org_id = 1 -# Default role new users will be automatically assigned (if auto_assign_org above is set to true) +# Default role new users will be automatically assigned ;auto_assign_org_role = Viewer # Require email validation before sign up completes diff --git a/pkg/login/social/azuread_oauth.go b/pkg/login/social/azuread_oauth.go index 7d41760d868..565541e75c3 100644 --- a/pkg/login/social/azuread_oauth.go +++ b/pkg/login/social/azuread_oauth.go @@ -83,10 +83,14 @@ func (s *SocialAzureAD) UserInfo(ctx context.Context, client *http.Client, token var role roletype.RoleType var grafanaAdmin bool if !s.skipOrgRoleSync { - role, grafanaAdmin = s.extractRoleAndAdmin(claims) - } - if s.roleAttributeStrict && !role.IsValid() { - return nil, &InvalidBasicRoleError{idP: "Azure", assignedRole: string(role)} + role, grafanaAdmin, err = s.extractRoleAndAdmin(claims) + if err != nil { + return nil, err + } + + if !role.IsValid() { + return nil, errInvalidRole.Errorf("AzureAD OAuth: invalid role %q", role) + } } s.log.Debug("AzureAD OAuth: extracted role", "email", email, "role", role) @@ -201,9 +205,12 @@ func (claims *azureClaims) extractEmail() string { } // extractRoleAndAdmin extracts the role from the claims and returns the role and whether the user is a Grafana admin. -func (s *SocialAzureAD) extractRoleAndAdmin(claims *azureClaims) (org.RoleType, bool) { +func (s *SocialAzureAD) extractRoleAndAdmin(claims *azureClaims) (org.RoleType, bool, error) { if len(claims.Roles) == 0 { - return s.defaultRole(false), false + if s.roleAttributeStrict { + return "", false, errRoleAttributeStrictViolation.Errorf("AzureAD OAuth: unset role") + } + return s.defaultRole(), false, nil } roleOrder := []org.RoleType{RoleGrafanaAdmin, org.RoleAdmin, org.RoleEditor, @@ -211,14 +218,18 @@ func (s *SocialAzureAD) extractRoleAndAdmin(claims *azureClaims) (org.RoleType, for _, role := range roleOrder { if found := hasRole(claims.Roles, role); found { if role == RoleGrafanaAdmin { - return org.RoleAdmin, true + return org.RoleAdmin, true, nil } - return role, false + return role, false, nil } } - return s.defaultRole(false), false + if s.roleAttributeStrict { + return "", false, errRoleAttributeStrictViolation.Errorf("AzureAD OAuth: idP did not return a valid role %q", claims.Roles) + } + + return s.defaultRole(), false, nil } func hasRole(roles []string, role org.RoleType) bool { diff --git a/pkg/login/social/azuread_oauth_test.go b/pkg/login/social/azuread_oauth_test.go index bfce0d03308..67e517bf187 100644 --- a/pkg/login/social/azuread_oauth_test.go +++ b/pkg/login/social/azuread_oauth_test.go @@ -379,7 +379,7 @@ func TestSocialAzureAD_UserInfo(t *testing.T) { Name: "My Name", Email: "me@example.com", Login: "me@example.com", - Role: "", + Role: "Viewer", Groups: []string{"foo", "bar"}, }, wantErr: false, diff --git a/pkg/login/social/errors.go b/pkg/login/social/errors.go index c0a71bb01e7..4459eab0186 100644 --- a/pkg/login/social/errors.go +++ b/pkg/login/social/errors.go @@ -2,29 +2,22 @@ package social import ( "errors" - "fmt" + + "github.com/grafana/grafana/pkg/util/errutil" ) var ( ErrIDTokenNotFound = errors.New("id_token not found") ErrEmailNotFound = errors.New("error getting user info: no email found in access token") + + errRoleAttributePathNotSet = errutil.NewBase(errutil.StatusBadRequest, + "oauth.role_attribute_path_not_set", + errutil.WithPublicMessage("Instance role_attribute_path misconfigured, please contact your administrator")) + + errRoleAttributeStrictViolation = errutil.NewBase(errutil.StatusBadRequest, + "oauth.role_attribute_strict_violation", + errutil.WithPublicMessage("IdP did not return a role attribute, please contact your administrator")) + + errInvalidRole = errutil.NewBase(errutil.StatusBadRequest, "oauth.invalid_role", + errutil.WithPublicMessage("IdP did not return a valid role attribute, please contact your administrator")) ) - -type InvalidBasicRoleError struct { - idP string - assignedRole string -} - -func (e *InvalidBasicRoleError) Error() string { - withFallback := func(v, fallback string) string { - if v == "" { - return fallback - } - return v - } - return fmt.Sprintf("Integration requires a valid org role assigned in %s. Assigned role: %s", withFallback(e.idP, "idP"), withFallback(e.assignedRole, "\" \"")) -} - -func (e *InvalidBasicRoleError) Unwrap() error { - return &Error{e.Error()} -} diff --git a/pkg/login/social/generic_oauth.go b/pkg/login/social/generic_oauth.go index 220787cacce..2e2491ca78e 100644 --- a/pkg/login/social/generic_oauth.go +++ b/pkg/login/social/generic_oauth.go @@ -140,21 +140,16 @@ func (s *SocialGenericOAuth) UserInfo(ctx context.Context, client *http.Client, } } - if userInfo.Role == "" { - if !s.skipOrgRoleSync { - role, grafanaAdmin := s.extractRoleAndAdmin(data.rawJSON, []string{}, true) - if role != "" { - s.log.Debug("Setting user info role from extracted role") - - userInfo.Role = role - if s.allowAssignGrafanaAdmin { - userInfo.IsGrafanaAdmin = &grafanaAdmin - } + if userInfo.Role == "" && !s.skipOrgRoleSync { + role, grafanaAdmin, err := s.extractRoleAndAdminOptional(data.rawJSON, []string{}) + if err != nil { + s.log.Warn("Failed to extract role", "err", err) + } else { + userInfo.Role = role + if s.allowAssignGrafanaAdmin { + userInfo.IsGrafanaAdmin = &grafanaAdmin } } - if s.allowAssignGrafanaAdmin && s.skipOrgRoleSync { - s.log.Warn("allowAssignGrafanaAdmin and skipOrgRoleSync are both set, Grafana Admin role will not be synced, consider setting one or the other") - } } if len(userInfo.Groups) == 0 { @@ -168,8 +163,15 @@ func (s *SocialGenericOAuth) UserInfo(ctx context.Context, client *http.Client, } } - if s.roleAttributeStrict && !userInfo.Role.IsValid() { - return nil, &InvalidBasicRoleError{assignedRole: string(userInfo.Role)} + if userInfo.Role == "" && !s.skipOrgRoleSync { + if s.roleAttributeStrict { + return nil, errRoleAttributeStrictViolation.Errorf("idP did not return a role attribute") + } + userInfo.Role = s.defaultRole() + } + + if s.allowAssignGrafanaAdmin && s.skipOrgRoleSync { + s.log.Warn("allowAssignGrafanaAdmin and skipOrgRoleSync are both set, Grafana Admin role will not be synced, consider setting one or the other") } if userInfo.Email == "" { diff --git a/pkg/login/social/generic_oauth_test.go b/pkg/login/social/generic_oauth_test.go index d954b696c6b..9fb60028fef 100644 --- a/pkg/login/social/generic_oauth_test.go +++ b/pkg/login/social/generic_oauth_test.go @@ -238,268 +238,272 @@ func TestSearchJSONForRole(t *testing.T) { } func TestUserInfoSearchesForEmailAndRole(t *testing.T) { - t.Run("Given a generic OAuth provider", func(t *testing.T) { - provider := SocialGenericOAuth{ - SocialBase: &SocialBase{ - log: newLogger("generic_oauth_test", "debug"), - }, - emailAttributePath: "email", - } + provider := SocialGenericOAuth{ + SocialBase: &SocialBase{ + log: newLogger("generic_oauth_test", "debug"), + }, + emailAttributePath: "email", + } - tests := []struct { - Name string - SkipOrgRoleSync bool - AllowAssignGrafanaAdmin bool - ResponseBody interface{} - OAuth2Extra interface{} - RoleAttributePath string - ExpectedEmail string - ExpectedRole org.RoleType - ExpectedGrafanaAdmin *bool - }{ - { - Name: "Given a valid id_token, a valid role path, no API response, use id_token", - OAuth2Extra: map[string]interface{}{ - // { "role": "Admin", "email": "john.doe@example.com" } - "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoiQWRtaW4iLCJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIn0.9PtHcCaXxZa2HDlASyKIaFGfOKlw2ILQo32xlvhvhRg", - }, - RoleAttributePath: "role", - ExpectedEmail: "john.doe@example.com", - ExpectedRole: "Admin", + tests := []struct { + Name string + SkipOrgRoleSync bool + AllowAssignGrafanaAdmin bool + ResponseBody interface{} + OAuth2Extra interface{} + RoleAttributePath string + ExpectedEmail string + ExpectedRole org.RoleType + ExpectedError error + ExpectedGrafanaAdmin *bool + }{ + { + Name: "Given a valid id_token, a valid role path, no API response, use id_token", + OAuth2Extra: map[string]interface{}{ + // { "role": "Admin", "email": "john.doe@example.com" } + "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoiQWRtaW4iLCJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIn0.9PtHcCaXxZa2HDlASyKIaFGfOKlw2ILQo32xlvhvhRg", }, - { - Name: "Given a valid id_token, no role path, no API response, use id_token", - OAuth2Extra: map[string]interface{}{ - // { "email": "john.doe@example.com" } - "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIn0.k5GwPcZvGe2BE_jgwN0ntz0nz4KlYhEd0hRRLApkTJ4", - }, - RoleAttributePath: "", - ExpectedEmail: "john.doe@example.com", - ExpectedRole: "", + RoleAttributePath: "role", + ExpectedEmail: "john.doe@example.com", + ExpectedRole: "Admin", + }, + { + Name: "Given a valid id_token, no role path, no API response, use id_token", + OAuth2Extra: map[string]interface{}{ + // { "email": "john.doe@example.com" } + "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIn0.k5GwPcZvGe2BE_jgwN0ntz0nz4KlYhEd0hRRLApkTJ4", }, - { - Name: "Given a valid id_token, an invalid role path, no API response, use id_token", - OAuth2Extra: map[string]interface{}{ - // { "role": "Admin", "email": "john.doe@example.com" } - "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoiQWRtaW4iLCJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIn0.9PtHcCaXxZa2HDlASyKIaFGfOKlw2ILQo32xlvhvhRg", - }, - RoleAttributePath: "invalid_path", - ExpectedEmail: "john.doe@example.com", - ExpectedRole: "", + RoleAttributePath: "", + ExpectedEmail: "john.doe@example.com", + ExpectedRole: "Viewer", + }, + { + Name: "Given a valid id_token, an invalid role path, no API response, use id_token", + OAuth2Extra: map[string]interface{}{ + // { "role": "Admin", "email": "john.doe@example.com" } + "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoiQWRtaW4iLCJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIn0.9PtHcCaXxZa2HDlASyKIaFGfOKlw2ILQo32xlvhvhRg", }, - { - Name: "Given no id_token, a valid role path, a valid API response, use API response", - ResponseBody: map[string]interface{}{ - "role": "Admin", - "email": "john.doe@example.com", - }, - RoleAttributePath: "role", - ExpectedEmail: "john.doe@example.com", - ExpectedRole: "Admin", + RoleAttributePath: "invalid_path", + ExpectedEmail: "john.doe@example.com", + ExpectedRole: "Viewer", + }, + { + Name: "Given no id_token, a valid role path, a valid API response, use API response", + ResponseBody: map[string]interface{}{ + "role": "Admin", + "email": "john.doe@example.com", }, - { - Name: "Given no id_token, no role path, a valid API response, use API response", - ResponseBody: map[string]interface{}{ - "email": "john.doe@example.com", - }, - RoleAttributePath: "", - ExpectedEmail: "john.doe@example.com", - ExpectedRole: "", + RoleAttributePath: "role", + ExpectedEmail: "john.doe@example.com", + ExpectedRole: "Admin", + }, + { + Name: "Given no id_token, no role path, a valid API response, use API response", + ResponseBody: map[string]interface{}{ + "email": "john.doe@example.com", }, - { - Name: "Given no id_token, a role path, a valid API response without a role, use API response", - ResponseBody: map[string]interface{}{ - "email": "john.doe@example.com", - }, - RoleAttributePath: "role", - ExpectedEmail: "john.doe@example.com", - ExpectedRole: "", + RoleAttributePath: "", + ExpectedEmail: "john.doe@example.com", + ExpectedRole: "Viewer", + }, + { + Name: "Given no id_token, a role path, a valid API response without a role, use API response", + ResponseBody: map[string]interface{}{ + "email": "john.doe@example.com", }, - { - Name: "Given no id_token, a valid role path, no API response, no data", - RoleAttributePath: "role", - ExpectedEmail: "", - ExpectedRole: "", + RoleAttributePath: "role", + ExpectedEmail: "john.doe@example.com", + ExpectedRole: "Viewer", + }, + { + Name: "Given no id_token, a valid role path, no API response, no data", + RoleAttributePath: "role", + ExpectedEmail: "", + ExpectedRole: "Viewer", + }, + { + Name: "Given a valid id_token, a valid role path, a valid API response, prefer id_token", + OAuth2Extra: map[string]interface{}{ + // { "role": "Admin", "email": "john.doe@example.com" } + "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoiQWRtaW4iLCJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIn0.9PtHcCaXxZa2HDlASyKIaFGfOKlw2ILQo32xlvhvhRg", }, - { - Name: "Given a valid id_token, a valid role path, a valid API response, prefer id_token", - OAuth2Extra: map[string]interface{}{ - // { "role": "Admin", "email": "john.doe@example.com" } - "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoiQWRtaW4iLCJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIn0.9PtHcCaXxZa2HDlASyKIaFGfOKlw2ILQo32xlvhvhRg", - }, - ResponseBody: map[string]interface{}{ - "role": "FromResponse", - "email": "from_response@example.com", - }, - RoleAttributePath: "role", - ExpectedEmail: "john.doe@example.com", - ExpectedRole: "Admin", + ResponseBody: map[string]interface{}{ + "role": "FromResponse", + "email": "from_response@example.com", }, - { - Name: "Given a valid id_token and AssignGrafanaAdmin is unchecked, don't grant Server Admin", - AllowAssignGrafanaAdmin: false, - OAuth2Extra: map[string]interface{}{ - // { "role": "GrafanaAdmin", "email": "john.doe@example.com" } - "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoiR3JhZmFuYUFkbWluIiwiZW1haWwiOiJqb2huLmRvZUBleGFtcGxlLmNvbSJ9.cQqMJpVjwdtJ8qEZLOo9RKNbAFfpkQcpnRG0nopmWEI", - }, - ResponseBody: map[string]interface{}{ - "role": "FromResponse", - "email": "from_response@example.com", - }, - RoleAttributePath: "role", - ExpectedEmail: "john.doe@example.com", - ExpectedRole: "Admin", - ExpectedGrafanaAdmin: nil, + RoleAttributePath: "role", + ExpectedEmail: "john.doe@example.com", + ExpectedRole: "Admin", + }, + { + Name: "Given a valid id_token and AssignGrafanaAdmin is unchecked, don't grant Server Admin", + AllowAssignGrafanaAdmin: false, + OAuth2Extra: map[string]interface{}{ + // { "role": "GrafanaAdmin", "email": "john.doe@example.com" } + "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoiR3JhZmFuYUFkbWluIiwiZW1haWwiOiJqb2huLmRvZUBleGFtcGxlLmNvbSJ9.cQqMJpVjwdtJ8qEZLOo9RKNbAFfpkQcpnRG0nopmWEI", }, - { - Name: "Given a valid id_token and AssignGrafanaAdmin is checked, grant Server Admin", - AllowAssignGrafanaAdmin: true, - OAuth2Extra: map[string]interface{}{ - // { "role": "GrafanaAdmin", "email": "john.doe@example.com" } - "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoiR3JhZmFuYUFkbWluIiwiZW1haWwiOiJqb2huLmRvZUBleGFtcGxlLmNvbSJ9.cQqMJpVjwdtJ8qEZLOo9RKNbAFfpkQcpnRG0nopmWEI", - }, - ResponseBody: map[string]interface{}{ - "role": "FromResponse", - "email": "from_response@example.com", - }, - RoleAttributePath: "role", - ExpectedEmail: "john.doe@example.com", - ExpectedRole: "Admin", - ExpectedGrafanaAdmin: trueBoolPtr(), + ResponseBody: map[string]interface{}{ + "role": "FromResponse", + "email": "from_response@example.com", }, - { - Name: "Given a valid id_token, an invalid role path, a valid API response, prefer id_token", - OAuth2Extra: map[string]interface{}{ - // { "role": "Admin", "email": "john.doe@example.com" } - "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoiQWRtaW4iLCJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIn0.9PtHcCaXxZa2HDlASyKIaFGfOKlw2ILQo32xlvhvhRg", - }, - ResponseBody: map[string]interface{}{ - "role": "FromResponse", - "email": "from_response@example.com", - }, - RoleAttributePath: "invalid_path", - ExpectedEmail: "john.doe@example.com", - ExpectedRole: "", + RoleAttributePath: "role", + ExpectedEmail: "john.doe@example.com", + ExpectedRole: "Admin", + ExpectedGrafanaAdmin: nil, + }, + { + Name: "Given a valid id_token and AssignGrafanaAdmin is checked, grant Server Admin", + AllowAssignGrafanaAdmin: true, + OAuth2Extra: map[string]interface{}{ + // { "role": "GrafanaAdmin", "email": "john.doe@example.com" } + "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoiR3JhZmFuYUFkbWluIiwiZW1haWwiOiJqb2huLmRvZUBleGFtcGxlLmNvbSJ9.cQqMJpVjwdtJ8qEZLOo9RKNbAFfpkQcpnRG0nopmWEI", }, - { - Name: "Given a valid id_token with no email, a valid role path, a valid API response with no role, merge", - OAuth2Extra: map[string]interface{}{ - // { "role": "Admin" } - "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoiQWRtaW4ifQ.k5GwPcZvGe2BE_jgwN0ntz0nz4KlYhEd0hRRLApkTJ4", - }, - ResponseBody: map[string]interface{}{ - "email": "from_response@example.com", - }, - RoleAttributePath: "role", - ExpectedEmail: "from_response@example.com", - ExpectedRole: "Admin", + ResponseBody: map[string]interface{}{ + "role": "FromResponse", + "email": "from_response@example.com", }, - { - Name: "Given a valid id_token with no role, a valid role path, a valid API response with no email, merge", - OAuth2Extra: map[string]interface{}{ - // { "email": "john.doe@example.com" } - "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIn0.k5GwPcZvGe2BE_jgwN0ntz0nz4KlYhEd0hRRLApkTJ4", - }, - ResponseBody: map[string]interface{}{ - "role": "FromResponse", - }, - RoleAttributePath: "role", - ExpectedEmail: "john.doe@example.com", - ExpectedRole: "Fromresponse", + RoleAttributePath: "role", + ExpectedEmail: "john.doe@example.com", + ExpectedRole: "Admin", + ExpectedGrafanaAdmin: trueBoolPtr(), + }, + { + Name: "Given a valid id_token, an invalid role path, a valid API response, prefer id_token", + OAuth2Extra: map[string]interface{}{ + // { "role": "Admin", "email": "john.doe@example.com" } + "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoiQWRtaW4iLCJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIn0.9PtHcCaXxZa2HDlASyKIaFGfOKlw2ILQo32xlvhvhRg", }, - { - Name: "Given a valid id_token, a valid advanced JMESPath role path, derive the role", - OAuth2Extra: map[string]interface{}{ - // { "email": "john.doe@example.com", - // "info": { "roles": [ "dev", "engineering" ] }} - "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIiwiaW5mbyI6eyJyb2xlcyI6WyJkZXYiLCJlbmdpbmVlcmluZyJdfX0.RmmQfv25eXb4p3wMrJsvXfGQ6EXhGtwRXo6SlCFHRNg", - }, - RoleAttributePath: "contains(info.roles[*], 'dev') && 'Editor'", - ExpectedEmail: "john.doe@example.com", - ExpectedRole: "Editor", + ResponseBody: map[string]interface{}{ + "role": "FromResponse", + "email": "from_response@example.com", }, - { - Name: "Given a valid id_token without role info, a valid advanced JMESPath role path, a valid API response, derive the correct role using the userinfo API response (JMESPath warning on id_token)", - OAuth2Extra: map[string]interface{}{ - // { "email": "john.doe@example.com" } - "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIn0.k5GwPcZvGe2BE_jgwN0ntz0nz4KlYhEd0hRRLApkTJ4", - }, - ResponseBody: map[string]interface{}{ - "info": map[string]interface{}{ - "roles": []string{"engineering", "SRE"}, - }, - }, - RoleAttributePath: "contains(info.roles[*], 'SRE') && 'Admin'", - ExpectedEmail: "john.doe@example.com", - ExpectedRole: "Admin", + RoleAttributePath: "invalid_path", + ExpectedEmail: "john.doe@example.com", + ExpectedRole: "Viewer", + }, + { + Name: "Given a valid id_token with no email, a valid role path, a valid API response with no role, merge", + OAuth2Extra: map[string]interface{}{ + // { "role": "Admin" } + "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoiQWRtaW4ifQ.k5GwPcZvGe2BE_jgwN0ntz0nz4KlYhEd0hRRLApkTJ4", }, - { - Name: "Given a valid id_token, a valid advanced JMESPath role path, a valid API response, prefer ID token", - OAuth2Extra: map[string]interface{}{ - // { "email": "john.doe@example.com", - // "info": { "roles": [ "dev", "engineering" ] }} - "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIiwiaW5mbyI6eyJyb2xlcyI6WyJkZXYiLCJlbmdpbmVlcmluZyJdfX0.RmmQfv25eXb4p3wMrJsvXfGQ6EXhGtwRXo6SlCFHRNg", - }, - ResponseBody: map[string]interface{}{ - "info": map[string]interface{}{ - "roles": []string{"engineering", "SRE"}, - }, - }, - RoleAttributePath: "contains(info.roles[*], 'SRE') && 'Admin' || contains(info.roles[*], 'dev') && 'Editor' || 'Viewer'", - ExpectedEmail: "john.doe@example.com", - ExpectedRole: "Editor", + ResponseBody: map[string]interface{}{ + "email": "from_response@example.com", }, - { - Name: "Given skip org role sync set to true, with a valid id_token, a valid advanced JMESPath role path, a valid API response, no org role should be set", - SkipOrgRoleSync: true, - OAuth2Extra: map[string]interface{}{ - // { "email": "john.doe@example.com", - // "info": { "roles": [ "dev", "engineering" ] }} - "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIiwiaW5mbyI6eyJyb2xlcyI6WyJkZXYiLCJlbmdpbmVlcmluZyJdfX0.RmmQfv25eXb4p3wMrJsvXfGQ6EXhGtwRXo6SlCFHRNg", - }, - ResponseBody: map[string]interface{}{ - "info": map[string]interface{}{ - "roles": []string{"engineering", "SRE"}, - }, - }, - RoleAttributePath: "contains(info.roles[*], 'SRE') && 'Admin' || contains(info.roles[*], 'dev') && 'Editor' || 'Viewer'", - ExpectedEmail: "john.doe@example.com", - ExpectedRole: "", + RoleAttributePath: "role", + ExpectedEmail: "from_response@example.com", + ExpectedRole: "Admin", + }, + { + Name: "Given a valid id_token with no role, a valid role path, a valid API response with no email, merge", + OAuth2Extra: map[string]interface{}{ + // { "email": "john.doe@example.com" } + "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIn0.k5GwPcZvGe2BE_jgwN0ntz0nz4KlYhEd0hRRLApkTJ4", }, - } + ResponseBody: map[string]interface{}{ + "role": "FromResponse", + }, + RoleAttributePath: "role", + ExpectedEmail: "john.doe@example.com", + ExpectedRole: "Viewer", + ExpectedError: nil, + }, + { + Name: "Given a valid id_token, a valid advanced JMESPath role path, derive the role", + OAuth2Extra: map[string]interface{}{ + // { "email": "john.doe@example.com", + // "info": { "roles": [ "dev", "engineering" ] }} + "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIiwiaW5mbyI6eyJyb2xlcyI6WyJkZXYiLCJlbmdpbmVlcmluZyJdfX0.RmmQfv25eXb4p3wMrJsvXfGQ6EXhGtwRXo6SlCFHRNg", + }, + RoleAttributePath: "contains(info.roles[*], 'dev') && 'Editor'", + ExpectedEmail: "john.doe@example.com", + ExpectedRole: "Editor", + }, + { + Name: "Given a valid id_token without role info, a valid advanced JMESPath role path, a valid API response, derive the correct role using the userinfo API response (JMESPath warning on id_token)", + OAuth2Extra: map[string]interface{}{ + // { "email": "john.doe@example.com" } + "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIn0.k5GwPcZvGe2BE_jgwN0ntz0nz4KlYhEd0hRRLApkTJ4", + }, + ResponseBody: map[string]interface{}{ + "info": map[string]interface{}{ + "roles": []string{"engineering", "SRE"}, + }, + }, + RoleAttributePath: "contains(info.roles[*], 'SRE') && 'Admin'", + ExpectedEmail: "john.doe@example.com", + ExpectedRole: "Admin", + }, + { + Name: "Given a valid id_token, a valid advanced JMESPath role path, a valid API response, prefer ID token", + OAuth2Extra: map[string]interface{}{ + // { "email": "john.doe@example.com", + // "info": { "roles": [ "dev", "engineering" ] }} + "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIiwiaW5mbyI6eyJyb2xlcyI6WyJkZXYiLCJlbmdpbmVlcmluZyJdfX0.RmmQfv25eXb4p3wMrJsvXfGQ6EXhGtwRXo6SlCFHRNg", + }, + ResponseBody: map[string]interface{}{ + "info": map[string]interface{}{ + "roles": []string{"engineering", "SRE"}, + }, + }, + RoleAttributePath: "contains(info.roles[*], 'SRE') && 'Admin' || contains(info.roles[*], 'dev') && 'Editor' || 'Viewer'", + ExpectedEmail: "john.doe@example.com", + ExpectedRole: "Editor", + }, + { + Name: "Given skip org role sync set to true, with a valid id_token, a valid advanced JMESPath role path, a valid API response, no org role should be set", + SkipOrgRoleSync: true, + OAuth2Extra: map[string]interface{}{ + // { "email": "john.doe@example.com", + // "info": { "roles": [ "dev", "engineering" ] }} + "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIiwiaW5mbyI6eyJyb2xlcyI6WyJkZXYiLCJlbmdpbmVlcmluZyJdfX0.RmmQfv25eXb4p3wMrJsvXfGQ6EXhGtwRXo6SlCFHRNg", + }, + ResponseBody: map[string]interface{}{ + "info": map[string]interface{}{ + "roles": []string{"engineering", "SRE"}, + }, + }, + RoleAttributePath: "contains(info.roles[*], 'SRE') && 'Admin' || contains(info.roles[*], 'dev') && 'Editor' || 'Viewer'", + ExpectedEmail: "john.doe@example.com", + ExpectedRole: "", + }, + } - for _, test := range tests { - provider.roleAttributePath = test.RoleAttributePath - provider.allowAssignGrafanaAdmin = test.AllowAssignGrafanaAdmin - provider.skipOrgRoleSync = test.SkipOrgRoleSync + for _, test := range tests { + provider.roleAttributePath = test.RoleAttributePath + provider.allowAssignGrafanaAdmin = test.AllowAssignGrafanaAdmin + provider.skipOrgRoleSync = test.SkipOrgRoleSync - t.Run(test.Name, func(t *testing.T) { - body, err := json.Marshal(test.ResponseBody) + t.Run(test.Name, func(t *testing.T) { + body, err := json.Marshal(test.ResponseBody) + require.NoError(t, err) + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Header().Set("Content-Type", "application/json") + _, err = w.Write(body) require.NoError(t, err) - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - w.Header().Set("Content-Type", "application/json") - _, err = w.Write(body) - require.NoError(t, err) - })) - provider.apiUrl = ts.URL - staticToken := oauth2.Token{ - AccessToken: "", - TokenType: "", - RefreshToken: "", - Expiry: time.Now(), - } + })) + provider.apiUrl = ts.URL + staticToken := oauth2.Token{ + AccessToken: "", + TokenType: "", + RefreshToken: "", + Expiry: time.Now(), + } - token := staticToken.WithExtra(test.OAuth2Extra) - actualResult, err := provider.UserInfo(context.Background(), ts.Client(), token) - require.NoError(t, err) - require.Equal(t, test.ExpectedEmail, actualResult.Email) - require.Equal(t, test.ExpectedEmail, actualResult.Login) - require.Equal(t, test.ExpectedRole, actualResult.Role) - require.Equal(t, test.ExpectedGrafanaAdmin, actualResult.IsGrafanaAdmin) - }) - } - }) + token := staticToken.WithExtra(test.OAuth2Extra) + actualResult, err := provider.UserInfo(context.Background(), ts.Client(), token) + if test.ExpectedError != nil { + require.ErrorIs(t, err, test.ExpectedError) + return + } + require.NoError(t, err) + require.Equal(t, test.ExpectedEmail, actualResult.Email) + require.Equal(t, test.ExpectedEmail, actualResult.Login) + require.Equal(t, test.ExpectedRole, actualResult.Role) + require.Equal(t, test.ExpectedGrafanaAdmin, actualResult.IsGrafanaAdmin) + }) + } } func TestUserInfoSearchesForLogin(t *testing.T) { diff --git a/pkg/login/social/github_oauth.go b/pkg/login/social/github_oauth.go index 18584a3b8a4..85a1c996e08 100644 --- a/pkg/login/social/github_oauth.go +++ b/pkg/login/social/github_oauth.go @@ -212,10 +212,9 @@ func (s *SocialGithub) UserInfo(ctx context.Context, client *http.Client, token if !s.skipOrgRoleSync { var grafanaAdmin bool - role, grafanaAdmin = s.extractRoleAndAdmin(response.Body, teams, true) - - if s.roleAttributeStrict && !role.IsValid() { - return nil, &InvalidBasicRoleError{idP: "Github", assignedRole: string(role)} + role, grafanaAdmin, err = s.extractRoleAndAdmin(response.Body, teams) + if err != nil { + return nil, err } if s.allowAssignGrafanaAdmin { diff --git a/pkg/login/social/github_oauth_test.go b/pkg/login/social/github_oauth_test.go index 38cc4b0ed03..df8771dfd4f 100644 --- a/pkg/login/social/github_oauth_test.go +++ b/pkg/login/social/github_oauth_test.go @@ -137,7 +137,7 @@ func TestSocialGitHub_UserInfo(t *testing.T) { Name: "monalisa octocat", Email: "octocat@github.com", Login: "octocat", - Role: "", + Role: "Viewer", Groups: []string{"https://github.com/orgs/github/teams/justice-league", "@github/justice-league"}, }, }, @@ -203,8 +203,8 @@ func TestSocialGitHub_UserInfo(t *testing.T) { IsGrafanaAdmin: boolPointer, }, }, - { // Case that's going to change with Grafana 10 - name: "No fallback to default org role (will change in Grafana 10)", + { + name: "fallback to default org role", roleAttributePath: "", userRawJSON: testGHUserJSON, autoAssignOrgRole: "Editor", @@ -214,7 +214,7 @@ func TestSocialGitHub_UserInfo(t *testing.T) { Name: "monalisa octocat", Email: "octocat@github.com", Login: "octocat", - Role: "", + Role: "Editor", Groups: []string{"https://github.com/orgs/github/teams/justice-league", "@github/justice-league"}, }, }, diff --git a/pkg/login/social/gitlab_oauth.go b/pkg/login/social/gitlab_oauth.go index 5cb9caceda9..b3e658799f7 100644 --- a/pkg/login/social/gitlab_oauth.go +++ b/pkg/login/social/gitlab_oauth.go @@ -175,9 +175,9 @@ func (s *SocialGitlab) extractFromAPI(ctx context.Context, client *http.Client, if !s.skipOrgRoleSync { var grafanaAdmin bool - role, grafanaAdmin := s.extractRoleAndAdmin(response.Body, idData.Groups, true) - if s.roleAttributeStrict && !role.IsValid() { - return nil, &InvalidBasicRoleError{idP: "Gitlab", assignedRole: string(role)} + role, grafanaAdmin, err := s.extractRoleAndAdmin(response.Body, idData.Groups) + if err != nil { + return nil, err } if s.allowAssignGrafanaAdmin { @@ -227,9 +227,9 @@ func (s *SocialGitlab) extractFromToken(ctx context.Context, client *http.Client } if !s.skipOrgRoleSync { - role, grafanaAdmin := s.extractRoleAndAdmin(rawJSON, data.Groups, true) - if s.roleAttributeStrict && !role.IsValid() { - return nil, &InvalidBasicRoleError{idP: "Gitlab", assignedRole: string(role)} + role, grafanaAdmin, errRole := s.extractRoleAndAdmin(rawJSON, data.Groups) + if errRole != nil { + return nil, errRole } if s.allowAssignGrafanaAdmin { diff --git a/pkg/login/social/gitlab_oauth_test.go b/pkg/login/social/gitlab_oauth_test.go index 77dfef1ce00..c1276a91a34 100644 --- a/pkg/login/social/gitlab_oauth_test.go +++ b/pkg/login/social/gitlab_oauth_test.go @@ -105,14 +105,14 @@ func TestSocialGitlab_UserInfo(t *testing.T) { ExpectedGrafanaAdmin: nilPointer, }, { // Case that's going to change with Grafana 10 - Name: "No fallback to default org role (will change in Grafana 10)", - Cfg: conf{AutoAssignOrgRole: org.RoleViewer}, + Name: "No fallback to default org role", + Cfg: conf{AutoAssignOrgRole: org.RoleAdmin}, UserRespBody: editorUserRespBody, GroupsRespBody: "[" + strings.Join([]string{}, ",") + "]", RoleAttributePath: gitlabAttrPath, ExpectedLogin: "gitlab-editor", ExpectedEmail: "gitlab-editor@example.org", - ExpectedRole: "", + ExpectedRole: "Admin", }, { Name: "Strict mode prevents fallback to default", @@ -120,25 +120,25 @@ func TestSocialGitlab_UserInfo(t *testing.T) { UserRespBody: editorUserRespBody, GroupsRespBody: "[" + strings.Join([]string{}, ",") + "]", RoleAttributePath: gitlabAttrPath, - ExpectedError: &InvalidBasicRoleError{idP: "Gitlab"}, + ExpectedError: errRoleAttributeStrictViolation, }, - { // Edge case, no match, no strict mode and no fallback => User has an empty role - Name: "Fallback with no default will create a user with an empty role", + { // Edge case, no match, no strict mode and no fallback => User has the Viewer role (hard coded) + Name: "Fallback with no default will create a user with a default role", Cfg: conf{}, UserRespBody: editorUserRespBody, GroupsRespBody: "[" + strings.Join([]string{}, ",") + "]", RoleAttributePath: gitlabAttrPath, ExpectedLogin: "gitlab-editor", ExpectedEmail: "gitlab-editor@example.org", - ExpectedRole: "", + ExpectedRole: "Viewer", }, - { // Edge case, no attribute path with strict mode => User has an empty role + { // Edge case, no attribute path with strict mode => Error Name: "Strict mode with no attribute path", Cfg: conf{RoleAttributeStrict: true, AutoAssignOrgRole: org.RoleViewer}, UserRespBody: editorUserRespBody, GroupsRespBody: "[" + strings.Join([]string{editorGroup}, ",") + "]", RoleAttributePath: "", - ExpectedError: &InvalidBasicRoleError{idP: "Gitlab"}, + ExpectedError: errRoleAttributePathNotSet, }, } @@ -167,7 +167,7 @@ func TestSocialGitlab_UserInfo(t *testing.T) { provider.apiUrl = ts.URL + apiURI actualResult, err := provider.UserInfo(context.Background(), ts.Client(), &oauth2.Token{}) if test.ExpectedError != nil { - require.Equal(t, err, test.ExpectedError) + require.ErrorIs(t, err, test.ExpectedError) return } @@ -246,7 +246,7 @@ func TestSocialGitlab_extractFromToken(t *testing.T) { Name: "John Doe", Groups: []string{"admins", "editors", "viewers"}, EmailVerified: true, - Role: "", + Role: "Viewer", IsGrafanaAdmin: nil, }, }, @@ -313,7 +313,7 @@ func TestSocialGitlab_extractFromToken(t *testing.T) { Name: "John Doe", Groups: []string{"admins"}, EmailVerified: true, - Role: "", + Role: "Viewer", IsGrafanaAdmin: nil, }, }, diff --git a/pkg/login/social/okta_oauth.go b/pkg/login/social/okta_oauth.go index 5ce3e416d71..c3b3cc2b453 100644 --- a/pkg/login/social/okta_oauth.go +++ b/pkg/login/social/okta_oauth.go @@ -83,10 +83,11 @@ func (s *SocialOkta) UserInfo(ctx context.Context, client *http.Client, token *o var isGrafanaAdmin *bool if !s.skipOrgRoleSync { var grafanaAdmin bool - role, grafanaAdmin = s.extractRoleAndAdmin(data.rawJSON, groups, true) - if s.roleAttributeStrict && !role.IsValid() { - return nil, &InvalidBasicRoleError{idP: "Okta", assignedRole: string(role)} + role, grafanaAdmin, err = s.extractRoleAndAdmin(data.rawJSON, groups) + if err != nil { + return nil, err } + if s.allowAssignGrafanaAdmin { isGrafanaAdmin = &grafanaAdmin } diff --git a/pkg/login/social/social.go b/pkg/login/social/social.go index 2654e788fa1..ad18fbac6de 100644 --- a/pkg/login/social/social.go +++ b/pkg/login/social/social.go @@ -388,11 +388,37 @@ func (s *SocialBase) SupportBundleContent(bf *bytes.Buffer) error { return nil } -func (s *SocialBase) extractRoleAndAdmin(rawJSON []byte, groups []string, legacy bool) (org.RoleType, bool) { +func (s *SocialBase) extractRoleAndAdminOptional(rawJSON []byte, groups []string) (org.RoleType, bool, error) { if s.roleAttributePath == "" { - return s.defaultRole(legacy), false + if s.roleAttributeStrict { + return "", false, errRoleAttributePathNotSet.Errorf("role_attribute_path not set and role_attribute_strict is set") + } + return "", false, nil } + if role, gAdmin := s.searchRole(rawJSON, groups); role.IsValid() { + return role, gAdmin, nil + } else if role != "" { + return "", false, errInvalidRole.Errorf("invalid role: %s", role) + } + + if s.roleAttributeStrict { + return "", false, errRoleAttributeStrictViolation.Errorf("idP did not return a role attribute, but role_attribute_strict is set") + } + + return "", false, nil +} + +func (s *SocialBase) extractRoleAndAdmin(rawJSON []byte, groups []string) (org.RoleType, bool, error) { + role, gAdmin, err := s.extractRoleAndAdminOptional(rawJSON, groups) + if role == "" { + role = s.defaultRole() + } + + return role, gAdmin, err +} + +func (s *SocialBase) searchRole(rawJSON []byte, groups []string) (org.RoleType, bool) { role, err := s.searchJSONForStringAttr(s.roleAttributePath, rawJSON) if err == nil && role != "" { return getRoleFromSearch(role) @@ -405,29 +431,19 @@ func (s *SocialBase) extractRoleAndAdmin(rawJSON []byte, groups []string, legacy } } - return s.defaultRole(legacy), false + return "", false } // defaultRole returns the default role for the user based on the autoAssignOrgRole setting // if legacy is enabled "" is returned indicating the previous role assignment is used. -func (s *SocialBase) defaultRole(legacy bool) org.RoleType { - if s.roleAttributeStrict { - s.log.Debug("RoleAttributeStrict is set, returning no role.") - return "" - } - - if s.autoAssignOrgRole != "" && !legacy { +func (s *SocialBase) defaultRole() org.RoleType { + if s.autoAssignOrgRole != "" { s.log.Debug("No role found, returning default.") return org.RoleType(s.autoAssignOrgRole) } - if legacy && !s.skipOrgRoleSync { - s.log.Warn("No valid role found. Skipping role sync. " + - "In Grafana 10, this will result in the user being assigned the default role and overriding manual assignment. " + - "If role sync is not desired, set skip_org_role_sync for your provider to true") - } - - return "" + // should never happen + return org.RoleViewer } // match grafana admin role and translate to org role and bool. diff --git a/pkg/setting/setting.go b/pkg/setting/setting.go index 61e93513328..a92bc329cfa 100644 --- a/pkg/setting/setting.go +++ b/pkg/setting/setting.go @@ -30,6 +30,7 @@ import ( "github.com/grafana/grafana-plugin-sdk-go/backend/gtime" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/models/roletype" "github.com/grafana/grafana/pkg/util" ) @@ -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{ + string(roletype.RoleViewer), + string(roletype.RoleEditor), + string(roletype.RoleAdmin)}) VerifyEmailEnabled = users.Key("verify_email_enabled").MustBool(false) cfg.CaseInsensitiveLogin = users.Key("case_insensitive_login").MustBool(true)