diff --git a/CHANGELOG.md b/CHANGELOG.md index 209052fc..3ab29653 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed +- **Local login routed through the auth provider registry** (#266, community contribution by @Justy116): `POST /auth/login` now authenticates via the same provider path used by OIDC and LDAP instead of the parallel `usersService.login` implementation, and registration auto-login rides that path too. If the auto-login after signup fails transiently, the endpoint returns 201 with the created user and no `session` (the account stays recoverable via a normal login) instead of propagating a 500 after the user row is already committed. The local provider also gained user-enumeration hardening: the password is verified before the disabled-account check, so a wrong password returns the same generic error whether or not the account is disabled, and only a correct password reveals the disabled state. New `docs/architecture/auth.md` documents the provider architecture. Follow-up fixes landed with the merge: the register page now handles the sessionless 201 (redirects to login with a "please sign in" toast instead of throwing), the disabled-account and SSO-required login rejections are now recorded as `auth.login_failed` audit events with a `reason` in the metadata (previously only wrong-password failures were audited; the reason never appears in the HTTP response, so anti-enumeration behavior is unchanged), and a failed post-registration auto-login is now logged through internal observability instead of being swallowed silently. Test-suite hardening that the registry routing made necessary: two auth test files deleted every `auth_providers` row including the migration-seeded `local` provider, which (now that login resolves through the registry) broke every login-based test that ran after them, masked intermittently by the registry's 5-minute cache; they now preserve the seeded row and the test setup re-seeds it defensively, since tracked migrations never re-run + ### Fixed - **Ingestion health counters no longer depend on the metering toggle** (#279). `METERING_ENABLED=false` silently suppressed every `ingestion.*` counter, including `ingestion.pii_rejected`, the safety counter for records dropped because PII masking failed. That toggle governs usage and resource metering, while the `ingestion.*` counters are operational health signals already excluded from tenant usage breakdowns by a prefix filter, so they now bypass it. The buffer hard cap still applies to them: that is a memory valve, not a feature switch. diff --git a/packages/backend/src/modules/users/routes.ts b/packages/backend/src/modules/users/routes.ts index 2bf98b6c..1259a7cb 100644 --- a/packages/backend/src/modules/users/routes.ts +++ b/packages/backend/src/modules/users/routes.ts @@ -1,5 +1,7 @@ import type { FastifyInstance } from 'fastify'; import { z } from 'zod'; +import { hub } from '@logtide/core'; +import { isInternalLoggingEnabled } from '../../utils/internal-logger.js'; import { usersService } from './service.js'; import { authenticationService } from '../auth/authentication-service.js'; import { config } from '../../config/index.js'; @@ -65,8 +67,16 @@ export async function usersRoutes(fastify: FastifyInstance) { password: body.password, }); session = result.session; - } catch { - // transient auto-login failure – user was created, session was not + } catch (autoLoginError) { + // The user row is already committed, so this stays a 201 without a + // session (recoverable via /login), but the cause must be visible to + // operators: a persistent failure here silently degrades every signup. + if (isInternalLoggingEnabled()) { + hub.captureLog('error', '[Auth] Post-registration auto-login failed', { + userId: user.id, + error: autoLoginError instanceof Error ? autoLoginError.message : String(autoLoginError), + }); + } } await auditLogService.record({ @@ -154,22 +164,28 @@ export async function usersRoutes(fastify: FastifyInstance) { } if (error instanceof Error) { - const isCredentialError = error.message === 'Invalid email or password'; - const isAuthError = isCredentialError || - error.message === 'Please log in using your organization SSO' || - error.message === 'This account has been disabled'; - - if (isAuthError) { - if (isCredentialError) { - // parsedBody is defined here (zod succeeded before the service threw) - await auditLogService.record({ - action: 'auth.login_failed', - outcome: 'failure', - organizationId: null, - actor: { type: 'user', id: null, label: parsedBody?.email ?? null }, - metadata: { method: 'local' }, - }); - } + const authFailureReasons: Record = { + 'Invalid email or password': 'invalid_credentials', + 'Please log in using your organization SSO': 'sso_required', + 'This account has been disabled': 'account_disabled', + }; + // hasOwn guard: a plain-object lookup would also match inherited + // Object.prototype keys (e.g. an error whose message is 'toString') + const reason = Object.hasOwn(authFailureReasons, error.message) + ? authFailureReasons[error.message] + : undefined; + + if (reason) { + // parsedBody is defined here (zod succeeded before the service threw). + // The reason lands only in the audit log, never in the HTTP response, + // so recording it does not weaken the anti-enumeration behavior. + await auditLogService.record({ + action: 'auth.login_failed', + outcome: 'failure', + organizationId: null, + actor: { type: 'user', id: null, label: parsedBody?.email ?? null }, + metadata: { method: 'local', reason }, + }); return reply.status(401).send({ error: error.message, }); diff --git a/packages/backend/src/tests/helpers/factories.ts b/packages/backend/src/tests/helpers/factories.ts index 55032d20..954fb890 100644 --- a/packages/backend/src/tests/helpers/factories.ts +++ b/packages/backend/src/tests/helpers/factories.ts @@ -7,14 +7,16 @@ import crypto from 'crypto'; */ export async function createTestUser(overrides: { email?: string; - password?: string; + /** null creates an SSO-only user with no local password */ + password?: string | null; name?: string; + disabled?: boolean; } = {}) { const email = overrides.email || `test-${Date.now()}@example.com`; - const password = overrides.password || 'password123'; + const password = overrides.password === undefined ? 'password123' : overrides.password; const name = overrides.name || 'Test User'; - const hashedPassword = await bcrypt.hash(password, 10); + const hashedPassword = password === null ? null : await bcrypt.hash(password, 10); const user = await db .insertInto('users') @@ -22,6 +24,7 @@ export async function createTestUser(overrides: { email, password_hash: hashedPassword, name, + disabled: overrides.disabled ?? false, }) .returningAll() .executeTakeFirstOrThrow(); diff --git a/packages/backend/src/tests/modules/audit-log/record-integration.test.ts b/packages/backend/src/tests/modules/audit-log/record-integration.test.ts index 1173f326..6225dc07 100644 --- a/packages/backend/src/tests/modules/audit-log/record-integration.test.ts +++ b/packages/backend/src/tests/modules/audit-log/record-integration.test.ts @@ -71,6 +71,53 @@ describe('auth audit records', () => { expect(row.actor_id).toBeNull(); expect(row.user_email).toBe('fail-audit@example.com'); expect((row.metadata as any)?.method).toBe('local'); + expect((row.metadata as any)?.reason).toBe('invalid_credentials'); + }); + + it('disabled-account login with correct password produces auth.login_failed row', async () => { + await createTestUser({ email: 'disabled-audit@example.com', password: 'password123', disabled: true }); + + await request(app.server) + .post('/api/v1/auth/login') + .send({ email: 'disabled-audit@example.com', password: 'password123' }) + .expect(401); + + const rows = await db + .selectFrom('audit_log') + .selectAll() + .where('action', '=', 'auth.login_failed') + .execute(); + + expect(rows).toHaveLength(1); + const row = rows[0]; + expect(row.outcome).toBe('failure'); + expect(row.actor_id).toBeNull(); + expect(row.user_email).toBe('disabled-audit@example.com'); + expect((row.metadata as any)?.method).toBe('local'); + expect((row.metadata as any)?.reason).toBe('account_disabled'); + }); + + it('local login against an SSO-only account produces auth.login_failed row', async () => { + await createTestUser({ email: 'sso-audit@example.com', password: null }); + + await request(app.server) + .post('/api/v1/auth/login') + .send({ email: 'sso-audit@example.com', password: 'anypassword' }) + .expect(401); + + const rows = await db + .selectFrom('audit_log') + .selectAll() + .where('action', '=', 'auth.login_failed') + .execute(); + + expect(rows).toHaveLength(1); + const row = rows[0]; + expect(row.outcome).toBe('failure'); + expect(row.actor_id).toBeNull(); + expect(row.user_email).toBe('sso-audit@example.com'); + expect((row.metadata as any)?.method).toBe('local'); + expect((row.metadata as any)?.reason).toBe('sso_required'); }); it('zod-invalid login payload produces NO auth.login_failed row', async () => { diff --git a/packages/backend/src/tests/modules/auth/authentication-service.test.ts b/packages/backend/src/tests/modules/auth/authentication-service.test.ts index 20d4cdf4..70fe0b84 100644 --- a/packages/backend/src/tests/modules/auth/authentication-service.test.ts +++ b/packages/backend/src/tests/modules/auth/authentication-service.test.ts @@ -28,7 +28,10 @@ describe('AuthenticationService', () => { await db.deleteFrom('projects').execute(); await db.deleteFrom('organizations').execute(); await db.deleteFrom('users').execute(); - await db.deleteFrom('auth_providers').execute(); + // Keep the migration-seeded 'local' provider: POST /auth/login and + // /auth/register resolve it through the provider registry, so wiping + // it breaks every login-based test file that runs after this one. + await db.deleteFrom('auth_providers').where('slug', '!=', 'local').execute(); }); afterAll(async () => { @@ -41,7 +44,7 @@ describe('AuthenticationService', () => { await db.deleteFrom('projects').execute(); await db.deleteFrom('organizations').execute(); await db.deleteFrom('users').execute(); - await db.deleteFrom('auth_providers').execute(); + await db.deleteFrom('auth_providers').where('slug', '!=', 'local').execute(); }); describe('authenticateWithProvider', () => { @@ -520,18 +523,19 @@ describe('AuthenticationService', () => { it('should throw error when identity not found', async () => { const user = await createTestUser(); + const providerAId = crypto.randomUUID(); + const providerBId = crypto.randomUUID(); // Create two identities so we can try to unlink + // (slugs must not collide with the migration-seeded 'local' provider) await db.insertInto('auth_providers').values([ - { id: crypto.randomUUID(), type: 'local', name: 'Local', slug: 'local', enabled: true, config: {} }, - { id: crypto.randomUUID(), type: 'oidc', name: 'OIDC', slug: 'oidc', enabled: true, config: {} }, + { id: providerAId, type: 'local', name: 'Local', slug: 'local-notfound', enabled: true, config: {} }, + { id: providerBId, type: 'oidc', name: 'OIDC', slug: 'oidc-notfound', enabled: true, config: {} }, ]).execute(); - const providers = await db.selectFrom('auth_providers').selectAll().execute(); - await db.insertInto('user_identities').values([ - { user_id: user.id, provider_id: providers[0].id, provider_user_id: user.id, metadata: {} }, - { user_id: user.id, provider_id: providers[1].id, provider_user_id: 'ext-id', metadata: {} }, + { user_id: user.id, provider_id: providerAId, provider_user_id: user.id, metadata: {} }, + { user_id: user.id, provider_id: providerBId, provider_user_id: 'ext-id', metadata: {} }, ]).execute(); // Use a valid UUID format that doesn't exist diff --git a/packages/backend/src/tests/modules/auth/external-routes.test.ts b/packages/backend/src/tests/modules/auth/external-routes.test.ts index 931eb4f1..ee697199 100644 --- a/packages/backend/src/tests/modules/auth/external-routes.test.ts +++ b/packages/backend/src/tests/modules/auth/external-routes.test.ts @@ -58,7 +58,10 @@ describe('External Auth Routes', () => { await db.deleteFrom('projects').execute(); await db.deleteFrom('organizations').execute(); await db.deleteFrom('users').execute(); - await db.deleteFrom('auth_providers').execute(); + // Keep the migration-seeded 'local' provider: POST /auth/login and + // /auth/register resolve it through the provider registry, so wiping + // it breaks every login-based test file that runs after this one. + await db.deleteFrom('auth_providers').where('slug', '!=', 'local').execute(); }); afterAll(async () => { diff --git a/packages/backend/src/tests/modules/users/routes.test.ts b/packages/backend/src/tests/modules/users/routes.test.ts index 7c6ed5ec..aad02792 100644 --- a/packages/backend/src/tests/modules/users/routes.test.ts +++ b/packages/backend/src/tests/modules/users/routes.test.ts @@ -1,9 +1,22 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import Fastify, { type FastifyInstance } from 'fastify'; +import { hub } from '@logtide/core'; import { db } from '../../../database/index.js'; import { usersRoutes } from '../../../modules/users/routes.js'; import { usersService } from '../../../modules/users/service.js'; import { settingsService } from '../../../modules/settings/service.js'; +import { authenticationService } from '../../../modules/auth/authentication-service.js'; + +// Mock internal observability so the auto-login failure path is fully +// exercised (isInternalLoggingEnabled is env-driven and off in tests) +vi.mock('@logtide/core', async (importOriginal) => ({ + ...(await importOriginal()), + hub: { captureLog: vi.fn() }, +})); +vi.mock('../../../utils/internal-logger.js', async (importOriginal) => ({ + ...(await importOriginal()), + isInternalLoggingEnabled: () => true, +})); // Mock settingsService vi.mock('../../../modules/settings/service.js', () => ({ @@ -73,6 +86,42 @@ describe('Users Routes', () => { expect(body.session.token).toBeDefined(); }); + it('returns 201 without a session and logs when auto-login fails', async () => { + const authSpy = vi.spyOn(authenticationService, 'authenticateWithProvider') + .mockRejectedValueOnce(new Error('session backend unavailable')); + + const response = await app.inject({ + method: 'POST', + url: '/register', + payload: { + email: 'no-session@example.com', + password: 'password123', + name: 'No Session', + }, + }); + + expect(response.statusCode).toBe(201); + const body = JSON.parse(response.body); + expect(body.user.email).toBe('no-session@example.com'); + expect(body.session).toBeUndefined(); + + // The user row must exist and be able to log in normally afterwards + const session = await usersService.login({ + email: 'no-session@example.com', + password: 'password123', + }); + expect(session.token).toBeDefined(); + + // The swallowed failure must be visible to operators + expect(hub.captureLog).toHaveBeenCalledWith( + 'error', + '[Auth] Post-registration auto-login failed', + expect.objectContaining({ error: 'session backend unavailable' }) + ); + + authSpy.mockRestore(); + }); + it('should return 400 for invalid email', async () => { const response = await app.inject({ method: 'POST', diff --git a/packages/backend/src/tests/setup.ts b/packages/backend/src/tests/setup.ts index 5e5ed51d..34bf8a47 100644 --- a/packages/backend/src/tests/setup.ts +++ b/packages/backend/src/tests/setup.ts @@ -25,7 +25,25 @@ beforeAll(async () => { console.log('Running database migrations...'); await migrateToLatest(); console.log('Database migrations completed'); - + + // Re-seed the 'local' auth provider (migration 010 seeds it, but tracked + // migrations never re-run, so a test file that deletes the row would + // otherwise leave the whole database without local login forever). + await db + .insertInto('auth_providers') + .values({ + type: 'local', + name: 'Email & Password', + slug: 'local', + enabled: true, + is_default: true, + display_order: 0, + icon: 'mail', + config: {}, + }) + .onConflict((oc) => oc.column('slug').doNothing()) + .execute(); + try { const dbUrl = process.env.DATABASE_URL; if (dbUrl) { diff --git a/packages/frontend/src/lib/api/auth.ts b/packages/frontend/src/lib/api/auth.ts index 3a016b41..48c35387 100644 --- a/packages/frontend/src/lib/api/auth.ts +++ b/packages/frontend/src/lib/api/auth.ts @@ -23,6 +23,13 @@ export interface AuthResponse { }; } +// Registration can succeed (201) without a session when the backend's +// auto-login fails transiently; the account is then recoverable via /login. +export interface RegisterResponse { + user: AuthResponse['user']; + session?: AuthResponse['session']; +} + export interface ErrorResponse { error: string; details?: any; @@ -53,7 +60,7 @@ export interface AuthConfig { } export class AuthAPI { - async register(input: RegisterInput): Promise { + async register(input: RegisterInput): Promise { const response = await fetch(`${getApiBaseUrl()}/auth/register`, { method: 'POST', headers: { diff --git a/packages/frontend/src/routes/register/+page.svelte b/packages/frontend/src/routes/register/+page.svelte index 7ea8a2aa..66d18ad6 100644 --- a/packages/frontend/src/routes/register/+page.svelte +++ b/packages/frontend/src/routes/register/+page.svelte @@ -120,7 +120,14 @@ try { const response = await authAPI.register({ name, email, password }); - await handleAuthSuccess(response, true); + if (!response.session) { + // 201 without a session: the account exists but auto-login failed + // transiently, so send the user to the login page instead of crashing. + toastStore.success('Your account has been created. Please sign in.'); + goto(`/login${redirectUrl ? `?redirect=${encodeURIComponent(redirectUrl)}` : ''}`); + return; + } + await handleAuthSuccess({ ...response, session: response.session }, true); } catch (e) { const errorMsg = e instanceof Error ? e.message : 'Registration failed'; error = errorMsg;