From 91b5337600119c917719f55a5584a0472c0ec41d Mon Sep 17 00:00:00 2001 From: Eric Leijonmarck Date: Wed, 8 Feb 2023 20:11:46 +0000 Subject: [PATCH] Auth: Refactoring of frontend skipOrgRoleSync logic to backend (#62921) * WIP * refactor: add function in login for externSynced * refactor: make function to make ExternalSyncedInfo - adds tests - refactors strings into consts * remove: console.log * remove: unnessecary comment * added exhaustive tests * refactor: labelname * removed unused code * missspelling * refactor: based on review comments * add: comment to functions about authinfo behavior * Update pkg/services/login/authinfo.go Co-authored-by: Ieva * Update pkg/services/login/authinfo.go Co-authored-by: Ieva * fix: tests --------- Co-authored-by: Ieva --- pkg/api/frontendsettings.go | 2 +- pkg/api/user.go | 1 + pkg/services/login/authinfo.go | 102 +++++++-- pkg/services/login/authinfo_test.go | 202 ++++++++++++++++++ .../authinfoservice/database/database.go | 2 + pkg/services/user/model.go | 29 +-- pkg/setting/setting.go | 9 + public/app/features/admin/UserAdminPage.tsx | 48 +---- public/app/types/user.ts | 1 + 9 files changed, 319 insertions(+), 77 deletions(-) create mode 100644 pkg/services/login/authinfo_test.go diff --git a/pkg/api/frontendsettings.go b/pkg/api/frontendsettings.go index dd4314d3ad4..922a5c726f3 100644 --- a/pkg/api/frontendsettings.go +++ b/pkg/api/frontendsettings.go @@ -147,7 +147,7 @@ func (hs *HTTPServer) getFrontendSettings(c *contextmodel.ReqContext) (*dtos.Fro Auth: dtos.FrontendSettingsAuthDTO{ OAuthSkipOrgRoleUpdateSync: hs.Cfg.OAuthSkipOrgRoleUpdateSync, - SAMLSkipOrgRoleSync: hs.Cfg.SectionWithEnvOverrides("auth.saml").Key("skip_org_role_sync").MustBool(false), + SAMLSkipOrgRoleSync: hs.Cfg.SAMLSkipOrgRoleSync, LDAPSkipOrgRoleSync: hs.Cfg.LDAPSkipOrgRoleSync, GoogleSkipOrgRoleSync: hs.Cfg.GoogleSkipOrgRoleSync, JWTAuthSkipOrgRoleSync: hs.Cfg.JWTAuthSkipOrgRoleSync, diff --git a/pkg/api/user.go b/pkg/api/user.go index 24b7f8ce994..b287d3102a2 100644 --- a/pkg/api/user.go +++ b/pkg/api/user.go @@ -68,6 +68,7 @@ func (hs *HTTPServer) getUserUserProfile(c *contextmodel.ReqContext, userID int6 authLabel := login.GetAuthProviderLabel(getAuthQuery.Result.AuthModule) userProfile.AuthLabels = append(userProfile.AuthLabels, authLabel) userProfile.IsExternal = true + userProfile.IsExternallySynced = login.IsExternallySynced(hs.Cfg, authLabel) } userProfile.AccessControl = hs.getAccessControlMetadata(c, c.OrgID, "global.users:id:", strconv.FormatInt(userID, 10)) diff --git a/pkg/services/login/authinfo.go b/pkg/services/login/authinfo.go index d44f088ed8a..6a796ad2f74 100644 --- a/pkg/services/login/authinfo.go +++ b/pkg/services/login/authinfo.go @@ -4,6 +4,7 @@ import ( "context" "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/setting" ) type AuthInfoService interface { @@ -17,37 +18,102 @@ type AuthInfoService interface { } const ( + // modules SAMLAuthModule = "auth.saml" LDAPAuthModule = "ldap" AuthProxyAuthModule = "authproxy" JWTModule = "jwt" RenderModule = "render" + // OAuth provider modules + AzureADAuthModule = "oauth_azuread" + GoogleAuthModule = "oauth_google" + GitLabAuthModule = "oauth_gitlab" + GithubAuthModule = "oauth_github" + GenericOAuthModule = "oauth_generic_oauth" + GrafanaComAuthModule = "oauth_grafana_com" + GrafanaNetAuthModule = "oauth_grafananet" + OktaAuthModule = "oauth_okta" + + // labels + SAMLLabel = "SAML" + LDAPLabel = "LDAP" + JWTLabel = "JWT" + // OAuth provider labels + AuthProxtLabel = "Auth Proxy" + AzureADLabel = "AzureAD" + GoogleLabel = "Google" + GenericOAuthLabel = "Generic OAuth" + GitLabLabel = "GitLab" + GithubLabel = "GitHub" + GrafanaComLabel = "grafana.com" + 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, autoProviderLabel string) bool { + // first check SAML, LDAP and JWT + switch autoProviderLabel { + case SAMLLabel: + return !cfg.SAMLSkipOrgRoleSync + case LDAPLabel: + return !cfg.LDAPSkipOrgRoleSync + case JWTLabel: + return !cfg.JWTAuthSkipOrgRoleSync + } + // then check the rest of the oauth providers + // FIXME: remove this once we remove the setting + // is a deprecated setting that is used to skip org role sync for all external oauth providers + if cfg.OAuthSkipOrgRoleUpdateSync { + return false + } + switch autoProviderLabel { + case GoogleLabel: + return !cfg.GoogleSkipOrgRoleSync + case OktaLabel: + return !cfg.OktaSkipOrgRoleSync + case AzureADLabel: + return !cfg.AzureADSkipOrgRoleSync + case GitLabLabel: + return !cfg.GitLabSkipOrgRoleSync + case GithubLabel: + return !cfg.GithubSkipOrgRoleSync + case GrafanaComLabel: + return !cfg.GrafanaComSkipOrgRoleSync + case GenericOAuthLabel: + return !cfg.GenericOAuthSkipOrgRoleSync + } + return true +} + func GetAuthProviderLabel(authModule string) string { switch authModule { - case "oauth_github": - return "GitHub" - case "oauth_google": - return "Google" - case "oauth_azuread": - return "AzureAD" - case "oauth_gitlab": - return "GitLab" - case "oauth_okta": - return "Okta" - case "oauth_grafana_com", "oauth_grafananet": - return "grafana.com" + case GithubAuthModule: + return GithubLabel + case GoogleAuthModule: + return GoogleLabel + case AzureADAuthModule: + return AzureADLabel + case GitLabAuthModule: + return GitLabLabel + case OktaAuthModule: + return OktaLabel + case GrafanaComAuthModule, GrafanaNetAuthModule: + return GrafanaComLabel case SAMLAuthModule: - return "SAML" + return SAMLLabel case LDAPAuthModule, "": // FIXME: verify this situation doesn't exist anymore - return "LDAP" + return LDAPLabel case JWTModule: - return "JWT" + return JWTLabel case AuthProxyAuthModule: - return "Auth Proxy" - case "oauth_generic_oauth": - return "Generic OAuth" + return AuthProxtLabel + case GenericOAuthModule: + return GenericOAuthLabel default: return "Unknown" } diff --git a/pkg/services/login/authinfo_test.go b/pkg/services/login/authinfo_test.go new file mode 100644 index 00000000000..45a1ff1c37e --- /dev/null +++ b/pkg/services/login/authinfo_test.go @@ -0,0 +1,202 @@ +package login + +import ( + "testing" + + "github.com/grafana/grafana/pkg/setting" + "github.com/stretchr/testify/assert" +) + +func TestIsExternallySynced(t *testing.T) { + testcases := []struct { + name string + cfg *setting.Cfg + provider string + expected bool + }{ + { + name: "AzureAD synced user should return that it is externally synced", + cfg: &setting.Cfg{AzureADSkipOrgRoleSync: false}, + provider: AzureADLabel, + expected: true, + }, + { + name: "AzureAD synced user should return that it is not externally synced when org role sync is set", + cfg: &setting.Cfg{AzureADSkipOrgRoleSync: true}, + provider: AzureADLabel, + expected: false, + }, + // FIXME: remove this test as soon as we remove the deprecated setting for skipping org role sync for all external oauth providers + { + name: "azuread external user should return that it is not externally synced when oauth org role sync is set", + cfg: &setting.Cfg{AzureADSkipOrgRoleSync: false, OAuthSkipOrgRoleUpdateSync: true}, + provider: AzureADLabel, + expected: false, + }, + { + name: "Google synced user should return that it is externally synced", + cfg: &setting.Cfg{GoogleSkipOrgRoleSync: false}, + provider: GoogleLabel, + expected: true, + }, + { + name: "Google synced user should return that it is not externally synced when org role sync is set", + cfg: &setting.Cfg{GoogleSkipOrgRoleSync: true}, + provider: GoogleLabel, + expected: false, + }, + // FIXME: remove this test as soon as we remove the deprecated setting for skipping org role sync for all external oauth providers + { + name: "google external user should return that it is not externally synced when oauth org role sync is set", + cfg: &setting.Cfg{GoogleSkipOrgRoleSync: false, OAuthSkipOrgRoleUpdateSync: true}, + provider: GoogleLabel, + expected: false, + }, + { + name: "external user should return that it is not externally synced when oauth org role sync is set and google skip org role sync set", + cfg: &setting.Cfg{GoogleSkipOrgRoleSync: true, OAuthSkipOrgRoleUpdateSync: true}, + provider: GoogleLabel, + expected: false, + }, + { + name: "Okta synced user should return that it is externally synced", + cfg: &setting.Cfg{OktaSkipOrgRoleSync: false}, + provider: OktaLabel, + expected: true, + }, + { + name: "Okta synced user should return that it is not externally synced when org role sync is set", + cfg: &setting.Cfg{OktaSkipOrgRoleSync: true}, + + provider: OktaLabel, + expected: false, + }, + // FIXME: remove this test as soon as we remove the deprecated setting for skipping org role sync for all external oauth providers + { + name: "okta external user should return that it is not externally synced when oauth org role sync is set", + cfg: &setting.Cfg{OktaSkipOrgRoleSync: false, OAuthSkipOrgRoleUpdateSync: true}, + provider: OktaLabel, + expected: false, + }, + { + name: "Github synced user should return that it is externally synced", + cfg: &setting.Cfg{GithubSkipOrgRoleSync: false}, + provider: GithubLabel, + expected: true, + }, + { + name: "Github synced user should return that it is not externally synced when org role sync is set", + cfg: &setting.Cfg{GithubSkipOrgRoleSync: true}, + provider: GithubLabel, + expected: false, + }, + // FIXME: remove this test as soon as we remove the deprecated setting for skipping org role sync for all external oauth providers + { + name: "github external user should return that it is not externally synced when oauth org role sync is set", + cfg: &setting.Cfg{GithubSkipOrgRoleSync: false, OAuthSkipOrgRoleUpdateSync: true}, + provider: GithubLabel, + expected: false, + }, + // gitlab + { + name: "Gitlab synced user should return that it is externally synced", + cfg: &setting.Cfg{GitLabSkipOrgRoleSync: false}, + provider: GitLabLabel, + expected: true, + }, + { + name: "Gitlab synced user should return that it is not externally synced when org role sync is set", + cfg: &setting.Cfg{GitLabSkipOrgRoleSync: true}, + provider: GitLabLabel, + expected: false, + }, + // FIXME: remove this test as soon as we remove the deprecated setting for skipping org role sync for all external oauth providers + { + name: "gitlab external user should return that it is not externally synced when oauth org role sync is set", + cfg: &setting.Cfg{GitLabSkipOrgRoleSync: false, OAuthSkipOrgRoleUpdateSync: true}, + provider: GitLabLabel, + expected: false, + }, + // grafana.com + { + name: "Grafana.com synced user should return that it is externally synced", + cfg: &setting.Cfg{GrafanaComSkipOrgRoleSync: false}, + provider: GrafanaComLabel, + expected: true, + }, + { + name: "Grafana.com synced user should return that it is not externally synced when org role sync is set", + cfg: &setting.Cfg{GrafanaComSkipOrgRoleSync: true}, + provider: GrafanaComLabel, + expected: false, + }, + // FIXME: remove this test as soon as we remove the deprecated setting for skipping org role sync for all external oauth providers + { + name: "grafanacom external user should return that it is not externally synced when oauth org role sync is set", + cfg: &setting.Cfg{GrafanaComSkipOrgRoleSync: false, OAuthSkipOrgRoleUpdateSync: true}, + provider: GrafanaComLabel, + expected: false, + }, + { + name: "Generic OAuth synced user should return that it is externally synced", + cfg: &setting.Cfg{OAuthSkipOrgRoleUpdateSync: false}, + provider: GenericOAuthLabel, + expected: true, + }, + { + name: "Generic OAuth synced user should return that it is not externally synced when org role sync is set", + cfg: &setting.Cfg{OAuthSkipOrgRoleUpdateSync: true}, + provider: GenericOAuthLabel, + expected: false, + }, + // FIXME: remove this test as soon as we remove the deprecated setting for skipping org role sync for all external oauth providers + { + name: "generic oauth external user should return that it is not externally synced when oauth org role sync is set", + cfg: &setting.Cfg{GenericOAuthSkipOrgRoleSync: false, OAuthSkipOrgRoleUpdateSync: true}, + provider: GenericOAuthLabel, + expected: false, + }, + { + name: "SAML synced user should return that it is externally synced", + cfg: &setting.Cfg{SAMLSkipOrgRoleSync: false}, + provider: SAMLLabel, + expected: true, + }, + { + name: "SAML synced user should return that it is not externally synced when org role sync is set", + cfg: &setting.Cfg{SAMLSkipOrgRoleSync: true}, + provider: SAMLLabel, + expected: false, + }, + { + name: "LDAP synced user should return that it is externally synced", + cfg: &setting.Cfg{LDAPSkipOrgRoleSync: false}, + provider: LDAPLabel, + expected: true, + }, + { + name: "LDAP synced user should return that it is not externally synced when org role sync is set", + cfg: &setting.Cfg{LDAPSkipOrgRoleSync: true}, + provider: LDAPLabel, + expected: false, + }, + { + name: "JWT synced user should return that it is externally synced", + cfg: &setting.Cfg{JWTAuthSkipOrgRoleSync: false}, + provider: JWTLabel, + expected: true, + }, + { + name: "JWT synced user should return that it is not externally synced when org role sync is set", + cfg: &setting.Cfg{JWTAuthSkipOrgRoleSync: true}, + provider: JWTLabel, + expected: false, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expected, IsExternallySynced(tc.cfg, tc.provider)) + }) + } +} diff --git a/pkg/services/login/authinfoservice/database/database.go b/pkg/services/login/authinfoservice/database/database.go index 1d4abbfe85b..afb5691fa9b 100644 --- a/pkg/services/login/authinfoservice/database/database.go +++ b/pkg/services/login/authinfoservice/database/database.go @@ -58,6 +58,8 @@ func (s *AuthInfoStore) GetExternalUserInfoByLogin(ctx context.Context, query *l return nil } +// GetAuthInfo returns the auth info for a user +// It will return the latest auth info for a user func (s *AuthInfoStore) GetAuthInfo(ctx context.Context, query *login.GetAuthInfoQuery) error { if query.UserId == 0 && query.AuthId == "" { return user.ErrUserNotFound diff --git a/pkg/services/user/model.go b/pkg/services/user/model.go index d2eb13b2a85..966e13a72d1 100644 --- a/pkg/services/user/model.go +++ b/pkg/services/user/model.go @@ -140,20 +140,21 @@ type GetUserProfileQuery struct { } type UserProfileDTO struct { - ID int64 `json:"id"` - Email string `json:"email"` - Name string `json:"name"` - Login string `json:"login"` - Theme string `json:"theme"` - OrgID int64 `json:"orgId,omitempty"` - IsGrafanaAdmin bool `json:"isGrafanaAdmin"` - IsDisabled bool `json:"isDisabled"` - IsExternal bool `json:"isExternal"` - AuthLabels []string `json:"authLabels"` - UpdatedAt time.Time `json:"updatedAt"` - CreatedAt time.Time `json:"createdAt"` - AvatarURL string `json:"avatarUrl"` - AccessControl map[string]bool `json:"accessControl,omitempty"` + ID int64 `json:"id"` + Email string `json:"email"` + Name string `json:"name"` + Login string `json:"login"` + Theme string `json:"theme"` + OrgID int64 `json:"orgId,omitempty"` + IsGrafanaAdmin bool `json:"isGrafanaAdmin"` + IsDisabled bool `json:"isDisabled"` + IsExternal bool `json:"isExternal"` + IsExternallySynced bool `json:"isExternallySynced"` + AuthLabels []string `json:"authLabels"` + UpdatedAt time.Time `json:"updatedAt"` + CreatedAt time.Time `json:"createdAt"` + AvatarURL string `json:"avatarUrl"` + AccessControl map[string]bool `json:"accessControl,omitempty"` } // implement Conversion interface to define custom field mapping (xorm feature) diff --git a/pkg/setting/setting.go b/pkg/setting/setting.go index 039d2b350d5..17e4bc3160c 100644 --- a/pkg/setting/setting.go +++ b/pkg/setting/setting.go @@ -505,6 +505,9 @@ type Cfg struct { SecureSocksDSProxy SecureSocksDSProxySettings + // SAML Auth + SAMLSkipOrgRoleSync bool + // Okta OAuth OktaSkipOrgRoleSync bool @@ -1095,6 +1098,7 @@ func (cfg *Cfg) Load(args CommandLineArgs) error { cfg.PluginsEnableAlpha = true } + cfg.readSAMLConfig() cfg.readLDAPConfig() cfg.handleAWSConfig() cfg.readAzureSettings() @@ -1194,6 +1198,11 @@ type RemoteCacheOptions struct { Encryption bool } +func (cfg *Cfg) readSAMLConfig() { + samlSec := cfg.Raw.Section("auth.saml") + cfg.SAMLSkipOrgRoleSync = samlSec.Key("skip_org_role_sync").MustBool(false) +} + func (cfg *Cfg) readLDAPConfig() { ldapSec := cfg.Raw.Section("auth.ldap") LDAPConfigFile = ldapSec.Key("config_file").String() diff --git a/public/app/features/admin/UserAdminPage.tsx b/public/app/features/admin/UserAdminPage.tsx index da70959ee2e..737cbd18d79 100644 --- a/public/app/features/admin/UserAdminPage.tsx +++ b/public/app/features/admin/UserAdminPage.tsx @@ -104,45 +104,9 @@ export class UserAdminPage extends PureComponent { render() { const { user, orgs, sessions, ldapSyncInfo, isLoading } = this.props; const isLDAPUser = user?.isExternal && user?.authLabels?.includes('LDAP'); - const isJWTUser = user?.authLabels?.includes('JWT'); const canReadSessions = contextSrv.hasPermission(AccessControlAction.UsersAuthTokenList); const canReadLDAPStatus = contextSrv.hasPermission(AccessControlAction.LDAPStatusRead); - const isSAMLUser = user?.isExternal && user?.authLabels?.includes('SAML'); - const isGoogleUser = user?.isExternal && user?.authLabels?.includes('Google'); - const isGithubUser = user?.isExternal && user?.authLabels?.includes('GitHub'); - const isGitLabUser = user?.isExternal && user?.authLabels?.includes('GitLab'); - const isAuthProxyUser = user?.isExternal && user?.authLabels?.includes('Auth Proxy'); - const isAzureADUser = user?.isExternal && user?.authLabels?.includes('AzureAD'); - const isOktaUser = user?.isExternal && user?.authLabels?.includes('Okta'); - const isGrafanaComUser = user?.isExternal && user?.authLabels?.includes('grafana.com'); - const isGenericOAuthUser = user?.isExternal && user?.authLabels?.includes('Generic OAuth'); - const isUserSynced = - !config.auth.DisableSyncLock && - ((user?.isExternal && - !( - isAuthProxyUser || - isGoogleUser || - isGitLabUser || - isGenericOAuthUser || - isSAMLUser || - isOktaUser || - isLDAPUser || - isGithubUser || - isAzureADUser || - isJWTUser || - isGrafanaComUser - )) || - (!config.auth.SAMLSkipOrgRoleSync && isSAMLUser) || - (!config.auth.LDAPSkipOrgRoleSync && isLDAPUser) || - (!config.auth.JWTAuthSkipOrgRoleSync && isJWTUser) || - // both OAuthSkipOrgRoleUpdateSync and specific provider settings needs to be false for a user to be synced - (!config.auth.OAuthSkipOrgRoleUpdateSync && !config.auth.GrafanaComSkipOrgRoleSync && isGrafanaComUser) || - (!config.auth.OAuthSkipOrgRoleUpdateSync && !config.auth.OktaSkipOrgRoleSync && isOktaUser) || - (!config.auth.OAuthSkipOrgRoleUpdateSync && !config.auth.GithubSkipOrgRoleSync && isGithubUser) || - (!config.auth.OAuthSkipOrgRoleUpdateSync && !config.auth.AzureADSkipOrgRoleSync && isAzureADUser) || - (!config.auth.OAuthSkipOrgRoleUpdateSync && !config.auth.GitLabSkipOrgRoleSync && isGitLabUser) || - (!config.auth.OAuthSkipOrgRoleUpdateSync && !config.auth.GenericOAuthSkipOrgRoleSync && isGenericOAuthUser) || - (!config.auth.OAuthSkipOrgRoleUpdateSync && !config.auth.GoogleSkipOrgRoleSync && isGoogleUser)); + const isUserSynced = !config.auth.DisableSyncLock && user?.isExternallySynced; const pageNav: NavModelItem = { text: user?.login ?? '', @@ -164,13 +128,9 @@ export class UserAdminPage extends PureComponent { onUserEnable={this.onUserEnable} onPasswordChange={this.onPasswordChange} /> - {!config.auth.LDAPSkipOrgRoleSync && - isLDAPUser && - featureEnabled('ldapsync') && - ldapSyncInfo && - canReadLDAPStatus && ( - - )} + {isLDAPUser && isUserSynced && featureEnabled('ldapsync') && ldapSyncInfo && canReadLDAPStatus && ( + + )} )} diff --git a/public/app/types/user.ts b/public/app/types/user.ts index 6177c311897..94fee64d417 100644 --- a/public/app/types/user.ts +++ b/public/app/types/user.ts @@ -46,6 +46,7 @@ export interface UserDTO extends WithAccessControlMetadata { permissions?: string[]; teams?: Unit[]; orgs?: Unit[]; + isExternallySynced?: boolean; } export interface Invitee {