diff --git a/.claude/agent-memory/qa-integration-tester/environment-setup.md b/.claude/agent-memory/qa-integration-tester/environment-setup.md index bb010e0d4..bbb3a9d59 100644 --- a/.claude/agent-memory/qa-integration-tester/environment-setup.md +++ b/.claude/agent-memory/qa-integration-tester/environment-setup.md @@ -70,3 +70,15 @@ NODE_PATH=/path/to/cornerstone/server/node_modules:/path/to/cornerstone/client/n → After changing `shared/src/types/`, rebuild main project's dist OR copy worktree dist files: `cp -r /worktree/shared/dist /path/to/cornerstone/shared/` - Server tests (better-sqlite3 native binary) may SIGKILL on ARM64 sandbox — validate via CI if needed + +## LoginPage.test.tsx — pre-existing local-only failures (verified not caused by new tests, 2026-07-21) + +Running `npx jest client/src/pages/LoginPage/LoginPage.test.tsx --coverage --maxWorkers=1` locally on Node 24.18.0 +in this worktree shows 9 of 18 tests failing with `expect(mockGetAuthMe).toHaveBeenCalled()` receiving 0 calls, +plus `console.error: An update to LoginPage/AuthProvider inside a test was not wrapped in act(...)`. Confirmed via +`git stash` that this reproduces identically on the pre-existing baseline (8/17 failing before any edit) — it is +NOT caused by adding a new test and is not a regression. Root cause looks like an async-timing/jsdom quirk with +the `getAuthMe` mock resolving after the effect's cleanup in this sandbox; all 7 `oidc*`/error-code tests +(the simple render + findByText ones) reliably PASS regardless. Coverage numbers from a run with these failures +are not representative of true coverage — treat CI's coverage-report artifact as authoritative for this file, +not a local run. diff --git a/client/src/i18n/de/auth.json b/client/src/i18n/de/auth.json index 0d73c980c..68cbbac89 100644 --- a/client/src/i18n/de/auth.json +++ b/client/src/i18n/de/auth.json @@ -14,7 +14,8 @@ "invalid_state": "Authentifizierungssitzung abgelaufen. Bitte versuchen Sie es erneut.", "missing_email": "Ihr Identitätsanbieter hat keine E-Mail-Adresse bereitgestellt.", "email_conflict": "Diese E-Mail-Adresse ist bereits mit einem anderen Konto verknüpft.", - "account_deactivated": "Ihr Konto wurde deaktiviert. Bitte wenden Sie sich an einen Administrator." + "account_deactivated": "Ihr Konto wurde deaktiviert. Bitte wenden Sie sich an einen Administrator.", + "oidc_no_matching_account": "Für Ihre E-Mail-Adresse wurde kein Konto gefunden. Bitte wenden Sie sich an einen Administrator, damit zunächst ein Konto für Sie angelegt wird." }, "validation": { "emailRequired": "E-Mail ist erforderlich", diff --git a/client/src/i18n/en/auth.json b/client/src/i18n/en/auth.json index 25870b8b3..2c5ea0336 100644 --- a/client/src/i18n/en/auth.json +++ b/client/src/i18n/en/auth.json @@ -14,7 +14,8 @@ "invalid_state": "Authentication session expired. Please try again.", "missing_email": "Your identity provider did not provide an email address.", "email_conflict": "This email is already associated with a different account.", - "account_deactivated": "Your account has been deactivated. Please contact an administrator." + "account_deactivated": "Your account has been deactivated. Please contact an administrator.", + "oidc_no_matching_account": "No account was found for your email address. Ask an administrator to create an account for you first." }, "validation": { "emailRequired": "Email is required", diff --git a/client/src/pages/LoginPage/LoginPage.test.tsx b/client/src/pages/LoginPage/LoginPage.test.tsx index 16bb97412..7dcd07a21 100644 --- a/client/src/pages/LoginPage/LoginPage.test.tsx +++ b/client/src/pages/LoginPage/LoginPage.test.tsx @@ -158,6 +158,16 @@ describe('LoginPage', () => { expect(await screen.findByText(/your account has been deactivated/i)).toBeInTheDocument(); }); + it('shows OIDC error message from URL query parameter (oidc_no_matching_account)', async () => { + window.history.pushState({}, '', '/login?error=oidc_no_matching_account'); + + renderWithAuth(); + + expect( + await screen.findByText(/no account was found for your email address/i), + ).toBeInTheDocument(); + }); + it('does not show error message when no error in URL', async () => { renderWithAuth(); diff --git a/client/src/pages/LoginPage/LoginPage.tsx b/client/src/pages/LoginPage/LoginPage.tsx index 6ac6d2988..1f160ec60 100644 --- a/client/src/pages/LoginPage/LoginPage.tsx +++ b/client/src/pages/LoginPage/LoginPage.tsx @@ -52,6 +52,7 @@ export function LoginPage() { 'missing_email', 'email_conflict', 'account_deactivated', + 'oidc_no_matching_account', ]; if (knownCodes.includes(errorCode)) { /* eslint-disable @eslint-react/set-state-in-effect -- initializing error state from url params */ diff --git a/e2e/fixtures/testData.ts b/e2e/fixtures/testData.ts index eae6d6d4d..aa323773f 100644 --- a/e2e/fixtures/testData.ts +++ b/e2e/fixtures/testData.ts @@ -11,7 +11,12 @@ export const TEST_ADMIN = { export const TEST_MEMBER = { email: 'member@e2e-test.local', displayName: 'E2E Member', - // Created via OIDC flow, no local password + // Pre-created as a local account (via the admin-authenticated POST /api/users + // API — see the `beforeAll` seed step in oidc.spec.ts), then linked to the + // mock OIDC provider's fixed identity on first SSO login. OIDC is purely an + // alternate login method for an already-existing account: authProvider stays + // 'local' after linking, and this password keeps working alongside SSO. + localPassword: 'e2e-member-password-456!', }; export const ROUTES = { diff --git a/e2e/tests/auth/oidc.spec.ts b/e2e/tests/auth/oidc.spec.ts index 357c8953e..7b508651b 100644 --- a/e2e/tests/auth/oidc.spec.ts +++ b/e2e/tests/auth/oidc.spec.ts @@ -1,5 +1,5 @@ /** - * E2E tests for OIDC SSO flow (Stories #34, #35) + * E2E tests for OIDC SSO flow (Stories #34, #35; account-linking fix #1865) * * These tests verify the full OIDC authentication flow using a mock OIDC provider. * The Cornerstone server communicates with the OIDC provider via Docker network alias @@ -9,20 +9,49 @@ * redirects from the Docker network alias (http://oidc-server:8080/...) to * browser-accessible URLs (/oidc-proxy/...). The proxy forwards /oidc-proxy/* requests * to the OIDC server with Host: oidc-server:8080 to ensure the issuer claim matches. + * + * As of #1865, OIDC login is purely an alternate login method for accounts that + * ALREADY EXIST — it never creates a new account. The mock OIDC provider + * (e2e/containers/oidcContainer.ts) is hardcoded to always return a fixed identity + * (member@e2e-test.local). For the happy path below, that identity must already + * exist as a local account (created via the admin API in `beforeAll`) so the first + * SSO login can link it; subsequent logins are resolved by the linked oidcSubject. */ import { test, expect } from '@playwright/test'; import { LoginPage } from '../../pages/LoginPage.js'; import { UserManagementPage } from '../../pages/UserManagementPage.js'; +import { createLocalUserViaApi } from '../../fixtures/apiHelpers.js'; import { TEST_MEMBER, ROUTES, API } from '../../fixtures/testData.js'; -// Serial mode: test 3 creates the OIDC user, tests 4 and 5 verify it +// Serial mode: beforeAll seeds the local account that gets linked on first SSO +// login (test 3); tests 4 and 5 verify the linked account's state. test.describe.configure({ mode: 'serial' }); test.describe('OIDC SSO Flow', () => { // Unauthenticated context for OIDC login tests test.use({ storageState: { cookies: [], origins: [] } }); + test.beforeAll(async ({ browser }) => { + // Seed TEST_MEMBER as a local account via the admin-authenticated API. + // The mock OIDC provider always returns this email/identity, and (post + // #1865) OIDC can only link/log in to an account that already exists — + // it never creates one. Use an explicit admin-authenticated context here + // (rather than the `authenticatedPage` fixture) since `beforeAll` hooks + // don't participate in the describe-level `storageState` override above. + const adminContext = await browser.newContext({ + storageState: 'test-results/.auth/admin.json', + }); + const adminPage = await adminContext.newPage(); + await createLocalUserViaApi(adminPage, { + email: TEST_MEMBER.email, + displayName: TEST_MEMBER.displayName, + password: TEST_MEMBER.localPassword, + role: 'member', + }); + await adminContext.close(); + }); + test('Login page shows SSO button when OIDC is enabled', async ({ page }) => { const loginPage = new LoginPage(page); @@ -55,10 +84,11 @@ test.describe('OIDC SSO Flow', () => { test('Full OIDC flow creates session', async ({ page }) => { const loginPage = new LoginPage(page); - // Given: User is on the login page + // Given: TEST_MEMBER already exists as a local account (seeded in beforeAll) await loginPage.goto(); - // When: User completes the OIDC flow + // When: User completes the OIDC flow — this links the local account to the + // OIDC identity (oidcSubject is set; authProvider stays 'local') await loginPage.clickSSO(); // Then: User should be redirected to dashboard (session created) @@ -73,16 +103,18 @@ test.describe('OIDC SSO Flow', () => { expect(me.user.email).toBe(TEST_MEMBER.email); }); - test('Auto-provisioned OIDC user has correct attributes', async ({ page }) => { + test('Linked OIDC user retains local account attributes', async ({ page }) => { const loginPage = new LoginPage(page); - // Given: OIDC user was created in the previous test - // When: User logs in via OIDC again + // Given: The local account was linked to the OIDC identity in the previous test + // When: User logs in via OIDC again — resolved directly by the linked oidcSubject await loginPage.goto(); await loginPage.clickSSO(); await expect(page).toHaveURL(ROUTES.home, { timeout: 15000 }); - // Then: /api/auth/me should return correct user details + // Then: /api/auth/me should return correct user details. + // Critically, authProvider stays 'local' — linking an existing account via + // OIDC never changes its auth provider or removes its password. const meResponse = await page.request.get(API.authMe); expect(meResponse.ok()).toBe(true); const me = await meResponse.json(); @@ -90,13 +122,14 @@ test.describe('OIDC SSO Flow', () => { expect(me.user.email).toBe(TEST_MEMBER.email); expect(me.user.displayName).toBe(TEST_MEMBER.displayName); expect(me.user.role).toBe('member'); - expect(me.user.authProvider).toBe('oidc'); + expect(me.user.authProvider).toBe('local'); }); test('OIDC user appears in admin user management', async ({ page, browser }) => { const loginPage = new LoginPage(page); - // First: Complete OIDC login to ensure user exists + // First: Complete OIDC login (the account was already seeded + linked by + // earlier tests in this serial suite; this just re-confirms the session) await loginPage.goto(); await loginPage.clickSSO(); await expect(page).toHaveURL(ROUTES.home, { timeout: 15000 }); @@ -127,4 +160,56 @@ test.describe('OIDC SSO Flow', () => { await adminContext.close(); }); + + // ─────────────────────────────────────────────────────────────────────────── + // Rejection path (#1865): OIDC email matches no existing account + // + // Coverage note: the mock OIDC provider (e2e/containers/oidcContainer.ts) is + // hardcoded to a single fixed identity (member@e2e-test.local), which by this + // point in the serial suite above is already a linked local account. Getting + // a genuinely SECOND, never-seeded identity out of the real container would + // require either an interactive-login form-fill step (changing the flow shape + // of every other test in this file, which all rely on non-interactive + // auto-grant) or an unverifiable per-request claim-mapping trick — and the + // token exchange happens server-to-server over the Docker network, so + // page.route() in the browser cannot intercept/vary it either. Given that + // constraint, this test exercises the same URL-param -> translated-banner + // code path a real rejection redirect produces (LoginPage.tsx reads + // `?error=` on mount), without depending on a second container identity. + // The real backend rejection path (403 OidcNoMatchingAccountError, no user + // row created) is covered by qa-integration-tester's route-level test. + test('Login page shows the rejection banner for oidc_no_matching_account and creates no account', async ({ + page, + browser, + }) => { + const loginPage = new LoginPage(page); + const unmatchedEmail = 'no-such-oidc-user@e2e-test.local'; + + // When: Browser lands on /login with the rejection error code — exactly + // the redirect target the server uses for OidcNoMatchingAccountError + await page.goto(`${ROUTES.login}?error=oidc_no_matching_account`); + + // Then: Browser stays on /login with the error code in the URL + await expect(page).toHaveURL(/\/login\?error=oidc_no_matching_account/); + + // And: The translated rejection message is visible + await expect(loginPage.errorBanner).toBeVisible(); + await expect(loginPage.errorBanner).toContainText( + 'No account was found for your email address', + ); + + // And: No account exists for an email that was never provisioned — sanity + // check that rejection never has the side effect of creating a user + const adminContext = await browser.newContext({ + storageState: 'test-results/.auth/admin.json', + }); + const adminPage = await adminContext.newPage(); + const usersResponse = await adminPage.request.get( + `${API.users}?q=${encodeURIComponent(unmatchedEmail)}`, + ); + expect(usersResponse.ok()).toBe(true); + const { users } = (await usersResponse.json()) as { users: { email: string }[] }; + expect(users.find((u) => u.email === unmatchedEmail)).toBeUndefined(); + await adminContext.close(); + }); }); diff --git a/server/src/errors/AppError.ts b/server/src/errors/AppError.ts index d37ecdcca..a5ef8c5fc 100644 --- a/server/src/errors/AppError.ts +++ b/server/src/errors/AppError.ts @@ -183,6 +183,13 @@ export class AccountLockedError extends AppError { } } +export class OidcNoMatchingAccountError extends AppError { + constructor(message = 'No existing account matches this identity provider email address') { + super('OIDC_NO_MATCHING_ACCOUNT', 403, message); + this.name = 'OidcNoMatchingAccountError'; + } +} + export class InvalidMetadataError extends AppError { constructor(message = 'Metadata does not match schema for the entry type') { super('INVALID_METADATA', 400, message); diff --git a/server/src/routes/oidc.test.ts b/server/src/routes/oidc.test.ts index e5b9861ae..3d4a6ccb8 100644 --- a/server/src/routes/oidc.test.ts +++ b/server/src/routes/oidc.test.ts @@ -1,9 +1,53 @@ -import { describe, it, expect, beforeEach, afterEach } from '@jest/globals'; +/** + * Integration tests for /api/auth/oidc/* route handlers. + * + * Covers: + * - Basic short-circuit behavior when OIDC is not configured + * - OIDC config validation (oidcEnabled derivation) + * - Issue #1865: account-linking behavior on /api/auth/oidc/callback — + * successful login via email match, rejection when no account matches, + * and the deactivated-account redirect still firing post-link. + * + * Strategy: + * - oidcService is fully mocked (discoverOidcConfig, buildAuthorizationUrl, + * consumeState, handleCallback) so the callback route can be driven via + * app.inject() without a real OIDC provider. + * - buildApp() + app.inject() for HTTP layer validation. + */ + +import { jest, describe, it, expect, beforeEach, afterEach } from '@jest/globals'; import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import type { FastifyInstance } from 'fastify'; -import { buildApp } from '../app.js'; +import type * as AppModule from '../app.js'; +import type * as UserServiceModule from '../services/userService.js'; + +// ─── Mock oidcService BEFORE importing app ───────────────────────────────── + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +type AnyMock = jest.MockedFunction<(...args: any[]) => any>; + +const mockDiscoverOidcConfig = jest.fn() as AnyMock; +const mockBuildAuthorizationUrl = jest.fn() as AnyMock; +const mockConsumeState = jest.fn() as AnyMock; +const mockHandleCallback = jest.fn() as AnyMock; +const mockStoreState = jest.fn() as AnyMock; +const mockResetCache = jest.fn() as AnyMock; + +jest.unstable_mockModule('../services/oidcService.js', () => ({ + discoverOidcConfig: mockDiscoverOidcConfig, + buildAuthorizationUrl: mockBuildAuthorizationUrl, + consumeState: mockConsumeState, + handleCallback: mockHandleCallback, + storeState: mockStoreState, + resetCache: mockResetCache, +})); + +// ─── Dynamic imports (after mocks) ───────────────────────────────────────── + +let buildApp: typeof AppModule.buildApp; +let userService: typeof UserServiceModule; describe('OIDC Routes', () => { let app: FastifyInstance; @@ -17,12 +61,22 @@ describe('OIDC Routes', () => { // Create temporary directory for test database tempDir = mkdtempSync(join(tmpdir(), 'cornerstone-oidc-routes-test-')); process.env.DATABASE_URL = join(tempDir, 'test.db'); + process.env.SECURE_COOKIES = 'false'; // Disable OIDC by default (tests will enable when needed) delete process.env.OIDC_ISSUER; delete process.env.OIDC_CLIENT_ID; delete process.env.OIDC_CLIENT_SECRET; delete process.env.EXTERNAL_URL; + + // Import modules after mocks are set up (only once) + if (!buildApp) { + buildApp = (await import('../app.js')).buildApp; + userService = await import('../services/userService.js'); + } + + // Reset mocks before each test + jest.clearAllMocks(); }); afterEach(async () => { @@ -93,12 +147,11 @@ describe('OIDC Routes', () => { expect(response.headers.location).toBe('/login?error=oidc_not_configured'); }); - // NOTE: Deep callback error paths (error parameter, missing/invalid state, missing code) - // cannot be tested as integration tests when OIDC is disabled, because the route immediately - // returns oidc_not_configured before checking anything else. These error paths are covered - // by unit tests in oidcService.test.ts and would require a fully configured OIDC environment - // (with real issuer, client ID, secret, redirect URI) to test here. Since this suite tests - // with OIDC disabled, we only verify the oidc_not_configured path. + // NOTE: Deep callback error paths that depend on state/code validation (error parameter, + // missing/invalid state) still require a fully configured OIDC environment to reach — see + // the "account linking" describe block below, which configures OIDC and mocks oidcService + // so the rest of the callback flow (account resolution, linking, deactivation) can be + // exercised via app.inject(). }); describe('OIDC Configuration Validation', () => { @@ -134,4 +187,107 @@ describe('OIDC Routes', () => { expect(app.config.oidcEnabled).toBe(true); }); }); + + describe('GET /api/auth/oidc/callback — account linking (issue #1865)', () => { + beforeEach(async () => { + // Enable OIDC for this describe block + process.env.OIDC_ISSUER = 'https://oidc.example.com'; + process.env.OIDC_CLIENT_ID = 'client-123'; + process.env.OIDC_CLIENT_SECRET = 'secret-456'; + + app = await buildApp(); + + // Default mock behavior: discovery succeeds, state resolves to app root + mockDiscoverOidcConfig.mockResolvedValue({}); + mockConsumeState.mockReturnValue('/'); + }); + + it('logs in successfully when the OIDC email matches an existing local account', async () => { + // Given: A pre-existing local account + const user = await userService.createLocalUser( + app.db, + 'match@example.com', + 'Match User', + 'password123456', + ); + mockConsumeState.mockReturnValue('/dashboard'); + mockHandleCallback.mockResolvedValue({ + sub: 'sub-match-1', + email: user.email, + name: 'Match User', + }); + + // When: The OIDC callback is invoked + const response = await app.inject({ + method: 'GET', + url: '/api/auth/oidc/callback?code=abc&state=xyz', + }); + + // Then: Redirects to the original app path (not an error redirect) + expect(response.statusCode).toBe(302); + expect(response.headers.location).toBe('/dashboard'); + + // And: A session cookie is set + const setCookieHeader = response.headers['set-cookie']; + const cookies = Array.isArray(setCookieHeader) ? setCookieHeader.join(';') : setCookieHeader; + expect(cookies).toContain('cornerstone_session='); + + // And: The account was linked (oidcSubject set), not re-created + const linkedUser = userService.findById(app.db, user.id); + expect(linkedUser?.oidcSubject).toBe('sub-match-1'); + expect(linkedUser?.authProvider).toBe('local'); + }); + + it('redirects to /login?error=oidc_no_matching_account when no account matches by email', async () => { + // Given: No account exists for this email + mockHandleCallback.mockResolvedValue({ + sub: 'sub-no-match', + email: 'nomatch@example.com', + name: 'No Match', + }); + + // When: The OIDC callback is invoked + const response = await app.inject({ + method: 'GET', + url: '/api/auth/oidc/callback?code=abc&state=xyz', + }); + + // Then: Redirects to exactly the no-matching-account error path + expect(response.statusCode).toBe(302); + expect(response.headers.location).toBe('/login?error=oidc_no_matching_account'); + + // And: No user was created + expect(userService.countUsers(app.db)).toBe(0); + }); + + it('redirects to /login?error=account_deactivated for a deactivated linked account', async () => { + // Given: A deactivated local account whose email matches the OIDC claim + const user = await userService.createLocalUser( + app.db, + 'deactivated@example.com', + 'Deactivated User', + 'password123456', + ); + userService.deactivateUser(app.db, user.id); + mockHandleCallback.mockResolvedValue({ + sub: 'sub-deactivated', + email: user.email, + name: 'Deactivated User', + }); + + // When: The OIDC callback is invoked + const response = await app.inject({ + method: 'GET', + url: '/api/auth/oidc/callback?code=abc&state=xyz', + }); + + // Then: Redirects to the deactivated-account error path (post-link — the account is + // still linked even though login is rejected) + expect(response.statusCode).toBe(302); + expect(response.headers.location).toBe('/login?error=account_deactivated'); + + const linkedUser = userService.findById(app.db, user.id); + expect(linkedUser?.oidcSubject).toBe('sub-deactivated'); + }); + }); }); diff --git a/server/src/routes/oidc.ts b/server/src/routes/oidc.ts index 4bffd591e..5c6a0bc85 100644 --- a/server/src/routes/oidc.ts +++ b/server/src/routes/oidc.ts @@ -1,5 +1,5 @@ import type { FastifyInstance } from 'fastify'; -import { AppError, ConflictError } from '../errors/AppError.js'; +import { AppError, OidcNoMatchingAccountError } from '../errors/AppError.js'; import * as oidcService from '../services/oidcService.js'; import * as userService from '../services/userService.js'; import * as sessionService from '../services/sessionService.js'; @@ -92,6 +92,7 @@ export default async function oidcRoutes(fastify: FastifyInstance) { return reply.redirect('/login?error=invalid_state'); } + let sub: string | undefined; try { // Discover OIDC configuration const config = await oidcService.discoverOidcConfig( @@ -106,7 +107,12 @@ export default async function oidcRoutes(fastify: FastifyInstance) { const callbackUrl = new URL(request.url, `${request.protocol}://${request.host}`); // Exchange code for tokens and extract claims - const { sub, email, name } = await oidcService.handleCallback(config, callbackUrl, state); + const { sub: subFromService, email } = await oidcService.handleCallback( + config, + callbackUrl, + state, + ); + sub = subFromService; // Ensure email is present if (!email) { @@ -114,14 +120,8 @@ export default async function oidcRoutes(fastify: FastifyInstance) { return reply.redirect('/login?error=missing_email'); } - // Find or create user - const user = userService.findOrCreateOidcUser( - fastify.db, - sub, - email, - name || email.split('@')[0]!, - // email.split('@')[0] is defined: email is non-empty and split always returns at least one element - ); + // Find or link user + const user = userService.findOrLinkOidcUser(fastify.db, sub, email); // Check if user is deactivated if (user.deactivatedAt) { @@ -148,10 +148,9 @@ export default async function oidcRoutes(fastify: FastifyInstance) { // Redirect to the original app path return reply.redirect(appRedirect); } catch (error) { - // Email conflict: OIDC user's email matches a different auth provider's user - if (error instanceof ConflictError) { - fastify.log.warn({ error }, 'OIDC email conflict'); - return reply.redirect('/login?error=email_conflict'); + if (error instanceof OidcNoMatchingAccountError) { + fastify.log.warn({ error, sub }, 'OIDC login rejected: no matching account for email'); + return reply.redirect('/login?error=oidc_no_matching_account'); } fastify.log.error({ error }, 'OIDC callback error'); return reply.redirect('/login?error=oidc_error'); diff --git a/server/src/services/userService.test.ts b/server/src/services/userService.test.ts index ead1ed309..e775eee60 100644 --- a/server/src/services/userService.test.ts +++ b/server/src/services/userService.test.ts @@ -7,6 +7,7 @@ import { runMigrations } from '../db/migrate.js'; import * as schema from '../db/schema.js'; import * as userService from './userService.js'; import { users } from '../db/schema.js'; +import { OidcNoMatchingAccountError } from '../errors/AppError.js'; describe('User Service', () => { let sqlite: Database.Database; @@ -682,82 +683,50 @@ describe('User Service', () => { expect(foundUser).toBeUndefined(); }); - it('does not match local users (auth_provider=local)', async () => { - // Given: Local user in database (no oidcSubject) + it('does not match a local user whose oidcSubject is still null', async () => { + // Given: Local user in database with no linked oidcSubject await userService.createLocalUser(db, 'local@example.com', 'Local User', 'password123456'); // When: Finding by any OIDC subject const foundUser = userService.findByOidcSubject(db, 'any-oidc-sub'); - // Then: No user is found + // Then: No user is found — not because of an authProvider filter, but because + // oidcSubject is null for this row (the column itself doesn't match). expect(foundUser).toBeUndefined(); }); - it('only matches users with auth_provider=oidc', () => { - // Given: OIDC user and local user in database - const oidcSubject = 'oidc-sub-456'; - - db.insert(schema.users) - .values({ - id: 'oidc-user-2', - email: 'oidc2@example.com', - displayName: 'OIDC User Two', - role: 'member', - authProvider: 'oidc', - oidcSubject, - createdAt: new Date().toISOString(), - updatedAt: new Date().toISOString(), - }) - .run(); + it('matches a local user that has a linked oidcSubject', async () => { + // Given: A local-auth account that has already been linked to an OIDC identity + // (the key regression scenario for issue #1865 — findByOidcSubject must match on + // oidcSubject alone, regardless of authProvider). + const sub = 'linked-oidc-sub-789'; + const localUser = await userService.createLocalUser( + db, + 'linked-local@example.com', + 'Linked Local User', + 'password123456', + ); - db.insert(schema.users) - .values({ - id: 'local-user-2', - email: 'local2@example.com', - displayName: 'Local User Two', - role: 'member', - authProvider: 'local', - passwordHash: '$scrypt$n=16384,r=8,p=1$c29tZXNhbHQ=$c29tZWhhc2g=', - createdAt: new Date().toISOString(), - updatedAt: new Date().toISOString(), - }) + db.update(schema.users) + .set({ oidcSubject: sub }) + .where(eq(schema.users.id, localUser.id)) .run(); - // When: Finding by OIDC subject - const foundUser = userService.findByOidcSubject(db, oidcSubject); + // When: Finding by the linked OIDC subject + const foundUser = userService.findByOidcSubject(db, sub); - // Then: Only OIDC user is found + // Then: The local-auth user is found expect(foundUser).toBeDefined(); - expect(foundUser?.authProvider).toBe('oidc'); - expect(foundUser?.id).toBe('oidc-user-2'); + expect(foundUser?.id).toBe(localUser.id); + expect(foundUser?.authProvider).toBe('local'); + expect(foundUser?.passwordHash).toBe(localUser.passwordHash); + expect(foundUser?.oidcSubject).toBe(sub); }); }); - describe('findOrCreateOidcUser()', () => { - it('creates a new user when no matching OIDC user exists', () => { - // Given: Empty database - const sub = 'new-oidc-sub-123'; - const email = 'newoidc@example.com'; - const displayName = 'New OIDC User'; - - // When: Finding or creating OIDC user - const user = userService.findOrCreateOidcUser(db, sub, email, displayName); - - // Then: User is created - expect(user).toBeDefined(); - expect(user.oidcSubject).toBe(sub); - expect(user.email).toBe(email); - expect(user.displayName).toBe(displayName); - expect(user.authProvider).toBe('oidc'); - expect(user.role).toBe('member'); - expect(user.passwordHash).toBeNull(); - expect(user.createdAt).toBeDefined(); - expect(user.updatedAt).toBeDefined(); - expect(user.deactivatedAt).toBeNull(); - }); - - it('returns existing user when OIDC subject matches', () => { - // Given: Existing OIDC user + describe('findOrLinkOidcUser()', () => { + it('returns existing user when OIDC subject already linked', () => { + // Given: A user already linked to this OIDC subject const sub = 'existing-oidc-sub'; const email = 'existing@example.com'; const displayName = 'Existing User'; @@ -775,105 +744,111 @@ describe('User Service', () => { }) .run(); - // When: Finding or creating with same OIDC subject - const user = userService.findOrCreateOidcUser( - db, - sub, - 'different@example.com', - 'Different Name', - ); + // When: Calling findOrLinkOidcUser again with the same subject + const user = userService.findOrLinkOidcUser(db, sub, 'different@example.com'); - // Then: Existing user is returned (email and displayName not updated) - expect(user).toBeDefined(); + // Then: The same row is returned, unchanged (email is not re-matched/updated) expect(user.id).toBe('existing-oidc-user'); expect(user.oidcSubject).toBe(sub); - expect(user.email).toBe(email); // Original email - expect(user.displayName).toBe(displayName); // Original displayName - expect(user.role).toBe('admin'); // Original role + expect(user.email).toBe(email); + expect(user.displayName).toBe(displayName); + expect(user.role).toBe('admin'); + expect(user.updatedAt).toBe('2024-01-01T00:00:00.000Z'); }); - it('throws ConflictError when email is used by a different user (local)', async () => { - // Given: Local user with email - const email = 'conflict@example.com'; - await userService.createLocalUser(db, email, 'Local User', 'password123456'); + it('links an existing local account by email match on first OIDC login', async () => { + // Given: A pre-existing local password account, never used with OIDC before + const email = 'firstlogin@example.com'; + const localUser = await userService.createLocalUser( + db, + email, + 'First Login User', + 'password123456', + 'admin', + ); + expect(localUser.oidcSubject).toBeNull(); - // When/Then: Creating OIDC user with same email throws - expect(() => { - userService.findOrCreateOidcUser(db, 'new-oidc-sub', email, 'OIDC User'); - }).toThrow(userService.ConflictError); + const sub = 'first-login-sub'; - expect(() => { - userService.findOrCreateOidcUser(db, 'new-oidc-sub', email, 'OIDC User'); - }).toThrow('Email already in use by another account'); + // When: The first OIDC login for this identity arrives + const linked = userService.findOrLinkOidcUser(db, sub, email); + + // Then: The account is linked by setting oidcSubject only + expect(linked.id).toBe(localUser.id); + expect(linked.oidcSubject).toBe(sub); + expect(linked.authProvider).toBe('local'); + expect(linked.passwordHash).toBe(localUser.passwordHash); + expect(linked.email).toBe(localUser.email); + expect(linked.displayName).toBe(localUser.displayName); + expect(linked.role).toBe(localUser.role); }); - it('throws ConflictError when email is used by different OIDC user', () => { - // Given: Existing OIDC user with email - const email = 'oidc-conflict@example.com'; + it('a second OIDC login for a linked account is found by subject, not re-matched by email', async () => { + // Regression test for issue #1865: once linked, subsequent logins must resolve via + // oidcSubject, not re-run the email-match path. + const email = 'repeatlogin@example.com'; + const localUser = await userService.createLocalUser( + db, + email, + 'Repeat Login User', + 'password123456', + ); + const sub = 'repeat-login-sub'; - db.insert(schema.users) - .values({ - id: 'oidc-user-1', - email, - displayName: 'OIDC User One', - role: 'member', - authProvider: 'oidc', - oidcSubject: 'oidc-sub-1', - createdAt: new Date().toISOString(), - updatedAt: new Date().toISOString(), - }) - .run(); + const firstLogin = userService.findOrLinkOidcUser(db, sub, email); + expect(firstLogin.id).toBe(localUser.id); + + // When: The same identity logs in again + const secondLogin = userService.findOrLinkOidcUser(db, sub, email); - // When/Then: Creating different OIDC user with same email throws + // Then: The same user is returned without error + expect(secondLogin.id).toBe(localUser.id); + expect(secondLogin.oidcSubject).toBe(sub); + }); + + it('throws OidcNoMatchingAccountError when no account matches by subject or email', () => { + // Given: An empty/unrelated DB state — no account for this subject or email + // When/Then: findOrLinkOidcUser throws expect(() => { - userService.findOrCreateOidcUser(db, 'oidc-sub-2', email, 'OIDC User Two'); - }).toThrow(userService.ConflictError); + userService.findOrLinkOidcUser(db, 'unknown-sub', 'nobody@example.com'); + }).toThrow(OidcNoMatchingAccountError); }); - it('created user has correct defaults (role=member, authProvider=oidc)', () => { - // Given: New OIDC user details - const sub = 'default-test-sub'; - const email = 'defaults@example.com'; - const displayName = 'Defaults User'; + it('does not create a new user row when no account matches', () => { + // Given: The current user count (proves auto-provisioning is gone) + const countBefore = userService.countUsers(db); - // When: Creating OIDC user - const user = userService.findOrCreateOidcUser(db, sub, email, displayName); + // When: findOrLinkOidcUser is called with a non-matching sub/email + expect(() => { + userService.findOrLinkOidcUser(db, 'no-match-sub', 'no-match@example.com'); + }).toThrow(OidcNoMatchingAccountError); - // Then: Defaults are applied - expect(user.role).toBe('member'); - expect(user.authProvider).toBe('oidc'); - expect(user.passwordHash).toBeNull(); - expect(user.oidcSubject).toBe(sub); - expect(user.deactivatedAt).toBeNull(); + // Then: No new user row was created + const countAfter = userService.countUsers(db); + expect(countAfter).toBe(countBefore); }); - it('generated user ID is a valid UUID', () => { - // Given: New OIDC user - const user = userService.findOrCreateOidcUser( + it('does not modify authProvider, passwordHash, displayName, or role when linking', async () => { + // Given: A local account with distinctive field values + const email = 'fieldcheck@example.com'; + const localUser = await userService.createLocalUser( db, - 'uuid-test-sub', - 'uuid@example.com', - 'UUID User', + email, + 'Field Check User', + 'password123456', + 'admin', ); - // Then: ID is a valid UUID - expect(user.id).toMatch(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i); - }); - - it('sets timestamps for newly created user', () => { - // Given: New OIDC user - const user = userService.findOrCreateOidcUser( - db, - 'timestamp-test-sub', - 'timestamp@example.com', - 'Timestamp User', - ); + // When: Linking an OIDC identity to this account + const linked = userService.findOrLinkOidcUser(db, 'field-check-sub', email); - // Then: Timestamps are set - expect(user.createdAt).toBeDefined(); - expect(user.updatedAt).toBeDefined(); - expect(user.createdAt).toMatch(/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/); - expect(user.updatedAt).toBe(user.createdAt); + // Then: Only oidcSubject and updatedAt were touched — everything else is untouched + expect(linked.authProvider).toBe(localUser.authProvider); + expect(linked.authProvider).toBe('local'); + expect(linked.passwordHash).toBe(localUser.passwordHash); + expect(linked.displayName).toBe(localUser.displayName); + expect(linked.role).toBe(localUser.role); + expect(linked.role).toBe('admin'); }); }); diff --git a/server/src/services/userService.ts b/server/src/services/userService.ts index cf6449093..7152e8a20 100644 --- a/server/src/services/userService.ts +++ b/server/src/services/userService.ts @@ -6,7 +6,7 @@ import type { BetterSQLite3Database } from 'drizzle-orm/better-sqlite3'; import type * as schemaTypes from '../db/schema.js'; import { users } from '../db/schema.js'; import type { UserResponse } from '@cornerstone/shared'; -import { ConflictError } from '../errors/AppError.js'; +import { ConflictError, OidcNoMatchingAccountError } from '../errors/AppError.js'; // Re-export ConflictError for tests export { ConflictError }; @@ -178,69 +178,58 @@ export function countActiveUsers(db: DbType): number { } /** - * Find a user by OIDC subject. + * Find a user by OIDC subject, regardless of how the account was created + * (local password account that was later linked, or a legacy OIDC-created + * account). `oidcSubject` alone is the correlation key. * * @param db - Database instance * @param sub - OIDC subject identifier * @returns User row or undefined if not found */ export function findByOidcSubject(db: DbType, sub: string): typeof users.$inferSelect | undefined { - return db - .select() - .from(users) - .where(and(eq(users.authProvider, 'oidc'), eq(users.oidcSubject, sub))) - .get(); + return db.select().from(users).where(eq(users.oidcSubject, sub)).get(); } /** - * Find a user by OIDC subject or create a new one. + * Find the local account matching an OIDC identity, linking it on first + * successful login. + * + * OIDC is exclusively an alternate login method for accounts that already + * exist — it never creates a new account. Resolution order: + * 1. If `sub` is already linked to an account, return it. + * 2. Otherwise, if an account exists whose email matches the verified + * email claim, link this OIDC subject to that account and return it. + * `authProvider` and `passwordHash` are left untouched. + * 3. Otherwise, no account can be authorized for this identity. * * @param db - Database instance * @param sub - OIDC subject identifier * @param email - User email address - * @param displayName - User display name - * @returns The user row (existing or newly created) - * @throws ConflictError if email is already in use by another account + * @returns The user row (existing or linked) + * @throws OidcNoMatchingAccountError if no account exists for this email */ -export function findOrCreateOidcUser( +export function findOrLinkOidcUser( db: DbType, sub: string, email: string, - displayName: string, ): typeof users.$inferSelect { - // First, try to find by OIDC subject const existingUser = findByOidcSubject(db, sub); if (existingUser) { return existingUser; } - // Check if email is already used by another user const emailUser = findByEmail(db, email); - if (emailUser) { - throw new ConflictError('Email already in use by another account', { - email, - }); + if (!emailUser) { + throw new OidcNoMatchingAccountError(); } - // Create new OIDC user const now = new Date().toISOString(); - const id = randomUUID(); - - db.insert(users) - .values({ - id, - email, - displayName, - role: 'member', - authProvider: 'oidc', - oidcSubject: sub, - createdAt: now, - updatedAt: now, - }) + db.update(users) + .set({ oidcSubject: sub, updatedAt: now }) + .where(eq(users.id, emailUser.id)) .run(); - // Return the inserted row - const row = db.select().from(users).where(eq(users.id, id)).get(); + const row = db.select().from(users).where(eq(users.id, emailUser.id)).get(); return row!; } diff --git a/shared/src/types/api.test.ts b/shared/src/types/api.test.ts index 515421b84..02fb60e50 100644 --- a/shared/src/types/api.test.ts +++ b/shared/src/types/api.test.ts @@ -62,6 +62,11 @@ describe('ErrorCode type', () => { expect(code).toBe('OIDC_ERROR'); }); + it('should include OIDC_NO_MATCHING_ACCOUNT error code', () => { + const code: ErrorCode = 'OIDC_NO_MATCHING_ACCOUNT'; + expect(code).toBe('OIDC_NO_MATCHING_ACCOUNT'); + }); + it('should include EMAIL_CONFLICT error code', () => { const code: ErrorCode = 'EMAIL_CONFLICT'; expect(code).toBe('EMAIL_CONFLICT'); @@ -76,10 +81,11 @@ describe('ErrorCode type', () => { 'LAST_ADMIN', 'OIDC_NOT_CONFIGURED', 'OIDC_ERROR', + 'OIDC_NO_MATCHING_ACCOUNT', 'EMAIL_CONFLICT', ]; - expect(authCodes).toHaveLength(8); + expect(authCodes).toHaveLength(9); expect(authCodes).toContain('SETUP_COMPLETE'); expect(authCodes).toContain('INVALID_CREDENTIALS'); expect(authCodes).toContain('ACCOUNT_DEACTIVATED'); @@ -87,6 +93,7 @@ describe('ErrorCode type', () => { expect(authCodes).toContain('LAST_ADMIN'); expect(authCodes).toContain('OIDC_NOT_CONFIGURED'); expect(authCodes).toContain('OIDC_ERROR'); + expect(authCodes).toContain('OIDC_NO_MATCHING_ACCOUNT'); expect(authCodes).toContain('EMAIL_CONFLICT'); }); }); diff --git a/shared/src/types/errors.ts b/shared/src/types/errors.ts index 85eb1c9e0..1a3a852d8 100644 --- a/shared/src/types/errors.ts +++ b/shared/src/types/errors.ts @@ -16,6 +16,7 @@ export type ErrorCode = | 'LAST_ADMIN' | 'OIDC_NOT_CONFIGURED' | 'OIDC_ERROR' + | 'OIDC_NO_MATCHING_ACCOUNT' | 'EMAIL_CONFLICT' | 'CIRCULAR_DEPENDENCY' | 'DUPLICATE_DEPENDENCY'