From 86962dc9bdd97f2be4832cb770db4a46cb915bf4 Mon Sep 17 00:00:00 2001 From: Jguer Date: Thu, 19 May 2022 10:03:04 +0000 Subject: [PATCH] Login: Fix mismatching label on auth_module in user list (#49177) * Login: Fix mismatching label on user auth_module * Login: ensure previous auth was set to differing * Login: ensure only one entry of auth is updated * compare both entries Co-authored-by: Gabriel MABILLE * remove noop Co-authored-by: Gabriel MABILLE --- .../authinfoservice/database/database.go | 16 ++++ pkg/services/login/authinfoservice/service.go | 22 ++++-- .../login/authinfoservice/user_auth_test.go | 79 +++++++++++++++++++ pkg/services/login/userprotection.go | 1 + 4 files changed, 110 insertions(+), 8 deletions(-) diff --git a/pkg/services/login/authinfoservice/database/database.go b/pkg/services/login/authinfoservice/database/database.go index efd4f2cadac..b2c2ea070bc 100644 --- a/pkg/services/login/authinfoservice/database/database.go +++ b/pkg/services/login/authinfoservice/database/database.go @@ -146,6 +146,22 @@ func (s *AuthInfoStore) SetAuthInfo(ctx context.Context, cmd *models.SetAuthInfo }) } +// UpdateAuthInfo updates the auth info for the user with the latest date. Avoids overlapping entries hiding +// the last used one (ex: LDAP->SAML->LDAP) +func (s *AuthInfoStore) UpdateAuthInfoDate(ctx context.Context, authInfo *models.UserAuth) error { + authInfo.Created = GetTime() + + cond := &models.UserAuth{ + Id: authInfo.Id, + UserId: authInfo.UserId, + AuthModule: authInfo.AuthModule, + } + return s.sqlStore.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { + _, err := sess.Update(authInfo, cond) + return err + }) +} + func (s *AuthInfoStore) UpdateAuthInfo(ctx context.Context, cmd *models.UpdateAuthInfoCommand) error { authUser := &models.UserAuth{ UserId: cmd.UserId, diff --git a/pkg/services/login/authinfoservice/service.go b/pkg/services/login/authinfoservice/service.go index bf61cbcf510..8fc26ab7b10 100644 --- a/pkg/services/login/authinfoservice/service.go +++ b/pkg/services/login/authinfoservice/service.go @@ -158,14 +158,20 @@ func (s *Implementation) LookupAndUpdate(ctx context.Context, query *models.GetU authInfo = ai } - if authInfo == nil && query.AuthModule != "" { - cmd := &models.SetAuthInfoCommand{ - UserId: user.Id, - AuthModule: query.AuthModule, - AuthId: query.AuthId, - } - if err := s.authInfoStore.SetAuthInfo(ctx, cmd); err != nil { - return nil, err + if query.AuthModule != "" { + if authInfo == nil { + cmd := &models.SetAuthInfoCommand{ + UserId: user.Id, + AuthModule: query.AuthModule, + AuthId: query.AuthId, + } + if err := s.authInfoStore.SetAuthInfo(ctx, cmd); err != nil { + return nil, err + } + } else { + if err := s.authInfoStore.UpdateAuthInfoDate(ctx, authInfo); 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 f719366adb7..2e07458b436 100644 --- a/pkg/services/login/authinfoservice/user_auth_test.go +++ b/pkg/services/login/authinfoservice/user_auth_test.go @@ -230,6 +230,85 @@ func TestUserAuth(t *testing.T) { require.Equal(t, getAuthQuery.Result.AuthModule, "test1") }) + t.Run("Keeps track of last used auth_module when not using oauth", func(t *testing.T) { + // Restore after destructive operation + sqlStore = sqlstore.InitTestDB(t) + + for i := 0; i < 5; i++ { + cmd := models.CreateUserCommand{ + Email: fmt.Sprint("user", i, "@test.com"), + Name: fmt.Sprint("user", i), + Login: fmt.Sprint("loginuser", i), + } + _, err := sqlStore.CreateUser(context.Background(), cmd) + require.Nil(t, err) + } + + // Find a user to set tokens on + login := "loginuser0" + + fixedTime := time.Now() + // Calling srv.LookupAndUpdateQuery on an existing user will populate an entry in the user_auth table + // Make the first log-in during the past + database.GetTime = func() time.Time { return fixedTime.AddDate(0, 0, -2) } + queryOne := &models.GetUserByAuthInfoQuery{Login: login, AuthModule: "test1", AuthId: "test1"} + user, err := srv.LookupAndUpdate(context.Background(), queryOne) + database.GetTime = time.Now + + require.Nil(t, err) + require.Equal(t, user.Login, login) + + // Add a second auth module for this user + // Have this module's last log-in be more recent + database.GetTime = func() time.Time { return fixedTime.AddDate(0, 0, -1) } + queryTwo := &models.GetUserByAuthInfoQuery{Login: login, AuthModule: "test2", AuthId: "test2"} + user, err = srv.LookupAndUpdate(context.Background(), queryTwo) + require.Nil(t, err) + require.Equal(t, user.Login, login) + + // Get the latest entry by not supply an authmodule or authid + getAuthQuery := &models.GetAuthInfoQuery{ + UserId: user.Id, + } + + err = authInfoStore.GetAuthInfo(context.Background(), getAuthQuery) + + require.Nil(t, err) + require.Equal(t, "test2", getAuthQuery.Result.AuthModule) + + // Now reuse first auth module and make sure it's updated to the most recent + database.GetTime = func() time.Time { return fixedTime } + user, err = srv.LookupAndUpdate(context.Background(), queryOne) + + require.Nil(t, err) + require.Equal(t, user.Login, login) + + err = authInfoStore.GetAuthInfo(context.Background(), getAuthQuery) + + require.Nil(t, err) + require.Equal(t, "test1", getAuthQuery.Result.AuthModule) + + // Now reuse second auth module and make sure it's updated to the most recent + database.GetTime = func() time.Time { return fixedTime.AddDate(0, 0, 1) } + user, err = srv.LookupAndUpdate(context.Background(), queryTwo) + require.Nil(t, err) + require.Equal(t, user.Login, login) + + err = authInfoStore.GetAuthInfo(context.Background(), getAuthQuery) + require.Nil(t, err) + require.Equal(t, "test2", getAuthQuery.Result.AuthModule) + + // Ensure test 1 did not have its entry modified + getAuthQueryUnchanged := &models.GetAuthInfoQuery{ + UserId: user.Id, + AuthModule: "test1", + } + err = authInfoStore.GetAuthInfo(context.Background(), getAuthQueryUnchanged) + require.Nil(t, err) + require.Equal(t, "test1", getAuthQueryUnchanged.Result.AuthModule) + require.Less(t, getAuthQueryUnchanged.Result.Created, getAuthQuery.Result.Created) + }) + t.Run("Can set & locate by generic oauth auth module and user id", func(t *testing.T) { // Find a user to set tokens on login := "loginuser0" diff --git a/pkg/services/login/userprotection.go b/pkg/services/login/userprotection.go index f811b2c0779..bdd8c23e255 100644 --- a/pkg/services/login/userprotection.go +++ b/pkg/services/login/userprotection.go @@ -15,6 +15,7 @@ type Store interface { GetAuthInfo(ctx context.Context, query *models.GetAuthInfoQuery) error SetAuthInfo(ctx context.Context, cmd *models.SetAuthInfoCommand) error UpdateAuthInfo(ctx context.Context, cmd *models.UpdateAuthInfoCommand) error + UpdateAuthInfoDate(ctx context.Context, authInfo *models.UserAuth) error DeleteAuthInfo(ctx context.Context, cmd *models.DeleteAuthInfoCommand) error GetUserById(ctx context.Context, id int64) (*models.User, error) GetUserByLogin(ctx context.Context, login string) (*models.User, error)