Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
User and PAT lookup support core/user/user.go, core/user/service.go, internal/store/postgres/user_repository.go, core/user/mocks/repository.go, core/deleter/service.go, core/deleter/mocks/*, core/user/mocks/repository.go, core/userpat/service.go, core/userpat/service_test.go, internal/store/postgres/user_repository_test.go |
The user repository and service add lookup of disabled users. The PostgreSQL lookup continues to exclude soft-deleted users. The PAT service adds a per-user PAT count. Deleter interfaces and mocks expose the lookup methods. |
Deletion assessment and cleanup core/deleter/deleter.go, core/deleter/service.go, core/deleter/service_test.go |
The deleter adds user-deletion check data and a sole-owner blocker. It classifies organizations for deletion or leaving, checks blockers before cleanup, and tests the assessment and deletion sequence. |
Blocked deletion API response internal/api/v1beta1connect/deleter.go, internal/api/v1beta1connect/user.go, internal/api/v1beta1connect/user_test.go |
The API maps blocked user deletions to failed-precondition responses and includes the user ID in blocked-deletion logs. Tests check the response code and structured precondition details. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Feature
Suggested reviewers: kushsharma, whoabhisheksah, rohilsurana
Merge Risk: 
332e3
-
User deletion now checks blockers before changing anything and returns structured failed-precondition responses when blocked. No concrete merge-blocking issue remains in the supplied context. Known non-atomic deletion edge cases have been acknowledged and deferred.
Pre-merge checks: 
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- 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.
Comment @coderabbitai help to get the list of available commands.
Coverage Report for CI Build 40692417526Coverage increased (+0.1%) to 55.922%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
CheckUserDelete lists the orgs where the user is the only member, which are deleted with the user, the orgs the user leaves, what blocks the delete, and the user's PAT count. DeleteUser runs the same check first, with invoices read from the billing provider, and refuses with every blocker before changing anything. Otherwise it deletes the orgs where the user is the only member and removes the user from the rest. A blocked delete returns FAILED_PRECONDITION with one violation per blocker.
1d4bf3c to
56bcc6a
Compare
| if err != nil { | ||
| return err | ||
| } | ||
| if blockers := check.allBlockers(); len(blockers) > 0 { |
There was a problem hiding this comment.
The flat list loses which org each blocker came from, so two sole member orgs with resources give two identical RESOURCES_EXIST violations. Can we add the org id or name to each blocker?
There was a problem hiding this comment.
Each blocker of an org that would be deleted now starts with organization[<name>]: in a refused delete, followed by its message, or by its type when it has no message. So the two entries read organization[acme]: RESOURCES_EXIST and organization[globex]: RESOURCES_EXIST. SOLE_OWNER_WITH_MEMBERS already names its org, and the check keeps each org's blockers under that org, so neither changes. Done in 1ecf313.
| } | ||
|
|
||
| type UserDeleteCheck struct { | ||
| Blockers []Blocker |
There was a problem hiding this comment.
Nothing sets UserDeleteCheck.Blockers, so the top level blockers in #2031's response are always empty. Should SOLE_OWNER_WITH_MEMBERS go here too, or can we drop the field?
There was a problem hiding this comment.
SOLE_OWNER_WITH_MEMBERS belongs to one org, so it sits under that org in organizations_to_leave. Putting it in the top-level list too would show it twice.
The top-level list is for any blocker that doesn't belong to one org. Nothing adds one yet, so it's empty for now. We'll keep the field rather than drop it.
| return &BlockedError{UserID: userID, Blockers: blockers} | ||
| } | ||
| for _, o := range check.OrganizationsToDelete { | ||
| if err := d.DeleteOrganization(ctx, o.Organization.ID); err != nil && !errors.Is(err, organization.ErrNotExist) { |
There was a problem hiding this comment.
If someone joins a sole member org between the check and this loop, the org is deleted with them in it. Is that fine until we have transactions?
There was a problem hiding this comment.
Yes, it's fine for now. The user was told this org would be deleted with their account and confirmed it, so deleting it is what they asked for.
To get in, someone has to accept an invite the user sent earlier, join through the org's verified domain, or be added by a superuser, all in the short gap between the check and that org's delete. A second member check just before the delete would only make the gap smaller. Closing it needs the check and the delete in one transaction across Postgres and SpiceDB, which we don't have yet.
| } | ||
| for _, o := range check.OrganizationsToDelete { | ||
| if err := d.DeleteOrganization(ctx, o.Organization.ID); err != nil && !errors.Is(err, organization.ErrNotExist) { | ||
| return fmt.Errorf("failed to delete org[%s]: %w", o.Organization.ID, err) |
There was a problem hiding this comment.
If an org delete fails after its policies are gone, the retry won't find the org and the user delete goes through, leaving a half deleted org. Do we have a follow up ticket for this?
There was a problem hiding this comment.
We have other cases like this one, where a step fails after an earlier step has already changed something. A plain org delete has this exact gap today. Reordering the steps would only move the problem to another step, so these will be fixed together once we have transaction support.
Until then, a failed step returns an error and raises an alert, so a developer can look at it and clean up if needed.
| // otherwise userService.Delete would leave orphan policy rows behind. | ||
| func (d Service) DeleteUser(ctx context.Context, userID string) error { | ||
| func (d Service) CheckUserDelete(ctx context.Context, userID string) (UserDeleteCheck, error) { | ||
| check, err := d.checkUser(ctx, userID, false) |
There was a problem hiding this comment.
The check reads local invoice rows but the delete asks the provider, so can_delete=true can still end in a refused delete. Can we say this in the proto doc or the PR body?
There was a problem hiding this comment.
Added to the PR description, next to where it says what each one reads: the check can allow the delete, and DeleteUser can still refuse it because of an unpaid invoice that hasn't synced yet.
| default: | ||
| toLeave := OrganizationToLeave{Organization: org} | ||
| if len(self.Roles) > 0 { | ||
| toLeave.RoleTitle = self.Roles[0].Title |
There was a problem hiding this comment.
Nit: Roles[0] has no fixed order, so a user who is both owner and viewer can show as Member. Prefer the owner role title.
There was a problem hiding this comment.
A user holds one org role at a time. Adding a member refuses when the user already has a role, and changing a member's role replaces the old one instead of adding a second. So Roles[0] is the user's role.
| toLeave.Blockers = []Blocker{{ | ||
| Type: BlockerSoleOwnerWithMembers, | ||
| Subject: org.ID, | ||
| SubjectType: schema.OrganizationNamespace, |
There was a problem hiding this comment.
Nit: the other subject types are plain words like invoice, so a SubjectOrganization = "organization" constant would fit better.
There was a problem hiding this comment.
Added SubjectOrganization = "organization", and the SOLE_OWNER_WITH_MEMBERS blocker uses it. Done in 332e38b.
Summary
DeleteUsernow checks what blocks the delete before it changes anything. It refuses while the user is the only owner of an org that has other members, and while an org that would be deleted with the user has blockers of its own. An org where the user is the only member is now deleted with the user.CheckUserDeletegives the same answer without deleting anything. Nothing calls it yet.Changes
core/deleter/service.go:CheckUserDeletegoes through each org where the user has an org-level policy and puts it in one of two lists:SOLE_OWNER_WITH_MEMBERSblocker when the user is the only owner.It also counts the user's PATs.
DeleteUserruns the same check first. If anything blocks, it returns every blocker at once and changes nothing. Otherwise it deletes the orgs where the user is the only member and removes the user from the other orgs. Then it deletes the remaining policies, the PATs and the user, as before.CheckOrganizationDeleteand the user check share the org's billing and resource check,orgDeleteBlockers.core/deleter/deleter.go: theSOLE_OWNER_WITH_MEMBERSblocker type with itsorganizationsubject type, andUserDeleteCheck,OrganizationToDeleteandOrganizationToLeave.BlockedErrorgainsUserID, and its message then starts with "user cannot be deleted yet".internal/api/v1beta1connect/user.go: a blockedDeleteUserreturnsFAILED_PRECONDITIONwith one violation per blocker, like the org and project deletes. This also covers an org delete's blockers passed up through the user delete.core/user,internal/store/postgres/user_repository.go:GetByIDsIncludingDisabled, the same lookup asGetByIDswithout the filter that hides disabled users.core/userpat/service.go:CountByUser.Technical Details
CheckUserDeletereads invoices from the local rows, likeCheckOrganizationDelete. Those rows sync from the billing provider on a timer, so the check can be a few minutes behind.DeleteUserreads the invoices from the provider for every org it would delete, before it deletes any of them. So the check can allow the delete, andDeleteUsercan still refuse it because of an unpaid invoice that hasn't synced yet.organization[<name>]:, followed by its message, or by its type when it has no message. A blocker of an org the user leaves already names its org. The check keeps each org's blockers under that org, without the label.Test Plan
core/deleter/service_test.goTestCheckUserDelete: each org goes to the right list. A disabled member counts, and a member without a user row does not. An org that goes with the user carries its blockers, judged on the local invoice rows. Nothing blocks when every org the user leaves keeps an owner. An org that no longer exists is skipped, and a failed read stops the check.TestDeleteUser: the steps run in order: orgs that go with the user, orgs the user leaves, remaining policies, PATs, user. A blocked delete returns every blocker, with invoices judged on provider data, before it changes anything. Each blocker of an org that would be deleted is labeled with the org's name, so the same blocker in two orgs stays apart. An org deleted since the check does not stop the delete. A failure at any step stops the delete before the next step.TestUserDeleteCheck_CanDelete: a blocker in any of the three places means the user cannot be deleted.internal/api/v1beta1connect/user_test.go: a blocked user delete returnsFAILED_PRECONDITIONwith one violation per blocker. So does an org delete's blocker passed up through the user delete.core/userpat/service_test.goTestService_CountByUserandinternal/store/postgres/user_repository_test.goTestGetByIDsIncludingDisabled.go test ./core/deleter/... ./core/user/... ./core/userpat/... ./internal/api/v1beta1connect/...passes.go test ./test/e2e/...passes. End-to-end tests for these rules are in test(e2e): cover the user delete check end to end #2028.make lintreports no issues.SQL Safety
goqu.Ex{}:GetByIDsIncludingDisableduses the samegoqu.Ex{"id": goqu.Op{"in": userIDs}}asGetByIDs.ToSQL()callers capture and forward params (query, params, err := stmt.ToSQL(); db.…Context(ctx, …, query, params...)). Neverquery, _, err := ….?placeholders inside single-quoted SQL literals ingoqu.L.//nolint:forbidigoor// #nosec G20xannotations.