fix(api): clear Google OAuth tokens on account deactivation

[thomasluizon]

Round-2 deactivation-leak audit (verify-first)

Two deactivation leaks flagged for verification against the #324 global query filter (HasQueryFilter(u => !u.IsDeactivated)).

1. FriendGraphService.ResolveTargetAsync — already covered ✅

Resolution by handle/referral code goes through IGenericRepository<User>.FindAsync, which runs _dbSet.AsNoTracking().Where(...) — the global filter applies (no IgnoreQueryFilters/raw SQL). Deactivated users are not resolvable. No source change; added FriendGraphServiceDeactivationTests (4 tests) exercising the actual ResolveTargetAsync code path by handle and referral code to lock it against regression.

2. Google OAuth tokens survived deactivation — fixed 🔧

User.Deactivate() set the deactivation flags but left GoogleAccessToken / GoogleRefreshToken (and GoogleCalendarAutoSyncEnabled) populated on the row for the 7-day (or plan-expiry + 7) deletion grace. The row is hidden by the global filter, but a live, decryptable, refreshable OAuth secret sitting on a scheduled-for-deletion account is unnecessary exposure — the refresh token can mint new Google access tokens.

Fix at source: clear both tokens and disable auto-sync in Deactivate(), mirroring the existing MarkCalendarSyncReconnectRequired / ResetAccount convention (which also null tokens on connection loss and preserve the EnableCalendarAutoSync invariant that auto-sync-enabled ⟹ token present). Reactivation re-establishes tokens via SetGoogleTokens in GoogleAuthCommand.HandlePostLogin when the client re-grants calendar access.

Tests

• Domain (UserTests): Deactivate_ClearsGoogleCalendarConnection, CancelDeactivation_LeavesGoogleTokensCleared_ReconnectRequired.
• Application (ConfirmAccountDeletionCommandHandlerTests): valid code clears tokens; invalid code leaves them intact.
• Infrastructure (FriendGraphServiceDeactivationTests): deactivated users unresolvable by handle/referral; active users still resolvable.
• Full affected suites green: Domain 506, Application 2630, Infrastructure 1538 — 0 failures.

Refs thomasluizon/orbit-ui-mobile#243

[claude]

Review Complete

Scope: PR #335 in thomasluizon/orbit-api — "fix(api): clear Google OAuth tokens on account deactivation"
Recommendation: APPROVE

Summary

The diff adds three lines to User.Deactivate() (src/Orbit.Domain/Entities/User.cs:363-371), clearing GoogleAccessToken/GoogleRefreshToken/GoogleCalendarAutoSyncEnabled when a user schedules account deletion, closing a leak where a live, decryptable, refreshable Google OAuth secret sat on a row hidden only by the EF global query filter (HasQueryFilter(u => !u.IsDeactivated), OrbitDbContext.cs:669). The change correctly mirrors the existing MarkCalendarSyncReconnectRequired/ResetAccount conventions, has a single well-guarded caller (ConfirmAccountDeletionCommandHandler, itself rate-limited and constant-time-code-checked), and ships solid new unit-test coverage across Domain, Application, and Infrastructure layers. All 4 changed files were reviewed and none show a Critical/High defect.

Findings

Critical

None.

High

None.

Medium

[MEDIUM] Local token-clear has no corresponding Google-side revocation

• dimension: Security (#12) — Cryptography / token lifecycle, defense-in-depth
• location: src/Orbit.Domain/Entities/User.cs:368-369 (and the deletion flow it serves, src/Orbit.Application/Auth/Commands/ConfirmAccountDeletionCommand.cs:48)
• issue: Deactivate() nulls the locally-stored GoogleAccessToken/GoogleRefreshToken, but nothing in the codebase calls Google's https://oauth2.googleapis.com/revoke (confirmed via full-repo grep for revoke/Revoke — the only hits are the unrelated ApiKeys/Commands/RevokeApiKeyCommand.cs app-level API-key feature).
• risk: If a refresh token was captured before deactivation (DB dump, log leak, a decrypted-at-rest backup snapshot), it remains valid and usable against Google's API indefinitely — refresh tokens don't expire on a fixed schedule — even though Orbit itself no longer stores or uses it.
• fix: Add a best-effort, non-blocking call to Google's OAuth revoke endpoint with the previous GoogleRefreshToken before/alongside nulling the fields (log-and-continue on failure so a Google-side outage never blocks account deletion). Track as a follow-up if out of scope for this PR — this is defense-in-depth, not a regression this PR introduces.
• reference: OWASP — credential/session invalidation on account termination

Low / Info

None posted (signal gate).

Subagents

Agent	Verdict
security-reviewer	PASS (1 Medium — Google-side token revocation gap, see above)
contract-aligner	N/A — diff touches no DTO, Controller route, or packages/shared type/endpoints.ts

Validation

Check	Result
Build (dotnet)	N/A — covered by separate required CI check (Build)
Tests (dotnet)	N/A — covered by separate required CI check (Unit Tests). PR body states the author ran the affected suites locally (Domain 506, Application 2630, Infrastructure 1538 — 0 failures).

Deferred — N/A dimensions & files not verdicted

• DESIGN.md/AI-slop (#8): N/A — no apps/* UI files in this diff.
• Parity web↔mobile (#9): N/A — backend-only diff, no apps/ changes on either side to mirror.
• i18n (#10): N/A — no new user-facing strings.
• Contract drift + backward-compat (#11): N/A — no DTO, Controller route, or shared-type field changed; the Google tokens were never exposed via a response DTO (confirmed GetProfileQuery/GetCalendarAutoSyncStateQuery expose only Enabled/Status/HasGoogleConnection, never the raw tokens), so there is no client-visible field to break.
• FEATURES.md parity (#14): N/A — pure bugfix/security-hardening of existing deactivation behavior, no new/changed/removed user-facing feature surface.
• All 4 changed files (User.cs, ConfirmAccountDeletionCommandHandlerTests.cs, UserTests.cs, FriendGraphServiceDeactivationTests.cs) received a verdict — nothing left unexamined.

What's good

• Correctly mirrors an established in-file convention (MarkCalendarSyncReconnectRequired, ResetAccount) instead of inventing a new pattern.
• Single, well-understood caller with existing rate-limiting and constant-time code comparison — no new attack surface.
• Test coverage lands at all three affected layers (Domain, Application, Infrastructure) exactly where the rubric's "every new command/handler/service has a unit test" rule expects it, including a regression-locking test for the already-safe FriendGraphService.ResolveTargetAsync path audited in the same PR.
• PR body is exemplary: explicit verify-first framing, cites the exact global-filter mechanism relied upon, and links the mobile-side issue (thomasluizon/orbit-ui-mobile#243).

Recommendation

Approve as-is. Open a follow-up ticket for Google-side refresh-token revocation (the Medium finding) — it closes a real residual-exposure gap but is additive defense-in-depth on top of a fix that already achieves its stated goal (getting the secret out of the row before the 7-day hard-delete window).

Severity	Count
Critical (incl. breaks old mobile clients)	0
High	0
Medium	1
Low / Info	0

[claude]

Code Review: PR #335 — fix(api): clear Google OAuth tokens on account deactivation

Scope: PR #335 in thomasluizon/orbit-api
Recommendation: APPROVE

Summary

Three-line domain change: User.Deactivate() now nulls GoogleAccessToken/GoogleRefreshToken and disables GoogleCalendarAutoSyncEnabled when an account is deactivated, closing a gap where a live, decryptable, refreshable Google OAuth secret sat on a scheduled-for-deletion row for up to the 7-day grace period. The PR also verifies (with new regression tests, no source change) that FriendGraphService.ResolveTargetAsync already can't resolve deactivated users, via the existing HasQueryFilter(u => !u.IsDeactivated) global filter. I independently traced the single production call site (ConfirmAccountDeletionCommandHandler.Handle), the reactivation path (GoogleAuthCommand.HandlePostLogin), the background sync path (RunCalendarAutoSyncCommand), and the encryption-at-rest config for these two columns — the change is correct, narrowly scoped, and covered by tests at the domain, application, and infrastructure layers.

Findings

Critical

None.

High

None.

Medium

[MEDIUM] Deactivate() clears the secrets but leaves related calendar-sync metadata stale
· dimension: Correctness / SOLID consistency
· location: orbit-api/src/Orbit.Domain/Entities/User.cs:363-371
· issue: Deactivate() nulls GoogleAccessToken, GoogleRefreshToken, and GoogleCalendarAutoSyncEnabled, but leaves GoogleCalendarAutoSyncStatus, GoogleCalendarSelectedIds, GoogleCalendarLastSyncError, and GoogleCalendarSyncReconciledAt untouched. The PR description states the fix "mirrors the existing MarkCalendarSyncReconnectRequired / ResetAccount convention," but MarkCalendarSyncReconnectRequired (User.cs:336-343) also resets GoogleCalendarAutoSyncStatus, and ResetAccount (User.cs:543-563) clears all eight Google-prefixed fields — this method only clears three.
· risk: Not a security hole (none of the four leftover fields are secrets, and EnableCalendarAutoSync() still hard-gates on GoogleAccessToken is null so sync can't silently resume), but it's a real inconsistency with the pattern the PR itself cites, and GetCalendarAutoSyncStateQuery will surface a stale Status enum value (e.g. still Idle/TransientError) alongside Enabled: false after a deactivate→reactivate cycle that doesn't go through Google re-auth — cosmetically confusing to a client reading the calendar-sync state endpoint.
· fix: Add the remaining four assignments to Deactivate() (mirroring ResetAccount): GoogleCalendarAutoSyncStatus = null; GoogleCalendarSelectedIds = null; GoogleCalendarLastSyncError = null; GoogleCalendarSyncReconciledAt = null;
· reference: orbit-api CLAUDE.md "No workarounds / root-cause every bug" + internal consistency with ResetAccount/MarkCalendarSyncReconnectRequired (User.cs:336-343, 543-563)

Low / Info

[INFO] Test name doesn't fully match its assertions
· dimension: test clarity (Low/Info, no action required)
· location: orbit-api/tests/Orbit.Domain.Tests/Entities/UserTests.cs:927 (CancelDeactivation_LeavesGoogleTokensCleared_ReconnectRequired)
· issue: The _ReconnectRequired suffix implies the test verifies GoogleCalendarAutoSyncStatus transitions to ReconnectRequired, but the test only asserts IsDeactivated, GoogleAccessToken, and GoogleRefreshToken.
· risk: None — purely a naming clarity nit.
· fix: Rename to CancelDeactivation_LeavesGoogleTokensCleared or add the status assertion.

Subagents

Agent	Verdict
security-reviewer	PASS (one Low-severity note, folded into the Medium finding above — no exploitable gap; encryption-at-rest, no-secret-field-missed, and the deletion-code timing-safe compare / rate limiting were all independently re-verified and are unaffected)
contract-aligner	N/A — no DTO, Controller route, or packages/shared surface changed in this diff

Validation

Check	Result
Build (dotnet)	N/A — this PR runs Build / Unit Tests as separate required checks; this review session's tooling is scoped to gh pr commands only
Tests (dotnet)	N/A — same as above

Deferred — N/A dimensions & files not verdicted

• Dimension 8 (DESIGN.md/AI-slop) — N/A, no apps/* UI files in this diff (backend-only PR).
• Dimension 9 (Parity web↔mobile) — N/A, not verifiable in CI (sibling orbit-ui-mobile repo not checked out); also not applicable — no frontend surface touched.
• Dimension 10 (i18n) — N/A, no user-facing strings added.
• Dimension 11 (Contract drift + backward-compat guard) — checked and N/A: no packages/shared Zod schema or orbit-api DTO/Controller route changed. The changed fields are domain-entity-internal mutations, never serialized directly to a response DTO (GetCalendarAutoSyncStateQuery already only exposed a derived HasGoogleConnection: bool, unchanged here) — no old-mobile-client break.
• Dimension 14 (FEATURES.md parity) — N/A, bugfix/security-hardening with no user-facing feature surface added, changed, or removed.
• Files: all 4 changed files (src/Orbit.Domain/Entities/User.cs, tests/Orbit.Application.Tests/Commands/Auth/ConfirmAccountDeletionCommandHandlerTests.cs, tests/Orbit.Domain.Tests/Entities/UserTests.cs, tests/Orbit.Infrastructure.Tests/Persistence/FriendGraphServiceDeactivationTests.cs) were read in full and given a verdict — nothing deferred there.

What's good

• Correctly scoped root-cause fix at the domain-entity boundary (User.Deactivate()), not a controller/service-level patch — the invariant (auto-sync-enabled ⟹ token present) is preserved by construction.
• Reactivation path (GoogleAuthCommand.HandlePostLogin) correctly re-establishes tokens only when the client actually re-grants Google access; a non-Google reactivation correctly leaves the account disconnected rather than resurrecting stale tokens.
• Test coverage spans all three affected layers (Domain factory/mutator test, Application handler test, Infrastructure query-filter regression test), including both success and failure branches of ConfirmAccountDeletionCommandHandler.
• The FriendGraphServiceDeactivationTests addition is good defensive regression coverage for a path that needed no source fix, locking in the already-correct behavior against future refactors of FindAsync/the query filter.
• Correctly left the existing encryption-at-rest for these two columns (NullableEncryptionValueConverter) alone — this fix is about active-secret lifecycle, not at-rest protection, which was never the gap.

Recommendation

Approve as-is. The one Medium finding (stale calendar-sync metadata fields) is a real but non-blocking consistency gap — safe to land now and pick up in a fast-follow that extends Deactivate() to clear the remaining four GoogleCalendar* fields, mirroring ResetAccount().

[claude]

Code Review: PR #335 — fix(api): clear Google OAuth tokens on account deactivation

Recommendation: APPROVE

Summary

Third review pass on this PR. The diff has not changed since the prior bot review — the two merge commits added afterward only bring in unrelated commits already on main (Stripe webhook retry fix, Sentry PII scrubber, push-notification payload sanitization); none touch this PR's 4 files. Independently re-verified the code (including a dedicated security-reviewer subagent pass) rather than trusting prior review text.

Findings

Critical

None.

High

None.

Medium

[Carried forward, non-blocking] Deactivate() clears Google secrets but leaves calendar-sync metadata stale, inconsistent with ResetAccount()

• Location: src/Orbit.Domain/Entities/User.cs:363-371 vs src/Orbit.Domain/Entities/User.cs:543-562 (ResetAccount)
• Deactivate() nulls GoogleAccessToken, GoogleRefreshToken, and sets GoogleCalendarAutoSyncEnabled = false — 3 of the 8 Google-prefixed fields. ResetAccount() clears all 8, including GoogleCalendarAutoSyncStatus, GoogleCalendarSelectedIds, GoogleCalendarLastSyncError, GoogleCalendarSyncReconciledAt.
• Not a security hole — EnableCalendarAutoSync() still hard-gates on GoogleAccessToken is null, so sync cannot silently resume. But GetCalendarAutoSyncStateQueryHandler surfaces Status: user.GoogleCalendarAutoSyncStatus ?? Idle directly to clients, so after a deactivate→CancelDeactivation() cycle a client could see a stale Status (e.g. TransientError/ReconnectRequired) alongside Enabled: false — cosmetically confusing, not unsafe.
• Suggested fix (non-blocking, fast-follow): mirror ResetAccount() by also clearing GoogleCalendarAutoSyncStatus, GoogleCalendarSelectedIds, GoogleCalendarLastSyncError, GoogleCalendarSyncReconciledAt in Deactivate().

Low / Info

None.

Security review (independent subagent pass)

• Tokens are encrypted at rest via nullableEncConverter on GoogleAccessToken/GoogleRefreshToken (OrbitDbContext.cs:693-694) — nulling is consistent with existing encrypted-column handling, no plaintext leakage.
• Only production call site (ConfirmAccountDeletionCommand.cs:48) is reached only after fixed-time code comparison (CryptographicOperations.FixedTimeEquals) and rate-limiting (MaxVerificationAttempts) — no auth bypass introduced.
• CancelDeactivation() intentionally does not restore Google tokens — correct, forces re-consent rather than resurrecting stale/possibly-revoked tokens.
• Global query filter (HasQueryFilter(u => !u.IsDeactivated)) and GenericRepository usage confirm deactivated users are unresolvable via handle/referral code, matching the PR's stated intent.
• No missing [Authorize], no new endpoints, no token values logged.
• No Critical/High security issues found.

Validation

Build/test: N/A for this review — covered by separate required CI checks (Build / Unit Tests / SonarCloud) per this PR's setup. PR body states author ran affected suites locally (Domain 506, Application 2630, Infrastructure 1538 — 0 failures).

Deferred / N/A dimensions

• Cross-repo parity (mobile consumer, packages/shared): not verifiable in this job — orbit-ui-mobile isn't checked out; also not applicable since no DTO/Controller/shared-type field changed.
• DESIGN.md/AI-slop, i18n, FEATURES.md parity: N/A — backend-only bugfix, no user-facing surface changed.

What's good

• Root-cause fix at the correct boundary (User.Deactivate()), preserving the invariant "auto-sync-enabled ⟹ token present" by construction.
• Reactivation path only re-establishes tokens on an actual Google re-grant.
• Test coverage spans Domain, Application, and Infrastructure layers, including a regression lock (FriendGraphServiceDeactivationTests) for a path that needed no source change.
• Clear, well-reasoned PR description.

Recommendation

APPROVE. The diff is correct and low-risk. The one surviving Medium (stale GoogleCalendar* metadata fields on Deactivate()) is real but non-blocking — safe to land now, recommend a fast-follow.

[claude]

Review Complete

Scope: PR #335 in thomasluizon/orbit-api — "fix(api): clear Google OAuth tokens on account deactivation"
Recommendation: APPROVE

Summary

This is the fourth bot review pass on PR #335. The three prior reviews all APPROVED, the last one after a dedicated security-reviewer subagent pass. Since then, one more merge commit (08e99f25, rate-limit hardening from #332) landed on main and was merged in, but it touches none of PR #335's files (AccountabilityController.cs, AchievementsController.cs, AiController.cs, FriendsController.cs, TagsController.cs, DistributedRateLimitService.cs and tests only). The diff under review is unchanged from the last approval.

Core claims were re-verified independently via a fresh security-reviewer subagent pass rather than trusting prior review text: GoogleAccessToken/GoogleRefreshToken are the only OAuth secret fields on User; the invariant "GoogleCalendarAutoSyncEnabled == true ⟹ GoogleAccessToken != null" holds before and after the change (EnableCalendarAutoSync() still gates on token presence); the sole production call site (ConfirmAccountDeletionCommand.cs:48) never reads the Google fields after calling Deactivate(); both tokens are encrypted at rest (OrbitDbContext.cs:693-694); and no downstream reader assumes the fields stay in lockstep in a way this change violates.

Findings

Critical: None.
High: None.
Medium: One carried-forward, non-blocking finding, unchanged from the prior review — Deactivate() (src/Orbit.Domain/Entities/User.cs:363-371) clears 3 of the 8 Google* fields (GoogleAccessToken/GoogleRefreshToken/GoogleCalendarAutoSyncEnabled), while ResetAccount() (User.cs:543-562) clears all 8, including GoogleCalendarAutoSyncStatus. Not a security hole — EnableCalendarAutoSync() still hard-gates on token presence — but after a Deactivate() → CancelDeactivation() cycle, GoogleCalendarAutoSyncStatus can remain stale (e.g. TransientError) alongside Enabled: false. Confirmed non-user-impacting: GetCalendarAutoSyncStateQuery derives HasGoogleConnection from GoogleAccessToken is not null, not from the stale status field. Suggested fast-follow: mirror ResetAccount() by also clearing GoogleCalendarAutoSyncStatus, GoogleCalendarSelectedIds, GoogleCalendarLastSyncError, GoogleCalendarSyncReconciledAt.
Low/Info: None.

Subagents

Agent	Verdict
security-reviewer	PASS — independently re-verified: OAuth-field scope correct, invariant holds, no use-after-clear, encryption-at-rest confirmed, no consumer breaks
contract-aligner	N/A — no DTO, Controller route, or packages/shared surface changed (pure domain-entity fix)

Validation

Check	Result
Build (dotnet)	N/A — CI runs Build/Unit Tests/SonarCloud as separate required checks; SonarCloud already reported Quality Gate Passed
Tests (dotnet)	N/A per above — PR body states author ran affected suites locally (Domain 506, Application 2630, Infrastructure 1538 — 0 failures)

Deferred — N/A dimensions

• Cross-repo parity / contract drift: not verifiable in this CI job (orbit-ui-mobile not checked out); also not applicable — no DTO/Controller/shared-type field changed.
• DESIGN.md/AI-slop, Parity, i18n, FEATURES.md parity: N/A — backend-only bugfix, no user-facing surface changed.

Severity	Count
Critical (incl. breaks old mobile clients)	0
High	0
Medium	1 (carried forward, non-blocking)
Low / Info	0

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud