From be3b81fecdd5b804679db65fdefa17953ae01a8d Mon Sep 17 00:00:00 2001 From: Jo Date: Mon, 16 Jan 2023 11:15:14 +0000 Subject: [PATCH] AuthN: Readd user protection service to user sync (#61534) * add user protection service to user sync * fix tests --- pkg/services/authn/authnimpl/service.go | 3 +- .../authn/authnimpl/usersync/usersync.go | 32 +++++++++++++++---- .../authn/authnimpl/usersync/usersync_test.go | 20 +++--------- 3 files changed, 31 insertions(+), 24 deletions(-) diff --git a/pkg/services/authn/authnimpl/service.go b/pkg/services/authn/authnimpl/service.go index 37dcc4b324d..9a874c33c41 100644 --- a/pkg/services/authn/authnimpl/service.go +++ b/pkg/services/authn/authnimpl/service.go @@ -40,6 +40,7 @@ func ProvideService( accessControlService accesscontrol.Service, apikeyService apikey.Service, userService user.Service, jwtService auth.JWTVerifierService, + userProtectionService login.UserProtectionService, loginAttempts loginattempt.Service, quotaService quota.Service, authInfoService login.AuthInfoService, renderService rendering.Service, ) *Service { @@ -83,7 +84,7 @@ func ProvideService( } // FIXME (jguer): move to User package - userSyncService := sync.ProvideUserSync(userService, authInfoService, quotaService) + userSyncService := sync.ProvideUserSync(userService, userProtectionService, authInfoService, quotaService) orgUserSyncService := sync.ProvideOrgSync(userService, orgService, accessControlService) s.RegisterPostAuthHook(userSyncService.SyncUser) s.RegisterPostAuthHook(orgUserSyncService.SyncOrgUser) diff --git a/pkg/services/authn/authnimpl/usersync/usersync.go b/pkg/services/authn/authnimpl/usersync/usersync.go index e200e0d1ef3..0736cc2891a 100644 --- a/pkg/services/authn/authnimpl/usersync/usersync.go +++ b/pkg/services/authn/authnimpl/usersync/usersync.go @@ -12,23 +12,37 @@ 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/util/errutil" ) -func ProvideUserSync(userService user.Service, authInfoService login.AuthInfoService, quotaService quota.Service) *UserSync { - return &UserSync{userService, authInfoService, quotaService, log.New("user.sync")} +var ( + errUserProtection = errutil.NewBase(errutil.StatusForbidden, + "user.sync.protected role", errutil.WithPublicMessage("Unable to sync due to protected role")) +) + +func ProvideUserSync(userService user.Service, + userProtectionService login.UserProtectionService, + authInfoService login.AuthInfoService, quotaService quota.Service) *UserSync { + return &UserSync{ + userService: userService, + authInfoService: authInfoService, + userProtectionService: userProtectionService, + quotaService: quotaService, + log: log.New("user.sync"), + } } type UserSync struct { - userService user.Service - authInfoService login.AuthInfoService - quotaService quota.Service - log log.Logger + userService user.Service + authInfoService login.AuthInfoService + userProtectionService login.UserProtectionService + quotaService quota.Service + log log.Logger } // SyncUser syncs a user with the database func (s *UserSync) SyncUser(ctx context.Context, id *authn.Identity, _ *authn.Request) error { if !id.ClientParams.SyncUser { - s.log.Debug("Not syncing user", "auth_module", id.AuthModule, "auth_id", id.AuthID) return nil } @@ -67,6 +81,10 @@ func (s *UserSync) SyncUser(ctx context.Context, id *authn.Identity, _ *authn.Re } } + if errProtection := s.userProtectionService.AllowUserMapping(usr, id.AuthModule); errProtection != nil { + return errUserProtection.Errorf("user mapping not allowed: %w", errProtection) + } + // update user if errUpdate := s.updateUserAttributes(ctx, usr, id); errUpdate != nil { return errUpdate diff --git a/pkg/services/authn/authnimpl/usersync/usersync_test.go b/pkg/services/authn/authnimpl/usersync/usersync_test.go index 634f6b3c93d..f836bd01071 100644 --- a/pkg/services/authn/authnimpl/usersync/usersync_test.go +++ b/pkg/services/authn/authnimpl/usersync/usersync_test.go @@ -4,10 +4,10 @@ import ( "context" "testing" - "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/authn" "github.com/grafana/grafana/pkg/services/login" + "github.com/grafana/grafana/pkg/services/login/authinfoservice" "github.com/grafana/grafana/pkg/services/login/logintest" "github.com/grafana/grafana/pkg/services/quota" "github.com/grafana/grafana/pkg/services/quota/quotatest" @@ -29,6 +29,8 @@ func ptrInt64(i int64) *int64 { } func TestUserSync_SyncUser(t *testing.T) { + userProtection := &authinfoservice.OSSUserProtectionImpl{} + authFakeNil := &logintest.AuthInfoServiceFake{ ExpectedUser: nil, ExpectedError: user.ErrUserNotFound, @@ -82,7 +84,6 @@ func TestUserSync_SyncUser(t *testing.T) { userService user.Service authInfoService login.AuthInfoService quotaService quota.Service - log log.Logger } type args struct { ctx context.Context @@ -101,7 +102,6 @@ func TestUserSync_SyncUser(t *testing.T) { userService: userService, authInfoService: authFakeNil, quotaService: "atest.FakeQuotaService{}, - log: log.NewNopLogger(), }, args: args{ ctx: context.Background(), @@ -140,7 +140,6 @@ func TestUserSync_SyncUser(t *testing.T) { userService: userService, authInfoService: authFakeNil, quotaService: "atest.FakeQuotaService{}, - log: log.NewNopLogger(), }, args: args{ ctx: context.Background(), @@ -182,7 +181,6 @@ func TestUserSync_SyncUser(t *testing.T) { userService: userService, authInfoService: authFakeNil, quotaService: "atest.FakeQuotaService{}, - log: log.NewNopLogger(), }, args: args{ ctx: context.Background(), @@ -224,7 +222,6 @@ func TestUserSync_SyncUser(t *testing.T) { userService: userService, authInfoService: authFakeNil, quotaService: "atest.FakeQuotaService{}, - log: log.NewNopLogger(), }, args: args{ ctx: context.Background(), @@ -266,7 +263,6 @@ func TestUserSync_SyncUser(t *testing.T) { userService: userService, authInfoService: authFakeUserID, quotaService: "atest.FakeQuotaService{}, - log: log.NewNopLogger(), }, args: args{ ctx: context.Background(), @@ -309,7 +305,6 @@ func TestUserSync_SyncUser(t *testing.T) { userService: userService, authInfoService: authFakeNil, quotaService: "atest.FakeQuotaService{}, - log: log.NewNopLogger(), }, args: args{ ctx: context.Background(), @@ -338,7 +333,6 @@ func TestUserSync_SyncUser(t *testing.T) { userService: userServiceNil, authInfoService: authFakeNil, quotaService: "atest.FakeQuotaService{}, - log: log.NewNopLogger(), }, args: args{ ctx: context.Background(), @@ -389,7 +383,6 @@ func TestUserSync_SyncUser(t *testing.T) { userService: userServiceMod, authInfoService: authFakeNil, quotaService: "atest.FakeQuotaService{}, - log: log.NewNopLogger(), }, args: args{ ctx: context.Background(), @@ -433,12 +426,7 @@ func TestUserSync_SyncUser(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - s := &UserSync{ - userService: tt.fields.userService, - authInfoService: tt.fields.authInfoService, - quotaService: tt.fields.quotaService, - log: tt.fields.log, - } + s := ProvideUserSync(tt.fields.userService, userProtection, tt.fields.authInfoService, tt.fields.quotaService) err := s.SyncUser(tt.args.ctx, tt.args.id, nil) if tt.wantErr { require.Error(t, err)