Skip to content

Commit

Permalink
Fix: Choose Lookup params per auth module (#399)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@Jguer
Jguer committed Jun 28, 2022
1 parent f6d351b commit 967e17d
Show file tree
Hide file tree
Showing 11 changed files with 100 additions and 47 deletions.
5 changes: 5 additions & 0 deletions pkg/api/ldap_debug.go
View file
Expand Up @@ -221,6 +221,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(c.Req.Context(), upsertCmd)
Expand Down
5 changes: 5 additions & 0 deletions pkg/api/login_oauth.go
View file
Expand Up @@ -313,6 +313,11 @@ func syncUser(
ReqContext: ctx,
ExternalUser: extUser,
SignupAllowed: connect.IsSignupAllowed(),
UserLookupParams: models.UserLookupParams{
Email: &extUser.Email,
UserID: nil,
Login: nil,
},
}
if err := bus.Dispatch(ctx.Req.Context(), cmd); err != nil {
return nil, err
Expand Down
3 changes: 2 additions & 1 deletion pkg/api/user_test.go
View file
Expand Up @@ -67,7 +67,8 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) {
}
idToken := "testidtoken"
token = token.WithExtra(map[string]interface{}{"id_token": idToken})
query := &models.GetUserByAuthInfoQuery{Login: "loginuser", AuthModule: "test", AuthId: "test"}
login := "loginuser"
query := &models.GetUserByAuthInfoQuery{AuthModule: "test", AuthId: "test", UserLookupParams: models.UserLookupParams{Login: &login}}
cmd := &models.UpdateAuthInfoCommand{
UserId: user.Id,
AuthId: query.AuthId,
Expand Down
8 changes: 6 additions & 2 deletions pkg/login/ldap_login.go
View file
Expand Up @@ -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.Dispatch(ctx, upsert)
if err != nil {
if err = bus.Dispatch(ctx, upsert); err != nil {
return true, err
}
query.User = upsert.Result
Expand Down
19 changes: 12 additions & 7 deletions pkg/models/user_auth.go
View file
Expand Up @@ -55,11 +55,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 {
Expand Down Expand Up @@ -96,9 +96,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 {
Expand Down
5 changes: 5 additions & 0 deletions pkg/services/contexthandler/auth_jwt.go
View file
Expand Up @@ -66,6 +66,11 @@ func (h *ContextHandler) initContextWithJWT(ctx *models.ReqContext, orgId int64)
ReqContext: ctx,
SignupAllowed: h.Cfg.JWTAuthAutoSignUp,
ExternalUser: extUser,
UserLookupParams: models.UserLookupParams{
UserID: nil,
Login: &query.Login,
Email: &query.Email,
},
}
if err := bus.Dispatch(ctx.Req.Context(), upsert); err != nil {
ctx.Logger.Error("Failed to upsert JWT user", "error", err)
Expand Down
10 changes: 10 additions & 0 deletions pkg/services/contexthandler/authproxy/authproxy.go
View file
Expand Up @@ -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(auth.ctx.Req.Context(), upsert); err != nil {
return 0, err
Expand Down Expand Up @@ -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(auth.ctx.Req.Context(), upsert)
Expand Down
37 changes: 19 additions & 18 deletions pkg/services/login/authinfoservice/service.go
View file
Expand Up @@ -89,11 +89,12 @@ func (s *Implementation) LookupAndFix(ctx context.Context, query *models.GetUser
}

// 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(ctx, &models.DeleteAuthInfoCommand{
if query.UserLookupParams.UserID != nil &&
*query.UserLookupParams.UserID != 0 &&
*query.UserLookupParams.UserID != authQuery.Result.UserId {
if err := s.DeleteAuthInfo(ctx, &models.DeleteAuthInfoCommand{
UserAuth: authQuery.Result,
})
if err != nil {
}); err != nil {
s.logger.Error("Error removing user_auth entry", "error", err)
}

Expand Down Expand Up @@ -124,42 +125,42 @@ func (s *Implementation) LookupAndFix(ctx context.Context, query *models.GetUser
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(ctx context.Context, 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(ctx context.Context, authModule string, authId string, userID int64) (*models.UserAuth, error) {
Expand Down Expand Up @@ -188,7 +189,7 @@ func (s *Implementation) LookupAndUpdate(ctx context.Context, query *models.GetU

// 2. FindByUserDetails
if !foundUser {
_, user, err = s.LookupByOneOf(query.UserId, query.Email, query.Login)
user, err = s.LookupByOneOf(ctx, &query.UserLookupParams)
if err != nil {
return nil, err
}
Expand Down
41 changes: 29 additions & 12 deletions pkg/services/login/authinfoservice/user_auth_test.go
View file
Expand Up @@ -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(context.Background(), query)

require.Nil(t, err)
Expand All @@ -48,23 +48,29 @@ func TestUserAuth(t *testing.T) {
// By ID
id := user.Id

_, user, err = srv.LookupByOneOf(id, "", "")
user, err = srv.LookupByOneOf(context.Background(), &models.UserLookupParams{
UserID: &id,
})

require.Nil(t, err)
require.Equal(t, user.Id, id)

// By Email
email := "user1@test.com"

_, user, err = srv.LookupByOneOf(0, email, "")
user, err = srv.LookupByOneOf(context.Background(), &models.UserLookupParams{
Email: &email,
})

require.Nil(t, err)
require.Equal(t, user.Email, email)

// Don't find nonexistent user
email = "nonexistent@test.com"

_, user, err = srv.LookupByOneOf(0, email, "")
user, err = srv.LookupByOneOf(context.Background(), &models.UserLookupParams{
Email: &email,
})

require.Equal(t, models.ErrUserNotFound, err)
require.Nil(t, user)
Expand All @@ -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(context.Background(), query)

require.Nil(t, err)
Expand All @@ -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(context.Background(), query)

require.Nil(t, err)
Expand Down Expand Up @@ -140,7 +146,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(context.Background(), query)

require.Nil(t, err)
Expand Down Expand Up @@ -189,7 +197,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(context.Background(), query)
getTime = time.Now

Expand All @@ -199,7 +209,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(context.Background(), query)
getTime = time.Now

Expand Down Expand Up @@ -239,16 +251,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(context.Background(), 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(context.Background(), query)
getTime = time.Now

Expand Down
8 changes: 3 additions & 5 deletions pkg/services/login/loginservice/loginservice.go
View file
Expand Up @@ -45,11 +45,9 @@ func (ls *Implementation) UpsertUser(ctx context.Context, cmd *models.UpsertUser
extUser := cmd.ExternalUser

user, err := ls.AuthInfoService.LookupAndUpdate(ctx, &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) {
Expand Down
6 changes: 4 additions & 2 deletions pkg/services/login/loginservice/loginservice_test.go
View file
Expand Up @@ -93,10 +93,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",
}
Expand Down

0 comments on commit 967e17d

Please sign in to comment.