refactor(group): migrate RemoveGroupUser handler to membership by whoAbhishekSah · Pull Request #1611 · raystack/frontier

whoAbhishekSah · 2026-05-14T08:15:20Z

Summary

Migrates the RemoveGroupUser RPC handler off groupService.RemoveUsers and onto a new membershipService.RemoveGroupMember, mirroring the add-side work in #1608. Proto RPC unchanged.

The new method enforces the min-owner constraint via the existing DeleteWithMinRoleGuard atomic pattern (same as SetGroupMemberRole). This replaces the handler-level pre-check that compared owner counts via userService.ListByGroup — that two-step had the same TOCTOU pattern fixed for org in #1590.

Behavior

User-facing behavior of RemoveGroupUser unchanged: same error code (InvalidArgument + ErrGroupMinOwnerCount) on last-owner attempts.
Audit emits a new GroupMemberRemovedEvent audit record + context auditor log.
RemoveGroupMember restricts principal types to app/user for now; the switch is extensible for future types (same convention as SetGroupMemberRole).

What's intentionally not deleted

groupService.RemoveUsers / removeUsers stay. They serve the user-cascade path (core/deleter + group.Delete), which must bypass the min-owner constraint when the user is being destroyed system-wide. Matches the org pattern where org.Service.RemoveUsers and membership.RemoveOrganizationMember coexist.

Test plan

go build ./...
go test ./core/membership/... ./internal/api/v1beta1connect/...
gofmt -l clean
New unit tests cover: not-found, invalid principal type, not-member, last-owner pre-check, TOCTOU guard race, happy-path member removal, happy-path owner removal with the atomic guard.

vercel · 2026-05-14T08:15:25Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
frontier	Ready	Preview, Comment	May 14, 2026 8:57am

coderabbitai · 2026-05-14T08:19:38Z

Warning

Rate limit exceeded

@whoAbhishekSah has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 18 minutes and 52 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ef50fde9-0e93-47d0-a77f-57b2b27ec638

📥 Commits

Reviewing files that changed from the base of the PR and between d4efb41 and 56c4f79.

📒 Files selected for processing (7)

core/membership/service.go
core/membership/service_test.go
internal/api/v1beta1connect/group.go
internal/api/v1beta1connect/group_test.go
internal/api/v1beta1connect/interfaces.go
internal/api/v1beta1connect/mocks/membership_service.go
pkg/auditrecord/consts.go

📝 Walkthrough

Walkthrough

This PR adds group member removal functionality by implementing RemoveGroupMember in the membership service with validation, policy/relation cleanup, and audit logging, then integrates it into the HTTP handler for RemoveGroupUser, replacing prior sole-owner checks with direct membership service error mapping.

Changes

Group Member Removal Feature

Layer / File(s)	Summary
Interface contract and audit event `internal/api/v1beta1connect/interfaces.go`, `pkg/auditrecord/consts.go`	`MembershipService` interface gains `RemoveGroupMember` method; `GroupMemberRemovedEvent` audit constant is introduced.
RemoveGroupMember service implementation `core/membership/service.go`	`RemoveGroupMember` validates group and principal, lists policies, enforces minimum-owner constraint with atomic guard, deletes policies and SpiceDB relations, and invokes audit helper to log the removal event with metadata.
Service unit tests `core/membership/service_test.go`	Test suite `TestService_RemoveGroupMember` covers error scenarios (invalid principal type, group not found, not a member, last-owner protection via TOCTOU guard) and success paths (member and owner removal with relation cleanup).
Generated mock `internal/api/v1beta1connect/mocks/membership_service.go`	Auto-generated `RemoveGroupMember` mock with `EXPECT()` and `Call` wrapper supporting test expectations.
RemoveGroupUser handler `internal/api/v1beta1connect/group.go`	Handler refactored to call `membershipService.RemoveGroupMember` and map returned errors (not-found, not-member, last-owner, invalid-argument) to Connect response codes, removing prior role-lookup and group-service deletion.
Handler tests `internal/api/v1beta1connect/group_test.go`	`TestConnectHandler_RemoveGroupUser` simplified to wire only `OrganizationService` and `MembershipService`, with test cases driven by `RemoveGroupMember` error returns instead of admin-role enumeration logic.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

raystack/frontier#1537: Both PRs extend the membership service layer; #1537 adds AddOrganizationMember while this PR adds the complementary RemoveGroupMember method.
raystack/frontier#1596: This PR's RemoveGroupMember logic enforces the group membership/ownership model and min-owner constraint established in #1596.
raystack/frontier#1567: Both PRs refactor the RemoveGroupUser handler flow; #1567 introduced membership-based checks while this PR replaces that with the new membership service method.

Suggested reviewers

rohilsurana
AmanGIT07

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coveralls · 2026-05-14T08:21:49Z

Coverage Report for CI Build 25851271234

Coverage increased (+0.09%) to 42.373%

Details

Coverage increased (+0.09%) from the base build.
Patch coverage: 11 uncovered changes across 1 file (77 of 88 lines covered, 87.5%).
4 coverage regressions across 1 file.

Uncovered Changes

File	Changed	Covered	%
core/membership/service.go	70	59	84.29%

Coverage Regressions

4 previously-covered lines in 1 file lost coverage.

File	Lines Losing Coverage	Coverage
internal/api/v1beta1connect/group.go	4	76.56%

Coverage Stats


Relevant Lines:	37651
Covered Lines:	15954
Line Coverage:	42.37%
Coverage Strength:	11.89 hits per line

💛 - Coveralls

coderabbitai

🧹 Nitpick comments (1)

internal/api/v1beta1connect/group_test.go (1)

1387-1501: ⚡ Quick win

Add coverage for remaining new RemoveGroupUser error mappings.

Please add table cases for membership.ErrInvalidPrincipalType / membership.ErrInvalidPrincipal (expect CodeInvalidArgument) and user.ErrDisabled (expect CodeFailedPrecondition) to lock the handler contract introduced in this refactor.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 74ca9954-69e3-4ac7-ab4b-730a557c6c25

📥 Commits

Reviewing files that changed from the base of the PR and between 294a560 and d4efb41.

📒 Files selected for processing (7)

core/membership/service.go
core/membership/service_test.go
internal/api/v1beta1connect/group.go
internal/api/v1beta1connect/group_test.go
internal/api/v1beta1connect/interfaces.go
internal/api/v1beta1connect/mocks/membership_service.go
pkg/auditrecord/consts.go

whoAbhishekSah · 2026-05-14T08:26:30Z

Manual Testing Results ✅

Tested RemoveGroupUser migration to membershipService.RemoveGroupMember with atomic last-owner guard.

RemoveGroupUser API Tests

#	Test Case	Expected	Result
1	Remove regular member	SUCCESS	✅
2	Verify member removed	Not in list	✅
3	Remove owner when another exists	SUCCESS	✅
4	Verify owner removed, other remains	Correct	✅
5	Remove last owner	`invalid_argument: group must have at least one owner...`	✅
6	Remove non-member	`failed_precondition: principal is not a member of the resource`	✅
7	Remove non-existent user	`not_found: user doesn't exist`	✅
8	Remove from non-existent group	`permission_denied`	✅
9	Remove already-removed user	`failed_precondition: principal is not a member of the resource`	✅

Ownership Transition Tests

#	Test Case	Expected	Result
10	Promote member to owner	SUCCESS	✅
11	Remove first owner (second exists)	SUCCESS	✅
12	Verify sole owner remains	Correct	✅
13	Remove sole owner	`invalid_argument: group must have at least one owner...`	✅
14	Add user back as owner	SUCCESS	✅
15	Demote owner to member	SUCCESS	✅
16	Remove demoted member	SUCCESS	✅

Unit Tests (all pass)

Test Case	Result
Group does not exist	✅
Unsupported principal type	✅
Not a member (ErrNotMember)	✅
Last owner pre-check	✅
TOCTOU race (DeleteWithMinRoleGuard)	✅
Remove member (non-owner)	✅
Remove owner via atomic guard	✅

The RemoveGroupUser RPC handler now delegates to a new membershipService.RemoveGroupMember, completing the symmetry with the AddGroupUsers migration (#1608). The proto RPC is unchanged. - Adds RemoveGroupMember to core/membership/service.go. Validates principal, enforces the min-owner constraint via the existing atomic DeleteWithMinRoleGuard, cleans up both group#owner and group#member relations, and emits an audit event. - Moves the min-owner pre-check out of the handler. Previously the handler called userService.ListByGroup(group.AdminRole) and compared counts; that race-prone two-step is replaced by validateMinGroupOwner + the atomic guard pattern already used by SetGroupMemberRole. - Adds the GroupMemberRemovedEvent audit constant. groupService.RemoveUsers / removeUsers are intentionally kept — they serve the user-cascade path (core/deleter and group.Delete), which must bypass the min-owner constraint when the user is being destroyed system-wide. Matches the org pattern where Service.RemoveUsers and RemoveOrganizationMember coexist. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Adds table cases for user.ErrDisabled, membership.ErrInvalidPrincipalType, and membership.ErrInvalidPrincipal so the handler's error mapping contract is fully locked. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

vercel Bot deployed to Preview May 14, 2026 08:15 View deployment

coderabbitai Bot reviewed May 14, 2026

View reviewed changes

vercel Bot deployed to Preview May 14, 2026 08:29 View deployment

whoAbhishekSah and others added 2 commits May 14, 2026 14:26

whoAbhishekSah force-pushed the refactor/removegroupuser-via-membership branch from d8819d4 to 56c4f79 Compare May 14, 2026 08:56

vercel Bot deployed to Preview May 14, 2026 08:57 View deployment

whoAbhishekSah requested a review from rohilsurana May 14, 2026 08:58

rohilsurana approved these changes May 14, 2026

View reviewed changes

whoAbhishekSah merged commit d631ed6 into main May 14, 2026
8 checks passed

whoAbhishekSah deleted the refactor/removegroupuser-via-membership branch May 14, 2026 09:37

whoAbhishekSah mentioned this pull request May 14, 2026

refactor(sdk): drop redundant policy CRUD from remove-member-dialog #1612

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(group): migrate RemoveGroupUser handler to membership#1611

refactor(group): migrate RemoveGroupUser handler to membership#1611
whoAbhishekSah merged 2 commits into
mainfrom
refactor/removegroupuser-via-membership

whoAbhishekSah commented May 14, 2026

Uh oh!

vercel Bot commented May 14, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented May 14, 2026 •

edited

Loading

Rate limit exceeded

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Uh oh!

coveralls commented May 14, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

whoAbhishekSah commented May 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

whoAbhishekSah commented May 14, 2026

Summary

Behavior

What's intentionally not deleted

Test plan

Uh oh!

vercel Bot commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Uh oh!

coveralls commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 25851271234

Coverage increased (+0.09%) to 42.373%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

whoAbhishekSah commented May 14, 2026

Manual Testing Results ✅

RemoveGroupUser API Tests

Ownership Transition Tests

Unit Tests (all pass)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vercel Bot commented May 14, 2026 •

edited

Loading

coderabbitai Bot commented May 14, 2026 •

edited

Loading

coveralls commented May 14, 2026 •

edited

Loading