diff --git a/client_v2/e2e/a11y-auth.spec.ts b/client_v2/e2e/a11y-auth.spec.ts new file mode 100644 index 00000000..8541c3ac --- /dev/null +++ b/client_v2/e2e/a11y-auth.spec.ts @@ -0,0 +1,197 @@ +import { test, type Page } from '@playwright/test'; +import { audit, settle } from './audit'; + +// Mobile-accessibility audit for the authentication surfaces. +// +// These live apart from a11y-mobile.spec.ts because they need something that file's +// backend does not have: authentication turned on. The sign-in screen never renders +// against a default instance, so auditing it there would silently measure the library +// page twice and report a pass. +// +// The file configures itself rather than needing a second Playwright config. It asks +// the server what it is, and: +// +// * skips entirely when authentication is disabled, which is the default and what CI +// runs, so `npm run audit:a11y` keeps working unchanged; +// * claims an unclaimed instance with the credentials below, so pointing the audit at +// a throwaway auth-enabled server needs no setup at all; +// * otherwise signs in with AUDIT_AUTH_USERNAME / AUDIT_AUTH_PASSWORD. +// +// One surface is always missing from any single run, unavoidably: claiming is one-way, +// so an instance shows either the claim screen or the sign-in screen, never both. A +// fresh instance audits sign-in (the test claims it first); pointing the audit at an +// already-claimed instance audits sign-in too. To audit the claim screen, run it once +// against a brand-new instance and read the report before the second run. +// +// Run it against an auth-enabled backend serving the built client with: +// npm run build +// SPOOLMAN_AUTH_ENABLED=TRUE uv run uvicorn spoolman.main:app --port 8000 +// AUDIT_BASE_URL=http://localhost:8000 npm run audit:a11y +// +// Or against `vite dev`, with VITE_APIURL pointing at the backend as usual. + +// Resolved the same way src/lib/api/config.ts resolves it, so the audit works against +// both layouts: a `vite dev` server on 5174 talking to a separate backend through an +// absolute VITE_APIURL, and the backend serving the built SPA itself, where the API is +// same-origin. Getting this wrong is silent — every request 404s and every test skips. +const API = process.env.VITE_APIURL?.replace(/\/+$/, '') ?? '/api/v1'; + +const USERNAME = process.env.AUDIT_AUTH_USERNAME ?? 'audit-owner'; +const PASSWORD = process.env.AUDIT_AUTH_PASSWORD ?? 'audit-owner-password'; + +// A second account, created by the suite, whose password is one the owner set. It is +// what makes the forced-change screen reachable — the flag cannot be set on oneself. +const TEMP_USERNAME = 'audit-temp-user'; +const TEMP_PASSWORD = 'audit-temp-password'; + +interface AuthConfig { + enabled: boolean; + setup_required: boolean; +} + +async function authConfig(page: Page): Promise { + try { + const response = await page.request.get(`${API}/auth/config`); + if (!response.ok()) return null; + return (await response.json()) as AuthConfig; + } catch { + // No backend reachable at all. Skipping beats failing the whole audit run. + return null; + } +} + +/** Read the CSRF cookie the server set, which every write must echo in a header. */ +async function csrfHeaders(page: Page): Promise> { + const cookies = await page.context().cookies(); + const csrf = cookies.find((c) => c.name === 'spoolman_csrf'); + return csrf ? { 'X-CSRF-Token': csrf.value } : {}; +} + +/** Sign in through the API, leaving the session cookie on the page's context. */ +async function signIn(page: Page, username: string, password: string): Promise { + const response = await page.request.post(`${API}/auth/login`, { + data: { username, password, remember: false } + }); + return response.ok(); +} + +/** Put the page in a signed-in state as an administrator, or return false. */ +async function signInAsOwner(page: Page): Promise { + const config = await authConfig(page); + if (config === null || !config.enabled) return false; + + if (config.setup_required) { + const claimed = await page.request.post(`${API}/auth/setup`, { + data: { username: USERNAME, password: PASSWORD, display_name: 'Audit Owner' } + }); + if (!claimed.ok()) return false; + return true; + } + return signIn(page, USERNAME, PASSWORD); +} + +test.describe('auth surfaces', () => { + test('a11y: sign-in screen', async ({ page }, testInfo) => { + await page.goto('/'); + // Claim the instance if it is unclaimed, rather than skipping: on a fresh server + // the sign-in screen does not exist until somebody owns it, and skipping here + // would mean the surface is never audited on exactly the throwaway instance this + // suite is meant to be pointed at. + test.skip(!(await signInAsOwner(page)), 'authentication is not enabled on this server'); + + await page.context().clearCookies(); + await page.goto('/'); + await settle(page); + await audit(page, testInfo, 'sign-in screen'); + }); + + test('a11y: claim (first-run setup) screen', async ({ page }, testInfo) => { + await page.goto('/'); + const config = await authConfig(page); + test.skip(config === null || !config.enabled, 'authentication is not enabled on this server'); + test.skip(!config!.setup_required, 'instance is already claimed'); + + await settle(page); + await audit(page, testInfo, 'claim screen'); + }); + + test('a11y: account page', async ({ page }, testInfo) => { + await page.goto('/'); + test.skip(!(await signInAsOwner(page)), 'could not sign in'); + + await page.goto('/account'); + await settle(page); + await audit(page, testInfo, 'account'); + + // The key-creation form is a separate surface: it is the only real form on the + // page and none of its controls exist until it is opened. + const newKey = page.getByRole('button', { name: /new api key/i }).first(); + if ((await newKey.count()) > 0) { + await newKey.click(); + await audit(page, testInfo, 'account — new API key form'); + } + }); + + test('a11y: users page', async ({ page }, testInfo) => { + await page.goto('/'); + test.skip(!(await signInAsOwner(page)), 'could not sign in'); + + await page.goto('/users'); + await settle(page); + await audit(page, testInfo, 'users'); + + const newUser = page.getByRole('button', { name: /new user/i }).first(); + if ((await newUser.count()) > 0) { + await newUser.click(); + await audit(page, testInfo, 'users — new user form'); + } + }); + + test('a11y: audit log', async ({ page }, testInfo) => { + await page.goto('/'); + test.skip(!(await signInAsOwner(page)), 'could not sign in'); + + await page.goto('/audit'); + await settle(page); + await audit(page, testInfo, 'audit log'); + }); + + test('a11y: forced password change', async ({ page }, testInfo) => { + await page.goto('/'); + test.skip(!(await signInAsOwner(page)), 'could not sign in'); + + // Create a throwaway account that must change its password. Re-running the audit + // against the same instance is fine: a 409 means it already exists, and the reset + // below puts it back into the state this surface needs either way. + const headers = await csrfHeaders(page); + await page.request.post(`${API}/auth/user`, { + headers, + data: { + username: TEMP_USERNAME, + password: TEMP_PASSWORD, + level: 'read', + is_admin: false, + must_change_password: true + } + }); + + const listed = await page.request.get(`${API}/auth/user`); + test.skip(!listed.ok(), 'could not list users'); + const users = (await listed.json()) as { id: number; username: string }[]; + const temp = users.find((u) => u.username === TEMP_USERNAME); + test.skip(temp === undefined, 'could not create the temporary account'); + + const reset = await page.request.post(`${API}/auth/user/${temp!.id}/password`, { + headers, + data: { password: TEMP_PASSWORD, must_change_password: true } + }); + test.skip(!reset.ok(), 'could not reset the temporary account'); + + await page.context().clearCookies(); + test.skip(!(await signIn(page, TEMP_USERNAME, TEMP_PASSWORD)), 'could not sign in as the temporary user'); + + await page.goto('/'); + await settle(page); + await audit(page, testInfo, 'forced password change'); + }); +}); diff --git a/client_v2/e2e/a11y-mobile.spec.ts b/client_v2/e2e/a11y-mobile.spec.ts index 76ced8e7..9c074c65 100644 --- a/client_v2/e2e/a11y-mobile.spec.ts +++ b/client_v2/e2e/a11y-mobile.spec.ts @@ -1,137 +1,13 @@ -import { test, expect, type Page, type TestInfo } from '@playwright/test'; -import AxeBuilder from '@axe-core/playwright'; -import { writeFile } from 'node:fs/promises'; -import path from 'node:path'; -import { SURFACES_DIR, TAP_MIN, type SurfaceResult, type TapTarget } from './report'; +import { test } from '@playwright/test'; +import { audit, settle } from './audit'; -// Mobile-accessibility audit for client_v2. +// Mobile-accessibility audit for client_v2's main surfaces, under a Pixel 5 mobile +// emulation (see playwright.config.ts). The measurements themselves live in audit.ts; +// this file is the list of what gets measured. // -// Two independent checks run against every surface, under a Pixel 5 mobile -// emulation (see playwright.config.ts): -// -// 1. axe-core — the full WCAG 2.0/2.1 A + AA ruleset PLUS the WCAG 2.2 -// `target-size` rule (SC 2.5.8, 24x24 CSS px minimum). Real conformance -// failures. -// -// 2. A custom tap-target measurement — flags interactive controls smaller -// than TAP_MIN px in either dimension. An *advisory* thumb-comfort check -// (Apple HIG 44px / Material 48px), stricter than the WCAG minimum and -// catching targets axe considers technically compliant. -// -// Each surface's findings are written to SURFACES_DIR as JSON; the consolidated -// `a11y-report.md` is rendered by globalTeardown (which survives worker -// restarts). Raw findings are also attached to the Playwright HTML report. - -let surfaceCount = 0; - -// Let the DOM and any async lists settle. The app holds an open live-sync -// WebSocket, so `networkidle` never fires — we bound it and fall through. -async function settle(page: Page, ms = 500) { - await page.waitForLoadState('domcontentloaded').catch(() => {}); - await page.waitForLoadState('networkidle', { timeout: 2500 }).catch(() => {}); - await page.waitForTimeout(ms); -} - -// Measure every interactive control in the live DOM and return those smaller -// than `min` px in either dimension. Runs in the browser. -function collectSmallTargets(min: number): TapTarget[] { - const SELECTOR = [ - 'a[href]', - 'button', - 'input:not([type="hidden"])', - 'select', - 'textarea', - 'summary', - '[role="button"]', - '[role="link"]', - '[role="checkbox"]', - '[role="radio"]', - '[role="switch"]', - '[role="tab"]', - '[role="menuitem"]', - '[tabindex]:not([tabindex="-1"])' - ].join(','); - - const out: TapTarget[] = []; - const seen = new Set(); - - for (const el of Array.from(document.querySelectorAll(SELECTOR))) { - if (seen.has(el)) continue; - seen.add(el); - - const style = getComputedStyle(el); - if (style.display === 'none' || style.visibility === 'hidden' || style.opacity === '0') continue; - if ((el as HTMLButtonElement).disabled) continue; - - // WCAG exempts targets rendered inline within a run of text (e.g. a link - // inside a sentence). Skip inline elements to avoid false positives. - if (el.tagName === 'A' && style.display.startsWith('inline')) continue; - - const rect = el.getBoundingClientRect(); - if (rect.width === 0 || rect.height === 0) continue; // not rendered - if (rect.width >= min && rect.height >= min) continue; - - const id = el.id ? `#${el.id}` : ''; - const rawCls = (el as HTMLElement).className; - const cls = - typeof rawCls === 'string' && rawCls.trim() - ? '.' + rawCls.trim().split(/\s+/).slice(0, 2).join('.') - : ''; - const aria = el.getAttribute('aria-label'); - - out.push({ - tag: el.tagName.toLowerCase(), - text: (aria || el.textContent || '').trim().replace(/\s+/g, ' ').slice(0, 40), - selector: `${el.tagName.toLowerCase()}${id}${cls}`, - width: Math.round(rect.width), - height: Math.round(rect.height) - }); - } - return out; -} - -// Run both checks against whatever is currently rendered and record the result. -async function audit(page: Page, testInfo: TestInfo, label: string) { - await settle(page); - - const axeResults = await new AxeBuilder({ page }) - .withTags(['wcag2a', 'wcag2aa', 'wcag21a', 'wcag21aa', 'wcag22aa']) - .analyze(); - - const smallTargets = await page.evaluate(collectSmallTargets, TAP_MIN); - - const surface: SurfaceResult = { - order: surfaceCount++, - label, - url: page.url(), - axe: axeResults.violations.map((v) => ({ - id: v.id, - impact: v.impact, - help: v.help, - nodes: v.nodes.length, - targets: v.nodes.slice(0, 5).map((n) => n.target.join(' ')) - })), - smallTargets - }; - - const file = path.join(SURFACES_DIR, `${label.replace(/[^a-z0-9]+/gi, '-')}.json`); - await writeFile(file, JSON.stringify(surface, null, 2), 'utf8'); - - await testInfo.attach(`${label} — axe violations`, { - body: JSON.stringify(axeResults.violations, null, 2), - contentType: 'application/json' - }); - await testInfo.attach(`${label} — small tap targets`, { - body: JSON.stringify(smallTargets, null, 2), - contentType: 'application/json' - }); - - // Soft assertions: the run completes and reports every surface, but the test - // is still marked failed if anything is out of spec — useful as a CI gate. - const serious = surface.axe.filter((v) => v.impact === 'serious' || v.impact === 'critical'); - expect.soft(serious, `${label}: serious/critical axe violations`).toEqual([]); - expect.soft(smallTargets, `${label}: tap targets under ${TAP_MIN}px`).toEqual([]); -} +// These run against the default, authentication-disabled configuration. The sign-in, +// account and administration surfaces need an authenticated backend and are audited by +// a11y-auth.spec.ts, which skips itself when authentication is off. const STATIC_ROUTES: { path: string; label: string }[] = [ { path: '/', label: 'library (home)' }, diff --git a/client_v2/e2e/audit.ts b/client_v2/e2e/audit.ts new file mode 100644 index 00000000..af91d87c --- /dev/null +++ b/client_v2/e2e/audit.ts @@ -0,0 +1,131 @@ +import AxeBuilder from '@axe-core/playwright'; +import { expect, type Page, type TestInfo } from '@playwright/test'; +import { writeFile } from 'node:fs/promises'; +import path from 'node:path'; +import { SURFACES_DIR, TAP_MIN, type SurfaceResult, type TapTarget } from './report'; + +// The two measurements the mobile-accessibility audit takes, and the bookkeeping that +// turns them into a report. Extracted from a11y-mobile.spec.ts so the auth surfaces — +// which need a different backend configuration and therefore live in their own spec — +// are measured by exactly the same code rather than a copy of it. +// +// 1. axe-core, the full WCAG 2.0/2.1 A + AA ruleset plus WCAG 2.2 `target-size` +// (SC 2.5.8, 24x24 CSS px minimum). Real conformance failures. +// +// 2. A custom tap-target measurement flagging interactive controls smaller than +// TAP_MIN px in either dimension. Advisory thumb-comfort (Apple HIG 44px / +// Material 48px), stricter than WCAG and catching targets axe considers compliant. +// +// Each surface's findings are written to SURFACES_DIR as JSON; globalTeardown renders +// the consolidated a11y-report.md from them. + +let surfaceCount = 0; + +// Let the DOM and any async lists settle. The app holds an open live-sync +// WebSocket, so `networkidle` never fires — we bound it and fall through. +export async function settle(page: Page, ms = 500) { + await page.waitForLoadState('domcontentloaded').catch(() => {}); + await page.waitForLoadState('networkidle', { timeout: 2500 }).catch(() => {}); + await page.waitForTimeout(ms); +} + +// Measure every interactive control in the live DOM and return those smaller +// than `min` px in either dimension. Runs in the browser. +function collectSmallTargets(min: number): TapTarget[] { + const SELECTOR = [ + 'a[href]', + 'button', + 'input:not([type="hidden"])', + 'select', + 'textarea', + 'summary', + '[role="button"]', + '[role="link"]', + '[role="checkbox"]', + '[role="radio"]', + '[role="switch"]', + '[role="tab"]', + '[role="menuitem"]', + '[tabindex]:not([tabindex="-1"])' + ].join(','); + + const out: TapTarget[] = []; + const seen = new Set(); + + for (const el of Array.from(document.querySelectorAll(SELECTOR))) { + if (seen.has(el)) continue; + seen.add(el); + + const style = getComputedStyle(el); + if (style.display === 'none' || style.visibility === 'hidden' || style.opacity === '0') continue; + if ((el as HTMLButtonElement).disabled) continue; + + // WCAG exempts targets rendered inline within a run of text (e.g. a link + // inside a sentence). Skip inline elements to avoid false positives. + if (el.tagName === 'A' && style.display.startsWith('inline')) continue; + + const rect = el.getBoundingClientRect(); + if (rect.width === 0 || rect.height === 0) continue; // not rendered + if (rect.width >= min && rect.height >= min) continue; + + const id = el.id ? `#${el.id}` : ''; + const rawCls = (el as HTMLElement).className; + const cls = + typeof rawCls === 'string' && rawCls.trim() + ? '.' + rawCls.trim().split(/\s+/).slice(0, 2).join('.') + : ''; + const aria = el.getAttribute('aria-label'); + + out.push({ + tag: el.tagName.toLowerCase(), + text: (aria || el.textContent || '').trim().replace(/\s+/g, ' ').slice(0, 40), + selector: `${el.tagName.toLowerCase()}${id}${cls}`, + width: Math.round(rect.width), + height: Math.round(rect.height) + }); + } + return out; +} + +// Run both checks against whatever is currently rendered and record the result. +export async function audit(page: Page, testInfo: TestInfo, label: string) { + await settle(page); + + const axeResults = await new AxeBuilder({ page }) + .withTags(['wcag2a', 'wcag2aa', 'wcag21a', 'wcag21aa', 'wcag22aa']) + .analyze(); + + const smallTargets = await page.evaluate(collectSmallTargets, TAP_MIN); + + const surface: SurfaceResult = { + order: surfaceCount++, + label, + url: page.url(), + axe: axeResults.violations.map((v) => ({ + id: v.id, + impact: v.impact, + help: v.help, + nodes: v.nodes.length, + targets: v.nodes.slice(0, 5).map((n) => n.target.join(' ')) + })), + smallTargets + }; + + const file = path.join(SURFACES_DIR, `${label.replace(/[^a-z0-9]+/gi, '-')}.json`); + await writeFile(file, JSON.stringify(surface, null, 2), 'utf8'); + + await testInfo.attach(`${label} — axe violations`, { + body: JSON.stringify(axeResults.violations, null, 2), + contentType: 'application/json' + }); + await testInfo.attach(`${label} — small tap targets`, { + body: JSON.stringify(smallTargets, null, 2), + contentType: 'application/json' + }); + + // Soft assertions: the run completes and reports every surface, but the test + // is still marked failed if anything is out of spec — useful as a CI gate. + const serious = surface.axe.filter((v) => v.impact === 'serious' || v.impact === 'critical'); + expect.soft(serious, `${label}: serious/critical axe violations`).toEqual([]); + expect.soft(smallTargets, `${label}: tap targets under ${TAP_MIN}px`).toEqual([]); +} diff --git a/client_v2/src/lib/components/Checkbox.svelte b/client_v2/src/lib/components/Checkbox.svelte new file mode 100644 index 00000000..e32a6fd2 --- /dev/null +++ b/client_v2/src/lib/components/Checkbox.svelte @@ -0,0 +1,86 @@ + + + + + diff --git a/client_v2/src/lib/components/auth/ApiKeyList.svelte b/client_v2/src/lib/components/auth/ApiKeyList.svelte index 94022e21..2e5294d8 100644 --- a/client_v2/src/lib/components/auth/ApiKeyList.svelte +++ b/client_v2/src/lib/components/auth/ApiKeyList.svelte @@ -309,8 +309,10 @@ .icon { display: grid; place-items: center; - width: 34px; - height: 34px; + /* 44px, matching PasswordInput: the auth surfaces are audited against the + advisory thumb-comfort minimum, not just WCAG's 24px. */ + width: 44px; + height: 44px; border: 0; border-radius: var(--radius-sm); background: none; @@ -342,7 +344,7 @@ .form input, .form select { - height: 40px; + height: 44px; padding: 0 10px; border: 1px solid var(--border-input); border-radius: var(--radius-sm); diff --git a/client_v2/src/lib/components/auth/LoginScreen.svelte b/client_v2/src/lib/components/auth/LoginScreen.svelte index c4454db9..2a2ce943 100644 --- a/client_v2/src/lib/components/auth/LoginScreen.svelte +++ b/client_v2/src/lib/components/auth/LoginScreen.svelte @@ -2,6 +2,7 @@ import AuthCard from './AuthCard.svelte'; import PasswordInput from './PasswordInput.svelte'; import Button from '$components/Button.svelte'; + import Checkbox from '$components/Checkbox.svelte'; import { auth } from '$lib/stores/auth.svelte'; import { HttpError } from '$lib/api/http'; import * as m from '$lib/paraglide/messages'; @@ -65,10 +66,7 @@ - + {#if error} @@ -106,7 +104,7 @@ color: var(--text-2); } - label:not(.check) { + label { margin-top: 8px; } @@ -125,25 +123,6 @@ border-color: var(--accent); } - .check { - display: flex; - align-items: center; - gap: 8px; - /* Keeps the row itself at the 44px tap target the a11y audit enforces. */ - min-height: 44px; - margin-top: 4px; - cursor: pointer; - color: var(--text); - font-size: 0.85rem; - } - - .check input { - width: 18px; - height: 18px; - accent-color: var(--accent); - cursor: pointer; - } - .error { margin: 4px 0 0; color: var(--danger); diff --git a/client_v2/src/routes/audit/+page.svelte b/client_v2/src/routes/audit/+page.svelte index 7d2eb2df..5cefcb28 100644 --- a/client_v2/src/routes/audit/+page.svelte +++ b/client_v2/src/routes/audit/+page.svelte @@ -243,7 +243,7 @@ .filter, .num { - height: 34px; + height: 44px; padding: 0 8px; border: 1px solid var(--border-input); border-radius: var(--radius-sm); diff --git a/client_v2/src/routes/users/+page.svelte b/client_v2/src/routes/users/+page.svelte index 287a1fbb..e71f2135 100644 --- a/client_v2/src/routes/users/+page.svelte +++ b/client_v2/src/routes/users/+page.svelte @@ -2,6 +2,7 @@ import Card from '$components/Card.svelte'; import Button from '$components/Button.svelte'; import Toggle from '$components/Toggle.svelte'; + import Checkbox from '$components/Checkbox.svelte'; import PasswordInput from '$components/auth/PasswordInput.svelte'; import SecretReveal from '$components/auth/SecretReveal.svelte'; import KeyRound from '@lucide/svelte/icons/key-round'; @@ -336,15 +337,12 @@ {/each} - - - + + {#if formError}{/if} @@ -456,7 +454,7 @@ } select { - height: 34px; + height: 44px; padding: 0 8px; border: 1px solid var(--border-input); border-radius: var(--radius-sm); @@ -478,8 +476,9 @@ .icon { display: grid; place-items: center; - width: 34px; - height: 34px; + /* 44px, matching PasswordInput and the rest of the auth surfaces. */ + width: 44px; + height: 44px; border: 0; border-radius: var(--radius-sm); background: none; @@ -504,14 +503,14 @@ gap: 6px; padding: 14px 16px; } - .form label:not(.check) { + .form label { margin-top: 6px; font-size: 0.8rem; color: var(--text-2); } .form input[type='text'], .form select { - height: 40px; + height: 44px; padding: 0 10px; border: 1px solid var(--border-input); border-radius: var(--radius-sm); @@ -525,21 +524,6 @@ .form select:focus { border-color: var(--accent); } - .check { - display: flex; - align-items: center; - gap: 8px; - min-height: 44px; - cursor: pointer; - font-size: 0.85rem; - color: var(--text); - } - .check input { - width: 18px; - height: 18px; - accent-color: var(--accent); - cursor: pointer; - } .error { margin: 4px 0 0; color: var(--danger); diff --git a/tests_integration/tests_auth/conftest.py b/tests_integration/tests_auth/conftest.py index 1d58ca71..a7330ab1 100644 --- a/tests_integration/tests_auth/conftest.py +++ b/tests_integration/tests_auth/conftest.py @@ -53,6 +53,10 @@ OWNER_DISPLAY_NAME = "The Owner" # Every failed sign-in answers with this exact text, whatever went wrong. INVALID_CREDENTIALS = "Incorrect username or password." +# Password for accounts the tests create for themselves. Long enough to clear the +# server's ten character minimum. +READER_PASSWORD = "reader-password-1" + # Websocket close codes, from spoolman.auth.dependencies. WS_UNAUTHENTICATED = 4401 WS_FORBIDDEN = 4403 @@ -221,3 +225,65 @@ def anonymous() -> httpx.Client: """Return a client with no credentials at all.""" with new_client() as client: yield client + + +def create_user( + admin: httpx.Client, + username: str, + *, + level: str = "read", + is_admin: bool = False, + password: str | None = None, + must_change_password: bool = False, +) -> tuple[int, str]: + """Create an account and return its ID and password. + + Phase 2's answer to what tests_auth could not do before: make a second account + without a command line. Every caller picks a username of its own, because the suite + shares one server and one database for the whole run. + + Args: + admin: A client signed in as an administrator. + username: The username to create. + level: The permission level to grant. + is_admin: Whether the account administers other users. + password: The password to set, or None to have one generated. + must_change_password: Whether to force a change at first sign-in. + + Returns: + tuple: The new user's ID, and the password it can sign in with. + + """ + response = admin.post( + "/auth/user", + json={ + "username": username, + "password": password, + "level": level, + "is_admin": is_admin, + "must_change_password": must_change_password, + }, + ) + assert_code(response, 201) + body = response.json() + # The server returns a generated password exactly once, here. If one was supplied it + # is deliberately absent from the response, so fall back to what the caller sent. + return body["user"]["id"], body.get("password") or (password or "") + + +@pytest.fixture +def reader(owner: httpx.Client) -> Iterator[httpx.Client]: + """Return a client signed in as a read-level, non-administrator user. + + This is what test_levels.py's docstring said phase 1 could not build. It exists for + exactly one purpose: proving that a low-level *user* is refused, which the anonymous + reader could only approximate. + """ + username = f"reader-{int(time.time() * 1000)}" + create_user(owner, username, level="read", password=READER_PASSWORD) + client, response = login(username=username, password=READER_PASSWORD) + assert_code(response, 200) + try: + yield client + finally: + client.close() diff --git a/tests_integration/tests_auth/test_apikey.py b/tests_integration/tests_auth/test_apikey.py new file mode 100644 index 00000000..a528a331 --- /dev/null +++ b/tests_integration/tests_auth/test_apikey.py @@ -0,0 +1,305 @@ +"""Integration tests: API keys. + +The properties worth defending, in order of how badly they fail if broken: + +* A key never outranks the account it belongs to, checked on every request rather than + frozen at creation. :func:`test_key_is_capped_by_owner_level` demotes a user and shows + the key it already issued weakening in the same breath. +* A key cannot manage keys, so a leak cannot be made permanent by minting a replacement. +* A key carries no administrative rights, whoever owns it. +* A key skips CSRF, because it is not an ambient credential -- and a session does not, + which is the half that proves the skip is targeted rather than a hole. +""" + +import time + +import httpx + +from .conftest import ( + CSRF_HEADER, + READER_PASSWORD, + assert_code, + create_user, + login, + new_client, +) + +KEY_HEADER = "X-API-Key" + + +def _unique(prefix: str) -> str: + """Build a name no other test in this run will collide with.""" + return f"{prefix}-{int(time.time() * 1000)}" + + +def _keyed_client(key: str) -> httpx.Client: + """Return a client that authenticates with an API key and nothing else.""" + client = new_client() + client.headers[KEY_HEADER] = key + return client + + +def test_create_returns_the_secret_once(owner: httpx.Client) -> None: + """The plaintext key is in the creation response and in no later one.""" + response = owner.post("/auth/apikey", json={"name": _unique("once"), "level": "read"}) + assert_code(response, 201) + body = response.json() + + key = body["key"] + assert key.startswith("spoolman_") + assert body["info"]["prefix"] in key + assert body["info"]["revoked"] is False + assert body["info"]["expired"] is False + + listed = owner.get("/auth/apikey") + assert_code(listed, 200) + entry = next(item for item in listed.json() if item["id"] == body["info"]["id"]) + # The secret must not survive anywhere in the listing, under any field name. + assert key not in str(entry) + + +def test_key_authenticates_by_header_and_bearer(owner: httpx.Client) -> None: + """Both presentation forms are accepted, because clients differ.""" + created = owner.post("/auth/apikey", json={"name": _unique("forms"), "level": "read"}) + assert_code(created, 201) + key = created.json()["key"] + + with _keyed_client(key) as client: + assert_code(client.get("/spool"), 200) + + with new_client() as client: + client.headers["Authorization"] = f"Bearer {key}" + assert_code(client.get("/spool"), 200) + + +def test_unknown_key_is_rejected() -> None: + """A well-formed but unknown key authenticates nobody.""" + with _keyed_client("spoolman_aaaaaaaaaaaa.not-a-real-secret") as client: + assert_code(client.get("/spool"), 401) + + # Malformed enough that it never reaches a database lookup at all. + with _keyed_client("not-a-key") as client: + assert_code(client.get("/spool"), 401) + + +def test_key_skips_csrf_but_a_session_does_not(owner: httpx.Client) -> None: + """The CSRF requirement applies to ambient credentials only. + + A key travels in a header that a cross-site page cannot set, so there is nothing for + a second factor to defend against. A session cookie is attached by the browser + without being asked, so it needs one. Both halves are asserted here: dropping the + header must break the session and must not break the key. + """ + created = owner.post("/auth/apikey", json={"name": _unique("csrf"), "level": "edit"}) + assert_code(created, 201) + key = created.json()["key"] + + vendor = owner.post("/vendor", json={"name": _unique("csrf-vendor")}) + assert_code(vendor, 200) + vendor_id = vendor.json()["id"] + + try: + with _keyed_client(key) as client: + # No CSRF header anywhere on this client, and the write still lands. + assert_code(client.patch(f"/vendor/{vendor_id}", json={"comment": "by key"}), 200) + + stripped = owner.headers.pop(CSRF_HEADER) + try: + assert_code(owner.patch(f"/vendor/{vendor_id}", json={"comment": "by session"}), 403) + finally: + owner.headers[CSRF_HEADER] = stripped + finally: + assert_code(owner.delete(f"/vendor/{vendor_id}"), 200) + + +def test_key_cannot_exceed_its_level(owner: httpx.Client) -> None: + """A read key is refused an edit route and a manage route.""" + created = owner.post("/auth/apikey", json={"name": _unique("readonly"), "level": "read"}) + assert_code(created, 201) + key = created.json()["key"] + + with _keyed_client(key) as client: + assert_code(client.get("/spool"), 200) + assert_code(client.post("/vendor", json={"name": "should not exist"}), 403) + + +def test_key_cannot_manage_keys(owner: httpx.Client) -> None: + """A key cannot mint, list or revoke keys, however senior its owner. + + Without this a leaked key is unrevocable in practice: whoever holds it issues + themselves a fresh one before the original is withdrawn. + """ + created = owner.post("/auth/apikey", json={"name": _unique("nomint"), "level": "manage"}) + assert_code(created, 201) + key = created.json()["key"] + key_id = created.json()["info"]["id"] + + with _keyed_client(key) as client: + assert_code(client.get("/auth/apikey"), 403) + assert_code(client.post("/auth/apikey", json={"name": "child", "level": "read"}), 403) + assert_code(client.post(f"/auth/apikey/{key_id}/revoke"), 403) + assert_code(client.delete(f"/auth/apikey/{key_id}"), 403) + + +def test_key_carries_no_admin_rights(owner: httpx.Client) -> None: + """A manage-level key held by the owner still cannot administer users.""" + created = owner.post("/auth/apikey", json={"name": _unique("noadmin"), "level": "manage"}) + assert_code(created, 201) + key = created.json()["key"] + + with _keyed_client(key) as client: + assert_code(client.get("/auth/user"), 403) + assert_code(client.get("/auth/audit"), 403) + # Nor can it change the password of the account it belongs to. + assert_code( + client.post("/auth/password", json={"current_password": "x" * 10, "new_password": "y" * 12}), + 403, + ) + + +def test_cannot_issue_above_own_level(owner: httpx.Client) -> None: + """A read-level user is refused when asking for an edit key. + + Refused rather than silently capped: a key quietly issued weaker than asked for is a + support ticket six months later, when a script starts failing for no visible reason. + """ + username = _unique("issuer") + create_user(owner, username, level="read", password=READER_PASSWORD) + client, response = login(username=username, password=READER_PASSWORD) + assert_code(response, 200) + try: + refused = client.post("/auth/apikey", json={"name": _unique("too-strong"), "level": "manage"}) + assert_code(refused, 403) + assert "above your own" in refused.json()["message"] + + allowed = client.post("/auth/apikey", json={"name": _unique("just-right"), "level": "read"}) + assert_code(allowed, 201) + finally: + client.close() + + +def test_key_is_capped_by_owner_level(owner: httpx.Client) -> None: + """Demoting a user weakens the keys they already hold, immediately. + + This is the property that makes a key safe to hand out: it is not a frozen grant, + it is a pointer at an account whose rights are re-read on every request. + """ + username = _unique("demoted") + user_id, password = create_user(owner, username, level="manage", password=READER_PASSWORD) + + client, response = login(username=username, password=password) + assert_code(response, 200) + try: + created = client.post("/auth/apikey", json={"name": _unique("capped"), "level": "manage"}) + assert_code(created, 201) + key = created.json()["key"] + key_id = created.json()["info"]["id"] + assert created.json()["info"]["effective_level"] == "manage" + + with _keyed_client(key) as keyed: + vendor = keyed.post("/vendor", json={"name": _unique("capped-vendor")}) + assert_code(vendor, 200) + vendor_id = vendor.json()["id"] + + assert_code(owner.patch(f"/auth/user/{user_id}", json={"level": "read"}), 200) + + with _keyed_client(key) as keyed: + # Same key, same request, different answer -- the cap is re-applied per call. + assert_code(keyed.get("/spool"), 200) + assert_code(keyed.post("/vendor", json={"name": "should not exist"}), 403) + + listed = client.get("/auth/apikey") + assert_code(listed, 200) + entry = next(item for item in listed.json() if item["id"] == key_id) + assert entry["level"] == "manage", "the issued level is a historical fact and does not change" + assert entry["effective_level"] == "read", "what it can actually do follows the owner" + + assert_code(owner.delete(f"/vendor/{vendor_id}"), 200) + finally: + client.close() + + +def test_revoke_stops_the_key_but_keeps_the_record(owner: httpx.Client) -> None: + """A revoked key stops authenticating and stays visible as revoked.""" + created = owner.post("/auth/apikey", json={"name": _unique("revoked"), "level": "read"}) + assert_code(created, 201) + key = created.json()["key"] + key_id = created.json()["info"]["id"] + + with _keyed_client(key) as client: + assert_code(client.get("/spool"), 200) + + revoked = owner.post(f"/auth/apikey/{key_id}/revoke") + assert_code(revoked, 200) + assert revoked.json()["revoked"] is True + + with _keyed_client(key) as client: + assert_code(client.get("/spool"), 401) + + listed = owner.get("/auth/apikey") + assert_code(listed, 200) + assert any(item["id"] == key_id and item["revoked"] for item in listed.json()) + + +def test_delete_removes_the_key(owner: httpx.Client) -> None: + """A deleted key stops working and leaves the listing.""" + created = owner.post("/auth/apikey", json={"name": _unique("deleted"), "level": "read"}) + assert_code(created, 201) + key = created.json()["key"] + key_id = created.json()["info"]["id"] + + assert_code(owner.delete(f"/auth/apikey/{key_id}"), 200) + + with _keyed_client(key) as client: + assert_code(client.get("/spool"), 401) + + listed = owner.get("/auth/apikey") + assert_code(listed, 200) + assert all(item["id"] != key_id for item in listed.json()) + + +def test_keys_are_private_to_their_owner(owner: httpx.Client, reader: httpx.Client) -> None: + """One user cannot see or touch another's keys. + + The refusal is 404 rather than 403 on purpose: answering "forbidden" would confirm + that a key with this ID exists and belongs to somebody else. + """ + created = owner.post("/auth/apikey", json={"name": _unique("private"), "level": "read"}) + assert_code(created, 201) + key_id = created.json()["info"]["id"] + + listed = reader.get("/auth/apikey") + assert_code(listed, 200) + assert all(item["id"] != key_id for item in listed.json()) + + assert_code(reader.post(f"/auth/apikey/{key_id}/revoke"), 404) + assert_code(reader.delete(f"/auth/apikey/{key_id}"), 404) + + +def test_key_belonging_to_a_disabled_user_stops_working(owner: httpx.Client) -> None: + """Disabling an account disables its machine credentials too.""" + username = _unique("disabled") + user_id, password = create_user(owner, username, level="edit", password=READER_PASSWORD) + + client, response = login(username=username, password=password) + assert_code(response, 200) + try: + created = client.post("/auth/apikey", json={"name": _unique("orphan"), "level": "read"}) + assert_code(created, 201) + key = created.json()["key"] + finally: + client.close() + + with _keyed_client(key) as keyed: + assert_code(keyed.get("/spool"), 200) + + assert_code(owner.patch(f"/auth/user/{user_id}", json={"is_active": False}), 200) + + with _keyed_client(key) as keyed: + assert_code(keyed.get("/spool"), 401) + + +def test_anonymous_cannot_reach_key_endpoints(anonymous: httpx.Client) -> None: + """Without a session there is no account to own a key.""" + assert_code(anonymous.get("/auth/apikey"), 401) + assert_code(anonymous.post("/auth/apikey", json={"name": "nope", "level": "read"}), 401) diff --git a/tests_integration/tests_auth/test_audit.py b/tests_integration/tests_auth/test_audit.py new file mode 100644 index 00000000..817df166 --- /dev/null +++ b/tests_integration/tests_auth/test_audit.py @@ -0,0 +1,166 @@ +"""Integration tests: the audit log. + +The log's value rests on three things, and each is asserted below. + +It records what happened even when the request failed -- a sign-in attempt against a +real account is exactly the event an administrator wants to see, and it is the one the +caller was told nothing about. + +It cannot be edited through the API. There is no write endpoint and no delete endpoint, +so a log its subjects could tamper with never exists in the first place. + +It outlives its subjects. Deleting an account detaches its entries rather than removing +them, so the record of what that account did survives the account. +""" + +import time + +import httpx + +from .conftest import ( + READER_PASSWORD, + assert_code, + create_user, + login, +) + + +def _unique(prefix: str) -> str: + """Build a name no other test in this run will collide with.""" + return f"{prefix}-{int(time.time() * 1000)}" + + +def _entries(owner: httpx.Client, **params: object) -> list[dict]: + """Read the audit log.""" + response = owner.get("/auth/audit", params=params) + assert_code(response, 200) + return response.json() + + +def test_successful_sign_in_is_recorded(owner: httpx.Client) -> None: + """Signing in leaves an attributable entry.""" + username = _unique("audited") + create_user(owner, username, level="read", password=READER_PASSWORD) + client, response = login(username=username, password=READER_PASSWORD) + assert_code(response, 200) + client.close() + + entries = _entries(owner, event="login.success", limit=100) + assert any(entry["target"] == username.lower() for entry in entries) + + +def test_failed_sign_in_is_recorded_with_a_reason(owner: httpx.Client) -> None: + """The log distinguishes what the response deliberately does not. + + The caller is told only "incorrect username or password", because saying more would + let anyone enumerate accounts. The administrator reading the log already knows which + accounts exist, so the entry says which case it was. + """ + username = _unique("badpass") + create_user(owner, username, level="read", password=READER_PASSWORD) + + _, response = login(username=username, password="definitely-not-the-password") + assert_code(response, 401) + + entries = _entries(owner, event="login.failure", limit=100) + match = next((entry for entry in entries if entry["target"] == username.lower()), None) + assert match is not None, "the failed attempt should have been recorded" + assert match["detail"]["reason"] == "bad_password" + assert match["actor_username"] == username.lower() + + +def test_user_and_key_changes_are_recorded(owner: httpx.Client) -> None: + """Creating an account and issuing a key both leave entries.""" + username = _unique("recorded") + user_id, _ = create_user(owner, username, level="read", password=READER_PASSWORD) + + key_name = _unique("recorded-key") + created = owner.post("/auth/apikey", json={"name": key_name, "level": "read"}) + assert_code(created, 201) + + assert_code(owner.patch(f"/auth/user/{user_id}", json={"level": "edit"}), 200) + + entries = _entries(owner, limit=200) + events = {(entry["event"], entry["target"]) for entry in entries} + assert ("user.created", username.lower()) in events + assert ("user.updated", username.lower()) in events + assert ("apikey.created", key_name) in events + + # The key's prefix is recorded, so a key seen in a log can be traced to its creation. + key_entry = next(entry for entry in entries if entry["event"] == "apikey.created" and entry["target"] == key_name) + assert key_entry["detail"]["prefix"] == created.json()["info"]["prefix"] + assert key_entry["actor_kind"] == "user" + + +def test_entries_survive_the_account_that_made_them(owner: httpx.Client) -> None: + """Deleting a user detaches its entries instead of erasing them. + + A log that loses its record of what an account did the moment the account is removed + is not evidence of anything. The username survives in ``target``. + """ + username = _unique("ghost") + user_id, password = create_user(owner, username, level="read", password=READER_PASSWORD) + + client, response = login(username=username, password=password) + assert_code(response, 200) + client.close() + + assert_code(owner.delete(f"/auth/user/{user_id}"), 200) + + entries = _entries(owner, event="login.success", limit=200) + match = next((entry for entry in entries if entry["target"] == username.lower()), None) + assert match is not None, "the sign-in should still be recorded" + assert match.get("actor_user_id") is None, "the foreign key should have been cleared" + assert match.get("actor_username") is None + + +def test_filtering_and_pagination(owner: httpx.Client) -> None: + """The event filter narrows the list, and the total counts what matched.""" + unfiltered = owner.get("/auth/audit", params={"limit": 1}) + assert_code(unfiltered, 200) + assert len(unfiltered.json()) <= 1 + total = int(unfiltered.headers["x-total-count"]) + assert total > 0 + + filtered = owner.get("/auth/audit", params={"event": "login.success", "limit": 100}) + assert_code(filtered, 200) + assert all(entry["event"] == "login.success" for entry in filtered.json()) + assert int(filtered.headers["x-total-count"]) <= total + + # An event nobody has produced yields an empty page rather than an error. + empty = owner.get("/auth/audit", params={"event": "no.such.event"}) + assert_code(empty, 200) + assert empty.json() == [] + + +def test_newest_first(owner: httpx.Client) -> None: + """Entries come back with the most recent first.""" + entries = _entries(owner, limit=50) + dates = [entry["date"] for entry in entries] + assert dates == sorted(dates, reverse=True) + + +def test_event_vocabulary_is_published(owner: httpx.Client) -> None: + """A client can learn the event names rather than hardcoding them.""" + response = owner.get("/auth/audit/events") + assert_code(response, 200) + events = response.json() + for expected in ("login.success", "login.failure", "user.created", "apikey.created"): + assert expected in events + + +def test_the_log_is_read_only(owner: httpx.Client) -> None: + """There is no way to add or remove an entry through the API.""" + assert owner.post("/auth/audit", json={"event": "login.success"}).status_code == 405 + assert owner.delete("/auth/audit").status_code == 405 + + +def test_non_admin_cannot_read_the_log(reader: httpx.Client) -> None: + """The log names accounts and addresses, so reading it needs admin.""" + assert_code(reader.get("/auth/audit"), 403) + assert_code(reader.get("/auth/audit/events"), 403) + + +def test_anonymous_cannot_read_the_log(anonymous: httpx.Client) -> None: + """Without a credential the log answers 401.""" + assert_code(anonymous.get("/auth/audit"), 401) diff --git a/tests_integration/tests_auth/test_levels.py b/tests_integration/tests_auth/test_levels.py index 1d5f5f1d..36d5bdb4 100644 --- a/tests_integration/tests_auth/test_levels.py +++ b/tests_integration/tests_auth/test_levels.py @@ -1,25 +1,26 @@ """Integration tests: permission levels. -Phase 1 ships no way to create a second account through the API -- there is no user -management endpoint, and the tester container has no access to a command line tool -- so -a read-level or edit-level *user* cannot be made from here. What can be exercised is -therefore split in two: +Phase 1 could only assert the ordering from above -- the owner, at manage, reaching +routes at all three levels -- because there was no way to create a second account +through the API, and the anonymous reader was the only low-level principal available. -* The owner, at the manage level, reaches routes at all three levels. That proves the - ordering read < edit < manage admits from above. -* A read-level principal is refused edit and manage routes. The anonymous reader is one, - so test_anonymous_read.py covers the refusing half of the ordering. - -When user management arrives, the natural home for a genuine read-user-versus-manage-route -test is this file. +Phase 2's POST /auth/user closes that. The `reader` fixture is a genuine read-level, +signed-in, non-anonymous user, which is what makes the refusing half of the ordering a +real test rather than an approximation: it distinguishes "refused for lack of level" +from "refused for being anonymous", which the two previous tests could not tell apart. """ +import time + import httpx import pytest from .conftest import ( OWNER_USERNAME_STORED, + READER_PASSWORD, assert_code, + create_user, + login, ) @@ -114,3 +115,74 @@ def test_owner_can_change_own_password_endpoint_level(owner: httpx.Client) -> No json={"current_password": "not-the-current-password", "new_password": "a-brand-new-password"}, ) assert_code(result, 400) + + +def test_read_user_is_refused_edit_and_manage(owner: httpx.Client, reader: httpx.Client) -> None: + """A signed-in read-level user reaches read routes and nothing above them. + + Distinct from the anonymous-reader tests: this principal is authenticated, so a + refusal here can only be about its level. An anonymous caller failing the same + request proves less, because ``allow_anonymous`` could have been what stopped it. + """ + session = reader.get("/auth/session") + assert_code(session, 200) + assert session.json()["authenticated"] is True + assert session.json()["anonymous"] is False + assert session.json()["level"] == "read" + + for path in ("/info", "/spool", "/filament", "/vendor", "/setting/"): + assert_code(reader.get(path), 200) + + vendor = owner.post("/vendor", json={"name": "Read-level test vendor"}) + assert_code(vendor, 200) + vendor_id = vendor.json()["id"] + try: + # edit + assert_code(reader.patch(f"/vendor/{vendor_id}", json={"comment": "nope"}), 403) + # manage + assert_code(reader.post("/vendor", json={"name": "should not exist"}), 403) + assert_code(reader.delete(f"/vendor/{vendor_id}"), 403) + assert_code(reader.post("/setting/currency", json='"USD"'), 403) + # The bulk exports sit at manage rather than read on purpose. + assert_code(reader.get("/export/spools", params={"fmt": "json"}), 403) + finally: + assert_code(owner.delete(f"/vendor/{vendor_id}"), 200) + + +def test_edit_user_can_edit_but_not_create_or_delete(owner: httpx.Client) -> None: + """The middle of the ordering, which nothing could exercise before. + + PUT /spool/{id}/use sits at edit rather than manage deliberately: it is Moonraker's + and OctoPrint's hot path, so a printer can be given a credential that tracks filament + usage without being able to delete anything. That distinction is only testable with a + real edit-level account. + """ + username = f"editor-{int(time.time() * 1000)}" + create_user(owner, username, level="edit", password=READER_PASSWORD) + client, response = login(username=username, password=READER_PASSWORD) + assert_code(response, 200) + + vendor = owner.post("/vendor", json={"name": "Edit-level test vendor"}) + assert_code(vendor, 200) + vendor_id = vendor.json()["id"] + filament = owner.post( + "/filament", + json={"name": "Edit-level filament", "vendor_id": vendor_id, "density": 1.24, "diameter": 1.75, "weight": 1000}, + ) + assert_code(filament, 200) + filament_id = filament.json()["id"] + spool = owner.post("/spool", json={"filament_id": filament_id, "remaining_weight": 500}) + assert_code(spool, 200) + spool_id = spool.json()["id"] + + try: + assert_code(client.patch(f"/vendor/{vendor_id}", json={"comment": "edited by editor"}), 200) + assert_code(client.put(f"/spool/{spool_id}/use", json={"use_weight": 5}), 200) + + assert_code(client.post("/vendor", json={"name": "should not exist"}), 403) + assert_code(client.delete(f"/spool/{spool_id}"), 403) + finally: + client.close() + assert_code(owner.delete(f"/spool/{spool_id}"), 200) + assert_code(owner.delete(f"/filament/{filament_id}"), 200) + assert_code(owner.delete(f"/vendor/{vendor_id}"), 200) diff --git a/tests_integration/tests_auth/test_unauthenticated.py b/tests_integration/tests_auth/test_unauthenticated.py index 99b868b0..75338e43 100644 --- a/tests_integration/tests_auth/test_unauthenticated.py +++ b/tests_integration/tests_auth/test_unauthenticated.py @@ -3,7 +3,7 @@ This is the load-bearing test of the suite. ``spoolman.auth.coverage`` already proves at import time that no route was left without a gate, but that is a static check on the route table -- it says a dependency is attached, not that the dependency actually -rejects anybody. This file exercises the real thing over HTTP: 40 gated HTTP routes, +rejects anybody. This file exercises the real thing over HTTP: every gated HTTP route, each of which must answer 401 to a request carrying nothing. The table below is written out rather than derived, so that adding a route forces a @@ -57,6 +57,13 @@ GATED_ROUTES: list[tuple[str, str, str]] = [ # Changing one's own password needs a real signed-in user, not merely read rights, # but the level recorded on the gate is read. ("POST", "/auth/password", "read"), + # The API key routes sit at read for the same reason: their gate is require_user, + # which is require_level(read) plus a refusal of anonymous callers and of API keys. + # A key that could mint a key would be unrevocable in practice. + ("GET", "/auth/apikey", "read"), + ("POST", "/auth/apikey", "read"), + ("POST", "/auth/apikey/1/revoke", "read"), + ("DELETE", "/auth/apikey/1", "read"), # -- edit ------------------------------------------------------------------- ("PATCH", "/filament/1", "edit"), ("PATCH", "/spool/1", "edit"), @@ -80,6 +87,21 @@ GATED_ROUTES: list[tuple[str, str, str]] = [ ("GET", "/export/spools?fmt=json", "manage"), ("GET", "/export/filaments?fmt=json", "manage"), ("GET", "/export/vendors?fmt=json", "manage"), + # User administration and the audit log are recorded as manage, which is what their + # gate enforces; require_admin adds the administrator check on top of it. An + # unauthenticated caller is refused by the level gate first, which is what this file + # asserts -- the admin half is covered in test_users.py and test_audit.py. + ("GET", "/auth/user", "manage"), + ("POST", "/auth/user", "manage"), + ("GET", "/auth/user/1", "manage"), + ("PATCH", "/auth/user/1", "manage"), + ("DELETE", "/auth/user/1", "manage"), + ("POST", "/auth/user/1/password", "manage"), + ("POST", "/auth/user/1/revoke-sessions", "manage"), + # The audit log holds addresses, user agents and the list of accounts, so serving it + # at read would hand an anonymous browser a map of the instance's users. + ("GET", "/auth/audit", "manage"), + ("GET", "/auth/audit/events", "manage"), ] # The routes that must stay reachable with no credentials, mirroring PUBLIC_ROUTES in @@ -113,6 +135,11 @@ def _template(path: str) -> str: "/field/spool/some_field/values": "/field/{entity_type}/{key}/values", "/field/spool/some_field": "/field/{entity_type}/{key}", "/field/spool": "/field/{entity_type}", + "/auth/apikey/1/revoke": "/auth/apikey/{key_id}/revoke", + "/auth/apikey/1": "/auth/apikey/{key_id}", + "/auth/user/1/revoke-sessions": "/auth/user/{user_id}/revoke-sessions", + "/auth/user/1/password": "/auth/user/{user_id}/password", + "/auth/user/1": "/auth/user/{user_id}", } return replacements.get(path, path) @@ -132,8 +159,12 @@ def test_gated_route_requires_credentials(anonymous: httpx.Client, method: str, def test_every_gated_route_is_listed() -> None: - """Guard the count, so a route added without a test entry is noticed.""" - assert len(GATED_ROUTES) == 40 + """Guard the count, so a route added without a test entry is noticed. + + Phase 1 gated 40 HTTP routes. Phase 2 added thirteen: four for API keys, seven for + user administration, two for the audit log. + """ + assert len(GATED_ROUTES) == 53 @pytest.mark.parametrize( diff --git a/tests_integration/tests_auth/test_users.py b/tests_integration/tests_auth/test_users.py new file mode 100644 index 00000000..be231cff --- /dev/null +++ b/tests_integration/tests_auth/test_users.py @@ -0,0 +1,248 @@ +"""Integration tests: user administration. + +Two things are being defended here. + +The first is that an instance cannot be administered into a state nobody can recover +from through the interface: the owner and the caller's own account are shielded from +demotion, disabling and deletion. Neither is a security boundary -- both are recoverable +with ``python -m spoolman.cli`` -- so the tests assert the refusal, not that it is +unbypassable. + +The second is that the protections only fire on changes that actually take something +away. A form that re-sends an account unchanged must not be refused just because it +included a field the caller may not lower. +""" + +import time + +import httpx + +from .conftest import ( + OWNER_USERNAME_STORED, + READER_PASSWORD, + assert_code, + create_user, + login, +) + + +def _unique(prefix: str) -> str: + """Build a username no other test in this run will collide with.""" + return f"{prefix}-{int(time.time() * 1000)}" + + +def test_create_generates_a_password_when_none_is_given(owner: httpx.Client) -> None: + """A generated password comes back once, and works.""" + username = _unique("generated") + response = owner.post("/auth/user", json={"username": username, "level": "read"}) + assert_code(response, 201) + body = response.json() + + password = body["password"] + assert password, "a password should have been generated" + assert body["user"]["must_change_password"] is True, "generated passwords default to one-time" + + client, login_response = login(username=username, password=password) + assert_code(login_response, 200) + client.close() + + # And it is never retrievable again. + fetched = owner.get(f"/auth/user/{body['user']['id']}") + assert_code(fetched, 200) + assert password not in str(fetched.json()) + + +def test_create_accepts_a_supplied_password(owner: httpx.Client) -> None: + """When the caller supplies a password, none is generated or echoed.""" + username = _unique("supplied") + response = owner.post( + "/auth/user", + json={"username": username, "password": READER_PASSWORD, "level": "edit"}, + ) + assert_code(response, 201) + assert response.json().get("password") is None + + client, login_response = login(username=username, password=READER_PASSWORD) + assert_code(login_response, 200) + client.close() + + +def test_usernames_are_unique_and_normalised(owner: httpx.Client) -> None: + """A second account with the same name, in any case, is refused.""" + username = _unique("Unique") + create_user(owner, username, password=READER_PASSWORD) + + clash = owner.post("/auth/user", json={"username": username.upper(), "level": "read"}) + assert_code(clash, 409) + assert "already exists" in clash.json()["message"] + + +def test_list_includes_every_account(owner: httpx.Client) -> None: + """The listing shows the owner alongside whatever the suite has created.""" + username = _unique("listed") + create_user(owner, username, password=READER_PASSWORD) + + response = owner.get("/auth/user") + assert_code(response, 200) + usernames = {user["username"] for user in response.json()} + assert OWNER_USERNAME_STORED in usernames + assert username.lower() in usernames + assert response.headers["x-total-count"] == str(len(response.json())) + + +def test_update_changes_level_and_flags(owner: httpx.Client) -> None: + """Level, administrator and display name all take effect.""" + user_id, _ = create_user(owner, _unique("updated"), level="read", password=READER_PASSWORD) + + response = owner.patch( + f"/auth/user/{user_id}", + json={"level": "manage", "is_admin": True, "display_name": "Renamed"}, + ) + assert_code(response, 200) + body = response.json() + assert body["level"] == "manage" + assert body["is_admin"] is True + assert body["display_name"] == "Renamed" + + +def test_owner_cannot_be_demoted_disabled_or_deleted(owner: httpx.Client) -> None: + """The account that can always fix the others cannot be taken apart.""" + session = owner.get("/auth/session") + assert_code(session, 200) + owner_id = session.json()["user"]["id"] + + for change in ({"level": "read"}, {"is_admin": False}, {"is_active": False}): + refused = owner.patch(f"/auth/user/{owner_id}", json=change) + assert_code(refused, 403) + assert "owner" in refused.json()["message"].lower() + + assert_code(owner.delete(f"/auth/user/{owner_id}"), 403) + + +def test_unchanged_fields_are_not_refused(owner: httpx.Client) -> None: + """Re-sending an account's current values is a no-op, not a violation. + + The protections compare against what is stored, so a form that submits every field + -- including the owner's own level -- succeeds as long as nothing actually changes. + """ + session = owner.get("/auth/session") + assert_code(session, 200) + owner_id = session.json()["user"]["id"] + + response = owner.patch( + f"/auth/user/{owner_id}", + json={"level": "manage", "is_admin": True, "is_active": True}, + ) + assert_code(response, 200) + assert response.json()["level"] == "manage" + + +def test_admin_cannot_demote_or_delete_themselves(owner: httpx.Client) -> None: + """Removing your own rights needs a second pair of hands.""" + username = _unique("selfadmin") + user_id, password = create_user(owner, username, level="manage", is_admin=True, password=READER_PASSWORD) + + client, response = login(username=username, password=password) + assert_code(response, 200) + try: + for change in ({"level": "read"}, {"is_admin": False}, {"is_active": False}): + refused = client.patch(f"/auth/user/{user_id}", json=change) + assert_code(refused, 403) + assert_code(client.delete(f"/auth/user/{user_id}"), 403) + + # Somebody else with the rights can, which is the point of the restriction. + assert_code(owner.patch(f"/auth/user/{user_id}", json={"is_admin": False}), 200) + finally: + client.close() + + +def test_disabling_an_account_ends_its_sessions(owner: httpx.Client) -> None: + """A disabled account whose cookie still worked would not be disabled.""" + username = _unique("cutoff") + user_id, password = create_user(owner, username, level="read", password=READER_PASSWORD) + + client, response = login(username=username, password=password) + assert_code(response, 200) + try: + assert_code(client.get("/spool"), 200) + assert_code(owner.patch(f"/auth/user/{user_id}", json={"is_active": False}), 200) + assert_code(client.get("/spool"), 401) + finally: + client.close() + + +def test_password_reset_ends_sessions_and_sets_the_new_password(owner: httpx.Client) -> None: + """A reset invalidates what the old password bought.""" + username = _unique("reset") + user_id, password = create_user(owner, username, level="read", password=READER_PASSWORD) + + client, response = login(username=username, password=password) + assert_code(response, 200) + try: + assert_code(client.get("/spool"), 200) + + reset = owner.post(f"/auth/user/{user_id}/password", json={"must_change_password": True}) + assert_code(reset, 200) + new_password = reset.json()["password"] + assert new_password + assert new_password != password + + # Whoever knew the old password may be exactly who the reset defends against. + assert_code(client.get("/spool"), 401) + finally: + client.close() + + fresh, response = login(username=username, password=new_password) + assert_code(response, 200) + try: + assert response.json()["user"]["must_change_password"] is True + finally: + fresh.close() + + +def test_revoke_sessions_without_changing_the_password(owner: httpx.Client) -> None: + """Signing somebody out everywhere leaves their credential alone.""" + username = _unique("revoked") + user_id, password = create_user(owner, username, level="read", password=READER_PASSWORD) + + client, response = login(username=username, password=password) + assert_code(response, 200) + try: + assert_code(owner.post(f"/auth/user/{user_id}/revoke-sessions"), 200) + assert_code(client.get("/spool"), 401) + finally: + client.close() + + again, response = login(username=username, password=password) + assert_code(response, 200) + again.close() + + +def test_delete_removes_the_account_and_its_sessions(owner: httpx.Client) -> None: + """A deleted account cannot sign in and is gone from the listing.""" + username = _unique("gone") + user_id, password = create_user(owner, username, level="read", password=READER_PASSWORD) + + assert_code(owner.delete(f"/auth/user/{user_id}"), 200) + assert_code(owner.get(f"/auth/user/{user_id}"), 404) + + listed = owner.get("/auth/user") + assert_code(listed, 200) + assert all(user["id"] != user_id for user in listed.json()) + + _, response = login(username=username, password=password) + assert_code(response, 401) + + +def test_non_admin_is_refused_every_user_endpoint(reader: httpx.Client) -> None: + """Managing accounts needs administrator rights, not merely a session.""" + assert_code(reader.get("/auth/user"), 403) + assert_code(reader.post("/auth/user", json={"username": "nope", "level": "read"}), 403) + assert_code(reader.patch("/auth/user/1", json={"level": "read"}), 403) + assert_code(reader.delete("/auth/user/1"), 403) + + +def test_anonymous_is_refused_every_user_endpoint(anonymous: httpx.Client) -> None: + """Without any credential the endpoints answer 401, not 403.""" + assert_code(anonymous.get("/auth/user"), 401) + assert_code(anonymous.post("/auth/user", json={"username": "nope", "level": "read"}), 401)