feat(account): add self-service deactivation and verified email change#300
Conversation
The account deactivate button did nothing and the email field was read-only
with no way to change it. Both are now real, self-service flows.
Backend: AccountService.Deactivate flips is_active off and evicts the user's
sessions (idempotent). The email change is a two-step verified flow backed by
an email_change_requests table: RequestEmailChange stores a hashed, expiring
code (bound to the user + new address) and returns it for the handler to email
to the new address; ConfirmEmailChange checks the code, re-checks availability,
and swaps the email. New endpoints POST /api/users/me/{deactivate,
change-email,change-email/verify}/.
Frontend: a confirmation dialog on Deactivate (then signs out), and an inline
change-email flow (new address -> emailed code -> confirm) on the profile page.
Closes Devlaner#209
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds self-service account deactivation and verified email-change flows across the API, database, service layer, and settings UI. New endpoints, persistence, validation, session eviction, email delivery, confirmation handling, and tests are included. ChangesAccount self-service
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsPage
participant AuthHandler
participant AccountService
participant EmailQueue
User->>SettingsPage: submit new email
SettingsPage->>AuthHandler: POST change-email
AuthHandler->>AccountService: request verification code
AccountService-->>AuthHandler: return code
AuthHandler->>EmailQueue: publish confirmation email
User->>SettingsPage: submit verification code
SettingsPage->>AuthHandler: POST change-email/verify
AuthHandler->>AccountService: confirm email change
AccountService-->>AuthHandler: return new email
AuthHandler-->>SettingsPage: return verified email
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
apps/api/internal/store/email_change_request.go (1)
48-53: 🔒 Security & Privacy | 🔵 TrivialNo cleanup path for expired, never-confirmed requests.
An
EmailChangeRequest(which stores a PII new-email address) is only removed when the user confirms or attempts to confirm after expiry. If the user never returns, the row — and the target email address — persists indefinitely. Consider a periodic purge job for rows pastExpiresAt.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/internal/store/email_change_request.go` around lines 48 - 53, Extend the email change request store with a periodic cleanup operation for expired requests, deleting rows whose ExpiresAt is in the past regardless of confirmation status. Anchor the implementation near EmailChangeRequestStore and DeleteByUserID, and ensure the purge can be invoked by a scheduled job using the provided context.apps/api/internal/service/account.go (1)
74-86: 🩺 Stability & Availability | 🔵 TrivialConsider guarding against deactivating the last active instance admin.
Since reactivation is admin-only, a sole admin who self-deactivates (accidentally or otherwise) could permanently lock the instance out of admin access, with no self-service recovery path. Worth checking
InstanceAdminStorefor remaining active admins before allowing self-deactivation of the last one — or documenting this as an accepted risk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/internal/service/account.go` around lines 74 - 86, The AccountService.Deactivate method should prevent deactivating the last active instance administrator. Before updating an active user to inactive, use the existing InstanceAdminStore to verify another active admin remains; reject the deactivation when this user is the sole active admin, while preserving normal user deactivation and session cleanup behavior otherwise.apps/api/internal/service/account_test.go (1)
46-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for code expiry.
No test exercises the
time.Now().After(req.ExpiresAt)branch inConfirmEmailChange. Since the store is directly accessible in tests, you could insert/backdate a request viastore.NewEmailChangeRequestStore(db)with an already-pastExpiresAtand assertErrInvalidEmailCodeplus that the row is deleted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/internal/service/account_test.go` around lines 46 - 73, The email-change tests lack coverage for expired confirmation codes. Add a test near TestAccount_EmailChange_HappyPath that creates or backdates an email-change request through store.NewEmailChangeRequestStore(db) with an expired ExpiresAt, then calls ConfirmEmailChange and asserts service.ErrInvalidEmailCode and verifies the expired request row is deleted.apps/api/internal/handler/account_test.go (1)
15-30: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTest only proves single-session eviction, not "all sessions."
The PR objective states deactivation evicts all sessions, but this test only checks that the one session used to deactivate gets invalidated. Add a second independent session for the same user and assert it's evicted too.
✅ Proposed test extension (assumes `testutil.LoginAs` can create independent sessions per call)
func TestAuth_DeactivateMe(t *testing.T) { ts := testutil.NewTestServer(t) u := testutil.CreateUser(t, ts.DB) session := testutil.LoginAs(t, ts.DB, u) + session2 := testutil.LoginAs(t, ts.DB, u) rr := ts.POST("/api/users/me/deactivate/", nil, session) require.Equal(t, http.StatusNoContent, rr.Code, "body=%s", rr.Body.String()) // The session was evicted, so the next authenticated call is unauthorized. rr2 := ts.GET("/api/users/me/", session) require.Equal(t, http.StatusUnauthorized, rr2.Code) + + // A second, independent session for the same user must also be evicted. + rr3 := ts.GET("/api/users/me/", session2) + require.Equal(t, http.StatusUnauthorized, rr3.Code) got, err := store.NewUserStore(ts.DB).GetByID(context.Background(), u.ID) require.NoError(t, err) require.False(t, got.IsActive) }Please confirm
testutil.LoginAssupports creating multiple independent sessions for the same user before applying this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/internal/handler/account_test.go` around lines 15 - 30, Extend TestAuth_DeactivateMe to create a second independent session with testutil.LoginAs before deactivation, then issue an authenticated request using that session after deactivation and assert it also returns http.StatusUnauthorized. Preserve the existing assertion for the deactivation session and the inactive-user verification, and confirm LoginAs creates distinct sessions per call before relying on it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/internal/router/router.go`:
- Around line 271-273: Apply middleware.RateLimit to both the change-email
request and verification routes in the router setup, matching the existing
rate-limit configuration used by sign-in, forgot-password, or
magic-code/request. Ensure the limits cover both authHandler.RequestEmailChange
and authHandler.VerifyEmailChange to throttle inbox-spam and OTP brute-force
attempts.
In `@apps/api/internal/service/account.go`:
- Around line 42-44: Update NewAccountService to reject an empty secret during
AccountService construction, failing fast instead of permitting insecure
operation. Remove the hardcoded fallback key from hashCode and ensure hashing
uses the validated service secret; preserve normal behavior when a non-empty
secret is provided.
- Around line 124-163: The ConfirmEmailChange method must translate
duplicate-key errors from s.users.Update into ErrEmailInUse. Wrap the update
error handling so duplicate-email failures delete the pending change and return
the same conflict response used by the existing availability check, while
preserving other update errors unchanged.
In `@apps/api/internal/store/email_change_request.go`:
- Around line 32-46: Replace the check-then-act logic in
EmailChangeRequestStore.Upsert with a single database-level atomic upsert keyed
by UserID. Ensure concurrent requests update the existing row’s NewEmail,
CodeHash, and ExpiresAt fields while inserting when absent, without calling
GetByUserID or separate Save/Create operations.
In `@apps/web/src/pages/SettingsPage.tsx`:
- Around line 415-423: Update deactivateAccount to capture the rejection from
accountService.deactivate or logout, store a user-facing message in a
deactivateError state, and continue resetting deactivateBusy. Render
deactivateError near the Deactivate account control or within the confirmation
Modal so failures remain visible after the Modal closes.
- Around line 380-413: Guard the asynchronous requestEmailChange and
verifyEmailChange state updates against cancellation so resetEmailChange cannot
be undone by stale promise results. Add a useRef cancellation/generation token,
invalidate it in resetEmailChange, and capture the current token when each
request starts; only apply success, error, and busy-state updates when the token
still matches, including preventing stale verify results from updating the
profile or refreshing the user.
---
Nitpick comments:
In `@apps/api/internal/handler/account_test.go`:
- Around line 15-30: Extend TestAuth_DeactivateMe to create a second independent
session with testutil.LoginAs before deactivation, then issue an authenticated
request using that session after deactivation and assert it also returns
http.StatusUnauthorized. Preserve the existing assertion for the deactivation
session and the inactive-user verification, and confirm LoginAs creates distinct
sessions per call before relying on it.
In `@apps/api/internal/service/account_test.go`:
- Around line 46-73: The email-change tests lack coverage for expired
confirmation codes. Add a test near TestAccount_EmailChange_HappyPath that
creates or backdates an email-change request through
store.NewEmailChangeRequestStore(db) with an expired ExpiresAt, then calls
ConfirmEmailChange and asserts service.ErrInvalidEmailCode and verifies the
expired request row is deleted.
In `@apps/api/internal/service/account.go`:
- Around line 74-86: The AccountService.Deactivate method should prevent
deactivating the last active instance administrator. Before updating an active
user to inactive, use the existing InstanceAdminStore to verify another active
admin remains; reject the deactivation when this user is the sole active admin,
while preserving normal user deactivation and session cleanup behavior
otherwise.
In `@apps/api/internal/store/email_change_request.go`:
- Around line 48-53: Extend the email change request store with a periodic
cleanup operation for expired requests, deleting rows whose ExpiresAt is in the
past regardless of confirmation status. Anchor the implementation near
EmailChangeRequestStore and DeleteByUserID, and ensure the purge can be invoked
by a scheduled job using the provided context.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 48e3be47-02d5-4a2a-a7d8-e5c1fcce8dd8
📒 Files selected for processing (11)
apps/api/internal/handler/account_test.goapps/api/internal/handler/auth.goapps/api/internal/model/email_change_request.goapps/api/internal/router/router.goapps/api/internal/service/account.goapps/api/internal/service/account_test.goapps/api/internal/store/email_change_request.goapps/api/migrations/000009_email_change_requests.down.sqlapps/api/migrations/000009_email_change_requests.up.sqlapps/web/src/pages/SettingsPage.tsxapps/web/src/services/accountService.ts
Addresses review feedback on the email-change/deactivation flows: - Rate-limit the change-email request and verify endpoints, and invalidate a code after 5 wrong attempts (attempts column), to stop OTP brute-forcing. - Make the pending-request upsert a single atomic OnConflict write so concurrent requests can't race into a unique-constraint error. - Reuse the shared magic-code dev fallback key instead of a second hardcoded HMAC key. - Web: ignore stale email-change responses after cancel (token guard), and surface deactivation failures in the confirm dialog instead of swallowing them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Feature summary
The account "Deactivate" button did nothing and the email field was read-only with no way to change it. Both are now real, self-service flows.
Linked issues / discussion
Closes #209
User-facing behavior
What changed
API (
apps/api/)000009:email_change_requests(one pending row per user; hashed code + expiry).service.AccountService:Deactivate— setsis_active=falseand evicts sessions (SessionStore.DeleteByUserID); idempotent.RequestEmailChange— validates (not same, not taken), stores a hashed 15-minute code bound to the user + new address, and returns the plaintext code for the handler to email.ConfirmEmailChange— constant-time code check, re-checks availability at confirm time, swaps the email, clears the request.AuthHandler:POST /api/users/me/deactivate/,POST /api/users/me/change-email/,POST /api/users/me/change-email/verify/. The request endpoint requires SMTP + the email queue (503 otherwise) and emails the code to the new address.UI (
apps/web/)accountServicefor the three endpoints; a confirmModalfor deactivate (thenlogout()); an inline request → verify email-change flow on the profile page.Database
Why this design
The code is stored server-side (hashed, expiring, bound to user + target email) in Postgres rather than Redis so the flow is durable, always available, and testable with the repo's testcontainers harness. The service returns the plaintext code to the handler (never logging/storing it) so the email-sending concern stays in the handler and the logic stays unit-testable.
Test plan
go vet ./...and fullgo test ./...green (testcontainers).TestAccount_Deactivate,TestAccount_EmailChange_HappyPath(wrong code rejected, correct code swaps + clears),TestAccount_EmailChange_InUse,TestAccount_EmailChange_SameEmail,TestAuth_DeactivateMe(204 + session eviction + inactive through the real router),TestAuth_RequestEmailChange_RequiresSMTP.npm run typecheck,npm run lint,npm run buildgreen.Out of scope (follow-ups)
Rollout notes
AI assistance
Claude Code— and AI-assisted commits include aCo-Authored-By:trailerChecklist
000009_*.up.sqlAND matching.down.sql--no-verifybypass on the commit (full suite ran on pre-commit)Summary by CodeRabbit