From fce0fb4c2674204b38622adec0d2515db3ebdc69 Mon Sep 17 00:00:00 2001 From: Tycho Hob Date: Tue, 20 Oct 2026 13:47:20 -0400 Subject: [PATCH] fix: Users can't remove their own admin roles --- CHANGELOG.rst | 7 ++ src/openedx_authz/rest_api/data.py | 1 + src/openedx_authz/rest_api/v1/views.py | 12 ++++ .../tests/rest_api/test_views.py | 67 +++++++++++++++++++ 4 files changed, 87 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index cc5c1324..c8855b56 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -64,6 +64,13 @@ Notes assignments. ``--force`` removes the role together with its assignments and writes a ``RoleAssignmentAudit`` record for each one. +Fixed +===== + +* ``DELETE /api/authz/v1/roles/users/`` no longer lets a user remove its own ``library_admin`` or + ``course_admin`` role, matching the admin console. The user is reported in ``errors`` with + ``cannot_remove_own_admin_role`` and keeps the role; other users in the request are processed. + 1.24.0 - 2026-09-14 ******************* diff --git a/src/openedx_authz/rest_api/data.py b/src/openedx_authz/rest_api/data.py index f68ea542..fbb21243 100644 --- a/src/openedx_authz/rest_api/data.py +++ b/src/openedx_authz/rest_api/data.py @@ -70,6 +70,7 @@ class RoleOperationError(BaseEnum): USER_DOES_NOT_HAVE_ROLE = "user_does_not_have_role" ROLE_ASSIGNMENT_ERROR = "role_assignment_error" ROLE_REMOVAL_ERROR = "role_removal_error" + CANNOT_REMOVE_OWN_ADMIN_ROLE = "cannot_remove_own_admin_role" class ScopesQuerySetFields(BaseEnum): diff --git a/src/openedx_authz/rest_api/v1/views.py b/src/openedx_authz/rest_api/v1/views.py index e571166c..707b4254 100644 --- a/src/openedx_authz/rest_api/v1/views.py +++ b/src/openedx_authz/rest_api/v1/views.py @@ -20,6 +20,7 @@ from openedx_authz import api from openedx_authz.api.utils import get_user_map from openedx_authz.constants import permissions +from openedx_authz.constants.roles import COURSE_ADMIN, LIBRARY_ADMIN from openedx_authz.filters import PermissionValidationRequested, RoleAssignmentRequested, RoleRemovalRequested from openedx_authz.rest_api.data import RoleOperationError, RoleOperationStatus from openedx_authz.rest_api.decorators import authz_permissions, view_auth_classes @@ -48,6 +49,10 @@ User = get_user_model() +# Admin roles a user may not remove from itself, so a scope is not left without an +# admin by accident. Another user who can manage the team can still revoke them. +SELF_PROTECTED_ROLES = (LIBRARY_ADMIN.external_key, COURSE_ADMIN.external_key) + @view_auth_classes() class PermissionValidationMeView(APIView): @@ -242,6 +247,9 @@ class RoleUserAPIView(APIView): "errors": [{"user_identifier": "jane_doe", "error": "user_does_not_have_role"}] } + A user cannot remove its own ``library_admin`` or ``course_admin`` role; that user is + reported in ``errors`` with ``cannot_remove_own_admin_role`` and keeps the role. + **Authentication and Permissions** - Requires authenticated user. @@ -375,6 +383,10 @@ def delete(self, request: HttpRequest) -> Response: response_dict = {"user_identifier": user_identifier} try: user = get_user_by_username_or_email(user_identifier) + if user.pk == request.user.pk and data["role"] in SELF_PROTECTED_ROLES: + response_dict["error"] = RoleOperationError.CANNOT_REMOVE_OWN_ADMIN_ROLE + errors.append(response_dict) + continue result = api.unassign_role_from_user(user.username, data["role"], data["scope"]) if result: response_dict["status"] = RoleOperationStatus.ROLE_REMOVED diff --git a/src/openedx_authz/tests/rest_api/test_views.py b/src/openedx_authz/tests/rest_api/test_views.py index 4342f828..395a7475 100644 --- a/src/openedx_authz/tests/rest_api/test_views.py +++ b/src/openedx_authz/tests/rest_api/test_views.py @@ -820,6 +820,73 @@ def test_remove_users_from_role_success(self, users: list[str], expected_complet self.assertEqual(len(response.data["completed"]), expected_completed) self.assertEqual(len(response.data["errors"]), expected_errors) + @data( + # library_admin removing its own library_admin: refused + ("admin_3", roles.LIBRARY_ADMIN.external_key, "lib:Org3:LIB3", "admin_3", "api.ContentLibraryData"), + # same, naming itself by email + ("admin_3", roles.LIBRARY_ADMIN.external_key, "lib:Org3:LIB3", "admin_3@example.com", "api.ContentLibraryData"), + # course_admin removing its own course_admin: refused + ("course_admin", roles.COURSE_ADMIN.external_key, COURSE_SCOPE_ORG1, "course_admin", "api.CourseOverviewData"), + ) + @unpack + def test_remove_own_admin_role_is_refused( + self, username: str, role: str, scope: str, identifier: str, scope_data: str + ): + """Test that a user cannot remove its own admin role, while other users in the request are removed. + + Expected result: + - Returns 207 MULTI-STATUS status + - The requester is reported with ``cannot_remove_own_admin_role`` and keeps the role + - Another admin in the same request is removed + """ + other = "regular_5" if role == roles.LIBRARY_ADMIN.external_key else "regular_2" + if role == roles.COURSE_ADMIN.external_key: + self._assign_roles_to_users( + assignments=[{"subject_name": other, "role_name": role, "scope_name": scope}] + ) + user = User.objects.get(username=username) + self.client.force_authenticate(user=user) + query_params = {"role": role, "scope": scope, "users": f"{identifier},{other}"} + + with patch.object(getattr(api, scope_data.split(".")[1]), "exists", return_value=True): + response = self.client.delete(f"{self.url}?{urlencode(query_params)}") + + self.assertEqual(response.status_code, status.HTTP_207_MULTI_STATUS) + self.assertEqual( + response.data["errors"], + [{"user_identifier": identifier, "error": RoleOperationError.CANNOT_REMOVE_OWN_ADMIN_ROLE}], + ) + self.assertEqual([c["user_identifier"] for c in response.data["completed"]], [other]) + self.assertIn( + role, + [r.external_key for a in api.get_user_role_assignments_in_scope(username, scope) for r in a.roles], + ) + + def test_remove_own_non_admin_role_is_allowed(self): + """Test that a user may remove a role of its own that is not an admin role. + + Expected result: + - Returns 207 MULTI-STATUS status with the requester in ``completed`` + """ + self.client.force_authenticate(user=User.objects.get(username="admin_3")) + self._assign_roles_to_users( + assignments=[ + { + "subject_name": "admin_3", + "role_name": roles.LIBRARY_USER.external_key, + "scope_name": "lib:Org3:LIB3", + } + ] + ) + query_params = {"role": roles.LIBRARY_USER.external_key, "scope": "lib:Org3:LIB3", "users": "admin_3"} + + with patch.object(api.ContentLibraryData, "exists", return_value=True): + response = self.client.delete(f"{self.url}?{urlencode(query_params)}") + + self.assertEqual(response.status_code, status.HTTP_207_MULTI_STATUS) + self.assertEqual(response.data["errors"], []) + self.assertEqual([c["user_identifier"] for c in response.data["completed"]], ["admin_3"]) + @patch.object(api, "unassign_role_from_user") def test_remove_users_from_role_exception_handling(self, mock_unassign_role_from_user): """Test removing users from a role with exception handling."""