From 2bfd26249bd620097944e74c0d8489d90577eafa Mon Sep 17 00:00:00 2001 From: idafurjes <36131195+idafurjes@users.noreply.github.com> Date: Tue, 27 Sep 2022 10:34:31 +0200 Subject: [PATCH] Chore: Remove methods from store interface (#55765) * Chore: Remove methods from store interface * Fix api tests * Remove sqlstore methods, add org store tests, add GetOrgUsers to org store interface * Fix lint * Remove debug logs * Remove commented out methods --- pkg/api/common_test.go | 2 +- pkg/api/org_users_test.go | 25 +- pkg/services/org/model.go | 14 +- pkg/services/org/orgimpl/org.go | 60 +--- pkg/services/org/orgimpl/org_test.go | 13 + pkg/services/org/orgimpl/store.go | 207 +++++++++++++- pkg/services/org/orgimpl/store_test.go | 352 +++++++++++++++++++++++- pkg/services/sqlstore/org_test.go | 83 ------ pkg/services/sqlstore/org_users.go | 97 ------- pkg/services/sqlstore/org_users_test.go | 125 --------- pkg/services/sqlstore/store.go | 3 - 11 files changed, 584 insertions(+), 397 deletions(-) diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index f35c115865d..36e982e0a5b 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -295,7 +295,7 @@ type accessControlScenarioContext struct { usermock *usertest.FakeUserService // db is a test database initialized with InitTestDB - db sqlstore.Store + db *sqlstore.SQLStore // cfg is the setting provider cfg *setting.Cfg diff --git a/pkg/api/org_users_test.go b/pkg/api/org_users_test.go index b3bd0297c41..b948ebd6f55 100644 --- a/pkg/api/org_users_test.go +++ b/pkg/api/org_users_test.go @@ -324,7 +324,7 @@ var ( // setupOrgUsersDBForAccessControlTests creates three users placed in two orgs // Org1: testServerAdminViewer, testEditorOrg1 // Org2: testServerAdminViewer, testAdminOrg2 -func setupOrgUsersDBForAccessControlTests(t *testing.T, db sqlstore.Store) { +func setupOrgUsersDBForAccessControlTests(t *testing.T, db *sqlstore.SQLStore) { t.Helper() var err error @@ -616,13 +616,6 @@ func TestPostOrgUsersAPIEndpoint_AccessControl(t *testing.T) { var message util.DynMap err := json.NewDecoder(response.Body).Decode(&message) require.NoError(t, err) - - getUsersQuery := models.GetOrgUsersQuery{OrgId: tc.targetOrg, User: &user.SignedInUser{ - OrgID: tc.targetOrg, - Permissions: map[int64]map[string][]string{tc.targetOrg: {"org.users:read": {"users:*"}}}, - }} - err = sc.db.GetOrgUsers(context.Background(), &getUsersQuery) - require.NoError(t, err) } }) } @@ -989,22 +982,6 @@ func TestDeleteOrgUsersAPIEndpoint_AccessControl(t *testing.T) { err := json.NewDecoder(response.Body).Decode(&message) require.NoError(t, err) assert.Equal(t, tc.expectedMessage, message) - - getUsersQuery := models.GetOrgUsersQuery{ - OrgId: tc.targetOrg, - User: &user.SignedInUser{ - OrgID: tc.targetOrg, - Permissions: map[int64]map[string][]string{tc.targetOrg: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, - }, - } - err = sc.db.GetOrgUsers(context.Background(), &getUsersQuery) - require.NoError(t, err) - assert.Len(t, getUsersQuery.Result, tc.expectedUserCount) - - // check all permissions for user is removed in org - permission, err := sc.hs.accesscontrolService.GetUserPermissions(context.Background(), &user.SignedInUser{UserID: tc.targetUserId, OrgID: tc.targetOrg}, accesscontrol.Options{}) - require.NoError(t, err) - assert.Len(t, permission, 0) } }) } diff --git a/pkg/services/org/model.go b/pkg/services/org/model.go index c0fe76defe3..d46bf6b19d7 100644 --- a/pkg/services/org/model.go +++ b/pkg/services/org/model.go @@ -120,8 +120,8 @@ type AddOrgUserCommand struct { LoginOrEmail string `json:"loginOrEmail" binding:"Required"` Role RoleType `json:"role" binding:"Required"` - OrgID int64 `json:"-"` - UserID int64 `json:"-"` + OrgID int64 `json:"-" xorm:"org_id"` + UserID int64 `json:"-" xorm:"user_id"` // internal use: avoid adding service accounts to orgs via user routes AllowAddingServiceAccount bool `json:"-"` @@ -135,11 +135,11 @@ type UpdateOrgUserCommand struct { } type OrgUserDTO struct { - OrgID int64 `json:"orgId"` - UserID int64 `json:"userId"` + OrgID int64 `json:"orgId" xorm:"org_id"` + UserID int64 `json:"userId" xorm:"user_id"` Email string `json:"email"` Name string `json:"name"` - AvatarURL string `json:"avatarUrl"` + AvatarURL string `json:"avatarUrl" xorm:"avatar_url"` Login string `json:"login"` Role string `json:"role"` LastSeenAt time.Time `json:"lastSeenAt"` @@ -158,8 +158,8 @@ type RemoveOrgUserCommand struct { } type GetOrgUsersQuery struct { - UserID int64 - OrgID int64 + UserID int64 `xorm:"pk autoincr 'user_id'"` + OrgID int64 `xorm:"org_id"` Query string Limit int // Flag used to allow oss edition to query users without access control diff --git a/pkg/services/org/orgimpl/org.go b/pkg/services/org/orgimpl/org.go index d8fd8bacc72..558efee915e 100644 --- a/pkg/services/org/orgimpl/org.go +++ b/pkg/services/org/orgimpl/org.go @@ -22,13 +22,16 @@ type Service struct { } func ProvideService(db sqlstore.Store, cfg *setting.Cfg) org.Service { + log := log.New("org service") return &Service{ store: &sqlStore{ db: db, dialect: db.GetDialect(), + log: log, + cfg: cfg, }, cfg: cfg, - log: log.New("org service"), + log: log, sqlStore: db, } } @@ -212,26 +215,14 @@ func (s *Service) GetOrCreate(ctx context.Context, orgName string) (int64, error return orga.ID, nil } -// TODO: remove wrapper around sqlstore +// TODO: refactor service to call store CRUD method func (s *Service) AddOrgUser(ctx context.Context, cmd *org.AddOrgUserCommand) error { - c := &models.AddOrgUserCommand{ - LoginOrEmail: cmd.LoginOrEmail, - OrgId: cmd.OrgID, - UserId: cmd.UserID, - Role: cmd.Role, - AllowAddingServiceAccount: cmd.AllowAddingServiceAccount, - } - return s.sqlStore.AddOrgUser(ctx, c) + return s.store.AddOrgUser(ctx, cmd) } -// TODO: remove wrapper around sqlstore +// TODO: refactor service to call store CRUD method func (s *Service) UpdateOrgUser(ctx context.Context, cmd *org.UpdateOrgUserCommand) error { - c := &models.UpdateOrgUserCommand{ - UserId: cmd.UserID, - OrgId: cmd.OrgID, - Role: cmd.Role, - } - return s.sqlStore.UpdateOrgUser(ctx, c) + return s.store.UpdateOrgUser(ctx, cmd) } // TODO: remove wrapper around sqlstore @@ -245,40 +236,9 @@ func (s *Service) RemoveOrgUser(ctx context.Context, cmd *org.RemoveOrgUserComma return s.sqlStore.RemoveOrgUser(ctx, c) } -// TODO: remove wrapper around sqlstore +// TODO: refactor service to call store CRUD method func (s *Service) GetOrgUsers(ctx context.Context, query *org.GetOrgUsersQuery) ([]*org.OrgUserDTO, error) { - q := &models.GetOrgUsersQuery{ - UserID: query.UserID, - OrgId: query.OrgID, - Query: query.Query, - Limit: query.Limit, - DontEnforceAccessControl: query.DontEnforceAccessControl, - User: query.User, - } - err := s.sqlStore.GetOrgUsers(ctx, q) - if err != nil { - return nil, err - } - - result := make([]*org.OrgUserDTO, 0) - for _, user := range q.Result { - result = append(result, &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.GetOrgUsers(ctx, query) } // TODO: remove wrapper around sqlstore diff --git a/pkg/services/org/orgimpl/org_test.go b/pkg/services/org/orgimpl/org_test.go index 51963f31b81..5187f10967f 100644 --- a/pkg/services/org/orgimpl/org_test.go +++ b/pkg/services/org/orgimpl/org_test.go @@ -60,6 +60,7 @@ type FakeOrgStore struct { ExpectedError error ExpectedUserOrgs []*org.UserOrgDTO ExpectedOrgs []*org.OrgDTO + ExpectedOrgUsers []*org.OrgUserDTO } func newOrgStoreFake() *FakeOrgStore { @@ -105,3 +106,15 @@ func (f *FakeOrgStore) Search(ctx context.Context, query *org.SearchOrgsQuery) ( func (f *FakeOrgStore) CreateWithMember(ctx context.Context, cmd *org.CreateOrgCommand) (*org.Org, error) { return f.ExpectedOrg, f.ExpectedError } + +func (f *FakeOrgStore) AddOrgUser(ctx context.Context, cmd *org.AddOrgUserCommand) error { + return f.ExpectedError +} + +func (f *FakeOrgStore) UpdateOrgUser(ctx context.Context, cmd *org.UpdateOrgUserCommand) error { + return f.ExpectedError +} + +func (f *FakeOrgStore) GetOrgUsers(ctx context.Context, query *org.GetOrgUsersQuery) ([]*org.OrgUserDTO, error) { + return f.ExpectedOrgUsers, f.ExpectedError +} diff --git a/pkg/services/org/orgimpl/store.go b/pkg/services/org/orgimpl/store.go index affe48cba56..2fb8cd1d8e3 100644 --- a/pkg/services/org/orgimpl/store.go +++ b/pkg/services/org/orgimpl/store.go @@ -4,14 +4,20 @@ import ( "context" "fmt" "sort" + "strings" "time" "github.com/grafana/grafana/pkg/events" + "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" + "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/sqlstore/db" "github.com/grafana/grafana/pkg/services/sqlstore/migrator" + "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/setting" + "github.com/grafana/grafana/pkg/util" ) const MainOrgName = "Main Org." @@ -29,11 +35,17 @@ type store interface { GetUserOrgList(context.Context, *org.GetUserOrgListQuery) ([]*org.UserOrgDTO, error) Search(context.Context, *org.SearchOrgsQuery) ([]*org.OrgDTO, error) CreateWithMember(context.Context, *org.CreateOrgCommand) (*org.Org, error) + AddOrgUser(context.Context, *org.AddOrgUserCommand) error + UpdateOrgUser(context.Context, *org.UpdateOrgUserCommand) error + GetOrgUsers(context.Context, *org.GetOrgUsersQuery) ([]*org.OrgUserDTO, error) } type sqlStore struct { db db.DB dialect migrator.Dialect + //TODO: moved to service + log log.Logger + cfg *setting.Cfg } func (ss *sqlStore) Get(ctx context.Context, orgID int64) (*org.Org, error) { @@ -112,7 +124,7 @@ func (ss *sqlStore) Update(ctx context.Context, cmd *org.UpdateOrgCommand) error return models.ErrOrgNameTaken } - org := models.Org{ + org := org.Org{ Name: cmd.Name, Updated: time.Now(), } @@ -129,7 +141,7 @@ func (ss *sqlStore) Update(ctx context.Context, cmd *org.UpdateOrgCommand) error sess.PublishAfterCommit(&events.OrgUpdated{ Timestamp: org.Updated, - Id: org.Id, + Id: org.ID, Name: org.Name, }) @@ -139,14 +151,14 @@ func (ss *sqlStore) Update(ctx context.Context, cmd *org.UpdateOrgCommand) error func isOrgNameTaken(name string, existingId int64, sess *sqlstore.DBSession) (bool, error) { // check if org name is taken - var org models.Org + var org org.Org exists, err := sess.Where("name=?", name).Get(&org) if err != nil { return false, nil } - if exists && existingId != org.Id { + if exists && existingId != org.ID { return true, nil } @@ -156,7 +168,7 @@ func isOrgNameTaken(name string, existingId int64, sess *sqlstore.DBSession) (bo // TODO: refactor move logic to service method func (ss *sqlStore) UpdateAddress(ctx context.Context, cmd *org.UpdateOrgAddressCommand) error { return ss.db.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { - org := models.Org{ + org := org.Org{ Address1: cmd.Address1, Address2: cmd.Address2, City: cmd.City, @@ -173,7 +185,7 @@ func (ss *sqlStore) UpdateAddress(ctx context.Context, cmd *org.UpdateOrgAddress sess.PublishAfterCommit(&events.OrgUpdated{ Timestamp: org.Updated, - Id: org.Id, + Id: org.ID, Name: org.Name, }) @@ -319,3 +331,186 @@ func (ss *sqlStore) CreateWithMember(ctx context.Context, cmd *org.CreateOrgComm } return &orga, nil } + +func (ss *sqlStore) AddOrgUser(ctx context.Context, cmd *org.AddOrgUserCommand) error { + return ss.db.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { + // check if user exists + var usr user.User + session := sess.ID(cmd.UserID) + if !cmd.AllowAddingServiceAccount { + session = session.Where(ss.notServiceAccountFilter()) + } + + if exists, err := session.Get(&usr); err != nil { + return err + } else if !exists { + return user.ErrUserNotFound + } + + if res, err := sess.Query("SELECT 1 from org_user WHERE org_id=? and user_id=?", cmd.OrgID, usr.ID); err != nil { + return err + } else if len(res) == 1 { + return models.ErrOrgUserAlreadyAdded + } + + if res, err := sess.Query("SELECT 1 from org WHERE id=?", cmd.OrgID); err != nil { + return err + } else if len(res) != 1 { + return models.ErrOrgNotFound + } + + entity := org.OrgUser{ + OrgID: cmd.OrgID, + UserID: cmd.UserID, + Role: cmd.Role, + Created: time.Now(), + Updated: time.Now(), + } + + _, err := sess.Insert(&entity) + if err != nil { + return err + } + + var userOrgs []*org.UserOrgDTO + sess.Table("org_user") + sess.Join("INNER", "org", "org_user.org_id=org.id") + sess.Where("org_user.user_id=? AND org_user.org_id=?", usr.ID, usr.OrgID) + sess.Cols("org.name", "org_user.role", "org_user.org_id") + err = sess.Find(&userOrgs) + + if err != nil { + return err + } + + if len(userOrgs) == 0 { + return setUsingOrgInTransaction(sess, usr.ID, cmd.OrgID) + } + + return nil + }) +} + +func setUsingOrgInTransaction(sess *sqlstore.DBSession, userID int64, orgID int64) error { + user := user.User{ + ID: userID, + OrgID: orgID, + } + + _, err := sess.ID(userID).Update(&user) + return err +} + +func (ss *sqlStore) UpdateOrgUser(ctx context.Context, cmd *org.UpdateOrgUserCommand) error { + return ss.db.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { + var orgUser org.OrgUser + exists, err := sess.Where("org_id=? AND user_id=?", cmd.OrgID, cmd.UserID).Get(&orgUser) + if err != nil { + return err + } + + if !exists { + return models.ErrOrgUserNotFound + } + + orgUser.Role = cmd.Role + orgUser.Updated = time.Now() + _, err = sess.ID(orgUser.ID).Update(&orgUser) + if err != nil { + return err + } + + return validateOneAdminLeftInOrg(cmd.OrgID, sess) + }) +} + +// validate that there is an org admin user left +func validateOneAdminLeftInOrg(orgID int64, sess *sqlstore.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 +} + +func (ss *sqlStore) GetOrgUsers(ctx context.Context, query *org.GetOrgUsersQuery) ([]*org.OrgUserDTO, error) { + result := 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) + + if query.UserID != 0 { + whereConditions = append(whereConditions, "org_user.user_id = ?") + whereParams = append(whereParams, query.UserID) + } + + whereConditions = append(whereConditions, fmt.Sprintf("%s.is_service_account = ?", ss.dialect.Quote("user"))) + whereParams = append(whereParams, ss.dialect.BooleanStr(false)) + + if query.User == nil { + ss.log.Warn("Query user not set for filtering.") + } + + if !query.DontEnforceAccessControl && !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 { + sess.Limit(query.Limit, 0) + } + + sess.Cols( + "org_user.org_id", + "org_user.user_id", + "user.email", + "user.name", + "user.login", + "org_user.role", + "user.last_seen_at", + "user.created", + "user.updated", + "user.is_disabled", + ) + sess.Asc("user.email", "user.login") + + if err := sess.Find(&result); err != nil { + return err + } + + for _, user := range result { + user.LastSeenAtAge = util.GetAgeString(user.LastSeenAt) + } + + return nil + }) + if err != nil { + return nil, err + } + return result, nil +} diff --git a/pkg/services/org/orgimpl/store_test.go b/pkg/services/org/orgimpl/store_test.go index e82141704e0..0a8c638da2b 100644 --- a/pkg/services/org/orgimpl/store_test.go +++ b/pkg/services/org/orgimpl/store_test.go @@ -3,12 +3,21 @@ package orgimpl import ( "context" "fmt" + "strings" "testing" "time" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "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/stretchr/testify/require" + "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/setting" ) func TestIntegrationOrgDataAccess(t *testing.T) { @@ -158,6 +167,7 @@ func TestIntegrationOrgUserDataAccess(t *testing.T) { orgUserStore := sqlStore{ db: ss, dialect: ss.GetDialect(), + cfg: setting.NewCfg(), } t.Run("org user inserted", func(t *testing.T) { @@ -210,4 +220,344 @@ func TestIntegrationOrgUserDataAccess(t *testing.T) { // require.NoError(t, err) // require.Equal(t, len(query.Result), 2) // }) + + t.Run("Update org users", func(t *testing.T) { + _, err := orgUserStore.InsertOrgUser(context.Background(), &org.OrgUser{ + ID: 1, + OrgID: 1, + UserID: 1, + Created: time.Now(), + Updated: time.Now(), + }) + require.NoError(t, err) + + cmd := &org.UpdateOrgUserCommand{ + Role: org.RoleAdmin, + UserID: 1, + OrgID: 1, + } + err = orgUserStore.UpdateOrgUser(context.Background(), cmd) + require.NoError(t, err) + }) + t.Run("GetOrgUsers and UpdateOrgUsers", func(t *testing.T) { + ss := sqlstore.InitTestDB(t) + ac1cmd := user.CreateUserCommand{Login: "ac1", Email: "ac1@test.com", Name: "ac1 name"} + ac2cmd := user.CreateUserCommand{Login: "ac2", Email: "ac2@test.com", Name: "ac2 name", IsAdmin: true} + ac1, err := ss.CreateUser(context.Background(), ac1cmd) + require.NoError(t, err) + ac2, err := ss.CreateUser(context.Background(), ac2cmd) + require.NoError(t, err) + cmd := org.AddOrgUserCommand{ + OrgID: ac1.OrgID, + UserID: ac2.ID, + Role: org.RoleViewer, + } + + err = orgUserStore.AddOrgUser(context.Background(), &cmd) + require.NoError(t, err) + + t.Run("Can update org user role", func(t *testing.T) { + updateCmd := org.UpdateOrgUserCommand{OrgID: ac1.OrgID, UserID: ac2.ID, Role: org.RoleAdmin} + err = orgUserStore.UpdateOrgUser(context.Background(), &updateCmd) + require.NoError(t, err) + + orgUsersQuery := org.GetOrgUsersQuery{ + OrgID: ac1.OrgID, + User: &user.SignedInUser{ + OrgID: ac1.OrgID, + Permissions: map[int64]map[string][]string{ac1.OrgID: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, + }, + } + result, err := orgUserStore.GetOrgUsers(context.Background(), &orgUsersQuery) + require.NoError(t, err) + + require.EqualValues(t, result[1].Role, org.RoleAdmin) + }) + t.Run("Can get organization users", func(t *testing.T) { + query := org.GetOrgUsersQuery{ + OrgID: ac1.OrgID, + User: &user.SignedInUser{ + OrgID: ac1.OrgID, + Permissions: map[int64]map[string][]string{ac1.OrgID: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, + }, + } + result, err := orgUserStore.GetOrgUsers(context.Background(), &query) + + require.NoError(t, err) + require.Equal(t, len(result), 2) + require.Equal(t, result[0].Role, "Admin") + }) + + t.Run("Can get organization users with query", func(t *testing.T) { + query := org.GetOrgUsersQuery{ + OrgID: ac1.OrgID, + Query: "ac1", + User: &user.SignedInUser{ + OrgID: ac1.OrgID, + Permissions: map[int64]map[string][]string{ac1.OrgID: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, + }, + } + result, err := orgUserStore.GetOrgUsers(context.Background(), &query) + + require.NoError(t, err) + require.Equal(t, len(result), 1) + require.Equal(t, result[0].Email, ac1.Email) + }) + t.Run("Can get organization users with query and limit", func(t *testing.T) { + query := org.GetOrgUsersQuery{ + OrgID: ac1.OrgID, + Query: "ac", + Limit: 1, + User: &user.SignedInUser{ + OrgID: ac1.OrgID, + Permissions: map[int64]map[string][]string{ac1.OrgID: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, + }, + } + result, err := orgUserStore.GetOrgUsers(context.Background(), &query) + + require.NoError(t, err) + require.Equal(t, len(result), 1) + 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) + 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) + }) + }) +} + +// This test will be refactore after the CRUD store refactor +func TestSQLStore_AddOrgUser(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } + var orgID int64 = 1 + 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: orgID, + }) + require.NoError(t, err) + + // create a service account with no org + sa, err := store.CreateUser(context.Background(), user.CreateUserCommand{ + Login: "sa-no-org", + IsServiceAccount: true, + SkipOrgSetup: true, + }) + + require.NoError(t, err) + require.Equal(t, int64(-1), sa.OrgID) + + // assign the sa to the org but without the override. should fail + err = orgUserStore.AddOrgUser(context.Background(), &org.AddOrgUserCommand{ + Role: "Viewer", + OrgID: orgID, + UserID: sa.ID, + }) + require.Error(t, err) + + // assign the sa to the org with the override. should succeed + err = orgUserStore.AddOrgUser(context.Background(), &org.AddOrgUserCommand{ + Role: "Viewer", + OrgID: orgID, + UserID: sa.ID, + AllowAddingServiceAccount: true, + }) + + require.NoError(t, err) + + // assert the org has been correctly set + saFound := new(user.User) + err = store.WithDbSession(context.Background(), func(sess *sqlstore.DBSession) error { + has, err := sess.ID(sa.ID).Get(saFound) + if err != nil { + return err + } else if !has { + return user.ErrUserNotFound + } + return nil + }) + + require.NoError(t, err) + require.Equal(t, saFound.OrgID, orgID) +} + +func TestSQLStore_GetOrgUsers(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } + tests := []struct { + desc string + query *org.GetOrgUsersQuery + expectedNumUsers int + }{ + { + desc: "should return all users", + query: &org.GetOrgUsersQuery{ + 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.GetOrgUsersQuery{ + 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.GetOrgUsersQuery{ + 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.IsEnterprise = true + defer func() { + orgUserStore.cfg.IsEnterprise = false + }() + seedOrgUsers(t, &orgUserStore, store, 10) + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + result, err := orgUserStore.GetOrgUsers(context.Background(), tt.query) + require.NoError(t, err) + require.Len(t, result, tt.expectedNumUsers) + + if !hasWildcardScope(tt.query.User, accesscontrol.ActionOrgUsersRead) { + for _, u := range result { + assert.Contains(t, tt.query.User.Permissions[tt.query.User.OrgID][accesscontrol.ActionOrgUsersRead], fmt.Sprintf("users:id:%d", u.UserID)) + } + } + }) + } +} + +func seedOrgUsers(t *testing.T, orgUserStore store, store *sqlstore.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 = orgUserStore.AddOrgUser(context.Background(), &org.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 +} + +func TestSQLStore_GetOrgUsers_PopulatesCorrectly(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } + // The millisecond part is not stored in the DB + constNow := time.Date(2022, 8, 17, 20, 34, 58, 0, time.UTC) + sqlstore.MockTimeNow(constNow) + defer sqlstore.ResetTimeNow() + + store := sqlstore.InitTestDB(t, sqlstore.InitTestDBOpt{}) + orgUserStore := sqlStore{ + db: store, + dialect: store.GetDialect(), + cfg: setting.NewCfg(), + } + _, err := store.CreateUser(context.Background(), user.CreateUserCommand{ + Login: "Admin", + Email: "admin@localhost", + OrgID: 1, + }) + require.NoError(t, err) + + newUser, err := store.CreateUser(context.Background(), user.CreateUserCommand{ + Login: "Viewer", + Email: "viewer@localhost", + OrgID: 1, + IsDisabled: true, + Name: "Viewer Localhost", + }) + require.NoError(t, err) + + err = orgUserStore.AddOrgUser(context.Background(), &org.AddOrgUserCommand{ + Role: "Viewer", + OrgID: 1, + UserID: newUser.ID, + }) + require.NoError(t, err) + + query := &org.GetOrgUsersQuery{ + OrgID: 1, + UserID: newUser.ID, + User: &user.SignedInUser{ + OrgID: 1, + Permissions: map[int64]map[string][]string{1: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, + }, + } + result, err := orgUserStore.GetOrgUsers(context.Background(), query) + require.NoError(t, err) + require.Len(t, result, 1) + + actual := result[0] + assert.Equal(t, int64(1), actual.OrgID) + assert.Equal(t, newUser.ID, actual.UserID) + assert.Equal(t, "viewer@localhost", actual.Email) + assert.Equal(t, "Viewer Localhost", actual.Name) + assert.Equal(t, "Viewer", actual.Login) + assert.Equal(t, "Viewer", actual.Role) + assert.Equal(t, constNow.AddDate(-10, 0, 0), actual.LastSeenAt) + assert.Equal(t, constNow, actual.Created) + assert.Equal(t, constNow, actual.Updated) + assert.Equal(t, true, actual.IsDisabled) } diff --git a/pkg/services/sqlstore/org_test.go b/pkg/services/sqlstore/org_test.go index 1434f4eabe7..20a5db63659 100644 --- a/pkg/services/sqlstore/org_test.go +++ b/pkg/services/sqlstore/org_test.go @@ -202,24 +202,6 @@ func TestIntegrationAccountDataAccess(t *testing.T) { require.NoError(t, err) }) - t.Run("Can update org user role", func(t *testing.T) { - updateCmd := models.UpdateOrgUserCommand{OrgId: ac1.OrgID, UserId: ac2.ID, Role: org.RoleAdmin} - err = sqlStore.UpdateOrgUser(context.Background(), &updateCmd) - require.NoError(t, err) - - orgUsersQuery := models.GetOrgUsersQuery{ - OrgId: ac1.OrgID, - User: &user.SignedInUser{ - OrgID: ac1.OrgID, - Permissions: map[int64]map[string][]string{ac1.OrgID: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, - }, - } - err = sqlStore.GetOrgUsers(context.Background(), &orgUsersQuery) - require.NoError(t, err) - - require.EqualValues(t, orgUsersQuery.Result[1].Role, org.RoleAdmin) - }) - t.Run("Can get logged in user projection", func(t *testing.T) { query := models.GetSignedInUserQuery{UserId: ac2.ID} err := sqlStore.GetSignedInUser(context.Background(), &query) @@ -242,54 +224,6 @@ func TestIntegrationAccountDataAccess(t *testing.T) { require.Equal(t, len(query.Result), 2) }) - t.Run("Can get organization users", func(t *testing.T) { - query := models.GetOrgUsersQuery{ - OrgId: ac1.OrgID, - User: &user.SignedInUser{ - OrgID: ac1.OrgID, - Permissions: map[int64]map[string][]string{ac1.OrgID: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, - }, - } - err := sqlStore.GetOrgUsers(context.Background(), &query) - - require.NoError(t, err) - require.Equal(t, len(query.Result), 2) - require.Equal(t, query.Result[0].Role, "Admin") - }) - - t.Run("Can get organization users with query", func(t *testing.T) { - query := models.GetOrgUsersQuery{ - OrgId: ac1.OrgID, - Query: "ac1", - User: &user.SignedInUser{ - OrgID: ac1.OrgID, - Permissions: map[int64]map[string][]string{ac1.OrgID: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, - }, - } - err := sqlStore.GetOrgUsers(context.Background(), &query) - - require.NoError(t, err) - require.Equal(t, len(query.Result), 1) - require.Equal(t, query.Result[0].Email, ac1.Email) - }) - - t.Run("Can get organization users with query and limit", func(t *testing.T) { - query := models.GetOrgUsersQuery{ - OrgId: ac1.OrgID, - Query: "ac", - Limit: 1, - User: &user.SignedInUser{ - OrgID: ac1.OrgID, - Permissions: map[int64]map[string][]string{ac1.OrgID: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, - }, - } - err := sqlStore.GetOrgUsers(context.Background(), &query) - - require.NoError(t, err) - require.Equal(t, len(query.Result), 1) - require.Equal(t, query.Result[0].Email, ac1.Email) - }) - t.Run("Can set using org", func(t *testing.T) { cmd := models.SetUsingOrgCommand{UserId: ac2.ID, OrgId: ac1.OrgID} err := sqlStore.SetUsingOrg(context.Background(), &cmd) @@ -341,12 +275,6 @@ func TestIntegrationAccountDataAccess(t *testing.T) { require.Equal(t, err, models.ErrLastOrgAdmin) }) - t.Run("Cannot update role so no one is admin user", func(t *testing.T) { - cmd := models.UpdateOrgUserCommand{OrgId: ac1.OrgID, UserId: ac1.ID, Role: org.RoleViewer} - err := sqlStore.UpdateOrgUser(context.Background(), &cmd) - require.Equal(t, err, models.ErrLastOrgAdmin) - }) - t.Run("Given an org user with dashboard permissions", func(t *testing.T) { ac3cmd := user.CreateUserCommand{Login: "ac3", Email: "ac3@test.com", Name: "ac3 name", IsAdmin: false} ac3, err := sqlStore.CreateUser(context.Background(), ac3cmd) @@ -361,17 +289,6 @@ func TestIntegrationAccountDataAccess(t *testing.T) { err = sqlStore.AddOrgUser(context.Background(), &orgUserCmd) require.NoError(t, err) - query := models.GetOrgUsersQuery{ - OrgId: ac1.OrgID, - User: &user.SignedInUser{ - OrgID: ac1.OrgID, - Permissions: map[int64]map[string][]string{ac1.OrgID: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, - }, - } - err = sqlStore.GetOrgUsers(context.Background(), &query) - require.NoError(t, err) - // require.Equal(t, len(query.Result), 3) - dash1 := insertTestDashboard(t, sqlStore, "1 test dash", ac1.OrgID, 0, false, "prod", "webapp") dash2 := insertTestDashboard(t, sqlStore, "2 test dash", ac3.OrgID, 0, false, "prod", "webapp") diff --git a/pkg/services/sqlstore/org_users.go b/pkg/services/sqlstore/org_users.go index 1e4bfc237c8..7f376a4416f 100644 --- a/pkg/services/sqlstore/org_users.go +++ b/pkg/services/sqlstore/org_users.go @@ -71,103 +71,6 @@ func (ss *SQLStore) AddOrgUser(ctx context.Context, cmd *models.AddOrgUserComman }) } -func (ss *SQLStore) UpdateOrgUser(ctx context.Context, cmd *models.UpdateOrgUserCommand) error { - return ss.WithTransactionalDbSession(ctx, func(sess *DBSession) error { - var orgUser models.OrgUser - exists, err := sess.Where("org_id=? AND user_id=?", cmd.OrgId, cmd.UserId).Get(&orgUser) - if err != nil { - return err - } - - if !exists { - return models.ErrOrgUserNotFound - } - - orgUser.Role = cmd.Role - orgUser.Updated = time.Now() - _, err = sess.ID(orgUser.Id).Update(&orgUser) - if err != nil { - return err - } - - return validateOneAdminLeftInOrg(cmd.OrgId, sess) - }) -} - -func (ss *SQLStore) GetOrgUsers(ctx context.Context, query *models.GetOrgUsersQuery) error { - return ss.WithDbSession(ctx, func(dbSession *DBSession) error { - query.Result = 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) - - if query.UserID != 0 { - whereConditions = append(whereConditions, "org_user.user_id = ?") - whereParams = append(whereParams, query.UserID) - } - - whereConditions = append(whereConditions, fmt.Sprintf("%s.is_service_account = ?", ss.Dialect.Quote("user"))) - whereParams = append(whereParams, ss.Dialect.BooleanStr(false)) - - if query.User == nil { - ss.log.Warn("Query user not set for filtering.") - } - - if !query.DontEnforceAccessControl && !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 { - sess.Limit(query.Limit, 0) - } - - sess.Cols( - "org_user.org_id", - "org_user.user_id", - "user.email", - "user.name", - "user.login", - "org_user.role", - "user.last_seen_at", - "user.created", - "user.updated", - "user.is_disabled", - ) - sess.Asc("user.email", "user.login") - - if err := sess.Find(&query.Result); err != nil { - return err - } - - for _, user := range query.Result { - user.LastSeenAtAge = util.GetAgeString(user.LastSeenAt) - } - - 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{ diff --git a/pkg/services/sqlstore/org_users_test.go b/pkg/services/sqlstore/org_users_test.go index b85fb0eea2d..81514aea3ac 100644 --- a/pkg/services/sqlstore/org_users_test.go +++ b/pkg/services/sqlstore/org_users_test.go @@ -5,7 +5,6 @@ import ( "fmt" "strings" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -15,130 +14,6 @@ import ( "github.com/grafana/grafana/pkg/services/user" ) -type getOrgUsersTestCase struct { - desc string - query *models.GetOrgUsersQuery - expectedNumUsers int -} - -func TestSQLStore_GetOrgUsers(t *testing.T) { - tests := []getOrgUsersTestCase{ - { - desc: "should return all users", - query: &models.GetOrgUsersQuery{ - 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.GetOrgUsersQuery{ - 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.GetOrgUsersQuery{ - 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{}) - store.Cfg.IsEnterprise = true - defer func() { - store.Cfg.IsEnterprise = false - }() - seedOrgUsers(t, store, 10) - - for _, tt := range tests { - t.Run(tt.desc, func(t *testing.T) { - err := store.GetOrgUsers(context.Background(), tt.query) - require.NoError(t, err) - require.Len(t, tt.query.Result, tt.expectedNumUsers) - - if !hasWildcardScope(tt.query.User, ac.ActionOrgUsersRead) { - for _, u := range tt.query.Result { - assert.Contains(t, tt.query.User.Permissions[tt.query.User.OrgID][ac.ActionOrgUsersRead], fmt.Sprintf("users:id:%d", u.UserId)) - } - } - }) - } -} - -func TestSQLStore_GetOrgUsers_PopulatesCorrectly(t *testing.T) { - // The millisecond part is not stored in the DB - constNow := time.Date(2022, 8, 17, 20, 34, 58, 0, time.UTC) - MockTimeNow(constNow) - defer ResetTimeNow() - - store := InitTestDB(t, InitTestDBOpt{}) - _, err := store.CreateUser(context.Background(), user.CreateUserCommand{ - Login: "Admin", - Email: "admin@localhost", - OrgID: 1, - }) - require.NoError(t, err) - - newUser, err := store.CreateUser(context.Background(), user.CreateUserCommand{ - Login: "Viewer", - Email: "viewer@localhost", - OrgID: 1, - IsDisabled: true, - Name: "Viewer Localhost", - }) - require.NoError(t, err) - - err = store.AddOrgUser(context.Background(), &models.AddOrgUserCommand{ - Role: "Viewer", - OrgId: 1, - UserId: newUser.ID, - }) - require.NoError(t, err) - - query := &models.GetOrgUsersQuery{ - OrgId: 1, - UserID: newUser.ID, - User: &user.SignedInUser{ - OrgID: 1, - Permissions: map[int64]map[string][]string{1: {ac.ActionOrgUsersRead: {ac.ScopeUsersAll}}}, - }, - } - err = store.GetOrgUsers(context.Background(), query) - require.NoError(t, err) - require.Len(t, query.Result, 1) - - actual := query.Result[0] - assert.Equal(t, int64(1), actual.OrgId) - assert.Equal(t, newUser.ID, actual.UserId) - assert.Equal(t, "viewer@localhost", actual.Email) - assert.Equal(t, "Viewer Localhost", actual.Name) - assert.Equal(t, "Viewer", actual.Login) - assert.Equal(t, "Viewer", actual.Role) - assert.Equal(t, constNow.AddDate(-10, 0, 0), actual.LastSeenAt) - assert.Equal(t, constNow, actual.Created) - assert.Equal(t, constNow, actual.Updated) - assert.Equal(t, true, actual.IsDisabled) -} - type searchOrgUsersTestCase struct { desc string query *models.SearchOrgUsersQuery diff --git a/pkg/services/sqlstore/store.go b/pkg/services/sqlstore/store.go index d34508d256b..e964f938313 100644 --- a/pkg/services/sqlstore/store.go +++ b/pkg/services/sqlstore/store.go @@ -41,9 +41,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 - AddOrgUser(ctx context.Context, cmd *models.AddOrgUserCommand) error - UpdateOrgUser(ctx context.Context, cmd *models.UpdateOrgUserCommand) error - GetOrgUsers(ctx context.Context, query *models.GetOrgUsersQuery) error SearchOrgUsers(ctx context.Context, query *models.SearchOrgUsersQuery) error RemoveOrgUser(ctx context.Context, cmd *models.RemoveOrgUserCommand) error Migrate(bool) error