Skip to content

feat(auth-server): add accountActivity table for per-client liveness#20620

Draft
nshirley wants to merge 1 commit into
mainfrom
FXA-13662
Draft

feat(auth-server): add accountActivity table for per-client liveness#20620
nshirley wants to merge 1 commit into
mainfrom
FXA-13662

Conversation

@nshirley
Copy link
Copy Markdown
Contributor

@nshirley nshirley commented May 18, 2026

Because:

  • FxA lacks a durable per-(userId, clientId, service) activity signal needed for inactive-account deletion, per-RP liveness, and support tooling.
  • Existing token tables either disappear on revoke, miss Firefox Desktop's online fxa-credentials service-token mints, or are sampled and Redis-biased and therefore under-detect active users.

This commit:

  • Adds a new accountActivity table to the fxa_oauth schema, keyed by (userId, clientId, service) with secondary indexes for time-window and per-service cohort queries.
  • Adds a fire-and-forget recordActivity helper that resolves the service tag from the requested scopes (scopeToServiceName) with a clientId fallback (oauthServer.clientIdToServiceNames), then writes via a SQL-throttled upsert. The throttle predicate uses VALUES(x) > x + ? to avoid BIGINT UNSIGNED underflow on out-of-order timestamps.
  • Wires a single write site at the OAuth grant convergence point in tokenHandler so all four grant types (authorization_code, fxa-credentials, refresh_token, token-exchange) are recorded uniformly.
  • Extends _removeTokensAndCodes so accountActivity rows are cleaned up alongside tokens on account deletion.
  • Stores scopeToServiceName as a JSON string
  • Emits accountActivity.recorded / accountActivity.write_failed StatsD metrics with service + grantType tags. Unclassified ('') rows trigger a deduped log.warn once per process per clientId.

Closes FXA-13662

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).
  • I have manually reviewed all AI generated code.

How to review (Optional)

  • Key files/areas to focus on:
  • Suggested review order:
  • Risky or complex parts:

Screenshots (Optional)

Here's a screenshot of the table after I signed in/up through 123Done, then signed in with scopeKeys
image

Other information (Optional)

Any other information that is important to this pull request.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pr that updates accountAuthorizations has several migrations so this is going to hit conflicts 🥴

Comment thread packages/fxa-auth-server/lib/oauth/account-activity.ts Outdated
Comment thread packages/fxa-auth-server/lib/oauth/account-activity.ts Outdated
Comment thread packages/fxa-auth-server/lib/oauth/account-activity.ts Outdated
Comment thread packages/fxa-auth-server/lib/oauth/account-activity.ts Outdated
Comment thread packages/fxa-auth-server/lib/oauth/account-activity.ts
Because:
- FxA lacks a durable per-(userId, clientId, service) activity signal
  needed for inactive-account deletion, per-RP liveness, and support
  tooling.
- Existing token tables either disappear on revoke, miss Firefox
  Desktop's online fxa-credentials service-token mints, or are sampled
  and Redis-biased and therefore under-detect active users.

This commit:
- Adds a new accountActivity table to the fxa_oauth schema (patch
  33->34), keyed by (userId, clientId, service) with secondary indexes
  for time-window and per-service cohort queries.
- Adds a fire-and-forget recordActivity helper that resolves the
  service tag from the requested scopes (scopeToServiceName) with a
  clientId fallback (oauthServer.clientIdToServiceNames), then writes
  via a SQL-throttled upsert. The throttle predicate uses
  `VALUES(x) > x + ?` to avoid BIGINT UNSIGNED underflow on
  out-of-order timestamps.
- Wires a single write site at the OAuth grant convergence point in
  tokenHandler so all four grant types (authorization_code,
  fxa-credentials, refresh_token, token-exchange) are recorded
  uniformly.
- Extends _removeTokensAndCodes so accountActivity rows are cleaned up
  alongside tokens on account deletion.
- Stores scopeToServiceName as a JSON string in convict to work around
  mozilla/node-convict#250 (dotted/URL keys would otherwise be split
  as nested config paths during validation).
- Emits accountActivity.recorded / accountActivity.write_failed StatsD
  metrics with service + grantType tags. Unclassified ('') rows
  trigger a deduped log.warn once per process per clientId.

Closes FXA-13662
throttleMs
);
statsd?.increment('accountActivity.recorded', metricTags);
if (service === '' && !warnedUnclassifiedClientIds.has(clientIdHex)) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the surface this makes sense to record an empty unclassified service scopes as an empty string '', BUT, I also wonder if there is risk in doing this and we should just drop the empty values. Rather, if there are unclassified scopes, we scrub them from being written at all AND log some kind of error to sentry with the "wrong" scope, and/or a statsd metric to track. It means we'd miss a 'sign in' potentially, but by recording an empty scope we're also missing what they signed into anyways 🤷

statsd?.increment('accountActivity.recorded', metricTags);
if (service === '' && !warnedUnclassifiedClientIds.has(clientIdHex)) {
warnedUnclassifiedClientIds.add(clientIdHex);
log?.warn('accountActivity.unclassified', {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the log here should be a Sentry alert instead? If we have a mismatching scopeToServiceName or clientidToServicenames then we need to know that and logs aren't great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant