diff --git a/server/src/nest/oidc/oidc.controller.ts b/server/src/nest/oidc/oidc.controller.ts index 8bd69ae62..873c35e13 100644 --- a/server/src/nest/oidc/oidc.controller.ts +++ b/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). diff --git a/server/tests/integration/oidc.test.ts b/server/tests/integration/oidc.test.ts index 76482a275..62c3f57f7 100644 --- a/server/tests/integration/oidc.test.ts +++ b/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 }); diff --git a/server/tests/unit/nest/oidc.controller.test.ts b/server/tests/unit/nest/oidc.controller.test.ts index 1ea093444..2f4e57bee 100644 --- a/server/tests/unit/nest/oidc.controller.test.ts +++ b/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'); }); diff --git a/wiki/Audit-Log.md b/wiki/Audit-Log.md index 0b7cd525f..ba34d8b78 100644 --- a/wiki/Audit-Log.md +++ b/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