Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BFF delete organization #983

Merged
merged 6 commits into from
Jan 17, 2023
Merged

BFF delete organization #983

merged 6 commits into from
Jan 17, 2023

Conversation

pdeziel
Copy link
Collaborator

@pdeziel pdeziel commented Jan 11, 2023

Scope of changes

This adds a delete organization endpoint to the BFF API. The tricky part here is making sure users in that organization can still login to the frontend. For example, if a user's current organization is the one being deleted, we need to update their app metadata on Auth0 so that they default to a different organization (if available).

SC-11817

Type of change

  • bug fix
  • new feature
  • documentation
  • other (describe)

Acceptance criteria

This endpoint should allow users with the delete:organizations permission to delete organizations that they are a collaborator on.

Author checklist

  • I have manually tested the change and/or added automation in the form of unit tests or integration tests
  • I have updated the dependencies list
  • I have recompiled and included new protocol buffers to reflect changes I made
  • I have added new test fixtures as needed to support added tests
  • Check this box if a reviewer can merge this pull request after approval (leave it unchecked if you want to do it yourself)
  • I have moved the associated Shortcut story to "Ready for Review"

Reviewer(s) checklist

  • The organization switching logic makes sense
  • There is sufficient test coverage

}

// Delete the organization from the database
if err = s.db.DeleteOrganization(org.UUID()); err != nil {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we will need at least a confirmation prompt on the frontend, since deleting the organization record will completely delete the registration from the database.

// Remove the organization from all collaborators so that they don't get an error
// when they try to log in. If a user no longer has an organization, they will be
// assigned one automatically the next time they log in.
for _, collab := range org.GetCollaborators() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty tricky because we're relying on state in Auth0 to decide which organization to attempt to log the user into. If the organization is deleted out from under them, we need to have a way in which they can still login. For most users, what this means is that they will be assigned a new default organization on login. For TSP users who can be assigned to multiple organizations at once, we need to do a bit of extra work to figure out which organization to default them to.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If GDS went down in the middle of this operation, could we end up with some collaborators who still have metadata for deleted orgs? If so, are there any places where you think this might cause problems?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would mostly cause problems when users try to login; in that case we might have to manually change things in Auth0 to make sure everything is in sync.

Copy link
Collaborator

@rebeccabilbro rebeccabilbro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @pdeziel ! I had just a few questions (probably resulting from the fact that I am not very familiar with the collaborators feature).

return
}

// The user must be a collaborator in the organization to delete it
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is being a collaborator equivalent to having delete permissions on the user's claims from auth0 or does collaborator status include different types of permissions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user will need the delete:organizations permission to get past the middleware to this endpoint. This is mainly a sanity check to make sure the user is actually in the organization (e.g. to protect against the frontend passing in the wrong orgID).

// Remove the organization from all collaborators so that they don't get an error
// when they try to log in. If a user no longer has an organization, they will be
// assigned one automatically the next time they log in.
for _, collab := range org.GetCollaborators() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If GDS went down in the middle of this operation, could we end up with some collaborators who still have metadata for deleted orgs? If so, are there any places where you think this might cause problems?

@@ -652,6 +652,34 @@ paths:
summary: Create a new organization [create:organizations]
tags:
- organizations
/organizations/{id}:
delete:
description: Completely delete an organization, including the registration and
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a little note like "Warning! Deletes are not reversible! Be sure to prompt user for confirmation before executing."

@pdeziel pdeziel merged commit b706fd7 into main Jan 17, 2023
@pdeziel pdeziel deleted the sc-11817 branch January 17, 2023 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants