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

@@ -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)

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,
+		allowNonProvisionedUsers:  scimSection.Key("allowed_non_provisioned_users").MustBool(false),
-		authInfoService:       authInfoService,
+		isUserProvisioningEnabled: scimSection.Key("user_sync_enabled").MustBool(false),
-		userProtectionService: userProtectionService,
+		userService:               userService,
-		quotaService:          quotaService,
+		authInfoService:           authInfoService,
-		log:                   log.New("user.sync"),
+		userProtectionService:     userProtectionService,
-		tracer:                tracer,
+		quotaService:              quotaService,
-		features:              features,
+		log:                       log.New("user.sync"),
-		lastSeenSF:            &singleflight.Group{},
+		tracer:                    tracer,
+		features:                  features,
+		lastSeenSF:                &singleflight.Group{},
 	}
 }
 
 type UserSync struct {
-	userService           user.Service
+	allowNonProvisionedUsers  bool
-	authInfoService       login.AuthInfoService
+	isUserProvisioningEnabled bool
-	userProtectionService login.UserProtectionService
+	userService               user.Service
-	quotaService          quota.Service
+	authInfoService           login.AuthInfoService
-	log                   log.Logger
+	userProtectionService     login.UserProtectionService
-	tracer                tracing.Tracer
+	quotaService              quota.Service
-	features              featuremgmt.FeatureToggles
+	log                       log.Logger
-	lastSeenSF            *singleflight.Group
+	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)

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) {
 		&quotatest.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 := &quotatest.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)
+		})
+	}
+}

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