feat: implement `deleteUsers` mutation #10498

tianrunhe · 2024-11-19T06:01:36Z

Description

Part 1 of #10186

Testing scenarios

mutation {
  deleteUsers(emails: [
    "[email protected]",
    "[email protected]"
  ]) {
    ...on ErrorPayload {
      error {
        message
      }
    }
    ...on DeleteUsersSuccess {
      success
    }
  }
}

Final checklist

I checked the code review guidelines
I have added Metrics Representative as reviewer(s) if my PR invovles metrics/data/analytics related changes
I have performed a self-review of my code, the same way I'd do it for any other team member
I have tested all cases I listed in the testing scenarios and I haven't found any issues or regressions
Whenever I took a non-obvious choice I added a comment explaining why I did it this way
I added the label Skip Maintainer Review Indicating the PR only requires reviewer review and can be merged right after it's approved if the PR introduces only minor changes, does not contain any architectural changes or does not introduce any new patterns and I think one review is sufficient'
PR title is human readable and could be used in changelog

Dschoordsch

I think this would work, but I'm a bit skeptical if it's the correct approach. If I'm a contractor at Contractors United and join Megacorp for a job, I don't want them to be able to delete my whole account just because they have admin rights there.

packages/server/postgres/constants.ts

packages/server/graphql/public/mutations/deleteUsers.ts

packages/server/graphql/public/typeDefs/DeleteUsersSuccess.graphql

Dschoordsch · 2024-11-20T09:42:49Z

I think there are 2 possible scenarios we could implement:

the org owns the user, i.e. it owns the email domain the user is registered with, then they can delete it
the org does not own the user, then we should only remove it from the org

tianrunhe · 2024-12-10T20:39:19Z

Hi @Dschoordsch , I'm coming back to this PR after vacation. Sorry for dragging it this long!

If I'm a contractor at Contractors United and join Megacorp for a job, I don't want them to be able to delete my whole account just because they have admin rights there.

I would argue that if the contractor at Contractors United is a billing leader or an org admin, then they should have the rights to delete users within the org. How would he/she has become a billing leader or an org admin in the first place? Well, for billing leader someone has to promote them, and for org admin, we need to run the mutation for them.

the org owns the user, i.e. it owns the email domain the user is registered with, then they can delete it

Can you elaborate what you are thinking on "owning the email domain"? Are you talking about verified org domains? How would we test that in the code?

Thanks for your time!

Dschoordsch · 2024-12-11T08:33:25Z

I, Bill, contractor at Contractors United, am in 2 organizations: Contractors United and Magacorp for a gig. I don't want Evil Bob, COO of Megacorp, be able to delete my account, which I also use for my Contractors United internal holiday planning.

packages/server/graphql/public/mutations/deleteUsers.ts

Dschoordsch · 2024-12-17T09:56:06Z

packages/server/graphql/public/mutations/deleteUsers.ts

+      const hasTeamPermission = teamMembers.some((userTeamMember) =>
+        viewerTeamLeadPermissions.has(userTeamMember.teamId)
+      )


-1 If I'm a grumpy employee I can create a team "Status Reports" and get the CEO to join, then I can delete his user?

Good point! I removed the team level permission check and only rely on org level permissions

Dschoordsch · 2024-12-17T10:05:16Z

packages/server/graphql/public/mutations/deleteUsers.ts

+  // Pre-compute org-level permissions
+  const viewerOrgPermissions = await Promise.all(
+    viewerOrgUsers.map(async (orgUser) => ({
+      orgId: orgUser.orgId,
+      isOrgAdmin: await isUserOrgAdmin(viewerId, orgUser.orgId, dataLoader),
+      isBillingLeader: await isUserBillingLeader(viewerId, orgUser.orgId, dataLoader)
+    }))
+  )


+1 I find it not very easy to read the whole delete operation. I think the mutation would benefit from not trying to do these kind of optimizations, but instead do a simple

const deletedUsers = await Promise.all(usersToDelete.map(... // load objects and check permission // delete ).filter(isNotNull)

The dataloader can still partially still do it's thing and it's much easier to ensure we don't have any index errors.

If you want to go even further, we put the permission in a separate function

canDeleteUser(actor: User, user: User)

I would like to implement the "all-or-nothing" strategy: if the caller does not have permissions to delete all of the users he/she requested, we won't delete any one and as a friendly robot, we return with error message to indicate which set of the users he/she requested cannot be deleted.
With that in mind, it's not possible to merge in the deletion operation into the permission check. WDYT?

I see. I went back to the issue to see how this would look in practice. I'm not sure this would be the best user experience. What I expect a user to do in this case is searching the selected users to unselect the ones they can't delete and try again.
I'm also not sure deleting users is what is actually required for this issue.

Dschoordsch

I think a mutation like this deleteUser one makes sense for enterprise contexts where the company basically owns the user accounts. In all other cases removing the org user should be sufficient, shouldn't it?

Dschoordsch · 2024-12-18T13:01:10Z

packages/server/graphql/public/mutations/deleteUsers.ts

+          const organization = await dataLoader.get('organizations').loadNonNull(orgId)
+          return organization.activeDomain === getDomainFromEmail(userToDelete.email)


-1 This is not enough for owning a domain. We can only allow to delete users fully for organizations with verified domains. This usually means just enterprise orgs with SAML, but we can manually verify domains for them without them using SAML.

Dschoordsch · 2024-12-18T13:06:02Z

packages/server/graphql/public/mutations/deleteUsers.ts

+  // Pre-compute org-level permissions
+  const viewerOrgPermissions = await Promise.all(
+    viewerOrgUsers.map(async (orgUser) => ({
+      orgId: orgUser.orgId,
+      isOrgAdmin: await isUserOrgAdmin(viewerId, orgUser.orgId, dataLoader),
+      isBillingLeader: await isUserBillingLeader(viewerId, orgUser.orgId, dataLoader)
+    }))
+  )


I see. I went back to the issue to see how this would look in practice. I'm not sure this would be the best user experience. What I expect a user to do in this case is searching the selected users to unselect the ones they can't delete and try again.
I'm also not sure deleting users is what is actually required for this issue.

tianrunhe · 2024-12-18T21:01:25Z

With Charles' comment here, I think the Safe-To-Try™️ version of next step is to be able to remove multiple users from the org, instead of deleting the accounts altogether. I'm therefore closing this PR.

Thanks a ton @Dschoordsch and apologize for taking too much of your time!

tianrunhe added 2 commits November 18, 2024 20:58

feat: implement mass user deletion

f710b79

Add check for number of users to delete at once

6682778

tianrunhe linked an issue Nov 19, 2024 that may be closed by this pull request

User can mass select and delete members from the members page #10186

Open

1 task

github-actions bot added the size/m label Nov 19, 2024

tianrunhe changed the title ~~feat/massUserDeletion~~ feat: implement deleteUsers mutations Nov 19, 2024

tianrunhe changed the title ~~feat: implement deleteUsers mutations~~ feat: implement deleteUsers mutation Nov 19, 2024

tianrunhe requested a review from Dschoordsch November 19, 2024 06:14

Dschoordsch requested changes Nov 20, 2024

View reviewed changes

tianrunhe added 6 commits December 17, 2024 10:43

Merge branch 'master' into feat/massUserDeletion

81cfc07

Minor fixes

9bf8587

Add domain ownership check

0374609

Better error message

e039f51

Fix unit test

6955d22

Modify success return type and fix unit tests

baf5ba1

tianrunhe requested a review from Dschoordsch December 17, 2024 00:16

Dschoordsch requested changes Dec 17, 2024

View reviewed changes

Remove team level permission

5f3771c

tianrunhe requested a review from Dschoordsch December 17, 2024 22:24

Dschoordsch requested changes Dec 18, 2024

View reviewed changes

tianrunhe closed this Dec 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement `deleteUsers` mutation #10498

feat: implement `deleteUsers` mutation #10498

tianrunhe commented Nov 19, 2024 •

edited

Loading

Dschoordsch left a comment

Dschoordsch commented Nov 20, 2024

tianrunhe commented Dec 10, 2024 •

edited

Loading

Dschoordsch commented Dec 11, 2024 •

edited

Loading

Dschoordsch Dec 17, 2024

tianrunhe Dec 17, 2024

Dschoordsch Dec 17, 2024

tianrunhe Dec 17, 2024

Dschoordsch Dec 18, 2024

Dschoordsch left a comment

Dschoordsch Dec 18, 2024

Dschoordsch Dec 18, 2024

tianrunhe commented Dec 18, 2024

		const organization = await dataLoader.get('organizations').loadNonNull(orgId)
		return organization.activeDomain === getDomainFromEmail(userToDelete.email)

feat: implement deleteUsers mutation #10498

feat: implement deleteUsers mutation #10498

Conversation

tianrunhe commented Nov 19, 2024 • edited Loading

Description

Testing scenarios

Final checklist

Dschoordsch left a comment

Choose a reason for hiding this comment

Dschoordsch commented Nov 20, 2024

tianrunhe commented Dec 10, 2024 • edited Loading

Dschoordsch commented Dec 11, 2024 • edited Loading

Dschoordsch Dec 17, 2024

Choose a reason for hiding this comment

tianrunhe Dec 17, 2024

Choose a reason for hiding this comment

Dschoordsch Dec 17, 2024

Choose a reason for hiding this comment

tianrunhe Dec 17, 2024

Choose a reason for hiding this comment

Dschoordsch Dec 18, 2024

Choose a reason for hiding this comment

Dschoordsch left a comment

Choose a reason for hiding this comment

Dschoordsch Dec 18, 2024

Choose a reason for hiding this comment

Dschoordsch Dec 18, 2024

Choose a reason for hiding this comment

tianrunhe commented Dec 18, 2024

feat: implement `deleteUsers` mutation #10498

feat: implement `deleteUsers` mutation #10498

tianrunhe commented Nov 19, 2024 •

edited

Loading

tianrunhe commented Dec 10, 2024 •

edited

Loading

Dschoordsch commented Dec 11, 2024 •

edited

Loading