From 207a55be663aad4b0e8c06817fc26653b5bd66f2 Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Wed, 22 Feb 2023 10:27:48 +0100 Subject: [PATCH] AuthN: add flag for org roles sync (#63507) * AuthN: Add flag to control org role syncs * JWT: Only sync org roles if the skip flag for jwt is false * LDAP: Only sync org role if skip flag for ldap is false * OAuth: Skip org roles sync if no roles were provided by upstream service * Grafana: Set SyncOrgRoles to true for authentication through proxy with grafana as backend --- pkg/services/authn/authn.go | 2 + pkg/services/authn/authnimpl/sync/org_sync.go | 2 +- .../authn/authnimpl/sync/org_sync_test.go | 4 +- pkg/services/authn/clients/grafana.go | 15 ++--- pkg/services/authn/clients/grafana_test.go | 9 +-- pkg/services/authn/clients/jwt.go | 57 ++++++++----------- pkg/services/authn/clients/jwt_test.go | 1 + pkg/services/authn/clients/ldap.go | 9 +-- pkg/services/authn/clients/ldap_test.go | 2 + pkg/services/authn/clients/oauth.go | 34 ++++------- pkg/services/authn/clients/oauth_test.go | 1 + pkg/services/authn/clients/utils.go | 30 ++++++++++ 12 files changed, 92 insertions(+), 74 deletions(-) create mode 100644 pkg/services/authn/clients/utils.go diff --git a/pkg/services/authn/authn.go b/pkg/services/authn/authn.go index 78de7ab075a..9c91eb9a34f 100644 --- a/pkg/services/authn/authn.go +++ b/pkg/services/authn/authn.go @@ -47,6 +47,8 @@ type ClientParams struct { FetchSyncedUser bool // SyncTeams will sync the groups from identity to teams in grafana, enterprise only feature SyncTeams bool + // SyncOrgRoles will sync the roles from the identity to orgs in grafana + SyncOrgRoles bool // CacheAuthProxyKey if this key is set we will try to cache the user id for proxy client CacheAuthProxyKey string // LookUpParams are the arguments used to look up the entity in the DB. diff --git a/pkg/services/authn/authnimpl/sync/org_sync.go b/pkg/services/authn/authnimpl/sync/org_sync.go index 47bbfc953ee..364167db3da 100644 --- a/pkg/services/authn/authnimpl/sync/org_sync.go +++ b/pkg/services/authn/authnimpl/sync/org_sync.go @@ -25,7 +25,7 @@ type OrgSync struct { } func (s *OrgSync) SyncOrgRolesHook(ctx context.Context, id *authn.Identity, _ *authn.Request) error { - if !id.ClientParams.SyncUser { + if !id.ClientParams.SyncOrgRoles { return nil } diff --git a/pkg/services/authn/authnimpl/sync/org_sync_test.go b/pkg/services/authn/authnimpl/sync/org_sync_test.go index 8c9852c0f6d..00bbbf5b1a3 100644 --- a/pkg/services/authn/authnimpl/sync/org_sync_test.go +++ b/pkg/services/authn/authnimpl/sync/org_sync_test.go @@ -79,7 +79,7 @@ func TestOrgSync_SyncOrgRolesHook(t *testing.T) { OrgRoles: map[int64]roletype.RoleType{1: org.RoleAdmin, 2: org.RoleEditor}, IsGrafanaAdmin: ptrBool(false), ClientParams: authn.ClientParams{ - SyncUser: true, + SyncOrgRoles: true, LookUpParams: login.UserLookupParams{ UserID: nil, Email: ptrString("test"), @@ -97,7 +97,7 @@ func TestOrgSync_SyncOrgRolesHook(t *testing.T) { OrgID: 1, //set using org IsGrafanaAdmin: ptrBool(false), ClientParams: authn.ClientParams{ - SyncUser: true, + SyncOrgRoles: true, LookUpParams: login.UserLookupParams{ UserID: nil, Email: ptrString("test"), diff --git a/pkg/services/authn/clients/grafana.go b/pkg/services/authn/clients/grafana.go index de52d5c1530..af77b62ae84 100644 --- a/pkg/services/authn/clients/grafana.go +++ b/pkg/services/authn/clients/grafana.go @@ -38,6 +38,7 @@ func (c *Grafana) AuthenticateProxy(ctx context.Context, r *authn.Request, usern SyncUser: true, SyncTeams: true, FetchSyncedUser: true, + SyncOrgRoles: true, AllowSignUp: c.cfg.AuthProxyAutoSignUp, }, } @@ -69,15 +70,11 @@ func (c *Grafana) AuthenticateProxy(ctx context.Context, r *authn.Request, usern } if v, ok := additional[proxyFieldRole]; ok { - role := org.RoleType(v) - if role.IsValid() { - orgID := int64(1) - if c.cfg.AutoAssignOrg && c.cfg.AutoAssignOrgId > 0 { - orgID = int64(c.cfg.AutoAssignOrgId) - } - identity.OrgID = orgID - identity.OrgRoles = map[int64]org.RoleType{orgID: role} - } + orgRoles, isGrafanaAdmin, _ := getRoles(c.cfg, func() (org.RoleType, *bool, error) { + return org.RoleType(v), nil, nil + }) + identity.OrgRoles = orgRoles + identity.IsGrafanaAdmin = isGrafanaAdmin } if v, ok := additional[proxyFieldGroups]; ok { diff --git a/pkg/services/authn/clients/grafana_test.go b/pkg/services/authn/clients/grafana_test.go index 55992829f4a..b33a00fd5a0 100644 --- a/pkg/services/authn/clients/grafana_test.go +++ b/pkg/services/authn/clients/grafana_test.go @@ -40,7 +40,6 @@ func TestGrafana_AuthenticateProxy(t *testing.T) { proxyFieldEmail: "email@email.com", }, expectedIdentity: &authn.Identity{ - OrgID: 1, OrgRoles: map[int64]org.RoleType{1: org.RoleViewer}, Login: "test", Name: "name", @@ -53,6 +52,7 @@ func TestGrafana_AuthenticateProxy(t *testing.T) { SyncTeams: true, AllowSignUp: true, FetchSyncedUser: true, + SyncOrgRoles: true, LookUpParams: login.UserLookupParams{ Email: strPtr("email@email.com"), Login: strPtr("test"), @@ -71,9 +71,10 @@ func TestGrafana_AuthenticateProxy(t *testing.T) { AuthModule: "authproxy", AuthID: "test@test.com", ClientParams: authn.ClientParams{ - SyncUser: true, - SyncTeams: true, - AllowSignUp: true, + SyncUser: true, + SyncTeams: true, + AllowSignUp: true, + SyncOrgRoles: true, LookUpParams: login.UserLookupParams{ Email: strPtr("test@test.com"), Login: strPtr("test@test.com"), diff --git a/pkg/services/authn/clients/jwt.go b/pkg/services/authn/clients/jwt.go index 9b7f6e654c4..8aa2c759367 100644 --- a/pkg/services/authn/clients/jwt.go +++ b/pkg/services/authn/clients/jwt.go @@ -10,7 +10,6 @@ import ( "github.com/jmespath/go-jmespath" "github.com/grafana/grafana/pkg/infra/log" - "github.com/grafana/grafana/pkg/models/roletype" "github.com/grafana/grafana/pkg/services/auth" authJWT "github.com/grafana/grafana/pkg/services/auth/jwt" "github.com/grafana/grafana/pkg/services/authn" @@ -23,11 +22,11 @@ import ( var _ authn.ContextAwareClient = new(JWT) var ( - ErrJWTInvalid = errutil.NewBase(errutil.StatusUnauthorized, + errJWTInvalid = errutil.NewBase(errutil.StatusUnauthorized, "jwt.invalid", errutil.WithPublicMessage("Failed to verify JWT")) - ErrJWTMissingClaim = errutil.NewBase(errutil.StatusUnauthorized, + errJWTMissingClaim = errutil.NewBase(errutil.StatusUnauthorized, "jwt.missing_claim", errutil.WithPublicMessage("Missing mandatory claim in JWT")) - ErrJWTInvalidRole = errutil.NewBase(errutil.StatusForbidden, + errJWTInvalidRole = errutil.NewBase(errutil.StatusForbidden, "jwt.invalid_role", errutil.WithPublicMessage("Invalid Role in claim")) ) @@ -55,13 +54,13 @@ func (s *JWT) Authenticate(ctx context.Context, r *authn.Request) (*authn.Identi claims, err := s.jwtService.Verify(ctx, jwtToken) if err != nil { s.log.Debug("Failed to verify JWT", "error", err) - return nil, ErrJWTInvalid.Errorf("failed to verify JWT: %w", err) + return nil, errJWTInvalid.Errorf("failed to verify JWT: %w", err) } sub, _ := claims["sub"].(string) if sub == "" { s.log.Warn("Got a JWT without the mandatory 'sub' claim", "error", err) - return nil, ErrJWTMissingClaim.Errorf("missing mandatory 'sub' claim in JWT") + return nil, errJWTMissingClaim.Errorf("missing mandatory 'sub' claim in JWT") } id := &authn.Identity{ @@ -71,6 +70,7 @@ func (s *JWT) Authenticate(ctx context.Context, r *authn.Request) (*authn.Identi ClientParams: authn.ClientParams{ SyncUser: true, FetchSyncedUser: true, + SyncOrgRoles: !s.cfg.JWTAuthSkipOrgRoleSync, AllowSignUp: s.cfg.JWTAuthAutoSignUp, }} @@ -87,41 +87,34 @@ func (s *JWT) Authenticate(ctx context.Context, r *authn.Request) (*authn.Identi id.Name = name } - var role roletype.RoleType - var grafanaAdmin bool - if !s.cfg.JWTAuthSkipOrgRoleSync { - role, grafanaAdmin = s.extractRoleAndAdmin(claims) + orgRoles, isGrafanaAdmin, err := getRoles(s.cfg, func() (org.RoleType, *bool, error) { + if s.cfg.JWTAuthSkipOrgRoleSync { + return "", nil, nil + } + + role, grafanaAdmin := s.extractRoleAndAdmin(claims) if s.cfg.JWTAuthRoleAttributeStrict && !role.IsValid() { - s.log.Warn("extracted Role is invalid", "role", role, "auth_id", id.AuthID) - return nil, ErrJWTInvalidRole.Errorf("invalid role claim in JWT: %s", role) + return "", nil, errJWTInvalidRole.Errorf("invalid role claim in JWT: %s", role) } - if role.IsValid() { - var orgID int64 - // FIXME (jguer): GetIDForNewUser already has the auto assign information - // just needs the org role. Find a meaningful way to pass this default - // role to it (that doesn't involve id.OrgRoles[0] = role) - if s.cfg.AutoAssignOrg && s.cfg.AutoAssignOrgId > 0 { - orgID = int64(s.cfg.AutoAssignOrgId) - s.log.Debug("The user has a role assignment and organization membership is auto-assigned", - "role", role, "orgId", orgID) - } else { - orgID = int64(1) - s.log.Debug("The user has a role assignment and organization membership is not auto-assigned", - "role", role, "orgId", orgID) - } - - id.OrgRoles[orgID] = role - if s.cfg.JWTAuthAllowAssignGrafanaAdmin { - id.IsGrafanaAdmin = &grafanaAdmin - } + if !s.cfg.JWTAuthAllowAssignGrafanaAdmin { + return role, nil, nil } + + return role, &grafanaAdmin, nil + }) + + if err != nil { + return nil, err } + id.OrgRoles = orgRoles + id.IsGrafanaAdmin = isGrafanaAdmin + if id.Login == "" && id.Email == "" { s.log.Debug("Failed to get an authentication claim from JWT", "login", id.Login, "email", id.Email) - return nil, ErrJWTMissingClaim.Errorf("missing login and email claim in JWT") + return nil, errJWTMissingClaim.Errorf("missing login and email claim in JWT") } return id, nil diff --git a/pkg/services/authn/clients/jwt_test.go b/pkg/services/authn/clients/jwt_test.go index 2fcac9b1b5b..6eb6d555a1c 100644 --- a/pkg/services/authn/clients/jwt_test.go +++ b/pkg/services/authn/clients/jwt_test.go @@ -52,6 +52,7 @@ func TestAuthenticateJWT(t *testing.T) { SyncUser: true, AllowSignUp: true, FetchSyncedUser: true, + SyncOrgRoles: true, LookUpParams: login.UserLookupParams{ UserID: nil, Email: stringPtr("eai.doe@cor.po"), diff --git a/pkg/services/authn/clients/ldap.go b/pkg/services/authn/clients/ldap.go index 66d05c977c9..c2e166bdfcd 100644 --- a/pkg/services/authn/clients/ldap.go +++ b/pkg/services/authn/clients/ldap.go @@ -41,7 +41,7 @@ func (c *LDAP) AuthenticateProxy(ctx context.Context, r *authn.Request, username return nil, err } - return identityFromLDAPInfo(r.OrgID, info, c.cfg.LDAPAllowSignup), nil + return c.identityFromLDAPInfo(r.OrgID, info), nil } func (c *LDAP) AuthenticatePassword(ctx context.Context, r *authn.Request, username, password string) (*authn.Identity, error) { @@ -66,10 +66,10 @@ func (c *LDAP) AuthenticatePassword(ctx context.Context, r *authn.Request, usern return nil, err } - return identityFromLDAPInfo(r.OrgID, info, c.cfg.LDAPAllowSignup), nil + return c.identityFromLDAPInfo(r.OrgID, info), nil } -func identityFromLDAPInfo(orgID int64, info *login.ExternalUserInfo, allowSignup bool) *authn.Identity { +func (c *LDAP) identityFromLDAPInfo(orgID int64, info *login.ExternalUserInfo) *authn.Identity { return &authn.Identity{ OrgID: orgID, OrgRoles: info.OrgRoles, @@ -85,7 +85,8 @@ func identityFromLDAPInfo(orgID int64, info *login.ExternalUserInfo, allowSignup SyncTeams: true, EnableDisabledUsers: true, FetchSyncedUser: true, - AllowSignUp: allowSignup, + SyncOrgRoles: !c.cfg.LDAPSkipOrgRoleSync, + AllowSignUp: c.cfg.LDAPAllowSignup, LookUpParams: login.UserLookupParams{ Login: &info.Login, Email: &info.Email, diff --git a/pkg/services/authn/clients/ldap_test.go b/pkg/services/authn/clients/ldap_test.go index 9e174bcf5da..0f7a0ace532 100644 --- a/pkg/services/authn/clients/ldap_test.go +++ b/pkg/services/authn/clients/ldap_test.go @@ -52,6 +52,7 @@ func TestLDAP_AuthenticateProxy(t *testing.T) { SyncTeams: true, EnableDisabledUsers: true, FetchSyncedUser: true, + SyncOrgRoles: true, LookUpParams: login.UserLookupParams{ Email: strPtr("test@test.com"), Login: strPtr("test"), @@ -116,6 +117,7 @@ func TestLDAP_AuthenticatePassword(t *testing.T) { SyncTeams: true, EnableDisabledUsers: true, FetchSyncedUser: true, + SyncOrgRoles: true, LookUpParams: login.UserLookupParams{ Email: strPtr("test@test.com"), Login: strPtr("test"), diff --git a/pkg/services/authn/clients/oauth.go b/pkg/services/authn/clients/oauth.go index 0ae3549dc30..480d4b82d14 100644 --- a/pkg/services/authn/clients/oauth.go +++ b/pkg/services/authn/clients/oauth.go @@ -123,22 +123,31 @@ func (c *OAuth) Authenticate(ctx context.Context, r *authn.Request) (*authn.Iden return nil, errOAuthEmailNotAllowed.Errorf("provided email is not allowed") } + orgRoles, isGrafanaAdmin, _ := getRoles(c.cfg, func() (org.RoleType, *bool, error) { + if c.cfg.OAuthSkipOrgRoleUpdateSync { + return "", nil, nil + } + return userInfo.Role, userInfo.IsGrafanaAdmin, nil + }) + return &authn.Identity{ Login: userInfo.Login, Name: userInfo.Name, Email: userInfo.Email, - IsGrafanaAdmin: userInfo.IsGrafanaAdmin, + IsGrafanaAdmin: isGrafanaAdmin, AuthModule: c.moduleName, AuthID: userInfo.Id, Groups: userInfo.Groups, OAuthToken: token, - OrgRoles: getOAuthOrgRole(userInfo, c.cfg), + OrgRoles: orgRoles, ClientParams: authn.ClientParams{ SyncUser: true, SyncTeams: true, FetchSyncedUser: true, AllowSignUp: c.connector.IsSignupAllowed(), - LookUpParams: login.UserLookupParams{Email: &userInfo.Email}, + // skip org role flag is checked and handled in the connector. For now we can skip the hook if no roles are passed + SyncOrgRoles: len(orgRoles) > 0, + LookUpParams: login.UserLookupParams{Email: &userInfo.Email}, }, }, nil } @@ -217,22 +226,3 @@ func hashOAuthState(state, secret, seed string) string { hashBytes := sha256.Sum256([]byte(state + secret + seed)) return hex.EncodeToString(hashBytes[:]) } - -func getOAuthOrgRole(userInfo *social.BasicUserInfo, cfg *setting.Cfg) map[int64]org.RoleType { - orgRoles := make(map[int64]org.RoleType, 0) - if cfg.OAuthSkipOrgRoleUpdateSync { - return orgRoles - } - - if userInfo.Role == "" || !userInfo.Role.IsValid() { - return orgRoles - } - - orgID := int64(1) - if cfg.AutoAssignOrg && cfg.AutoAssignOrgId > 0 { - orgID = int64(cfg.AutoAssignOrgId) - } - - orgRoles[orgID] = userInfo.Role - return orgRoles -} diff --git a/pkg/services/authn/clients/oauth_test.go b/pkg/services/authn/clients/oauth_test.go index db8f8d4b866..4999e5d0293 100644 --- a/pkg/services/authn/clients/oauth_test.go +++ b/pkg/services/authn/clients/oauth_test.go @@ -139,6 +139,7 @@ func TestOAuth_Authenticate(t *testing.T) { SyncTeams: true, AllowSignUp: true, FetchSyncedUser: true, + SyncOrgRoles: true, LookUpParams: login.UserLookupParams{Email: strPtr("some@email.com")}, }, }, diff --git a/pkg/services/authn/clients/utils.go b/pkg/services/authn/clients/utils.go new file mode 100644 index 00000000000..8941dec502d --- /dev/null +++ b/pkg/services/authn/clients/utils.go @@ -0,0 +1,30 @@ +package clients + +import ( + "github.com/grafana/grafana/pkg/services/org" + "github.com/grafana/grafana/pkg/setting" +) + +// roleExtractor should return the org role, optional isGrafanaAdmin or an error +type roleExtractor func() (org.RoleType, *bool, error) + +// getRoles only handles one org role for now, could be subject to change +func getRoles(cfg *setting.Cfg, extract roleExtractor) (map[int64]org.RoleType, *bool, error) { + role, isGrafanaAdmin, err := extract() + orgRoles := make(map[int64]org.RoleType, 0) + if err != nil { + return orgRoles, nil, err + } + + if role == "" || !role.IsValid() { + return orgRoles, nil, nil + } + + orgID := int64(1) + if cfg.AutoAssignOrg && cfg.AutoAssignOrgId > 0 { + orgID = int64(cfg.AutoAssignOrgId) + } + orgRoles[orgID] = role + + return orgRoles, isGrafanaAdmin, nil +}