diff --git a/src/porchlight/admin/routes.py b/src/porchlight/admin/routes.py index 4bcc0a8..fe7404f 100644 --- a/src/porchlight/admin/routes.py +++ b/src/porchlight/admin/routes.py @@ -11,6 +11,15 @@ from porchlight.validation import GroupListInput, ProfileUpdate, UsernameInput, router = APIRouter(prefix="/admin", tags=["admin"]) +ADMIN_GROUP = "admin" + + +async def _is_last_active_admin(request: Request, user: User) -> bool: + """True if removing this user's admin access would leave zero active admins.""" + if not user.active or ADMIN_GROUP not in user.groups: + return False + return await request.app.state.user_repo.count_active_admins() <= 1 + async def _get_admin_user(request: Request) -> User | None: """Return the current user if they are an admin, else None.""" @@ -198,6 +207,9 @@ async def update_user_groups( if user is None: return HTMLResponse("User not found", status_code=404) + if ADMIN_GROUP not in validated.group_list and await _is_last_active_admin(request, user): + return HTMLResponse('
Cannot remove the last active admin
') + updated = user.model_copy(update={"groups": validated.group_list}) await user_repo.update(updated) return HTMLResponse('
Groups updated
') @@ -241,6 +253,9 @@ async def deactivate_user(request: Request, userid: str) -> Response: if user is None: return HTMLResponse("User not found", status_code=404) + if await _is_last_active_admin(request, user): + return HTMLResponse('
Cannot deactivate the last active admin
') + updated = user.model_copy(update={"active": False}) await user_repo.update(updated) return HTMLResponse( @@ -261,10 +276,12 @@ async def delete_user_password(request: Request, userid: str) -> Response: return HTMLResponse("Forbidden", status_code=403) cred_repo = request.app.state.credential_repo - await cred_repo.delete_password(userid) + # Refuse atomically if this is the user's last credential (avoids lockout). + deleted = await cred_repo.delete_password_if_not_last(userid) # Re-render credentials section webauthn_credentials = await cred_repo.get_webauthn_by_user(userid) + password = await cred_repo.get_password_by_user(userid) templates = request.app.state.templates return templates.TemplateResponse( request, @@ -272,7 +289,8 @@ async def delete_user_password(request: Request, userid: str) -> Response: { "target_user": await request.app.state.user_repo.get_by_userid(userid), "webauthn_credentials": webauthn_credentials, - "has_password": False, + "has_password": password is not None, + "error": None if deleted else "Cannot remove the user's last credential", }, ) @@ -289,7 +307,8 @@ async def delete_user_webauthn(request: Request, userid: str, credential_id_b64: padded = credential_id_b64 + "=" * (-len(credential_id_b64) % 4) credential_id = urlsafe_b64decode(padded) cred_repo = request.app.state.credential_repo - await cred_repo.delete_webauthn(userid, credential_id) + # Refuse atomically if this is the user's last credential (avoids lockout). + deleted = await cred_repo.delete_webauthn_if_not_last(userid, credential_id) # Re-render credentials section webauthn_credentials = await cred_repo.get_webauthn_by_user(userid) @@ -302,6 +321,7 @@ async def delete_user_webauthn(request: Request, userid: str, credential_id_b64: "target_user": await request.app.state.user_repo.get_by_userid(userid), "webauthn_credentials": webauthn_credentials, "has_password": password is not None, + "error": None if deleted else "Cannot remove the user's last credential", }, ) @@ -345,6 +365,10 @@ async def delete_user(request: Request, userid: str) -> Response: return HTMLResponse('
Cannot delete your own account
') user_repo = request.app.state.user_repo + target = await user_repo.get_by_userid(userid) + if target is not None and await _is_last_active_admin(request, target): + return HTMLResponse('
Cannot delete the last active admin
') + deleted = await user_repo.delete(userid) if not deleted: return HTMLResponse("User not found", status_code=404) diff --git a/src/porchlight/authn/routes.py b/src/porchlight/authn/routes.py index c710f3f..606b315 100644 --- a/src/porchlight/authn/routes.py +++ b/src/porchlight/authn/routes.py @@ -23,6 +23,21 @@ def _login_redirect_target(request: Request) -> str: return "/manage/credentials" +def _establish_authenticated_session(request: Request, user: User) -> None: + """Reset the session before recording the authenticated identity. + + Clearing first defeats session fixation: any values an attacker planted in + the pre-auth session are dropped and the session cookie is reissued. A + pending OIDC authorization request is the only pre-auth state worth keeping. + """ + pending_oidc = request.session.get("oidc_auth_request") + request.session.clear() + if pending_oidc is not None: + request.session["oidc_auth_request"] = pending_oidc + request.session["userid"] = user.userid + request.session["username"] = user.username + + @router.get("/login", response_class=HTMLResponse) async def login_page(request: Request) -> HTMLResponse: templates = request.app.state.templates @@ -56,8 +71,7 @@ async def login_password( if not user.active: return HTMLResponse(error_html) - request.session["userid"] = user.userid - request.session["username"] = user.username + _establish_authenticated_session(request, user) response = Response() response.headers["HX-Redirect"] = _login_redirect_target(request) @@ -72,12 +86,34 @@ async def logout(request: Request) -> Response: return response -@router.get("/register/{token}") +@router.get("/register/{token}", response_class=HTMLResponse) +async def register_magic_link_page(request: Request, token: str) -> Response: + """Show the registration confirmation page. + + GET is side-effect free: it only validates the token (without consuming it) + and renders a form. The token is consumed by the POST below, so that simply + visiting the link (email scanners, prefetchers) cannot create a session. + """ + magic_link_service = request.app.state.magic_link_service + link = await magic_link_service.validate(token) + if link is None: + return HTMLResponse("

Invalid or expired registration link.

", status_code=400) + + templates = request.app.state.templates + return templates.TemplateResponse( + request, + "register.html", + {"token": token, "username": link.username}, + ) + + +@router.post("/register/{token}") async def register_magic_link(request: Request, token: str) -> Response: magic_link_service = request.app.state.magic_link_service user_repo = request.app.state.user_repo - link = await magic_link_service.validate(token) + # Atomically validate and consume the token (single-use, no replay race). + link = await magic_link_service.consume(token) if link is None: return HTMLResponse("

Invalid or expired registration link.

", status_code=400) @@ -85,16 +121,25 @@ async def register_magic_link(request: Request, token: str) -> Response: if existing_user is not None: if not existing_user.active: return HTMLResponse("

This account has been deactivated.

", status_code=400) + # An invite link is for account setup, not authentication. If the + # account already has credentials, refuse to establish a session — + # otherwise a re-invite would be a passwordless login. Account + # recovery must go through a separate, explicitly authenticated flow. + cred_repo = request.app.state.credential_repo + has_password = await cred_repo.get_password_by_user(existing_user.userid) is not None + has_webauthn = bool(await cred_repo.get_webauthn_by_user(existing_user.userid)) + if has_password or has_webauthn: + return HTMLResponse( + "

This account is already set up. Please sign in instead.

", + status_code=400, + ) user = existing_user else: userid = await generate_unique_userid(user_repo) user = User(userid=userid, username=link.username, groups=["users"]) await user_repo.create(user) - await magic_link_service.mark_used(token) - - request.session["userid"] = user.userid - request.session["username"] = user.username + _establish_authenticated_session(request, user) return RedirectResponse("/manage/credentials?setup=1", status_code=303) @@ -156,7 +201,6 @@ async def login_webauthn_complete(request: Request) -> Response: if user is None or not user.active: return JSONResponse({"error": "Authentication failed"}, status_code=400) - request.session["userid"] = user.userid - request.session["username"] = user.username + _establish_authenticated_session(request, user) return JSONResponse({"redirect": _login_redirect_target(request)}) diff --git a/src/porchlight/config.py b/src/porchlight/config.py index 38fb597..dff55a5 100644 --- a/src/porchlight/config.py +++ b/src/porchlight/config.py @@ -54,6 +54,10 @@ class Settings(BaseSettings): # Rate limiting (disable for e2e/load tests that authenticate repeatedly) rate_limit_enabled: bool = True + # Number of trusted reverse proxies in front of the app. When > 0, the + # client IP for rate limiting is taken from X-Forwarded-For, counting this + # many hops from the right. Keep 0 unless deployed behind a known proxy. + trusted_proxy_count: int = 0 # Signing keys signing_key_path: str = "data/keys" diff --git a/src/porchlight/invite/service.py b/src/porchlight/invite/service.py index fd5bebf..4766442 100644 --- a/src/porchlight/invite/service.py +++ b/src/porchlight/invite/service.py @@ -1,3 +1,5 @@ +import hashlib +import hmac import secrets from datetime import UTC, datetime, timedelta @@ -6,11 +8,23 @@ from porchlight.store.protocols import MagicLinkRepository class MagicLinkService: - """Magic link token lifecycle management.""" + """Magic link token lifecycle management. - def __init__(self, repo: MagicLinkRepository, ttl: int = 86400) -> None: + The raw token is only ever exposed to the user (in the registration URL). + Only a hash of the token is persisted, so a database disclosure does not + yield usable tokens. A pepper, when configured, keys the hash (HMAC); + otherwise a plain SHA-256 of the high-entropy token is stored. + """ + + def __init__(self, repo: MagicLinkRepository, ttl: int = 86400, pepper: str | None = None) -> None: self._repo = repo self._ttl = ttl + self._pepper = pepper + + def _hash_token(self, token: str) -> str: + if self._pepper: + return hmac.new(self._pepper.encode(), token.encode(), hashlib.sha256).hexdigest() + return hashlib.sha256(token.encode()).hexdigest() async def create( self, @@ -18,34 +32,52 @@ class MagicLinkService: created_by: str | None = None, note: str | None = None, ) -> MagicLink: - """Generate and store a new magic link for the given username.""" + """Generate and store a new magic link for the given username. + + Persists only the hashed token; the returned MagicLink carries the raw + token so the caller can build the registration URL. + """ token = secrets.token_urlsafe(32) expires_at = datetime.now(UTC) + timedelta(seconds=self._ttl) - link = MagicLink( - token=token, + stored = MagicLink( + token=self._hash_token(token), username=username, expires_at=expires_at, created_by=created_by, note=note, ) - return await self._repo.create(link) + await self._repo.create(stored) + return stored.model_copy(update={"token": token}) async def validate(self, token: str) -> MagicLink | None: """Validate a magic link token. - Returns the MagicLink if it exists, is not used, and has not expired. - Returns None otherwise. + Returns the MagicLink (with the raw token re-attached) if it exists, is + not used, and has not expired. Returns None otherwise. """ - link = await self._repo.get_by_token(token) + link = await self._repo.get_by_token(self._hash_token(token)) if link is None or link.used: return None if link.expires_at < datetime.now(UTC): return None - return link + return link.model_copy(update={"token": token}) async def mark_used(self, token: str) -> bool: """Mark a magic link as used. Returns True if found and marked.""" - return await self._repo.mark_used(token) + return await self._repo.mark_used(self._hash_token(token)) + + async def consume(self, token: str) -> MagicLink | None: + """Atomically validate and consume a token in one step. + + Prefer this over validate()+mark_used(): it closes the replay race + where two concurrent requests could both pass validation before either + marks the token used. Returns the link (raw token re-attached) on + success, else None. + """ + link = await self._repo.consume(self._hash_token(token)) + if link is None: + return None + return link.model_copy(update={"token": token}) async def cleanup_expired(self) -> int: """Delete expired unused links. Returns count deleted.""" diff --git a/src/porchlight/manage/routes.py b/src/porchlight/manage/routes.py index 69afdff..92a7912 100644 --- a/src/porchlight/manage/routes.py +++ b/src/porchlight/manage/routes.py @@ -13,13 +13,6 @@ from porchlight.validation import PasswordChange, PasswordSet, ProfileUpdate, fo router = APIRouter(prefix="/manage", tags=["manage"]) -async def _count_credentials(cred_repo: object, userid: str) -> int: - """Count total credentials (password + webauthn) for a user.""" - webauthn = await cred_repo.get_webauthn_by_user(userid) # type: ignore[union-attr] - password = await cred_repo.get_password_by_user(userid) # type: ignore[union-attr] - return len(webauthn) + (1 if password else 0) - - @router.get("/credentials", response_class=HTMLResponse) async def credentials_page(request: Request) -> Response: session_user = get_session_user(request) @@ -107,11 +100,9 @@ async def delete_password(request: Request) -> Response: userid, _username = session_user cred_repo = request.app.state.credential_repo - count = await _count_credentials(cred_repo, userid) - if count <= 1: + # Atomic: refuses if this is the user's last credential (not raceable). + if not await cred_repo.delete_password_if_not_last(userid): return HTMLResponse('
Cannot remove your last credential
') - - await cred_repo.delete_password(userid) return HTMLResponse('
Password removed
') @@ -182,11 +173,9 @@ async def delete_webauthn(request: Request, credential_id_b64: str) -> Response: padded = credential_id_b64 + "=" * (-len(credential_id_b64) % 4) credential_id = urlsafe_b64decode(padded) - count = await _count_credentials(cred_repo, userid) - if count <= 1: + # Atomic: refuses if this is the user's last credential (not raceable). + if not await cred_repo.delete_webauthn_if_not_last(userid, credential_id): return HTMLResponse('
Cannot remove your last credential
') - - await cred_repo.delete_webauthn(userid, credential_id) return HTMLResponse('
Security key removed
') diff --git a/src/porchlight/oidc/endpoints.py b/src/porchlight/oidc/endpoints.py index 67d4968..12af079 100644 --- a/src/porchlight/oidc/endpoints.py +++ b/src/porchlight/oidc/endpoints.py @@ -2,6 +2,7 @@ from __future__ import annotations +import html import json from types import SimpleNamespace from urllib.parse import urlencode @@ -14,6 +15,15 @@ from porchlight.oidc.claims import PorchlightUserInfo, user_to_claims router = APIRouter(tags=["oidc"]) + +def _error_page(message: object, status_code: int = 400, title: str = "Error") -> HTMLResponse: + """Render an error page, escaping the (possibly request-derived) message.""" + return HTMLResponse( + f"

{html.escape(title)}

{html.escape(str(message))}

", + status_code=status_code, + ) + + SCOPE_DESCRIPTIONS: dict[str, str] = { "openid": "Sign you in (required)", "profile": "Your name and profile information", @@ -59,11 +69,11 @@ async def authorization(request: Request) -> Response: try: parsed = endpoint.parse_request(query_params) except Exception as exc: - return HTMLResponse(f"

Invalid Request

{exc}

", status_code=400) + return _error_page(exc, title="Invalid Request") if "error" in parsed: error_desc = parsed.get("error_description", parsed["error"]) - return HTMLResponse(f"

Error

{error_desc}

", status_code=400) + return _error_page(error_desc) # Check if user is authenticated userid = request.session.get("userid") @@ -95,11 +105,11 @@ async def authorization_complete(request: Request) -> Response: try: parsed = endpoint.parse_request(auth_request_params) except Exception as exc: - return HTMLResponse(f"

Error

{exc}

", status_code=400) + return _error_page(exc) if "error" in parsed: error_desc = parsed.get("error_description", parsed["error"]) - return HTMLResponse(f"

Error

{error_desc}

", status_code=400) + return _error_page(error_desc) return await _check_consent_or_complete( request, oidc_server, endpoint, parsed, userid, username, auth_request_params @@ -172,7 +182,7 @@ async def _complete_authorization( # noqa: PLR0913 if "error" in result.get("response_args", {}): response_args = result["response_args"] error_desc = response_args.get("error_description", response_args["error"]) - return HTMLResponse(f"

Error

{error_desc}

", status_code=400) + return _error_page(error_desc) # Build redirect URL response_args = result.get("response_args", {}) @@ -338,8 +348,10 @@ async def consent_submit(request: Request) -> Response: return RedirectResponse(f"{redirect_uri}?{params}", status_code=303) return HTMLResponse("

Error

Invalid action

", status_code=400) - # Allow — collect approved scopes - approved_scopes: list[str] = [str(s) for s in form.getlist("scope")] + # Allow — collect approved scopes, rejecting anything outside the + # originally requested set (a forged form must not escalate scope). + requested_scopes = set(auth_params.get("scope", "openid").split()) + approved_scopes: list[str] = [str(s) for s in form.getlist("scope") if str(s) in requested_scopes] if "openid" not in approved_scopes: approved_scopes = ["openid", *list(approved_scopes)] @@ -359,6 +371,6 @@ async def consent_submit(request: Request) -> Response: if "error" in parsed: raise ValueError(parsed.get("error_description", parsed["error"])) except Exception as exc: - return HTMLResponse(f"

Error

{exc}

", status_code=400) + return _error_page(exc) return await _complete_authorization(request, oidc_server, endpoint, parsed, userid, username) diff --git a/src/porchlight/oidc/provider.py b/src/porchlight/oidc/provider.py index d1ac38b..aae64d3 100644 --- a/src/porchlight/oidc/provider.py +++ b/src/porchlight/oidc/provider.py @@ -3,6 +3,7 @@ from pathlib import Path from idpyoidc.server import Server +from idpyoidc.server.oauth2.add_on.pkce import CC_METHOD from porchlight.config import Settings from porchlight.oidc.claims import PorchlightUserInfo @@ -126,6 +127,20 @@ def _build_server_config(settings: Settings) -> dict: "phone": ["phone_number", "phone_number_verified"], }, "authentication": {}, + # PKCE: advertise and verify S256 (only). Not "essential", so existing + # relying parties that do not yet send a code_challenge keep working; + # but any RP that does use PKCE must use S256 and is verified at the + # token endpoint. Set essential=True (or per-client pkce_essential) to + # require it once all clients have migrated. + "add_on": { + "pkce": { + "function": "idpyoidc.server.oauth2.add_on.pkce.add_support", + "kwargs": { + "essential": False, + "code_challenge_methods": {"S256": CC_METHOD["S256"]}, + }, + }, + }, } diff --git a/src/porchlight/rate_limit.py b/src/porchlight/rate_limit.py index 38404a8..a0a6eff 100644 --- a/src/porchlight/rate_limit.py +++ b/src/porchlight/rate_limit.py @@ -1,4 +1,25 @@ from slowapi import Limiter from slowapi.util import get_remote_address +from starlette.requests import Request -limiter = Limiter(key_func=get_remote_address) + +def _client_identifier(request: Request) -> str: + """Rate-limit key: the client IP, proxy-aware. + + By default the direct peer address is used. When ``trusted_proxy_count`` is + configured (the number of trusted reverse proxies in front of the app), the + client address is taken from the ``X-Forwarded-For`` chain instead, counting + that many hops back from the right. The header is ignored unless proxies are + trusted, so it cannot be spoofed in a direct-connection deployment. + """ + settings = getattr(getattr(request.app, "state", None), "settings", None) + hops: int = getattr(settings, "trusted_proxy_count", 0) or 0 + if hops > 0: + forwarded = request.headers.get("x-forwarded-for", "") + parts = [p.strip() for p in forwarded.split(",") if p.strip()] + if len(parts) >= hops: + return parts[-hops] + return get_remote_address(request) + + +limiter = Limiter(key_func=_client_identifier) diff --git a/src/porchlight/static/style.css b/src/porchlight/static/style.css index 17319ff..15dc79e 100644 --- a/src/porchlight/static/style.css +++ b/src/porchlight/static/style.css @@ -205,6 +205,10 @@ main { line-height: 1; } +#user-table-container { + overflow-x: auto; +} + .admin-table { width: 100%; border-collapse: collapse; @@ -214,7 +218,7 @@ main { .admin-table th, .admin-table td { text-align: left; - padding: var(--sp-2) var(--sp-3); + padding: var(--sp-2); border-bottom: 1px solid var(--border); } diff --git a/src/porchlight/store/protocols.py b/src/porchlight/store/protocols.py index ba1af34..53e7c37 100644 --- a/src/porchlight/store/protocols.py +++ b/src/porchlight/store/protocols.py @@ -25,6 +25,8 @@ class UserRepository(Protocol): async def count_users(self, query: str | None = None) -> int: ... + async def count_active_admins(self) -> int: ... + async def delete(self, userid: str) -> bool: ... @@ -46,6 +48,10 @@ class CredentialRepository(Protocol): async def delete_password(self, user_id: str) -> bool: ... + async def delete_password_if_not_last(self, user_id: str) -> bool: ... + + async def delete_webauthn_if_not_last(self, user_id: str, credential_id: bytes) -> bool: ... + @runtime_checkable class MagicLinkRepository(Protocol): @@ -55,6 +61,8 @@ class MagicLinkRepository(Protocol): async def mark_used(self, token: str) -> bool: ... + async def consume(self, token: str) -> MagicLink | None: ... + async def delete_expired(self) -> int: ... diff --git a/src/porchlight/store/sqlite/repositories.py b/src/porchlight/store/sqlite/repositories.py index 37d02bb..f9ebd81 100644 --- a/src/porchlight/store/sqlite/repositories.py +++ b/src/porchlight/store/sqlite/repositories.py @@ -163,6 +163,17 @@ class SQLiteUserRepository: row = await cursor.fetchone() return row[0] if row else 0 + async def count_active_admins(self) -> int: + async with self._db.execute( + """ + SELECT COUNT(*) FROM users u + JOIN user_groups g ON g.userid = u.userid + WHERE u.active = 1 AND g.group_name = 'admin' + """ + ) as cursor: + row = await cursor.fetchone() + return row[0] if row else 0 + async def delete(self, userid: str) -> bool: cursor = await self._db.execute("DELETE FROM users WHERE userid = ?", (userid,)) await self._db.commit() @@ -266,6 +277,41 @@ class SQLiteCredentialRepository: await self._db.commit() return cursor.rowcount > 0 + async def delete_password_if_not_last(self, user_id: str) -> bool: + """Delete the password credential only if it is not the user's last + credential. The count and delete happen in one atomic statement, so it + is not raceable. Returns True if a row was deleted.""" + cursor = await self._db.execute( + """ + DELETE FROM password_credentials + WHERE user_id = ? + AND ( + (SELECT COUNT(*) FROM password_credentials WHERE user_id = ?) + + (SELECT COUNT(*) FROM webauthn_credentials WHERE user_id = ?) + ) > 1 + """, + (user_id, user_id, user_id), + ) + await self._db.commit() + return cursor.rowcount > 0 + + async def delete_webauthn_if_not_last(self, user_id: str, credential_id: bytes) -> bool: + """Delete a WebAuthn credential only if it is not the user's last + credential, atomically. Returns True if a row was deleted.""" + cursor = await self._db.execute( + """ + DELETE FROM webauthn_credentials + WHERE user_id = ? AND credential_id = ? + AND ( + (SELECT COUNT(*) FROM password_credentials WHERE user_id = ?) + + (SELECT COUNT(*) FROM webauthn_credentials WHERE user_id = ?) + ) > 1 + """, + (user_id, credential_id, user_id, user_id), + ) + await self._db.commit() + return cursor.rowcount > 0 + class SQLiteMagicLinkRepository: def __init__(self, db: aiosqlite.Connection) -> None: @@ -311,6 +357,25 @@ class SQLiteMagicLinkRepository: await self._db.commit() return cursor.rowcount > 0 + async def consume(self, token: str) -> MagicLink | None: + """Atomically validate-and-mark a token as used. + + The conditional UPDATE is the single point of decision, so two + concurrent requests cannot both consume the same token. Returns the + link if this call won the race (unused and unexpired), else None. + """ + now = datetime.now(UTC).isoformat() + cursor = await self._db.execute( + "UPDATE magic_links SET used = 1 WHERE token = ? AND used = 0 AND expires_at > ?", + (token, now), + ) + await self._db.commit() + if cursor.rowcount == 0: + return None + async with self._db.execute("SELECT * FROM magic_links WHERE token = ?", (token,)) as c: + row = await c.fetchone() + return self._row_to_magic_link(row) if row is not None else None + async def delete_expired(self) -> int: now = datetime.now(UTC).isoformat() cursor = await self._db.execute("DELETE FROM magic_links WHERE expires_at < ? AND used = 0", (now,)) diff --git a/src/porchlight/templates/admin/_credentials_section.html b/src/porchlight/templates/admin/_credentials_section.html index b2351eb..fbfbe8e 100644 --- a/src/porchlight/templates/admin/_credentials_section.html +++ b/src/porchlight/templates/admin/_credentials_section.html @@ -1,3 +1,4 @@ +{% if error %}
{{ error }}
{% endif %}

Password

{% if has_password %}

Password is set. diff --git a/src/porchlight/templates/admin/base.html b/src/porchlight/templates/admin/base.html index 686a815..4a82870 100644 --- a/src/porchlight/templates/admin/base.html +++ b/src/porchlight/templates/admin/base.html @@ -4,6 +4,7 @@

{% block admin_content %}{% endblock %} diff --git a/src/porchlight/templates/register.html b/src/porchlight/templates/register.html new file mode 100644 index 0000000..6b9a668 --- /dev/null +++ b/src/porchlight/templates/register.html @@ -0,0 +1,14 @@ +{% extends "base.html" %} + +{% block title %}Set up your account — Porchlight{% endblock %} + +{% block content %} +

Set up your account

+ +

You're about to set up the account for {{ username }}.

+ +
+ + +
+{% endblock %} diff --git a/src/porchlight/validation.py b/src/porchlight/validation.py index 5d3b48c..e675e0b 100644 --- a/src/porchlight/validation.py +++ b/src/porchlight/validation.py @@ -1,3 +1,4 @@ +import html import re from typing import Annotated from urllib.parse import urlparse @@ -166,7 +167,9 @@ def format_validation_errors(exc: ValidationError) -> str: display_msg = f"{label}: {raw}" else: display_msg = f"{label}: {msg}" - messages.append(display_msg) + # Escape: messages can echo user input (e.g. an invalid group name), + # and the result is interpolated into HTML below. + messages.append(html.escape(display_msg)) if len(messages) == 1: return f'
{messages[0]}
' diff --git a/tests/e2e/full-flow.spec.js b/tests/e2e/full-flow.spec.js index be797d2..584078f 100644 --- a/tests/e2e/full-flow.spec.js +++ b/tests/e2e/full-flow.spec.js @@ -9,7 +9,9 @@ test.describe('Full user journey', () => { expect(fixtures.register_token).toBeTruthy(); // ---- Step 1: Register via magic link ---- + // GET shows a confirmation page; submitting it (POST) consumes the token. await page.goto(`/register/${fixtures.register_token}`); + await page.click('button[type="submit"]'); // Should redirect to /manage/credentials?setup=1 await page.waitForURL('**/manage/credentials?setup=1', { timeout: 5000 }); diff --git a/tests/test_admin/test_admin_routes.py b/tests/test_admin/test_admin_routes.py index 603af60..ae87b93 100644 --- a/tests/test_admin/test_admin_routes.py +++ b/tests/test_admin/test_admin_routes.py @@ -328,11 +328,15 @@ async def test_delete_password_credential(client: AsyncClient) -> None: await _login(client) target = await _create_target_user(client, username="bob") - # Create a password credential for the target user + # Create a password credential for the target user, plus a second + # credential so the password is not the last one (last-credential guard). app = client._transport.app # type: ignore[union-attr] cred_repo = app.state.credential_repo svc = PasswordService(hasher=PasswordHasher(time_cost=1, memory_cost=8192)) await cred_repo.create_password(PasswordCredential(user_id=target.userid, password_hash=svc.hash("bobpass"))) + await cred_repo.create_webauthn( + WebAuthnCredential(user_id=target.userid, credential_id=b"\xaa\xbb", public_key=b"\x00" * 32) + ) token = await get_csrf_token(client) response = await client.request( @@ -351,7 +355,8 @@ async def test_delete_webauthn_credential(client: AsyncClient) -> None: await _login(client) target = await _create_target_user(client, username="bob") - # Create a webauthn credential for the target user + # Create a webauthn credential for the target user, plus a second + # credential so the key is not the last one (last-credential guard). app = client._transport.app # type: ignore[union-attr] cred_repo = app.state.credential_repo credential_id = b"\x01\x02\x03\x04\x05\x06\x07\x08" @@ -364,6 +369,8 @@ async def test_delete_webauthn_credential(client: AsyncClient) -> None: device_name="test-key", ) ) + svc = PasswordService(hasher=PasswordHasher(time_cost=1, memory_cost=8192)) + await cred_repo.create_password(PasswordCredential(user_id=target.userid, password_hash=svc.hash("bobpass"))) # URL uses base64url without padding credential_id_b64 = urlsafe_b64encode(credential_id).decode().rstrip("=") @@ -434,3 +441,92 @@ async def test_reinvite_user_404_for_nonexistent(client: AsyncClient) -> None: response = await client.post("/admin/users/nonexistent-id/invite", headers={"X-CSRF-Token": token}) assert response.status_code == 404 assert "not found" in response.text.lower() + + +@pytest.mark.asyncio +async def test_admin_cannot_delete_users_last_credential(client: AsyncClient) -> None: + await _login(client) + target = await _create_target_user(client, userid="lonecred-01", username="lonecred") + + app = client._transport.app # type: ignore[union-attr] + cred_repo = app.state.credential_repo + svc = PasswordService(hasher=PasswordHasher(time_cost=1, memory_cost=8192)) + await cred_repo.create_password(PasswordCredential(user_id=target.userid, password_hash=svc.hash("x"))) + + token = await get_csrf_token(client) + res = await client.request( + "DELETE", + f"/admin/users/{target.userid}/credentials/password", + headers={"X-CSRF-Token": token, "HX-Request": "true"}, + ) + + # The last credential must survive, and the admin is told why. + assert await cred_repo.get_password_by_user(target.userid) is not None + assert "last credential" in res.text.lower() + + +async def _make_sole_admin_target(client: AsyncClient) -> User: + """Create a second admin, then return it as the target. The logged-in + admin from _login plus this one means 2 admins; callers deactivate the + _login admin first if they need this to be the *last* admin.""" + return await _create_target_user(client, userid="admin2-01", username="admin2", groups=["admin"]) + + +@pytest.mark.asyncio +async def test_cannot_deactivate_last_active_admin(client: AsyncClient) -> None: + await _login(client) # the only admin + app = client._transport.app # type: ignore[union-attr] + admin = await app.state.user_repo.get_by_username("admin") + + token = await get_csrf_token(client) + res = await client.post( + f"/admin/users/{admin.userid}/deactivate", + headers={"X-CSRF-Token": token, "HX-Request": "true"}, + ) + still = await app.state.user_repo.get_by_userid(admin.userid) + assert still.active is True + msg = res.text.lower() + assert "last" in msg + assert "admin" in msg + + +@pytest.mark.asyncio +async def test_cannot_remove_admin_group_from_last_admin(client: AsyncClient) -> None: + await _login(client) + app = client._transport.app # type: ignore[union-attr] + admin = await app.state.user_repo.get_by_username("admin") + + token = await get_csrf_token(client) + res = await client.post( + f"/admin/users/{admin.userid}/groups", + data={"groups": "users"}, # drops "admin" + headers={"X-CSRF-Token": token, "HX-Request": "true"}, + ) + still = await app.state.user_repo.get_by_userid(admin.userid) + assert "admin" in still.groups + msg = res.text.lower() + assert "last" in msg + assert "admin" in msg + + +@pytest.mark.asyncio +async def test_cannot_delete_last_active_admin(client: AsyncClient) -> None: + await _login(client) + app = client._transport.app # type: ignore[union-attr] + # A second, separate admin to delete (not self — self-delete is blocked separately). + target = await _create_target_user(client, userid="otheradmin-01", username="otheradmin", groups=["admin"]) + # Deactivate the logged-in admin's peer? Instead make target the only ACTIVE admin + # by deactivating the _login admin via repo, so deleting target would orphan admins. + login_admin = await app.state.user_repo.get_by_username("admin") + await app.state.user_repo.update(login_admin.model_copy(update={"active": False})) + + token = await get_csrf_token(client) + res = await client.request( + "DELETE", + f"/admin/users/{target.userid}", + headers={"X-CSRF-Token": token, "HX-Request": "true"}, + ) + assert await app.state.user_repo.get_by_userid(target.userid) is not None + msg = res.text.lower() + assert "last" in msg + assert "admin" in msg diff --git a/tests/test_admin_groups_validation.py b/tests/test_admin_groups_validation.py index ff662e9..32b1ecc 100644 --- a/tests/test_admin_groups_validation.py +++ b/tests/test_admin_groups_validation.py @@ -41,6 +41,8 @@ async def _setup_admin_and_target(client: AsyncClient) -> tuple[str, str]: data={"username": "admin_g", "password": "AdminPass123!"}, headers={"HX-Request": "true", "X-CSRF-Token": token}, ) + # Login resets the session (fixation defense); fetch a fresh CSRF token. + token = await get_csrf_token(client) return token, target.userid diff --git a/tests/test_admin_invite_validation.py b/tests/test_admin_invite_validation.py index 6bf1b68..73ccd73 100644 --- a/tests/test_admin_invite_validation.py +++ b/tests/test_admin_invite_validation.py @@ -32,7 +32,8 @@ async def _login_admin(client: AsyncClient) -> str: data={"username": "admin", "password": "AdminPass123!"}, headers={"HX-Request": "true", "X-CSRF-Token": token}, ) - return token + # Login resets the session (fixation defense); fetch a fresh CSRF token. + return await get_csrf_token(client) @pytest.mark.asyncio diff --git a/tests/test_auth_routes/test_login_session.py b/tests/test_auth_routes/test_login_session.py new file mode 100644 index 0000000..8a7f0cc --- /dev/null +++ b/tests/test_auth_routes/test_login_session.py @@ -0,0 +1,42 @@ +from datetime import UTC, datetime + +from httpx import AsyncClient + +from porchlight.authn.password import PasswordHasher, PasswordService +from porchlight.models import PasswordCredential, User +from tests.conftest import get_csrf_token + + +async def _create_active_user(client: AsyncClient) -> None: + app = client._transport.app # type: ignore[union-attr] + user = User( + userid="login-01", + username="loginuser", + created_at=datetime.now(UTC), + updated_at=datetime.now(UTC), + ) + await app.state.user_repo.create(user) + svc = PasswordService(hasher=PasswordHasher(time_cost=1, memory_cost=8192)) + await app.state.credential_repo.create_password( + PasswordCredential(user_id=user.userid, password_hash=svc.hash("password123!Secure")) + ) + + +async def test_login_resets_session_to_prevent_fixation(client: AsyncClient) -> None: + """Logging in must reset the pre-auth session so an attacker-fixed session + cannot become authenticated. We observe this via the CSRF token (stored in + the session) changing across login.""" + await _create_active_user(client) + + pre_login_token = await get_csrf_token(client) + + res = await client.post( + "/login/password", + data={"username": "loginuser", "password": "password123!Secure"}, + headers={"HX-Request": "true", "X-CSRF-Token": pre_login_token}, + ) + assert res.status_code == 200 + assert res.headers.get("HX-Redirect") # logged in + + post_login_token = await get_csrf_token(client) + assert post_login_token != pre_login_token diff --git a/tests/test_auth_routes/test_register_magic_link.py b/tests/test_auth_routes/test_register_magic_link.py index 1a03fed..7ff47ba 100644 --- a/tests/test_auth_routes/test_register_magic_link.py +++ b/tests/test_auth_routes/test_register_magic_link.py @@ -1,8 +1,10 @@ -from datetime import UTC, datetime, timedelta - +from argon2 import PasswordHasher from httpx import AsyncClient -from porchlight.models import MagicLink, User +from porchlight.authn.password import PasswordService +from porchlight.invite.service import MagicLinkService +from porchlight.models import PasswordCredential, User +from tests.conftest import get_csrf_token async def test_register_invalid_token_returns_error_page(client: AsyncClient) -> None: @@ -13,42 +15,48 @@ async def test_register_invalid_token_returns_error_page(client: AsyncClient) -> async def test_register_expired_token_returns_error_page(client: AsyncClient) -> None: app = client._transport.app # type: ignore[union-attr] - repo = app.state.magic_link_repo - await repo.create( - MagicLink( - token="expired", - username="newuser", - expires_at=datetime.now(UTC) - timedelta(hours=1), - ) - ) + # An already-expired link (negative TTL), stored hashed like the real service. + expired_service = MagicLinkService(repo=app.state.magic_link_repo, ttl=-3600) + link = await expired_service.create(username="newuser") - res = await client.get("/register/expired", follow_redirects=False) + res = await client.get(f"/register/{link.token}", follow_redirects=False) assert res.status_code == 400 assert "Invalid or expired" in res.text -async def test_register_valid_token_creates_user_and_redirects(client: AsyncClient) -> None: +async def test_register_get_does_not_consume_token(client: AsyncClient) -> None: + """GET is side-effect free: it shows a confirmation form and must not + consume the token or create a session.""" app = client._transport.app # type: ignore[union-attr] - magic_link_repo = app.state.magic_link_repo + magic_link_service = app.state.magic_link_service + + link = await magic_link_service.create(username="newuser") + res = await client.get(f"/register/{link.token}", follow_redirects=False) + assert res.status_code == 200 + assert f'action="/register/{link.token}"' in res.text + # Token still valid (not consumed by the GET). + assert await magic_link_service.validate(link.token) is not None + + +async def test_register_post_creates_user_and_redirects(client: AsyncClient) -> None: + app = client._transport.app # type: ignore[union-attr] + magic_link_service = app.state.magic_link_service user_repo = app.state.user_repo - await magic_link_repo.create( - MagicLink( - token="t1", - username="newuser", - expires_at=datetime.now(UTC) + timedelta(hours=1), - ) - ) + link = await magic_link_service.create(username="newuser") - res = await client.get("/register/t1", follow_redirects=False) + csrf = await get_csrf_token(client) + res = await client.post( + f"/register/{link.token}", + headers={"X-CSRF-Token": csrf}, + follow_redirects=False, + ) assert res.status_code in (302, 303) assert "/manage/credentials" in res.headers["location"] assert "setup=1" in res.headers["location"] - # Token should be marked used - link = await magic_link_repo.get_by_token("t1") - assert link is not None - assert link.used is True + # Token should be consumed (no longer valid) + assert await magic_link_service.validate(link.token) is None # User should exist user = await user_repo.get_by_username("newuser") @@ -56,53 +64,81 @@ async def test_register_valid_token_creates_user_and_redirects(client: AsyncClie assert "users" in user.groups +async def test_register_post_requires_csrf(client: AsyncClient) -> None: + app = client._transport.app # type: ignore[union-attr] + magic_link_service = app.state.magic_link_service + link = await magic_link_service.create(username="newuser") + + res = await client.post(f"/register/{link.token}", follow_redirects=False) + assert res.status_code == 403 + # Token must not have been consumed by the rejected request. + assert await magic_link_service.validate(link.token) is not None + + async def test_register_used_token_returns_error(client: AsyncClient) -> None: app = client._transport.app # type: ignore[union-attr] - repo = app.state.magic_link_repo - await repo.create( - MagicLink( - token="used", - username="newuser", - expires_at=datetime.now(UTC) + timedelta(hours=1), - used=True, - ) - ) + magic_link_service = app.state.magic_link_service + link = await magic_link_service.create(username="newuser") + await magic_link_service.mark_used(link.token) - res = await client.get("/register/used", follow_redirects=False) + res = await client.get(f"/register/{link.token}", follow_redirects=False) assert res.status_code == 400 async def test_register_existing_user_logs_in_and_redirects(client: AsyncClient) -> None: - """When initial-admin creates a user, the invite link should log them in.""" + """When initial-admin creates a user (no credentials yet), the invite link + should let them set up their account.""" app = client._transport.app # type: ignore[union-attr] - magic_link_repo = app.state.magic_link_repo + magic_link_service = app.state.magic_link_service user_repo = app.state.user_repo - # Pre-create the user (as initial-admin would) + # Pre-create the user (as initial-admin would), with no credentials. user = User(userid="lusab-bansen", username="admin", groups=["admin", "users"]) await user_repo.create(user) - # Create invite for the same username - await magic_link_repo.create( - MagicLink( - token="admin-setup", - username="admin", - expires_at=datetime.now(UTC) + timedelta(hours=1), - ) - ) + link = await magic_link_service.create(username="admin") - res = await client.get("/register/admin-setup", follow_redirects=False) + csrf = await get_csrf_token(client) + res = await client.post( + f"/register/{link.token}", + headers={"X-CSRF-Token": csrf}, + follow_redirects=False, + ) assert res.status_code in (302, 303) assert "/manage/credentials" in res.headers["location"] assert "setup=1" in res.headers["location"] - # Token should be marked used - link = await magic_link_repo.get_by_token("admin-setup") - assert link is not None - assert link.used is True + # Token should be consumed + assert await magic_link_service.validate(link.token) is None # Original user should still exist with original groups existing = await user_repo.get_by_username("admin") assert existing is not None assert existing.userid == "lusab-bansen" assert "admin" in existing.groups + + +async def test_register_rejects_user_that_already_has_credentials(client: AsyncClient) -> None: + """An invite/re-invite link must not act as a passwordless login for an + account that already has credentials. Recovery is a separate flow.""" + app = client._transport.app # type: ignore[union-attr] + magic_link_service = app.state.magic_link_service + user_repo = app.state.user_repo + cred_repo = app.state.credential_repo + + user = User(userid="lusab-hascreds", username="hascreds", groups=["users"]) + await user_repo.create(user) + svc = PasswordService(hasher=PasswordHasher(time_cost=1, memory_cost=8192)) + await cred_repo.create_password(PasswordCredential(user_id=user.userid, password_hash=svc.hash("existing-pass"))) + + link = await magic_link_service.create(username="hascreds") + csrf = await get_csrf_token(client) + res = await client.post( + f"/register/{link.token}", + headers={"X-CSRF-Token": csrf}, + follow_redirects=False, + ) + + # No passwordless login: rejected, not redirected to setup. + assert res.status_code == 400 + assert "setup=1" not in res.headers.get("location", "") diff --git a/tests/test_authn_active.py b/tests/test_authn_active.py index 3564bb5..6126343 100644 --- a/tests/test_authn_active.py +++ b/tests/test_authn_active.py @@ -61,6 +61,11 @@ async def test_inactive_user_cannot_register_magic_link(client: AsyncClient) -> link = await magic_link_service.create(username="deactivated", created_by="admin", note="test") - response = await client.get(f"/register/{link.token}", follow_redirects=False) + csrf = await get_csrf_token(client) + response = await client.post( + f"/register/{link.token}", + headers={"X-CSRF-Token": csrf}, + follow_redirects=False, + ) assert response.status_code == 400 or "deactivated" in response.text.lower() or "Invalid" in response.text diff --git a/tests/test_invite/test_service.py b/tests/test_invite/test_service.py index 6464fce..166e7cd 100644 --- a/tests/test_invite/test_service.py +++ b/tests/test_invite/test_service.py @@ -42,6 +42,18 @@ async def test_create_returns_magic_link(service: MagicLinkService) -> None: assert link.expires_at > datetime.now(UTC) +async def test_token_stored_hashed_not_plaintext(service: MagicLinkService, repo: SQLiteMagicLinkRepository) -> None: + # The raw token handed to the user must never be the value persisted in + # the store; a DB read must not yield a usable token. + link = await service.create(username="alice") + raw = link.token + assert await repo.get_by_token(raw) is None + # ...but the raw token must still validate through the service. + validated = await service.validate(raw) + assert validated is not None + assert validated.username == "alice" + + async def test_create_generates_unique_tokens(service: MagicLinkService) -> None: link1 = await service.create(username="alice") link2 = await service.create(username="bob") @@ -90,6 +102,22 @@ async def test_validate_expired_token(service: MagicLinkService, repo: SQLiteMag assert result is None +async def test_consume_validates_and_is_single_use(service: MagicLinkService) -> None: + link = await service.create(username="alice") + consumed = await service.consume(link.token) + assert consumed is not None + assert consumed.username == "alice" + assert consumed.token == link.token # raw token re-attached + # A second consume of the same token fails. + assert await service.consume(link.token) is None + + +async def test_consume_expired_returns_none(service: MagicLinkService, repo: SQLiteMagicLinkRepository) -> None: + expired_service = MagicLinkService(repo=repo, ttl=-1) + link = await expired_service.create(username="alice") + assert await service.consume(link.token) is None + + async def test_mark_used_returns_true(service: MagicLinkService) -> None: link = await service.create(username="alice") result = await service.mark_used(link.token) diff --git a/tests/test_oidc/test_authorization.py b/tests/test_oidc/test_authorization.py index 5994134..de3c70b 100644 --- a/tests/test_oidc/test_authorization.py +++ b/tests/test_oidc/test_authorization.py @@ -2,6 +2,16 @@ import secrets from httpx import AsyncClient +from porchlight.oidc.endpoints import _error_page + + +def test_error_page_escapes_html() -> None: + # OIDC error pages must not interpolate request-derived text as raw HTML. + resp = _error_page("") + body = resp.body.decode() + assert "