From a25265b88a32f972b3cc0d27c2dc1d98ec017384 Mon Sep 17 00:00:00 2001 From: Nick Szaunig Date: Thu, 1 Oct 2026 01:20:49 +0200 Subject: [PATCH] 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 "|" (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 Imker@x.de and imker@x.de 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) --- .../migration.sql | 15 ++++ ApiCore-Backend/prisma/schema.prisma | 5 ++ ApiCore-Backend/src/auth/dto/login.dto.ts | 2 + .../src/auth/sso/sso.service.spec.ts | 90 +++++++++++++++++-- ApiCore-Backend/src/auth/sso/sso.service.ts | 77 +++++++++++++--- .../src/common/validation/normalize-email.ts | 12 +++ .../src/profile/dto/update-profile.dto.ts | 2 + .../src/profile/profile.service.spec.ts | 11 ++- .../src/profile/profile.service.ts | 4 +- .../src/users/dto/create-user.dto.ts | 2 + .../src/users/dto/update-user.dto.ts | 2 + .../src/users/users.service.spec.ts | 6 ++ ApiCore-Backend/src/users/users.service.ts | 30 ++++++- 13 files changed, 239 insertions(+), 19 deletions(-) create mode 100644 ApiCore-Backend/prisma/migrations/20261001090000_sso_subject_and_email_case/migration.sql create mode 100644 ApiCore-Backend/src/common/validation/normalize-email.ts diff --git a/ApiCore-Backend/prisma/migrations/20261001090000_sso_subject_and_email_case/migration.sql b/ApiCore-Backend/prisma/migrations/20261001090000_sso_subject_and_email_case/migration.sql new file mode 100644 index 0000000..3a86a82 --- /dev/null +++ b/ApiCore-Backend/prisma/migrations/20261001090000_sso_subject_and_email_case/migration.sql @@ -0,0 +1,15 @@ +-- Microsoft sign-in is bound to the IdP identity ("|"), 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") + ); diff --git a/ApiCore-Backend/prisma/schema.prisma b/ApiCore-Backend/prisma/schema.prisma index e410a8d..3b6efcd 100644 --- a/ApiCore-Backend/prisma/schema.prisma +++ b/ApiCore-Backend/prisma/schema.prisma @@ -88,6 +88,11 @@ model User { // nach einer Passwortaenderung nicht bis zu 15 Minuten weiter funktioniert. passwordChangedAt DateTime? + // Unveraenderliche Identitaet beim Identity Provider ("|"), + // 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[] diff --git a/ApiCore-Backend/src/auth/dto/login.dto.ts b/ApiCore-Backend/src/auth/dto/login.dto.ts index 72222dd..90af27a 100644 --- a/ApiCore-Backend/src/auth/dto/login.dto.ts +++ b/ApiCore-Backend/src/auth/dto/login.dto.ts @@ -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; diff --git a/ApiCore-Backend/src/auth/sso/sso.service.spec.ts b/ApiCore-Backend/src/auth/sso/sso.service.spec.ts index fe99a26..7e33e7c 100644 --- a/ApiCore-Backend/src/auth/sso/sso.service.spec.ts +++ b/ApiCore-Backend/src/auth/sso/sso.service.spec.ts @@ -18,9 +18,26 @@ jest.mock('openid-client', () => ({ const mockedClient = client as jest.Mocked; +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) { + return { + claims: () => claims, + } as unknown as Awaited>; +} + 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 >); @@ -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: 'ghost@example.com' }), + claims: () => ({ ...IDENTITY, email: 'ghost@example.com' }), } as unknown as Awaited< ReturnType >); @@ -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: 'imker@example.com' }), + claims: () => ({ + ...IDENTITY, + preferred_username: 'imker@example.com', + }), } as unknown as Awaited< ReturnType >); @@ -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: 'renamed@example.com' }), + ); + 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: 'imker@example.com', + }), + ); + 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(); }); }); diff --git a/ApiCore-Backend/src/auth/sso/sso.service.ts b/ApiCore-Backend/src/auth/sso/sso.service.ts index ea1cadf..152cc21 100644 --- a/ApiCore-Backend/src/auth/sso/sso.service.ts +++ b/ApiCore-Backend/src/auth/sso/sso.service.ts @@ -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 { @@ -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 } { diff --git a/ApiCore-Backend/src/common/validation/normalize-email.ts b/ApiCore-Backend/src/common/validation/normalize-email.ts new file mode 100644 index 0000000..e4021bd --- /dev/null +++ b/ApiCore-Backend/src/common/validation/normalize-email.ts @@ -0,0 +1,12 @@ +import { Transform } from 'class-transformer'; + +/** + * Emails are compared case-insensitively (see UsersService.findByEmail); + * storing them trimmed and lower-case keeps "Imker@x.de" and "imker@x.de" + * from becoming two accounts. + */ +export function NormalizeEmail(): PropertyDecorator { + return Transform(({ value }: { value: unknown }) => + typeof value === 'string' ? value.trim().toLowerCase() : value, + ); +} diff --git a/ApiCore-Backend/src/profile/dto/update-profile.dto.ts b/ApiCore-Backend/src/profile/dto/update-profile.dto.ts index 696387d..3eaefdf 100644 --- a/ApiCore-Backend/src/profile/dto/update-profile.dto.ts +++ b/ApiCore-Backend/src/profile/dto/update-profile.dto.ts @@ -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; diff --git a/ApiCore-Backend/src/profile/profile.service.spec.ts b/ApiCore-Backend/src/profile/profile.service.spec.ts index 3f8fe41..96c8577 100644 --- a/ApiCore-Backend/src/profile/profile.service.spec.ts +++ b/ApiCore-Backend/src/profile/profile.service.spec.ts @@ -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) }; diff --git a/ApiCore-Backend/src/profile/profile.service.ts b/ApiCore-Backend/src/profile/profile.service.ts index 08b327a..f60e4da 100644 --- a/ApiCore-Backend/src/profile/profile.service.ts +++ b/ApiCore-Backend/src/profile/profile.service.ts @@ -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'); diff --git a/ApiCore-Backend/src/users/dto/create-user.dto.ts b/ApiCore-Backend/src/users/dto/create-user.dto.ts index 81fae08..5cf3259 100644 --- a/ApiCore-Backend/src/users/dto/create-user.dto.ts +++ b/ApiCore-Backend/src/users/dto/create-user.dto.ts @@ -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; diff --git a/ApiCore-Backend/src/users/dto/update-user.dto.ts b/ApiCore-Backend/src/users/dto/update-user.dto.ts index b760703..e23818f 100644 --- a/ApiCore-Backend/src/users/dto/update-user.dto.ts +++ b/ApiCore-Backend/src/users/dto/update-user.dto.ts @@ -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; diff --git a/ApiCore-Backend/src/users/users.service.spec.ts b/ApiCore-Backend/src/users/users.service.spec.ts index 38e2b3b..141e80d 100644 --- a/ApiCore-Backend/src/users/users.service.spec.ts +++ b/ApiCore-Backend/src/users/users.service.spec.ts @@ -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(), diff --git a/ApiCore-Backend/src/users/users.service.ts b/ApiCore-Backend/src/users/users.service.ts index 86d6166..636b493 100644 --- a/ApiCore-Backend/src/users/users.service.ts +++ b/ApiCore-Backend/src/users/users.service.ts @@ -169,8 +169,33 @@ export class UsersService { return this.toSummary(await this.findById(id)); } + /** Case-insensitive: "Imker@x.de" and "imker@x.de" are one account. */ async findByEmail(email: string): Promise { - 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 { + return this.prisma.user.findUnique({ + where: { ssoSubject }, + ...USER_WITH_ROLE, + }); + } + + async linkSsoSubject(userId: string, ssoSubject: string): Promise { + 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, });