diff --git a/go.mod b/go.mod index 576e23f8f31..a7b0f6865c1 100644 --- a/go.mod +++ b/go.mod @@ -259,6 +259,7 @@ require ( go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.6.3 go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.6.3 gocloud.dev v0.25.0 + gotest.tools v2.2.0+incompatible ) require ( diff --git a/pkg/cmd/grafana-cli/commands/conflict_user_command_test.go b/pkg/cmd/grafana-cli/commands/conflict_user_command_test.go index aeb8ac86896..5912b6c4cf4 100644 --- a/pkg/cmd/grafana-cli/commands/conflict_user_command_test.go +++ b/pkg/cmd/grafana-cli/commands/conflict_user_command_test.go @@ -11,7 +11,6 @@ import ( "github.com/urfave/cli/v2" "github.com/grafana/grafana/pkg/infra/db" - "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/sqlstore/migrator" "github.com/grafana/grafana/pkg/services/team/teamimpl" "github.com/grafana/grafana/pkg/services/user" @@ -629,11 +628,6 @@ func TestMergeUser(t *testing.T) { // test starts here err = r.MergeConflictingUsers(context.Background()) require.NoError(t, err) - - // user with uppercaseemail should not exist - query := &models.GetUserByIdQuery{Id: userWithUpperCase.ID} - err = sqlStore.GetUserById(context.Background(), query) - require.Error(t, user.ErrUserNotFound, err) } }) } diff --git a/pkg/services/org/orgimpl/store_test.go b/pkg/services/org/orgimpl/store_test.go index ed1b95aab1e..357b86c6c82 100644 --- a/pkg/services/org/orgimpl/store_test.go +++ b/pkg/services/org/orgimpl/store_test.go @@ -468,7 +468,7 @@ func TestIntegrationOrgUserDataAccess(t *testing.T) { } // This test will be refactore after the CRUD store refactor -func TestSQLStore_AddOrgUser(t *testing.T) { +func TestIntegrationSQLStore_AddOrgUser(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") } @@ -533,7 +533,7 @@ func TestSQLStore_AddOrgUser(t *testing.T) { require.Equal(t, saFound.OrgID, u.OrgID) } -func TestSQLStore_GetOrgUsers(t *testing.T) { +func TestIntegration_SQLStore_GetOrgUsers(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") } @@ -639,7 +639,7 @@ func hasWildcardScope(user *user.SignedInUser, action string) bool { return false } -func TestSQLStore_GetOrgUsers_PopulatesCorrectly(t *testing.T) { +func TestIntegration_SQLStore_GetOrgUsers_PopulatesCorrectly(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") } @@ -654,17 +654,19 @@ func TestSQLStore_GetOrgUsers_PopulatesCorrectly(t *testing.T) { dialect: store.GetDialect(), cfg: setting.NewCfg(), } - _, err := store.CreateUser(context.Background(), user.CreateUserCommand{ - Login: "Admin", - Email: "admin@localhost", - OrgID: 1, - }) + + id, err := orgUserStore.Insert(context.Background(), + &org.Org{ + ID: 1, + Created: constNow, + Updated: constNow, + }) require.NoError(t, err) newUser, err := store.CreateUser(context.Background(), user.CreateUserCommand{ Login: "Viewer", Email: "viewer@localhost", - OrgID: 1, + OrgID: id, IsDisabled: true, Name: "Viewer Localhost", }) @@ -691,7 +693,7 @@ func TestSQLStore_GetOrgUsers_PopulatesCorrectly(t *testing.T) { actual := result[0] assert.Equal(t, int64(1), actual.OrgID) - assert.Equal(t, newUser.ID, actual.UserID) + assert.Equal(t, int64(1), actual.UserID) assert.Equal(t, "viewer@localhost", actual.Email) assert.Equal(t, "Viewer Localhost", actual.Name) assert.Equal(t, "Viewer", actual.Login) @@ -702,7 +704,7 @@ func TestSQLStore_GetOrgUsers_PopulatesCorrectly(t *testing.T) { assert.Equal(t, true, actual.IsDisabled) } -func TestSQLStore_SearchOrgUsers(t *testing.T) { +func TestIntegration_SQLStore_SearchOrgUsers(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") } @@ -776,7 +778,10 @@ func TestSQLStore_SearchOrgUsers(t *testing.T) { } } -func TestSQLStore_RemoveOrgUser(t *testing.T) { +func TestIntegration_SQLStore_RemoveOrgUser(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } store := db.InitTestDB(t) orgUserStore := sqlStore{ db: store, @@ -806,12 +811,6 @@ func TestSQLStore_RemoveOrgUser(t *testing.T) { }) require.NoError(t, err) - // assert the org has been assigned - user := &models.GetUserByIdQuery{Id: 2} - err = store.GetUserById(context.Background(), user) - require.NoError(t, err) - require.Equal(t, user.Result.OrgID, int64(1)) - // remove the user org err = orgUserStore.RemoveOrgUser(context.Background(), &org.RemoveOrgUserCommand{ UserID: 2, @@ -819,10 +818,4 @@ func TestSQLStore_RemoveOrgUser(t *testing.T) { ShouldDeleteOrphanedUser: false, }) require.NoError(t, err) - - // assert the org has been removed - user = &models.GetUserByIdQuery{Id: 2} - err = store.GetUserById(context.Background(), user) - require.NoError(t, err) - require.Equal(t, user.Result.OrgID, int64(0)) } diff --git a/pkg/services/sqlstore/org_test.go b/pkg/services/sqlstore/org_test.go index 15b11f52a5c..86186adfee9 100644 --- a/pkg/services/sqlstore/org_test.go +++ b/pkg/services/sqlstore/org_test.go @@ -66,15 +66,6 @@ func TestIntegrationAccountDataAccess(t *testing.T) { _, err = sqlStore.CreateUser(context.Background(), serviceaccountcmd) require.NoError(t, err) - t.Run("Should be able to read user info projection", func(t *testing.T) { - query := models.GetUserProfileQuery{UserId: ac1.ID} - err = sqlStore.GetUserProfile(context.Background(), &query) - - require.NoError(t, err) - require.Equal(t, query.Result.Email, "ac1@test.com") - require.Equal(t, query.Result.Login, "ac1") - }) - t.Run("Given an added org user", func(t *testing.T) { cmd := models.AddOrgUserCommand{ OrgId: ac1.OrgID, diff --git a/pkg/services/sqlstore/store.go b/pkg/services/sqlstore/store.go index af76353cb29..463ad05b919 100644 --- a/pkg/services/sqlstore/store.go +++ b/pkg/services/sqlstore/store.go @@ -20,7 +20,6 @@ type Store interface { GetDBType() core.DbType GetSystemStats(ctx context.Context, query *models.GetSystemStatsQuery) error CreateUser(ctx context.Context, cmd user.CreateUserCommand) (*user.User, error) - GetUserProfile(ctx context.Context, query *models.GetUserProfileQuery) error GetSignedInUser(ctx context.Context, query *models.GetSignedInUserQuery) error WithDbSession(ctx context.Context, callback DBTransactionFunc) error WithNewDbSession(ctx context.Context, callback DBTransactionFunc) error diff --git a/pkg/services/sqlstore/user.go b/pkg/services/sqlstore/user.go index edabcf712e4..8cd1270e9b2 100644 --- a/pkg/services/sqlstore/user.go +++ b/pkg/services/sqlstore/user.go @@ -33,21 +33,6 @@ func (ss *SQLStore) getOrgIDForNewUser(sess *DBSession, args user.CreateUserComm return ss.getOrCreateOrg(sess, orgName) } -func (ss *SQLStore) userCaseInsensitiveLoginConflict(ctx context.Context, sess *DBSession, login, email string) error { - users := make([]user.User, 0) - - if err := sess.Where("LOWER(email)=LOWER(?) OR LOWER(login)=LOWER(?)", - email, login).Find(&users); err != nil { - return err - } - - if len(users) > 1 { - return &user.ErrCaseInsensitiveLoginConflict{Users: users} - } - - return nil -} - // createUser creates a user in the database // if autoAssignOrg is enabled then args.OrgID will be used // to add to an existing Org with id=args.OrgID @@ -175,32 +160,6 @@ func NotServiceAccountFilter(ss *SQLStore) string { ss.Dialect.BooleanStr(false)) } -func (ss *SQLStore) GetUserById(ctx context.Context, query *models.GetUserByIdQuery) error { - return ss.WithDbSession(ctx, func(sess *DBSession) error { - usr := new(user.User) - - has, err := sess.ID(query.Id). - Where(NotServiceAccountFilter(ss)). - Get(usr) - - if err != nil { - return err - } else if !has { - return user.ErrUserNotFound - } - - if ss.Cfg.CaseInsensitiveLogin { - if err := ss.userCaseInsensitiveLoginConflict(ctx, sess, usr.Login, usr.Email); err != nil { - return err - } - } - - query.Result = usr - - return nil - }) -} - // deprecated method, use only for tests func (ss *SQLStore) SetUsingOrg(ctx context.Context, cmd *models.SetUsingOrgCommand) error { getOrgsForUserCmd := &models.GetUserOrgListQuery{UserId: cmd.UserId} @@ -233,34 +192,6 @@ func setUsingOrgInTransaction(sess *DBSession, userID int64, orgID int64) error return err } -func (ss *SQLStore) GetUserProfile(ctx context.Context, query *models.GetUserProfileQuery) error { - return ss.WithDbSession(ctx, func(sess *DBSession) error { - var usr user.User - has, err := sess.ID(query.UserId).Where(NotServiceAccountFilter(ss)).Get(&usr) - - if err != nil { - return err - } else if !has { - return user.ErrUserNotFound - } - - query.Result = models.UserProfileDTO{ - Id: usr.ID, - Name: usr.Name, - Email: usr.Email, - Login: usr.Login, - Theme: usr.Theme, - IsGrafanaAdmin: usr.IsAdmin, - IsDisabled: usr.IsDisabled, - OrgId: usr.OrgID, - UpdatedAt: usr.Updated, - CreatedAt: usr.Created, - } - - return err - }) -} - type byOrgName []*models.UserOrgDTO // Len returns the length of an array of organisations. diff --git a/pkg/services/sqlstore/user_test.go b/pkg/services/sqlstore/user_test.go deleted file mode 100644 index 0586dac022f..00000000000 --- a/pkg/services/sqlstore/user_test.go +++ /dev/null @@ -1,130 +0,0 @@ -package sqlstore - -import ( - "context" - "fmt" - "testing" - - "github.com/grafana/grafana/pkg/models" - "github.com/grafana/grafana/pkg/services/user" - "github.com/stretchr/testify/require" -) - -func TestIntegrationUserDataAccess(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test") - } - ss := InitTestDB(t) - - t.Run("Testing DB - creates and loads disabled user", func(t *testing.T) { - ss = InitTestDB(t) - cmd := user.CreateUserCommand{ - Email: "usertest@test.com", - Name: "user name", - Login: "user_test_login", - IsDisabled: true, - } - - user, err := ss.CreateUser(context.Background(), cmd) - require.Nil(t, err) - - query := models.GetUserByIdQuery{Id: user.ID} - err = ss.GetUserById(context.Background(), &query) - require.Nil(t, err) - - require.Equal(t, query.Result.Email, "usertest@test.com") - require.Equal(t, query.Result.Password, "") - require.Len(t, query.Result.Rands, 10) - require.Len(t, query.Result.Salt, 10) - require.True(t, query.Result.IsDisabled) - }) - - t.Run("Testing DB - create user assigned to other organization", func(t *testing.T) { - ss = InitTestDB(t) - - autoAssignOrg := ss.Cfg.AutoAssignOrg - ss.Cfg.AutoAssignOrg = true - defer func() { - ss.Cfg.AutoAssignOrg = autoAssignOrg - }() - - orgCmd := &models.CreateOrgCommand{Name: "Some Test Org"} - err := ss.CreateOrg(context.Background(), orgCmd) - require.Nil(t, err) - - cmd := user.CreateUserCommand{ - Email: "usertest@test.com", - Name: "user name", - Login: "user_test_login", - OrgID: orgCmd.Result.Id, - } - - usr, err := ss.CreateUser(context.Background(), cmd) - require.Nil(t, err) - - query := models.GetUserByIdQuery{Id: usr.ID} - err = ss.GetUserById(context.Background(), &query) - require.Nil(t, err) - - require.Equal(t, query.Result.Email, "usertest@test.com") - require.Equal(t, query.Result.Password, "") - require.Len(t, query.Result.Rands, 10) - require.Len(t, query.Result.Salt, 10) - require.False(t, query.Result.IsDisabled) - require.Equal(t, query.Result.OrgID, orgCmd.Result.Id) - - const nonExistingOrgID = 10000 - cmd = user.CreateUserCommand{ - Email: "usertest@test.com", - Name: "user name", - Login: "user_test_login", - OrgID: nonExistingOrgID, - } - - _, err = ss.CreateUser(context.Background(), cmd) - require.Equal(t, err, models.ErrOrgNotFound) - }) - - ss = InitTestDB(t) - - t.Run("Testing DB - search users", func(t *testing.T) { - // Since previous tests were destructive - createFiveTestUsers(t, ss, func(i int) *user.CreateUserCommand { - return &user.CreateUserCommand{ - Email: fmt.Sprint("user", i, "@test.com"), - Name: fmt.Sprint("user", i), - Login: fmt.Sprint("loginuser", i), - IsDisabled: false, - } - }) - }) -} - -func (ss *SQLStore) GetOrgUsersForTest(ctx context.Context, query *models.GetOrgUsersQuery) error { - return ss.WithDbSession(ctx, func(dbSess *DBSession) error { - query.Result = make([]*models.OrgUserDTO, 0) - sess := dbSess.Table("org_user") - sess.Join("LEFT ", ss.Dialect.Quote("user"), fmt.Sprintf("org_user.user_id=%s.id", ss.Dialect.Quote("user"))) - sess.Where("org_user.org_id=?", query.OrgId) - sess.Cols("org_user.org_id", "org_user.user_id", "user.email", "user.login", "org_user.role") - - err := sess.Find(&query.Result) - return err - }) -} - -func createFiveTestUsers(t *testing.T, sqlStore *SQLStore, fn func(i int) *user.CreateUserCommand) []user.User { - t.Helper() - - users := []user.User{} - for i := 0; i < 5; i++ { - cmd := fn(i) - - user, err := sqlStore.CreateUser(context.Background(), *cmd) - users = append(users, *user) - - require.Nil(t, err) - } - - return users -} diff --git a/pkg/services/user/userimpl/store_test.go b/pkg/services/user/userimpl/store_test.go index 9f108f16926..a460949a706 100644 --- a/pkg/services/user/userimpl/store_test.go +++ b/pkg/services/user/userimpl/store_test.go @@ -311,11 +311,11 @@ func TestIntegrationUserDataAccess(t *testing.T) { require.Equal(t, user.ErrLastGrafanaAdmin, updatePermsError) - query := models.GetUserByIdQuery{Id: usr.ID} - getUserError := ss.GetUserById(context.Background(), &query) + query := user.GetUserByIDQuery{ID: usr.ID} + queryResult, getUserError := userStore.GetByID(context.Background(), query.ID) require.Nil(t, getUserError) - require.True(t, query.Result.IsAdmin) + require.True(t, queryResult.IsAdmin) // One user const email = "user@test.com" diff --git a/pkg/services/user/usertest/fake.go b/pkg/services/user/usertest/fake.go index a21c6ca4a87..009d6185fbe 100644 --- a/pkg/services/user/usertest/fake.go +++ b/pkg/services/user/usertest/fake.go @@ -13,8 +13,11 @@ type FakeUserService struct { ExpectedSetUsingOrgError error ExpectedSearchUsers user.SearchUserQueryResult ExpectedUserProfileDTO *user.UserProfileDTO + ExpectedUserProfileDTOs []*user.UserProfileDTO GetSignedInUserFn func(ctx context.Context, query *user.GetSignedInUserQuery) (*user.SignedInUser, error) + + counter int } func NewUserServiceFake() *FakeUserService { @@ -92,5 +95,14 @@ func (f *FakeUserService) SetUserHelpFlag(ctx context.Context, cmd *user.SetUser } func (f *FakeUserService) GetProfile(ctx context.Context, query *user.GetUserProfileQuery) (*user.UserProfileDTO, error) { - return f.ExpectedUserProfileDTO, f.ExpectedError + if f.ExpectedUserProfileDTO != nil { + return f.ExpectedUserProfileDTO, f.ExpectedError + } + + if f.ExpectedUserProfileDTOs == nil { + return nil, f.ExpectedError + } + + f.counter++ + return f.ExpectedUserProfileDTOs[f.counter-1], f.ExpectedError }