From c9492585b586ae936080641bd302743a06c3aadb Mon Sep 17 00:00:00 2001 From: Jguer Date: Tue, 28 Jun 2022 14:38:19 +0000 Subject: [PATCH] Fix: Choose Lookup params per auth module v8.3.x (#401) * Fix: Choose Lookup params per auth module Co-authored-by: Karl Persson Fix: Prefer pointer to struct in lookup Co-authored-by: Karl Persson Fix: user email for ldap Co-authored-by: Karl Persson Fix: Use only login for lookup in LDAP Co-authored-by: Karl Persson Fix: use user email for ldap Co-authored-by: Karl Persson fix remaining test fix nit picks * update lock * fix integration tests --- pkg/api/ldap_debug.go | 5 +++ pkg/api/login_oauth.go | 5 +++ pkg/login/ldap_login.go | 8 +++- pkg/models/user_auth.go | 19 +++++---- .../contexthandler/authproxy/authproxy.go | 10 +++++ pkg/services/login/authinfoservice/service.go | 37 +++++++++-------- .../login/authinfoservice/user_auth_test.go | 41 +++++++++++++------ .../login/loginservice/loginservice.go | 8 ++-- .../login/loginservice/loginservice_test.go | 6 ++- yarn.lock | 40 +++++++++--------- 10 files changed, 113 insertions(+), 66 deletions(-) diff --git a/pkg/api/ldap_debug.go b/pkg/api/ldap_debug.go index bba65c70257..aff5ef20b78 100644 --- a/pkg/api/ldap_debug.go +++ b/pkg/api/ldap_debug.go @@ -217,6 +217,11 @@ func (hs *HTTPServer) PostSyncUserWithLDAP(c *models.ReqContext) response.Respon ReqContext: c, ExternalUser: user, SignupAllowed: hs.Cfg.LDAPAllowSignup, + UserLookupParams: models.UserLookupParams{ + UserID: &query.Result.Id, // Upsert by ID only + Email: nil, + Login: nil, + }, } err = bus.Dispatch(upsertCmd) diff --git a/pkg/api/login_oauth.go b/pkg/api/login_oauth.go index 436d8878e21..d35e797d694 100644 --- a/pkg/api/login_oauth.go +++ b/pkg/api/login_oauth.go @@ -310,6 +310,11 @@ func syncUser( ReqContext: ctx, ExternalUser: extUser, SignupAllowed: connect.IsSignupAllowed(), + UserLookupParams: models.UserLookupParams{ + Email: &extUser.Email, + UserID: nil, + Login: nil, + }, } if err := bus.Dispatch(cmd); err != nil { return nil, err diff --git a/pkg/login/ldap_login.go b/pkg/login/ldap_login.go index 0cf5e6d8c7a..93398841e96 100644 --- a/pkg/login/ldap_login.go +++ b/pkg/login/ldap_login.go @@ -57,9 +57,13 @@ var loginUsingLDAP = func(ctx context.Context, query *models.LoginUserQuery) (bo ReqContext: query.ReqContext, ExternalUser: externalUser, SignupAllowed: setting.LDAPAllowSignup, + UserLookupParams: models.UserLookupParams{ + Login: &externalUser.Login, + Email: &externalUser.Email, + UserID: nil, + }, } - err = bus.DispatchCtx(ctx, upsert) - if err != nil { + if err = bus.DispatchCtx(ctx, upsert); err != nil { return true, err } query.User = upsert.Result diff --git a/pkg/models/user_auth.go b/pkg/models/user_auth.go index 6a96ccb6bad..2f670031b36 100644 --- a/pkg/models/user_auth.go +++ b/pkg/models/user_auth.go @@ -54,11 +54,11 @@ type RequestURIKey struct{} // COMMANDS type UpsertUserCommand struct { - ReqContext *ReqContext - ExternalUser *ExternalUserInfo + ReqContext *ReqContext + ExternalUser *ExternalUserInfo + UserLookupParams + Result *User SignupAllowed bool - - Result *User } type SetAuthInfoCommand struct { @@ -95,9 +95,14 @@ type LoginUserQuery struct { type GetUserByAuthInfoQuery struct { AuthModule string AuthId string - UserId int64 - Email string - Login string + UserLookupParams +} + +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 } type GetExternalUserInfoByLoginQuery struct { diff --git a/pkg/services/contexthandler/authproxy/authproxy.go b/pkg/services/contexthandler/authproxy/authproxy.go index adc99ce7eb6..f6642fce037 100644 --- a/pkg/services/contexthandler/authproxy/authproxy.go +++ b/pkg/services/contexthandler/authproxy/authproxy.go @@ -247,6 +247,11 @@ func (auth *AuthProxy) LoginViaLDAP() (int64, error) { ReqContext: auth.ctx, SignupAllowed: auth.cfg.LDAPAllowSignup, ExternalUser: extUser, + UserLookupParams: models.UserLookupParams{ + Login: &extUser.Login, + Email: &extUser.Email, + UserID: nil, + }, } if err := bus.Dispatch(upsert); err != nil { return 0, err @@ -303,6 +308,11 @@ func (auth *AuthProxy) LoginViaHeader() (int64, error) { ReqContext: auth.ctx, SignupAllowed: auth.cfg.AuthProxyAutoSignUp, ExternalUser: extUser, + UserLookupParams: models.UserLookupParams{ + UserID: nil, + Login: &extUser.Login, + Email: &extUser.Email, + }, } err := bus.Dispatch(upsert) diff --git a/pkg/services/login/authinfoservice/service.go b/pkg/services/login/authinfoservice/service.go index 096dd796832..ac64fc2ea33 100644 --- a/pkg/services/login/authinfoservice/service.go +++ b/pkg/services/login/authinfoservice/service.go @@ -85,11 +85,12 @@ func (s *Implementation) LookupAndFix(query *models.GetUserByAuthInfoQuery) (boo } // if user id was specified and doesn't match the user_auth entry, remove it - if query.UserId != 0 && query.UserId != authQuery.Result.UserId { - err := s.DeleteAuthInfo(&models.DeleteAuthInfoCommand{ + if query.UserLookupParams.UserID != nil && + *query.UserLookupParams.UserID != 0 && + *query.UserLookupParams.UserID != authQuery.Result.UserId { + if err := s.DeleteAuthInfo(&models.DeleteAuthInfoCommand{ UserAuth: authQuery.Result, - }) - if err != nil { + }); err != nil { s.logger.Error("Error removing user_auth entry", "error", err) } @@ -120,42 +121,42 @@ func (s *Implementation) LookupAndFix(query *models.GetUserByAuthInfoQuery) (boo return false, nil, nil, models.ErrUserNotFound } -func (s *Implementation) LookupByOneOf(userId int64, email string, login string) (bool, *models.User, error) { - foundUser := false +func (s *Implementation) LookupByOneOf(params *models.UserLookupParams) (*models.User, error) { var user *models.User var err error + foundUser := false // If not found, try to find the user by id - if userId != 0 { - foundUser, user, err = s.getUserById(userId) + if params.UserID != nil && *params.UserID != 0 { + foundUser, user, err = s.getUserById(*params.UserID) if err != nil { - return false, nil, err + return nil, err } } // If not found, try to find the user by email address - if !foundUser && email != "" { - user = &models.User{Email: email} + if !foundUser && params.Email != nil && *params.Email != "" { + user = &models.User{Email: *params.Email} foundUser, err = s.getUser(user) if err != nil { - return false, nil, err + return nil, err } } // If not found, try to find the user by login - if !foundUser && login != "" { - user = &models.User{Login: login} + if !foundUser && params.Login != nil && *params.Login != "" { + user = &models.User{Login: *params.Login} foundUser, err = s.getUser(user) if err != nil { - return false, nil, err + return nil, err } } if !foundUser { - return false, nil, models.ErrUserNotFound + return nil, models.ErrUserNotFound } - return foundUser, user, nil + return user, nil } func (s *Implementation) GenericOAuthLookup(authModule string, authId string, userID int64) (*models.UserAuth, error) { @@ -184,7 +185,7 @@ func (s *Implementation) LookupAndUpdate(query *models.GetUserByAuthInfoQuery) ( // 2. FindByUserDetails if !foundUser { - _, user, err = s.LookupByOneOf(query.UserId, query.Email, query.Login) + user, err = s.LookupByOneOf(&query.UserLookupParams) if err != nil { return nil, err } diff --git a/pkg/services/login/authinfoservice/user_auth_test.go b/pkg/services/login/authinfoservice/user_auth_test.go index bba765b3fb9..880a964a219 100644 --- a/pkg/services/login/authinfoservice/user_auth_test.go +++ b/pkg/services/login/authinfoservice/user_auth_test.go @@ -39,7 +39,7 @@ func TestUserAuth(t *testing.T) { // By Login login := "loginuser0" - query := &models.GetUserByAuthInfoQuery{Login: login} + query := &models.GetUserByAuthInfoQuery{UserLookupParams: models.UserLookupParams{Login: &login}} user, err := srv.LookupAndUpdate(query) require.Nil(t, err) @@ -48,7 +48,9 @@ func TestUserAuth(t *testing.T) { // By ID id := user.Id - _, user, err = srv.LookupByOneOf(id, "", "") + user, err = srv.LookupByOneOf(&models.UserLookupParams{ + UserID: &id, + }) require.Nil(t, err) require.Equal(t, user.Id, id) @@ -56,7 +58,9 @@ func TestUserAuth(t *testing.T) { // By Email email := "user1@test.com" - _, user, err = srv.LookupByOneOf(0, email, "") + user, err = srv.LookupByOneOf(&models.UserLookupParams{ + Email: &email, + }) require.Nil(t, err) require.Equal(t, user.Email, email) @@ -64,7 +68,9 @@ func TestUserAuth(t *testing.T) { // Don't find nonexistent user email = "nonexistent@test.com" - _, user, err = srv.LookupByOneOf(0, email, "") + user, err = srv.LookupByOneOf(&models.UserLookupParams{ + Email: &email, + }) require.Equal(t, models.ErrUserNotFound, err) require.Nil(t, user) @@ -81,7 +87,7 @@ func TestUserAuth(t *testing.T) { // create user_auth entry login := "loginuser0" - query.Login = login + query.UserLookupParams.Login = &login user, err = srv.LookupAndUpdate(query) require.Nil(t, err) @@ -95,9 +101,9 @@ func TestUserAuth(t *testing.T) { require.Equal(t, user.Login, login) // get with non-matching id - id := user.Id + idPlusOne := user.Id + 1 - query.UserId = id + 1 + query.UserLookupParams.UserID = &idPlusOne user, err = srv.LookupAndUpdate(query) require.Nil(t, err) @@ -138,7 +144,9 @@ func TestUserAuth(t *testing.T) { login := "loginuser0" // Calling GetUserByAuthInfoQuery on an existing user will populate an entry in the user_auth table - query := &models.GetUserByAuthInfoQuery{Login: login, AuthModule: "test", AuthId: "test"} + query := &models.GetUserByAuthInfoQuery{AuthModule: "test", AuthId: "test", UserLookupParams: models.UserLookupParams{ + Login: &login, + }} user, err := srv.LookupAndUpdate(query) require.Nil(t, err) @@ -186,7 +194,9 @@ func TestUserAuth(t *testing.T) { // Calling srv.LookupAndUpdateQuery on an existing user will populate an entry in the user_auth table // Make the first log-in during the past getTime = func() time.Time { return time.Now().AddDate(0, 0, -2) } - query := &models.GetUserByAuthInfoQuery{Login: login, AuthModule: "test1", AuthId: "test1"} + query := &models.GetUserByAuthInfoQuery{AuthModule: "test1", AuthId: "test1", UserLookupParams: models.UserLookupParams{ + Login: &login, + }} user, err := srv.LookupAndUpdate(query) getTime = time.Now @@ -196,7 +206,9 @@ func TestUserAuth(t *testing.T) { // Add a second auth module for this user // Have this module's last log-in be more recent getTime = func() time.Time { return time.Now().AddDate(0, 0, -1) } - query = &models.GetUserByAuthInfoQuery{Login: login, AuthModule: "test2", AuthId: "test2"} + query = &models.GetUserByAuthInfoQuery{AuthModule: "test2", AuthId: "test2", UserLookupParams: models.UserLookupParams{ + Login: &login, + }} user, err = srv.LookupAndUpdate(query) getTime = time.Now @@ -236,16 +248,21 @@ func TestUserAuth(t *testing.T) { // Expect to pass since there's a matching login user getTime = func() time.Time { return time.Now().AddDate(0, 0, -2) } - query := &models.GetUserByAuthInfoQuery{Login: login, AuthModule: genericOAuthModule, AuthId: ""} + query := &models.GetUserByAuthInfoQuery{AuthModule: genericOAuthModule, AuthId: "", UserLookupParams: models.UserLookupParams{ + Login: &login, + }} user, err := srv.LookupAndUpdate(query) getTime = time.Now require.Nil(t, err) require.Equal(t, user.Login, login) + otherLoginUser := "aloginuser" // Should throw a "user not found" error since there's no matching login user getTime = func() time.Time { return time.Now().AddDate(0, 0, -2) } - query = &models.GetUserByAuthInfoQuery{Login: "aloginuser", AuthModule: genericOAuthModule, AuthId: ""} + query = &models.GetUserByAuthInfoQuery{AuthModule: genericOAuthModule, AuthId: "", UserLookupParams: models.UserLookupParams{ + Login: &otherLoginUser, + }} user, err = srv.LookupAndUpdate(query) getTime = time.Now diff --git a/pkg/services/login/loginservice/loginservice.go b/pkg/services/login/loginservice/loginservice.go index 4b05a81d6e7..41aecd1e309 100644 --- a/pkg/services/login/loginservice/loginservice.go +++ b/pkg/services/login/loginservice/loginservice.go @@ -45,11 +45,9 @@ func (ls *Implementation) UpsertUser(cmd *models.UpsertUserCommand) error { extUser := cmd.ExternalUser user, err := ls.AuthInfoService.LookupAndUpdate(&models.GetUserByAuthInfoQuery{ - AuthModule: extUser.AuthModule, - AuthId: extUser.AuthId, - UserId: extUser.UserId, - Email: extUser.Email, - Login: extUser.Login, + AuthModule: extUser.AuthModule, + AuthId: extUser.AuthId, + UserLookupParams: cmd.UserLookupParams, }) if err != nil { if !errors.Is(err, models.ErrUserNotFound) { diff --git a/pkg/services/login/loginservice/loginservice_test.go b/pkg/services/login/loginservice/loginservice_test.go index 573aebe1cba..bde2f74461f 100644 --- a/pkg/services/login/loginservice/loginservice_test.go +++ b/pkg/services/login/loginservice/loginservice_test.go @@ -94,10 +94,12 @@ func Test_teamSync(t *testing.T) { AuthInfoService: authInfoMock, } - upserCmd := &models.UpsertUserCommand{ExternalUser: &models.ExternalUserInfo{Email: "test_user@example.org"}} + email := "test_user@example.org" + upserCmd := &models.UpsertUserCommand{ExternalUser: &models.ExternalUserInfo{Email: email}, + UserLookupParams: models.UserLookupParams{Email: &email}} expectedUser := &models.User{ Id: 1, - Email: "test_user@example.org", + Email: email, Name: "test_user", Login: "test_user", } diff --git a/yarn.lock b/yarn.lock index eb4bb2eeefd..fe0fbd18220 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2329,9 +2329,9 @@ __metadata: version: 0.0.0-use.local resolution: "@grafana-plugins/input-datasource@workspace:plugins-bundled/internal/input-datasource" dependencies: - "@grafana/data": 8.3.7 - "@grafana/toolkit": 8.3.7 - "@grafana/ui": 8.3.7 + "@grafana/data": 8.3.8 + "@grafana/toolkit": 8.3.8 + "@grafana/ui": 8.3.8 "@types/jest": 26.0.15 "@types/lodash": 4.14.149 "@types/react": 17.0.30 @@ -2372,12 +2372,12 @@ __metadata: languageName: node linkType: hard -"@grafana/data@8.3.7, @grafana/data@workspace:*, @grafana/data@workspace:packages/grafana-data": +"@grafana/data@8.3.8, @grafana/data@workspace:*, @grafana/data@workspace:packages/grafana-data": version: 0.0.0-use.local resolution: "@grafana/data@workspace:packages/grafana-data" dependencies: "@braintree/sanitize-url": 5.0.2 - "@grafana/schema": 8.3.7 + "@grafana/schema": 8.3.8 "@grafana/tsconfig": ^1.0.0-rc1 "@rollup/plugin-commonjs": 21.0.1 "@rollup/plugin-json": 4.1.0 @@ -2432,7 +2432,7 @@ __metadata: languageName: unknown linkType: soft -"@grafana/e2e-selectors@8.3.7, @grafana/e2e-selectors@workspace:*, @grafana/e2e-selectors@workspace:packages/grafana-e2e-selectors": +"@grafana/e2e-selectors@8.3.8, @grafana/e2e-selectors@workspace:*, @grafana/e2e-selectors@workspace:packages/grafana-e2e-selectors": version: 0.0.0-use.local resolution: "@grafana/e2e-selectors@workspace:packages/grafana-e2e-selectors" dependencies: @@ -2464,7 +2464,7 @@ __metadata: "@babel/core": 7.14.6 "@babel/preset-env": 7.14.7 "@cypress/webpack-preprocessor": 5.9.1 - "@grafana/e2e-selectors": 8.3.7 + "@grafana/e2e-selectors": 8.3.8 "@grafana/tsconfig": ^1.0.0-rc1 "@mochajs/json-file-reporter": ^1.2.0 "@rollup/plugin-commonjs": 21.0.1 @@ -2522,10 +2522,10 @@ __metadata: resolution: "@grafana/runtime@workspace:packages/grafana-runtime" dependencies: "@emotion/css": 11.1.3 - "@grafana/data": 8.3.7 - "@grafana/e2e-selectors": 8.3.7 + "@grafana/data": 8.3.8 + "@grafana/e2e-selectors": 8.3.8 "@grafana/tsconfig": ^1.0.0-rc1 - "@grafana/ui": 8.3.7 + "@grafana/ui": 8.3.8 "@rollup/plugin-commonjs": 21.0.1 "@rollup/plugin-node-resolve": 13.0.6 "@sentry/browser": 5.25.0 @@ -2557,7 +2557,7 @@ __metadata: languageName: unknown linkType: soft -"@grafana/schema@8.3.7, @grafana/schema@workspace:*, @grafana/schema@workspace:packages/grafana-schema": +"@grafana/schema@8.3.8, @grafana/schema@workspace:*, @grafana/schema@workspace:packages/grafana-schema": version: 0.0.0-use.local resolution: "@grafana/schema@workspace:packages/grafana-schema" dependencies: @@ -2607,16 +2607,16 @@ __metadata: languageName: node linkType: hard -"@grafana/toolkit@8.3.7, @grafana/toolkit@workspace:*, @grafana/toolkit@workspace:packages/grafana-toolkit": +"@grafana/toolkit@8.3.8, @grafana/toolkit@workspace:*, @grafana/toolkit@workspace:packages/grafana-toolkit": version: 0.0.0-use.local resolution: "@grafana/toolkit@workspace:packages/grafana-toolkit" dependencies: "@babel/core": 7.13.14 "@babel/preset-env": 7.13.12 - "@grafana/data": 8.3.7 + "@grafana/data": 8.3.8 "@grafana/eslint-config": 2.5.1 "@grafana/tsconfig": ^1.0.0-rc1 - "@grafana/ui": 8.3.7 + "@grafana/ui": 8.3.8 "@jest/core": 26.6.3 "@rushstack/eslint-patch": 1.0.6 "@types/command-exists": ^1.2.0 @@ -2700,7 +2700,7 @@ __metadata: languageName: node linkType: hard -"@grafana/ui@8.3.7, @grafana/ui@workspace:*, @grafana/ui@workspace:packages/grafana-ui": +"@grafana/ui@8.3.8, @grafana/ui@workspace:*, @grafana/ui@workspace:packages/grafana-ui": version: 0.0.0-use.local resolution: "@grafana/ui@workspace:packages/grafana-ui" dependencies: @@ -2708,9 +2708,9 @@ __metadata: "@emotion/css": 11.1.3 "@emotion/react": 11.1.5 "@grafana/aws-sdk": 0.0.3 - "@grafana/data": 8.3.7 - "@grafana/e2e-selectors": 8.3.7 - "@grafana/schema": 8.3.7 + "@grafana/data": 8.3.8 + "@grafana/e2e-selectors": 8.3.8 + "@grafana/schema": 8.3.8 "@grafana/slate-react": 0.22.10-grafana "@grafana/tsconfig": ^1.0.0-rc1 "@mdx-js/react": 1.6.22 @@ -2923,9 +2923,9 @@ __metadata: dependencies: "@emotion/css": 11.1.3 "@emotion/react": 11.1.5 - "@grafana/data": 8.3.7 + "@grafana/data": 8.3.8 "@grafana/tsconfig": ^1.0.0-rc1 - "@grafana/ui": 8.3.7 + "@grafana/ui": 8.3.8 "@types/classnames": ^2.2.7 "@types/deep-freeze": ^0.1.1 "@types/grafana__slate-react": "npm:@types/slate-react@0.22.5"