From 718994af4421d5ca5ad3a75d0b6cc98cad258e01 Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Fri, 7 Jun 2024 13:55:45 +0200 Subject: [PATCH] [v11.0.x] User: Fix GetByID (#88894) User: Fix GetByID (#86282) * Auth: Remove unused lookup param * Remove case sensitive lookup for GetByID (cherry picked from commit 852089292325895c24f0bfdb11af9e11f525b638) --- .../authn/authnimpl/sync/org_sync_test.go | 10 +- .../authn/authnimpl/sync/user_sync.go | 8 -- .../authn/authnimpl/sync/user_sync_test.go | 110 +++++------------- pkg/services/authn/clients/ext_jwt_test.go | 5 - pkg/services/authn/clients/grafana_test.go | 1 - pkg/services/authn/clients/jwt_test.go | 10 +- pkg/services/authn/clients/oauth_test.go | 1 - pkg/services/ldap/api/service.go | 3 - pkg/services/login/model.go | 5 +- pkg/services/user/userimpl/user.go | 9 +- 10 files changed, 37 insertions(+), 125 deletions(-) diff --git a/pkg/services/authn/authnimpl/sync/org_sync_test.go b/pkg/services/authn/authnimpl/sync/org_sync_test.go index b2083f3a3e4..93d46e8f823 100644 --- a/pkg/services/authn/authnimpl/sync/org_sync_test.go +++ b/pkg/services/authn/authnimpl/sync/org_sync_test.go @@ -84,9 +84,8 @@ func TestOrgSync_SyncOrgRolesHook(t *testing.T) { ClientParams: authn.ClientParams{ SyncOrgRoles: true, LookUpParams: login.UserLookupParams{ - UserID: nil, - Email: ptrString("test"), - Login: nil, + Email: ptrString("test"), + Login: nil, }, }, }, @@ -102,9 +101,8 @@ func TestOrgSync_SyncOrgRolesHook(t *testing.T) { ClientParams: authn.ClientParams{ SyncOrgRoles: true, LookUpParams: login.UserLookupParams{ - UserID: nil, - Email: ptrString("test"), - Login: nil, + Email: ptrString("test"), + Login: nil, }, }, }, diff --git a/pkg/services/authn/authnimpl/sync/user_sync.go b/pkg/services/authn/authnimpl/sync/user_sync.go index 6ba757b5615..4834288ef0f 100644 --- a/pkg/services/authn/authnimpl/sync/user_sync.go +++ b/pkg/services/authn/authnimpl/sync/user_sync.go @@ -353,14 +353,6 @@ func (s *UserSync) lookupByOneOf(ctx context.Context, params login.UserLookupPar var usr *user.User var err error - // If not found, try to find the user by id - if params.UserID != nil && *params.UserID != 0 { - usr, err = s.userService.GetByID(ctx, &user.GetUserByIDQuery{ID: *params.UserID}) - if err != nil && !errors.Is(err, user.ErrUserNotFound) { - return nil, err - } - } - // If not found, try to find the user by email address if usr == nil && params.Email != nil && *params.Email != "" { usr, err = s.userService.GetByEmail(ctx, &user.GetUserByEmailQuery{Email: *params.Email}) diff --git a/pkg/services/authn/authnimpl/sync/user_sync_test.go b/pkg/services/authn/authnimpl/sync/user_sync_test.go index b36033b286c..ec8ba6b6330 100644 --- a/pkg/services/authn/authnimpl/sync/user_sync_test.go +++ b/pkg/services/authn/authnimpl/sync/user_sync_test.go @@ -25,10 +25,6 @@ func ptrBool(b bool) *bool { return &b } -func ptrInt64(i int64) *int64 { - return &i -} - func TestUserSync_SyncUserHook(t *testing.T) { userProtection := &authinfoimpl.OSSUserProtectionImpl{} @@ -110,9 +106,8 @@ func TestUserSync_SyncUserHook(t *testing.T) { Email: "test", ClientParams: authn.ClientParams{ LookUpParams: login.UserLookupParams{ - UserID: nil, - Email: ptrString("test"), - Login: nil, + Email: ptrString("test"), + Login: nil, }, }, }, @@ -125,9 +120,8 @@ func TestUserSync_SyncUserHook(t *testing.T) { Email: "test", ClientParams: authn.ClientParams{ LookUpParams: login.UserLookupParams{ - UserID: nil, - Email: ptrString("test"), - Login: nil, + Email: ptrString("test"), + Login: nil, }, }, }, @@ -149,9 +143,8 @@ func TestUserSync_SyncUserHook(t *testing.T) { ClientParams: authn.ClientParams{ SyncUser: true, LookUpParams: login.UserLookupParams{ - UserID: nil, - Email: ptrString("test"), - Login: nil, + Email: ptrString("test"), + Login: nil, }, }, }, @@ -166,9 +159,8 @@ func TestUserSync_SyncUserHook(t *testing.T) { ClientParams: authn.ClientParams{ SyncUser: true, LookUpParams: login.UserLookupParams{ - UserID: nil, - Email: ptrString("test"), - Login: nil, + Email: ptrString("test"), + Login: nil, }, }, }, @@ -190,9 +182,8 @@ func TestUserSync_SyncUserHook(t *testing.T) { ClientParams: authn.ClientParams{ SyncUser: true, LookUpParams: login.UserLookupParams{ - UserID: nil, - Email: nil, - Login: ptrString("test"), + Email: nil, + Login: ptrString("test"), }, }, }, @@ -206,55 +197,13 @@ func TestUserSync_SyncUserHook(t *testing.T) { IsGrafanaAdmin: ptrBool(false), ClientParams: authn.ClientParams{ LookUpParams: login.UserLookupParams{ - UserID: nil, - Email: nil, - Login: ptrString("test"), + Email: nil, + Login: ptrString("test"), }, SyncUser: true, }, }, }, - { - name: "sync - user found in DB - by ID", - fields: fields{ - userService: userService, - authInfoService: authFakeNil, - quotaService: "atest.FakeQuotaService{}, - }, - args: args{ - ctx: context.Background(), - id: &authn.Identity{ - ID: "", - Login: "test", - Name: "test", - Email: "test", - ClientParams: authn.ClientParams{ - SyncUser: true, - LookUpParams: login.UserLookupParams{ - UserID: ptrInt64(1), - Email: nil, - Login: nil, - }, - }, - }, - }, - wantErr: false, - wantID: &authn.Identity{ - ID: "user:1", - Login: "test", - Name: "test", - Email: "test", - IsGrafanaAdmin: ptrBool(false), - ClientParams: authn.ClientParams{ - SyncUser: true, - LookUpParams: login.UserLookupParams{ - UserID: ptrInt64(1), - Email: nil, - Login: nil, - }, - }, - }, - }, { name: "sync - user found in authInfo", fields: fields{ @@ -274,9 +223,8 @@ func TestUserSync_SyncUserHook(t *testing.T) { ClientParams: authn.ClientParams{ SyncUser: true, LookUpParams: login.UserLookupParams{ - UserID: nil, - Email: nil, - Login: nil, + Email: nil, + Login: nil, }, }, }, @@ -293,9 +241,8 @@ func TestUserSync_SyncUserHook(t *testing.T) { ClientParams: authn.ClientParams{ SyncUser: true, LookUpParams: login.UserLookupParams{ - UserID: nil, - Email: nil, - Login: nil, + Email: nil, + Login: nil, }, }, }, @@ -319,9 +266,8 @@ func TestUserSync_SyncUserHook(t *testing.T) { ClientParams: authn.ClientParams{ SyncUser: true, LookUpParams: login.UserLookupParams{ - UserID: nil, - Email: nil, - Login: nil, + Email: nil, + Login: nil, }, }, }, @@ -350,9 +296,8 @@ func TestUserSync_SyncUserHook(t *testing.T) { AllowSignUp: true, EnableUser: true, LookUpParams: login.UserLookupParams{ - UserID: nil, - Email: ptrString("test_create"), - Login: nil, + Email: ptrString("test_create"), + Login: nil, }, }, }, @@ -371,9 +316,8 @@ func TestUserSync_SyncUserHook(t *testing.T) { AllowSignUp: true, EnableUser: true, LookUpParams: login.UserLookupParams{ - UserID: nil, - Email: ptrString("test_create"), - Login: nil, + Email: ptrString("test_create"), + Login: nil, }, }, }, @@ -398,9 +342,8 @@ func TestUserSync_SyncUserHook(t *testing.T) { SyncUser: true, EnableUser: true, LookUpParams: login.UserLookupParams{ - UserID: ptrInt64(3), - Email: nil, - Login: nil, + Email: nil, + Login: ptrString("test"), }, }, }, @@ -417,9 +360,8 @@ func TestUserSync_SyncUserHook(t *testing.T) { SyncUser: true, EnableUser: true, LookUpParams: login.UserLookupParams{ - UserID: ptrInt64(3), - Email: nil, - Login: nil, + Email: nil, + Login: ptrString("test"), }, }, }, diff --git a/pkg/services/authn/clients/ext_jwt_test.go b/pkg/services/authn/clients/ext_jwt_test.go index 64ff1c5095c..df43dd15e52 100644 --- a/pkg/services/authn/clients/ext_jwt_test.go +++ b/pkg/services/authn/clients/ext_jwt_test.go @@ -182,11 +182,6 @@ func TestExtendedJWT_Authenticate(t *testing.T) { SyncOrgRoles: false, SyncTeams: false, SyncPermissions: false, - LookUpParams: login.UserLookupParams{ - UserID: nil, - Email: nil, - Login: nil, - }, }, }, wantErr: false, diff --git a/pkg/services/authn/clients/grafana_test.go b/pkg/services/authn/clients/grafana_test.go index f77d233744f..b2cb45154ec 100644 --- a/pkg/services/authn/clients/grafana_test.go +++ b/pkg/services/authn/clients/grafana_test.go @@ -116,7 +116,6 @@ func TestGrafana_AuthenticateProxy(t *testing.T) { assert.EqualValues(t, tt.expectedIdentity.ClientParams.LookUpParams.Email, identity.ClientParams.LookUpParams.Email) assert.EqualValues(t, tt.expectedIdentity.ClientParams.LookUpParams.Login, identity.ClientParams.LookUpParams.Login) - assert.EqualValues(t, tt.expectedIdentity.ClientParams.LookUpParams.UserID, identity.ClientParams.LookUpParams.UserID) } else { assert.Nil(t, tt.expectedIdentity) } diff --git a/pkg/services/authn/clients/jwt_test.go b/pkg/services/authn/clients/jwt_test.go index d9476f37320..f5d0dbd09ad 100644 --- a/pkg/services/authn/clients/jwt_test.go +++ b/pkg/services/authn/clients/jwt_test.go @@ -57,9 +57,8 @@ func TestAuthenticateJWT(t *testing.T) { SyncPermissions: true, SyncTeams: true, LookUpParams: login.UserLookupParams{ - UserID: nil, - Email: stringPtr("eai.doe@cor.po"), - Login: stringPtr("eai-doe"), + Email: stringPtr("eai.doe@cor.po"), + Login: stringPtr("eai-doe"), }, }, }, @@ -111,9 +110,8 @@ func TestAuthenticateJWT(t *testing.T) { SyncPermissions: true, SyncTeams: false, LookUpParams: login.UserLookupParams{ - UserID: nil, - Email: stringPtr("eai.doe@cor.po"), - Login: stringPtr("eai-doe"), + Email: stringPtr("eai.doe@cor.po"), + Login: stringPtr("eai-doe"), }, }, }, diff --git a/pkg/services/authn/clients/oauth_test.go b/pkg/services/authn/clients/oauth_test.go index 7432ac2ad83..567608ce066 100644 --- a/pkg/services/authn/clients/oauth_test.go +++ b/pkg/services/authn/clients/oauth_test.go @@ -308,7 +308,6 @@ func TestOAuth_Authenticate(t *testing.T) { assert.EqualValues(t, tt.expectedIdentity.ClientParams.LookUpParams.Email, identity.ClientParams.LookUpParams.Email) assert.EqualValues(t, tt.expectedIdentity.ClientParams.LookUpParams.Login, identity.ClientParams.LookUpParams.Login) - assert.EqualValues(t, tt.expectedIdentity.ClientParams.LookUpParams.UserID, identity.ClientParams.LookUpParams.UserID) } else { assert.Nil(t, tt.expectedIdentity) } diff --git a/pkg/services/ldap/api/service.go b/pkg/services/ldap/api/service.go index 2d5de2a1da2..71c85c4fafe 100644 --- a/pkg/services/ldap/api/service.go +++ b/pkg/services/ldap/api/service.go @@ -331,9 +331,6 @@ func (s *Service) identityFromLDAPUser(user *login.ExternalUserInfo) *authn.Iden EnableUser: true, SyncOrgRoles: !s.cfg.LDAPSkipOrgRoleSync, AllowSignUp: s.cfg.LDAPAllowSignup, - LookUpParams: login.UserLookupParams{ - UserID: &user.UserId, - }, }, } } diff --git a/pkg/services/login/model.go b/pkg/services/login/model.go index 3ec4b55d99e..b11ec2161f3 100644 --- a/pkg/services/login/model.go +++ b/pkg/services/login/model.go @@ -104,9 +104,8 @@ type GetUserByAuthInfoQuery struct { type UserLookupParams struct { // Describes lookup order as well - UserID *int64 // if set, will try to find the user by id - Email *string // if set, will try to find the user by email - Login *string // if set, will try to find the user by login + Email *string // if set, will try to find the user by email + Login *string // if set, will try to find the user by login } type GetAuthInfoQuery struct { diff --git a/pkg/services/user/userimpl/user.go b/pkg/services/user/userimpl/user.go index 4dbe3e4114d..14c3be1ce7a 100644 --- a/pkg/services/user/userimpl/user.go +++ b/pkg/services/user/userimpl/user.go @@ -209,14 +209,7 @@ func (s *Service) Delete(ctx context.Context, cmd *user.DeleteUserCommand) error } func (s *Service) GetByID(ctx context.Context, query *user.GetUserByIDQuery) (*user.User, error) { - user, err := s.store.GetByID(ctx, query.ID) - if err != nil { - return nil, err - } - if err := s.store.CaseInsensitiveLoginConflict(ctx, user.Login, user.Email); err != nil { - return nil, err - } - return user, nil + return s.store.GetByID(ctx, query.ID) } func (s *Service) GetByLogin(ctx context.Context, query *user.GetUserByLoginQuery) (*user.User, error) {