fix(sso): bind Microsoft sign-in to the IdP identity, not the email claim
SSO picked the local account by the email/preferred_username claim, which Entra does not verify. Anyone able to present a matching email (another tenant with a multi-tenant issuer, or an editable attribute) could sign in as that user without a password. - The first SSO login links the account to "<iss>|<oid>" (new unique column User.ssoSubject); afterwards only that identity signs in, and a matching email from another identity is refused. - Multi-tenant issuers (common/organizations/consumers) disable SSO. - Emails are compared case-insensitively and stored lower-case, so [email protected] and [email protected] can no longer be two accounts; the migration lower-cases existing emails where that does not collide. - An admin changing a user's email clears the link. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
This commit is contained in:
1 parent
c4a11b4704
commit
a25265b88a
13 files changed
+238
-18
No files matched your search
+15
@@ -0,0 +1,15 @@
|
||||
-- Microsoft sign-in is bound to the IdP identity ("<iss>|<oid>"), set on the
|
||||
-- first SSO login; the unverified email claim no longer picks the account.
|
||||
ALTER TABLE "User" ADD COLUMN "ssoSubject" TEXT;
|
||||
CREATE UNIQUE INDEX "User_ssoSubject_key" ON "User"("ssoSubject");
|
||||
|
||||
-- Emails are compared case-insensitively from now on. Store existing ones
|
||||
-- lower-case, except where that would collide with another account - those
|
||||
-- stay as they are and are still found by the case-insensitive lookup.
|
||||
UPDATE "User" AS u
|
||||
SET "email" = lower(u."email")
|
||||
WHERE u."email" <> lower(u."email")
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM "User" AS o
|
||||
WHERE o."id" <> u."id" AND lower(o."email") = lower(u."email")
|
||||
);
|
||||
@@ -88,6 +88,11 @@ model User {
|
||||
// nach einer Passwortaenderung nicht bis zu 15 Minuten weiter funktioniert.
|
||||
passwordChangedAt DateTime?
|
||||
|
||||
// Unveraenderliche Identitaet beim Identity Provider ("<iss>|<oid>"),
|
||||
// gesetzt bei der ersten Microsoft-Anmeldung. Danach entscheidet nur sie,
|
||||
// nicht mehr der (unverifizierte) E-Mail-Claim, welches Konto sich anmeldet.
|
||||
ssoSubject String? @unique
|
||||
|
||||
refreshTokens RefreshToken[]
|
||||
|
||||
inspections Inspection[]
|
||||
|
||||
@@ -1,8 +1,10 @@
|
||||
import { ApiProperty } from '@nestjs/swagger';
|
||||
import { IsEmail, IsString, MinLength } from 'class-validator';
|
||||
import { NormalizeEmail } from '../../common/validation/normalize-email';
|
||||
|
||||
export class LoginDto {
|
||||
@ApiProperty()
|
||||
@NormalizeEmail()
|
||||
@IsEmail()
|
||||
email!: string;
|
||||
|
||||
|
||||
@@ -18,9 +18,26 @@ jest.mock('openid-client', () => ({
|
||||
|
||||
const mockedClient = client as jest.Mocked<typeof client>;
|
||||
|
||||
const IDENTITY = {
|
||||
iss: 'https://login.microsoftonline.com/tenant/v2.0',
|
||||
sub: 'pairwise-sub',
|
||||
oid: 'object-id-1',
|
||||
};
|
||||
const SSO_SUBJECT = `${IDENTITY.iss}|${IDENTITY.oid}`;
|
||||
|
||||
function grantWithClaims(claims: Record<string, unknown>) {
|
||||
return {
|
||||
claims: () => claims,
|
||||
} as unknown as Awaited<ReturnType<typeof client.authorizationCodeGrant>>;
|
||||
}
|
||||
|
||||
describe('SsoService', () => {
|
||||
let ssoService: SsoService;
|
||||
let usersService: { findByEmail: jest.Mock };
|
||||
let usersService: {
|
||||
findByEmail: jest.Mock;
|
||||
findBySsoSubject: jest.Mock;
|
||||
linkSsoSubject: jest.Mock;
|
||||
};
|
||||
let jwtService: { sign: jest.Mock; verify: jest.Mock };
|
||||
const originalEnv = { ...process.env };
|
||||
|
||||
@@ -33,7 +50,11 @@ describe('SsoService', () => {
|
||||
process.env.OIDC_REDIRECT_URI =
|
||||
'https://api.example.com/api/v1/auth/sso/callback';
|
||||
process.env.JWT_ACCESS_SECRET = 'unit-test-jwt-access-secret';
|
||||
usersService = { findByEmail: jest.fn() };
|
||||
usersService = {
|
||||
findByEmail: jest.fn(),
|
||||
findBySsoSubject: jest.fn().mockResolvedValue(null),
|
||||
linkSsoSubject: jest.fn(),
|
||||
};
|
||||
jwtService = { sign: jest.fn(), verify: jest.fn() };
|
||||
ssoService = new SsoService(
|
||||
usersService as unknown as UsersService,
|
||||
@@ -50,6 +71,12 @@ describe('SsoService', () => {
|
||||
expect(ssoService.isConfigured()).toBe(true);
|
||||
});
|
||||
|
||||
it('returns false for a multi-tenant issuer', () => {
|
||||
process.env.OIDC_ISSUER_URL =
|
||||
'https://login.microsoftonline.com/common/v2.0';
|
||||
expect(ssoService.isConfigured()).toBe(false);
|
||||
});
|
||||
|
||||
it('returns false when a variable is missing', () => {
|
||||
delete process.env.OIDC_CLIENT_SECRET;
|
||||
expect(ssoService.isConfigured()).toBe(false);
|
||||
@@ -116,7 +143,7 @@ describe('SsoService', () => {
|
||||
|
||||
it('returns no_email when the ID token has neither email nor preferred_username', async () => {
|
||||
mockedClient.authorizationCodeGrant.mockResolvedValue({
|
||||
claims: () => ({ sub: 'abc' }),
|
||||
claims: () => ({ ...IDENTITY }),
|
||||
} as unknown as Awaited<
|
||||
ReturnType<typeof client.authorizationCodeGrant>
|
||||
>);
|
||||
@@ -132,7 +159,7 @@ describe('SsoService', () => {
|
||||
|
||||
it('returns no_account when no active local user matches the email claim', async () => {
|
||||
mockedClient.authorizationCodeGrant.mockResolvedValue({
|
||||
claims: () => ({ email: '[email protected]' }),
|
||||
claims: () => ({ ...IDENTITY, email: '[email protected]' }),
|
||||
} as unknown as Awaited<
|
||||
ReturnType<typeof client.authorizationCodeGrant>
|
||||
>);
|
||||
@@ -149,7 +176,10 @@ describe('SsoService', () => {
|
||||
|
||||
it('signs a short-lived sso-complete token for an active matching user', async () => {
|
||||
mockedClient.authorizationCodeGrant.mockResolvedValue({
|
||||
claims: () => ({ preferred_username: '[email protected]' }),
|
||||
claims: () => ({
|
||||
...IDENTITY,
|
||||
preferred_username: '[email protected]',
|
||||
}),
|
||||
} as unknown as Awaited<
|
||||
ReturnType<typeof client.authorizationCodeGrant>
|
||||
>);
|
||||
@@ -175,6 +205,56 @@ describe('SsoService', () => {
|
||||
expect.objectContaining({ sub: 'user-1', purpose: 'sso-complete' }),
|
||||
expect.objectContaining({ expiresIn: '60s' }),
|
||||
);
|
||||
expect(usersService.linkSsoSubject).toHaveBeenCalledWith(
|
||||
'user-1',
|
||||
SSO_SUBJECT,
|
||||
);
|
||||
});
|
||||
|
||||
it('signs in a linked account by its identity, whatever the email claim says', async () => {
|
||||
mockedClient.authorizationCodeGrant.mockResolvedValue(
|
||||
grantWithClaims({ ...IDENTITY, email: '[email protected]' }),
|
||||
);
|
||||
usersService.findBySsoSubject.mockResolvedValue({
|
||||
id: 'user-1',
|
||||
isActive: true,
|
||||
});
|
||||
jwtService.sign.mockReturnValue('signed-sso-code');
|
||||
|
||||
const result = await ssoService.handleCallback(
|
||||
new URL('https://api.example.com/callback?code=x'),
|
||||
'verifier',
|
||||
'random-state',
|
||||
);
|
||||
|
||||
expect(result).toEqual({ status: 'success', ssoCode: 'signed-sso-code' });
|
||||
expect(usersService.findBySsoSubject).toHaveBeenCalledWith(SSO_SUBJECT);
|
||||
expect(usersService.findByEmail).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('refuses a matching email when the account is linked to another identity', async () => {
|
||||
mockedClient.authorizationCodeGrant.mockResolvedValue(
|
||||
grantWithClaims({
|
||||
...IDENTITY,
|
||||
oid: 'attacker-object-id',
|
||||
email: '[email protected]',
|
||||
}),
|
||||
);
|
||||
usersService.findByEmail.mockResolvedValue({
|
||||
id: 'user-1',
|
||||
isActive: true,
|
||||
ssoSubject: SSO_SUBJECT,
|
||||
});
|
||||
|
||||
const result = await ssoService.handleCallback(
|
||||
new URL('https://api.example.com/callback?code=x'),
|
||||
'verifier',
|
||||
'random-state',
|
||||
);
|
||||
|
||||
expect(result).toEqual({ status: 'error', reason: 'no_account' });
|
||||
expect(jwtService.sign).not.toHaveBeenCalled();
|
||||
expect(usersService.linkSsoSubject).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -21,6 +21,21 @@ interface SsoCompleteTokenPayload {
|
||||
purpose: typeof SSO_COMPLETE_TOKEN_PURPOSE;
|
||||
}
|
||||
|
||||
// Entra's shared endpoints accept users of ANY tenant - with them, anyone
|
||||
// able to create an account in some tenant could present a matching email.
|
||||
const MULTI_TENANT_SEGMENTS = new Set(['common', 'organizations', 'consumers']);
|
||||
|
||||
function isSingleTenantIssuer(issuerUrl: string): boolean {
|
||||
try {
|
||||
const segments = new URL(issuerUrl).pathname.split('/').filter(Boolean);
|
||||
return !segments.some((segment) =>
|
||||
MULTI_TENANT_SEGMENTS.has(segment.toLowerCase()),
|
||||
);
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
export type SsoCallbackResult =
|
||||
| { status: 'success'; ssoCode: string }
|
||||
| { status: 'error'; reason: 'exchange_failed' | 'no_email' | 'no_account' };
|
||||
@@ -36,12 +51,22 @@ export class SsoService {
|
||||
) {}
|
||||
|
||||
isConfigured(): boolean {
|
||||
return Boolean(
|
||||
process.env.OIDC_ISSUER_URL &&
|
||||
process.env.OIDC_CLIENT_ID &&
|
||||
process.env.OIDC_CLIENT_SECRET &&
|
||||
process.env.OIDC_REDIRECT_URI,
|
||||
const issuer = process.env.OIDC_ISSUER_URL;
|
||||
if (
|
||||
!issuer ||
|
||||
!process.env.OIDC_CLIENT_ID ||
|
||||
!process.env.OIDC_CLIENT_SECRET ||
|
||||
!process.env.OIDC_REDIRECT_URI
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
if (!isSingleTenantIssuer(issuer)) {
|
||||
this.logger.error(
|
||||
'SSO disabled: OIDC_ISSUER_URL must name one tenant (not common/organizations/consumers)',
|
||||
);
|
||||
return false;
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
private async getConfig(): Promise<client.Configuration> {
|
||||
@@ -98,6 +123,27 @@ export class SsoService {
|
||||
return { status: 'error', reason: 'exchange_failed' };
|
||||
}
|
||||
const claims = tokens.claims();
|
||||
// The account is bound to the IdP's immutable identity (issuer + object
|
||||
// id), not to the email claim: Entra does not verify email or
|
||||
// preferred_username, so a mutable claim alone must never pick the
|
||||
// account.
|
||||
const objectId =
|
||||
(claims?.oid as string | undefined) ?? claims?.sub ?? undefined;
|
||||
const ssoSubject =
|
||||
claims?.iss && objectId ? `${claims.iss}|${objectId}` : null;
|
||||
if (!ssoSubject) {
|
||||
this.logger.warn('SSO callback succeeded but ID token had no subject');
|
||||
return { status: 'error', reason: 'no_account' };
|
||||
}
|
||||
const linked = await this.usersService.findBySsoSubject(ssoSubject);
|
||||
if (linked) {
|
||||
if (!linked.isActive) {
|
||||
this.logger.warn(`SSO login denied: user ${linked.id} is deactivated`);
|
||||
return { status: 'error', reason: 'no_account' };
|
||||
}
|
||||
return { status: 'success', ssoCode: this.signSsoCode(linked.id) };
|
||||
}
|
||||
|
||||
const email =
|
||||
(claims?.email as string | undefined) ??
|
||||
(claims?.preferred_username as string | undefined);
|
||||
@@ -109,16 +155,27 @@ export class SsoService {
|
||||
}
|
||||
const user = await this.usersService.findByEmail(email);
|
||||
if (!user || !user.isActive) {
|
||||
this.logger.warn('SSO login denied: no active local account for email');
|
||||
return { status: 'error', reason: 'no_account' };
|
||||
}
|
||||
if (user.ssoSubject) {
|
||||
// Already bound to a different IdP identity: someone else is
|
||||
// presenting this email.
|
||||
this.logger.warn(
|
||||
`SSO login denied: no active local account for ${email}`,
|
||||
`SSO login denied: user ${user.id} is linked to another identity`,
|
||||
);
|
||||
return { status: 'error', reason: 'no_account' };
|
||||
}
|
||||
const ssoCode = this.jwtService.sign(
|
||||
{ sub: user.id, purpose: SSO_COMPLETE_TOKEN_PURPOSE },
|
||||
// First sign-in: bind the account to this identity for good.
|
||||
await this.usersService.linkSsoSubject(user.id, ssoSubject);
|
||||
return { status: 'success', ssoCode: this.signSsoCode(user.id) };
|
||||
}
|
||||
|
||||
private signSsoCode(userId: string): string {
|
||||
return this.jwtService.sign(
|
||||
{ sub: userId, purpose: SSO_COMPLETE_TOKEN_PURPOSE },
|
||||
jwtSignOptions(TOKEN_AUDIENCE.ssoComplete, SSO_COMPLETE_TOKEN_TTL),
|
||||
);
|
||||
return { status: 'success', ssoCode };
|
||||
}
|
||||
|
||||
verifySsoCode(ssoCode: string): { userId: string } {
|
||||
|
||||
@@ -0,0 +1,12 @@
|
||||
import { Transform } from 'class-transformer';
|
||||
|
||||
/**
|
||||
* Emails are compared case-insensitively (see UsersService.findByEmail);
|
||||
* storing them trimmed and lower-case keeps "[email protected]" and "[email protected]"
|
||||
* from becoming two accounts.
|
||||
*/
|
||||
export function NormalizeEmail(): PropertyDecorator {
|
||||
return Transform(({ value }: { value: unknown }) =>
|
||||
typeof value === 'string' ? value.trim().toLowerCase() : value,
|
||||
);
|
||||
}
|
||||
@@ -1,5 +1,6 @@
|
||||
import { ApiPropertyOptional } from '@nestjs/swagger';
|
||||
import { IsEmail, IsOptional, IsString, MinLength } from 'class-validator';
|
||||
import { NormalizeEmail } from '../../common/validation/normalize-email';
|
||||
|
||||
export class UpdateProfileDto {
|
||||
@ApiPropertyOptional()
|
||||
@@ -10,6 +11,7 @@ export class UpdateProfileDto {
|
||||
|
||||
@ApiPropertyOptional()
|
||||
@IsOptional()
|
||||
@NormalizeEmail()
|
||||
@IsEmail()
|
||||
email?: string;
|
||||
|
||||
|
||||
@@ -20,6 +20,7 @@ describe('ProfileService', () => {
|
||||
let prisma: {
|
||||
user: {
|
||||
findUnique: jest.Mock;
|
||||
findFirst: jest.Mock;
|
||||
update: jest.Mock;
|
||||
};
|
||||
refreshToken: { updateMany: jest.Mock };
|
||||
@@ -48,7 +49,15 @@ describe('ProfileService', () => {
|
||||
baseUser.mfaEnabled = false;
|
||||
baseUser.mfaSecret = null;
|
||||
prisma = {
|
||||
user: { findUnique: jest.fn(), update: jest.fn() },
|
||||
user: {
|
||||
findUnique: jest.fn(),
|
||||
// The email-in-use check is case-insensitive (findFirst); tests
|
||||
// stub it through findUnique.
|
||||
findFirst: jest.fn(
|
||||
(args: unknown): unknown => prisma.user.findUnique(args) as unknown,
|
||||
),
|
||||
update: jest.fn(),
|
||||
},
|
||||
refreshToken: { updateMany: jest.fn().mockResolvedValue({}) },
|
||||
};
|
||||
auditLog = { record: jest.fn().mockResolvedValue(undefined) };
|
||||
|
||||
@@ -69,8 +69,8 @@ export class ProfileService {
|
||||
'Aktuelles Passwort erforderlich, um die E-Mail-Adresse zu ändern',
|
||||
);
|
||||
}
|
||||
const existing = await this.prisma.user.findUnique({
|
||||
where: { email: dto.email },
|
||||
const existing = await this.prisma.user.findFirst({
|
||||
where: { email: { equals: dto.email, mode: 'insensitive' } },
|
||||
});
|
||||
if (existing) {
|
||||
throw new ConflictException('Email already in use');
|
||||
|
||||
@@ -1,8 +1,10 @@
|
||||
import { ApiProperty } from '@nestjs/swagger';
|
||||
import { IsEmail, IsString, MinLength } from 'class-validator';
|
||||
import { NormalizeEmail } from '../../common/validation/normalize-email';
|
||||
|
||||
export class CreateUserDto {
|
||||
@ApiProperty()
|
||||
@NormalizeEmail()
|
||||
@IsEmail()
|
||||
email!: string;
|
||||
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import { ApiPropertyOptional } from '@nestjs/swagger';
|
||||
import { IsEmail, IsOptional, IsString, MinLength } from 'class-validator';
|
||||
import { NormalizeEmail } from '../../common/validation/normalize-email';
|
||||
|
||||
export class UpdateUserDto {
|
||||
@ApiPropertyOptional()
|
||||
@@ -16,6 +17,7 @@ export class UpdateUserDto {
|
||||
*/
|
||||
@ApiPropertyOptional()
|
||||
@IsOptional()
|
||||
@NormalizeEmail()
|
||||
@IsEmail()
|
||||
email?: string;
|
||||
|
||||
|
||||
@@ -14,6 +14,7 @@ describe('UsersService', () => {
|
||||
user: {
|
||||
findMany: jest.Mock;
|
||||
findUnique: jest.Mock;
|
||||
findFirst: jest.Mock;
|
||||
create: jest.Mock;
|
||||
update: jest.Mock;
|
||||
count: jest.Mock;
|
||||
@@ -73,6 +74,11 @@ describe('UsersService', () => {
|
||||
user: {
|
||||
findMany: jest.fn(),
|
||||
findUnique: jest.fn(),
|
||||
// Email lookups are case-insensitive (findFirst); the tests stub
|
||||
// both lookups through findUnique.
|
||||
findFirst: jest.fn(
|
||||
(args: unknown): unknown => prisma.user.findUnique(args) as unknown,
|
||||
),
|
||||
create: jest.fn(),
|
||||
update: jest.fn(),
|
||||
count: jest.fn(),
|
||||
|
||||
@@ -169,8 +169,33 @@ export class UsersService {
|
||||
return this.toSummary(await this.findById(id));
|
||||
}
|
||||
|
||||
/** Case-insensitive: "[email protected]" and "[email protected]" are one account. */
|
||||
async findByEmail(email: string): Promise<UserWithRole | null> {
|
||||
return this.prisma.user.findUnique({ where: { email }, ...USER_WITH_ROLE });
|
||||
return this.prisma.user.findFirst({
|
||||
where: { email: { equals: email.trim(), mode: 'insensitive' } },
|
||||
...USER_WITH_ROLE,
|
||||
});
|
||||
}
|
||||
|
||||
async findBySsoSubject(ssoSubject: string): Promise<UserWithRole | null> {
|
||||
return this.prisma.user.findUnique({
|
||||
where: { ssoSubject },
|
||||
...USER_WITH_ROLE,
|
||||
});
|
||||
}
|
||||
|
||||
async linkSsoSubject(userId: string, ssoSubject: string): Promise<void> {
|
||||
await this.prisma.user.update({
|
||||
where: { id: userId },
|
||||
data: { ssoSubject },
|
||||
});
|
||||
await this.auditLog.record({
|
||||
entityType: 'User',
|
||||
entityId: userId,
|
||||
action: 'UPDATE',
|
||||
userId,
|
||||
afterData: { ssoLinked: true },
|
||||
});
|
||||
}
|
||||
|
||||
async create(
|
||||
@@ -233,6 +258,9 @@ export class UsersService {
|
||||
name: dto.name,
|
||||
email: dto.email,
|
||||
roleId: dto.roleId,
|
||||
// A new email is a new identity: the next Microsoft sign-in links
|
||||
// afresh instead of reusing the old account's binding.
|
||||
...(emailChanged ? { ssoSubject: null } : {}),
|
||||
},
|
||||
...USER_WITH_ROLE,
|
||||
});
|
||||
|
||||
Reference in new issue
Block a user