From e54764cda9de1d2c334f31adc755d9f8da423c23 Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Fri, 5 Jun 2026 13:27:00 +0200 Subject: [PATCH] fix(security): guard admin credential deletion against lockout Admin credential deletion removed password/WebAuthn credentials with no last-credential check, so an admin could delete a user's only credential and lock them out. Use the atomic delete_*_if_not_last repo methods; on refusal re-render the credentials section unchanged with an explanatory alert. Refs: porchlight-lg7 Co-Authored-By: Claude Opus 4.8 (1M context) --- src/porchlight/admin/routes.py | 11 +++++-- .../templates/admin/_credentials_section.html | 1 + tests/test_admin/test_admin_routes.py | 33 +++++++++++++++++-- 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/src/porchlight/admin/routes.py b/src/porchlight/admin/routes.py index 4bcc0a8..de20d8a 100644 --- a/src/porchlight/admin/routes.py +++ b/src/porchlight/admin/routes.py @@ -261,10 +261,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 +274,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 +292,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 +306,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", }, ) 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/tests/test_admin/test_admin_routes.py b/tests/test_admin/test_admin_routes.py index 603af60..b2138bf 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,25 @@ 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()