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) <noreply@anthropic.com>
This commit is contained in:
parent
1bb76899a5
commit
e54764cda9
3 changed files with 40 additions and 5 deletions
|
|
@ -261,10 +261,12 @@ async def delete_user_password(request: Request, userid: str) -> Response:
|
||||||
return HTMLResponse("Forbidden", status_code=403)
|
return HTMLResponse("Forbidden", status_code=403)
|
||||||
|
|
||||||
cred_repo = request.app.state.credential_repo
|
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
|
# Re-render credentials section
|
||||||
webauthn_credentials = await cred_repo.get_webauthn_by_user(userid)
|
webauthn_credentials = await cred_repo.get_webauthn_by_user(userid)
|
||||||
|
password = await cred_repo.get_password_by_user(userid)
|
||||||
templates = request.app.state.templates
|
templates = request.app.state.templates
|
||||||
return templates.TemplateResponse(
|
return templates.TemplateResponse(
|
||||||
request,
|
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),
|
"target_user": await request.app.state.user_repo.get_by_userid(userid),
|
||||||
"webauthn_credentials": webauthn_credentials,
|
"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)
|
padded = credential_id_b64 + "=" * (-len(credential_id_b64) % 4)
|
||||||
credential_id = urlsafe_b64decode(padded)
|
credential_id = urlsafe_b64decode(padded)
|
||||||
cred_repo = request.app.state.credential_repo
|
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
|
# Re-render credentials section
|
||||||
webauthn_credentials = await cred_repo.get_webauthn_by_user(userid)
|
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),
|
"target_user": await request.app.state.user_repo.get_by_userid(userid),
|
||||||
"webauthn_credentials": webauthn_credentials,
|
"webauthn_credentials": webauthn_credentials,
|
||||||
"has_password": password is not None,
|
"has_password": password is not None,
|
||||||
|
"error": None if deleted else "Cannot remove the user's last credential",
|
||||||
},
|
},
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1,3 +1,4 @@
|
||||||
|
{% if error %}<div role="alert">{{ error }}</div>{% endif %}
|
||||||
<h3>Password</h3>
|
<h3>Password</h3>
|
||||||
{% if has_password %}
|
{% if has_password %}
|
||||||
<p>Password is set.
|
<p>Password is set.
|
||||||
|
|
|
||||||
|
|
@ -328,11 +328,15 @@ async def test_delete_password_credential(client: AsyncClient) -> None:
|
||||||
await _login(client)
|
await _login(client)
|
||||||
target = await _create_target_user(client, username="bob")
|
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]
|
app = client._transport.app # type: ignore[union-attr]
|
||||||
cred_repo = app.state.credential_repo
|
cred_repo = app.state.credential_repo
|
||||||
svc = PasswordService(hasher=PasswordHasher(time_cost=1, memory_cost=8192))
|
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_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)
|
token = await get_csrf_token(client)
|
||||||
response = await client.request(
|
response = await client.request(
|
||||||
|
|
@ -351,7 +355,8 @@ async def test_delete_webauthn_credential(client: AsyncClient) -> None:
|
||||||
await _login(client)
|
await _login(client)
|
||||||
target = await _create_target_user(client, username="bob")
|
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]
|
app = client._transport.app # type: ignore[union-attr]
|
||||||
cred_repo = app.state.credential_repo
|
cred_repo = app.state.credential_repo
|
||||||
credential_id = b"\x01\x02\x03\x04\x05\x06\x07\x08"
|
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",
|
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
|
# URL uses base64url without padding
|
||||||
credential_id_b64 = urlsafe_b64encode(credential_id).decode().rstrip("=")
|
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})
|
response = await client.post("/admin/users/nonexistent-id/invite", headers={"X-CSRF-Token": token})
|
||||||
assert response.status_code == 404
|
assert response.status_code == 404
|
||||||
assert "not found" in response.text.lower()
|
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()
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue