You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(membership): allow removing a member from a disabled project or group - #2052
Removing a member from a disabled project or a disabled group now works. Before, it failed with not found, so an admin had to enable the project or group, remove the member, and disable it again. Closes#2051.
Changes
RemoveProjectMember finds the project even when it is disabled. It lists the project by id with IncludeDisabled.
RemoveGroupMember finds the group the same way, through the existing getGroupIncludingDisabled that OnGroupDeleted uses.
The RemoveGroupUser handler no longer loads the group before calling membership. That lookup skipped disabled groups. Membership loads the group and returns the same errors, so the response codes do not change.
Technical Details
Deleted projects and groups are still not found.
Changing a member's role on a disabled project or group is still refused.
RemoveProjectMember takes only a project id. The RPC validates project_id as a UUID.
Test Plan
go test ./core/membership/ passes. New cases: removing a member from a disabled project and from a disabled group, a missing project, and a failed project or group lookup.
go test ./internal/api/v1beta1connect/ passes. RemoveGroupUser no longer loads the group itself.
e2e TestMemberRemovalFromDisabledProjectAndGroup: an organization owner (not a superuser) disables a project and a group, removes a member from each, and enables both again. The member has no live policy left, no get on either, and no access through the group to another project. Without the fix, every check fails.
…roup
RemoveProjectMember and RemoveGroupMember now find the project or group even when it is disabled, so an admin can take a member's access away without enabling it first. The RemoveGroupUser handler no longer loads the group itself, since that lookup skipped disabled groups.
No actionable comments were generated in the recent review.
Summary
Summary by CodeRabbit
Bug Fixes
Members can now be removed from disabled projects and groups. After those resources are re-enabled, the removed members no longer retain access through direct or group-based permissions.
Member removal now retrieves disabled projects and groups. The group API delegates lookup and error handling to the membership service. Unit and end-to-end tests cover removal from disabled resources and verify that access remains removed after re-enabling them.
Changes: Disabled project and group membership removal
Layer / File(s)
Summary
Disabled-resource lookup and membership removal core/membership/group.go, core/membership/group_test.go, core/membership/project.go, core/membership/project_test.go
Group removal retrieves the group with disabled resources included. Project removal lists the project with disabled resources included, propagates lookup errors, and returns project.ErrNotExist when no project is found. Unit tests cover these lookups and removal cases.
API delegation and end-to-end regression internal/api/v1beta1connect/group.go, internal/api/v1beta1connect/group_test.go, test/e2e/regression/api_test.go
RemoveGroupUser relies on membership removal to report group errors. The end-to-end test removes a member from a disabled project and group, then checks that access stays removed after re-enabling them.
Removing a member from a disabled project or group now works without re-enabling it, and the reported tests cover that case. No merge-blocking risk was found in the supplied changes.
Pre-merge checks: 2
Check name
Status
Explanation
Linked Issues check
Issue #2051 requires removal of a member from a disabled project or disabled group. The PR meets this requirement in the reviewed code. RemoveProjectMember now loads the project through getProjectIncl…
Out of Scope Changes check
All changed files relate to the objective of #2051. The changes are in core/membership/project.go and group.go, the RemoveGroupUser handler in internal/api/v1beta1connect/group.go, and the matching un…
Full details: Linked Issues check
Explanation
Issue #2051 requires removal of a member from a disabled project or disabled group. The PR meets this requirement in the reviewed code. RemoveProjectMember now loads the project through getProjectIncludingDisabled, which lists by project ID with IncludeDisabled: true. RemoveGroupMember now loads the group through the existing getGroupIncludingDisabled. The RemoveGroupUser handler no longer loads the group first, because that lookup skipped disabled groups. Unit tests in core/membership/project_test.go and group_test.go cover removal from a disabled project and a disabled group, a missing project, and lookup failures. The handler test in internal/api/v1beta1connect/group_test.go is updated for the removed Get call. An e2e test, TestMemberRemovalFromDisabledProjectAndGroup, checks that the member loses live policies and access after re-enabling. The issue asks to decide separately on adding members and changing roles. The PR keeps those refused, which the issue allows. The e2e run for ./test/e2e/... is unchecked in the PR description. The added e2e test is present in the diff, so this does not block the verdict. The result rests on the summary and the described code paths. I did not inspect the source directly.
Full details: Out of Scope Changes check
Explanation
All changed files relate to the objective of #2051. The changes are in core/membership/project.go and group.go, the RemoveGroupUser handler in internal/api/v1beta1connect/group.go, and the matching unit tests. The e2e regression test covers the same removal flow. The handler change removes a lookup that blocked disabled groups. Its response codes stay the same, because membership returns the same errors. The PR does not change role-change or add-member behavior, and it does not change deletion behavior. No unrelated change appears in the summary.
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Reviewing files that changed from the base of the PR and between 885fc0f and 4d3c7a8.
Files selected for processing: 7
core/membership/group.go
core/membership/group_test.go
core/membership/project.go
core/membership/project_test.go
internal/api/v1beta1connect/group.go
internal/api/v1beta1connect/group_test.go
test/e2e/regression/api_test.go
Files with no reviewable changes: 2
internal/api/v1beta1connect/group.go
internal/api/v1beta1connect/group_test.go
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removing a member from a disabled project or a disabled group now works. Before, it failed with not found, so an admin had to enable the project or group, remove the member, and disable it again. Closes #2051.
Changes
RemoveProjectMemberfinds the project even when it is disabled. It lists the project by id withIncludeDisabled.RemoveGroupMemberfinds the group the same way, through the existinggetGroupIncludingDisabledthatOnGroupDeleteduses.RemoveGroupUserhandler no longer loads the group before calling membership. That lookup skipped disabled groups. Membership loads the group and returns the same errors, so the response codes do not change.Technical Details
RemoveProjectMembertakes only a project id. The RPC validatesproject_idas a UUID.Test Plan
go test ./core/membership/passes. New cases: removing a member from a disabled project and from a disabled group, a missing project, and a failed project or group lookup.go test ./internal/api/v1beta1connect/passes.RemoveGroupUserno longer loads the group itself.TestMemberRemovalFromDisabledProjectAndGroup: an organization owner (not a superuser) disables a project and a group, removes a member from each, and enables both again. The member has no live policy left, nogeton either, and no access through the group to another project. Without the fix, every check fails.go test ./test/e2e/...passes.make testpasses.golangci-lint runreports no issues.