diff --git a/pkg/services/org/orgimpl/store.go b/pkg/services/org/orgimpl/store.go index 2d987b1539e..2935020cf25 100644 --- a/pkg/services/org/orgimpl/store.go +++ b/pkg/services/org/orgimpl/store.go @@ -681,6 +681,15 @@ func (ss *sqlStore) RemoveOrgUser(ctx context.Context, cmd *org.RemoveOrgUserCom return user.ErrUserNotFound } + // check if user belongs to org + var orgUser org.OrgUser + if exists, err := sess.Where("org_id=? AND user_id=?", cmd.OrgID, cmd.UserID).Get(&orgUser); err != nil { + return err + } else if !exists { + ss.log.Debug("User not in org, nothing to do", "user_id", cmd.UserID, "org_id", cmd.OrgID) + return nil + } + deletes := []string{ "DELETE FROM org_user WHERE org_id=? and user_id=?", "DELETE FROM dashboard_acl WHERE org_id=? and user_id = ?", @@ -727,7 +736,7 @@ func (ss *sqlStore) RemoveOrgUser(ctx context.Context, cmd *org.RemoveOrgUserCom return err } } - } else if cmd.ShouldDeleteOrphanedUser { + } else if cmd.ShouldDeleteOrphanedUser && !usr.IsAdmin { // no other orgs, delete the full user if err := ss.deleteUserInTransaction(sess, &user.DeleteUserCommand{UserID: usr.ID}); err != nil { return err diff --git a/pkg/services/org/orgimpl/store_test.go b/pkg/services/org/orgimpl/store_test.go index a4f839d6842..cafa209058a 100644 --- a/pkg/services/org/orgimpl/store_test.go +++ b/pkg/services/org/orgimpl/store_test.go @@ -12,6 +12,7 @@ import ( "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/infra/db" + "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/org" @@ -38,6 +39,7 @@ func TestIntegrationOrgDataAccess(t *testing.T) { orgStore := sqlStore{ db: ss, dialect: ss.GetDialect(), + log: log.NewNopLogger(), } t.Run("org not found", func(t *testing.T) { @@ -258,6 +260,7 @@ func TestIntegrationOrgUserDataAccess(t *testing.T) { orgUserStore := sqlStore{ db: ss, dialect: ss.GetDialect(), + log: log.NewNopLogger(), } t.Run("org user inserted", func(t *testing.T) { @@ -333,7 +336,7 @@ func TestIntegrationOrgUserDataAccess(t *testing.T) { ss, cfg := db.InitTestDBWithCfg(t) _, usrSvc := createOrgAndUserSvc(t, ss, cfg) 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} + ac2cmd := &user.CreateUserCommand{Login: "ac2", Email: "ac2@test.com", Name: "ac2 name"} ac1, err := usrSvc.Create(context.Background(), ac1cmd) require.NoError(t, err) ac2, err := usrSvc.Create(context.Background(), ac2cmd) @@ -460,6 +463,15 @@ func TestIntegrationOrgUserDataAccess(t *testing.T) { err := orgUserStore.Delete(context.Background(), &org.DeleteOrgCommand{ID: ac2.OrgID}) require.NoError(t, err) + // make sure ac2 is in ac1 org + cmd := org.AddOrgUserCommand{ + OrgID: ac1.OrgID, + UserID: ac2.ID, + Role: org.RoleViewer, + } + err = orgUserStore.AddOrgUser(context.Background(), &cmd) + 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) @@ -545,6 +557,7 @@ func TestIntegrationSQLStore_AddOrgUser(t *testing.T) { orgUserStore := sqlStore{ db: store, dialect: store.GetDialect(), + log: log.NewNopLogger(), } orgSvc, usrSvc := createOrgAndUserSvc(t, store, cfg) @@ -610,6 +623,7 @@ func TestIntegration_SQLStore_GetOrgUsers(t *testing.T) { orgUserStore := sqlStore{ db: store, dialect: store.GetDialect(), + log: log.NewNopLogger(), } cfg.IsEnterprise = true defer func() { @@ -728,6 +742,7 @@ func TestIntegration_SQLStore_GetOrgUsers_PopulatesCorrectly(t *testing.T) { orgUserStore := sqlStore{ db: store, dialect: store.GetDialect(), + log: log.NewNopLogger(), } _, usrSvc := createOrgAndUserSvc(t, store, cfg) @@ -789,6 +804,7 @@ func TestIntegration_SQLStore_SearchOrgUsers(t *testing.T) { orgUserStore := sqlStore{ db: store, dialect: store.GetDialect(), + log: log.NewNopLogger(), } // orgUserStore.cfg.Skip orgSvc, userSvc := createOrgAndUserSvc(t, store, cfg) @@ -865,12 +881,18 @@ func TestIntegration_SQLStore_RemoveOrgUser(t *testing.T) { orgUserStore := sqlStore{ db: store, dialect: store.GetDialect(), + log: log.NewNopLogger(), } + orgSvc, usrSvc := createOrgAndUserSvc(t, store, cfg) o, err := orgSvc.CreateWithMember(context.Background(), &org.CreateOrgCommand{Name: MainOrgName}) require.NoError(t, err) + // create 2nd org + o2, err := orgSvc.CreateWithMember(context.Background(), &org.CreateOrgCommand{Name: "test org 2"}) + require.NoError(t, err) + // create org and admin _, err = usrSvc.Create(context.Background(), &user.CreateUserCommand{ Login: "admin", @@ -879,28 +901,116 @@ func TestIntegration_SQLStore_RemoveOrgUser(t *testing.T) { require.NoError(t, err) // create a user with no org - _, err = usrSvc.Create(context.Background(), &user.CreateUserCommand{ - Login: "user", - OrgID: 1, + viewer, err := usrSvc.Create(context.Background(), &user.CreateUserCommand{ + Login: "viewer", SkipOrgSetup: true, }) require.NoError(t, err) + // create a user with no org + viewer2, err := usrSvc.Create(context.Background(), &user.CreateUserCommand{ + Login: "viewer2", + SkipOrgSetup: true, + }) + require.NoError(t, err) + + // create a user with no org + viewer3, err := usrSvc.Create(context.Background(), &user.CreateUserCommand{ + Login: "viewer3", + SkipOrgSetup: true, + }) + require.NoError(t, err) + + // create an admin user with no org + admin, err := usrSvc.Create(context.Background(), &user.CreateUserCommand{ + Login: "serverAdmin", + SkipOrgSetup: true, + IsAdmin: true, + }) + require.NoError(t, err) + // assign the user to the org err = orgUserStore.AddOrgUser(context.Background(), &org.AddOrgUserCommand{ Role: "Viewer", - OrgID: 1, - UserID: 2, + OrgID: o.ID, + UserID: viewer.ID, + }) + require.NoError(t, err) + + // assign the admin user to the org + err = orgUserStore.AddOrgUser(context.Background(), &org.AddOrgUserCommand{ + Role: "Admin", + OrgID: o.ID, + UserID: admin.ID, + }) + require.NoError(t, err) + + // assign the viewer3 user to the 2nd org + err = orgUserStore.AddOrgUser(context.Background(), &org.AddOrgUserCommand{ + Role: "Viewer", + OrgID: o2.ID, + UserID: viewer3.ID, }) require.NoError(t, err) // remove the user org err = orgUserStore.RemoveOrgUser(context.Background(), &org.RemoveOrgUserCommand{ - UserID: 2, - OrgID: 1, - ShouldDeleteOrphanedUser: false, + UserID: viewer.ID, + OrgID: o.ID, + ShouldDeleteOrphanedUser: true, + }) + require.NoError(t, err) + + // remove the admin user + err = orgUserStore.RemoveOrgUser(context.Background(), &org.RemoveOrgUserCommand{ + UserID: admin.ID, + OrgID: o.ID, + ShouldDeleteOrphanedUser: true, + }) + require.NoError(t, err) + + // remove the viewer3 user from first org they don't belong to + err = orgUserStore.RemoveOrgUser(context.Background(), &org.RemoveOrgUserCommand{ + UserID: viewer3.ID, + OrgID: o.ID, + ShouldDeleteOrphanedUser: true, + }) + require.NoError(t, err) + + // remove the viewer2 user from first org they don't belong to + err = orgUserStore.RemoveOrgUser(context.Background(), &org.RemoveOrgUserCommand{ + UserID: viewer2.ID, + OrgID: o.ID, + ShouldDeleteOrphanedUser: true, + }) + require.NoError(t, err) + + // verify the user is deleted + _, err = usrSvc.GetByID(context.Background(), &user.GetUserByIDQuery{ + ID: viewer.ID, + }) + require.ErrorIs(t, err, user.ErrUserNotFound) + + // verify the admin user is not deleted + usr, err := usrSvc.GetByID(context.Background(), &user.GetUserByIDQuery{ + ID: admin.ID, + }) + require.NoError(t, err) + assert.NotNil(t, usr) + + // verify the viewer2 user is not deleted + _, err = usrSvc.GetByID(context.Background(), &user.GetUserByIDQuery{ + ID: viewer2.ID, + }) + require.NoError(t, err) + assert.NotNil(t, usr) + + // verify the viewer3 user is not deleted + _, err = usrSvc.GetByID(context.Background(), &user.GetUserByIDQuery{ + ID: viewer3.ID, }) require.NoError(t, err) + assert.NotNil(t, usr) } func createOrgAndUserSvc(t *testing.T, store db.DB, cfg *setting.Cfg) (org.Service, user.Service) {