From 5b5cb6622d88db4b21d9828a1b6dd05c9bd7e609 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Thu, 11 Oct 2018 07:48:35 +0200 Subject: [PATCH 1/4] Remove user form org now completely removes the user from the system if the user is orphaned --- pkg/api/org_users.go | 20 ++++++++------- pkg/models/org_user.go | 5 ++-- pkg/services/sqlstore/org_test.go | 14 +++++++++++ pkg/services/sqlstore/org_users.go | 33 ++++++++++++++++-------- pkg/services/sqlstore/user.go | 40 ++++++++++++++++-------------- 5 files changed, 73 insertions(+), 39 deletions(-) diff --git a/pkg/api/org_users.go b/pkg/api/org_users.go index 4e2ed36431e..83b6d56b2c6 100644 --- a/pkg/api/org_users.go +++ b/pkg/api/org_users.go @@ -102,21 +102,23 @@ func updateOrgUserHelper(cmd m.UpdateOrgUserCommand) Response { // DELETE /api/org/users/:userId func RemoveOrgUserForCurrentOrg(c *m.ReqContext) Response { - userID := c.ParamsInt64(":userId") - return removeOrgUserHelper(c.OrgId, userID) + return removeOrgUserHelper(&m.RemoveOrgUserCommand{ + UserId: c.ParamsInt64(":userId"), + OrgId: c.OrgId, + ShouldDeleteOrphanedUser: true, + }) } // DELETE /api/orgs/:orgId/users/:userId func RemoveOrgUser(c *m.ReqContext) Response { - userID := c.ParamsInt64(":userId") - orgID := c.ParamsInt64(":orgId") - return removeOrgUserHelper(orgID, userID) + return removeOrgUserHelper(&m.RemoveOrgUserCommand{ + UserId: c.ParamsInt64(":userId"), + OrgId: c.ParamsInt64(":orgId"), + }) } -func removeOrgUserHelper(orgID int64, userID int64) Response { - cmd := m.RemoveOrgUserCommand{OrgId: orgID, UserId: userID} - - if err := bus.Dispatch(&cmd); err != nil { +func removeOrgUserHelper(cmd *m.RemoveOrgUserCommand) Response { + if err := bus.Dispatch(cmd); err != nil { if err == m.ErrLastOrgAdmin { return Error(400, "Cannot remove last organization admin", nil) } diff --git a/pkg/models/org_user.go b/pkg/models/org_user.go index 9231d18cfd6..51ab2f62e68 100644 --- a/pkg/models/org_user.go +++ b/pkg/models/org_user.go @@ -72,8 +72,9 @@ type OrgUser struct { // COMMANDS type RemoveOrgUserCommand struct { - UserId int64 - OrgId int64 + UserId int64 + OrgId int64 + ShouldDeleteOrphanedUser bool } type AddOrgUserCommand struct { diff --git a/pkg/services/sqlstore/org_test.go b/pkg/services/sqlstore/org_test.go index af8500707d5..eda20fe1b91 100644 --- a/pkg/services/sqlstore/org_test.go +++ b/pkg/services/sqlstore/org_test.go @@ -182,6 +182,20 @@ func TestAccountDataAccess(t *testing.T) { }) }) + Convey("Removing user from org should delete user completely if in no other org", func() { + // make sure ac2 has no org + err := DeleteOrg(&m.DeleteOrgCommand{Id: ac2.OrgId}) + So(err, ShouldBeNil) + + // remove frome ac2 from ac1 org + remCmd := m.RemoveOrgUserCommand{OrgId: ac1.OrgId, UserId: ac2.Id, ShouldDeleteOrphanedUser: true} + err = RemoveOrgUser(&remCmd) + So(err, ShouldBeNil) + + err = GetSignedInUser(&m.GetSignedInUserQuery{UserId: ac2.Id}) + So(err, ShouldEqual, m.ErrUserNotFound) + }) + Convey("Cannot delete last admin org user", func() { cmd := m.RemoveOrgUserCommand{OrgId: ac1.OrgId, UserId: ac1.Id} err := RemoveOrgUser(&cmd) diff --git a/pkg/services/sqlstore/org_users.go b/pkg/services/sqlstore/org_users.go index 14981cfde64..a4d7cb52136 100644 --- a/pkg/services/sqlstore/org_users.go +++ b/pkg/services/sqlstore/org_users.go @@ -157,6 +157,12 @@ func RemoveOrgUser(cmd *m.RemoveOrgUserCommand) error { } } + // 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 []*m.UserOrgDTO sess.Table("org_user") sess.Join("INNER", "org", "org_user.org_id=org.id") @@ -168,22 +174,29 @@ func RemoveOrgUser(cmd *m.RemoveOrgUserCommand) error { return err } - hasCurrentOrgSet := false - for _, userOrg := range userOrgs { - if user.OrgId == userOrg.OrgId { - hasCurrentOrgSet = true - break + if len(userOrgs) > 0 { + hasCurrentOrgSet := false + for _, userOrg := range userOrgs { + if user.OrgId == userOrg.OrgId { + hasCurrentOrgSet = true + break + } } - } - if !hasCurrentOrgSet && len(userOrgs) > 0 { - err = setUsingOrgInTransaction(sess, user.Id, userOrgs[0].OrgId) - if err != nil { + if !hasCurrentOrgSet { + err = setUsingOrgInTransaction(sess, user.Id, userOrgs[0].OrgId) + if err != nil { + return err + } + } + } else if cmd.ShouldDeleteOrphanedUser { + // no other orgs, delete the full user + if err := deleteUserInTransaction(sess, &m.DeleteUserCommand{UserId: user.Id}); err != nil { return err } } - return validateOneAdminLeftInOrg(cmd.OrgId, sess) + return nil }) } diff --git a/pkg/services/sqlstore/user.go b/pkg/services/sqlstore/user.go index 848a11d81ab..72d5654a777 100644 --- a/pkg/services/sqlstore/user.go +++ b/pkg/services/sqlstore/user.go @@ -445,27 +445,31 @@ func SearchUsers(query *m.SearchUsersQuery) error { func DeleteUser(cmd *m.DeleteUserCommand) error { return inTransaction(func(sess *DBSession) error { - deletes := []string{ - "DELETE FROM star WHERE user_id = ?", - "DELETE FROM " + 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 = ?", - } - - for _, sql := range deletes { - _, err := sess.Exec(sql, cmd.UserId) - if err != nil { - return err - } - } - - return nil + return deleteUserInTransaction(sess, cmd) }) } +func deleteUserInTransaction(sess *DBSession, cmd *m.DeleteUserCommand) error { + deletes := []string{ + "DELETE FROM star WHERE user_id = ?", + "DELETE FROM " + 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 = ?", + } + + for _, sql := range deletes { + _, err := sess.Exec(sql, cmd.UserId) + if err != nil { + return err + } + } + + return nil +} + func UpdateUserPermissions(cmd *m.UpdateUserPermissionsCommand) error { return inTransaction(func(sess *DBSession) error { user := m.User{} From 9585dc782599192511de2f04828b163d52f1c8db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Thu, 11 Oct 2018 07:58:22 +0200 Subject: [PATCH 2/4] added the UserWasRemoved flag to make api aware of what happened to return correct message to UI --- pkg/api/org_users.go | 4 ++++ pkg/models/org_user.go | 1 + pkg/services/sqlstore/org_users.go | 2 ++ 3 files changed, 7 insertions(+) diff --git a/pkg/api/org_users.go b/pkg/api/org_users.go index 83b6d56b2c6..6b3159d799b 100644 --- a/pkg/api/org_users.go +++ b/pkg/api/org_users.go @@ -125,5 +125,9 @@ func removeOrgUserHelper(cmd *m.RemoveOrgUserCommand) Response { return Error(500, "Failed to remove user from organization", err) } + if cmd.UserWasRemoved { + return Success("User deleted") + } + return Success("User removed from organization") } diff --git a/pkg/models/org_user.go b/pkg/models/org_user.go index 51ab2f62e68..e7896b3cab8 100644 --- a/pkg/models/org_user.go +++ b/pkg/models/org_user.go @@ -75,6 +75,7 @@ type RemoveOrgUserCommand struct { UserId int64 OrgId int64 ShouldDeleteOrphanedUser bool + UserWasRemoved bool } type AddOrgUserCommand struct { diff --git a/pkg/services/sqlstore/org_users.go b/pkg/services/sqlstore/org_users.go index a4d7cb52136..925893325c1 100644 --- a/pkg/services/sqlstore/org_users.go +++ b/pkg/services/sqlstore/org_users.go @@ -194,6 +194,8 @@ func RemoveOrgUser(cmd *m.RemoveOrgUserCommand) error { if err := deleteUserInTransaction(sess, &m.DeleteUserCommand{UserId: user.Id}); err != nil { return err } + + cmd.UserWasRemoved = true } return nil From b671b9704f2531d3804247967d2650f92530e3f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Thu, 11 Oct 2018 12:20:53 -0700 Subject: [PATCH 3/4] changed property name to UserWasDeleted and added an assert for it --- pkg/api/org_users.go | 6 +++--- pkg/models/org_user.go | 2 +- pkg/services/sqlstore/org_test.go | 1 + pkg/services/sqlstore/org_users.go | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/pkg/api/org_users.go b/pkg/api/org_users.go index 6b3159d799b..d79707d3ae2 100644 --- a/pkg/api/org_users.go +++ b/pkg/api/org_users.go @@ -103,8 +103,8 @@ func updateOrgUserHelper(cmd m.UpdateOrgUserCommand) Response { // DELETE /api/org/users/:userId func RemoveOrgUserForCurrentOrg(c *m.ReqContext) Response { return removeOrgUserHelper(&m.RemoveOrgUserCommand{ - UserId: c.ParamsInt64(":userId"), - OrgId: c.OrgId, + UserId: c.ParamsInt64(":userId"), + OrgId: c.OrgId, ShouldDeleteOrphanedUser: true, }) } @@ -125,7 +125,7 @@ func removeOrgUserHelper(cmd *m.RemoveOrgUserCommand) Response { return Error(500, "Failed to remove user from organization", err) } - if cmd.UserWasRemoved { + if cmd.UserWasDeleted { return Success("User deleted") } diff --git a/pkg/models/org_user.go b/pkg/models/org_user.go index e7896b3cab8..b6ecd924e9a 100644 --- a/pkg/models/org_user.go +++ b/pkg/models/org_user.go @@ -75,7 +75,7 @@ type RemoveOrgUserCommand struct { UserId int64 OrgId int64 ShouldDeleteOrphanedUser bool - UserWasRemoved bool + UserWasDeleted bool } type AddOrgUserCommand struct { diff --git a/pkg/services/sqlstore/org_test.go b/pkg/services/sqlstore/org_test.go index eda20fe1b91..c02686c24ba 100644 --- a/pkg/services/sqlstore/org_test.go +++ b/pkg/services/sqlstore/org_test.go @@ -191,6 +191,7 @@ func TestAccountDataAccess(t *testing.T) { remCmd := m.RemoveOrgUserCommand{OrgId: ac1.OrgId, UserId: ac2.Id, ShouldDeleteOrphanedUser: true} err = RemoveOrgUser(&remCmd) So(err, ShouldBeNil) + So(remCmd.UserWasDeleted, ShouldBeTrue) err = GetSignedInUser(&m.GetSignedInUserQuery{UserId: ac2.Id}) So(err, ShouldEqual, m.ErrUserNotFound) diff --git a/pkg/services/sqlstore/org_users.go b/pkg/services/sqlstore/org_users.go index 925893325c1..abbc320020e 100644 --- a/pkg/services/sqlstore/org_users.go +++ b/pkg/services/sqlstore/org_users.go @@ -195,7 +195,7 @@ func RemoveOrgUser(cmd *m.RemoveOrgUserCommand) error { return err } - cmd.UserWasRemoved = true + cmd.UserWasDeleted = true } return nil From 19b69a82afcf9586a6da07f1635d355c78a3b2cc Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Thu, 11 Oct 2018 15:26:06 -0400 Subject: [PATCH 4/4] fmt --- pkg/api/org_users.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/api/org_users.go b/pkg/api/org_users.go index d79707d3ae2..c11d2e265b4 100644 --- a/pkg/api/org_users.go +++ b/pkg/api/org_users.go @@ -103,8 +103,8 @@ func updateOrgUserHelper(cmd m.UpdateOrgUserCommand) Response { // DELETE /api/org/users/:userId func RemoveOrgUserForCurrentOrg(c *m.ReqContext) Response { return removeOrgUserHelper(&m.RemoveOrgUserCommand{ - UserId: c.ParamsInt64(":userId"), - OrgId: c.OrgId, + UserId: c.ParamsInt64(":userId"), + OrgId: c.OrgId, ShouldDeleteOrphanedUser: true, }) }