fix(authflow): evaluate max_age from per-session auth time, not global LastLogin

[SashaMIT]

Problem

trySessionLogin is the only place that enforces the OIDC max_age parameter. It compares now against ui.LastLogin, where ui is the storage.UserIdentity keyed by (UserID, ConnectorID) — a single global row per identity that finalize.go rewrites to now() on every interactive login, from any browser, device, or session.

Net effect: a fresh login on a second device satisfies an RP's max_age re-authentication demand for a stale session on the first device, because the global LastLogin was just bumped. The RP asked for proof of recent authentication for this session and got proof of some recent authentication by this user anywhere.

The correct value already exists and is already populated: storage.ClientAuthState.AuthenticatedAt (written on direct login, deliberately carried across for SSO). The same line also stamps AuthTime = ui.LastLogin, so the auth_time claim the RP receives is likewise the global value rather than this session's.

Fix

Evaluate max_age against session.ClientStates[authReq.ClientID].AuthenticatedAt (guaranteed non-nil at this point), falling back to ui.LastLogin only when the session has no client state for the client.

Tests

The existing TestTrySessionLogin_MaxAge fixture set the session's AuthenticatedAt to a fixed now-1min while expressing the login time via LastLogin; it now ties AuthenticatedAt to the same lastLogin the test models, so the scenario is consistent. Full ./server/authflow/ suite passes; go build ./server/... clean.

Made with Cursor

Made with Cursor

[nabokihms]

Thanks for this one too. The problem is real: LastLogin is one row per identity, so now that a session is one browser, a login on the phone answers an RP's max_age demand for a stale session on the laptop. Three things before it does that, though.

1. On the SSO path the new client state is written to storage but not to the session value in hand. UpdateAuthSession mutates old inside the updater, and the caller's copy is only affected on the memory backend, where the returned struct happens to share the same map. On SQL, ent, Kubernetes and etcd the updater works on a freshly deserialized session, so session.ClientStates[authReq.ClientID] is still nil two lines later and the fallback takes ui.LastLogin, which is the value this PR sets out to stop using. Tests pass because they run on memory. sourceState.AuthenticatedAt is right there in that branch and can be carried down directly. "Guaranteed non-nil here" in the comment holds for the direct-login path only.

2. auth_time keeps the global value. The description names this and the diff leaves it: finishSessionLogin still stamps a.AuthTime = ui.LastLogin. Dex would then gate on one timestamp and hand the RP another, and plenty of RPs enforce max_age themselves by reading auth_time, which OIDC requires in the ID token whenever max_age was requested. That is the half the RP actually sees.

3. The test no longer separates the two values. Setting the fixture's AuthenticatedAt to the same lastLogin the identity carries makes both candidates identical, so TestTrySessionLogin_MaxAge passes with the production change reverted. A case that holds them apart would pin the fix: session authenticated well in the past, LastLogin bumped a second ago, small max_age, expect re-authentication.

[SashaMIT]

Addressed maintainer feedback:

1. Fixed ClientStates nil after UpdateAuthSession: Added explicit synchronization of the session object after UpdateAuthSession calls, since some storage backends mutate a copy rather than the caller's object.

2. Fixed auth_time source: Changed finishSessionLogin to accept the authenticatedAt parameter and use it for AuthTime instead of falling back to UserIdentity.LastLogin. This ensures max_age validation correctly uses the per-session authentication time.

3. Updated tests: Modified the max_age tests to properly validate the new behavior, including setting up distinct AuthenticatedAt and LastLogin values to ensure the correct time is used.

All authflow tests pass locally.

[nabokihms]

Thanks, both halves look right now, and the test finally separates the two timestamps. Small thing left: the comment says the client state is guaranteed non-nil, and the code below it keeps a nil fallback. One of the two should go.

[SashaMIT]

Thanks @nabokihms. Dropped the LastLogin fallback for max_age and the "guaranteed non-nil" comment: we now require ClientAuthState.AuthenticatedAt (direct or SSO-synced) and force re-authentication if it is missing.

go test ./server/authflow/ -count=1 is green locally.

[nabokihms]

Fail-closed works for me. One note: the guard now sits above the max_age check, so a missing AuthenticatedAt costs a full re-login even when the RP never asked for recency. Unreachable today, since direct login always writes the field and SSO inherits it, so a test on that branch would keep it that way.

[SashaMIT]

Fail-closed works for me too.

Added a regression that zeros AuthenticatedAt with max_age unset and asserts we force re-login, so that branch stays pinned even if login or SSO later stop writing the field.