fix(oauth): purge v2 consent rows on delete, log dual-write failures - #20972
fix(oauth): purge v2 consent rows on delete, log dual-write failures#20972nshirley wants to merge 1 commit into
Conversation
| } | ||
| } catch { | ||
| // v2 is best-effort; v1 already landed. Intentionally swallowed. | ||
| } catch (err) { |
There was a problem hiding this comment.
Thankfully, once the v2 table is backfilled and fully working in prod we can come back and rip out a lot of the code complexity here. This is just logging while v1 and v2 are running so we can make sure it's working as expected
There was a problem hiding this comment.
Is there a cleanup ticket that we can reference so this isn't missed? Is there a ticket we can't reference to make sure this gets cleaned up when no longer needed?
There was a problem hiding this comment.
Pull request overview
This PR hardens the OAuth account-consent v2 migration by ensuring account deletion purges the v2 mirror table and by adding logging so dual-write problems (missing scopes / v2 write failures) are observable during the bake period.
Changes:
- Delete
accountAuthorizations_v2rows during_deleteAllAccountConsentsForUser(v2-first, then v1). - Add warning/error logs for v2 dual-write missing-scope resolution and v2 write failures (without logging full DB errors).
- Update/add integration tests to reflect the new “delete clears both tables” behavior and to assert v2 rows are removed on deletion.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/fxa-auth-server/lib/oauth/db/mysql/index.js | Adds v2 consent purge on delete and logs dual-write missing scopes / write failures. |
| packages/fxa-auth-server/test/remote/account_consents.in.spec.ts | Updates an existing v2-read test to delete v1 directly, and adds a test ensuring deletion clears v2 rows. |
| this.log?.warn('accountAuthorizations.v2.missing_scope', { | ||
| scopes: missing.slice(0, 10), | ||
| count: missing.length, | ||
| }); |
vpomerleau
left a comment
vpomerleau
left a comment
There was a problem hiding this comment.
A few small comments, take them or leave them!
| } | ||
| } catch { | ||
| // v2 is best-effort; v1 already landed. Intentionally swallowed. | ||
| } catch (err) { |
There was a problem hiding this comment.
Is there a cleanup ticket that we can reference so this isn't missed? Is there a ticket we can't reference to make sure this gets cleaned up when no longer needed?
| // Drop the v1 rows only; the v2 row remains, so a v2 read is the only | ||
| // thing that can still find this consent. | ||
| await db.deleteAllConsentsForUser(id); | ||
| // thing that can still find this consent. Deletes both tables now, so this |
There was a problem hiding this comment.
No sure I understand the comment here - this seems to explain why we aren't using deleteAllConsentsForUser without naming it?
| expect(await db.hasConsentForSignIn(id, UNKNOWN_SCOPE, '')).toBe(true); | ||
| }); | ||
|
|
||
| it('account deletion clears the v2 row, not just v1', async () => { |
There was a problem hiding this comment.
nit: should this be account deletion clears both v1 and v2 rows, + add a check for v1 (maybe it's there but not as clearly asserted as v2 checks)
Because
accountAuthorizationsbut left theaccountAuthorizations_v2mirror behind, so consent records for deleted accounts persist indefinitely. Once v1 is dropped in Phase 4, those orphans become deleted users' consent living in the only remaining table.This pull request
DELETE FROM accountAuthorizations_v2to_deleteAllAccountConsentsForUserinpackages/fxa-auth-server/lib/oauth/db/mysql/index.js. It is deliberately not gated ondualWriteV2, since turning that flag off stops new v2 writes but leaves existing rows needing purged, and it runs before the v1 delete so a partial failure can't leavereadV2answering for an account whose v1 rows are already gone.accountAuthorizations.v2.missing_scope(warn) when the scope cache can't resolve a scope, carrying the unresolved strings as a seed list for thescopestable. The array is capped because scope strings are caller-supplied.accountAuthorizations.v2.write_failed(error) when the v2 mirror throws. The write is still swallowed so it can never fail a grant. Only the drivercode/errnois logged, not the error, since the mysql driver decorates errors with connection options that can include credentials.deleteAllConsentsForUserdropping v1 only.Issue that this pull request solves
Closes: N/A
Checklist
Put an
xin the boxes that applyHow to review (Optional)
Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Integration tests were not run locally — the local MySQL container is missing the
fxa_oauthschema, so every suite touching it fails at connection time. Lint passes; relying on CI to runtest/remote/account_consents.in.spec.ts.