secure(l4d2web): block non-admin writes on system overlays; last-admin guard on deactivate
_load_files_overlay docs already promised "owner or admin" for mutations, but the check only filtered by overlay.type — system overlays (user_id IS NULL) were writable by any logged-in user. Add the explicit 403 for non-admins; read-only routes remain open across all overlay types. Mirror the delete-route last-admin guard on /admin/users/<id>/deactivate so a future auth-model change (service accounts bypassing require_admin, etc.) can't accidentally lock out the system.
This commit is contained in:
parent
74b7f61437
commit
0e2a78e065
4 changed files with 97 additions and 2 deletions
|
|
@ -88,13 +88,16 @@ def _load_overlay_for_user(overlay_id: int, user) -> Overlay | Response:
|
|||
|
||||
def _load_files_overlay(overlay_id: int, user) -> Overlay | Response:
|
||||
"""Like `_load_overlay_for_user`, but additionally 404s for any overlay
|
||||
whose type isn't `files`. Mutating endpoints use this; read-only ones
|
||||
keep working across all types."""
|
||||
whose type isn't `files`, and 403s when a non-admin tries to mutate a
|
||||
system overlay (`user_id IS NULL`). Read-only endpoints keep working
|
||||
across all overlay types for everyone."""
|
||||
result = _load_overlay_for_user(overlay_id, user)
|
||||
if isinstance(result, Response):
|
||||
return result
|
||||
if result.type != "files":
|
||||
return Response(status=404)
|
||||
if result.user_id is None and not user.admin:
|
||||
return Response(status=403)
|
||||
return result
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -69,6 +69,19 @@ def admin_users_deactivate(user_id: int) -> Response:
|
|||
target = db.scalar(select(User).where(User.id == user_id))
|
||||
if target is None:
|
||||
return Response(status=404)
|
||||
# Defense-in-depth: mirror the last-admin guard from delete. Through
|
||||
# normal flow this is unreachable (self-deactivate refused above), but
|
||||
# it survives future auth-model changes like service accounts.
|
||||
if target.admin and target.active:
|
||||
other_active_admins = db.scalar(
|
||||
select(func.count(User.id)).where(
|
||||
User.admin.is_(True),
|
||||
User.active.is_(True),
|
||||
User.id != user_id,
|
||||
)
|
||||
) or 0
|
||||
if other_active_admins == 0:
|
||||
return Response("cannot deactivate the last active admin", status=409)
|
||||
target.active = False
|
||||
return redirect("/admin/users")
|
||||
|
||||
|
|
|
|||
|
|
@ -78,6 +78,21 @@ def test_deactivate_flips_active_false(admin_client):
|
|||
assert _user_active(target) is False
|
||||
|
||||
|
||||
def test_deactivate_other_admin_succeeds_when_more_than_one_admin(admin_client):
|
||||
"""Defense-in-depth mirror of the delete-admin test. The fixture creates
|
||||
two admins; admin can deactivate admin2 because at least one admin (the
|
||||
actor) remains."""
|
||||
client, _ = admin_client
|
||||
with session_scope() as db:
|
||||
admin2 = db.scalar(select(User).where(User.username == "admin2"))
|
||||
admin2_id = admin2.id
|
||||
|
||||
response = _post(client, f"/admin/users/{admin2_id}/deactivate")
|
||||
|
||||
assert response.status_code == 302
|
||||
assert _user_active(admin2_id) is False
|
||||
|
||||
|
||||
def test_activate_flips_active_true(admin_client):
|
||||
client, _ = admin_client
|
||||
target = _add_user("bob", active=False)
|
||||
|
|
|
|||
|
|
@ -64,6 +64,70 @@ def _make_overlay(left4me_root: Path, *, user_id: int | None, name: str) -> int:
|
|||
return overlay_id
|
||||
|
||||
|
||||
def _make_files_overlay(left4me_root: Path, *, user_id: int | None, name: str) -> int:
|
||||
with session_scope() as s:
|
||||
overlay = Overlay(name=name, path="", type="files", user_id=user_id)
|
||||
s.add(overlay)
|
||||
s.flush()
|
||||
overlay.path = str(overlay.id)
|
||||
overlay_id = overlay.id
|
||||
(left4me_root / "overlays" / str(overlay_id)).mkdir(parents=True)
|
||||
return overlay_id
|
||||
|
||||
|
||||
def test_non_admin_cannot_mkdir_in_system_overlay(app, left4me_root: Path) -> None:
|
||||
"""System overlays (user_id IS NULL) are readable by everyone but only
|
||||
writable by admins. A regular user attempting to mutate gets 403."""
|
||||
user_id = _make_user(username="bob", admin=False)
|
||||
overlay_id = _make_files_overlay(left4me_root, user_id=None, name="system")
|
||||
|
||||
client = _client_for(app, user_id)
|
||||
response = client.post(
|
||||
f"/overlays/{overlay_id}/files/mkdir",
|
||||
json={"path": "newdir"},
|
||||
headers={"X-CSRF-Token": "test-token"},
|
||||
)
|
||||
assert response.status_code == 403
|
||||
|
||||
|
||||
def test_non_admin_can_still_read_system_overlay(app, left4me_root: Path) -> None:
|
||||
user_id = _make_user(username="bob", admin=False)
|
||||
overlay_id = _make_files_overlay(left4me_root, user_id=None, name="system")
|
||||
(left4me_root / "overlays" / str(overlay_id) / "config.cfg").write_text("ok")
|
||||
|
||||
client = _client_for(app, user_id)
|
||||
response = client.get(f"/overlays/{overlay_id}/files")
|
||||
assert response.status_code == 200
|
||||
assert b"config.cfg" in response.data
|
||||
|
||||
|
||||
def test_admin_can_mkdir_in_system_overlay(app, left4me_root: Path) -> None:
|
||||
admin_id = _make_user(username="admin", admin=True)
|
||||
overlay_id = _make_files_overlay(left4me_root, user_id=None, name="system")
|
||||
|
||||
client = _client_for(app, admin_id)
|
||||
response = client.post(
|
||||
f"/overlays/{overlay_id}/files/mkdir",
|
||||
json={"path": "newdir"},
|
||||
headers={"X-CSRF-Token": "test-token"},
|
||||
)
|
||||
assert response.status_code in (200, 201, 204)
|
||||
assert (left4me_root / "overlays" / str(overlay_id) / "newdir").is_dir()
|
||||
|
||||
|
||||
def test_owner_can_mkdir_in_own_overlay(app, left4me_root: Path) -> None:
|
||||
user_id = _make_user(username="bob", admin=False)
|
||||
overlay_id = _make_files_overlay(left4me_root, user_id=user_id, name="mine")
|
||||
|
||||
client = _client_for(app, user_id)
|
||||
response = client.post(
|
||||
f"/overlays/{overlay_id}/files/mkdir",
|
||||
json={"path": "newdir"},
|
||||
headers={"X-CSRF-Token": "test-token"},
|
||||
)
|
||||
assert response.status_code in (200, 201, 204)
|
||||
|
||||
|
||||
def test_files_fragment_lists_root_directory(app, left4me_root: Path) -> None:
|
||||
user_id = _make_user()
|
||||
overlay_id = _make_overlay(left4me_root, user_id=user_id, name="my")
|
||||
|
|
|
|||
Loading…
Reference in a new issue