fix(oidc): a successful SSO login writes the user.login audit row

Every other login method writes one; the SSO callback touched last_login and
login_count and wrote nothing, so an instance moving its users to SSO watched
its audit log go quiet while looking complete. The row is written in the
callback, where the provider has vouched for the user and the client IP is,
next to the role-change row, with method: oidc in the details the way the
passkey login names its method.

Closes #2417

server/src/nest/oidc/oidc.controller.ts

@@ -182,6 +182,11 @@ export class OidcController {
       }
 
       this.oidc.touchLastLogin(result.user.id);
+      // The login row every other method writes (#2417). Here rather than at
+      // /exchange, because this is where the provider has vouched for the user
+      // and where the client IP is, the same place the role change is recorded;
+      // `method` names the way in, as the passkey login does.
+      this.audit.writeAudit({ userId: result.user.id, action: 'user.login', ip: getClientIp(req), details: { method: 'oidc' } });
       // Pass the flag through untouched: `undefined` must reach the token as
       // "absent", not `false`, or the sliding renewal would later downgrade the
       // default persistent cookie to a browser-session one (remember-me, #1927).

server/tests/integration/oidc.test.ts

@@ -190,6 +190,27 @@ describe('GET /api/auth/oidc/callback', () => {
     expect(res.headers.location).toContain('/login?oidc_code=');
   });
 
+  it('OIDC-004b: a successful login writes the user.login row every other method writes (#2417)', async () => {
+    const { user } = createUser(testDb, { email: 'audited@example.com' });
+    mockDiscover.mockResolvedValueOnce(MOCK_DISCOVERY_DOC);
+    mockExchangeCode.mockResolvedValueOnce({ access_token: 'test-access-token', id_token: 'fake.id.token', _ok: true, _status: 200 });
+    mockVerifyIdToken.mockResolvedValueOnce({ ok: true, claims: { sub: 'sub-audited-1' } });
+    mockGetUserInfo.mockResolvedValueOnce({ sub: 'sub-audited-1', email: 'audited@example.com', name: 'Audited', email_verified: true });
+    const { state } = oidcSvc.createState('http://localhost:3001/api/auth/oidc/callback');
+
+    const res = await request(app).get(`/api/auth/oidc/callback?code=authcode123&state=${state}`).set('Cookie', `trek_oidc_state=${state}`);
+
+    expect(res.status).toBe(302);
+    // The query from the report, against the same table.
+    const rows = testDb.prepare('SELECT user_id, action, details FROM audit_log WHERE user_id = ? ORDER BY id DESC').all(user.id) as
+      { user_id: number; action: string; details: string | null }[];
+    expect(rows.map(r => r.action)).toContain('user.login');
+    const login = rows.find(r => r.action === 'user.login')!;
+    expect(JSON.parse(login.details || '{}')).toEqual({ method: 'oidc' });
+    const counted = testDb.prepare('SELECT login_count FROM users WHERE id = ?').get(user.id) as { login_count: number };
+    expect(counted.login_count).toBe(1);
+  });
+
   it('OIDC-005: new user gets created when registration is open', async () => {
     mockDiscover.mockResolvedValueOnce(MOCK_DISCOVERY_DOC);
     mockExchangeCode.mockResolvedValueOnce({ access_token: 'new-token', id_token: 'fake.id.token', _ok: true, _status: 200 });

server/tests/unit/nest/oidc.controller.test.ts

@@ -206,6 +206,34 @@ describe('OidcController /callback', () => {
     expect(ok.cookie).toHaveBeenCalledWith('trek_oidc_exchange', 'bnd', expect.objectContaining({ httpOnly: true, maxAge: 60000, sameSite: 'lax' }));
   });
 
+  it('OIDC-CB-LOGIN-001: a successful login writes the user.login row every other method writes (#2417)', async () => {
+    const res = makeRes();
+    const reqIp = { query: {}, headers: {}, ip: '203.0.113.9', cookies: { trek_oidc_state: 's' } } as unknown as Request;
+    await ctl(svc({
+      exchangeCodeForToken: vi.fn().mockResolvedValue({ _ok: true, access_token: 'at', id_token: 'it' }),
+      verifyIdToken: vi.fn().mockResolvedValue({ ok: true, claims: { sub: 'u1' } }),
+      getUserInfo: vi.fn().mockResolvedValue({ email: 'a@b.c', sub: 'u1' }),
+      findOrCreateUser: vi.fn().mockReturnValue({ user: { id: 7 } }),
+    })).callback('c', 's', undefined, reqIp, res);
+
+    expect(writeAudit).toHaveBeenCalledTimes(1);
+    expect(writeAudit).toHaveBeenCalledWith({ userId: 7, action: 'user.login', ip: '203.0.113.9', details: { method: 'oidc' } });
+    expect(res.redirectedTo).toBe('https://app/login?oidc_code=ac');
+  });
+
+  it('OIDC-CB-LOGIN-002: a login refused by the user lookup writes no login row', async () => {
+    const res = makeRes();
+    await ctl(svc({
+      exchangeCodeForToken: vi.fn().mockResolvedValue({ _ok: true, access_token: 'at', id_token: 'it' }),
+      verifyIdToken: vi.fn().mockResolvedValue({ ok: true, claims: { sub: 'u1' } }),
+      getUserInfo: vi.fn().mockResolvedValue({ email: 'a@b.c', sub: 'u1' }),
+      findOrCreateUser: vi.fn().mockReturnValue({ error: 'registration_disabled' }),
+    })).callback('c', 's', undefined, reqCb('s'), res);
+
+    expect(writeAudit).not.toHaveBeenCalled();
+    expect(res.redirectedTo).toBe('https://app/login?oidc_error=registration_disabled');
+  });
+
   it('audits a role change the claim mapping made, with the client IP and the claim name only', async () => {
     const res = makeRes();
     const reqIp = { query: {}, headers: {}, ip: '203.0.113.9', cookies: { trek_oidc_state: 's' } } as unknown as Request;
@@ -223,10 +251,12 @@ describe('OidcController /callback', () => {
       ip: '203.0.113.9',
       details: { from: 'admin', to: 'user', claim: 'entitlements' },
     });
+    // The login row still follows, the role change does not replace it.
+    expect(writeAudit).toHaveBeenCalledWith(expect.objectContaining({ userId: 7, action: 'user.login' }));
     expect(res.redirectedTo).toBe('https://app/login?oidc_code=ac');
   });
 
-  it('writes no audit row when the login left the role alone', async () => {
+  it('writes no role-change row when the login left the role alone', async () => {
     const res = makeRes();
     await ctl(svc({
       exchangeCodeForToken: vi.fn().mockResolvedValue({ _ok: true, access_token: 'at', id_token: 'it' }),
@@ -235,7 +265,7 @@ describe('OidcController /callback', () => {
       findOrCreateUser: vi.fn().mockReturnValue({ user: { id: 7 } }),
     })).callback('c', 's', undefined, reqCb('s'), res);
 
-    expect(writeAudit).not.toHaveBeenCalled();
+    expect(writeAudit).not.toHaveBeenCalledWith(expect.objectContaining({ action: 'oidc.role_change' }));
     expect(res.redirectedTo).toBe('https://app/login?oidc_code=ac');
   });
 

wiki/Audit-Log.md

@@ -25,7 +25,7 @@ Actions are grouped by area below. The **Action key** is the raw value stored in
 | `user.password_reset_success` | Password reset completed |
 | `user.password_reset_fail` | Password reset attempt rejected (`reason` in details) |
 
-A request for an account that can actually be reset writes **two** rows: one with `delivered: "pending"` when the mail is handed off, one with the delivery result. Every other outcome writes a single row carrying a `reason` instead — `no_user`, `oidc_only`, `throttled_per_email` or `password_login_disabled`. Passkey logins are not a separate key: they land as `user.login` with `method: passkey` in the details.
+A request for an account that can actually be reset writes **two** rows: one with `delivered: "pending"` when the mail is handed off, one with the delivery result. Every other outcome writes a single row carrying a `reason` instead — `no_user`, `oidc_only`, `throttled_per_email` or `password_login_disabled`. Passkey logins are not a separate key: they land as `user.login` with `method: passkey` in the details, and SSO logins the same way with `method: oidc`.
 
 ### MFA