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

Remove SiteUsers #4683

Merged
merged 1 commit into from
Dec 6, 2024
Merged

Remove SiteUsers #4683

merged 1 commit into from
Dec 6, 2024

Conversation

drewbo
Copy link
Contributor

@drewbo drewbo commented Nov 25, 2024

Changes proposed in this pull request:

  • closes Remove SiteUsers concept #4339
  • Rewrites Build.forSiteUser and Site.forUser to only reference organization membership as a condition for viewing Builds/Sites.
  • Removes the ability to add/remove users from a Site throughout the application (api/controller/actions/reducers)
  • Adds a new convenience instance method to the Organization model, addRoleUser for more easily adding users to an organization
  • Adds a new convenience instance method get(Site)OrgUsers to Build and Site for getting users which match the site's organization.
  • Adds a new convenience test support function createSiteUserOrg which creates, as needed:
    • A user
    • Who is a member of an org
    • To which a site belongs
  • A significant test suite rewrite to update our now singular access pattern (i.e. users must have an organization membership matching the site to access site resources instead of using Site.users)
    • Also removes "Basic Auth" tests and routes
    • Removes tests which reference organization membership as an alternative access pattern
    • Rewrites many tests (though not all, I got tired) to prefer async over Promise-based testing.

Todo:

  • Both loadBuildUserAccessToken and the createBuildForWebhookRequest need a significant rewrite to query organization users rather than site users.
  • Analyze the potential impact of the above change. While we haven't added any new users this way for ~18 months, I'm still concerned that a number of sites build using much older user tokens
  • Test the new model methods
  • Remove all associated SiteUser concepts (table, Site.users column) from the database(s)

security considerations

This is likely a significant security improvement (removes one access method) but it's also a major change that we should review thoroughly

@drewbo drewbo force-pushed the chore-remove-site-users branch from edb2ed2 to 05451c5 Compare November 25, 2024 19:24
@drewbo
Copy link
Contributor Author

drewbo commented Dec 3, 2024

I could be missing something but it seems like this change (if implemented correctly) won't impact any builds. This returned zero results:

notoken = sites.filter(site => site.Organization.OrganizationRoles.every(role => !role.User.githubAccessToken))

@drewbo
Copy link
Contributor Author

drewbo commented Dec 4, 2024

I should note that I didn't verify the permissions of each token (by calling GitHub.checkPermissions) so it's possible some of the tokens aren't valid for the given repo and we could still run into some, albeit more minor, issues.

@cloud-gov-pages-operations
Copy link
Contributor

cloud-gov-pages-operations commented Dec 4, 2024

🤖 This is an automated code coverage report

Total coverage (lines): 16.08%
Coverage diff: 0.01% 📈

@drewbo drewbo force-pushed the chore-remove-site-users branch from cc9b136 to f527257 Compare December 5, 2024 17:06
@drewbo drewbo changed the title [WIP] Remove SiteUsers Remove SiteUsers Dec 5, 2024
@drewbo drewbo force-pushed the chore-remove-site-users branch from baf98bd to 5492983 Compare December 5, 2024 17:40
@drewbo drewbo requested a review from a team December 5, 2024 17:41
@drewbo drewbo force-pushed the chore-remove-site-users branch from e6a4e9e to 60598ad Compare December 6, 2024 14:21
@sknep
Copy link
Contributor

sknep commented Dec 6, 2024

We should also update related documentation to make sure people understand that they can no longer add or remove people from individual sites.

@drewbo
Copy link
Contributor Author

drewbo commented Dec 6, 2024

@sknep do you know if our docs say that anywhere? We should update the bottom of https://cloud.gov/pages/documentation/access-permissions/ for inaccuracies but the top describes (correctly) organization access instead of the old method

@sknep
Copy link
Contributor

sknep commented Dec 6, 2024

afaik, just that section. @Ephraim-G are you any more familiar with our docs? Or will you need to update any ZD macros now that everyone must be added to that site's assigned org to get access?

@drewbo drewbo force-pushed the chore-remove-site-users branch from 60598ad to b8fb0d9 Compare December 6, 2024 17:52
Copy link
Contributor

@apburnes apburnes left a comment

Choose a reason for hiding this comment

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

This well tested and is a much needed cleanup of our user, org, and site model. We've worked through this update a lot and now it looks good to go.

@drewbo drewbo merged commit e3d8f75 into main Dec 6, 2024
8 checks passed
@drewbo drewbo deleted the chore-remove-site-users branch December 6, 2024 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove SiteUsers concept
4 participants