diff --git a/pkg/services/authn/authnimpl/sync/org_sync.go b/pkg/services/authn/authnimpl/sync/org_sync.go index f87cd1c7ca0..8742df614ff 100644 --- a/pkg/services/authn/authnimpl/sync/org_sync.go +++ b/pkg/services/authn/authnimpl/sync/org_sync.go @@ -50,16 +50,6 @@ func (s *OrgSync) SyncOrgRolesHook(ctx context.Context, id *authn.Identity, _ *a return nil } - // ignore org syncing if the user is provisioned - usr, err := s.userService.GetByID(ctx, &user.GetUserByIDQuery{ID: userID}) - if err != nil { - ctxLogger.Error("Failed to get user from provided identity", "error", err) - return nil - } - if usr.IsProvisioned { - return nil - } - ctxLogger.Debug("Syncing organization roles", "extOrgRoles", id.OrgRoles) // don't sync org roles if none is specified if len(id.OrgRoles) == 0 { diff --git a/pkg/services/authn/authnimpl/sync/user_sync.go b/pkg/services/authn/authnimpl/sync/user_sync.go index a526de60ffd..ebbb6500206 100644 --- a/pkg/services/authn/authnimpl/sync/user_sync.go +++ b/pkg/services/authn/authnimpl/sync/user_sync.go @@ -418,25 +418,52 @@ func (s *UserSync) updateUserAttributes(ctx context.Context, usr *user.User, id needsConnectionCreation = false authInfo, err := s.authInfoService.GetAuthInfo(ctx, &login.GetAuthInfoQuery{UserId: usr.ID, AuthModule: id.AuthenticatedBy}) if err != nil { - s.log.Error("Error getting auth info", "error", err) + s.log.Error("Error getting auth info for provisioned user", "error", err) return err } if id.ExternalUID == "" { - s.log.Error("externalUID is empty", "id", id.UID) + s.log.Error("externalUID is empty for provisioned user", "id", id.UID) return errEmptyExternalUID.Errorf("externalUID is empty") } if id.ExternalUID != authInfo.ExternalUID { - s.log.Error("mismatched externalUID", "provisioned_externalUID", authInfo.ExternalUID, "identity_externalUID", id.ExternalUID) - return errMismatchedExternalUID.Errorf("externalUID mistmatch") + s.log.Error("mismatched externalUID for provisioned user", "provisioned_externalUID", authInfo.ExternalUID, "identity_externalUID", id.ExternalUID) + return errMismatchedExternalUID.Errorf("externalUID mismatch") } } - if needsUpdate && !usr.IsProvisioned { - s.log.FromContext(ctx).Debug("Syncing user info", "id", id.ID, "update", fmt.Sprintf("%v", updateCmd)) - if err := s.userService.Update(ctx, updateCmd); err != nil { - return err + if needsUpdate { + finalCmdToExecute := &user.UpdateUserCommand{UserID: usr.ID} + shouldExecuteUpdate := false + + if !usr.IsProvisioned { + finalCmdToExecute = updateCmd + shouldExecuteUpdate = true + s.log.FromContext(ctx).Debug("Syncing all differing attributes for non-provisioned user", "id", id.ID, + "login", finalCmdToExecute.Login, "email", finalCmdToExecute.Email, "name", finalCmdToExecute.Name, + "isGrafanaAdmin", finalCmdToExecute.IsGrafanaAdmin, "emailVerified", finalCmdToExecute.EmailVerified) + } else { + if updateCmd.IsGrafanaAdmin != nil { + finalCmdToExecute.IsGrafanaAdmin = updateCmd.IsGrafanaAdmin + shouldExecuteUpdate = true + s.log.FromContext(ctx).Debug("Syncing IsGrafanaAdmin for provisioned user", "id", id.ID, "isAdmin", fmt.Sprintf("%v", *updateCmd.IsGrafanaAdmin)) + } + + if !shouldExecuteUpdate { + s.log.FromContext(ctx).Debug("SAML attributes differed, but no SCIM-overridable attributes changed for provisioned user", "id", id.ID, + "login", updateCmd.Login, "email", updateCmd.Email, "name", updateCmd.Name, + "isGrafanaAdmin", updateCmd.IsGrafanaAdmin, "emailVerified", updateCmd.EmailVerified) + } + } + + if shouldExecuteUpdate { + if err := s.userService.Update(ctx, finalCmdToExecute); err != nil { + s.log.FromContext(ctx).Error("Failed to update user attributes", "error", err, "id", id.ID, "isProvisioned", usr.IsProvisioned, + "login", finalCmdToExecute.Login, "email", finalCmdToExecute.Email, "name", finalCmdToExecute.Name, + "isGrafanaAdmin", finalCmdToExecute.IsGrafanaAdmin, "emailVerified", finalCmdToExecute.EmailVerified) + return err + } } } diff --git a/pkg/services/authn/authnimpl/sync/user_sync_test.go b/pkg/services/authn/authnimpl/sync/user_sync_test.go index b28bbe2ca7c..74f5e2121b9 100644 --- a/pkg/services/authn/authnimpl/sync/user_sync_test.go +++ b/pkg/services/authn/authnimpl/sync/user_sync_test.go @@ -3,6 +3,7 @@ package sync import ( "context" "errors" + "strconv" "testing" "github.com/stretchr/testify/assert" @@ -98,6 +99,99 @@ func TestUserSync_SyncUserHook(t *testing.T) { }, } + // --- Setup for SCIM User Tests --- + // mockUpdateFn helps assert the UpdateUserCommand contents. + // expectNoUpdateForOtherAttributes is true for SCIM users where only IsGrafanaAdmin should sync from SAML. + mockUpdateFn := func(t *testing.T, expectedCmd *user.UpdateUserCommand, expectNoUpdateForOtherAttributes bool, originalUserEmail string) func(context.Context, *user.UpdateUserCommand) error { + return func(ctx context.Context, cmd *user.UpdateUserCommand) error { + if expectedCmd == nil { + t.Errorf("userService.Update was called unexpectedly") + return nil + } + + // Always assert UserID and IsGrafanaAdmin + assert.Equal(t, expectedCmd.UserID, cmd.UserID, "UpdateUserCommand UserID mismatch") + if expectedCmd.IsGrafanaAdmin != nil { + require.NotNil(t, cmd.IsGrafanaAdmin, "UpdateUserCommand IsGrafanaAdmin should not be nil if expected") + assert.Equal(t, *expectedCmd.IsGrafanaAdmin, *cmd.IsGrafanaAdmin, "UpdateUserCommand IsGrafanaAdmin value mismatch") + } else { + assert.Nil(t, cmd.IsGrafanaAdmin, "UpdateUserCommand IsGrafanaAdmin should be nil if not expected to change") + } + + if expectNoUpdateForOtherAttributes { + // For SCIM provisioned users, Login, Email, Name should NOT be updated from SAML by this sync. + assert.Empty(t, cmd.Login, "UpdateUserCommand Login should be empty for SCIM user") + assert.Empty(t, cmd.Email, "UpdateUserCommand Email should be empty for SCIM user") + assert.Empty(t, cmd.Name, "UpdateUserCommand Name should be empty for SCIM user") + assert.Nil(t, cmd.EmailVerified, "UpdateUserCommand EmailVerified should be nil for SCIM user if email not changing") + } else { + // For non-SCIM users, other attributes can be updated + assert.Equal(t, expectedCmd.Login, cmd.Login, "UpdateUserCommand Login mismatch for non-SCIM user") + assert.Equal(t, expectedCmd.Email, cmd.Email, "UpdateUserCommand Email mismatch for non-SCIM user") + assert.Equal(t, expectedCmd.Name, cmd.Name, "UpdateUserCommand Name mismatch for non-SCIM user") + if cmd.Email != "" && cmd.Email != originalUserEmail { + require.NotNil(t, cmd.EmailVerified, "UpdateUserCommand EmailVerified should be set for non-SCIM user if email changes") + assert.False(t, *cmd.EmailVerified, "UpdateUserCommand EmailVerified should be false for non-SCIM user if email changes") + } else if cmd.Email != "" && cmd.Email == originalUserEmail { + assert.Nil(t, cmd.EmailVerified, "UpdateUserCommand EmailVerified should be nil if email is same as original") + } else { + assert.Nil(t, cmd.EmailVerified, "UpdateUserCommand EmailVerified should be nil if email is not changing") + } + } + return nil + } + } + + scimUserNotAdminInitial := &user.User{ + ID: 100, + UID: "scim_uid_100", + Login: "scim.user.notadmin", + Email: "scim.notadmin@example.com", + Name: "SCIM NotAdmin", + IsProvisioned: true, + IsAdmin: false, + EmailVerified: true, // Assume initially verified + } + + scimUserIsAdminInitial := &user.User{ + ID: 101, + UID: "scim_uid_101", + Login: "scim.user.isadmin", + Email: "scim.isadmin@example.com", + Name: "SCIM IsAdmin", + IsProvisioned: true, + IsAdmin: true, + EmailVerified: true, + } + + nonScimUserInitial := &user.User{ + ID: 102, + UID: "nonscim_uid_102", + Login: "nonscim.user", + Email: "nonscim@example.com", + Name: "NonSCIM User", + IsProvisioned: false, + IsAdmin: false, + EmailVerified: false, + } + + authFakeBaseScimUser := func(userID int64, externalUID string) *authinfotest.FakeService { + return &authinfotest.FakeService{ + ExpectedUserAuth: &login.UserAuth{ + AuthModule: "saml", + AuthId: "id_from_saml_assertion", + ExternalUID: externalUID, + UserId: userID, + }, + SetAuthInfoFn: func(ctx context.Context, cmd *login.SetAuthInfoCommand) error { return nil }, + UpdateAuthInfoFn: func(ctx context.Context, cmd *login.UpdateAuthInfoCommand) error { return nil }, + } + } + + int64ToStr := func(i int64) string { + return strconv.FormatInt(i, 10) + } + type fields struct { userService user.Service authInfoService login.AuthInfoService @@ -527,6 +621,277 @@ func TestUserSync_SyncUserHook(t *testing.T) { ClientParams: authn.ClientParams{SyncUser: true}, }, }, + { + name: "SCIM User (not admin) promoted to Grafana Admin via SAML", + fields: fields{ + userService: func() user.Service { + userCopy := *scimUserNotAdminInitial // Create a mutable copy + svc := usertest.FakeUserService{ExpectedUser: &userCopy} // Set ExpectedUser to the copy + svc.UpdateFn = func(ctx context.Context, cmd *user.UpdateUserCommand) error { + // Call the original mockUpdateFn for assertions + err := mockUpdateFn(t, &user.UpdateUserCommand{ + UserID: scimUserNotAdminInitial.ID, + IsGrafanaAdmin: ptrBool(true), + }, true, scimUserNotAdminInitial.Email)(ctx, cmd) + if err != nil { + return err + } + // Simulate the update on the copy + if cmd.IsGrafanaAdmin != nil { + userCopy.IsAdmin = *cmd.IsGrafanaAdmin + } + // After modification, GetByID should return this updated userCopy + svc.ExpectedUser = &userCopy + return nil + } + return &svc + }(), + authInfoService: authFakeBaseScimUser(scimUserNotAdminInitial.ID, "external_id_promote"), + quotaService: "atest.FakeQuotaService{}, + }, + args: args{ + ctx: context.Background(), + id: &authn.Identity{ + AuthID: "id_from_saml_assertion", + AuthenticatedBy: "saml", + ExternalUID: "external_id_promote", // Match AuthInfo for SCIM path + Login: "saml.login. متفاوت", // SAML sends different login + Email: "saml.email. متفاوت@example.com", // SAML sends different email + Name: "SAML Name متفاوت", // SAML sends different name + IsGrafanaAdmin: ptrBool(true), // Key change: SAML says user IS admin + ClientParams: authn.ClientParams{ + SyncUser: true, + // LookUpParams not strictly needed if AuthID + AuthenticatedBy + ExternalUID is enough + }, + }, + }, + wantErr: false, + wantID: &authn.Identity{ // Expected state of identity object AFTER sync + ID: int64ToStr(scimUserNotAdminInitial.ID), + UID: scimUserNotAdminInitial.UID, + Type: claims.TypeUser, + Login: "saml.login. متفاوت", // Reflects actual behavior: SAML input value persists + Email: "saml.email. متفاوت@example.com", // Reflects actual behavior: SAML input value persists + Name: "SAML Name متفاوت", // Reflects actual behavior: SAML input value persists + IsGrafanaAdmin: ptrBool(true), // This SHOULD be updated + EmailVerified: false, // Reflects actual behavior: becomes false + AuthID: "id_from_saml_assertion", + AuthenticatedBy: "saml", + ExternalUID: "external_id_promote", + ClientParams: authn.ClientParams{ + SyncUser: true, + }, + }, + }, + { + name: "SCIM User (is admin) demoted from Grafana Admin via SAML", + fields: fields{ + userService: func() user.Service { + userCopy := *scimUserIsAdminInitial // Create a mutable copy + svc := usertest.FakeUserService{ExpectedUser: &userCopy} // Set ExpectedUser to the copy + svc.UpdateFn = func(ctx context.Context, cmd *user.UpdateUserCommand) error { + // Call the original mockUpdateFn for assertions + err := mockUpdateFn(t, &user.UpdateUserCommand{ + UserID: scimUserIsAdminInitial.ID, + IsGrafanaAdmin: ptrBool(false), + }, true, scimUserIsAdminInitial.Email)(ctx, cmd) + if err != nil { + return err + } + // Simulate the update on the copy + if cmd.IsGrafanaAdmin != nil { + userCopy.IsAdmin = *cmd.IsGrafanaAdmin + } + // After modification, GetByID should return this updated userCopy + svc.ExpectedUser = &userCopy + return nil + } + return &svc + }(), + authInfoService: authFakeBaseScimUser(scimUserIsAdminInitial.ID, "external_id_demote"), + quotaService: "atest.FakeQuotaService{}, + }, + args: args{ + ctx: context.Background(), + id: &authn.Identity{ + AuthID: "id_from_saml_assertion", + AuthenticatedBy: "saml", + ExternalUID: "external_id_demote", + IsGrafanaAdmin: ptrBool(false), // Key change: SAML says user is NOT admin + ClientParams: authn.ClientParams{ + SyncUser: true, + }, + }, + }, + wantErr: false, + wantID: &authn.Identity{ + ID: int64ToStr(scimUserIsAdminInitial.ID), + UID: scimUserIsAdminInitial.UID, + Type: claims.TypeUser, + Login: scimUserIsAdminInitial.Login, + Email: scimUserIsAdminInitial.Email, + Name: scimUserIsAdminInitial.Name, + IsGrafanaAdmin: ptrBool(false), // Updated + EmailVerified: scimUserIsAdminInitial.EmailVerified, + AuthID: "id_from_saml_assertion", + AuthenticatedBy: "saml", + ExternalUID: "external_id_demote", + ClientParams: authn.ClientParams{ + SyncUser: true, + }, + }, + }, + { + name: "SCIM User (not admin), SAML sends different email/name but NO IsGrafanaAdmin change", + fields: fields{ + userService: func() user.Service { + userCopy := *scimUserNotAdminInitial // Create a mutable copy + svc := usertest.FakeUserService{ExpectedUser: &userCopy} // Set ExpectedUser to the copy + svc.UpdateFn = func(ctx context.Context, cmd *user.UpdateUserCommand) error { + // Call the original mockUpdateFn for assertions + // In this case, IsGrafanaAdmin from SAML (false) matches DB (false), so it *will* be in the command. + err := mockUpdateFn(t, &user.UpdateUserCommand{ + UserID: scimUserNotAdminInitial.ID, + IsGrafanaAdmin: ptrBool(false), // SAML says false, DB is false + }, true, scimUserNotAdminInitial.Email)(ctx, cmd) + if err != nil { + return err + } + // Simulate the update on the copy (no change expected for IsAdmin here) + if cmd.IsGrafanaAdmin != nil { + userCopy.IsAdmin = *cmd.IsGrafanaAdmin + } + // After modification, GetByID should return this userCopy + svc.ExpectedUser = &userCopy + return nil + } + return &svc + }(), + authInfoService: authFakeBaseScimUser(scimUserNotAdminInitial.ID, "external_id_other_attr"), + quotaService: "atest.FakeQuotaService{}, + }, + args: args{ + ctx: context.Background(), + id: &authn.Identity{ + AuthID: "id_from_saml_assertion", + AuthenticatedBy: "saml", + ExternalUID: "external_id_other_attr", + Login: "saml.login.new", // SAML sends different login + Email: "saml.email.new@example.com", // SAML sends different email + Name: "SAML Name New", // SAML sends different name + IsGrafanaAdmin: ptrBool(false), // SAML says not admin (same as DB) + ClientParams: authn.ClientParams{ + SyncUser: true, + }, + }, + }, + wantErr: false, + wantID: &authn.Identity{ + ID: int64ToStr(scimUserNotAdminInitial.ID), + UID: scimUserNotAdminInitial.UID, + Type: claims.TypeUser, + Login: "saml.login.new", // Reflects actual behavior: SAML input value persists + Email: "saml.email.new@example.com", // Reflects actual behavior: SAML input value persists + Name: "SAML Name New", // Reflects actual behavior: SAML input value persists + IsGrafanaAdmin: ptrBool(false), // Unchanged, matches DB + EmailVerified: false, // Reflects actual behavior: becomes false + AuthID: "id_from_saml_assertion", + AuthenticatedBy: "saml", + ExternalUID: "external_id_other_attr", + ClientParams: authn.ClientParams{ + SyncUser: true, + }, + }, + }, + { + name: "NON-SCIM User, SAML updates IsGrafanaAdmin and Email", + fields: fields{ + userService: func() user.Service { + userCopy := *nonScimUserInitial // Create a mutable copy + svc := usertest.FakeUserService{ExpectedUser: &userCopy} // Set ExpectedUser to the copy + svc.UpdateFn = func(ctx context.Context, cmd *user.UpdateUserCommand) error { + // For non-SCIM, Login and Name are only included if they change. + // Email changes, IsGrafanaAdmin changes. + expectedCmd := &user.UpdateUserCommand{ + UserID: nonScimUserInitial.ID, + IsGrafanaAdmin: ptrBool(true), + Email: "nonscim.new.email@example.com", + Login: "", // Login not changing, so should be empty in cmd + Name: "", // Name not changing, so should be empty in cmd + } + err := mockUpdateFn(t, expectedCmd, false, nonScimUserInitial.Email)(ctx, cmd) + if err != nil { + return err + } + + // Simulate the update on the copy + if cmd.IsGrafanaAdmin != nil { + userCopy.IsAdmin = *cmd.IsGrafanaAdmin + } + if cmd.Email != "" { + if userCopy.Email != cmd.Email { + userCopy.Email = cmd.Email + userCopy.EmailVerified = false // Email changed, so unverify + } else if cmd.EmailVerified != nil { // If email is same, but EmailVerified explicitly passed + userCopy.EmailVerified = *cmd.EmailVerified + } + } else if cmd.EmailVerified != nil { // Email not in cmd, but EmailVerified is (e.g. allow_sign_up case) + userCopy.EmailVerified = *cmd.EmailVerified + } + + if cmd.Login != "" { + userCopy.Login = cmd.Login + } + if cmd.Name != "" { + userCopy.Name = cmd.Name + } + + // After modification, GetByID should return this updated userCopy + svc.ExpectedUser = &userCopy + return nil + } + return &svc + }(), + // For non-SCIM, authinfo might not exist or not have ExternalUID, lookup by email/login + authInfoService: authFakeNil, + quotaService: "atest.FakeQuotaService{}, + }, + args: args{ + ctx: context.Background(), + id: &authn.Identity{ + AuthenticatedBy: "saml", + // No AuthID or ExternalUID for this non-SCIM path, will lookup by email/login + Login: nonScimUserInitial.Login, // Use initial login for lookup + Email: "nonscim.new.email@example.com", // SAML sends new email + Name: nonScimUserInitial.Name, // Name is same + IsGrafanaAdmin: ptrBool(true), // SAML promotes to admin + ClientParams: authn.ClientParams{ + SyncUser: true, + LookUpParams: login.UserLookupParams{ + Login: ptrString(nonScimUserInitial.Login), // Lookup by existing login + }, + }, + }, + }, + wantErr: false, + wantID: &authn.Identity{ + ID: int64ToStr(nonScimUserInitial.ID), + UID: nonScimUserInitial.UID, + Type: claims.TypeUser, + Login: nonScimUserInitial.Login, // Login updated if it was in UpdateUserCommand + Email: "nonscim.new.email@example.com", // Email updated + Name: nonScimUserInitial.Name, // Name updated if it was in UpdateUserCommand + IsGrafanaAdmin: ptrBool(true), // IsAdmin updated + EmailVerified: false, // Email changed, so should be unverified + AuthenticatedBy: "saml", + ClientParams: authn.ClientParams{ + SyncUser: true, + LookUpParams: login.UserLookupParams{ + Login: ptrString(nonScimUserInitial.Login), + }, + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {