fix(security): prevent removing the last active admin
Admins could remove the admin group from, deactivate, or delete the last active admin, locking the system out of all administration. Add a count_active_admins() repo method and a _is_last_active_admin() guard, and block all three operations when they would leave zero active admins. Refs: porchlight-yq7 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
e54764cda9
commit
aedb451128
5 changed files with 110 additions and 0 deletions
|
|
@ -463,3 +463,70 @@ async def test_admin_cannot_delete_users_last_credential(client: AsyncClient) ->
|
|||
# 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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue