From 94ee07eebfa6ccb2bc02a2b1163e2cc31334a10f Mon Sep 17 00:00:00 2001 From: "grafana-delivery-bot[bot]" <132647405+grafana-delivery-bot[bot]@users.noreply.github.com> Date: Tue, 3 Jun 2025 12:48:42 +0200 Subject: [PATCH] [release-12.0.2] SCIM: Change SCIM hook registration (#106255) SCIM: Change SCIM hook registration (#106200) * Add function to skip provisioning hook * Rework provisioning hook to PostLoginHook * Revert back to PostAuthHook and remove unused tests * Fix tests (cherry picked from commit 374bd5bec7244866c6447f8355a12de03ca75e65) Co-authored-by: linoman <2051016+linoman@users.noreply.github.com> --- pkg/services/authn/authnimpl/registration.go | 5 +- .../authn/authnimpl/sync/user_sync.go | 57 +++++---- .../authn/authnimpl/sync/user_sync_test.go | 111 +++++++++++------- 3 files changed, 104 insertions(+), 69 deletions(-) diff --git a/pkg/services/authn/authnimpl/registration.go b/pkg/services/authn/authnimpl/registration.go index 13dde1f6280..7231889c72c 100644 --- a/pkg/services/authn/authnimpl/registration.go +++ b/pkg/services/authn/authnimpl/registration.go @@ -135,12 +135,15 @@ func ProvideRegistration( orgSync := sync.ProvideOrgSync(userService, orgService, accessControlService, cfg, tracer) authnSvc.RegisterPostAuthHook(userSync.SyncUserHook, 10) authnSvc.RegisterPostAuthHook(userSync.EnableUserHook, 20) - authnSvc.RegisterPostAuthHook(userSync.ValidateUserProvisioningHook, 30) authnSvc.RegisterPostAuthHook(orgSync.SyncOrgRolesHook, 40) authnSvc.RegisterPostAuthHook(userSync.SyncLastSeenHook, 130) authnSvc.RegisterPostAuthHook(sync.ProvideOAuthTokenSync(oauthTokenService, sessionService, socialService, tracer, features).SyncOauthTokenHook, 60) authnSvc.RegisterPostAuthHook(userSync.FetchSyncedUserHook, 100) + if features.IsEnabledGlobally(featuremgmt.FlagEnableSCIM) { + authnSvc.RegisterPostAuthHook(userSync.ValidateUserProvisioningHook, 30) + } + rbacSync := sync.ProvideRBACSync(accessControlService, tracer, permRegistry) if features.IsEnabledGlobally(featuremgmt.FlagCloudRBACRoles) { authnSvc.RegisterPostAuthHook(rbacSync.SyncCloudRoles, 110) diff --git a/pkg/services/authn/authnimpl/sync/user_sync.go b/pkg/services/authn/authnimpl/sync/user_sync.go index 3d092bb968d..a526de60ffd 100644 --- a/pkg/services/authn/authnimpl/sync/user_sync.go +++ b/pkg/services/authn/authnimpl/sync/user_sync.go @@ -111,28 +111,20 @@ type UserSync struct { } // ValidateUserProvisioningHook validates if a user should be allowed access based on provisioning status and configuration -func (s *UserSync) ValidateUserProvisioningHook(ctx context.Context, id *authn.Identity, _ *authn.Request) error { - log := s.log.FromContext(ctx).New("auth_module", id.AuthenticatedBy, "auth_id", id.AuthID) +func (s *UserSync) ValidateUserProvisioningHook(ctx context.Context, currentIdentity *authn.Identity, _ *authn.Request) error { + log := s.log.FromContext(ctx).New("auth_module", currentIdentity.AuthenticatedBy, "auth_id", currentIdentity.AuthID) - log.Debug("Validating user provisioning") - ctx, span := s.tracer.Start(ctx, "user.sync.ValidateUserProvisioningHook") - defer span.End() - - // Skip validation if user provisioning is disabled - if !s.isUserProvisioningEnabled { - log.Debug("User provisioning is disabled, skipping validation") + if !currentIdentity.ClientParams.SyncUser { + log.Debug("Skipping user provisioning validation, syncUser is disabled") return nil } - // Skip validation if non-provisioned users are allowed - if s.allowNonProvisionedUsers { - log.Debug("User provisioning is enabled, but non-provisioned users are allowed, skipping validation") - return nil - } + log.Debug("Validating user provisioning") + ctx, span := s.tracer.Start(ctx, "user.sync.ValidateUserProvisioningHook") + defer span.End() - // Skip validation if the auth module is GrafanaComAuthModule - if id.AuthenticatedBy == login.GrafanaComAuthModule { - log.Debug("User is authenticated via GrafanaComAuthModule, skipping validation") + if s.skipProvisioningValidation(ctx, currentIdentity) { + log.Debug("Skipping user provisioning validation") return nil } @@ -140,7 +132,7 @@ func (s *UserSync) ValidateUserProvisioningHook(ctx context.Context, id *authn.I // we must validate the authinfo.ExternalUID with the identity.ExternalUID // Retrieve user and authinfo from database - usr, authInfo, err := s.getUser(ctx, id) + usr, authInfo, err := s.getUser(ctx, currentIdentity) if err != nil { if errors.Is(err, user.ErrUserNotFound) { return nil @@ -154,14 +146,8 @@ func (s *UserSync) ValidateUserProvisioningHook(ctx context.Context, id *authn.I return errUnableToRetrieveUser.Errorf("unable to retrieve user for validation") } - // Validate the provisioned user.ExternalUID with the authinfo.ExternalUID if usr.IsProvisioned { - // Allow non-SAML requests for SAML-provisioned users to proceed if incoming ExternalUID is empty (e.g. session access). - if authInfo.AuthModule == login.SAMLAuthModule && id.AuthenticatedBy != login.SAMLAuthModule && authInfo.ExternalUID != "" && id.ExternalUID == "" { - log.Debug("Skipping ExternalUID validation for non-SAML request to SAML-provisioned user") - return nil - } - if authInfo.ExternalUID == "" || authInfo.ExternalUID != id.ExternalUID { + if authInfo.ExternalUID == "" || authInfo.ExternalUID != currentIdentity.ExternalUID { log.Error("The provisioned user.ExternalUID does not match the authinfo.ExternalUID") return errUserExternalUIDMismatch.Errorf("the provisioned user.ExternalUID does not match the authinfo.ExternalUID") } @@ -174,6 +160,27 @@ func (s *UserSync) ValidateUserProvisioningHook(ctx context.Context, id *authn.I return errUserNotProvisioned.Errorf("user is not provisioned") } +func (s *UserSync) skipProvisioningValidation(ctx context.Context, currentIdentity *authn.Identity) bool { + log := s.log.FromContext(ctx).New("auth_module", currentIdentity.AuthenticatedBy, "auth_id", currentIdentity.AuthID, "id", currentIdentity.ID) + + if !s.isUserProvisioningEnabled { + log.Debug("User provisioning is disabled, skipping validation") + return true + } + + if s.allowNonProvisionedUsers { + log.Debug("Non-provisioned users are allowed, skipping validation") + return true + } + + if currentIdentity.AuthenticatedBy == login.GrafanaComAuthModule { + log.Debug("User is authenticated via GrafanaComAuthModule, skipping validation") + return true + } + + return false +} + // SyncUserHook syncs a user with the database func (s *UserSync) SyncUserHook(ctx context.Context, id *authn.Identity, _ *authn.Request) error { ctx, span := s.tracer.Start(ctx, "user.sync.SyncUserHook") diff --git a/pkg/services/authn/authnimpl/sync/user_sync_test.go b/pkg/services/authn/authnimpl/sync/user_sync_test.go index 6f565a974fc..b28bbe2ca7c 100644 --- a/pkg/services/authn/authnimpl/sync/user_sync_test.go +++ b/pkg/services/authn/authnimpl/sync/user_sync_test.go @@ -691,6 +691,21 @@ func TestUserSync_ValidateUserProvisioningHook(t *testing.T) { } tests := []testCase{ + { + desc: "it should skip validation if the user identity is not syncying a user", + userSyncServiceSetup: func() *UserSync { + userSyncService := initUserSyncService() + userSyncService.isUserProvisioningEnabled = true + return userSyncService + }, + identity: &authn.Identity{ + ID: "1", + Type: claims.TypeAPIKey, + ClientParams: authn.ClientParams{ + SyncUser: false, + }, + }, + }, { desc: "it should skip validation if the user provisioning is disabled", userSyncServiceSetup: func() *UserSync { @@ -701,6 +716,9 @@ func TestUserSync_ValidateUserProvisioningHook(t *testing.T) { identity: &authn.Identity{ AuthenticatedBy: login.GenericOAuthModule, AuthID: "1", + ClientParams: authn.ClientParams{ + SyncUser: true, + }, }, }, { @@ -714,6 +732,9 @@ func TestUserSync_ValidateUserProvisioningHook(t *testing.T) { identity: &authn.Identity{ AuthenticatedBy: login.GenericOAuthModule, AuthID: "1", + ClientParams: authn.ClientParams{ + SyncUser: true, + }, }, }, { @@ -727,6 +748,9 @@ func TestUserSync_ValidateUserProvisioningHook(t *testing.T) { identity: &authn.Identity{ AuthenticatedBy: login.GrafanaComAuthModule, AuthID: "1", + ClientParams: authn.ClientParams{ + SyncUser: true, + }, }, }, { @@ -744,6 +768,9 @@ func TestUserSync_ValidateUserProvisioningHook(t *testing.T) { AuthenticatedBy: login.SAMLAuthModule, AuthID: "1", ExternalUID: "random-external-uid", + ClientParams: authn.ClientParams{ + SyncUser: true, + }, }, expectedErr: errUnableToRetrieveUserOrAuthInfo.Errorf("unable to retrieve user or authInfo for validation"), }, @@ -760,6 +787,9 @@ func TestUserSync_ValidateUserProvisioningHook(t *testing.T) { AuthenticatedBy: login.SAMLAuthModule, AuthID: "1", ExternalUID: "random-external-uid", + ClientParams: authn.ClientParams{ + SyncUser: true, + }, }, expectedErr: errUnableToRetrieveUser.Errorf("unable to retrieve user for validation"), }, @@ -788,6 +818,9 @@ func TestUserSync_ValidateUserProvisioningHook(t *testing.T) { AuthenticatedBy: login.SAMLAuthModule, AuthID: "1", ExternalUID: "random-external-uid", + ClientParams: authn.ClientParams{ + SyncUser: true, + }, }, expectedErr: errUserExternalUIDMismatch.Errorf("the provisioned user.ExternalUID does not match the authinfo.ExternalUID"), }, @@ -817,6 +850,9 @@ func TestUserSync_ValidateUserProvisioningHook(t *testing.T) { AuthenticatedBy: login.SAMLAuthModule, AuthID: "1", ExternalUID: "random-external-uid", + ClientParams: authn.ClientParams{ + SyncUser: true, + }, }, expectedErr: errUserExternalUIDMismatch.Errorf("the provisioned user.ExternalUID does not match the authinfo.ExternalUID"), }, @@ -846,6 +882,9 @@ func TestUserSync_ValidateUserProvisioningHook(t *testing.T) { AuthenticatedBy: login.SAMLAuthModule, AuthID: "1", ExternalUID: "random-external-uid", + ClientParams: authn.ClientParams{ + SyncUser: true, + }, }, }, { @@ -874,38 +913,12 @@ func TestUserSync_ValidateUserProvisioningHook(t *testing.T) { AuthenticatedBy: login.SAMLAuthModule, AuthID: "1", ExternalUID: "random-external-uid", + ClientParams: authn.ClientParams{ + SyncUser: true, + }, }, expectedErr: errUserNotProvisioned.Errorf("user is not provisioned"), }, - { - desc: "it should skip validation if identity is incomplete because it's not from the SAML auth flow", - userSyncServiceSetup: func() *UserSync { - userSyncService := initUserSyncService() - userSyncService.allowNonProvisionedUsers = false - userSyncService.isUserProvisioningEnabled = true - userSyncService.userService = &usertest.FakeUserService{ - ExpectedUser: &user.User{ - ID: 1, - IsProvisioned: true, - }, - } - userSyncService.authInfoService = &authinfotest.FakeService{ - ExpectedUserAuth: &login.UserAuth{ - UserId: 1, - AuthModule: login.SAMLAuthModule, - AuthId: "1", - ExternalUID: "random-external-uid", - }, - } - return userSyncService - }, - identity: &authn.Identity{ - AuthenticatedBy: login.GenericOAuthModule, - AuthID: "1", - ExternalUID: "", - }, - expectedErr: nil, - }, { desc: "ValidateProvisioning: DB ExternalUID is empty, Incoming ExternalUID is empty - expect mismatch (stricter logic)", userSyncServiceSetup: func() *UserSync { @@ -915,19 +928,14 @@ func TestUserSync_ValidateUserProvisioningHook(t *testing.T) { userSyncService.authInfoService = &authinfotest.FakeService{ExpectedUserAuth: &login.UserAuth{UserId: 1, AuthModule: login.SAMLAuthModule, ExternalUID: ""}} return userSyncService }, - identity: &authn.Identity{AuthenticatedBy: login.SAMLAuthModule, AuthID: "1", ExternalUID: ""}, - expectedErr: errUserExternalUIDMismatch, - }, - { - desc: "ValidateProvisioning: DB ExternalUID non-empty, Incoming ExternalUID is empty - expect mismatch", - userSyncServiceSetup: func() *UserSync { - userSyncService := initUserSyncService() - userSyncService.isUserProvisioningEnabled = true - userSyncService.userService = &usertest.FakeUserService{ExpectedUser: &user.User{ID: 1, IsProvisioned: true}} - userSyncService.authInfoService = &authinfotest.FakeService{ExpectedUserAuth: &login.UserAuth{UserId: 1, AuthModule: login.SAMLAuthModule, ExternalUID: "valid-uid"}} - return userSyncService + identity: &authn.Identity{ + AuthenticatedBy: login.SAMLAuthModule, + AuthID: "1", + ClientParams: authn.ClientParams{ + SyncUser: true, + }, + ExternalUID: "", }, - identity: &authn.Identity{AuthenticatedBy: login.SAMLAuthModule, AuthID: "1", ExternalUID: ""}, expectedErr: errUserExternalUIDMismatch, }, { @@ -939,7 +947,14 @@ func TestUserSync_ValidateUserProvisioningHook(t *testing.T) { userSyncService.authInfoService = &authinfotest.FakeService{ExpectedUserAuth: &login.UserAuth{UserId: 1, AuthModule: login.SAMLAuthModule, ExternalUID: ""}} return userSyncService }, - identity: &authn.Identity{AuthenticatedBy: login.SAMLAuthModule, AuthID: "1", ExternalUID: "valid-uid"}, + identity: &authn.Identity{ + AuthenticatedBy: login.SAMLAuthModule, + AuthID: "1", + ClientParams: authn.ClientParams{ + SyncUser: true, + }, + ExternalUID: "valid-uid", + }, expectedErr: errUserExternalUIDMismatch, }, { @@ -951,7 +966,14 @@ func TestUserSync_ValidateUserProvisioningHook(t *testing.T) { userSyncService.authInfoService = &authinfotest.FakeService{ExpectedUserAuth: &login.UserAuth{UserId: 1, AuthModule: login.SAMLAuthModule, ExternalUID: "db-uid"}} return userSyncService }, - identity: &authn.Identity{AuthenticatedBy: login.SAMLAuthModule, AuthID: "1", ExternalUID: "incoming-uid"}, + identity: &authn.Identity{ + AuthenticatedBy: login.SAMLAuthModule, + AuthID: "1", + ClientParams: authn.ClientParams{ + SyncUser: true, + }, + ExternalUID: "incoming-uid", + }, expectedErr: errUserExternalUIDMismatch, }, { @@ -1009,6 +1031,9 @@ func TestUserSync_ValidateUserProvisioningHook(t *testing.T) { AuthenticatedBy: login.SAMLAuthModule, AuthID: "1", ExternalUID: "", + ClientParams: authn.ClientParams{ + SyncUser: true, + }, }, expectedErr: errUserExternalUIDMismatch.Errorf("the provisioned user.ExternalUID does not match the authinfo.ExternalUID"), },