Chore: Remove methods from sqlstore interface (#55802)

* Remove SearchOrgUsers from sqlstore interface

* Remove RemoveOrgUser method from sqlstore interface

* Delete RemoveOrgUser from sqlstore

* Fix lint
This commit is contained in:
idafurjes
2022-09-27 15:33:38 +02:00
committed by GitHub
parent 332e8477c7
commit 56cfe02ca2
10 changed files with 501 additions and 493 deletions

View File

@@ -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

View File

@@ -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)
}

View File

@@ -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
}

View File

@@ -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
}

View File

@@ -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))
}