fix(oidc): a sign-in links to an account by email only when the address was not set by the account itself

An SSO login that matched no account by its subject took the account with the
same email address and linked the provider identity to it. That address proves
nothing when the account typed it in itself: anyone could sign up with, or
switch their profile to, a colleague's address and receive the colleague's
first SSO login. The provider switch branch (#2110) relinked a row on any email
match without even asking for email_verified, and also for another subject of
the same provider.

users.email_self_asserted remembers an address the account set itself: signup
(not the first account) and an email change in the profile settings set it, an
admin entering a new address clears it, and accounts the provider creates start
without it. Existing accounts keep linking, except those whose address moved
away from the registered one without an admin editing it. A login by email now
needs a verified address and an unmarked account, and is refused for another
subject of the same issuer, with email_link_blocked and a translated message on
the login screen. An email change in the profile settings writes a
user.email_change audit row.

Refs #2110
This commit is contained in:
Maurice
2026-10-12 17:06:52 +02:00
parent 1b58acc78f
commit 3d7e8be9aa
49 changed files with 688 additions and 29 deletions
+87
View File
@@ -12,6 +12,7 @@ import { db as testDb } from '../../src/db/database';
import { AuditLog } from '../../src/db/entities/AuditLog.entity';
import { Users } from '../../src/db/entities/Users.entity';
import { OidcService } from '../../src/nest/oidc/oidc.service';
import { authCookie } from '../helpers/auth';
import { createUser } from '../helpers/factories';
import type { FactoryOrm } from '../helpers/factories/context';
import { findRow, findRows } from '../helpers/factories/rows';
@@ -349,6 +350,92 @@ describe('GET /api/auth/oidc/callback', () => {
});
});
// ── an email the account set itself ──────────────────────────────────────────
describe('GET /api/auth/oidc/callback onto an email the account set itself', () => {
/** One verified SSO sign-in for this sub and email, run through the real callback. */
async function ssoSignIn(sub: string, email: string): Promise<request.Response> {
mockDiscover.mockResolvedValueOnce(MOCK_DISCOVERY_DOC);
mockExchangeCode.mockResolvedValueOnce({ access_token: 'tok', id_token: 'fake.id.token', _ok: true, _status: 200 });
mockVerifyIdToken.mockResolvedValueOnce({ ok: true, claims: { sub } });
mockGetUserInfo.mockResolvedValueOnce({ sub, email, name: 'Colleague', email_verified: true });
const { state } = await oidcSvc.createState('http://localhost:3001/api/auth/oidc/callback');
return request(app)
.get(`/api/auth/oidc/callback?code=c-${sub}&state=${state}`)
.set('Cookie', `trek_oidc_state=${state}`);
}
async function register(username: string, email: string): Promise<request.Response> {
const res = await request(app).post('/api/auth/register').send({ username, email, password: 'Secure123!x' });
expect(res.status).toBe(201);
return res;
}
it('OIDC-030: a signup with a colleague address does not receive the colleague sign-in', async () => {
await register('owner', 'owner@example.com');
const squatter = await register('squatter', 'colleague@example.com');
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
try {
const res = await ssoSignIn('sub-colleague', 'colleague@example.com');
expect(res.status).toBe(302);
expect(res.headers.location).toContain('/login?oidc_error=email_link_blocked');
const row = await readUser(orm(), squatter.body.user.id);
expect(row.oidc_sub).toBeNull();
} finally {
warn.mockRestore();
}
});
it('OIDC-031: an address changed in the profile settings does not receive the sign-in either', async () => {
await register('owner', 'owner@example.com');
const member = await register('member', 'member@example.com');
const cookie = readCookie(member, 'trek_session');
const change = await request(app)
.put('/api/auth/me/settings')
.set('Cookie', `trek_session=${cookie}`)
.send({ email: 'colleague@example.com' });
expect(change.status).toBe(200);
const audit = await findRows(orm(), AuditLog, { user: member.body.user.id, action: 'user.email_change' });
expect(audit).toHaveLength(1);
expect(audit[0].details).toBeNull();
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
try {
const res = await ssoSignIn('sub-colleague', 'colleague@example.com');
expect(res.headers.location).toContain('/login?oidc_error=email_link_blocked');
expect((await readUser(orm(), member.body.user.id)).oidc_sub).toBeNull();
} finally {
warn.mockRestore();
}
});
it('OIDC-032: an account an admin created, or whose email an admin changed, links on its first sign-in', async () => {
const owner = await register('owner', 'owner@example.com');
const created = await request(app)
.post('/api/admin/users')
.set('Cookie', authCookie(owner.body.user.id))
.send({ username: 'colleague', email: 'colleague@example.com', password: 'Secure123!x' });
expect(created.status).toBe(201);
const linked = await ssoSignIn('sub-colleague', 'colleague@example.com');
expect(linked.headers.location).toContain('/login?oidc_code=');
expect((await readUser(orm(), created.body.user.id)).oidc_sub).toBe('sub-colleague');
// A blocked account is released by the admin entering the address.
const member = await register('member', 'typo@example.com');
const edited = await request(app)
.put(`/api/admin/users/${member.body.user.id}`)
.set('Cookie', authCookie(owner.body.user.id))
.send({ email: 'member@example.com' });
expect(edited.status).toBe(200);
const res = await ssoSignIn('sub-member', 'member@example.com');
expect(res.headers.location).toContain('/login?oidc_code=');
expect((await readUser(orm(), member.body.user.id)).oidc_sub).toBe('sub-member');
});
});
// ── /exchange ─────────────────────────────────────────────────────────────────
describe('GET /api/auth/oidc/exchange', () => {
@@ -0,0 +1,94 @@
/**
* `Migration20200101042800_an_email_the_account_typed_in_itself`: every
* existing account starts out trusted (0), except one whose address provably
* moved away from the one it registered with while no admin ever edited its
* email. A broken audit row is skipped, and a second run changes nothing.
*/
import { Migration20200101042800_an_email_the_account_typed_in_itself } from '../../../src/db/migrations/Migration20200101042800_an_email_the_account_typed_in_itself';
import {
createMigrationOrm,
migrateTo,
pendingNames,
rawExec,
rawQuery,
runMigrationDirect,
} from '../../helpers/migration-step';
import type { MikroORM } from '@mikro-orm/sqlite';
import { describe, expect, it } from 'vitest';
const TARGET = 'Migration20200101042800_an_email_the_account_typed_in_itself';
async function ormBeforeTarget(): Promise<MikroORM> {
const orm = await createMigrationOrm();
const names = await pendingNames(orm);
const idx = names.indexOf(TARGET);
expect(idx).toBeGreaterThan(0);
await migrateTo(orm, names[idx - 1]);
return orm;
}
async function seed(orm: MikroORM): Promise<void> {
// 1 registered and kept the address; 2 registered and changed it itself;
// 3 registered, changed it, and an admin set an email later; 4 was made by
// an admin (no register row); 5 registered in another case only; 6 has a
// broken register row.
await rawExec(
orm,
`INSERT INTO users (id, username, email, password_hash) VALUES
(1, 'kept', 'kept@test', 'x'),
(2, 'moved', 'colleague@test', 'x'),
(3, 'fixed', 'fixed-by-admin@test', 'x'),
(4, 'made', 'made@test', 'x'),
(5, 'cased', 'Cased@Test', 'x'),
(6, 'broken', 'broken-now@test', 'x')`,
);
await rawExec(
orm,
`INSERT INTO audit_log (user_id, action, resource, details) VALUES
(1, 'user.register', NULL, '{"username":"kept","email":"kept@test","role":"user"}'),
(2, 'user.register', NULL, '{"username":"moved","email":"moved@test","role":"user"}'),
(3, 'user.register', NULL, '{"username":"fixed","email":"fixed@test","role":"user"}'),
(4, 'admin.user_update', '3', '{"targetUser":"fixed-self@test","fields":["username","email","role"]}'),
(4, 'admin.user_update', '2', '{"targetUser":"colleague@test","fields":["role"]}'),
(5, 'user.register', NULL, '{"username":"cased","email":"cased@test","role":"user"}'),
(6, 'user.register', NULL, 'not json')`,
);
}
const flags = (orm: MikroORM) =>
rawQuery<{ id: number; email_self_asserted: number }>(orm, 'SELECT id, email_self_asserted FROM users ORDER BY id');
const EXPECTED = [
{ id: 1, email_self_asserted: 0 },
{ id: 2, email_self_asserted: 1 },
{ id: 3, email_self_asserted: 0 },
{ id: 4, email_self_asserted: 0 },
{ id: 5, email_self_asserted: 0 },
{ id: 6, email_self_asserted: 0 },
];
describe('email self-asserted migration', () => {
it('EMAILSELF-MIG-001: only an address the account provably changed itself is marked', async () => {
const orm = await ormBeforeTarget();
try {
await seed(orm);
await migrateTo(orm, TARGET);
expect(await flags(orm)).toEqual(EXPECTED);
} finally {
await orm.close(true);
}
}, 30000);
it('EMAILSELF-MIG-002: running it again changes nothing', async () => {
const orm = await ormBeforeTarget();
try {
await seed(orm);
await migrateTo(orm, TARGET);
await runMigrationDirect(orm, Migration20200101042800_an_email_the_account_typed_in_itself);
expect(await flags(orm)).toEqual(EXPECTED);
} finally {
await orm.close(true);
}
}, 30000);
});
@@ -39,6 +39,7 @@ const UNNUMBERED = [
'Migration20200101042500_a_cron_tick_runs_in_one_process',
'Migration20200101042600_a_session_row_per_sign_in',
'Migration20200101042700_timestamps_stored_in_one_spelling',
'Migration20200101042800_an_email_the_account_typed_in_itself',
];
// The last step the positional runner ever had; planLegacyBaseline refuses a
// schema_version past it.
@@ -240,6 +240,19 @@ describe('UsersRepository', () => {
expect(raw).toMatchObject({ username: 'localuser', email: 'local@example.com', role: 'user', login_count: 0 });
});
it('USERSREPO-016b: the self-set email mark is 0 unless the caller names it', async () => {
const base = { password_hash: 'hashed', role: 'user', first_seen_version: '1.2.3' };
const plain = await users.insertUser({ ...base, username: 'plainmark', email: 'plain@example.com' });
const own = await users.insertUser({
...base,
username: 'ownmark',
email: 'own@example.com',
email_self_asserted: 1,
});
expect(plain.email_self_asserted).toBe(0);
expect(own.email_self_asserted).toBe(1);
});
it('USERSREPO-017: SSO registration shape (O14) — oidc_sub/oidc_issuer/avatar set', async () => {
const row = await users.insertUser({
username: 'ssouser',
@@ -795,6 +795,21 @@ describe('admin quirk fixes (post-fold)', () => {
const row = await readUser(orm, user.id);
expect(row.username).toBe(user.username);
});
// The edit dialog sends the email on every save, so only a new address is the
// admin vouching for it; the account's own mark survives any other edit.
it('ADMIN-SVC-073 — a new email from the admin clears the self-set mark, an unchanged one keeps it', async () => {
const { user } = createUser(testDb, { email: 'self@example.com' });
await updateRows(orm, Users, { id: user.id }, { email_self_asserted: 1 });
await updateUser(String(user.id), { email: 'self@example.com', role: 'user', username: user.username });
expect((await readUser(orm, user.id)).email_self_asserted).toBe(1);
await updateUser(String(user.id), { email: 'SELF@example.com' });
expect((await readUser(orm, user.id)).email_self_asserted).toBe(1);
await updateUser(String(user.id), { email: 'confirmed@example.com' });
expect((await readUser(orm, user.id)).email_self_asserted).toBe(0);
});
});
// ── What an admin password reset ends, and what an ordinary edit must not ─────
@@ -707,6 +707,37 @@ describe('AuthController (authenticated)', () => {
expect(writeAudit).not.toHaveBeenCalled();
});
it('update-settings logs a new email address without the address, and nothing for any other save', async () => {
writeAudit.mockClear();
const withEmail = ac(
asvc({}),
rl(),
{},
{
updateSettings: vi
.fn()
.mockReturnValue({ success: true, user: { id: 1 }, changedKeys: [], emailChanged: true }),
},
);
expect(await withEmail.updateSettings(user, {}, req)).toEqual({ success: true, user: { id: 1 } });
expect(writeAudit).toHaveBeenCalledTimes(1);
expect(writeAudit).toHaveBeenCalledWith(expect.objectContaining({ userId: user.id, action: 'user.email_change' }));
expect(writeAudit.mock.calls[0][0].details).toBeUndefined();
writeAudit.mockClear();
await ac(
asvc({}),
rl(),
{},
{
updateSettings: vi
.fn()
.mockReturnValue({ success: true, user: { id: 1 }, changedKeys: [], emailChanged: false }),
},
).updateSettings(user, {}, req);
expect(writeAudit).not.toHaveBeenCalled();
});
it('update-settings + get-settings map errors, else return their payloads', async () => {
expect(
await thrownAsync(() =>
@@ -921,6 +921,13 @@ describe('registerUser — success paths', () => {
expect(result.auditDetails).toEqual({ username: 'first', email: 'first@x.com', role: 'admin' });
});
it('AUTH-DB-056b: a signup marks the address it typed in, except for the first account', async () => {
const first = await svc.registerUser({ username: 'owner', email: 'owner@x.com', password: 'Secure123!' });
const second = await svc.registerUser({ username: 'joiner', email: 'joiner@x.com', password: 'Secure123!' });
expect((await readUser(orm, first.auditUserId!)).email_self_asserted).toBe(0);
expect((await readUser(orm, second.auditUserId!)).email_self_asserted).toBe(1);
});
it('AUTH-DB-057: missing fields / bad email / duplicate answer their bespoke 400/409s', async () => {
createUser(testDb, { username: 'taken', email: 'taken@x.com' });
expect(await asLegacyResult(svc.registerUser({ username: '', email: 'a@x.com', password: 'Secure123!' }))).toEqual({
+148 -1
View File
@@ -125,7 +125,13 @@ const orm = () => sharedTestOrm(testDb);
/** Writes columns of the user row directly, the state a login starts from. */
async function setUserColumns(
userId: number,
data: { oidc_sub?: string | null; oidc_issuer?: string | null; avatar?: string | null; updated_at?: string },
data: {
oidc_sub?: string | null;
oidc_issuer?: string | null;
avatar?: string | null;
updated_at?: string;
email_self_asserted?: number;
},
): Promise<void> {
await updateRows(await orm(), Users, { id: userId }, data);
}
@@ -1030,6 +1036,147 @@ describe('findOrCreateUser', () => {
// ── claim-based role mapping on login (#2364) ────────────────────────────────
// A sign-in that finds an account only through its email address. The address is
// proof of nothing when the account typed it in itself (signup, profile settings):
// whoever did that would otherwise get every later SSO login for that address.
describe('findOrCreateUser linking by email', () => {
const SELF = { email_self_asserted: 1 };
it('OIDC-SVC-100: a verified sign-in is not linked to an account that set this email itself', async () => {
const { user } = createUser(testDb, { email: 'colleague@corp.test' });
await setUserColumns(user.id, { oidc_sub: null, oidc_issuer: null, ...SELF });
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
try {
const result = await svc.findOrCreateUser(
{ sub: 'victim-sub', email: 'colleague@corp.test', name: 'Colleague', email_verified: true },
MOCK_CONFIG,
);
expect(result).toEqual({ error: 'email_link_blocked' });
const row = await readUser(await orm(), user.id);
expect({ oidc_sub: row.oidc_sub, oidc_issuer: row.oidc_issuer }).toEqual({ oidc_sub: null, oidc_issuer: null });
} finally {
warn.mockRestore();
}
});
it('OIDC-SVC-101: an unverified sign-in against such an account says email_not_verified, not that the account exists', async () => {
const { user } = createUser(testDb, { email: 'quiet@corp.test' });
await setUserColumns(user.id, { oidc_sub: null, oidc_issuer: null, ...SELF });
const result = await svc.findOrCreateUser({ sub: 'other-sub', email: 'quiet@corp.test' }, MOCK_CONFIG);
expect(result).toEqual({ error: 'email_not_verified' });
expect((await readUser(await orm(), user.id)).oidc_sub).toBeNull();
});
it('OIDC-SVC-102: an account an admin created still links on its first verified sign-in', async () => {
const { user } = createUser(testDb, { email: 'made-by-admin@corp.test' });
await setUserColumns(user.id, { oidc_sub: null, oidc_issuer: null, email_self_asserted: 0 });
const result = await svc.findOrCreateUser(
{ sub: 'sub-made-by-admin', email: 'made-by-admin@corp.test', email_verified: 'true' },
MOCK_CONFIG,
);
expect((result as { user: { id: number } }).user.id).toBe(user.id);
expect((await readUser(await orm(), user.id)).oidc_sub).toBe('sub-made-by-admin');
});
it('OIDC-SVC-103: the same provider with another subject is refused, verified or not, and the row keeps its link', async () => {
const { user } = createUser(testDb, { email: 'linked@corp.test' });
await setUserColumns(user.id, { oidc_sub: 'sub-owner', oidc_issuer: MOCK_CONFIG.issuer });
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
try {
const verified = await svc.findOrCreateUser(
{ sub: 'sub-stranger', email: 'linked@corp.test', email_verified: true },
MOCK_CONFIG,
);
const unverified = await svc.findOrCreateUser({ sub: 'sub-stranger', email: 'linked@corp.test' }, MOCK_CONFIG);
expect(verified).toEqual({ error: 'email_link_blocked' });
expect(unverified).toEqual({ error: 'email_not_verified' });
const row = await readUser(await orm(), user.id);
expect({ oidc_sub: row.oidc_sub, oidc_issuer: row.oidc_issuer }).toEqual({
oidc_sub: 'sub-owner',
oidc_issuer: MOCK_CONFIG.issuer,
});
} finally {
warn.mockRestore();
}
});
it('OIDC-SVC-104: a provider switch needs a verified email, the row stays on the old provider otherwise', async () => {
const { user } = createUser(testDb, { email: 'mover@corp.test' });
await setUserColumns(user.id, { oidc_sub: 'sub-old', oidc_issuer: 'https://old-idp.example.com' });
const result = await svc.findOrCreateUser(
{ sub: 'sub-new', email: 'mover@corp.test' },
{ ...MOCK_CONFIG, issuer: 'https://new-idp.example.com' },
);
expect(result).toEqual({ error: 'email_not_verified' });
const row = await readUser(await orm(), user.id);
expect({ oidc_sub: row.oidc_sub, oidc_issuer: row.oidc_issuer }).toEqual({
oidc_sub: 'sub-old',
oidc_issuer: 'https://old-idp.example.com',
});
});
it('OIDC-SVC-105: a provider switch onto an email the account set itself is refused', async () => {
const { user } = createUser(testDb, { email: 'switched-self@corp.test' });
await setUserColumns(user.id, { oidc_sub: 'sub-old', oidc_issuer: 'https://old-idp.example.com', ...SELF });
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
try {
const result = await svc.findOrCreateUser(
{ sub: 'sub-new', email: 'switched-self@corp.test', email_verified: true },
{ ...MOCK_CONFIG, issuer: 'https://new-idp.example.com' },
);
expect(result).toEqual({ error: 'email_link_blocked' });
expect((await readUser(await orm(), user.id)).oidc_sub).toBe('sub-old');
} finally {
warn.mockRestore();
}
});
it('OIDC-SVC-106: a refused link never hands the account the role from the claims', async () => {
process.env.OIDC_ADMIN_VALUE = 'trek-admins';
const { user } = createUser(testDb, { email: 'trap@corp.test', role: 'user' });
await setUserColumns(user.id, { oidc_sub: null, oidc_issuer: null, ...SELF });
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
try {
await svc.findOrCreateUser(
{ sub: 'admin-sub', email: 'trap@corp.test', email_verified: true, groups: ['trek-admins'] },
MOCK_CONFIG,
);
expect((await readUser(await orm(), user.id)).role).toBe('user');
} finally {
warn.mockRestore();
}
});
it('OIDC-SVC-107: an account found by its own subject signs in, whoever set its email', async () => {
const { user } = createUser(testDb, { email: 'own@corp.test' });
await setUserColumns(user.id, { oidc_sub: 'sub-own', oidc_issuer: MOCK_CONFIG.issuer, ...SELF });
const result = await svc.findOrCreateUser({ sub: 'sub-own', email: 'own@corp.test' }, MOCK_CONFIG);
expect((result as { user: { id: number } }).user.id).toBe(user.id);
});
it('OIDC-SVC-108: an account the provider creates starts out with a trusted email', async () => {
createUser(testDb, { email: 'someone@corp.test' });
await svc.findOrCreateUser({ sub: 'sub-fresh', email: 'fresh@corp.test', name: 'Fresh' }, MOCK_CONFIG);
expect((await userByEmail('fresh@corp.test'))?.email_self_asserted).toBe(0);
});
});
describe('findOrCreateUser role mapping', () => {
type RoleResult = { user: any; roleChange?: { from: string; to: string; claim: string } };
@@ -143,6 +143,32 @@ describe('updateSettings', () => {
});
});
// An address the account types in itself is not one an SSO sign-in may link to by email.
describe('updateSettings marks an email the account set itself', () => {
const flagOf = async (id: number) => (await readUser(await sharedTestOrm(testDb), id)).email_self_asserted;
it('AUTH-DB-120: a new address is marked and reported as changed', async () => {
const { user } = createUser(testDb, { email: 'before@example.com' });
const result = await profile.updateSettings(user.id, { email: 'colleague@example.com' });
expect(result.emailChanged).toBe(true);
expect(await flagOf(user.id)).toBe(1);
});
it('AUTH-DB-121: the same address in another case keeps its standing', async () => {
const { user } = createUser(testDb, { email: 'same@example.com' });
const result = await profile.updateSettings(user.id, { email: ' Same@Example.com ' });
expect(result.emailChanged).toBe(false);
expect(await flagOf(user.id)).toBe(0);
});
it('AUTH-DB-122: a save without an email leaves the mark alone', async () => {
const { user } = createUser(testDb);
const result = await profile.updateSettings(user.id, { username: 'renamed' });
expect(result.emailChanged).toBe(false);
expect(await flagOf(user.id)).toBe(0);
});
});
// ---------------------------------------------------------------------------
// getSettings
// ---------------------------------------------------------------------------