public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix: Choose Lookup params per auth module v8.3.x (#401)

Browse Source 
* Fix: Choose Lookup params per auth module

Co-authored-by: Karl Persson <kalle.persson@grafana.com>

Fix: Prefer pointer to struct in lookup

Co-authored-by: Karl Persson <kalle.persson@grafana.com>

Fix: user email for ldap

Co-authored-by: Karl Persson <kalle.persson@grafana.com>

Fix: Use only login for lookup in LDAP

Co-authored-by: Karl Persson <kalle.persson@grafana.com>

Fix: use user email for ldap

Co-authored-by: Karl Persson <kalle.persson@grafana.com>

fix remaining test

fix nit picks

* update lock

* fix integration tests
This commit is contained in:
Jguer
2022-06-29 15:53:29 +03:00
committed by dsotirakis
co-authored by Karl Persson
parent 7f19190647
commit c9492585b5
10 changed files with 113 additions and 66 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/api/ldap_debug.go
+5
View File
@@ -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)
pkg/api/login_oauth.go
+5
View File
@@ -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
pkg/login/ldap_login.go
+6 -2
View File
@@ -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
pkg/models/user_auth.go
+12 -7
View File
@@ -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 {
pkg/services/contexthandler/authproxy/authproxy.go
+10
View File
@@ -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)
pkg/services/login/authinfoservice/service.go
+19 -18
View File
@@ -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
}
pkg/services/login/authinfoservice/user_auth_test.go
+29 -12
View File
@@ -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
pkg/services/login/loginservice/loginservice.go
+3 -5
View File
@@ -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) {
pkg/services/login/loginservice/loginservice_test.go
+4 -2
View File
@@ -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",
}
yarn.lock
+20 -20
View File
@@ -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"