[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 374bd5bec7)

Co-authored-by: linoman <2051016+linoman@users.noreply.github.com>

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)

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 {
+func (s *UserSync) ValidateUserProvisioningHook(ctx context.Context, currentIdentity *authn.Identity, _ *authn.Request) error {
-	log := s.log.FromContext(ctx).New("auth_module", id.AuthenticatedBy, "auth_id", id.AuthID)
+	log := s.log.FromContext(ctx).New("auth_module", currentIdentity.AuthenticatedBy, "auth_id", currentIdentity.AuthID)
+
+	if !currentIdentity.ClientParams.SyncUser {
+		log.Debug("Skipping user provisioning validation, syncUser is disabled")
+		return nil
+	}
 
 	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.skipProvisioningValidation(ctx, currentIdentity) {
-	if !s.isUserProvisioningEnabled {
+		log.Debug("Skipping user provisioning validation")
-		log.Debug("User provisioning is disabled, skipping validation")
-		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
-	}
-
-	// Skip validation if the auth module is GrafanaComAuthModule
-	if id.AuthenticatedBy == login.GrafanaComAuthModule {
-		log.Debug("User is authenticated via GrafanaComAuthModule, skipping 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.ExternalUID == "" || authInfo.ExternalUID != currentIdentity.ExternalUID {
-		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 {
 			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")

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: ""},
+			identity: &authn.Identity{
-			expectedErr: errUserExternalUIDMismatch,
+				AuthenticatedBy: login.SAMLAuthModule,
-		},
+				AuthID:          "1",
-		{
+				ClientParams: authn.ClientParams{
-			desc: "ValidateProvisioning: DB ExternalUID non-empty, Incoming ExternalUID is empty - expect mismatch",
+					SyncUser: true,
-			userSyncServiceSetup: func() *UserSync {
+				},
-				userSyncService := initUserSyncService()
+				ExternalUID: "",
-				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", 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"),
 		},