fix: Allow user leave their own groups#1410
Open
mdrkrg wants to merge 10 commits intosysadminsmedia:mainfrom
Open
fix: Allow user leave their own groups#1410mdrkrg wants to merge 10 commits intosysadminsmedia:mainfrom
mdrkrg wants to merge 10 commits intosysadminsmedia:mainfrom
Conversation
Change member delete button, disable when is current user.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a self-service "leave group" flow: new DELETE /api/v1/groups/leave route and handler, service and repository logic to validate and perform leaving (with default-group reassignment and transactional membership removal), and frontend client + UI updates to call the new endpoint. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant Frontend as Frontend
participant API as Backend_API
participant Service as Group_Service
participant Repo as Group_Repo
Browser->>Frontend: Confirm "Leave group"
Frontend->>API: DELETE /api/v1/groups/leave (optional X-Tenant)
API->>Service: build auth ctx, call LeaveGroup(ctx)
Service->>Repo: GroupLeave(ctx, groupID, userID, newDefaultGroupID)
Repo->>Repo: Begin TX → check counts → update DefaultGroupID? → remove membership → commit
Repo-->>Service: success / error
Service-->>API: success / error
API-->>Frontend: 204 No Content / error
Frontend-->>Browser: toast + refresh or reload
Security Recommendations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/pages/collection/index.vue (1)
126-138:⚠️ Potential issue | 🟠 MajorPass
selectedCollection.value.idtoleave()and remove unuseduserIdlookup.The
leave()method only sets theX-Tenantheader when agroupIdargument is provided. Currently, line 138 callsapi.group.leave()with no arguments, which means the backend will fall back to the user'sdefaultGroupIdinstead of the selected collection. This violates multi-tenancy enforcement and could cause the user to unintentionally leave the wrong group.Additionally, the
userIdlookup on lines 126-130 is dead code—it's fetched and checked but never used in the leave call.Fix: Change line 138 to
api.group.leave(selectedCollection.value.id)and remove lines 126-136. Also, line 133 contains a hardcoded string"Missing user id"that should be translatable; use a translation key instead (e.g.,t("errors.missing_user_id")).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/collection/index.vue` around lines 126 - 138, Remove the unused userId lookup (the block that defines and fetches userId and the related missing-user check), replace the call to api.group.leave() with api.group.leave(selectedCollection.value.id) so the X-Tenant header uses the selected collection, and ensure any remaining hardcoded message like "Missing user id" is replaced with the translation key t("errors.missing_user_id") if you keep a similar check elsewhere; target symbols: userId, selectedCollection.value.id, and api.group.leave().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/handlers/v1/v1_ctrl_group.go`:
- Around line 294-307: The HandleGroupLeave handler currently calls
repo.Groups.RemoveMember(auth, auth.GID, auth.UID) directly, bypassing
safeguards; change it to load the current user and the group membership list
(scoped with auth.GID), validate and reject leaves for the user’s only group,
for leaving a group that would be left with zero members, and for leaving their
DefaultGroupID without first reassigning DefaultGroupID, and then perform
removal as an atomic service-layer operation (e.g., add a method in the groups
service like LeaveGroup(auth, gid, uid) that checks user membership, member
count, and updates the user's DefaultGroupID if needed before calling
repo.Groups.RemoveMember), reusing logic from HandleGroupMemberRemove and
HandleGroupDelete and add tests for only-group, last-member, and default-group
scenarios; ensure all queries use auth.GID for multi-tenancy.
---
Outside diff comments:
In `@frontend/pages/collection/index.vue`:
- Around line 126-138: Remove the unused userId lookup (the block that defines
and fetches userId and the related missing-user check), replace the call to
api.group.leave() with api.group.leave(selectedCollection.value.id) so the
X-Tenant header uses the selected collection, and ensure any remaining hardcoded
message like "Missing user id" is replaced with the translation key
t("errors.missing_user_id") if you keep a similar check elsewhere; target
symbols: userId, selectedCollection.value.id, and api.group.leave().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 79f528c0-c6d4-44e4-aa50-8ce0fa76c6b8
📒 Files selected for processing (5)
backend/app/api/handlers/v1/v1_ctrl_group.gobackend/app/api/routes.gofrontend/lib/api/classes/group.tsfrontend/pages/collection/index.vuefrontend/pages/collection/index/members.vue
* Add `LeaveGroup()` service method with validation for only-group, last-member, and default-group reassignment. * Add `LeaveGroupTransaction()` to repo for atomic default group update and member removal. * Refactor `RemoveMember()` and `DeleteGroup()` to use shared validation helpers. * Update HandleGroupLeave handler to delegate to service layer.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/internal/core/services/service_group.go`:
- Around line 121-134: The default-group reassignment happens outside the delete
transaction: move the UpdateDefaultGroup and GroupDelete calls into a single
repository transaction using repo.WithTx (i.e., call svc.repos.WithTx and inside
its closure perform svc.repos.Users.UpdateDefaultGroup(ctx, ctx.UID,
newDefaultGroupID) and then svc.repos.Groups.GroupDelete(ctx.Context, ctx.GID)),
so both the user's DefaultGroupID change and the group deletion occur atomically
and rollback together on error.
In `@backend/internal/data/repo/repo_group.go`:
- Around line 535-546: The current transaction updates DefaultGroupID by userID
alone and then removes the group edge separately, which can leave user state
mutated if the user wasn't actually a member of groupID; change both mutations
to explicitly predicate on the membership (userID + groupID) and fail if no
membership is affected. Concretely, modify the DefaultGroupID update to include
the same membership predicate used to remove the edge (e.g., use the ent
predicate/edge-check with userID and groupID or UpdateOne with a Where/HasGroup
clause) so it only runs when the user is a member of that group, execute both
updates inside the same tx, and check the affected result (or expect a non-nil
error) from both tx.User.Update...SetDefaultGroupID(...) and
tx.Group.Update...RemoveUserIDs(...) — if either affects zero rows, rollback and
return an error to ensure no partial commit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 197bd70f-3018-477d-b94e-edc0243466cf
📒 Files selected for processing (4)
backend/app/api/handlers/v1/v1_ctrl_group.gobackend/internal/core/services/service_group.gobackend/internal/data/repo/repo_group.gofrontend/pages/collection/index.vue
* Before updating the user's default group, ensure user is a member of the group to leave, and it is their default group. * Before removing the user from the group, ensure user is a member of the group.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/internal/core/services/service_group.go`:
- Around line 32-40: Keep the existing pre-check for UX but also enforce the
“last member cannot leave” rule atomically inside the write transaction: inside
the GroupLeave transactional function (the transaction that ultimately calls
RemoveUserIDs), re-query the group's current members (e.g., via
svc.repos.Users.GetUsersByGroupID or a dedicated repo call) and if the length is
<= 1 return a validate.NewRequestError (same error and http.StatusBadRequest)
before calling RemoveUserIDs; this ensures two concurrent leave requests cannot
both succeed even if they passed the initial pre-check.
In `@backend/internal/data/repo/repo_group.go`:
- Around line 536-540: The update call using tx.User.UpdateOneID(userID) only
checks membership in the leaving group (user.HasGroupsWith(group.ID(groupID)))
and DefaultGroupID equals groupID, but it must also validate that the
replacement group is one the user belongs to and scoped to the same tenant: add
a Where(user.HasGroupsWith(group.ID(newDefaultGroupID))) (or equivalent
predicate) to the chain before SetDefaultGroupID(newDefaultGroupID) so the
update requires membership in both the leaving group and newDefaultGroupID and
include group_id in all repo query predicates to maintain multi-tenancy
isolation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ac7667a8-92ef-4a99-a872-ea12727b961b
📒 Files selected for processing (3)
backend/app/api/handlers/v1/v1_ctrl_group.gobackend/internal/core/services/service_group.gobackend/internal/data/repo/repo_group.go
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/api/handlers/v1/v1_ctrl_group.go
Comment on lines
+32
to
+40
| members, err := svc.repos.Users.GetUsersByGroupID(ctx, ctx.GID) | ||
| if err != nil { | ||
| return repo.UserOut{}, err | ||
| } | ||
|
|
||
| // Validate not last member | ||
| if len(members) <= 1 { | ||
| return repo.UserOut{}, validate.NewRequestError(errors.New("cannot leave the group as its last member"), http.StatusBadRequest) | ||
| } |
Contributor
There was a problem hiding this comment.
Make the “last member cannot leave” rule atomic.
Line 32 through Line 40 performs a pre-check outside the write transaction. Two concurrent leave requests can both pass and empty the group. Keep this check for UX, but enforce it again inside GroupLeave transaction right before RemoveUserIDs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/internal/core/services/service_group.go` around lines 32 - 40, Keep
the existing pre-check for UX but also enforce the “last member cannot leave”
rule atomically inside the write transaction: inside the GroupLeave
transactional function (the transaction that ultimately calls RemoveUserIDs),
re-query the group's current members (e.g., via
svc.repos.Users.GetUsersByGroupID or a dedicated repo call) and if the length is
<= 1 return a validate.NewRequestError (same error and http.StatusBadRequest)
before calling RemoveUserIDs; this ensures two concurrent leave requests cannot
both succeed even if they passed the initial pre-check.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/internal/data/repo/repo_group.go`:
- Around line 536-550: The pre-check using
tx.User.Query().Where(user.HasGroupsWith(group.ID(groupID))).Count(ctx) followed
by a separate removal can be raced by concurrent leaves; change the logic to
perform the "not last member" check atomically within the mutation by either
acquiring a row lock on the target group (e.g., SELECT ... FOR UPDATE on the
group row inside the same tx before deleting the membership) or by executing the
membership deletion with a conditional predicate (delete membership only where
the group's membership count > 1) so the delete fails if it would remove the
last user; update the code paths around the Count call and the subsequent
membership remove statement (the tx.User.Query...Count and the membership
deletion) to use one of these atomic approaches and return a clear error if the
conditional delete affected 0 rows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 06c535b8-4868-4796-b476-c1404bd39209
📒 Files selected for processing (1)
backend/internal/data/repo/repo_group.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/internal/core/services/service_group.go`:
- Around line 27-40: Before running the "only group" and "last member" checks,
ensure the request context is the multi-tenant services.Context (not raw
context.Context) and validate that the active group (ctx.GID) is actually
present in the user's memberships; if the user is not a member of ctx.GID return
a request error indicating they are not a member. Concretely: in the function
containing currentUser and the checks, convert/accept the incoming context to
services.Context, verify currentUser.GroupIDs contains ctx.GID, and return a
validate.NewRequestError when it's missing; then proceed to the existing checks
that call svc.repos.Users.GetUsersByGroupID and the validations that use
repo.ErrCannotLeaveLastMember.
- Around line 45-53: getNewDefaultGroupID can pick a stale fallback outside the
transaction causing a not-found 500 in GroupLeave; fix by performing the
replacement selection inside the same DB transaction (use repo.WithTx in
GroupLeave) or move the fallback-selection into the repo layer so the repo
method that updates/validates memberships (repo.GroupRepo methods used by
GroupLeave) chooses and returns the new default within the same tx; additionally
ensure the repo returns a typed domain error (not-found sentinel) that
GroupLeave translates to a request error if revalidation still fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5afbbe11-e799-4fca-ab24-fa1abe098390
📒 Files selected for processing (2)
backend/internal/core/services/service_group.gobackend/internal/data/repo/repo_group.go
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/internal/data/repo/repo_group.go
Comment on lines
+27
to
+40
| // Validate not leaving only group | ||
| if len(currentUser.GroupIDs) <= 1 { | ||
| return repo.UserOut{}, validate.NewRequestError(errors.New("cannot leave the only group you are a member of"), http.StatusBadRequest) | ||
| } | ||
|
|
||
| members, err := svc.repos.Users.GetUsersByGroupID(ctx, ctx.GID) | ||
| if err != nil { | ||
| return repo.UserOut{}, err | ||
| } | ||
|
|
||
| // Validate not last member | ||
| if len(members) <= 1 { | ||
| return repo.UserOut{}, validate.NewRequestError(errors.New(repo.ErrCannotLeaveLastMember.Error()), http.StatusBadRequest) | ||
| } |
Contributor
There was a problem hiding this comment.
Validate that the active group is still one of the user’s memberships.
If ctx.GID is stale, this path can return "cannot leave last member" even though the user is no longer in the group. Fail fast on membership before the only-group / last-member checks to keep the response accurate and preserve multi-tenant scoping.
Suggested fix
- // Validate not leaving only group
+ // Validate membership in the active group
+ isMember := false
+ for _, gid := range currentUser.GroupIDs {
+ if gid == ctx.GID {
+ isMember = true
+ break
+ }
+ }
+ if !isMember {
+ return repo.UserOut{}, validate.NewRequestError(errors.New("you are not a member of this group"), http.StatusBadRequest)
+ }
+
+ // Validate not leaving only group
if len(currentUser.GroupIDs) <= 1 {Based on learnings: "Applies to /backend/internal/core/services/**/*.go : Use the custom services.Context type that extends context.Context with Group ID (GID) and User ID (UID) for multi-tenancy, not raw context.Context"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/internal/core/services/service_group.go` around lines 27 - 40, Before
running the "only group" and "last member" checks, ensure the request context is
the multi-tenant services.Context (not raw context.Context) and validate that
the active group (ctx.GID) is actually present in the user's memberships; if the
user is not a member of ctx.GID return a request error indicating they are not a
member. Concretely: in the function containing currentUser and the checks,
convert/accept the incoming context to services.Context, verify
currentUser.GroupIDs contains ctx.GID, and return a validate.NewRequestError
when it's missing; then proceed to the existing checks that call
svc.repos.Users.GetUsersByGroupID and the validations that use
repo.ErrCannotLeaveLastMember.
Comment on lines
+45
to
+53
| func (svc *GroupService) getNewDefaultGroupID(currentUser repo.UserOut, leavingGroupID uuid.UUID) uuid.UUID { | ||
| if currentUser.DefaultGroupID == leavingGroupID { | ||
| newDefaultGroupID, _ := lo.Find(currentUser.GroupIDs, func(gid uuid.UUID) bool { | ||
| return gid != leavingGroupID | ||
| }) | ||
| return newDefaultGroupID | ||
| } | ||
| return uuid.Nil | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t let a stale replacement default bubble up as a 500.
getNewDefaultGroupID picks the fallback group from a read taken before the write. If the user’s memberships or DefaultGroupID change before backend/internal/data/repo/repo_group.go Lines 533-552 revalidate them, GroupLeave returns a raw not-found error and Lines 131-135 pass it through unchanged. Either choose the replacement default inside the same transaction or have the repo return a typed domain error that gets translated to a request error here.
Based on learnings: "Applies to /backend/internal/core/services/**/*.go : Use repo.WithTx() for multi-step service operations that require transactional consistency"
Also applies to: 129-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/internal/core/services/service_group.go` around lines 45 - 53,
getNewDefaultGroupID can pick a stale fallback outside the transaction causing a
not-found 500 in GroupLeave; fix by performing the replacement selection inside
the same DB transaction (use repo.WithTx in GroupLeave) or move the
fallback-selection into the repo layer so the repo method that updates/validates
memberships (repo.GroupRepo methods used by GroupLeave) chooses and returns the
new default within the same tx; additionally ensure the repo returns a typed
domain error (not-found sentinel) that GroupLeave translates to a request error
if revalidation still fails.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What type of PR is this?
What this PR does / why we need it:
Currently, users cannot leave a group by pressing leave button. This button calls
DELETE /api/v1/groups/members/{user_id}, but inv1Ctrl.HandleGroupMemberRemove, there is a safeguard to prevent user from removing themselves.This PR adds an API at
DELETE /groups/leaveto let the user leave the current group. It also madehandleLeaveCollectioncall this API instead.Which issue(s) this PR fixes:
None.
Special notes for your reviewer:
I doubt whether the safeguard is needed. If we remove this safeguard, we do not need a seperate API endpoint for leaving the group.
Summary by CodeRabbit
New Features
Bug Fixes / Behavior Changes
UI