Skip to content

feat(identity): per-player auto-logout window and 2FA on every login - #158

Open
jakubfilinger-b wants to merge 2 commits into
devfrom
feat/player-remember-device
Open

feat(identity): per-player auto-logout window and 2FA on every login#158
jakubfilinger-b wants to merge 2 commits into
devfrom
feat/player-remember-device

Conversation

@jakubfilinger-b

@jakubfilinger-b jakubfilinger-b commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Backs BF-522 (Betfeel "Remember Device" settings). Adds the two per-player security preferences that page needs, plus the enforcement behind them.

autoLogoutDuration

How long a session may sit idle before it is cut: 15m / 1h / 24h / 7d / 30d. There is deliberately no "off" - the longest option already coincides with better-auth's absolute 30-day expiry, which this PR does not touch, so a "never" would have been indistinguishable from 30d while reading as a real escape hatch. Defaults to 7d.

Enforcement goes through a new SESSION_IDLE_POLICY port, bound by identity and resolved once per authenticated request in the create-app middleware, right beside the existing PLAYER_ACTIVITY_TRACKER call. It stamps session.lastSeenAt and expires a session that has outrun the window, using the same shape an explicit revoke does (expiresAt = now(), updatedAt left alone so the device list still shows real last-use). An expired session returns expired, and the middleware then serves the request on the unauthenticated path rather than publishing a session it has just killed.

Two details worth a reviewer's eye:

  • The lastSeenAt write is throttled to 60s, deliberately tighter than the admin guard's 5 minutes. There the column is a display value; here it is the input to the idle comparison, and a 5-minute lag would stretch the shortest window by a third before it bit.
  • A session whose lastSeenAt is null (it predates the column) is stamped and left alive rather than read as "idle since the beginning of time". That is what keeps existing rows from being cut on deploy day as they adopt the 7-day default.

requireTwoFactorOnLogin

When set, a trusted device buys nothing. Honoured in three places: login strips the trust cookie without consulting the trusted-device rows, verifyTwoFactor refuses to grant trust, and trustCurrentDevice rejects outright rather than handing back a device that would still be challenged on the next login. Turning the preference on revokes the devices already trusted, so the player is not shown grants that no longer mean anything.

Turning it on is refused when the account has no second factor enrolled, mirroring how setLoginWithdrawalAlerts refuses on an unverified email - otherwise the toggle is a no-op that reads as protection.

Shared

Both setters mirror setLoginWithdrawalAlerts, including its player-only guard - extracted into assertPlayerPreferenceCaller now that three setters share it, with the audit denial signal preserved verbatim - and each carries its own audit event so the trail says which preference moved rather than just "a security preference did".

Tests

New session-idle.service.int.test.ts (8 cases: the 7-day default applied and respected, over-window expiry, under-window survival, last-used preservation, first-activity stamping, the system-attributed revoke event, throttle) and a login case asserting a live trusted device is still challenged while the preference holds.

Note for the downstream ticket

BF-522's acceptance criteria and its Confluence scenario both list "Never" as a selectable option. It was dropped on the operator's call after the reasoning above; the deviation is recorded on the Betfeel MR.

Two per-player security preferences the Security page needs, both stored on
`user` and read back through `security.me`:

- `autoLogoutDuration` - how long a session may sit idle before it is cut.
  Enforced by a new SESSION_IDLE_POLICY port, bound by identity and resolved
  by the request middleware once per authenticated request: it stamps
  `session.lastSeenAt` (throttled to a minute, tighter than the admin guard's
  five, because the value is the input to the comparison here) and expires a
  session that has outrun the window, in the same shape an explicit revoke
  uses. Defaults to `never`, so existing sessions behave exactly as before
  until a player chooses otherwise.

- `requireTwoFactorOnLogin` - when set, the trust cookie buys nothing: login
  strips it without consulting the trusted-device rows, verify2fa refuses to
  grant trust, and trustCurrentDevice rejects outright rather than handing
  back a device that would still be challenged. Turning it on revokes the
  devices already trusted.

Both setters mirror setLoginWithdrawalAlerts, including the player-only guard
- extracted now that three of them share it - and carry their own audit event
so the trail says which preference moved.
Every account now carries an idle window: `15m` / `1h` / `24h` / `7d` / `30d`,
with no way to switch the check off. The longest option already coincides with
better-auth's absolute 30-day expiry, so "never" was indistinguishable from
"30d" in practice while reading as a real escape hatch.

`AUTO_LOGOUT_MINUTES` loses its null case and the idle comparison loses a
branch with it. The column now defaults to `7d`; existing rows are not cut on
deploy, because a session with no activity on record yet is stamped rather
than judged.

Migration 0014 is regenerated rather than followed by a second one - it has
not shipped, so replacing it keeps the enum's history to a single statement.

@jakubfilinger-b jakubfilinger-b left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Self-review pass before asking anyone else to read this — GitHub won't take an approval or a changes-request from the author, so these are notes on my own branch rather than a verdict.

Four things I want to fix or justify before this goes out for review. In rough order of how much they change the design:

  1. An idled-out tab is never told. expire emits identity.session.revoked, but streamSession only subscribes to identity.sessions.revoked_all — so the session dies in the database and the open tab keeps rendering a logged-in UI until someone clicks. For an inactivity timeout that is the defining case: the player walked away, and "idle" means the next request is not coming. Thread below.
  2. The enforcement is universal but the control is player-only. Thread below.
  3. touch is awaited unguarded on the request path, where its immediate neighbour is fire-and-forget. Thread below.
  4. An extra read per authenticated request that could probably be folded into the tracker's.

On the AC deviation already recorded downstream (no "Never" option): the reasoning still holds — the longest window coincides with better-auth's absolute 30-day expiry, so "Never" would have read as an escape hatch that does not exist. Leaving it as documented rather than reopening it.

.where(eq(session.id, sessionId));

// No `actorId`: nobody revoked this, the player's own inactivity window did.
this.events.emit('identity.session.revoked', {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is the gap that matters most. streamSession (router/index.ts:145) subscribes to identity.sessions.revoked_all only, so nothing pushes on identity.session.revoked — the tab that has been sitting idle gets no signal at all. The session is expired in the database, the audit row is written, and the browser carries on showing an authenticated UI until the next request happens to fall through to the unauthenticated path.

For an inactivity timeout that inverts the feature. The scenario it exists for is a player who walked away from a shared machine; "the next request kicks them" is exactly the request that never arrives, so the screen stays logged in for as long as the tab is open.

The gap is pre-existing — a single-session revoke from the device list has the same silence — but this is the feature that makes it load-bearing, so it should be closed here. streamSession should also subscribe to identity.session.revoked and push { type: 'revoked' } when the event's sessionId matches the connection's own. The change-password work already established the pattern of comparing against the id the SSE connection captured at open, so the filter has a precedent to copy.

Worth an e2e alongside it: open a stream, expire that session through touch, assert the connection receives revoked.

@@ -0,0 +1,3 @@
CREATE TYPE "public"."auto_logout_duration" AS ENUM('15m', '1h', '24h', '7d', '30d');--> statement-breakpoint
ALTER TABLE "user" ADD COLUMN "auto_logout_duration" "auto_logout_duration" DEFAULT '7d' NOT NULL;--> statement-breakpoint

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

auto_logout_duration lands on user with DEFAULT '7d' NOT NULL, which means every row gets it — admin accounts included — and the middleware at create-app.ts:472 calls touch for every authenticated session without asking whether it is a player's.

So this PR, titled "per-player auto-logout", also ships a seven-day idle logout for backoffice admins. The window is mild and arguably a good thing, but it is a behaviour change on a surface the description says is untouched, and it is asymmetric in a way that will confuse someone later: the setter is behind assertPlayerPreferenceCaller, so an admin is pinned to 7d for ever with no route and no UI to change it. Enforcement everywhere, control for players only.

Two honest options. Either scope the enforcement to players (skip touch when the resolved session is not a player's, and say so in the port doc), or own it deliberately — state in the description and the changeset that admin sessions now idle out at seven days, and file the follow-up that gives operators a knob. I lean towards the first for this PR, since admins already have their own lastSeenAt tracking through AdminGuard and this adds a second mechanism doing a similar job.

// published onto the context, so an idled-out request falls through to the
// unauthenticated path above rather than being served and 401ing somewhere deeper.
if (sessionId && container.has(SESSION_IDLE_POLICY)) {
const state = await container.get(SESSION_IDLE_POLICY).touch(userId, sessionId);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This await is unguarded, and the PLAYER_ACTIVITY_TRACKER call twelve lines down is deliberately not: it is fire-and-forget with a .catch that logs. So a transient database error inside touch now turns every authenticated request into a 500, where the same error in its neighbour is a log line.

Failing closed on a security control is the right instinct and I do not want to swap it for the tracker's swallow-and-continue. But a 500 is the wrong shape of failing closed: it tells the client the server is broken rather than that they are not signed in, and it takes down every authenticated route on a blip in one query. Falling through to the unauthenticated path — the same branch an expired result already takes — fails closed and degrades honestly.

Whichever we pick, it needs a comment saying it was a choice, because the contrast with the line below reads as an oversight.

}

async touch(userId: User['id'], sessionId: Session['id']): Promise<'active' | 'expired'> {
const [row] = await this.drizzle.db

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Two smaller things on this read.

The joined select runs on every authenticated request — the throttle only spares the write. Right below it in the middleware, PLAYER_ACTIVITY_TRACKER.touchLastSeen goes to the same user/session rows for a closely related purpose. That is two round trips per request where the data overlaps almost entirely. Worth checking whether the idle read can return what the tracker needs (or the reverse) before this ships, because it is much harder to merge them once both are established on the hot path.

And AUTO_LOGOUT_MINUTES[row.autoLogoutDuration] at :52 has no fallback. Today the column enum and the map come from the same contract constant so they cannot disagree — but if they ever do, windowMs is NaN, idleForMs > NaN is false, and the session simply never expires. A security control whose failure mode is silent permanence deserves a default and a log rather than trusting the two definitions to stay in step.

.innerJoin(user, eq(user.id, session.userId))
.where(and(eq(session.id, sessionId), eq(session.userId, userId)))
.limit(1);
if (!row) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

return 'active' when the row is missing is failing open, and it is the one branch here with no comment explaining itself. It is reachable: the session can be revoked between better-auth resolving it and this query running, and the caller then publishes the session onto the context and serves the request as authenticated.

Narrow window, and better-auth has already validated the session, so I do not think it is a real hole — but every other decision in this file is annotated with why, and the branch that hands back active without evidence is the one that most needs it. Either return 'expired' here (the caller already handles it, and "no such session" is not a worse answer than "idle") or write down why active is safe.

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.

2 participants