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 <ieva.vasiljeva@grafana.com>

* Update pkg/services/login/authinfo.go

Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>

* fix: tests

---------

Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>

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,

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))

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":
+	case GithubAuthModule:
-		return "GitHub"
+		return GithubLabel
-	case "oauth_google":
+	case GoogleAuthModule:
-		return "Google"
+		return GoogleLabel
-	case "oauth_azuread":
+	case AzureADAuthModule:
-		return "AzureAD"
+		return AzureADLabel
-	case "oauth_gitlab":
+	case GitLabAuthModule:
-		return "GitLab"
+		return GitLabLabel
-	case "oauth_okta":
+	case OktaAuthModule:
-		return "Okta"
+		return OktaLabel
-	case "oauth_grafana_com", "oauth_grafananet":
+	case GrafanaComAuthModule, GrafanaNetAuthModule:
-		return "grafana.com"
+		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"
+		return AuthProxtLabel
-	case "oauth_generic_oauth":
+	case GenericOAuthModule:
-		return "Generic OAuth"
+		return GenericOAuthLabel
 	default:
 		return "Unknown"
 	}

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))
+		})
+	}
+}

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

pkg/services/user/model.go

@@ -140,20 +140,21 @@ type GetUserProfileQuery struct {
 }
 
 type UserProfileDTO struct {
-	ID             int64           `json:"id"`
+	ID                 int64           `json:"id"`
-	Email          string          `json:"email"`
+	Email              string          `json:"email"`
-	Name           string          `json:"name"`
+	Name               string          `json:"name"`
-	Login          string          `json:"login"`
+	Login              string          `json:"login"`
-	Theme          string          `json:"theme"`
+	Theme              string          `json:"theme"`
-	OrgID          int64           `json:"orgId,omitempty"`
+	OrgID              int64           `json:"orgId,omitempty"`
-	IsGrafanaAdmin bool            `json:"isGrafanaAdmin"`
+	IsGrafanaAdmin     bool            `json:"isGrafanaAdmin"`
-	IsDisabled     bool            `json:"isDisabled"`
+	IsDisabled         bool            `json:"isDisabled"`
-	IsExternal     bool            `json:"isExternal"`
+	IsExternal         bool            `json:"isExternal"`
-	AuthLabels     []string        `json:"authLabels"`
+	IsExternallySynced bool            `json:"isExternallySynced"`
-	UpdatedAt      time.Time       `json:"updatedAt"`
+	AuthLabels         []string        `json:"authLabels"`
-	CreatedAt      time.Time       `json:"createdAt"`
+	UpdatedAt          time.Time       `json:"updatedAt"`
-	AvatarURL      string          `json:"avatarUrl"`
+	CreatedAt          time.Time       `json:"createdAt"`
-	AccessControl  map[string]bool `json:"accessControl,omitempty"`
+	AvatarURL          string          `json:"avatarUrl"`
+	AccessControl      map[string]bool `json:"accessControl,omitempty"`
 }
 
 // implement Conversion interface to define custom field mapping (xorm feature)

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()

public/app/features/admin/UserAdminPage.tsx

@@ -104,45 +104,9 @@ export class UserAdminPage extends PureComponent<Props> {
   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 isUserSynced = !config.auth.DisableSyncLock && user?.isExternallySynced;
-    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 pageNav: NavModelItem = {
       text: user?.login ?? '',
@@ -164,13 +128,9 @@ export class UserAdminPage extends PureComponent<Props> {
                 onUserEnable={this.onUserEnable}
                 onPasswordChange={this.onPasswordChange}
               />
-              {!config.auth.LDAPSkipOrgRoleSync &&
+              {isLDAPUser && isUserSynced && featureEnabled('ldapsync') && ldapSyncInfo && canReadLDAPStatus && (
-                isLDAPUser &&
+                <UserLdapSyncInfo ldapSyncInfo={ldapSyncInfo} user={user} onUserSync={this.onUserSync} />
-                featureEnabled('ldapsync') &&
+              )}
-                ldapSyncInfo &&
-                canReadLDAPStatus && (
-                  <UserLdapSyncInfo ldapSyncInfo={ldapSyncInfo} user={user} onUserSync={this.onUserSync} />
-                )}
               <UserPermissions isGrafanaAdmin={user.isGrafanaAdmin} onGrafanaAdminChange={this.onGrafanaAdminChange} />
             </>
           )}

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 {