fix(api): exclude deactivated users at the root via EF global query filter#324
Conversation
…ilter Deactivated users leaked into friend lists, the friend feed, public profiles (queryable by slug), challenge-invite notifications, and the marketing audience because no query excluded them by default (1 Blocker + 5 High, all the same root cause). Add a global query filter `!u.IsDeactivated` on the User entity so every query excludes deactivated users by default, then restore the handful of paths that MUST still see them via explicit .IgnoreQueryFilters(): - login/auth email lookup (VerifyCode, GoogleAuth) so a deactivated user is found and reactivated instead of a spurious re-create/unique clash; - the account-deletion background job (scheduled-users query + delete); - admin authorization (live IsAdmin check, behavior preserved); - Stripe/Play subscription-webhook reconciliation and the Play purchase-token uniqueness guard, so billing events for a deactivated- but-still-subscribed user are not dropped. Current-user self-service paths (GetByIdAsync/Find honors the filter in EF Core 10) now resolve a deactivated caller as UserNotFound until they reactivate by logging in - the intended feature behavior. No DTO or endpoint contract change, so no client-side edits. Refs thomasluizon/orbit-ui-mobile#243 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #324
Scope: fix(api): exclude deactivated users at the root via EF global query filter — adds HasQueryFilter(u => !u.IsDeactivated) on User, plus IgnoreQueryFilters() escape hatches for login/admin/webhook/deletion paths.
Summary
The core change (global EF query filter + FindOneTrackedIgnoringFiltersAsync/AnyIgnoringFiltersAsync escape hatches) is architecturally sound and well-tested for every call site the diff actually touches. However, the PR misses one call site that needed the same fix: OAuthController.cs (untouched by this diff) contains a third, independent Google-login implementation with the identical "look up by email → reactivate if deactivated" shape as GoogleAuthCommand.cs/VerifyCodeCommand.cs, but it was never rewired to the new IgnoringFilters methods. This turns a previously-working reactivation flow into an unhandled crash.
Findings
🔴 Critical
/oauth/google crashes instead of reactivating a deactivated user
- Location:
src/Orbit.Api/Controllers/OAuthController.cs:226,243,260 - Issue:
FindOrCreateGoogleUserAsync(line 243) looks up the user by email viauserRepository.FindOneTrackedAsync(...)— the plain, filtered variant, not the newFindOneTrackedIgnoringFiltersAsync. With the new global!u.IsDeactivatedfilter, a deactivated user's row resolves tonullhere even though it still exists (theEmailunique index is a DB-level constraint, unaffected by the LINQ-level filter). The code then falls through toUser.Create(name, email)+AddAsync+SaveChangesAsync, which throwsDbUpdateException(Postgres 23505 on the uniqueEmailindex). Thecatchblock's race-retry lookup at line 260 uses the same filteredFindOneTrackedAsync, also returnsnull, andif (raced is null) throw;re-throws the original exception. The reactivation block at line 226 (if (user.IsDeactivated) { ... tracked = FindOneTrackedAsync(...); tracked?.CancelDeactivation(); }) has the identical bug and is now unreachable in this scenario. - Risk: A user who self-deactivated (7-day grace window before deletion) and later reconnects via
/oauth/googlegets an unhandled 500 instead of a successful reactivation — a direct regression of behavior that worked before this PR, on the exact "login reactivation" class of path the PR's own scope targets. This is also untested:OAuthControllerTests.csmocksFindOneTrackedAsyncdirectly (not the real EF filter), so the existing test suite gives false confidence and would not catch this. - Fix: Mirror
GoogleAuthCommand.cs's pattern inOAuthController.cs: change lines 226, 243, and 260 touserRepository.FindOneTrackedIgnoringFiltersAsync(...), and add a reactivation-path test analogous toUserDeactivationQueryFilterTests.cs(or refactor this controller's duplicate logic to reuseGoogleAuthCommand's handler, eliminating the triplication that caused this miss). - Verdict: CONFIRMED — verified directly:
OAuthController.csis absent fromgit diff a3d8851..d01af96 --stat,FindOneTrackedAsynchas no filter-bypass, theEmailunique index is a separate DB constraint, and no earlier gate intercepts[AllowAnonymous]/oauth/google.
High / Medium / Low
None concretely actionable beyond the Critical above. The rest of the diff (repository additions, AccountDeletionService.cs, AdminAuthorizationHandler.cs, the Stripe/Play reconciliation call sites, and the new test suite) is correct and consistent with the stated intent.
What's good
- The global query filter is the right architectural fix for the underlying data-leak cluster — replacing scattered ad-hoc
!u.IsDeactivatedchecks with one enforced-at-the-root filter, consistent with the pattern already used forIsDeletedelsewhere inOrbitDbContext.cs. - The new
FindOneTrackedIgnoringFiltersAsync/AnyIgnoringFiltersAsyncrepository methods are well-documented and consistently applied everywhere the diff touches. UserDeactivationQueryFilterTests.csis a strong, targeted persistence-layer test suite coveringFindAsync,GetByIdAsync,AnyAsync/CountAsync, and the two new ignoring-filter methods, plus the rawIgnoreQueryFilters()deletion-job query.AccountDeletionService.cs's switch fromFindAsync(id)to an explicit.IgnoreQueryFilters().FirstOrDefaultAsync(...)correctly avoidsDbSet.FindAsync's undocumented interaction with query filters.
Not verifiable in CI
- Cross-repo (
orbit-ui-mobile) contract-alignment: sibling repo not checked out; not applicable anyway since no DTO/route surface changed. - dotnet build/test: covered by this PR's own required CI checks.
Recommendation
Fix OAuthController.cs's three FindOneTrackedAsync call sites (lines 226, 243, 260) to use FindOneTrackedIgnoringFiltersAsync, matching GoogleAuthCommand.cs. Once fixed, this is a clean, well-tested PR.
…global-query-filter
… regressions The global User query filter (!IsDeactivated) rewired several handlers to the IgnoringFilters repository variants but left three gaps that failed CI: - Update the auth/subscription handler test mocks (Google/VerifyCode auth, Stripe webhook, Play notification, Play purchase) to mock the FindOneTrackedIgnoringFiltersAsync/AnyIgnoringFiltersAsync variants the handlers now call, fixing the 28 failing Unit Tests (and the SonarCloud Analysis job that died on them). - OAuthController: switch the Google-login lookup, reactivation, and unique- race paths to FindOneTrackedIgnoringFiltersAsync so a deactivated user reactivates instead of hitting a 500 (review Critical). - AgentPolicyEvaluator and AgentStepUpService load the user with IgnoreQueryFilters, so a deactivated account still reaches the documented auth/deletion flows instead of a spurious user_not_found (dead account_deactivated branch otherwise). Add OAuth reactivation, agent-policy deactivation (both branches), and step-up deactivated-user coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the review + fixed CI: Review Critical ( Unit Tests (28 failures) + SonarCloud — the query-filter fix rewired the auth/subscription handlers to Two regressions the filter introduced beyond the diff — Full suite green locally: 4662 passed / 0 failed. |
|
There was a problem hiding this comment.
Code Review: PR #324
Scope: fix(api): exclude deactivated users at the root via EF global query filter — adds HasQueryFilter(u => !u.IsDeactivated) on User plus narrowly-scoped IgnoreQueryFilters() escape hatches for login/admin/webhook/deletion paths.
Recommendation: APPROVE
Update since my last review
My prior review (CHANGES_REQUESTED) flagged a Critical: OAuthController.cs used the filtered FindOneTrackedAsync at lines 226/243/260, which would crash reactivation of a deactivated user hitting /oauth/google instead of restoring their account. This is now fixed — all three call sites use FindOneTrackedIgnoringFiltersAsync, matching the pattern already used in GoogleAuthCommand.cs/VerifyCodeCommand.cs. Verified directly against the current file contents.
| Severity | Count |
|---|---|
| Critical (incl. old-client breaks) | 0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 1 |
Summary
This PR closes a deactivated-user data-leak cluster by adding an EF Core global query filter (!u.IsDeactivated) on User (OrbitDbContext.cs:666), so every ordinary query now excludes deactivated/pending-deletion accounts, restoring visibility only where required — login/reactivation (OAuthController.cs, GoogleAuthCommand.cs, VerifyCodeCommand.cs), the account-deletion background job (AccountDeletionService.cs), admin authorization (AdminAuthorizationHandler.cs), Stripe/Play webhook reconciliation (HandleWebhookCommand.cs, HandlePlayNotificationCommand.cs), the purchase-token uniqueness guard (VerifyPlayPurchaseCommand.cs), and Astra agent policy/step-up (AgentPolicyEvaluator.cs, AgentStepUpService.cs) — via two new, narrowly-named repository escape hatches (FindOneTrackedIgnoringFiltersAsync, AnyIgnoringFiltersAsync) plus direct .IgnoreQueryFilters() calls. Every escape-hatch call site is keyed by a caller-owned or server-verified identifier (JWT userId claim, Google/OTP-verified email, Stripe/Play signature-verified identifiers) — none widen to an attacker-controlled arbitrary lookup. No DTO/route/packages/shared change ships, so no contract or backward-compat surface is touched.
Findings
Critical / High
None.
Medium
None concretely actionable.
Low / Info
- Info ·
src/Orbit.Api/Controllers/OAuthController.cs:226·GoogleAuth's reactivation path re-fetches the user viaFindOneTrackedIgnoringFiltersAsyncinside theConcurrencyRetry.SaveWithRetryAsyncclosure even thoughuseris already a tracked instance from the sameDbContext. Not a defect — required byConcurrencyRetry's contract (ResetTracking()runs between retry attempts, so the mutate callback must reload rather than close over an earlier reference; seeConcurrencyRetry.cs:22-32). Noted for completeness only. - Pre-existing, not introduced by this PR:
AdminAuthorizationHandler.cs:27'sAnyIgnoringFiltersAsync(user => user.Id == userId && user.IsAdmin)lets a deactivated admin's JWT still pass the admin check. Behavior is unchanged from before this PR (there was no deactivation filter at all previously). Worth a follow-up ticket to decide whether deactivated admins should lose admin-panel access, but out of scope here.
What's good
- Root-cause fix at the correct layer — one
HasQueryFilterinstead of patching every friend/feed/challenge/marketing query individually. - New
UserDeactivationQueryFilterTests.csexercises the filter and every escape hatch against a realOrbitDbContext(in-memory), including the sharpAccountDeletionServiceedge case. - Escape hatches are narrowly named with XML docs explaining exactly why and when to use them.
- Admin authorization and reactivation-on-login behavior is deliberately preserved rather than silently changed by the new default filter.
VerifyPlayPurchaseCommand.cs's account-mismatch guard now also fails closed against a deactivated user squatting on a purchase token — a correctness improvement, not a regression.
Not verifiable in CI
- Cross-repo (
orbit-ui-mobile) contract-alignment: sibling repo not checked out; not applicable regardless since no DTO/route surface changed. - dotnet build/test: covered by this PR's own required CI checks (Build / Unit Tests / SonarCloud).
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | PASS — 9 IgnoreQueryFilters/escape-hatch call sites reviewed; all keyed to caller/server-owned identifiers; no missing [Authorize], no webhook signature regression, no new IDOR surface |
| contract-aligner | N/A — no DTO, Controller route, or packages/shared surface touched |
Deactivate() left the encrypted Google access + refresh tokens (and GoogleCalendarAutoSyncEnabled) populated on the deactivated row for the 7-day deletion grace, keeping a refreshable OAuth secret alive on an account otherwise hidden by the #324 global query filter. Null the tokens and disable auto-sync at the deactivation transition, mirroring the existing MarkCalendarSyncReconnectRequired/ResetAccount convention; reactivation re-establishes them via SetGoogleTokens when the client re-grants calendar access. FriendGraphService.ResolveTargetAsync already excludes deactivated users (it uses the filtered FindAsync, not IgnoreQueryFilters) - added a regression test locking that behaviour rather than a source change. Refs thomasluizon/orbit-ui-mobile#243 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
GetFriendsQuery and GetCheersQuery materialized a user's full friendship/cheer history, then filtered blocked users and capped in memory. Add ISocialGraphReader (Infrastructure) so the blocked-user anti-join, ordering, lookback window, and row cap run server-side. - GetFriends: blocked exclusion moves into an EF anti-join (either direction); rows are ordered accepted-first, newest-first and capped at MaxFriends (was unbounded). Deactivated counterparties still drop via the #324 User query filter on the display-name resolve. - GetCheers: adds a 90-day lookback and a 200-row cap, newest-first, with blocked senders/recipients excluded in the query. Behavior-preserving for realistic data (accepted friends are hard-capped at 500; the cheers list is a bounded feed) and the response DTOs are unchanged, so there is no consumer contract change. Reader query composition is exposed as internal statics and unit-tested via LINQ-to-objects at the filtering/ordering/pagination boundaries. Refs thomasluizon/orbit-ui-mobile#243 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Closes the deactivated-user data-leak cluster (1 Blocker + 5 High, same root cause) by adding a global EF query filter
!u.IsDeactivatedon the User entity, then restoring the paths that must still see them via explicit.IgnoreQueryFilters()(login reactivation, account-deletion job, admin authz, Stripe/Play webhook reconciliation + purchase-token guard).Verified in EF Core 10:
Find/GetByIdAsynchonors the filter, so current-user paths resolve a deactivated caller as UserNotFound until they reactivate by logging in (intended). No DTO/endpoint contract change, so no client-side edits.Tests: new persistence-layer suite proves the filter excludes deactivated users from the friend/feed/challenge/marketing query shapes and from public-profile-by-slug, and that the ignore-filter escape hatches (login, deletion job, admin/token guards) still resolve them; two existing real-context tests updated for the new behavior.
Refs thomasluizon/orbit-ui-mobile#243