From 3f60ef51463c3c5bca2a4d489e7cc9987f949c35 Mon Sep 17 00:00:00 2001 From: Misi Date: Mon, 31 Mar 2025 15:29:33 +0200 Subject: [PATCH] [release-11.3.6] Auth: Fix SAML user IsExternallySynced not being set correctly (#103101) Auth: Fix SAML user IsExternallySynced not being set correctly (#98487) (cherry picked from commit 345757c3ae1f2016bac08cccab5d4352056768d2) Co-authored-by: xavi <114113189+volcanonoodle@users.noreply.github.com> --- pkg/api/admin_users.go | 3 +- pkg/api/admin_users_test.go | 93 ++++++++++----- pkg/api/login.go | 90 +++++++++++++++ pkg/api/login_test.go | 166 +++++++++++++++++++++++++++ pkg/api/org_users.go | 6 +- pkg/api/org_users_test.go | 12 +- pkg/api/password.go | 3 +- pkg/api/user.go | 5 +- pkg/api/user_test.go | 60 +++++++++- pkg/api/utils.go | 2 +- pkg/login/social/social.go | 8 ++ pkg/services/authn/authn.go | 4 + pkg/services/authn/authntest/fake.go | 29 ++++- pkg/services/login/authinfo.go | 70 ----------- pkg/services/login/authinfo_test.go | 131 --------------------- 15 files changed, 432 insertions(+), 250 deletions(-) delete mode 100644 pkg/services/login/authinfo_test.go diff --git a/pkg/api/admin_users.go b/pkg/api/admin_users.go index 456484d54f8..306bf3be52e 100644 --- a/pkg/api/admin_users.go +++ b/pkg/api/admin_users.go @@ -158,8 +158,7 @@ func (hs *HTTPServer) AdminUpdateUserPermissions(c *contextmodel.ReqContext) res } if authInfo, err := hs.authInfoService.GetAuthInfo(c.Req.Context(), &login.GetAuthInfoQuery{UserId: userID}); err == nil && authInfo != nil { - oauthInfo := hs.SocialService.GetOAuthInfoProvider(authInfo.AuthModule) - if login.IsGrafanaAdminExternallySynced(hs.Cfg, oauthInfo, authInfo.AuthModule) { + if hs.isGrafanaAdminExternallySynced(hs.Cfg, authInfo.AuthModule) { return response.Error(http.StatusForbidden, "Cannot change Grafana Admin role for externally synced user", nil) } } diff --git a/pkg/api/admin_users_test.go b/pkg/api/admin_users_test.go index 0b6011d95e2..52e64aad75f 100644 --- a/pkg/api/admin_users_test.go +++ b/pkg/api/admin_users_test.go @@ -14,10 +14,11 @@ import ( "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/db/dbtest" - "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/login/social/socialtest" "github.com/grafana/grafana/pkg/services/auth" "github.com/grafana/grafana/pkg/services/auth/authtest" + "github.com/grafana/grafana/pkg/services/authn" + "github.com/grafana/grafana/pkg/services/authn/authntest" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/login/authinfotest" @@ -243,39 +244,72 @@ func Test_AdminUpdateUserPermissions(t *testing.T) { authEnabled bool skipOrgRoleSync bool expectedRespCode int + enabledAuthnClients []string + authnClientConfig authn.SSOClientConfig }{ + // oauth { - name: "Should allow updating an externally synced OAuth user if Grafana Admin role is not synced", - authModule: login.GenericOAuthModule, - authEnabled: true, - allowAssignGrafanaAdmin: false, - skipOrgRoleSync: false, - expectedRespCode: http.StatusOK, + name: "Should allow updating an externally synced OAuth user if Grafana Admin role is not synced", + authModule: login.GenericOAuthModule, + enabledAuthnClients: []string{authn.ClientWithPrefix("generic_oauth")}, + authnClientConfig: &authntest.FakeSSOClientConfig{ + ExpectedIsSkipOrgRoleSyncEnabled: false, + ExpectedIsAllowAssignGrafanaAdminEnabled: false, + }, + expectedRespCode: http.StatusOK, }, { - name: "Should allow updating an externally synced OAuth user if OAuth provider is not enabled", - authModule: login.GenericOAuthModule, - authEnabled: false, - allowAssignGrafanaAdmin: true, - skipOrgRoleSync: false, - expectedRespCode: http.StatusOK, + name: "Should allow updating an externally synced OAuth user if OAuth provider is not enabled", + authModule: login.GenericOAuthModule, + expectedRespCode: http.StatusOK, + enabledAuthnClients: []string{}, + authnClientConfig: &authntest.FakeSSOClientConfig{ + ExpectedIsSkipOrgRoleSyncEnabled: false, + ExpectedIsAllowAssignGrafanaAdminEnabled: true, + }, }, { - name: "Should allow updating an externally synced OAuth user if org roles are not being synced", - authModule: login.GenericOAuthModule, - authEnabled: true, - allowAssignGrafanaAdmin: true, - skipOrgRoleSync: true, - expectedRespCode: http.StatusOK, + name: "Should allow updating an externally synced OAuth user if org roles are not being synced", + authModule: login.GenericOAuthModule, + expectedRespCode: http.StatusOK, + enabledAuthnClients: []string{authn.ClientWithPrefix("generic_oauth")}, + authnClientConfig: &authntest.FakeSSOClientConfig{ + ExpectedIsSkipOrgRoleSyncEnabled: true, + ExpectedIsAllowAssignGrafanaAdminEnabled: true, + }, }, { - name: "Should not allow updating an externally synced OAuth user", - authModule: login.GenericOAuthModule, - authEnabled: true, - allowAssignGrafanaAdmin: true, - skipOrgRoleSync: false, - expectedRespCode: http.StatusForbidden, + name: "Should not allow updating an externally synced OAuth user", + authModule: login.GenericOAuthModule, + expectedRespCode: http.StatusForbidden, + enabledAuthnClients: []string{authn.ClientWithPrefix("generic_oauth")}, + authnClientConfig: &authntest.FakeSSOClientConfig{ + ExpectedIsSkipOrgRoleSyncEnabled: false, + ExpectedIsAllowAssignGrafanaAdminEnabled: true, + }, }, + // saml + { + name: "Should allow updating an externally synced SAML user if org roles are not being synced", + authModule: login.SAMLAuthModule, + expectedRespCode: http.StatusOK, + enabledAuthnClients: []string{authn.ClientSAML}, + authnClientConfig: &authntest.FakeSSOClientConfig{ + ExpectedIsSkipOrgRoleSyncEnabled: true, + ExpectedIsAllowAssignGrafanaAdminEnabled: true, + }, + }, + { + name: "Should not allow updating an externally synced SAML user", + authModule: login.SAMLAuthModule, + expectedRespCode: http.StatusForbidden, + enabledAuthnClients: []string{authn.ClientSAML}, + authnClientConfig: &authntest.FakeSSOClientConfig{ + ExpectedIsSkipOrgRoleSyncEnabled: false, + ExpectedIsAllowAssignGrafanaAdminEnabled: true, + }, + }, + // jwt { name: "Should allow updating an externally synced JWT user if Grafana Admin role is not synced", authModule: login.JWTModule, @@ -316,10 +350,7 @@ func Test_AdminUpdateUserPermissions(t *testing.T) { socialService := &socialtest.FakeSocialService{} cfg := setting.NewCfg() - switch tc.authModule { - case login.GenericOAuthModule: - socialService.ExpectedAuthInfoProvider = &social.OAuthInfo{AllowAssignGrafanaAdmin: tc.allowAssignGrafanaAdmin, Enabled: tc.authEnabled, SkipOrgRoleSync: tc.skipOrgRoleSync} - case login.JWTModule: + if tc.authModule == login.JWTModule { cfg.JWTAuth.Enabled = tc.authEnabled cfg.JWTAuth.SkipOrgRoleSync = tc.skipOrgRoleSync cfg.JWTAuth.AllowAssignGrafanaAdmin = tc.allowAssignGrafanaAdmin @@ -330,6 +361,10 @@ func Test_AdminUpdateUserPermissions(t *testing.T) { authInfoService: authInfoService, SocialService: socialService, userService: usertest.NewUserServiceFake(), + authnService: &authntest.FakeService{ + ExpectedClientConfig: tc.authnClientConfig, + EnabledClients: tc.enabledAuthnClients, + }, } sc := setupScenarioContext(t, "/api/admin/users/1/permissions") diff --git a/pkg/api/login.go b/pkg/api/login.go index 338ccf5a24e..3376b6e4984 100644 --- a/pkg/api/login.go +++ b/pkg/api/login.go @@ -24,6 +24,7 @@ import ( pref "github.com/grafana/grafana/pkg/services/preference" "github.com/grafana/grafana/pkg/services/secrets" "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" ) @@ -382,6 +383,22 @@ func (hs *HTTPServer) samlAutoLoginEnabled() bool { return hs.samlEnabled() && config.IsAutoLoginEnabled() } +func (hs *HTTPServer) samlSkipOrgRoleSyncEnabled() bool { + config, ok := hs.authnService.GetClientConfig(authn.ClientSAML) + if !ok { + return false + } + return hs.samlEnabled() && config.IsSkipOrgRoleSyncEnabled() +} + +func (hs *HTTPServer) samlAllowAssignGrafanaAdminEnabled() bool { + config, ok := hs.authnService.GetClientConfig(authn.ClientSAML) + if !ok { + return false + } + return hs.samlEnabled() && config.IsAllowAssignGrafanaAdminEnabled() +} + func getLoginExternalError(err error) string { var createTokenErr *auth.CreateTokenErr if errors.As(err, &createTokenErr) { @@ -411,3 +428,76 @@ func getFirstPublicErrorMessage(err *errutil.Error) string { return errPublic.Message } + +// isExternalySynced is used to tell if the user roles are externally synced +// true means that the org role sync is handled by Grafana +// Note: currently the users authinfo is overridden each time the user logs in +// https://github.com/grafana/grafana/blob/4181acec72f76df7ad02badce13769bae4a1f840/pkg/services/login/authinfoservice/database/database.go#L61 +// this means that if the user has multiple auth providers and one of them is set to sync org roles +// then isExternallySynced will be true for this one provider and false for the others +func (hs *HTTPServer) isExternallySynced(cfg *setting.Cfg, authModule string) bool { + // provider enabled in config + if !hs.isProviderEnabled(cfg, authModule) { + return false + } + // first check SAML, LDAP and JWT + switch authModule { + case loginservice.SAMLAuthModule: + return !hs.samlSkipOrgRoleSyncEnabled() + case loginservice.LDAPAuthModule: + return !cfg.LDAPSkipOrgRoleSync + case loginservice.JWTModule: + return !cfg.JWTAuth.SkipOrgRoleSync + } + switch authModule { + case loginservice.GoogleAuthModule, loginservice.OktaAuthModule, loginservice.AzureADAuthModule, loginservice.GitLabAuthModule, loginservice.GithubAuthModule, loginservice.GrafanaComAuthModule, loginservice.GenericOAuthModule: + config, ok := hs.authnService.GetClientConfig(oauthModuleToAuthnClient(authModule)) + if !ok { + return false + } + return !config.IsSkipOrgRoleSyncEnabled() + } + return true +} + +// isGrafanaAdminExternallySynced returns true if Grafana server admin role is being managed by an external auth provider, and false otherwise. +// Grafana admin role sync is available for JWT, OAuth providers and LDAP. +// For JWT and OAuth providers there is an additional config option `allow_assign_grafana_admin` that has to be enabled for Grafana Admin role to be synced. +func (hs *HTTPServer) isGrafanaAdminExternallySynced(cfg *setting.Cfg, authModule string) bool { + if !hs.isExternallySynced(cfg, authModule) { + return false + } + + switch authModule { + case loginservice.JWTModule: + return cfg.JWTAuth.AllowAssignGrafanaAdmin + case loginservice.SAMLAuthModule: + return hs.samlAllowAssignGrafanaAdminEnabled() + case loginservice.LDAPAuthModule: + return true + default: + config, ok := hs.authnService.GetClientConfig(oauthModuleToAuthnClient(authModule)) + if !ok { + return false + } + return config.IsAllowAssignGrafanaAdminEnabled() + } +} + +func (hs *HTTPServer) isProviderEnabled(cfg *setting.Cfg, authModule string) bool { + switch authModule { + case loginservice.SAMLAuthModule: + return hs.authnService.IsClientEnabled(authn.ClientSAML) + case loginservice.LDAPAuthModule: + return cfg.LDAPAuthEnabled + case loginservice.JWTModule: + return cfg.JWTAuth.Enabled + case loginservice.GoogleAuthModule, loginservice.OktaAuthModule, loginservice.AzureADAuthModule, loginservice.GitLabAuthModule, loginservice.GithubAuthModule, loginservice.GrafanaComAuthModule, loginservice.GenericOAuthModule: + return hs.authnService.IsClientEnabled(oauthModuleToAuthnClient(authModule)) + } + return false +} + +func oauthModuleToAuthnClient(authModule string) string { + return authn.ClientWithPrefix(strings.TrimPrefix(authModule, "oauth_")) +} diff --git a/pkg/api/login_test.go b/pkg/api/login_test.go index c6dad435ae0..16fffedc2ad 100644 --- a/pkg/api/login_test.go +++ b/pkg/api/login_test.go @@ -688,6 +688,172 @@ func TestLogoutSaml(t *testing.T) { require.Equal(t, 302, sc.resp.Code) } +func TestIsExternallySynced(t *testing.T) { + testcases := []struct { + name string + cfg *setting.Cfg + provider string + enabledAuthnClients []string + authnClientConfig authn.SSOClientConfig + expected bool + }{ + // Same for all of the OAuth providers + { + name: "AzureAD external user should return that it is externally synced", + cfg: &setting.Cfg{}, + provider: loginservice.AzureADAuthModule, + enabledAuthnClients: []string{authn.ClientWithPrefix("azuread")}, + authnClientConfig: &authntest.FakeSSOClientConfig{ + ExpectedIsSkipOrgRoleSyncEnabled: false, + }, + expected: true, + }, + { + name: "AzureAD external user should return that it is not externally synced when org role sync is set", + cfg: &setting.Cfg{}, + provider: loginservice.AzureADAuthModule, + enabledAuthnClients: []string{authn.ClientWithPrefix(strings.TrimPrefix(loginservice.AzureADAuthModule, "oauth_"))}, + authnClientConfig: &authntest.FakeSSOClientConfig{ + ExpectedIsSkipOrgRoleSyncEnabled: true, + }, + expected: false, + }, + { + name: "AzureAD external user should return that it is not externally synced when the provider is not enabled", + cfg: &setting.Cfg{}, + enabledAuthnClients: []string{}, + authnClientConfig: &authntest.FakeSSOClientConfig{ + ExpectedIsSkipOrgRoleSyncEnabled: false, + }, + provider: loginservice.AzureADAuthModule, + expected: false, + }, + // saml + { + name: "SAML synced user should return that it is not externally synced when the provider is disabled", + cfg: &setting.Cfg{}, + provider: loginservice.SAMLAuthModule, + enabledAuthnClients: []string{}, + authnClientConfig: &authntest.FakeSSOClientConfig{ + ExpectedIsSkipOrgRoleSyncEnabled: false, + }, + expected: false, + }, + { + name: "SAML synced user should return that it is externally synced", + cfg: &setting.Cfg{}, + provider: loginservice.SAMLAuthModule, + enabledAuthnClients: []string{authn.ClientSAML}, + authnClientConfig: &authntest.FakeSSOClientConfig{ + ExpectedIsSkipOrgRoleSyncEnabled: false, + }, + expected: true, + }, + { + name: "SAML synced user should return that it is not externally synced when org role sync is set", + cfg: &setting.Cfg{}, + provider: loginservice.SAMLAuthModule, + enabledAuthnClients: []string{authn.ClientSAML}, + authnClientConfig: &authntest.FakeSSOClientConfig{ + ExpectedIsSkipOrgRoleSyncEnabled: true, + }, + expected: false, + }, + // ldap + { + name: "LDAP synced user should return that it is externally synced", + cfg: &setting.Cfg{LDAPAuthEnabled: true, LDAPSkipOrgRoleSync: false}, + provider: loginservice.LDAPAuthModule, + expected: true, + }, + { + name: "LDAP synced user should return that it is not externally synced when org role sync is set", + cfg: &setting.Cfg{LDAPAuthEnabled: true, LDAPSkipOrgRoleSync: true}, + provider: loginservice.LDAPAuthModule, + expected: false, + }, + // jwt + { + name: "JWT synced user should return that it is externally synced", + cfg: &setting.Cfg{JWTAuth: setting.AuthJWTSettings{Enabled: true, SkipOrgRoleSync: false}}, + provider: loginservice.JWTModule, + expected: true, + }, + { + name: "JWT synced user should return that it is not externally synced when org role sync is set", + cfg: &setting.Cfg{JWTAuth: setting.AuthJWTSettings{Enabled: true, SkipOrgRoleSync: true}}, + provider: loginservice.JWTModule, + expected: false, + }, + // IsProvider test + { + name: "If no provider enabled should return false", + cfg: &setting.Cfg{JWTAuth: setting.AuthJWTSettings{Enabled: false, SkipOrgRoleSync: true}}, + provider: loginservice.JWTModule, + expected: false, + }, + } + + for _, tc := range testcases { + hs := &HTTPServer{ + authnService: &authntest.FakeService{ + ExpectedClientConfig: tc.authnClientConfig, + EnabledClients: tc.enabledAuthnClients, + }, + } + + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expected, hs.isExternallySynced(tc.cfg, tc.provider)) + }) + } +} + +func TestIsProviderEnabled(t *testing.T) { + testcases := []struct { + name string + provider string + enabledAuthnClients []string + expected bool + }{ + { + name: "Github should return true if enabled", + provider: loginservice.GithubAuthModule, + enabledAuthnClients: []string{authn.ClientWithPrefix(strings.TrimPrefix(loginservice.GithubAuthModule, "oauth_"))}, + expected: true, + }, + { + name: "Github should return false if not enabled", + provider: loginservice.GithubAuthModule, + enabledAuthnClients: []string{}, + expected: false, + }, + // saml + { + name: "SAML should return true if enabled", + provider: loginservice.SAMLAuthModule, + enabledAuthnClients: []string{authn.ClientSAML}, + expected: true, + }, + { + name: "SAML should return false if not enabled", + provider: loginservice.SAMLAuthModule, + enabledAuthnClients: []string{}, + expected: false, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + hs := &HTTPServer{ + authnService: &authntest.FakeService{ + EnabledClients: tc.enabledAuthnClients, + }, + } + assert.Equal(t, tc.expected, hs.isProviderEnabled(setting.NewCfg(), tc.provider)) + }) + } +} + type mockSocialService struct { oAuthInfo *social.OAuthInfo oAuthInfos map[string]*social.OAuthInfo diff --git a/pkg/api/org_users.go b/pkg/api/org_users.go index 401148307c8..f8cbae0e1c3 100644 --- a/pkg/api/org_users.go +++ b/pkg/api/org_users.go @@ -338,9 +338,8 @@ func (hs *HTTPServer) searchOrgUsersHelper(c *contextmodel.ReqContext, query *or for i := range filteredUsers { filteredUsers[i].AccessControl = accessControlMetadata[fmt.Sprint(filteredUsers[i].UserID)] if module, ok := modules[filteredUsers[i].UserID]; ok { - oauthInfo := hs.SocialService.GetOAuthInfoProvider(module) filteredUsers[i].AuthLabels = []string{login.GetAuthProviderLabel(module)} - filteredUsers[i].IsExternallySynced = login.IsExternallySynced(hs.Cfg, module, oauthInfo) + filteredUsers[i].IsExternallySynced = hs.isExternallySynced(hs.Cfg, module) } } @@ -427,8 +426,7 @@ func (hs *HTTPServer) updateOrgUserHelper(c *contextmodel.ReqContext, cmd org.Up } } if authInfo != nil && authInfo.AuthModule != "" { - oauthInfo := hs.SocialService.GetOAuthInfoProvider(authInfo.AuthModule) - if login.IsExternallySynced(hs.Cfg, authInfo.AuthModule, oauthInfo) { + if hs.isExternallySynced(hs.Cfg, authInfo.AuthModule) { return response.Err(org.ErrCannotChangeRoleForExternallySyncedUser.Errorf("Cannot change role for externally synced user")) } } diff --git a/pkg/api/org_users_test.go b/pkg/api/org_users_test.go index ea5f4930661..59bb47330d6 100644 --- a/pkg/api/org_users_test.go +++ b/pkg/api/org_users_test.go @@ -9,6 +9,8 @@ import ( "strings" "testing" + "github.com/grafana/grafana/pkg/services/authn" + "github.com/grafana/grafana/pkg/services/authn/authntest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -258,9 +260,17 @@ func TestOrgUsersAPIEndpoint_updateOrgRole(t *testing.T) { server := SetupAPITestServer(t, func(hs *HTTPServer) { hs.Cfg = setting.NewCfg() hs.Cfg.LDAPAuthEnabled = tt.AuthEnabled - if tt.AuthModule == login.LDAPAuthModule { + switch tt.AuthModule { + case login.LDAPAuthModule: hs.Cfg.LDAPAuthEnabled = tt.AuthEnabled hs.Cfg.LDAPSkipOrgRoleSync = tt.SkipOrgRoleSync + case login.GrafanaComAuthModule: + hs.authnService = &authntest.FakeService{ + EnabledClients: []string{authn.ClientWithPrefix("grafana_com")}, + ExpectedClientConfig: &authntest.FakeSSOClientConfig{ + ExpectedIsSkipOrgRoleSyncEnabled: tt.SkipOrgRoleSync, + }, + } } // AuthModule empty means basic auth diff --git a/pkg/api/password.go b/pkg/api/password.go index fb6db45d939..b7adf54eed9 100644 --- a/pkg/api/password.go +++ b/pkg/api/password.go @@ -39,8 +39,7 @@ func (hs *HTTPServer) SendResetPasswordEmail(c *contextmodel.ReqContext) respons getAuthQuery := login.GetAuthInfoQuery{UserId: usr.ID} if authInfo, err := hs.authInfoService.GetAuthInfo(c.Req.Context(), &getAuthQuery); err == nil { - oauthInfo := hs.SocialService.GetOAuthInfoProvider(authInfo.AuthModule) - if login.IsProviderEnabled(hs.Cfg, authInfo.AuthModule, oauthInfo) { + if hs.isProviderEnabled(hs.Cfg, authInfo.AuthModule) { c.Logger.Info("Requested password reset for external user", nil) return response.Error(http.StatusOK, "Email sent", nil) } diff --git a/pkg/api/user.go b/pkg/api/user.go index c8eaf8ed223..c45f8d136bd 100644 --- a/pkg/api/user.go +++ b/pkg/api/user.go @@ -86,9 +86,8 @@ func (hs *HTTPServer) getUserUserProfile(c *contextmodel.ReqContext, userID int6 userProfile.AuthLabels = append(userProfile.AuthLabels, authLabel) userProfile.IsExternal = true - oauthInfo := hs.SocialService.GetOAuthInfoProvider(authInfo.AuthModule) - userProfile.IsExternallySynced = login.IsExternallySynced(hs.Cfg, authInfo.AuthModule, oauthInfo) - userProfile.IsGrafanaAdminExternallySynced = login.IsGrafanaAdminExternallySynced(hs.Cfg, oauthInfo, authInfo.AuthModule) + userProfile.IsExternallySynced = hs.isExternallySynced(hs.Cfg, authInfo.AuthModule) + userProfile.IsGrafanaAdminExternallySynced = hs.isGrafanaAdminExternallySynced(hs.Cfg, authInfo.AuthModule) } userProfile.AccessControl = getAccessControlMetadata(c, "global.users:id:", strconv.FormatInt(userID, 10)) diff --git a/pkg/api/user_test.go b/pkg/api/user_test.go index 5e1ec0d112c..3ecf9f752d9 100644 --- a/pkg/api/user_test.go +++ b/pkg/api/user_test.go @@ -10,6 +10,8 @@ import ( "testing" "time" + "github.com/grafana/grafana/pkg/services/authn" + "github.com/grafana/grafana/pkg/services/authn/authntest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/oauth2" @@ -243,6 +245,7 @@ func Test_GetUserByID(t *testing.T) { authEnabled bool skipOrgRoleSync bool expectedIsGrafanaAdminSynced bool + expectedIsExternallySynced bool }{ { name: "Should return IsGrafanaAdminExternallySynced = false for an externally synced OAuth user if Grafana Admin role is not synced", @@ -251,6 +254,7 @@ func Test_GetUserByID(t *testing.T) { allowAssignGrafanaAdmin: false, skipOrgRoleSync: false, expectedIsGrafanaAdminSynced: false, + expectedIsExternallySynced: true, }, { name: "Should return IsGrafanaAdminExternallySynced = false for an externally synced OAuth user if OAuth provider is not enabled", @@ -259,6 +263,7 @@ func Test_GetUserByID(t *testing.T) { allowAssignGrafanaAdmin: true, skipOrgRoleSync: false, expectedIsGrafanaAdminSynced: false, + expectedIsExternallySynced: false, }, { name: "Should return IsGrafanaAdminExternallySynced = false for an externally synced OAuth user if org roles are not being synced", @@ -267,6 +272,7 @@ func Test_GetUserByID(t *testing.T) { allowAssignGrafanaAdmin: true, skipOrgRoleSync: true, expectedIsGrafanaAdminSynced: false, + expectedIsExternallySynced: false, }, { name: "Should return IsGrafanaAdminExternallySynced = true for an externally synced OAuth user", @@ -275,6 +281,7 @@ func Test_GetUserByID(t *testing.T) { allowAssignGrafanaAdmin: true, skipOrgRoleSync: false, expectedIsGrafanaAdminSynced: true, + expectedIsExternallySynced: true, }, { name: "Should return IsGrafanaAdminExternallySynced = false for an externally synced JWT user if Grafana Admin role is not synced", @@ -283,6 +290,7 @@ func Test_GetUserByID(t *testing.T) { allowAssignGrafanaAdmin: false, skipOrgRoleSync: false, expectedIsGrafanaAdminSynced: false, + expectedIsExternallySynced: true, }, { name: "Should return IsGrafanaAdminExternallySynced = false for an externally synced JWT user if JWT provider is not enabled", @@ -291,6 +299,7 @@ func Test_GetUserByID(t *testing.T) { allowAssignGrafanaAdmin: true, skipOrgRoleSync: false, expectedIsGrafanaAdminSynced: false, + expectedIsExternallySynced: false, }, { name: "Should return IsGrafanaAdminExternallySynced = false for an externally synced JWT user if org roles are not being synced", @@ -299,6 +308,7 @@ func Test_GetUserByID(t *testing.T) { allowAssignGrafanaAdmin: true, skipOrgRoleSync: true, expectedIsGrafanaAdminSynced: false, + expectedIsExternallySynced: false, }, { name: "Should return IsGrafanaAdminExternallySynced = true for an externally synced JWT user", @@ -307,6 +317,31 @@ func Test_GetUserByID(t *testing.T) { allowAssignGrafanaAdmin: true, skipOrgRoleSync: false, expectedIsGrafanaAdminSynced: true, + expectedIsExternallySynced: true, + }, + { + name: "Should return IsExternallySynced = true for an externally synced SAML user", + authModule: login.SAMLAuthModule, + authEnabled: true, + skipOrgRoleSync: false, + expectedIsGrafanaAdminSynced: false, + expectedIsExternallySynced: true, + }, + { + name: "Should return IsExternallySynced = false for an externally synced SAML user if SAML provider is not enabled", + authModule: login.SAMLAuthModule, + authEnabled: false, + skipOrgRoleSync: false, + expectedIsGrafanaAdminSynced: false, + expectedIsExternallySynced: false, + }, + { + name: "Should return IsExternallySynced = false for an externally synced SAML user if if org roles are not being synced", + authModule: login.SAMLAuthModule, + authEnabled: true, + skipOrgRoleSync: true, + expectedIsGrafanaAdminSynced: false, + expectedIsExternallySynced: false, }, } for _, tc := range testcases { @@ -315,15 +350,28 @@ func Test_GetUserByID(t *testing.T) { authInfoService := &authinfotest.FakeService{ExpectedUserAuth: userAuth} socialService := &socialtest.FakeSocialService{} userService := &usertest.FakeUserService{ExpectedUserProfileDTO: &user.UserProfileDTO{}} + authnService := &authntest.FakeService{ + ExpectedClientConfig: &authntest.FakeSSOClientConfig{ + ExpectedIsSkipOrgRoleSyncEnabled: tc.skipOrgRoleSync, + ExpectedIsAllowAssignGrafanaAdminEnabled: tc.allowAssignGrafanaAdmin, + }, + EnabledClients: []string{}, + } cfg := setting.NewCfg() switch tc.authModule { case login.GenericOAuthModule: - socialService.ExpectedAuthInfoProvider = &social.OAuthInfo{AllowAssignGrafanaAdmin: tc.allowAssignGrafanaAdmin, Enabled: tc.authEnabled, SkipOrgRoleSync: tc.skipOrgRoleSync} + if tc.authEnabled { + authnService.EnabledClients = []string{authn.ClientWithPrefix("generic_oauth")} + } case login.JWTModule: cfg.JWTAuth.Enabled = tc.authEnabled cfg.JWTAuth.SkipOrgRoleSync = tc.skipOrgRoleSync cfg.JWTAuth.AllowAssignGrafanaAdmin = tc.allowAssignGrafanaAdmin + case login.SAMLAuthModule: + if tc.authEnabled { + authnService.EnabledClients = []string{authn.ClientSAML} + } } hs := &HTTPServer{ @@ -331,6 +379,7 @@ func Test_GetUserByID(t *testing.T) { authInfoService: authInfoService, SocialService: socialService, userService: userService, + authnService: authnService, } sc := setupScenarioContext(t, "/api/users/1") @@ -348,13 +397,13 @@ func Test_GetUserByID(t *testing.T) { require.NoError(t, err) assert.Equal(t, tc.expectedIsGrafanaAdminSynced, resp.IsGrafanaAdminExternallySynced) + assert.Equal(t, tc.expectedIsExternallySynced, resp.IsExternallySynced) }) } } func TestHTTPServer_UpdateUser(t *testing.T) { settings := setting.NewCfg() - settings.SAMLAuthEnabled = true sqlStore := db.InitTestDB(t) hs := &HTTPServer{ @@ -362,6 +411,9 @@ func TestHTTPServer_UpdateUser(t *testing.T) { SQLStore: sqlStore, AccessControl: acmock.New(), SocialService: &socialtest.FakeSocialService{ExpectedAuthInfoProvider: &social.OAuthInfo{Enabled: true}}, + authnService: &authntest.FakeService{ + EnabledClients: []string{authn.ClientSAML}, + }, } updateUserCommand := user.UpdateUserCommand{ @@ -1102,13 +1154,15 @@ func updateUserScenario(t *testing.T, ctx updateUserContext, hs *HTTPServer) { func TestHTTPServer_UpdateSignedInUser(t *testing.T) { settings := setting.NewCfg() sqlStore := db.InitTestDB(t) - settings.SAMLAuthEnabled = true hs := &HTTPServer{ Cfg: settings, SQLStore: sqlStore, AccessControl: acmock.New(), SocialService: &socialtest.FakeSocialService{}, + authnService: &authntest.FakeService{ + EnabledClients: []string{authn.ClientSAML}, + }, } updateUserCommand := user.UpdateUserCommand{ diff --git a/pkg/api/utils.go b/pkg/api/utils.go index f9c648cc2fa..ec21cf3f3a3 100644 --- a/pkg/api/utils.go +++ b/pkg/api/utils.go @@ -49,7 +49,7 @@ func (hs *HTTPServer) isExternalUser(ctx context.Context, userID int64) (bool, e return true, err } - return login.IsProviderEnabled(hs.Cfg, info.AuthModule, hs.SocialService.GetOAuthInfoProvider(info.AuthModule)), nil + return hs.isProviderEnabled(hs.Cfg, info.AuthModule), nil } func ValidateAndNormalizeEmail(email string) (string, error) { diff --git a/pkg/login/social/social.go b/pkg/login/social/social.go index 12cf2dfd0a7..93523dd4e4f 100644 --- a/pkg/login/social/social.go +++ b/pkg/login/social/social.go @@ -112,6 +112,14 @@ func (o *OAuthInfo) IsAutoLoginEnabled() bool { return o.AutoLogin } +func (o *OAuthInfo) IsSkipOrgRoleSyncEnabled() bool { + return o.SkipOrgRoleSync +} + +func (o *OAuthInfo) IsAllowAssignGrafanaAdminEnabled() bool { + return o.AllowAssignGrafanaAdmin +} + type BasicUserInfo struct { Id string Name string diff --git a/pkg/services/authn/authn.go b/pkg/services/authn/authn.go index a689fdfa0f7..94a78bf4da3 100644 --- a/pkg/services/authn/authn.go +++ b/pkg/services/authn/authn.go @@ -93,6 +93,10 @@ type SSOClientConfig interface { IsAutoLoginEnabled() bool // IsSingleLogoutEnabled returns true if the client has single logout enabled IsSingleLogoutEnabled() bool + // IsSkipOrgRoleSyncEnabled returns true if the client has enabled skipping org role sync + IsSkipOrgRoleSyncEnabled() bool + // IsAllowAssignGrafanaAdminEnabled returns true if the client has enabled assigning grafana admin + IsAllowAssignGrafanaAdminEnabled() bool } type Service interface { diff --git a/pkg/services/authn/authntest/fake.go b/pkg/services/authn/authntest/fake.go index 60892f99bf0..fc42a91a115 100644 --- a/pkg/services/authn/authntest/fake.go +++ b/pkg/services/authn/authntest/fake.go @@ -11,9 +11,11 @@ import ( var _ authn.SSOClientConfig = new(FakeSSOClientConfig) type FakeSSOClientConfig struct { - ExpectedName string - ExpectedIsAutoLoginEnabled bool - ExpectedIsSingleLogoutEnabled bool + ExpectedName string + ExpectedIsAutoLoginEnabled bool + ExpectedIsSingleLogoutEnabled bool + ExpectedIsSkipOrgRoleSyncEnabled bool + ExpectedIsAllowAssignGrafanaAdminEnabled bool } func (f *FakeSSOClientConfig) GetDisplayName() string { @@ -28,6 +30,14 @@ func (f *FakeSSOClientConfig) IsSingleLogoutEnabled() bool { return f.ExpectedIsSingleLogoutEnabled } +func (f *FakeSSOClientConfig) IsSkipOrgRoleSyncEnabled() bool { + return f.ExpectedIsSkipOrgRoleSyncEnabled +} + +func (f *FakeSSOClientConfig) IsAllowAssignGrafanaAdminEnabled() bool { + return f.ExpectedIsAllowAssignGrafanaAdminEnabled +} + var ( _ authn.Service = new(FakeService) _ authn.IdentitySynchronizer = new(FakeService) @@ -41,6 +51,7 @@ type FakeService struct { ExpectedErrs []error ExpectedIdentities []*authn.Identity CurrentIndex int + EnabledClients []string } func (f *FakeService) Authenticate(ctx context.Context, r *authn.Request) (*authn.Identity, error) { @@ -64,7 +75,17 @@ func (f *FakeService) Authenticate(ctx context.Context, r *authn.Request) (*auth } func (f *FakeService) IsClientEnabled(name string) bool { - return true + // Consider all clients as enabled if EnabledClients is not explicitly set + if f.EnabledClients == nil { + return true + } + // Check if client is in the list of enabled clients + for _, s := range f.EnabledClients { + if s == name { + return true + } + } + return false } func (f *FakeService) GetClientConfig(name string) (authn.SSOClientConfig, bool) { diff --git a/pkg/services/login/authinfo.go b/pkg/services/login/authinfo.go index 103372c942d..eccf6490a30 100644 --- a/pkg/services/login/authinfo.go +++ b/pkg/services/login/authinfo.go @@ -2,9 +2,6 @@ package login import ( "context" - - "github.com/grafana/grafana/pkg/login/social" - "github.com/grafana/grafana/pkg/setting" ) type AuthInfoService interface { @@ -58,73 +55,6 @@ const ( OktaLabel = "Okta" ) -// IsExternnalySynced is used to tell if the user roles are externally synced -// true means that the org role sync is handled by Grafana -// Note: currently the users authinfo is overridden each time the user logs in -// https://github.com/grafana/grafana/blob/4181acec72f76df7ad02badce13769bae4a1f840/pkg/services/login/authinfoservice/database/database.go#L61 -// this means that if the user has multiple auth providers and one of them is set to sync org roles -// then IsExternallySynced will be true for this one provider and false for the others -func IsExternallySynced(cfg *setting.Cfg, authModule string, oauthInfo *social.OAuthInfo) bool { - // provider enabled in config - if !IsProviderEnabled(cfg, authModule, oauthInfo) { - return false - } - // first check SAML, LDAP and JWT - switch authModule { - case SAMLAuthModule: - return !cfg.SAMLSkipOrgRoleSync - case LDAPAuthModule: - return !cfg.LDAPSkipOrgRoleSync - case JWTModule: - return !cfg.JWTAuth.SkipOrgRoleSync - } - switch authModule { - case GoogleAuthModule, OktaAuthModule, AzureADAuthModule, GitLabAuthModule, GithubAuthModule, GrafanaComAuthModule, GenericOAuthModule: - if oauthInfo == nil { - return false - } - return !oauthInfo.SkipOrgRoleSync - } - return true -} - -// IsGrafanaAdminExternallySynced returns true if Grafana server admin role is being managed by an external auth provider, and false otherwise. -// Grafana admin role sync is available for JWT, OAuth providers and LDAP. -// For JWT and OAuth providers there is an additional config option `allow_assign_grafana_admin` that has to be enabled for Grafana Admin role to be synced. -func IsGrafanaAdminExternallySynced(cfg *setting.Cfg, oauthInfo *social.OAuthInfo, authModule string) bool { - if !IsExternallySynced(cfg, authModule, oauthInfo) { - return false - } - - switch authModule { - case JWTModule: - return cfg.JWTAuth.AllowAssignGrafanaAdmin - case SAMLAuthModule: - return cfg.SAMLRoleValuesGrafanaAdmin != "" - case LDAPAuthModule: - return true - default: - return oauthInfo != nil && oauthInfo.AllowAssignGrafanaAdmin - } -} - -func IsProviderEnabled(cfg *setting.Cfg, authModule string, oauthInfo *social.OAuthInfo) bool { - switch authModule { - case SAMLAuthModule: - return cfg.SAMLAuthEnabled - case LDAPAuthModule: - return cfg.LDAPAuthEnabled - case JWTModule: - return cfg.JWTAuth.Enabled - case GoogleAuthModule, OktaAuthModule, AzureADAuthModule, GitLabAuthModule, GithubAuthModule, GrafanaComAuthModule, GenericOAuthModule: - if oauthInfo == nil { - return false - } - return oauthInfo.Enabled - } - return false -} - // used for frontend to display a more user friendly label func GetAuthProviderLabel(authModule string) string { switch authModule { diff --git a/pkg/services/login/authinfo_test.go b/pkg/services/login/authinfo_test.go deleted file mode 100644 index 53996b6785c..00000000000 --- a/pkg/services/login/authinfo_test.go +++ /dev/null @@ -1,131 +0,0 @@ -package login - -import ( - "testing" - - "github.com/stretchr/testify/assert" - - "github.com/grafana/grafana/pkg/login/social" - "github.com/grafana/grafana/pkg/setting" -) - -func TestIsExternallySynced(t *testing.T) { - testcases := []struct { - name string - cfg *setting.Cfg - oauthInfo *social.OAuthInfo - provider string - expected bool - }{ - // Same for all of the OAuth providers - { - name: "AzureAD external user should return that it is externally synced", - cfg: &setting.Cfg{}, - oauthInfo: &social.OAuthInfo{Enabled: true, SkipOrgRoleSync: false}, - provider: AzureADAuthModule, - expected: true, - }, - { - name: "AzureAD external user should return that it is not externally synced when org role sync is set", - cfg: &setting.Cfg{}, - oauthInfo: &social.OAuthInfo{Enabled: true, SkipOrgRoleSync: true}, - provider: AzureADAuthModule, - expected: false, - }, - { - name: "AzureAD external user should return that it is not externally synced when the provider is not enabled", - cfg: &setting.Cfg{}, - oauthInfo: &social.OAuthInfo{Enabled: false, SkipOrgRoleSync: false}, - provider: AzureADAuthModule, - expected: false, - }, - { - name: "AzureAD synced user should return that it is not externally synced when the provider is not enabled and nil", - cfg: &setting.Cfg{}, - oauthInfo: nil, - provider: AzureADAuthModule, - expected: false, - }, - // saml - { - name: "SAML synced user should return that it is externally synced", - cfg: &setting.Cfg{SAMLAuthEnabled: true, SAMLSkipOrgRoleSync: false}, - provider: SAMLAuthModule, - expected: true, - }, - { - name: "SAML synced user should return that it is not externally synced when org role sync is set", - cfg: &setting.Cfg{SAMLAuthEnabled: true, SAMLSkipOrgRoleSync: true}, - provider: SAMLAuthModule, - expected: false, - }, - // ldap - { - name: "LDAP synced user should return that it is externally synced", - cfg: &setting.Cfg{LDAPAuthEnabled: true, LDAPSkipOrgRoleSync: false}, - provider: LDAPAuthModule, - expected: true, - }, - { - name: "LDAP synced user should return that it is not externally synced when org role sync is set", - cfg: &setting.Cfg{LDAPAuthEnabled: true, LDAPSkipOrgRoleSync: true}, - provider: LDAPAuthModule, - expected: false, - }, - // jwt - { - name: "JWT synced user should return that it is externally synced", - cfg: &setting.Cfg{JWTAuth: setting.AuthJWTSettings{Enabled: true, SkipOrgRoleSync: false}}, - provider: JWTModule, - expected: true, - }, - { - name: "JWT synced user should return that it is not externally synced when org role sync is set", - cfg: &setting.Cfg{JWTAuth: setting.AuthJWTSettings{Enabled: true, SkipOrgRoleSync: true}}, - provider: JWTModule, - expected: false, - }, - // IsProvider test - { - name: "If no provider enabled should return false", - cfg: &setting.Cfg{JWTAuth: setting.AuthJWTSettings{Enabled: false, SkipOrgRoleSync: true}}, - provider: JWTModule, - expected: false, - }, - } - - for _, tc := range testcases { - t.Run(tc.name, func(t *testing.T) { - assert.Equal(t, tc.expected, IsExternallySynced(tc.cfg, tc.provider, tc.oauthInfo)) - }) - } -} - -func TestIsProviderEnabled(t *testing.T) { - testcases := []struct { - name string - oauthInfo *social.OAuthInfo - provider string - expected bool - }{ - // github - { - name: "Github should return true if enabled", - oauthInfo: &social.OAuthInfo{Enabled: true}, - provider: GithubAuthModule, - expected: true, - }, - { - name: "Github should return false if not enabled", - oauthInfo: &social.OAuthInfo{Enabled: false}, - provider: GithubAuthModule, - expected: false, - }, - } - - for _, tc := range testcases { - t.Run(tc.name, func(t *testing.T) { - assert.Equal(t, tc.expected, IsProviderEnabled(setting.NewCfg(), tc.provider, tc.oauthInfo)) - }) - } -}