From c8db771939f783d1533421876ccf17648d77019e Mon Sep 17 00:00:00 2001 From: Jo Date: Fri, 24 Feb 2023 17:08:44 +0000 Subject: [PATCH] Users: Fix org user always getting org id = 1 on auto assign false (#63708) * fix org user always getting org id = 1 on auto assign false * make tests explicit * use correct cfg in service accounts * fix api tests * fix database test of ac * fix InsertOrgUser returning affected rows as orgID --- pkg/api/org_users_test.go | 4 ++ .../accesscontrol/database/database_test.go | 9 +++++ pkg/services/org/orgimpl/org.go | 12 +++--- pkg/services/org/orgimpl/org_test.go | 39 ++++++++++++------- pkg/services/org/orgimpl/store.go | 13 +++++-- pkg/services/org/orgimpl/store_test.go | 12 ++++-- .../serviceaccounts/database/store_test.go | 3 +- pkg/services/user/userimpl/user.go | 4 +- 8 files changed, 65 insertions(+), 31 deletions(-) diff --git a/pkg/api/org_users_test.go b/pkg/api/org_users_test.go index 5c19c3c9784..af8901699a2 100644 --- a/pkg/api/org_users_test.go +++ b/pkg/api/org_users_test.go @@ -45,6 +45,10 @@ func setUpGetOrgUsersDB(t *testing.T, sqlStore *sqlstore.SQLStore) { usrSvc, err := userimpl.ProvideService(sqlStore, orgService, sqlStore.Cfg, nil, nil, quotaService, supportbundlestest.NewFakeBundleService()) require.NoError(t, err) + id, err := orgService.GetOrCreate(context.Background(), "testOrg") + require.NoError(t, err) + require.Equal(t, testOrgID, id) + _, err = usrSvc.Create(context.Background(), &user.CreateUserCommand{Email: "testUser@grafana.com", Login: testUserLogin}) require.NoError(t, err) _, err = usrSvc.Create(context.Background(), &user.CreateUserCommand{Email: "user1@grafana.com", Login: "user1"}) diff --git a/pkg/services/accesscontrol/database/database_test.go b/pkg/services/accesscontrol/database/database_test.go index 3c54802cfc1..29a693a1fdd 100644 --- a/pkg/services/accesscontrol/database/database_test.go +++ b/pkg/services/accesscontrol/database/database_test.go @@ -263,6 +263,7 @@ func createUsersAndTeams(t *testing.T, svcs helperServices, orgID int64, users [ IsAdmin: users[i].isAdmin, }) require.NoError(t, err) + require.Equal(t, orgID, user.OrgID) // User is not member of the org if users[i].orgRole == "" { @@ -292,11 +293,19 @@ func createUsersAndTeams(t *testing.T, svcs helperServices, orgID int64, users [ func setupTestEnv(t testing.TB) (*AccessControlStore, rs.Store, user.Service, team.Service, org.Service) { sql, cfg := db.InitTestDBwithCfg(t) + cfg.AutoAssignOrg = true + cfg.AutoAssignOrgRole = "Viewer" + cfg.AutoAssignOrgId = 1 acstore := ProvideService(sql) permissionStore := rs.NewStore(sql) teamService := teamimpl.ProvideService(sql, cfg) orgService, err := orgimpl.ProvideService(sql, cfg, quotatest.New(false, nil)) require.NoError(t, err) + + orgID, err := orgService.GetOrCreate(context.Background(), "test") + require.Equal(t, int64(1), orgID) + require.NoError(t, err) + userService, err := userimpl.ProvideService(sql, orgService, cfg, teamService, localcache.ProvideService(), quotatest.New(false, nil), supportbundlestest.NewFakeBundleService()) require.NoError(t, err) return acstore, permissionStore, userService, teamService, orgService diff --git a/pkg/services/org/orgimpl/org.go b/pkg/services/org/orgimpl/org.go index 05a296c1b24..3c6c9ffed3b 100644 --- a/pkg/services/org/orgimpl/org.go +++ b/pkg/services/org/orgimpl/org.go @@ -58,7 +58,7 @@ func (s *Service) GetIDForNewUser(ctx context.Context, cmd org.GetOrgIDForNewUse return -1, nil } - if setting.AutoAssignOrg && cmd.OrgID != 0 { + if s.cfg.AutoAssignOrg && cmd.OrgID != 0 { _, err := s.store.Get(ctx, cmd.OrgID) if err != nil { return -1, err @@ -73,7 +73,7 @@ func (s *Service) GetIDForNewUser(ctx context.Context, cmd org.GetOrgIDForNewUse } orga.Name = orgName - if setting.AutoAssignOrg { + if s.cfg.AutoAssignOrg { orga, err := s.store.Get(ctx, int64(s.cfg.AutoAssignOrgId)) if err != nil { return 0, err @@ -81,14 +81,14 @@ func (s *Service) GetIDForNewUser(ctx context.Context, cmd org.GetOrgIDForNewUse if orga.ID != 0 { return orga.ID, nil } - if setting.AutoAssignOrgId != 1 { + if s.cfg.AutoAssignOrgId != 1 { s.log.Error("Could not create user: organization ID does not exist", "orgID", - setting.AutoAssignOrgId) + s.cfg.AutoAssignOrgId) return 0, fmt.Errorf("could not create user: organization ID %d does not exist", - setting.AutoAssignOrgId) + s.cfg.AutoAssignOrgId) } orga.Name = MainOrgName - orga.ID = int64(setting.AutoAssignOrgId) + orga.ID = int64(s.cfg.AutoAssignOrgId) } else { orga.Name = orgName } diff --git a/pkg/services/org/orgimpl/org_test.go b/pkg/services/org/orgimpl/org_test.go index 952a96fab88..639f101d303 100644 --- a/pkg/services/org/orgimpl/org_test.go +++ b/pkg/services/org/orgimpl/org_test.go @@ -4,6 +4,7 @@ import ( "context" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/services/org" @@ -19,35 +20,47 @@ func TestOrgService(t *testing.T) { } t.Run("create org", func(t *testing.T) { - _, err := orgService.GetIDForNewUser(context.Background(), org.GetOrgIDForNewUserCommand{}) + orgService.cfg.AutoAssignOrg = false + orgService.cfg.AutoAssignOrgId = 1 + orgStore.ExpectedOrgID = 3 + id, err := orgService.GetIDForNewUser(context.Background(), org.GetOrgIDForNewUserCommand{}) require.NoError(t, err) + assert.Equal(t, int64(3), id) }) - t.Run("create org", func(t *testing.T) { - _, err := orgService.GetIDForNewUser(context.Background(), org.GetOrgIDForNewUserCommand{}) + // Should return a new org instead of the org defined in the command + t.Run("no autoassign - org defined", func(t *testing.T) { + orgService.cfg.AutoAssignOrg = false + orgService.cfg.AutoAssignOrgId = 1 + orgStore.ExpectedOrgID = 3 + orgStore.ExpectedOrg = &org.Org{ID: 1} + id, err := orgService.GetIDForNewUser(context.Background(), org.GetOrgIDForNewUserCommand{OrgID: 1}) require.NoError(t, err) + assert.Equal(t, int64(3), id) }) t.Run("create org with auto assign org ID", func(t *testing.T) { - setting.AutoAssignOrg = true - setting.AutoAssignOrgId = 1 + orgService.cfg.AutoAssignOrg = true + orgService.cfg.AutoAssignOrgId = 1 orgStore.ExpectedOrgID = 1 - orgStore.ExpectedOrg = &org.Org{} - _, err := orgService.GetIDForNewUser(context.Background(), org.GetOrgIDForNewUserCommand{}) + orgStore.ExpectedOrg = &org.Org{ID: 1} + id, err := orgService.GetIDForNewUser(context.Background(), org.GetOrgIDForNewUserCommand{}) require.NoError(t, err) + assert.Equal(t, int64(1), id) }) t.Run("create org with auto assign org ID and orgID", func(t *testing.T) { - setting.AutoAssignOrg = true - setting.AutoAssignOrgId = 1 + orgService.cfg.AutoAssignOrg = true + orgService.cfg.AutoAssignOrgId = 1 orgStore.ExpectedOrgID = 1 - orgStore.ExpectedOrg = &org.Org{} - _, err := orgService.GetIDForNewUser(context.Background(), org.GetOrgIDForNewUserCommand{OrgID: 1}) + orgStore.ExpectedOrg = &org.Org{ID: 1} + id, err := orgService.GetIDForNewUser(context.Background(), org.GetOrgIDForNewUserCommand{OrgID: 1}) require.NoError(t, err) + assert.Equal(t, int64(1), id) }) - setting.AutoAssignOrg = false - setting.AutoAssignOrgId = 0 + orgService.cfg.AutoAssignOrg = false + orgService.cfg.AutoAssignOrgId = 0 t.Run("delete user from all orgs", func(t *testing.T) { err := orgService.DeleteUserFromAll(context.Background(), 1) diff --git a/pkg/services/org/orgimpl/store.go b/pkg/services/org/orgimpl/store.go index aafa97ac96d..462a6bd0b6f 100644 --- a/pkg/services/org/orgimpl/store.go +++ b/pkg/services/org/orgimpl/store.go @@ -25,7 +25,9 @@ const MainOrgName = "Main Org." type store interface { Get(context.Context, int64) (*org.Org, error) + // Insert adds a new organization. returns organization id Insert(context.Context, *org.Org) (int64, error) + // InsertOrgUser adds a new membership record for a user in an organization. returns membership id InsertOrgUser(context.Context, *org.OrgUser) (int64, error) DeleteUserFromAll(context.Context, int64) error Update(ctx context.Context, cmd *org.UpdateOrgCommand) error @@ -76,9 +78,12 @@ func (ss *sqlStore) Insert(ctx context.Context, org *org.Org) (int64, error) { var orgID int64 var err error err = ss.db.WithDbSession(ctx, func(sess *db.Session) error { - if orgID, err = sess.InsertOne(org); err != nil { + if _, err = sess.Insert(org); err != nil { return err } + + orgID = org.ID + if org.ID != 0 { // it sets the setval in the sequence if err := ss.dialect.PostInsertId("org", sess.Session); err != nil { @@ -98,11 +103,11 @@ func (ss *sqlStore) Insert(ctx context.Context, org *org.Org) (int64, error) { return orgID, nil } +// InsertOrgUser adds a new membership record for a user in an organization. func (ss *sqlStore) InsertOrgUser(ctx context.Context, cmd *org.OrgUser) (int64, error) { - var orgID int64 var err error err = ss.db.WithDbSession(ctx, func(sess *db.Session) error { - if orgID, err = sess.Insert(cmd); err != nil { + if _, err = sess.Insert(cmd); err != nil { return err } return nil @@ -110,7 +115,7 @@ func (ss *sqlStore) InsertOrgUser(ctx context.Context, cmd *org.OrgUser) (int64, if err != nil { return 0, err } - return orgID, nil + return cmd.ID, nil } func (ss *sqlStore) DeleteUserFromAll(ctx context.Context, userID int64) error { diff --git a/pkg/services/org/orgimpl/store_test.go b/pkg/services/org/orgimpl/store_test.go index 37edce6d778..5a72372b30a 100644 --- a/pkg/services/org/orgimpl/store_test.go +++ b/pkg/services/org/orgimpl/store_test.go @@ -415,6 +415,10 @@ func TestIntegrationOrgUserDataAccess(t *testing.T) { t.Run("Given single org and 2 users inserted", func(t *testing.T) { ss = db.InitTestDB(t) + ss.Cfg.AutoAssignOrg = true + ss.Cfg.AutoAssignOrgId = 1 + ss.Cfg.AutoAssignOrgRole = "Viewer" + _, usrSvc := createOrgAndUserSvc(t, ss, ss.Cfg) testUser := &user.SignedInUser{ @@ -422,17 +426,17 @@ func TestIntegrationOrgUserDataAccess(t *testing.T) { 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 := usrSvc.CreateUserForTests(context.Background(), ac1cmd) testUser.OrgID = ac1.OrgID + require.Equal(t, int64(1), ac1.OrgID) require.NoError(t, err) - _, err = usrSvc.Create(context.Background(), ac2cmd) + + ac2, err := usrSvc.Create(context.Background(), ac2cmd) + require.Equal(t, int64(1), ac2.OrgID) require.NoError(t, err) t.Run("Can get organization users paginated with query", func(t *testing.T) { diff --git a/pkg/services/serviceaccounts/database/store_test.go b/pkg/services/serviceaccounts/database/store_test.go index f281bd746e9..0f0b11ae843 100644 --- a/pkg/services/serviceaccounts/database/store_test.go +++ b/pkg/services/serviceaccounts/database/store_test.go @@ -20,7 +20,6 @@ import ( "github.com/grafana/grafana/pkg/services/supportbundles/supportbundlestest" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/services/user/userimpl" - "github.com/grafana/grafana/pkg/setting" ) // Service Account should not create an org on its own @@ -118,7 +117,7 @@ func setupTestDatabase(t *testing.T) (*sqlstore.SQLStore, *ServiceAccountsStoreI apiKeyService, err := apikeyimpl.ProvideService(db, db.Cfg, quotaService) require.NoError(t, err) kvStore := kvstore.ProvideService(db) - orgService, err := orgimpl.ProvideService(db, setting.NewCfg(), quotaService) + orgService, err := orgimpl.ProvideService(db, db.Cfg, quotaService) require.NoError(t, err) userSvc, err := userimpl.ProvideService(db, orgService, db.Cfg, nil, nil, quotaService, supportbundlestest.NewFakeBundleService()) require.NoError(t, err) diff --git a/pkg/services/user/userimpl/user.go b/pkg/services/user/userimpl/user.go index c49b54f3530..354374bb536 100644 --- a/pkg/services/user/userimpl/user.go +++ b/pkg/services/user/userimpl/user.go @@ -149,11 +149,11 @@ func (s *Service) Create(ctx context.Context, cmd *user.CreateUserCommand) (*use Updated: time.Now(), } - if setting.AutoAssignOrg && !usr.IsAdmin { + if s.cfg.AutoAssignOrg && !usr.IsAdmin { if len(cmd.DefaultOrgRole) > 0 { orgUser.Role = org.RoleType(cmd.DefaultOrgRole) } else { - orgUser.Role = org.RoleType(setting.AutoAssignOrgRole) + orgUser.Role = org.RoleType(s.cfg.AutoAssignOrgRole) } } _, err = s.orgService.InsertOrgUser(ctx, &orgUser)