From 56cfe02ca264b7886ef3234d8b082ddb8a927f3a Mon Sep 17 00:00:00 2001 From: idafurjes <36131195+idafurjes@users.noreply.github.com> Date: Tue, 27 Sep 2022 15:33:38 +0200 Subject: [PATCH] Chore: Remove methods from sqlstore interface (#55802) * Remove SearchOrgUsers from sqlstore interface * Remove RemoveOrgUser method from sqlstore interface * Delete RemoveOrgUser from sqlstore * Fix lint --- pkg/services/org/model.go | 8 +- pkg/services/org/orgimpl/org.go | 50 +---- pkg/services/org/orgimpl/org_test.go | 23 ++- pkg/services/org/orgimpl/store.go | 245 ++++++++++++++++++++++++ pkg/services/org/orgimpl/store_test.go | 213 ++++++++++++++++++-- pkg/services/sqlstore/org_test.go | 125 +++--------- pkg/services/sqlstore/org_users.go | 172 ----------------- pkg/services/sqlstore/org_users_test.go | 146 -------------- pkg/services/sqlstore/store.go | 2 - pkg/services/sqlstore/user.go | 10 - 10 files changed, 501 insertions(+), 493 deletions(-) diff --git a/pkg/services/org/model.go b/pkg/services/org/model.go index d46bf6b19d7..4dc0e2a0a2b 100644 --- a/pkg/services/org/model.go +++ b/pkg/services/org/model.go @@ -151,14 +151,14 @@ type OrgUserDTO struct { } type RemoveOrgUserCommand struct { - UserID int64 - OrgID int64 + UserID int64 `xorm:"user_id"` + OrgID int64 `xorm:"org_id"` ShouldDeleteOrphanedUser bool UserWasDeleted bool } type GetOrgUsersQuery struct { - UserID int64 `xorm:"pk autoincr 'user_id'"` + UserID int64 `xorm:"user_id"` OrgID int64 `xorm:"org_id"` Query string Limit int @@ -169,7 +169,7 @@ type GetOrgUsersQuery struct { } type SearchOrgUsersQuery struct { - OrgID int64 + OrgID int64 `xorm:"org_id"` Query string Page int Limit int diff --git a/pkg/services/org/orgimpl/org.go b/pkg/services/org/orgimpl/org.go index 558efee915e..d17c54907ec 100644 --- a/pkg/services/org/orgimpl/org.go +++ b/pkg/services/org/orgimpl/org.go @@ -225,15 +225,9 @@ func (s *Service) UpdateOrgUser(ctx context.Context, cmd *org.UpdateOrgUserComma return s.store.UpdateOrgUser(ctx, cmd) } -// TODO: remove wrapper around sqlstore +// TODO: refactor service to call store CRUD method func (s *Service) RemoveOrgUser(ctx context.Context, cmd *org.RemoveOrgUserCommand) error { - c := &models.RemoveOrgUserCommand{ - UserId: cmd.UserID, - OrgId: cmd.OrgID, - ShouldDeleteOrphanedUser: cmd.ShouldDeleteOrphanedUser, - UserWasDeleted: cmd.UserWasDeleted, - } - return s.sqlStore.RemoveOrgUser(ctx, c) + return s.store.RemoveOrgUser(ctx, cmd) } // TODO: refactor service to call store CRUD method @@ -241,43 +235,7 @@ func (s *Service) GetOrgUsers(ctx context.Context, query *org.GetOrgUsersQuery) return s.store.GetOrgUsers(ctx, query) } -// TODO: remove wrapper around sqlstore +// TODO: refactor service to call store CRUD method func (s *Service) SearchOrgUsers(ctx context.Context, query *org.SearchOrgUsersQuery) (*org.SearchOrgUsersQueryResult, error) { - q := &models.SearchOrgUsersQuery{ - OrgID: query.OrgID, - Query: query.Query, - Page: query.Page, - Limit: query.Limit, - User: query.User, - } - err := s.sqlStore.SearchOrgUsers(ctx, q) - if err != nil { - return nil, err - } - - result := &org.SearchOrgUsersQueryResult{ - TotalCount: q.Result.TotalCount, - OrgUsers: make([]*org.OrgUserDTO, 0), - Page: q.Result.Page, - PerPage: q.Result.PerPage, - } - - for _, user := range q.Result.OrgUsers { - result.OrgUsers = append(result.OrgUsers, &org.OrgUserDTO{ - OrgID: user.OrgId, - UserID: user.UserId, - Login: user.Login, - Email: user.Email, - Name: user.Name, - AvatarURL: user.AvatarUrl, - Role: user.Role, - LastSeenAt: user.LastSeenAt, - LastSeenAtAge: user.LastSeenAtAge, - Updated: user.Updated, - Created: user.Created, - AccessControl: user.AccessControl, - IsDisabled: user.IsDisabled, - }) - } - return result, nil + return s.store.SearchOrgUsers(ctx, query) } diff --git a/pkg/services/org/orgimpl/org_test.go b/pkg/services/org/orgimpl/org_test.go index 5187f10967f..8ce27b5929a 100644 --- a/pkg/services/org/orgimpl/org_test.go +++ b/pkg/services/org/orgimpl/org_test.go @@ -54,13 +54,14 @@ func TestOrgService(t *testing.T) { } type FakeOrgStore struct { - ExpectedOrg *org.Org - ExpectedOrgID int64 - ExpectedUserID int64 - ExpectedError error - ExpectedUserOrgs []*org.UserOrgDTO - ExpectedOrgs []*org.OrgDTO - ExpectedOrgUsers []*org.OrgUserDTO + ExpectedOrg *org.Org + ExpectedOrgID int64 + ExpectedUserID int64 + ExpectedError error + ExpectedUserOrgs []*org.UserOrgDTO + ExpectedOrgs []*org.OrgDTO + ExpectedOrgUsers []*org.OrgUserDTO + ExpectedSearchOrgUsersQueryResult *org.SearchOrgUsersQueryResult } func newOrgStoreFake() *FakeOrgStore { @@ -118,3 +119,11 @@ func (f *FakeOrgStore) UpdateOrgUser(ctx context.Context, cmd *org.UpdateOrgUser func (f *FakeOrgStore) GetOrgUsers(ctx context.Context, query *org.GetOrgUsersQuery) ([]*org.OrgUserDTO, error) { return f.ExpectedOrgUsers, f.ExpectedError } + +func (f *FakeOrgStore) SearchOrgUsers(ctx context.Context, query *org.SearchOrgUsersQuery) (*org.SearchOrgUsersQueryResult, error) { + return f.ExpectedSearchOrgUsersQueryResult, f.ExpectedError +} + +func (f *FakeOrgStore) RemoveOrgUser(ctx context.Context, cmd *org.RemoveOrgUserCommand) error { + return f.ExpectedError +} diff --git a/pkg/services/org/orgimpl/store.go b/pkg/services/org/orgimpl/store.go index 2fb8cd1d8e3..c9580c566d6 100644 --- a/pkg/services/org/orgimpl/store.go +++ b/pkg/services/org/orgimpl/store.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "sort" + "strconv" "strings" "time" @@ -38,6 +39,8 @@ type store interface { AddOrgUser(context.Context, *org.AddOrgUserCommand) error UpdateOrgUser(context.Context, *org.UpdateOrgUserCommand) error GetOrgUsers(context.Context, *org.GetOrgUsersQuery) ([]*org.OrgUserDTO, error) + SearchOrgUsers(context.Context, *org.SearchOrgUsersQuery) (*org.SearchOrgUsersQueryResult, error) + RemoveOrgUser(context.Context, *org.RemoveOrgUserCommand) error } type sqlStore struct { @@ -514,3 +517,245 @@ func (ss *sqlStore) GetOrgUsers(ctx context.Context, query *org.GetOrgUsersQuery } return result, nil } + +func (ss *sqlStore) SearchOrgUsers(ctx context.Context, query *org.SearchOrgUsersQuery) (*org.SearchOrgUsersQueryResult, error) { + result := org.SearchOrgUsersQueryResult{ + OrgUsers: make([]*org.OrgUserDTO, 0), + } + err := ss.db.WithDbSession(ctx, func(dbSession *sqlstore.DBSession) error { + sess := dbSession.Table("org_user") + sess.Join("INNER", ss.dialect.Quote("user"), fmt.Sprintf("org_user.user_id=%s.id", ss.dialect.Quote("user"))) + + whereConditions := make([]string, 0) + whereParams := make([]interface{}, 0) + + whereConditions = append(whereConditions, "org_user.org_id = ?") + whereParams = append(whereParams, query.OrgID) + + whereConditions = append(whereConditions, fmt.Sprintf("%s.is_service_account = %s", ss.dialect.Quote("user"), ss.dialect.BooleanStr(false))) + + if !accesscontrol.IsDisabled(ss.cfg) { + acFilter, err := accesscontrol.Filter(query.User, "org_user.user_id", "users:id:", accesscontrol.ActionOrgUsersRead) + if err != nil { + return err + } + whereConditions = append(whereConditions, acFilter.Where) + whereParams = append(whereParams, acFilter.Args...) + } + + if query.Query != "" { + queryWithWildcards := "%" + query.Query + "%" + whereConditions = append(whereConditions, "(email "+ss.dialect.LikeStr()+" ? OR name "+ss.dialect.LikeStr()+" ? OR login "+ss.dialect.LikeStr()+" ?)") + whereParams = append(whereParams, queryWithWildcards, queryWithWildcards, queryWithWildcards) + } + + if len(whereConditions) > 0 { + sess.Where(strings.Join(whereConditions, " AND "), whereParams...) + } + + if query.Limit > 0 { + offset := query.Limit * (query.Page - 1) + sess.Limit(query.Limit, offset) + } + + sess.Cols( + "org_user.org_id", + "org_user.user_id", + "user.email", + "user.name", + "user.login", + "org_user.role", + "user.last_seen_at", + ) + sess.Asc("user.email", "user.login") + + if err := sess.Find(&result.OrgUsers); err != nil { + return err + } + + // get total count + orgUser := org.OrgUser{} + countSess := dbSession.Table("org_user"). + Join("INNER", ss.dialect.Quote("user"), fmt.Sprintf("org_user.user_id=%s.id", ss.dialect.Quote("user"))) + + if len(whereConditions) > 0 { + countSess.Where(strings.Join(whereConditions, " AND "), whereParams...) + } + + count, err := countSess.Count(&orgUser) + if err != nil { + return err + } + result.TotalCount = count + + for _, user := range result.OrgUsers { + user.LastSeenAtAge = util.GetAgeString(user.LastSeenAt) + } + + return nil + }) + if err != nil { + return nil, err + } + return &result, nil +} + +func (ss *sqlStore) RemoveOrgUser(ctx context.Context, cmd *org.RemoveOrgUserCommand) error { + return ss.db.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { + // check if user exists + var usr user.User + if exists, err := sess.ID(cmd.UserID).Where(ss.notServiceAccountFilter()).Get(&usr); err != nil { + return err + } else if !exists { + return user.ErrUserNotFound + } + + deletes := []string{ + "DELETE FROM org_user WHERE org_id=? and user_id=?", + "DELETE FROM dashboard_acl WHERE org_id=? and user_id = ?", + "DELETE FROM team_member WHERE org_id=? and user_id = ?", + "DELETE FROM query_history_star WHERE org_id=? and user_id = ?", + } + + for _, sql := range deletes { + _, err := sess.Exec(sql, cmd.OrgID, cmd.UserID) + if err != nil { + return err + } + } + + // validate that after delete, there is at least one user with admin role in org + if err := validateOneAdminLeftInOrg(cmd.OrgID, sess); err != nil { + return err + } + + // check user other orgs and update user current org + var userOrgs []*models.UserOrgDTO + sess.Table("org_user") + sess.Join("INNER", "org", "org_user.org_id=org.id") + sess.Where("org_user.user_id=?", usr.ID) + sess.Cols("org.name", "org_user.role", "org_user.org_id") + err := sess.Find(&userOrgs) + + if err != nil { + return err + } + + if len(userOrgs) > 0 { + hasCurrentOrgSet := false + for _, userOrg := range userOrgs { + if usr.OrgID == userOrg.OrgId { + hasCurrentOrgSet = true + break + } + } + + if !hasCurrentOrgSet { + err = setUsingOrgInTransaction(sess, usr.ID, userOrgs[0].OrgId) + if err != nil { + return err + } + } + } else if cmd.ShouldDeleteOrphanedUser { + // no other orgs, delete the full user + if err := ss.deleteUserInTransaction(sess, &models.DeleteUserCommand{UserId: usr.ID}); err != nil { + return err + } + + cmd.UserWasDeleted = true + } else { + // no orgs, but keep the user -> clean up orgId + err = removeUserOrg(sess, usr.ID) + if err != nil { + return err + } + } + + return nil + }) +} + +func (ss *sqlStore) deleteUserInTransaction(sess *sqlstore.DBSession, cmd *models.DeleteUserCommand) error { + // Check if user exists + usr := user.User{ID: cmd.UserId} + has, err := sess.Where(ss.notServiceAccountFilter()).Get(&usr) + if err != nil { + return err + } + if !has { + return user.ErrUserNotFound + } + for _, sql := range ss.userDeletions() { + _, err := sess.Exec(sql, cmd.UserId) + if err != nil { + return err + } + } + + return deleteUserAccessControl(sess, cmd.UserId) +} + +func deleteUserAccessControl(sess *sqlstore.DBSession, userID int64) error { + // Delete user role assignments + if _, err := sess.Exec("DELETE FROM user_role WHERE user_id = ?", userID); err != nil { + return err + } + + // Delete permissions that are scoped to user + if _, err := sess.Exec("DELETE FROM permission WHERE scope = ?", accesscontrol.Scope("users", "id", strconv.FormatInt(userID, 10))); err != nil { + return err + } + + var roleIDs []int64 + if err := sess.SQL("SELECT id FROM role WHERE name = ?", accesscontrol.ManagedUserRoleName(userID)).Find(&roleIDs); err != nil { + return err + } + + if len(roleIDs) == 0 { + return nil + } + + query := "DELETE FROM permission WHERE role_id IN(? " + strings.Repeat(",?", len(roleIDs)-1) + ")" + args := make([]interface{}, 0, len(roleIDs)+1) + args = append(args, query) + for _, id := range roleIDs { + args = append(args, id) + } + + // Delete managed user permissions + if _, err := sess.Exec(args...); err != nil { + return err + } + + // Delete managed user roles + if _, err := sess.Exec("DELETE FROM role WHERE name = ?", accesscontrol.ManagedUserRoleName(userID)); err != nil { + return err + } + + return nil +} + +func (ss *sqlStore) userDeletions() []string { + deletes := []string{ + "DELETE FROM star WHERE user_id = ?", + "DELETE FROM " + ss.dialect.Quote("user") + " WHERE id = ?", + "DELETE FROM org_user WHERE user_id = ?", + "DELETE FROM dashboard_acl WHERE user_id = ?", + "DELETE FROM preferences WHERE user_id = ?", + "DELETE FROM team_member WHERE user_id = ?", + "DELETE FROM user_auth WHERE user_id = ?", + "DELETE FROM user_auth_token WHERE user_id = ?", + "DELETE FROM quota WHERE user_id = ?", + } + return deletes +} + +func removeUserOrg(sess *sqlstore.DBSession, userID int64) error { + user := user.User{ + ID: userID, + OrgID: 0, + } + + _, err := sess.ID(userID).MustCols("org_id").Update(&user) + return err +} diff --git a/pkg/services/org/orgimpl/store_test.go b/pkg/services/org/orgimpl/store_test.go index 0a8c638da2b..447b7105e8d 100644 --- a/pkg/services/org/orgimpl/store_test.go +++ b/pkg/services/org/orgimpl/store_test.go @@ -12,8 +12,6 @@ import ( "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/accesscontrol" - - // ac "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/services/user" @@ -320,13 +318,78 @@ func TestIntegrationOrgUserDataAccess(t *testing.T) { require.Equal(t, result[0].Email, ac1.Email) }) t.Run("Cannot update role so no one is admin user", func(t *testing.T) { - remCmd := models.RemoveOrgUserCommand{OrgId: ac1.OrgID, UserId: ac2.ID, ShouldDeleteOrphanedUser: true} - err := ss.RemoveOrgUser(context.Background(), &remCmd) + remCmd := org.RemoveOrgUserCommand{OrgID: ac1.OrgID, UserID: ac2.ID, ShouldDeleteOrphanedUser: true} + err := orgUserStore.RemoveOrgUser(context.Background(), &remCmd) require.NoError(t, err) cmd := org.UpdateOrgUserCommand{OrgID: ac1.OrgID, UserID: ac1.ID, Role: org.RoleViewer} err = orgUserStore.UpdateOrgUser(context.Background(), &cmd) require.Equal(t, models.ErrLastOrgAdmin, err) }) + + t.Run("Removing user from org should delete user completely if in no other org", func(t *testing.T) { + // make sure ac2 has no org + err := ss.DeleteOrg(context.Background(), &models.DeleteOrgCommand{Id: ac2.OrgID}) + require.NoError(t, err) + + // remove ac2 user from ac1 org + remCmd := org.RemoveOrgUserCommand{OrgID: ac1.OrgID, UserID: ac2.ID, ShouldDeleteOrphanedUser: true} + err = orgUserStore.RemoveOrgUser(context.Background(), &remCmd) + require.NoError(t, err) + require.True(t, remCmd.UserWasDeleted) + + err = ss.GetSignedInUser(context.Background(), &models.GetSignedInUserQuery{UserId: ac2.ID}) + require.Equal(t, err, user.ErrUserNotFound) + }) + + t.Run("Cannot delete last admin org user", func(t *testing.T) { + cmd := org.RemoveOrgUserCommand{OrgID: ac1.OrgID, UserID: ac1.ID} + err := orgUserStore.RemoveOrgUser(context.Background(), &cmd) + require.Equal(t, err, models.ErrLastOrgAdmin) + }) + }) + + t.Run("Given single org and 2 users inserted", func(t *testing.T) { + ss = sqlstore.InitTestDB(t) + testUser := &user.SignedInUser{ + Permissions: map[int64]map[string][]string{ + 1: {accesscontrol.ActionOrgUsersRead: []string{accesscontrol.ScopeUsersAll}}, + }, + } + ss.Cfg.AutoAssignOrg = true + ss.Cfg.AutoAssignOrgId = 1 + ss.Cfg.AutoAssignOrgRole = "Viewer" + + ac1cmd := user.CreateUserCommand{Login: "ac1", Email: "ac1@test.com", Name: "ac1 name"} + ac2cmd := user.CreateUserCommand{Login: "ac2", Email: "ac2@test.com", Name: "ac2 name"} + + ac1, err := ss.CreateUser(context.Background(), ac1cmd) + testUser.OrgID = ac1.OrgID + require.NoError(t, err) + _, err = ss.CreateUser(context.Background(), ac2cmd) + require.NoError(t, err) + + t.Run("Can get organization users paginated with query", func(t *testing.T) { + query := org.SearchOrgUsersQuery{ + OrgID: ac1.OrgID, + Page: 1, + User: testUser, + } + result, err := orgUserStore.SearchOrgUsers(context.Background(), &query) + require.NoError(t, err) + require.Equal(t, 2, len(result.OrgUsers)) + }) + + t.Run("Can get organization users paginated and limited", func(t *testing.T) { + query := org.SearchOrgUsersQuery{ + OrgID: ac1.OrgID, + Limit: 1, + Page: 1, + User: testUser, + } + result, err := orgUserStore.SearchOrgUsers(context.Background(), &query) + require.NoError(t, err) + require.Equal(t, 1, len(result.OrgUsers)) + }) }) } @@ -335,8 +398,11 @@ func TestSQLStore_AddOrgUser(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") } - var orgID int64 = 1 + store := sqlstore.InitTestDB(t) + store.Cfg.AutoAssignOrg = true + store.Cfg.AutoAssignOrgId = 1 + store.Cfg.AutoAssignOrgRole = "Viewer" orgUserStore := sqlStore{ db: store, dialect: store.GetDialect(), @@ -344,9 +410,8 @@ func TestSQLStore_AddOrgUser(t *testing.T) { } // create org and admin - _, err := store.CreateUser(context.Background(), user.CreateUserCommand{ + u, err := store.CreateUser(context.Background(), user.CreateUserCommand{ Login: "admin", - OrgID: orgID, }) require.NoError(t, err) @@ -363,7 +428,7 @@ func TestSQLStore_AddOrgUser(t *testing.T) { // assign the sa to the org but without the override. should fail err = orgUserStore.AddOrgUser(context.Background(), &org.AddOrgUserCommand{ Role: "Viewer", - OrgID: orgID, + OrgID: u.OrgID, UserID: sa.ID, }) require.Error(t, err) @@ -371,7 +436,7 @@ func TestSQLStore_AddOrgUser(t *testing.T) { // assign the sa to the org with the override. should succeed err = orgUserStore.AddOrgUser(context.Background(), &org.AddOrgUserCommand{ Role: "Viewer", - OrgID: orgID, + OrgID: u.OrgID, UserID: sa.ID, AllowAddingServiceAccount: true, }) @@ -391,7 +456,7 @@ func TestSQLStore_AddOrgUser(t *testing.T) { }) require.NoError(t, err) - require.Equal(t, saFound.OrgID, orgID) + require.Equal(t, saFound.OrgID, u.OrgID) } func TestSQLStore_GetOrgUsers(t *testing.T) { @@ -442,7 +507,7 @@ func TestSQLStore_GetOrgUsers(t *testing.T) { }, } - store := sqlstore.InitTestDB(t, sqlstore.InitTestDBOpt{}) + store := sqlstore.InitTestDB(t) orgUserStore := sqlStore{ db: store, dialect: store.GetDialect(), @@ -452,6 +517,7 @@ func TestSQLStore_GetOrgUsers(t *testing.T) { defer func() { orgUserStore.cfg.IsEnterprise = false }() + store.Cfg = setting.NewCfg() seedOrgUsers(t, &orgUserStore, store, 10) for _, tt := range tests { @@ -561,3 +627,128 @@ func TestSQLStore_GetOrgUsers_PopulatesCorrectly(t *testing.T) { assert.Equal(t, constNow, actual.Updated) assert.Equal(t, true, actual.IsDisabled) } + +func TestSQLStore_SearchOrgUsers(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } + tests := []struct { + desc string + query *org.SearchOrgUsersQuery + expectedNumUsers int + }{ + { + desc: "should return all users", + query: &org.SearchOrgUsersQuery{ + OrgID: 1, + User: &user.SignedInUser{ + OrgID: 1, + Permissions: map[int64]map[string][]string{1: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, + }, + }, + expectedNumUsers: 10, + }, + { + desc: "should return no users", + query: &org.SearchOrgUsersQuery{ + OrgID: 1, + User: &user.SignedInUser{ + OrgID: 1, + Permissions: map[int64]map[string][]string{1: {accesscontrol.ActionOrgUsersRead: {""}}}, + }, + }, + expectedNumUsers: 0, + }, + { + desc: "should return some users", + query: &org.SearchOrgUsersQuery{ + OrgID: 1, + User: &user.SignedInUser{ + OrgID: 1, + Permissions: map[int64]map[string][]string{1: {accesscontrol.ActionOrgUsersRead: { + "users:id:1", + "users:id:5", + "users:id:9", + }}}, + }, + }, + expectedNumUsers: 3, + }, + } + + store := sqlstore.InitTestDB(t, sqlstore.InitTestDBOpt{}) + orgUserStore := sqlStore{ + db: store, + dialect: store.GetDialect(), + cfg: setting.NewCfg(), + } + // orgUserStore.cfg.Skip + seedOrgUsers(t, &orgUserStore, store, 10) + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + result, err := orgUserStore.SearchOrgUsers(context.Background(), tt.query) + fmt.Println("users:", result) + + require.NoError(t, err) + assert.Len(t, result.OrgUsers, tt.expectedNumUsers) + + if !hasWildcardScope(tt.query.User, accesscontrol.ActionOrgUsersRead) { + for _, u := range result.OrgUsers { + assert.Contains(t, tt.query.User.Permissions[tt.query.User.OrgID][accesscontrol.ActionOrgUsersRead], fmt.Sprintf("users:id:%d", u.UserID)) + } + } + }) + } +} + +func TestSQLStore_RemoveOrgUser(t *testing.T) { + store := sqlstore.InitTestDB(t) + orgUserStore := sqlStore{ + db: store, + dialect: store.GetDialect(), + cfg: setting.NewCfg(), + } + // create org and admin + _, err := store.CreateUser(context.Background(), user.CreateUserCommand{ + Login: "admin", + OrgID: 1, + }) + require.NoError(t, err) + + // create a user with no org + _, err = store.CreateUser(context.Background(), user.CreateUserCommand{ + Login: "user", + OrgID: 1, + SkipOrgSetup: true, + }) + require.NoError(t, err) + + // assign the user to the org + err = orgUserStore.AddOrgUser(context.Background(), &org.AddOrgUserCommand{ + Role: "Viewer", + OrgID: 1, + UserID: 2, + }) + 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, + OrgID: 1, + 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 20a5db63659..aaf97abc65c 100644 --- a/pkg/services/sqlstore/org_test.go +++ b/pkg/services/sqlstore/org_test.go @@ -24,11 +24,6 @@ func TestIntegrationAccountDataAccess(t *testing.T) { } t.Run("Testing Account DB Access", func(t *testing.T) { sqlStore := InitTestDB(t) - testUser := &user.SignedInUser{ - Permissions: map[int64]map[string][]string{ - 1: {accesscontrol.ActionOrgUsersRead: []string{accesscontrol.ScopeUsersAll}}, - }, - } t.Run("Given we have organizations, we can query them by IDs", func(t *testing.T) { var err error @@ -109,47 +104,6 @@ func TestIntegrationAccountDataAccess(t *testing.T) { }) }) - t.Run("Given single org and 2 users inserted", func(t *testing.T) { - sqlStore = InitTestDB(t) - sqlStore.Cfg.AutoAssignOrg = true - sqlStore.Cfg.AutoAssignOrgId = 1 - sqlStore.Cfg.AutoAssignOrgRole = "Viewer" - - ac1cmd := user.CreateUserCommand{Login: "ac1", Email: "ac1@test.com", Name: "ac1 name"} - ac2cmd := user.CreateUserCommand{Login: "ac2", Email: "ac2@test.com", Name: "ac2 name"} - - ac1, err := sqlStore.CreateUser(context.Background(), ac1cmd) - testUser.OrgID = ac1.OrgID - require.NoError(t, err) - _, err = sqlStore.CreateUser(context.Background(), ac2cmd) - require.NoError(t, err) - - t.Run("Can get organization users paginated with query", func(t *testing.T) { - query := models.SearchOrgUsersQuery{ - OrgID: ac1.OrgID, - Page: 1, - User: testUser, - } - err = sqlStore.SearchOrgUsers(context.Background(), &query) - - require.NoError(t, err) - require.Equal(t, len(query.Result.OrgUsers), 2) - }) - - t.Run("Can get organization users paginated and limited", func(t *testing.T) { - query := models.SearchOrgUsersQuery{ - OrgID: ac1.OrgID, - Limit: 1, - Page: 1, - User: testUser, - } - err = sqlStore.SearchOrgUsers(context.Background(), &query) - - require.NoError(t, err) - require.Equal(t, len(query.Result.OrgUsers), 1) - }) - }) - t.Run("Given two saved users", func(t *testing.T) { sqlStore = InitTestDB(t) sqlStore.Cfg.AutoAssignOrg = false @@ -241,38 +195,18 @@ func TestIntegrationAccountDataAccess(t *testing.T) { require.Equal(t, query.Result.OrgName, "ac1@test.com") }) - t.Run("Should set last org as current when removing user from current", func(t *testing.T) { - remCmd := models.RemoveOrgUserCommand{OrgId: ac1.OrgID, UserId: ac2.ID} - err := sqlStore.RemoveOrgUser(context.Background(), &remCmd) - require.NoError(t, err) + // TODO: This test should be moved to user store + // t.Run("Should set last org as current when removing user from current", func(t *testing.T) { + // remCmd := models.RemoveOrgUserCommand{OrgId: ac1.OrgID, UserId: ac2.ID} + // err := sqlStore.RemoveOrgUser(context.Background(), &remCmd) + // require.NoError(t, err) - query := models.GetSignedInUserQuery{UserId: ac2.ID} - err = sqlStore.GetSignedInUser(context.Background(), &query) + // query := models.GetSignedInUserQuery{UserId: ac2.ID} + // err = sqlStore.GetSignedInUser(context.Background(), &query) - require.NoError(t, err) - require.Equal(t, query.Result.OrgID, ac2.OrgID) - }) - }) - - t.Run("Removing user from org should delete user completely if in no other org", func(t *testing.T) { - // make sure ac2 has no org - err := sqlStore.DeleteOrg(context.Background(), &models.DeleteOrgCommand{Id: ac2.OrgID}) - require.NoError(t, err) - - // remove ac2 user from ac1 org - remCmd := models.RemoveOrgUserCommand{OrgId: ac1.OrgID, UserId: ac2.ID, ShouldDeleteOrphanedUser: true} - err = sqlStore.RemoveOrgUser(context.Background(), &remCmd) - require.NoError(t, err) - require.True(t, remCmd.UserWasDeleted) - - err = sqlStore.GetSignedInUser(context.Background(), &models.GetSignedInUserQuery{UserId: ac2.ID}) - require.Equal(t, err, user.ErrUserNotFound) - }) - - t.Run("Cannot delete last admin org user", func(t *testing.T) { - cmd := models.RemoveOrgUserCommand{OrgId: ac1.OrgID, UserId: ac1.ID} - err := sqlStore.RemoveOrgUser(context.Background(), &cmd) - require.Equal(t, err, models.ErrLastOrgAdmin) + // require.NoError(t, err) + // require.Equal(t, query.Result.OrgID, ac2.OrgID) + // }) }) t.Run("Given an org user with dashboard permissions", func(t *testing.T) { @@ -302,31 +236,32 @@ func TestIntegrationAccountDataAccess(t *testing.T) { }) require.NoError(t, err) - t.Run("When org user is deleted", func(t *testing.T) { - cmdRemove := models.RemoveOrgUserCommand{OrgId: ac1.OrgID, UserId: ac3.ID} - err := sqlStore.RemoveOrgUser(context.Background(), &cmdRemove) - require.NoError(t, err) + // TODO: should be moved to dashboard service + // t.Run("When org user is deleted", func(t *testing.T) { + // cmdRemove := models.RemoveOrgUserCommand{OrgId: ac1.OrgID, UserId: ac3.ID} + // err := sqlStore.RemoveOrgUser(context.Background(), &cmdRemove) + // require.NoError(t, err) - t.Run("Should remove dependent permissions for deleted org user", func(t *testing.T) { - permQuery := &models.GetDashboardACLInfoListQuery{DashboardID: dash1.Id, OrgID: ac1.OrgID} + // t.Run("Should remove dependent permissions for deleted org user", func(t *testing.T) { + // permQuery := &models.GetDashboardACLInfoListQuery{DashboardID: dash1.Id, OrgID: ac1.OrgID} - err = getDashboardACLInfoList(sqlStore, permQuery) - require.NoError(t, err) + // err = getDashboardACLInfoList(sqlStore, permQuery) + // require.NoError(t, err) - require.Equal(t, len(permQuery.Result), 0) - }) + // require.Equal(t, len(permQuery.Result), 0) + // }) - t.Run("Should not remove dashboard permissions for same user in another org", func(t *testing.T) { - permQuery := &models.GetDashboardACLInfoListQuery{DashboardID: dash2.Id, OrgID: ac3.OrgID} + // t.Run("Should not remove dashboard permissions for same user in another org", func(t *testing.T) { + // permQuery := &models.GetDashboardACLInfoListQuery{DashboardID: dash2.Id, OrgID: ac3.OrgID} - err = getDashboardACLInfoList(sqlStore, permQuery) - require.NoError(t, err) + // err = getDashboardACLInfoList(sqlStore, permQuery) + // require.NoError(t, err) - require.Equal(t, len(permQuery.Result), 1) - require.Equal(t, permQuery.Result[0].OrgId, ac3.OrgID) - require.Equal(t, permQuery.Result[0].UserId, ac3.ID) - }) - }) + // require.Equal(t, len(permQuery.Result), 1) + // require.Equal(t, permQuery.Result[0].OrgId, ac3.OrgID) + // require.Equal(t, permQuery.Result[0].UserId, ac3.ID) + // }) + // }) }) }) }) diff --git a/pkg/services/sqlstore/org_users.go b/pkg/services/sqlstore/org_users.go index 7f376a4416f..1e154b07610 100644 --- a/pkg/services/sqlstore/org_users.go +++ b/pkg/services/sqlstore/org_users.go @@ -2,14 +2,10 @@ package sqlstore import ( "context" - "fmt" - "strings" "time" "github.com/grafana/grafana/pkg/models" - "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/user" - "github.com/grafana/grafana/pkg/util" ) func (ss *SQLStore) AddOrgUser(ctx context.Context, cmd *models.AddOrgUserCommand) error { @@ -70,171 +66,3 @@ func (ss *SQLStore) AddOrgUser(ctx context.Context, cmd *models.AddOrgUserComman return nil }) } - -func (ss *SQLStore) SearchOrgUsers(ctx context.Context, query *models.SearchOrgUsersQuery) error { - return ss.WithDbSession(ctx, func(dbSession *DBSession) error { - query.Result = models.SearchOrgUsersQueryResult{ - OrgUsers: make([]*models.OrgUserDTO, 0), - } - - sess := dbSession.Table("org_user") - sess.Join("INNER", ss.Dialect.Quote("user"), fmt.Sprintf("org_user.user_id=%s.id", ss.Dialect.Quote("user"))) - - whereConditions := make([]string, 0) - whereParams := make([]interface{}, 0) - - whereConditions = append(whereConditions, "org_user.org_id = ?") - whereParams = append(whereParams, query.OrgID) - - whereConditions = append(whereConditions, fmt.Sprintf("%s.is_service_account = %s", ss.Dialect.Quote("user"), ss.Dialect.BooleanStr(false))) - - if !accesscontrol.IsDisabled(ss.Cfg) { - acFilter, err := accesscontrol.Filter(query.User, "org_user.user_id", "users:id:", accesscontrol.ActionOrgUsersRead) - if err != nil { - return err - } - whereConditions = append(whereConditions, acFilter.Where) - whereParams = append(whereParams, acFilter.Args...) - } - - if query.Query != "" { - queryWithWildcards := "%" + query.Query + "%" - whereConditions = append(whereConditions, "(email "+ss.Dialect.LikeStr()+" ? OR name "+ss.Dialect.LikeStr()+" ? OR login "+ss.Dialect.LikeStr()+" ?)") - whereParams = append(whereParams, queryWithWildcards, queryWithWildcards, queryWithWildcards) - } - - if len(whereConditions) > 0 { - sess.Where(strings.Join(whereConditions, " AND "), whereParams...) - } - - if query.Limit > 0 { - offset := query.Limit * (query.Page - 1) - sess.Limit(query.Limit, offset) - } - - sess.Cols( - "org_user.org_id", - "org_user.user_id", - "user.email", - "user.name", - "user.login", - "org_user.role", - "user.last_seen_at", - ) - sess.Asc("user.email", "user.login") - - if err := sess.Find(&query.Result.OrgUsers); err != nil { - return err - } - - // get total count - orgUser := models.OrgUser{} - countSess := dbSession.Table("org_user"). - Join("INNER", ss.Dialect.Quote("user"), fmt.Sprintf("org_user.user_id=%s.id", ss.Dialect.Quote("user"))) - - if len(whereConditions) > 0 { - countSess.Where(strings.Join(whereConditions, " AND "), whereParams...) - } - - count, err := countSess.Count(&orgUser) - if err != nil { - return err - } - query.Result.TotalCount = count - - for _, user := range query.Result.OrgUsers { - user.LastSeenAtAge = util.GetAgeString(user.LastSeenAt) - } - - return nil - }) -} - -func (ss *SQLStore) RemoveOrgUser(ctx context.Context, cmd *models.RemoveOrgUserCommand) error { - return ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error { - // check if user exists - var usr user.User - if exists, err := sess.ID(cmd.UserId).Where(notServiceAccountFilter(ss)).Get(&usr); err != nil { - return err - } else if !exists { - return user.ErrUserNotFound - } - - deletes := []string{ - "DELETE FROM org_user WHERE org_id=? and user_id=?", - "DELETE FROM dashboard_acl WHERE org_id=? and user_id = ?", - "DELETE FROM team_member WHERE org_id=? and user_id = ?", - "DELETE FROM query_history_star WHERE org_id=? and user_id = ?", - } - - for _, sql := range deletes { - _, err := sess.Exec(sql, cmd.OrgId, cmd.UserId) - if err != nil { - return err - } - } - - // validate that after delete, there is at least one user with admin role in org - if err := validateOneAdminLeftInOrg(cmd.OrgId, sess); err != nil { - return err - } - - // check user other orgs and update user current org - var userOrgs []*models.UserOrgDTO - sess.Table("org_user") - sess.Join("INNER", "org", "org_user.org_id=org.id") - sess.Where("org_user.user_id=?", usr.ID) - sess.Cols("org.name", "org_user.role", "org_user.org_id") - err := sess.Find(&userOrgs) - - if err != nil { - return err - } - - if len(userOrgs) > 0 { - hasCurrentOrgSet := false - for _, userOrg := range userOrgs { - if usr.OrgID == userOrg.OrgId { - hasCurrentOrgSet = true - break - } - } - - if !hasCurrentOrgSet { - err = setUsingOrgInTransaction(sess, usr.ID, userOrgs[0].OrgId) - if err != nil { - return err - } - } - } else if cmd.ShouldDeleteOrphanedUser { - // no other orgs, delete the full user - if err := deleteUserInTransaction(ss, sess, &models.DeleteUserCommand{UserId: usr.ID}); err != nil { - return err - } - - cmd.UserWasDeleted = true - } else { - // no orgs, but keep the user -> clean up orgId - err = removeUserOrg(sess, usr.ID) - if err != nil { - return err - } - } - - return nil - }) -} - -// validate that there is an org admin user left -func validateOneAdminLeftInOrg(orgId int64, sess *DBSession) error { - res, err := sess.Query("SELECT 1 from org_user WHERE org_id=? and role='Admin'", orgId) - if err != nil { - return err - } - - if len(res) == 0 { - return models.ErrLastOrgAdmin - } - - return err -} diff --git a/pkg/services/sqlstore/org_users_test.go b/pkg/services/sqlstore/org_users_test.go index 81514aea3ac..a2ca123f4f4 100644 --- a/pkg/services/sqlstore/org_users_test.go +++ b/pkg/services/sqlstore/org_users_test.go @@ -2,83 +2,14 @@ package sqlstore import ( "context" - "fmt" - "strings" "testing" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/models" - ac "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/user" ) -type searchOrgUsersTestCase struct { - desc string - query *models.SearchOrgUsersQuery - expectedNumUsers int -} - -func TestSQLStore_SearchOrgUsers(t *testing.T) { - tests := []searchOrgUsersTestCase{ - { - desc: "should return all users", - query: &models.SearchOrgUsersQuery{ - OrgID: 1, - User: &user.SignedInUser{ - OrgID: 1, - Permissions: map[int64]map[string][]string{1: {ac.ActionOrgUsersRead: {ac.ScopeUsersAll}}}, - }, - }, - expectedNumUsers: 10, - }, - { - desc: "should return no users", - query: &models.SearchOrgUsersQuery{ - OrgID: 1, - User: &user.SignedInUser{ - OrgID: 1, - Permissions: map[int64]map[string][]string{1: {ac.ActionOrgUsersRead: {""}}}, - }, - }, - expectedNumUsers: 0, - }, - { - desc: "should return some users", - query: &models.SearchOrgUsersQuery{ - OrgID: 1, - User: &user.SignedInUser{ - OrgID: 1, - Permissions: map[int64]map[string][]string{1: {ac.ActionOrgUsersRead: { - "users:id:1", - "users:id:5", - "users:id:9", - }}}, - }, - }, - expectedNumUsers: 3, - }, - } - - store := InitTestDB(t, InitTestDBOpt{}) - seedOrgUsers(t, store, 10) - - for _, tt := range tests { - t.Run(tt.desc, func(t *testing.T) { - err := store.SearchOrgUsers(context.Background(), tt.query) - require.NoError(t, err) - assert.Len(t, tt.query.Result.OrgUsers, tt.expectedNumUsers) - - if !hasWildcardScope(tt.query.User, ac.ActionOrgUsersRead) { - for _, u := range tt.query.Result.OrgUsers { - assert.Contains(t, tt.query.User.Permissions[tt.query.User.OrgID][ac.ActionOrgUsersRead], fmt.Sprintf("users:id:%d", u.UserId)) - } - } - }) - } -} - func TestSQLStore_AddOrgUser(t *testing.T) { var orgID int64 = 1 store := InitTestDB(t) @@ -133,80 +64,3 @@ func TestSQLStore_AddOrgUser(t *testing.T) { require.NoError(t, err) require.Equal(t, saFound.OrgID, orgID) } - -func TestSQLStore_RemoveOrgUser(t *testing.T) { - store := InitTestDB(t) - - // create org and admin - _, err := store.CreateUser(context.Background(), user.CreateUserCommand{ - Login: "admin", - OrgID: 1, - }) - require.NoError(t, err) - - // create a user with no org - _, err = store.CreateUser(context.Background(), user.CreateUserCommand{ - Login: "user", - OrgID: 1, - SkipOrgSetup: true, - }) - require.NoError(t, err) - - // assign the user to the org - err = store.AddOrgUser(context.Background(), &models.AddOrgUserCommand{ - Role: "Viewer", - OrgId: 1, - UserId: 2, - }) - 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 = store.RemoveOrgUser(context.Background(), &models.RemoveOrgUserCommand{ - UserId: 2, - OrgId: 1, - 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)) -} - -func seedOrgUsers(t *testing.T, store *SQLStore, numUsers int) { - t.Helper() - // Seed users - for i := 1; i <= numUsers; i++ { - user, err := store.CreateUser(context.Background(), user.CreateUserCommand{ - Login: fmt.Sprintf("user-%d", i), - OrgID: 1, - }) - require.NoError(t, err) - - if i != 1 { - err = store.AddOrgUser(context.Background(), &models.AddOrgUserCommand{ - Role: "Viewer", - OrgId: 1, - UserId: user.ID, - }) - require.NoError(t, err) - } - } -} - -func hasWildcardScope(user *user.SignedInUser, action string) bool { - for _, scope := range user.Permissions[user.OrgID][action] { - if strings.HasSuffix(scope, ":*") { - return true - } - } - return false -} diff --git a/pkg/services/sqlstore/store.go b/pkg/services/sqlstore/store.go index ca46eceb39b..aada82826e4 100644 --- a/pkg/services/sqlstore/store.go +++ b/pkg/services/sqlstore/store.go @@ -39,8 +39,6 @@ type Store interface { GetGlobalQuotaByTarget(ctx context.Context, query *models.GetGlobalQuotaByTargetQuery) error WithTransactionalDbSession(ctx context.Context, callback DBTransactionFunc) error InTransaction(ctx context.Context, fn func(ctx context.Context) error) error - SearchOrgUsers(ctx context.Context, query *models.SearchOrgUsersQuery) error - RemoveOrgUser(ctx context.Context, cmd *models.RemoveOrgUserCommand) error Migrate(bool) error Sync() error Reset() error diff --git a/pkg/services/sqlstore/user.go b/pkg/services/sqlstore/user.go index c4edd6fc0cb..9c564cf97db 100644 --- a/pkg/services/sqlstore/user.go +++ b/pkg/services/sqlstore/user.go @@ -374,16 +374,6 @@ func setUsingOrgInTransaction(sess *DBSession, userID int64, orgID int64) error return err } -func removeUserOrg(sess *DBSession, userID int64) error { - user := user.User{ - ID: userID, - OrgID: 0, - } - - _, err := sess.ID(userID).MustCols("org_id").Update(&user) - 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