Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
Coverage Report for CI Build 43047826767Coverage remained the same at 56.097%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
rohilsurana
left a comment
There was a problem hiding this comment.
The fix looks right. Both list calls now include disabled projects and groups, and the postgres repos honor the flag. The project list also feeds the custom resource cleanup, so orders in a disabled project get cleaned up too.
I ran the new e2e test locally. It passes with the fix. With the old org.go all three sub tests fail. Unit tests and lint pass.
Two things for later, not for this PR:
- Removing someone straight from a disabled project or group still fails with "not found".
RemoveProjectMember,SetGroupMemberRoleandRemoveGroupMembercallGet, andGetByIDskips disabled rows. Worth a follow up issue. - Members removed before this fix may still have live policies and group relations on disabled projects and groups. Do we want a one time cleanup, or is it fine to leave them?
| }) | ||
| } | ||
|
|
||
| func (s *APIRegressionTestSuite) TestOrgMemberRemovalLeavesNoAccessOnDisabledProjectsAndGroups() { |
There was a problem hiding this comment.
Nit: this covers the API removal for a user. The PR says user delete gets the same fix through RemoveOrganizationMemberOnDelete. A small case for that path, or for a PAT, would back that claim. Fine as a follow up.
There was a problem hiding this comment.
User delete does not change in this PR. I reworded the description so it no longer reads that way. User delete already ends by removing all of the user's policies and every relation they are in (core/user/service.go:167), and TestUserDeleteLeavesNoAccessAndNoLivePolicy already covers a disabled project and group.
A PAT scoped to a disabled project is covered by #2048. Its revoke removes every policy the PAT holds: listing policies by principal does not look at the project's state (internal/store/postgres/policy_repository.go:104), and neither does deleting them (core/policy/service.go:81).
cascadeRemovePrincipal now lists the org's projects and groups with IncludeDisabled, so a removed member's policies there, their custom-resource policies in disabled projects, and their group member relations are removed too.
4ed5181 to
cad5c3c
Compare
|
On the two things for later:
|
Summary
Removing a member from an organization now also removes their access on disabled projects and groups. Stacked on #2048.
Changes
cascadeRemovePrincipallists the organization's projects and groups withIncludeDisabled: true. The member's policies on disabled projects and groups, their policies on custom resources in disabled projects, and theirmemberrelation on disabled groups are removed with the rest.Technical Details
IncludeDisabled: true.TestUserDeleteLeavesNoAccessAndNoLivePolicyalready covers a disabled project and group.Test Plan
go test ./core/membership/passes. The removal and user delete tests expectIncludeDisabled: trueon both lists. Without the flag, 18 cases fail.TestOrgMemberRemovalLeavesNoAccessOnDisabledProjectsAndGroups: a member holds a role on a project, a group, and an order in that project, and the group holds viewer on a second project. The project and group are disabled, the member is removed, and both are enabled again. No live policy is left, the member has nogeton any of them, and the group no longer gives access to the second project. Without the fix, all three checks fail.go test ./test/e2e/...passes.make testpasses.golangci-lint runreports no issues.