From 3cd0f6fd999096eddc3c30b68a4c84ddefaa5201 Mon Sep 17 00:00:00 2001 From: kitos Date: Wed, 22 Jul 2026 10:52:46 +0200 Subject: [PATCH] fix(users): admin can never strip their own admin permission Both backend (PATCH /users/{id}) and frontend (Edit User modal) now block an admin from changing their own primary role away from admin or removing admin from their own extra_roles, closing off a self-lockout scenario. --- backend/app/routers/users.py | 11 ++++++ backend/tests/test_multi_role_switch.py | 36 +++++++++++++++++++ frontend/src/pages/UsersPage.tsx | 46 ++++++++++++++++--------- 3 files changed, 77 insertions(+), 16 deletions(-) diff --git a/backend/app/routers/users.py b/backend/app/routers/users.py index fffe67a..78bbec4 100644 --- a/backend/app/routers/users.py +++ b/backend/app/routers/users.py @@ -17,6 +17,7 @@ from app.dependencies.auth import require_role, require_any_role_strict # Import UnitOfWork from app.domain.unit_of_work from app.domain.unit_of_work import UnitOfWork +from app.domain.errors import BusinessRuleViolation # Import User from app.models.user from app.models.user import User @@ -269,6 +270,16 @@ def update_user_route( """Update one or more fields of an existing user. **Requires admin role.**.""" # Assign update_data = payload.model_dump(exclude_unset=True) update_data = payload.model_dump(exclude_unset=True) + + # An admin can never strip their own admin permission — otherwise a + # careless self-edit (or a compromised session) could lock every admin + # out of the platform with no way back in. + if user_id == current_user.id and current_user.role == "admin": + final_role = update_data.get("role", current_user.role) + final_extra_roles = update_data.get("extra_roles", current_user.extra_roles or []) + if "admin" not in {final_role, *(final_extra_roles or [])}: + raise BusinessRuleViolation("You cannot remove your own admin permission.") + # Open context manager with UnitOfWork(db) as uow: # Assign user = update_user(db, user_id, **update_data) diff --git a/backend/tests/test_multi_role_switch.py b/backend/tests/test_multi_role_switch.py index b73ac23..f304630 100644 --- a/backend/tests/test_multi_role_switch.py +++ b/backend/tests/test_multi_role_switch.py @@ -4,6 +4,42 @@ instead of ever mixing permissions from more than one. """ +def test_admin_cannot_remove_own_admin_role(api, auth_headers, admin_user): + resp = api( + "patch", f"/api/v1/users/{admin_user.id}", auth_headers, + json={"role": "viewer"}, + ) + assert resp.status_code == 400 + assert "admin" in resp.text.lower() + + +def test_admin_cannot_drop_admin_via_extra_roles_either(api, auth_headers, admin_user): + # Give themselves a second role first, then try to swap the primary + # role away from admin without admin anywhere in extra_roles. + resp = api( + "patch", f"/api/v1/users/{admin_user.id}", auth_headers, + json={"role": "red_lead", "extra_roles": ["viewer"]}, + ) + assert resp.status_code == 400 + + +def test_admin_can_switch_primary_role_if_admin_kept_in_extra_roles(api, auth_headers, admin_user): + resp = api( + "patch", f"/api/v1/users/{admin_user.id}", auth_headers, + json={"role": "red_lead", "extra_roles": ["admin"]}, + ) + assert resp.status_code == 200, resp.text + + +def test_admin_can_edit_own_other_fields_without_touching_role(api, auth_headers, admin_user): + resp = api( + "patch", f"/api/v1/users/{admin_user.id}", auth_headers, + json={"full_name": "Renamed Admin"}, + ) + assert resp.status_code == 200, resp.text + assert resp.json()["role"] == "admin" + + def test_admin_can_grant_extra_roles(api, auth_headers): created = api( "post", "/api/v1/users", auth_headers, diff --git a/frontend/src/pages/UsersPage.tsx b/frontend/src/pages/UsersPage.tsx index 3aef9a3..ad64f1d 100644 --- a/frontend/src/pages/UsersPage.tsx +++ b/frontend/src/pages/UsersPage.tsx @@ -268,6 +268,7 @@ export default function UsersPage() { {editingUser && ( setEditingUser(null)} onSubmit={(payload) => updateMutation.mutate({ userId: editingUser.id, payload }) @@ -415,13 +416,16 @@ function CreateUserModal({ onClose, onSubmit, isSubmitting, error }: CreateUserM interface EditUserModalProps { user: UserOut; + /** True when the account being edited is the logged-in admin's own — + * an admin must never be able to strip their own admin permission. */ + isSelfAdmin: boolean; onClose: () => void; onSubmit: (payload: Parameters[1]) => void; isSubmitting: boolean; error: Error | null; } -function EditUserModal({ user, onClose, onSubmit, isSubmitting, error }: EditUserModalProps) { +function EditUserModal({ user, isSelfAdmin, onClose, onSubmit, isSubmitting, error }: EditUserModalProps) { const [formData, setFormData] = useState({ full_name: user.full_name || "", email: user.email || "", @@ -489,8 +493,9 @@ function EditUserModal({ user, onClose, onSubmit, isSubmitting, error }: EditUse + {isSelfAdmin && ( +

+ You can't change your own role away from admin. +

+ )}
@@ -505,20 +515,24 @@ function EditUserModal({ user, onClose, onSubmit, isSubmitting, error }: EditUse Additional Roles (switchable via the top-bar role switcher)
- {ROLES.filter((r) => r.value !== formData.role).map((role) => ( - - ))} + {ROLES.filter((r) => r.value !== formData.role).map((role) => { + const lockAdmin = isSelfAdmin && role.value === "admin" && formData.extra_roles.includes("admin"); + return ( + + ); + })}