SCIM: Update Provisioned User's Role using SAML Assertion (#106374)

* SCIM: fix provisioned user role assignment from SAML assertion

* revert org_sync_test changes

* clean up tests

* skip user lookup during org sync

* sanitize log output

* only log non-sensitive fields
This commit is contained in:
colin-stuart
2025-06-06 10:09:51 -04:00
committed by GitHub
parent db21e9e4bc
commit 34b6d51016
3 changed files with 400 additions and 18 deletions
@@ -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 {
+35 -8
View File
@@ -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
}
}
}
@@ -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: &quotatest.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: &quotatest.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: &quotatest.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: &quotatest.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) {