From eeb4c045d3a9db4c8d23e0a4fe2ae5417c66157e Mon Sep 17 00:00:00 2001 From: linoman <2051016+linoman@users.noreply.github.com> Date: Tue, 8 Apr 2025 22:50:39 +0200 Subject: [PATCH] SCIM: Add access control for non provisioned users (#103596) * Add hook to validate access for users based on provisioning logic * Wire the hook * Add tests * declare new variables for errors * rework the authorization flow for provisioned users * Add scim feature to testinfra opts * Grant access if the identity doesn't have associated a user * skip external uid check for subsequent calls * Update tests --- pkg/services/authn/authnimpl/registration.go | 5 +- .../authn/authnimpl/sync/user_sync.go | 129 +++++++-- .../authn/authnimpl/sync/user_sync_test.go | 261 +++++++++++++++++- pkg/tests/testinfra/testinfra.go | 7 + 4 files changed, 375 insertions(+), 27 deletions(-) diff --git a/pkg/services/authn/authnimpl/registration.go b/pkg/services/authn/authnimpl/registration.go index 8143c4e755a..13dde1f6280 100644 --- a/pkg/services/authn/authnimpl/registration.go +++ b/pkg/services/authn/authnimpl/registration.go @@ -131,11 +131,12 @@ func ProvideRegistration( } // FIXME (jguer): move to User package - userSync := sync.ProvideUserSync(userService, userProtectionService, authInfoService, quotaService, tracer, features) + userSync := sync.ProvideUserSync(userService, userProtectionService, authInfoService, quotaService, tracer, features, cfg) orgSync := sync.ProvideOrgSync(userService, orgService, accessControlService, cfg, tracer) authnSvc.RegisterPostAuthHook(userSync.SyncUserHook, 10) authnSvc.RegisterPostAuthHook(userSync.EnableUserHook, 20) - authnSvc.RegisterPostAuthHook(orgSync.SyncOrgRolesHook, 30) + 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) diff --git a/pkg/services/authn/authnimpl/sync/user_sync.go b/pkg/services/authn/authnimpl/sync/user_sync.go index 60539ff6b3d..df082c12019 100644 --- a/pkg/services/authn/authnimpl/sync/user_sync.go +++ b/pkg/services/authn/authnimpl/sync/user_sync.go @@ -19,6 +19,7 @@ import ( "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/quota" "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/setting" ) var ( @@ -54,6 +55,22 @@ var ( "user.sync.empty-externalUID", errutil.WithPublicMessage("Empty externalUID"), ) + errUnableToRetrieveUserOrAuthInfo = errutil.Internal( + "user.sync.unable-to-retrieve-user-or-authinfo", + errutil.WithPublicMessage("Unable to retrieve user or authInfo for validation"), + ) + errUnableToRetrieveUser = errutil.Internal( + "user.sync.unable-to-retrieve-user", + errutil.WithPublicMessage("Unable to retrieve user for validation"), + ) + errUserNotProvisioned = errutil.Forbidden( + "user.sync.user-not-provisioned", + errutil.WithPublicMessage("User is not provisioned"), + ) + errUserExternalUIDMismatch = errutil.Unauthorized( + "user.sync.user-externalUID-mismatch", + errutil.WithPublicMessage("User externalUID mismatch"), + ) ) var ( @@ -63,29 +80,98 @@ var ( ) func ProvideUserSync(userService user.Service, userProtectionService login.UserProtectionService, authInfoService login.AuthInfoService, - quotaService quota.Service, tracer tracing.Tracer, features featuremgmt.FeatureToggles, + quotaService quota.Service, tracer tracing.Tracer, features featuremgmt.FeatureToggles, cfg *setting.Cfg, ) *UserSync { + scimSection := cfg.Raw.Section("auth.scim") return &UserSync{ - userService: userService, - authInfoService: authInfoService, - userProtectionService: userProtectionService, - quotaService: quotaService, - log: log.New("user.sync"), - tracer: tracer, - features: features, - lastSeenSF: &singleflight.Group{}, + allowNonProvisionedUsers: scimSection.Key("allowed_non_provisioned_users").MustBool(false), + isUserProvisioningEnabled: scimSection.Key("user_sync_enabled").MustBool(false), + userService: userService, + authInfoService: authInfoService, + userProtectionService: userProtectionService, + quotaService: quotaService, + log: log.New("user.sync"), + tracer: tracer, + features: features, + lastSeenSF: &singleflight.Group{}, } } type UserSync struct { - userService user.Service - authInfoService login.AuthInfoService - userProtectionService login.UserProtectionService - quotaService quota.Service - log log.Logger - tracer tracing.Tracer - features featuremgmt.FeatureToggles - lastSeenSF *singleflight.Group + allowNonProvisionedUsers bool + isUserProvisioningEnabled bool + userService user.Service + authInfoService login.AuthInfoService + userProtectionService login.UserProtectionService + quotaService quota.Service + log log.Logger + tracer tracing.Tracer + features featuremgmt.FeatureToggles + lastSeenSF *singleflight.Group +} + +// 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) + + 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") + 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 + } + + // In order to guarantee the provisioned user is the same as the identity, + // we must validate the authinfo.ExternalUID with the identity.ExternalUID + + // Retrieve user and authinfo from database + usr, authInfo, err := s.getUser(ctx, id) + if err != nil { + if errors.Is(err, user.ErrUserNotFound) { + return nil + } + log.Error("Failed to fetch user for validation", "error", err) + return errUnableToRetrieveUserOrAuthInfo.Errorf("unable to retrieve user or authInfo for validation") + } + + if usr == nil { + log.Error("Failed to fetch user for validation", "error", err) + return errUnableToRetrieveUser.Errorf("unable to retrieve user for validation") + } + + // Validate the provisioned user.ExternalUID with the authinfo.ExternalUID + if usr.IsProvisioned { + // The user is provisioned via SAML and the identity is empty, meaning this request is not from the SAML auth flow + if authInfo.AuthModule == 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") + } + log.Debug("User is provisioned, access granted") + return nil + } + + // Reject non-provisioned users + log.Error("Failed to access user, user is not provisioned") + return errUserNotProvisioned.Errorf("user is not provisioned") } // SyncUserHook syncs a user with the database @@ -134,11 +220,6 @@ func (s *UserSync) SyncUserHook(ctx context.Context, id *authn.Identity, _ *auth } } - if usr.IsProvisioned && id.ExternalUID != userAuth.ExternalUID { - s.log.Error("mismatched externalUID", "provisioned_externalUID", userAuth.ExternalUID, "identity_externalUID", id.ExternalUID) - return errMismatchedExternalUID.Errorf("externalUID mistmatch") - } - syncUserToIdentity(usr, id) return nil } @@ -326,7 +407,7 @@ func (s *UserSync) updateUserAttributes(ctx context.Context, usr *user.User, id attribute.String("identity.ExternalUID", id.ExternalUID), ) if usr.IsProvisioned { - s.log.Debug("User is provisioned", "id,UID", id.UID) + s.log.Debug("User is provisioned", "id.UID", id.UID) needsConnectionCreation = false authInfo, err := s.authInfoService.GetAuthInfo(ctx, &login.GetAuthInfoQuery{UserId: usr.ID, AuthModule: id.AuthenticatedBy}) if err != nil { @@ -400,7 +481,7 @@ func (s *UserSync) getUser(ctx context.Context, identity *authn.Identity) (*user ctx, span := s.tracer.Start(ctx, "user.sync.getUser") defer span.End() - // Check auth info fist + // Check auth info first if identity.AuthID != "" && identity.AuthenticatedBy != "" { query := &login.GetAuthInfoQuery{AuthId: identity.AuthID, AuthModule: identity.AuthenticatedBy} authInfo, errGetAuthInfo := s.authInfoService.GetAuthInfo(ctx, query) diff --git a/pkg/services/authn/authnimpl/sync/user_sync_test.go b/pkg/services/authn/authnimpl/sync/user_sync_test.go index 8999a4b6979..266ba3a739a 100644 --- a/pkg/services/authn/authnimpl/sync/user_sync_test.go +++ b/pkg/services/authn/authnimpl/sync/user_sync_test.go @@ -2,6 +2,7 @@ package sync import ( "context" + "errors" "testing" "github.com/stretchr/testify/assert" @@ -10,6 +11,7 @@ import ( claims "github.com/grafana/authlib/types" + "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/services/authn" "github.com/grafana/grafana/pkg/services/featuremgmt" @@ -20,6 +22,7 @@ import ( "github.com/grafana/grafana/pkg/services/quota/quotatest" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/services/user/usertest" + "github.com/grafana/grafana/pkg/setting" ) func ptrString(s string) *string { @@ -439,7 +442,7 @@ func TestUserSync_SyncUserHook(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - s := ProvideUserSync(tt.fields.userService, userProtection, tt.fields.authInfoService, tt.fields.quotaService, tracing.InitializeTracerForTest(), featuremgmt.WithFeatures()) + s := ProvideUserSync(tt.fields.userService, userProtection, tt.fields.authInfoService, tt.fields.quotaService, tracing.InitializeTracerForTest(), featuremgmt.WithFeatures(), setting.NewCfg()) err := s.SyncUserHook(tt.args.ctx, tt.args.id, nil) if tt.wantErr { require.Error(t, err) @@ -465,6 +468,7 @@ func TestUserSync_SyncUserRetryFetch(t *testing.T) { "atest.FakeQuotaService{}, tracing.NewNoopTracerService(), featuremgmt.WithFeatures(), + setting.NewCfg(), ) email := "test@test.com" @@ -569,3 +573,258 @@ func TestUserSync_EnableDisabledUserHook(t *testing.T) { }) } } + +func initUserSyncService() *UserSync { + userSvc := usertest.NewUserServiceFake() + log := log.New("test") + authInfoSvc := &authinfotest.FakeService{ + ExpectedUserAuth: &login.UserAuth{ + UserId: 1, + AuthModule: login.SAMLAuthModule, + AuthId: "1", + }, + } + quotaSvc := "atest.FakeQuotaService{} + return &UserSync{ + userService: userSvc, + authInfoService: authInfoSvc, + quotaService: quotaSvc, + tracer: tracing.InitializeTracerForTest(), + log: log, + } +} + +func TestUserSync_ValidateUserProvisioningHook(t *testing.T) { + type testCase struct { + desc string + identity *authn.Identity + userSyncServiceSetup func() *UserSync + expectedErr error + } + + tests := []testCase{ + { + desc: "it should skip validation if the user provisioning is disabled", + userSyncServiceSetup: func() *UserSync { + userSyncService := initUserSyncService() + userSyncService.isUserProvisioningEnabled = false + return userSyncService + }, + identity: &authn.Identity{ + AuthenticatedBy: login.GenericOAuthModule, + AuthID: "1", + }, + }, + { + desc: "it should skip validation if allowedNonProvisionedUsers is enabled", + userSyncServiceSetup: func() *UserSync { + userSyncService := initUserSyncService() + userSyncService.allowNonProvisionedUsers = true + userSyncService.isUserProvisioningEnabled = true + return userSyncService + }, + identity: &authn.Identity{ + AuthenticatedBy: login.GenericOAuthModule, + AuthID: "1", + }, + }, + { + desc: "it should skip validation if the user is authenticated via GrafanaComAuthModule", + userSyncServiceSetup: func() *UserSync { + userSyncService := initUserSyncService() + userSyncService.allowNonProvisionedUsers = false + userSyncService.isUserProvisioningEnabled = true + return userSyncService + }, + identity: &authn.Identity{ + AuthenticatedBy: login.GrafanaComAuthModule, + AuthID: "1", + }, + }, + { + desc: "it should fail to validate the identity with the provisioned user, unexpected error", + userSyncServiceSetup: func() *UserSync { + userSyncService := initUserSyncService() + userSyncService.allowNonProvisionedUsers = false + userSyncService.isUserProvisioningEnabled = true + userSyncService.userService = &usertest.FakeUserService{ + ExpectedError: errors.New("random error"), + } + return userSyncService + }, + identity: &authn.Identity{ + AuthenticatedBy: login.SAMLAuthModule, + AuthID: "1", + ExternalUID: "random-external-uid", + }, + expectedErr: errUnableToRetrieveUserOrAuthInfo.Errorf("unable to retrieve user or authInfo for validation"), + }, + { + desc: "it should fail to validate the identity with the provisioned user, no user found", + userSyncServiceSetup: func() *UserSync { + userSyncService := initUserSyncService() + userSyncService.allowNonProvisionedUsers = false + userSyncService.isUserProvisioningEnabled = true + userSyncService.userService = &usertest.FakeUserService{} + return userSyncService + }, + identity: &authn.Identity{ + AuthenticatedBy: login.SAMLAuthModule, + AuthID: "1", + ExternalUID: "random-external-uid", + }, + expectedErr: errUnableToRetrieveUser.Errorf("unable to retrieve user for validation"), + }, + { + desc: "it should fail to validate the provisioned user.ExternalUID with the identity.ExternalUID - empty ExternalUID", + 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", + }, + } + return userSyncService + }, + identity: &authn.Identity{ + AuthenticatedBy: login.SAMLAuthModule, + AuthID: "1", + ExternalUID: "random-external-uid", + }, + expectedErr: errUserExternalUIDMismatch.Errorf("the provisioned user.ExternalUID does not match the authinfo.ExternalUID"), + }, + { + desc: "it should fail to validate the provisioned user.ExternalUID with the identity.ExternalUID - different ExternalUID", + 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: "different-external-uid", + }, + } + return userSyncService + }, + identity: &authn.Identity{ + AuthenticatedBy: login.SAMLAuthModule, + AuthID: "1", + ExternalUID: "random-external-uid", + }, + expectedErr: errUserExternalUIDMismatch.Errorf("the provisioned user.ExternalUID does not match the authinfo.ExternalUID"), + }, + { + desc: "it should successfully validate the provisioned user.ExternalUID with the identity.ExternalUID", + 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.SAMLAuthModule, + AuthID: "1", + ExternalUID: "random-external-uid", + }, + }, + { + desc: "it should failed to validate a non provisioned user when retrieved from the database", + userSyncServiceSetup: func() *UserSync { + userSyncService := initUserSyncService() + userSyncService.allowNonProvisionedUsers = false + userSyncService.isUserProvisioningEnabled = true + userSyncService.userService = &usertest.FakeUserService{ + ExpectedUser: &user.User{ + ID: 1, + IsProvisioned: false, + }, + } + 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.SAMLAuthModule, + AuthID: "1", + ExternalUID: "random-external-uid", + }, + 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.SAMLAuthModule, + AuthID: "1", + ExternalUID: "", + }, + expectedErr: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + userSyncService := tt.userSyncServiceSetup() + err := userSyncService.ValidateUserProvisioningHook(context.Background(), tt.identity, nil) + require.ErrorIs(t, err, tt.expectedErr) + }) + } +} diff --git a/pkg/tests/testinfra/testinfra.go b/pkg/tests/testinfra/testinfra.go index f0afa3ddc05..5375e491525 100644 --- a/pkg/tests/testinfra/testinfra.go +++ b/pkg/tests/testinfra/testinfra.go @@ -494,6 +494,12 @@ func CreateGrafDir(t *testing.T, opts GrafanaOpts) (string, string) { _, err = pathsSect.NewKey("permitted_provisioning_paths", opts.PermittedProvisioningPaths) require.NoError(t, err) } + if opts.EnableSCIM { + scimSection, err := getOrCreateSection("auth.scim") + require.NoError(t, err) + _, err = scimSection.NewKey("user_sync_enabled", "true") + require.NoError(t, err) + } dbSection, err := getOrCreateSection("database") require.NoError(t, err) @@ -558,6 +564,7 @@ type GrafanaOpts struct { GrafanaComSSOAPIToken string LicensePath string EnableRecordingRules bool + EnableSCIM bool // When "unified-grpc" is selected it will also start the grpc server APIServerStorageType options.StorageType