From 5156177079f9f539f7ff453197e14c5734cda33e Mon Sep 17 00:00:00 2001 From: colin-stuart Date: Fri, 12 Dec 2025 13:51:37 -0600 Subject: [PATCH] SCIM: show error if SCIM-provisioned user attempts login with non-SAML auth module (#115271) --- .../authn/authnimpl/sync/user_sync.go | 19 ++++ .../authn/authnimpl/sync/user_sync_test.go | 97 +++++++++++++++++++ 2 files changed, 116 insertions(+) diff --git a/pkg/services/authn/authnimpl/sync/user_sync.go b/pkg/services/authn/authnimpl/sync/user_sync.go index cb8099ba1d1..ada03081add 100644 --- a/pkg/services/authn/authnimpl/sync/user_sync.go +++ b/pkg/services/authn/authnimpl/sync/user_sync.go @@ -77,6 +77,10 @@ var ( "user.sync.user-externalUID-mismatch", errutil.WithPublicMessage("User externalUID mismatch"), ) + errSCIMAuthModuleMismatch = errutil.Unauthorized( + "user.sync.scim-auth-module-mismatch", + errutil.WithPublicMessage("User was provisioned via SCIM and must login via SAML"), + ) ) var ( @@ -308,6 +312,21 @@ func (s *UserSync) SyncUserHook(ctx context.Context, id *authn.Identity, _ *auth // just try to fetch the user one more to make the other request work. if errors.Is(err, user.ErrUserAlreadyExists) { usr, _, err = s.getUser(ctx, id) + + // Check if this is a SCIM-provisioned user trying to login via an auth module that is not SAML or GCOM + if err == nil && usr != nil && usr.IsProvisioned && id.AuthenticatedBy != login.GrafanaComAuthModule { + _, authErr := s.authInfoService.GetAuthInfo(ctx, &login.GetAuthInfoQuery{ + UserId: usr.ID, + AuthModule: id.AuthenticatedBy, + }) + if errors.Is(authErr, user.ErrUserNotFound) { + s.log.FromContext(ctx).Error("SCIM-provisioned user attempted login via non-SAML auth module", + "user_id", usr.ID, + "attempted_module", id.AuthenticatedBy, + ) + return errSCIMAuthModuleMismatch.Errorf("user was provisioned via SCIM but attempted login via %s", id.AuthenticatedBy) + } + } } if err != nil { diff --git a/pkg/services/authn/authnimpl/sync/user_sync_test.go b/pkg/services/authn/authnimpl/sync/user_sync_test.go index dd19836b0a5..ad863164aee 100644 --- a/pkg/services/authn/authnimpl/sync/user_sync_test.go +++ b/pkg/services/authn/authnimpl/sync/user_sync_test.go @@ -1926,3 +1926,100 @@ func TestUserSync_SCIMLoginUsageStatSet(t *testing.T) { finalCount := finalStats["stats.features.scim.has_successful_login.count"].(int) require.Equal(t, int(1), finalCount) } + +func TestUserSync_SyncUserHook_SCIMAuthModuleMismatch(t *testing.T) { + userSrv := usertest.NewMockService(t) + authInfoSrv := authinfotest.NewMockAuthInfoService(t) + + userSrv.On("GetByEmail", mock.Anything, mock.Anything).Return(nil, user.ErrUserNotFound).Once() + + userSrv.On("Create", mock.Anything, mock.Anything).Return(nil, user.ErrUserAlreadyExists).Once() + + userSrv.On("GetByEmail", mock.Anything, mock.Anything).Return(&user.User{ + ID: 1, + Email: "test@test.com", + IsProvisioned: true, + }, nil).Once() + + authInfoSrv.On("GetAuthInfo", mock.Anything, mock.MatchedBy(func(q *login.GetAuthInfoQuery) bool { + return q.AuthModule == "oauth_azuread" + })).Return(nil, user.ErrUserNotFound).Once() + + s := ProvideUserSync( + userSrv, + authinfoimpl.ProvideOSSUserProtectionService(), + authInfoSrv, + "atest.FakeQuotaService{}, + tracing.NewNoopTracerService(), + featuremgmt.WithFeatures(), + setting.NewCfg(), + nil, + ) + + email := "test@test.com" + + err := s.SyncUserHook(context.Background(), &authn.Identity{ + AuthenticatedBy: "oauth_azuread", + ClientParams: authn.ClientParams{ + SyncUser: true, + AllowSignUp: true, + LookUpParams: login.UserLookupParams{ + Email: &email, + }, + }, + }, nil) + + require.Error(t, err) + assert.ErrorIs(t, err, errSCIMAuthModuleMismatch) + assert.Contains(t, err.Error(), "SCIM") + assert.Contains(t, err.Error(), "oauth_azuread") +} + +func TestUserSync_SyncUserHook_SCIMUserAllowsGCOMLogin(t *testing.T) { + userSrv := usertest.NewMockService(t) + authInfoSrv := authinfotest.NewMockAuthInfoService(t) + + authInfoSrv.On("GetAuthInfo", mock.Anything, mock.MatchedBy(func(q *login.GetAuthInfoQuery) bool { + return q.AuthModule == login.GrafanaComAuthModule && q.AuthId == "gcom-user-123" + })).Return(nil, user.ErrUserNotFound).Once() + + userSrv.On("GetByEmail", mock.Anything, mock.Anything).Return(nil, user.ErrUserNotFound).Once() + userSrv.On("Create", mock.Anything, mock.Anything).Return(nil, user.ErrUserAlreadyExists).Once() + + authInfoSrv.On("GetAuthInfo", mock.Anything, mock.MatchedBy(func(q *login.GetAuthInfoQuery) bool { + return q.AuthModule == login.GrafanaComAuthModule && q.AuthId == "gcom-user-123" + })).Return(nil, user.ErrUserNotFound).Once() + + userSrv.On("GetByEmail", mock.Anything, mock.Anything).Return(&user.User{ + ID: 1, + Email: "test@test.com", + IsProvisioned: true, + }, nil).Once() + + s := ProvideUserSync( + userSrv, + authinfoimpl.ProvideOSSUserProtectionService(), + authInfoSrv, + "atest.FakeQuotaService{}, + tracing.NewNoopTracerService(), + featuremgmt.WithFeatures(), + setting.NewCfg(), + nil, + ) + + email := "test@test.com" + + err := s.SyncUserHook(context.Background(), &authn.Identity{ + AuthenticatedBy: login.GrafanaComAuthModule, + AuthID: "gcom-user-123", + ClientParams: authn.ClientParams{ + SyncUser: true, + AllowSignUp: true, + LookUpParams: login.UserLookupParams{ + Email: &email, + }, + }, + }, nil) + + require.NoError(t, err) +}