fix(users): admin can never strip their own admin permission
Aegis CI / lint-and-test (push) Has been cancelled
Snyk Security Scan / Python vulnerabilities (backend) (push) Has been cancelled
Snyk Security Scan / npm vulnerabilities (frontend) (push) Has been cancelled
Snyk Security Scan / Docker image vulnerabilities (backend) (push) Has been cancelled
Aegis CI / lint-and-test (push) Has been cancelled
Snyk Security Scan / Python vulnerabilities (backend) (push) Has been cancelled
Snyk Security Scan / npm vulnerabilities (frontend) (push) Has been cancelled
Snyk Security Scan / Docker image vulnerabilities (backend) (push) Has been cancelled
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.
This commit is contained in:
@@ -17,6 +17,7 @@ from app.dependencies.auth import require_role, require_any_role_strict
|
|||||||
|
|
||||||
# Import UnitOfWork from app.domain.unit_of_work
|
# Import UnitOfWork from app.domain.unit_of_work
|
||||||
from app.domain.unit_of_work import UnitOfWork
|
from app.domain.unit_of_work import UnitOfWork
|
||||||
|
from app.domain.errors import BusinessRuleViolation
|
||||||
|
|
||||||
# Import User from app.models.user
|
# Import User from app.models.user
|
||||||
from app.models.user import 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.**."""
|
"""Update one or more fields of an existing user. **Requires admin role.**."""
|
||||||
# Assign update_data = payload.model_dump(exclude_unset=True)
|
# Assign update_data = payload.model_dump(exclude_unset=True)
|
||||||
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
|
# Open context manager
|
||||||
with UnitOfWork(db) as uow:
|
with UnitOfWork(db) as uow:
|
||||||
# Assign user = update_user(db, user_id, **update_data)
|
# Assign user = update_user(db, user_id, **update_data)
|
||||||
|
|||||||
@@ -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):
|
def test_admin_can_grant_extra_roles(api, auth_headers):
|
||||||
created = api(
|
created = api(
|
||||||
"post", "/api/v1/users", auth_headers,
|
"post", "/api/v1/users", auth_headers,
|
||||||
|
|||||||
@@ -268,6 +268,7 @@ export default function UsersPage() {
|
|||||||
{editingUser && (
|
{editingUser && (
|
||||||
<EditUserModal
|
<EditUserModal
|
||||||
user={editingUser}
|
user={editingUser}
|
||||||
|
isSelfAdmin={editingUser.id === currentUser?.id && currentUser?.role === "admin"}
|
||||||
onClose={() => setEditingUser(null)}
|
onClose={() => setEditingUser(null)}
|
||||||
onSubmit={(payload) =>
|
onSubmit={(payload) =>
|
||||||
updateMutation.mutate({ userId: editingUser.id, payload })
|
updateMutation.mutate({ userId: editingUser.id, payload })
|
||||||
@@ -415,13 +416,16 @@ function CreateUserModal({ onClose, onSubmit, isSubmitting, error }: CreateUserM
|
|||||||
|
|
||||||
interface EditUserModalProps {
|
interface EditUserModalProps {
|
||||||
user: UserOut;
|
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;
|
onClose: () => void;
|
||||||
onSubmit: (payload: Parameters<typeof updateUser>[1]) => void;
|
onSubmit: (payload: Parameters<typeof updateUser>[1]) => void;
|
||||||
isSubmitting: boolean;
|
isSubmitting: boolean;
|
||||||
error: Error | null;
|
error: Error | null;
|
||||||
}
|
}
|
||||||
|
|
||||||
function EditUserModal({ user, onClose, onSubmit, isSubmitting, error }: EditUserModalProps) {
|
function EditUserModal({ user, isSelfAdmin, onClose, onSubmit, isSubmitting, error }: EditUserModalProps) {
|
||||||
const [formData, setFormData] = useState({
|
const [formData, setFormData] = useState({
|
||||||
full_name: user.full_name || "",
|
full_name: user.full_name || "",
|
||||||
email: user.email || "",
|
email: user.email || "",
|
||||||
@@ -489,8 +493,9 @@ function EditUserModal({ user, onClose, onSubmit, isSubmitting, error }: EditUse
|
|||||||
<label className="block text-sm font-medium text-gray-300">Role</label>
|
<label className="block text-sm font-medium text-gray-300">Role</label>
|
||||||
<select
|
<select
|
||||||
value={formData.role}
|
value={formData.role}
|
||||||
|
disabled={isSelfAdmin}
|
||||||
onChange={(e) => setFormData({ ...formData, role: e.target.value })}
|
onChange={(e) => setFormData({ ...formData, role: e.target.value })}
|
||||||
className="mt-1 w-full rounded-lg border border-gray-700 bg-gray-800 px-3 py-2 text-gray-200"
|
className="mt-1 w-full rounded-lg border border-gray-700 bg-gray-800 px-3 py-2 text-gray-200 disabled:cursor-not-allowed disabled:opacity-50"
|
||||||
>
|
>
|
||||||
{ROLES.map((role) => (
|
{ROLES.map((role) => (
|
||||||
<option key={role.value} value={role.value}>
|
<option key={role.value} value={role.value}>
|
||||||
@@ -498,6 +503,11 @@ function EditUserModal({ user, onClose, onSubmit, isSubmitting, error }: EditUse
|
|||||||
</option>
|
</option>
|
||||||
))}
|
))}
|
||||||
</select>
|
</select>
|
||||||
|
{isSelfAdmin && (
|
||||||
|
<p className="mt-1 text-xs text-gray-500">
|
||||||
|
You can't change your own role away from admin.
|
||||||
|
</p>
|
||||||
|
)}
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
<div>
|
<div>
|
||||||
@@ -505,7 +515,9 @@ function EditUserModal({ user, onClose, onSubmit, isSubmitting, error }: EditUse
|
|||||||
Additional Roles <span className="text-gray-500">(switchable via the top-bar role switcher)</span>
|
Additional Roles <span className="text-gray-500">(switchable via the top-bar role switcher)</span>
|
||||||
</label>
|
</label>
|
||||||
<div className="mt-1.5 grid grid-cols-2 gap-1.5">
|
<div className="mt-1.5 grid grid-cols-2 gap-1.5">
|
||||||
{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 (
|
||||||
<label
|
<label
|
||||||
key={role.value}
|
key={role.value}
|
||||||
className="flex items-center gap-2 rounded-lg border border-gray-700 bg-gray-800 px-2.5 py-1.5 text-sm text-gray-300"
|
className="flex items-center gap-2 rounded-lg border border-gray-700 bg-gray-800 px-2.5 py-1.5 text-sm text-gray-300"
|
||||||
@@ -513,12 +525,14 @@ function EditUserModal({ user, onClose, onSubmit, isSubmitting, error }: EditUse
|
|||||||
<input
|
<input
|
||||||
type="checkbox"
|
type="checkbox"
|
||||||
checked={formData.extra_roles.includes(role.value)}
|
checked={formData.extra_roles.includes(role.value)}
|
||||||
|
disabled={lockAdmin}
|
||||||
onChange={() => toggleExtraRole(role.value)}
|
onChange={() => toggleExtraRole(role.value)}
|
||||||
className="h-3.5 w-3.5 rounded border-gray-600 bg-gray-900 text-cyan-500 focus:ring-cyan-500"
|
className="h-3.5 w-3.5 rounded border-gray-600 bg-gray-900 text-cyan-500 focus:ring-cyan-500 disabled:cursor-not-allowed disabled:opacity-50"
|
||||||
/>
|
/>
|
||||||
{role.label}
|
{role.label}
|
||||||
</label>
|
</label>
|
||||||
))}
|
);
|
||||||
|
})}
|
||||||
</div>
|
</div>
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user