Transfer or release a device's push subscription when it changes accounts (#932)

[thomasluizon]

Fixes thomasluizon/orbit-tickets#932.

Problem

A browser keeps one push endpoint, and an Android install keeps one FCM token, across the accounts that sign in on it. When a second account registered that endpoint, SubscribePushCommand returned PUSH_ENDPOINT_OWNED_BY_OTHER_USER. The client then rotated the endpoint and registered the new one, and the first account kept a row for a device that no longer received its pushes. UnsubscribePushCommand removed only the caller's own row, so the new account could not release it either. Each switch added one dead row to the first account's device list and moved it toward the five-device cap.

Change

• PushSubscription.MatchesCredentials(p256dh, auth) decides whether a request proves control of the device. A Web Push row needs the stored p256dh key and auth secret, compared in fixed time. An FCM row needs the FCM sentinel, because the caller already matched the row by its token.
• PushSubscription.TransferTo(userId) moves the row to another account and gives it a fresh CreatedAtUtc.
• SubscribePushCommand: when the endpoint belongs to another account and the request proves control, the row moves to the caller. The caller's oldest rows beyond the cap are evicted, never the moved row. Without proof, the command still returns PUSH_ENDPOINT_OWNED_BY_OTHER_USER.
• UnsubscribePushCommand now carries P256dh and Auth. The controller forwards them from the body it already requires, and the unsubscribe_push chat tool forwards them when given. The owner can release its own row as before. Another account can release the row only with the same proof. Without proof the call changes nothing and still returns 200, so it does not reveal who owns an endpoint.

Repeated switches now move one row between accounts, so neither account grows toward the cap. The request and response shapes do not change, so orbit-ui-mobile needs no contract change. Its existing PUSH_ENDPOINT_OWNED_BY_OTHER_USER recovery still applies to a request without proof.

Test evidence

Part 1, subscribe transfer.

1. Existing test, unchanged, defect present. dotnet test tests/Orbit.Application.Tests --no-build --filter "FullyQualifiedName~SubscribePushCommandHandlerTests.Handle_ExistingEndpointDifferentUser_RejectsToPreventHijack" passed (1 of 1). It sends different keys, so it never exercised the same device under a second account.
2. New tests, defect present:

$ dotnet test tests/Orbit.Application.Tests --no-build --filter "FullyQualifiedName~SubscribePushCommandHandlerTests"
Failed Handle_SameAndroidDeviceUnderNewAccount_TransfersTheRowToTheNewAccount
  Expected result.IsSuccess to be True, but found False.
Failed Handle_TransferIntoAccountAtCap_EvictsItsOldestAndKeepsTheTransferredRow
  Expected result.IsSuccess to be True, but found False.
Failed Handle_SameBrowserUnderNewAccount_TransfersTheRowToTheNewAccount
  Expected result.IsSuccess to be True, but found False.
Failed!  - Failed: 3, Passed: 15, Total: 18
$ dotnet test tests/Orbit.Infrastructure.Tests --no-build --filter "FullyQualifiedName~PushSubscriptionAccountSwitchTests"
Failed SameAndroidDeviceSignsInToAnotherAccount_OldAccountNoLongerListsIt
  Expected result.IsSuccess to be True, but found False.
Failed SameBrowserSignsInToAnotherAccount_OldAccountNoLongerListsIt
  Expected result.IsSuccess to be True, but found False.
Failed RepeatedAccountSwitchesOnOneDevice_NeverGrowEitherAccountTowardTheCap
  Expected (Subscribe(account, WebEndpoint, WebP256dh, WebAuth)).IsSuccess to be True, but found False.
Failed!  - Failed: 3, Passed: 1, Total: 4

3. After the fix, the same commands pass: 18 of 18 and 4 of 4. PushSubscriptionTests in Orbit.Domain.Tests passes 18 of 18.
   Part 2, unsubscribe release.
4. Existing tests, unchanged, defect present. dotnet test tests/Orbit.Application.Tests --no-build --filter "FullyQualifiedName~UnsubscribePushCommandHandlerTests" passed (2 of 2).
5. New tests, with P256dh and Auth added to the command and the handler unchanged:

$ dotnet test tests/Orbit.Infrastructure.Tests --no-build --filter "FullyQualifiedName~PushSubscriptionAccountSwitchTests"
Failed NewAccountTurnsPushOffOnTheBrowser_OldAccountNoLongerListsIt
  Expected CountFor(_firstAccount) to be 0, but found 1 (difference of 1).
Failed NewAccountSignsOutOfTheAndroidDevice_OldAccountNoLongerListsIt
  Expected CountFor(_firstAccount) to be 0, but found 1 (difference of 1).
Failed!  - Failed: 2, Passed: 4, Total: 6

The mocked handler tests are not the proof for this part. The mocked repository ignores the query predicate, so before the fix Handle_OtherAccountsRowWithItsDeviceKeys_RemovesAndSaves passed and both Handle_OtherAccountsRowWithoutItsDeviceKeys_LeavesItInPlace cases failed with ReceivedCallsException. That is a mock artifact. The SQLite test above runs the real predicate and shows the defect.
3. After the fix, UnsubscribePushCommandHandlerTests passes 6 of 6 and PushSubscriptionAccountSwitchTests passes 6 of 6.
Whole suite, dotnet test Orbit.slnx --no-build:

Orbit.Domain.Tests           Passed: 640,  Failed: 0
Orbit.Application.Tests      Passed: 3730, Failed: 0
Orbit.Analyzers.Tests        Passed: 32,   Failed: 0
Orbit.Infrastructure.Tests   Passed: 2606, Failed: 0

node tools/check-suppression-allowlist.mjs, node tools/check-timeless.mjs --base origin/main, node tools/check-dashes.mjs --files <changed files>, node tools/check-root-allowlist.mjs and node tools/arch-map.mjs exit 0. dotnet format Orbit.slnx --verify-no-changes on the changed files exits 0.

• Before changes: dotnet test tests/Orbit.Infrastructure.Tests --filter FullyQualifiedName~PushSubscriptionAccountSwitchTests passed all six existing tests with the defect present.
• Before fixing the handler: dotnet test tests/Orbit.Infrastructure.Tests --filter FullyQualifiedName~PreviousAccountsDelayedUnsubscribe failed both web and Android cases: B’s expected count was 1, actual 0.
• After fixing: infrastructure focused tests passed 43/43, including both regressions, using dotnet test tests/Orbit.Infrastructure.Tests --filter 'FullyQualifiedName~PushSubscriptionAccountSwitchTests|FullyQualifiedName~NotificationControllerTests|FullyQualifiedName~NotificationToolsTests' --no-restore.
• Application focused tests passed 76/76 using dotnet test tests/Orbit.Application.Tests --filter 'FullyQualifiedName~NotificationPushCommand|FullyQualifiedName~UnsubscribePushCommand|FullyQualifiedName~ProfileNotificationCalendarToolTests' --no-restore.
• dotnet build Orbit.slnx: zero errors.
• After commit, dotnet test: 7,022 passed, zero failures.
• Architecture generator, dash, timeless, and suppression checks passed.
• Existing test passed with the defect present: 2 cases.

  dotnet test tests/Orbit.Infrastructure.Tests --filter 'FullyQualifiedName~PushSubscriptionAccountSwitchTests.PreviousAccountsDelayedUnsubscribe_AfterAnotherAccountClaimsTheDevice_LeavesTheClaimInPlace'

• Corrected regression test failed before the fix: all 4 cases expected B’s count to remain 1 but observed 0.

  dotnet test tests/Orbit.Infrastructure.Tests --filter 'FullyQualifiedName~PushSubscriptionAccountSwitchTests.UnsubscribeReadsPreviousOwner_AnotherAccountClaimsBeforeDelete_LeavesTheNewClaimInPlace'

• After the fix, all 12 account-switch cases passed, including those 4 regressions.

  dotnet test tests/Orbit.Infrastructure.Tests --filter 'FullyQualifiedName~PushSubscriptionAccountSwitchTests'

• Focused handler and validator tests: 21 passed.

  dotnet test tests/Orbit.Application.Tests --filter 'FullyQualifiedName~UnsubscribePushCommandHandlerTests|FullyQualifiedName~UnsubscribePushCommandValidatorTests'

• Both full-suite runs passed 7,026 tests each:

  env -u LANG dotnet test
  env -u LANG LC_ALL=en_US.UTF-8 dotnet test

• Formatting and root-allowlist, dash, timeless and suppression guards passed.
  EF Core 10.0.12’s installed nuspec supplied the revision used to verify immediate deletion semantics and the interceptor’s command-source mapping.

Assumptions

• Web Push proof is the stored p256dh key plus auth secret. The endpoint alone is not enough, because the push sender writes it to warning logs. Rejected alternative: accept a matching endpoint alone.
• FCM proof is the registration token itself. The native client sends the constant fcm in both key fields, the API never returns a token, and logs keep only a 20-character preview. Rejected alternative: a device-held secret for Android, which needs a client and wire change.
• A moved row gets a fresh CreatedAtUtc, so it counts as the new account's newest device and is not the first one evicted at the cap. Rejected alternative: keep the first account's registration time.
• Unsubscribe without proof for another account's row still returns 200 and changes nothing. Rejected alternative: return PUSH_ENDPOINT_OWNED_BY_OTHER_USER, which reveals that another account owns the endpoint.
• The base is main, where GET api/notifications/subscriptions does not exist yet (api#649 added it to redesign/main). Its handler counts rows by UserId, so the tests assert that row count directly. Rejected alternative: target redesign/main.
• PushSubscriptionAccountSwitchTests runs both handlers over the shared SqliteOrbitDbContextFactory with the real repository and unique endpoint index, the same pattern as IdempotencyBehaviorDbTests. Rejected alternative: mock-only coverage, which cannot show a per-account count.
• The unsubscribe_push chat tool forwards p256dh and auth, which its schema already declares. Rejected alternative: leave the tool on the endpoint alone.
• Web onboarding calls subscribeToPushNotifications, which drops the browser's endpoint before it registers, and web sign-out does not release it. Both are client work, filed as thomasluizon/orbit-tickets#994. Rejected alternative: edit a second repository in this pull request.
• Dead rows that earlier rotations left behind get no backfill. The sender already deletes a row when its push returns 404 or 410, or FCM reports the token stale. Rejected alternative: a data migration, which cannot tell a dead endpoint from a live one.
  🤖 Generated with Claude Code
• Used a dedicated UnsubscribeRequest instead of adding unsubscribe-specific fields to SubscribeRequest.
• Chose the review’s explicit opt-in mechanism instead of introducing claim tokens and a migration.
• Chose an owner-bound repository delete over a migration-backed concurrency token.
• Used tracked entries instead of Local, which excludes entities marked for deletion.

Manual steps

• Deploy through GitHub Actions → Release API → Run workflow, with environment=production and branch=main after merge. Proof: successful workflow and Render deployment.
• Ticket #994’s consumer cleanup must send releaseOtherAccount: true with device credentials to POST /api/notifications/unsubscribe; ordinary sign-out must omit it. Proof: explicit cleanup removes the prior account’s row, while delayed ordinary sign-out preserves the new claim.
• No new environment variables, secrets, dashboard settings, migrations, or backfills.
  Once the change reaches main, dispatch GitHub Actions Release API in thomasluizon/orbit-api, using workflow ref main, environment=production and branch=main. A successful workflow verifies API health and that Render serves the released commit.
  No new configuration, secrets, migration or backfill is required.

[pullfrog]

Important

The browser account-switch path still leaves the previous account's subscription behind, so this change does not resolve the reported browser behavior without a coordinated client update.

Reviewed changes in the two commits at ecb9a832 across the API, domain entity, chat tool, and unit tests:

• Registration transfer: A matching Web Push key pair or FCM token can move an existing endpoint to the signed-in account, enforcing that account's device cap.
• Subscription release: Unsubscribe forwards device credentials and can remove a previous account's row when those credentials match.
• Coverage: Handler, domain, controller, tool, and SQLite account-switch tests exercise these paths and rejection of mismatched Web Push credentials.

⚠️ Browser account switches never reach the transfer path

Both existing web registration flows unsubscribe the browser's old subscription and create a new endpoint before calling the API. The new account therefore never presents the old endpoint or its keys to ClaimExisting; the old account's row remains and still counts toward the device cap, while the web prompt skips registration entirely if it sees an existing subscription. Please coordinate the acknowledged client follow-up with this rollout rather than treating the server change alone as a fix for browser account switches.

Technical details

# Browser registration does not claim the existing row

## Affected sites
- `apps/web/hooks/use-push-notification-preferences.ts:127-145` in `orbit-ui-mobile` discards the existing subscription before posting the replacement.
- `apps/web/components/ui/push-prompt.tsx:63-67,116-133` skips a prompt for an existing subscription and otherwise rotates the endpoint before registration.
- `src/Orbit.Application/Notifications/Commands/SubscribePushCommand.cs:34-39,75-84` can transfer only when the posted endpoint matches the previously stored row.

## Required outcome
- Ensure the browser presents the existing endpoint and keys under the new account before rotating it, and that account changes trigger registration even when the browser already has a subscription. Sequence or explicitly track the dependent client rollout so the original browser device-cap and stale-owner behavior is not presented as resolved prematurely.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[thomasluizon]

Disposition of the review body finding (browser account switches never reach the transfer path): filed as thomasluizon/orbit-tickets#994. This pull request is the API half of thomasluizon/orbit-tickets#932 and does not claim to fix the browser flow on its own. #994 is the dependent web rollout: it sends the existing subscription (endpoint and keys) to unsubscribe before onboarding rotates it and on sign-out, and a scope comment added today covers the two paths this review names: the push prompt that skips or rotates an existing subscription after an account change (apps/web/components/ui/push-prompt.tsx:63-67,116-133), and the account-switch cleanup from #842, which must release the previous account's row through this release path before it unsubscribes in the browser. Android keeps its FCM token across accounts, so this API change covers it without a client change.

[thomasluizon]

@pullfrog review

[pullfrog]

Important

A delayed unsubscribe from the previous account can remove the newly claimed account's push subscription.

Reviewed changes in the two commits at ecb9a832 across the API, application, domain, and tests:

• Registration transfer: Matching browser credentials or an FCM token transfer an existing row and enforce the destination account's device cap.
• Subscription release: The HTTP endpoint and chat tool forward credentials so a different account can remove the row.
• Coverage: Handler, domain, controller, and SQLite tests exercise transfers, releases, and mismatched browser keys.

---

Note: 1 inline comment(s) dropped because they did not anchor to lines inside the PR diff:

• src/Orbit.Application/Notifications/Commands/UnsubscribePushCommand.cs:44 (RIGHT) — line 44 (RIGHT) is not inside a diff hunk

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[thomasluizon]

Disposition of the second review's finding (a delayed unsubscribe from the previous account can remove the newly claimed row): fixed in a69c7ba. An unsubscribe from an account that does not own the row now changes nothing unless it sets the new optional releaseOtherAccount: true and presents the device keys; ordinary sign-out and turn-off omit the flag, so a late request from the previous account can no longer delete the new account's claim. PreviousAccountsDelayedUnsubscribe (web and Android cases) failed before the handler change and passes after; full dotnet test 7,022 passed.

[pullfrog]

ℹ️ No new findings in this delta. The existing concurrency thread remains open because the opt-in change does not protect a delete that read the row before another account claimed it.

Reviewed changes since the prior Pullfrog review at ecb9a832:

• Explicit cross-account release: Added releaseOtherAccount to the unsubscribe request and command, defaulting ordinary sign-out to owner-only release.
• Tool forwarding: Propagated the opt-in flag and device credentials through the HTTP, MCP, and chat-tool paths.
• Regression coverage: Added legacy payload, opt-in validation, and delayed sign-out tests for Web Push and FCM.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes since the prior Pullfrog review at a69c7ba8:

• Owner-bound deletion: Replaced tracked removal with an immediate delete constrained by subscription ID and the owner observed during unsubscribe, preserving a newer account's claim if a transfer happens after the read.
• Concurrency coverage: Added SQLite interleaving tests for browser and Android subscriptions, with both ordinary sign-out and explicit cross-account release.

  ^{｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarQube Cloud