From 2200e65519929021c0806dc8105c65041f68db8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Jes=C3=BAs=20Pe=C3=B1a=20Rodr=C3=ADguez?= Date: Mon, 22 Sep 2025 14:04:39 +0200 Subject: [PATCH] feat(auth): add safeguards to prevent self-role removal and enforce MANAGE_ACCOUNT role presence (#8729) --- api/CHANGELOG.md | 1 + api/src/backend/api/specs/v1.yaml | 16 ++- api/src/backend/api/tests/test_views.py | 158 +++++++++++++++++++++++- api/src/backend/api/v1/serializers.py | 59 +++++++++ api/src/backend/api/v1/views.py | 87 +++++++++++-- 5 files changed, 303 insertions(+), 18 deletions(-) diff --git a/api/CHANGELOG.md b/api/CHANGELOG.md index 0813151f81..659137c6b2 100644 --- a/api/CHANGELOG.md +++ b/api/CHANGELOG.md @@ -9,6 +9,7 @@ All notable changes to the **Prowler API** are documented in this file. ### Changed - Now the MANAGE_ACCOUNT permission is required to modify or read user permissions instead of MANAGE_USERS [(#8281)](https://github.com/prowler-cloud/prowler/pull/8281) +- Now at least one user with MANAGE_ACCOUNT permission is required in the tenant [(#8729)](https://github.com/prowler-cloud/prowler/pull/8729) --- diff --git a/api/src/backend/api/specs/v1.yaml b/api/src/backend/api/specs/v1.yaml index a2b36ac3dd..235fd3569f 100644 --- a/api/src/backend/api/specs/v1.yaml +++ b/api/src/backend/api/specs/v1.yaml @@ -6363,8 +6363,10 @@ paths: description: '' patch: operationId: roles_partial_update - description: Update certain fields of an existing role's information without - affecting other fields. + description: Update selected fields on an existing role. When changing the `users` + relationship of a role that grants MANAGE_ACCOUNT, the API blocks attempts + that would leave the tenant without any MANAGE_ACCOUNT assignees and prevents + callers from removing their own assignment to that role. summary: Partially update a role parameters: - in: path @@ -6399,7 +6401,8 @@ paths: description: '' delete: operationId: roles_destroy - description: Remove a role from the system by their ID. + description: Delete the specified role. The API rejects deletion of the last + role in the tenant that grants MANAGE_ACCOUNT. summary: Delete a role parameters: - in: path @@ -8439,7 +8442,8 @@ paths: patch: operationId: users_relationships_roles_partial_update description: Update the user-roles relationship information without affecting - other fields. + other fields. If the update would remove MANAGE_ACCOUNT from the last remaining + user in the tenant, the API rejects the request with a 400 response. summary: Partially update a user-roles relationship tags: - User @@ -8463,6 +8467,10 @@ paths: delete: operationId: users_relationships_roles_destroy description: Remove the user-roles relationship from the system by their ID. + If removing MANAGE_ACCOUNT would take it away from the last remaining user + in the tenant, the API rejects the request with a 400 response. Users also + cannot delete their own role assignments; attempting to do so returns a 400 + response. summary: Delete a user-roles relationship tags: - User diff --git a/api/src/backend/api/tests/test_views.py b/api/src/backend/api/tests/test_views.py index 1aaf6224a8..71fe3d98e6 100644 --- a/api/src/backend/api/tests/test_views.py +++ b/api/src/backend/api/tests/test_views.py @@ -51,6 +51,7 @@ from api.models import ( UserRoleRelationship, ) from api.rls import Tenant +from api.v1.serializers import TokenSerializer from api.v1.views import ComplianceOverviewViewSet, TenantFinishACSView @@ -4720,6 +4721,36 @@ class TestRoleViewSet: assert role.users.count() == 0 assert role.provider_groups.count() == 0 + def test_cannot_remove_own_assignment_via_role_update( + self, authenticated_client, roles_fixture + ): + role = roles_fixture[0] + # Ensure the authenticated user is assigned to this role + user = User.objects.get(email=TEST_USER) + if not UserRoleRelationship.objects.filter(user=user, role=role).exists(): + UserRoleRelationship.objects.create( + user=user, role=role, tenant_id=role.tenant_id + ) + + # Attempt to update role users to exclude the current user + data = { + "data": { + "id": str(role.id), + "type": "roles", + "relationships": {"users": {"data": []}}, + } + } + response = authenticated_client.patch( + reverse("role-detail", kwargs={"pk": role.id}), + data=json.dumps(data), + content_type="application/vnd.api+json", + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert ( + "cannot remove their own role" + in response.json()["errors"][0]["detail"].lower() + ) + def test_role_create_with_invalid_user_relationship( self, authenticated_client, provider_groups_fixture ): @@ -4841,15 +4872,134 @@ class TestUserRoleRelationshipViewSet: roles_fixture[2].id, } - def test_destroy_relationship( - self, authenticated_client, roles_fixture, create_test_user + def test_destroy_relationship_other_user( + self, authenticated_client, roles_fixture, create_test_user, tenants_fixture ): + # Create another user in same tenant and assign a role + tenant = tenants_fixture[0] + other_user = User.objects.create_user( + name="other", + email="other_user@prowler.com", + password="TmpPass123@", + ) + Membership.objects.create(user=other_user, tenant=tenant) + UserRoleRelationship.objects.create( + user=other_user, role=roles_fixture[0], tenant_id=tenant.id + ) + + # Delete roles for the other user (allowed) + response = authenticated_client.delete( + reverse("user-roles-relationship", kwargs={"pk": other_user.id}), + ) + assert response.status_code == status.HTTP_204_NO_CONTENT + relationships = UserRoleRelationship.objects.filter(user=other_user.id) + assert relationships.count() == 0 + + def test_cannot_delete_own_roles(self, authenticated_client, create_test_user): + # Attempt to delete own roles should be forbidden response = authenticated_client.delete( reverse("user-roles-relationship", kwargs={"pk": create_test_user.id}), ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_prevent_removing_last_manage_account_on_patch( + self, authenticated_client, roles_fixture, create_test_user, tenants_fixture + ): + # roles_fixture[1] has manage_account=False + limited_role = roles_fixture[1] + + # Ensure there is no other user with MANAGE_ACCOUNT in the tenant + tenant = tenants_fixture[0] + # Create a secondary user without MANAGE_ACCOUNT + user2 = User.objects.create_user( + name="limited_user", + email="limited_user@prowler.com", + password="TmpPass123@", + ) + Membership.objects.create(user=user2, tenant=tenant) + UserRoleRelationship.objects.create( + user=user2, role=limited_role, tenant_id=tenant.id + ) + + # Attempt to switch the only MANAGE_ACCOUNT user to a role without it + data = {"data": [{"type": "roles", "id": str(limited_role.id)}]} + response = authenticated_client.patch( + reverse("user-roles-relationship", kwargs={"pk": create_test_user.id}), + data=data, + content_type="application/vnd.api+json", + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "MANAGE_ACCOUNT" in response.json()["errors"][0]["detail"] + + def test_allow_role_change_when_other_user_has_manage_account_on_patch( + self, authenticated_client, roles_fixture, create_test_user, tenants_fixture + ): + # roles_fixture[1] has manage_account=False, roles_fixture[0] has manage_account=True + limited_role = roles_fixture[1] + ma_role = roles_fixture[0] + + tenant = tenants_fixture[0] + # Create another user with MANAGE_ACCOUNT + user2 = User.objects.create_user( + name="ma_user", + email="ma_user@prowler.com", + password="TmpPass123@", + ) + Membership.objects.create(user=user2, tenant=tenant) + UserRoleRelationship.objects.create( + user=user2, role=ma_role, tenant_id=tenant.id + ) + + # Now changing the first user's roles to a non-MA role should succeed + data = {"data": [{"type": "roles", "id": str(limited_role.id)}]} + response = authenticated_client.patch( + reverse("user-roles-relationship", kwargs={"pk": create_test_user.id}), + data=data, + content_type="application/vnd.api+json", + ) assert response.status_code == status.HTTP_204_NO_CONTENT - relationships = UserRoleRelationship.objects.filter(role=roles_fixture[0].id) - assert relationships.count() == 0 + + def test_role_destroy_only_manage_account_blocked( + self, authenticated_client, tenants_fixture + ): + # Use a tenant without default admin role (tenant3) + tenant = tenants_fixture[2] + user = User.objects.get(email=TEST_USER) + # Add membership for this tenant + Membership.objects.create(user=user, tenant=tenant) + + # Create a single MANAGE_ACCOUNT role in this tenant + only_role = Role.objects.create( + name="only_ma", + tenant=tenant, + manage_users=True, + manage_account=True, + manage_billing=False, + manage_providers=False, + manage_integrations=False, + manage_scans=False, + unlimited_visibility=False, + ) + + # Switch token to this tenant + serializer = TokenSerializer( + data={ + "type": "tokens", + "email": TEST_USER, + "password": TEST_PASSWORD, + "tenant_id": str(tenant.id), + } + ) + serializer.is_valid(raise_exception=True) + access_token = serializer.validated_data["access"] + authenticated_client.defaults["HTTP_AUTHORIZATION"] = f"Bearer {access_token}" + + # Attempt to delete the only MANAGE_ACCOUNT role + response = authenticated_client.delete( + reverse("role-detail", kwargs={"pk": only_role.id}) + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert Role.objects.filter(id=only_role.id).exists() def test_invalid_provider_group_id(self, authenticated_client, create_test_user): invalid_id = "non-existent-id" diff --git a/api/src/backend/api/v1/serializers.py b/api/src/backend/api/v1/serializers.py index 20590b3388..edac0acab7 100644 --- a/api/src/backend/api/v1/serializers.py +++ b/api/src/backend/api/v1/serializers.py @@ -422,6 +422,34 @@ class UserRoleRelationshipSerializer(RLSSerializer, BaseWriteSerializer): roles = Role.objects.filter(id__in=role_ids) tenant_id = self.context.get("tenant_id") + # Safeguard: A tenant must always have at least one user with MANAGE_ACCOUNT. + # If the target roles do NOT include MANAGE_ACCOUNT, and the current user is + # the only one in the tenant with MANAGE_ACCOUNT, block the update. + target_includes_manage_account = roles.filter(manage_account=True).exists() + if not target_includes_manage_account: + # Check if any other user has MANAGE_ACCOUNT + other_users_have_manage_account = ( + UserRoleRelationship.objects.filter( + tenant_id=tenant_id, role__manage_account=True + ) + .exclude(user_id=instance.id) + .exists() + ) + + # Check if the current user has MANAGE_ACCOUNT + instance_has_manage_account = instance.roles.filter( + tenant_id=tenant_id, manage_account=True + ).exists() + + # If the current user is the last holder of MANAGE_ACCOUNT, prevent removal + if instance_has_manage_account and not other_users_have_manage_account: + raise serializers.ValidationError( + { + "roles": "At least one user in the tenant must retain MANAGE_ACCOUNT. " + "Assign MANAGE_ACCOUNT to another user before removing it here." + } + ) + instance.roles.clear() new_relationships = [ UserRoleRelationship(user=instance, role=r, tenant_id=tenant_id) @@ -1741,6 +1769,37 @@ class RoleUpdateSerializer(RoleSerializer): if "users" in validated_data: users = validated_data.pop("users") + # Prevent a user from removing their own role assignment via Role update + request = self.context.get("request") + if request and getattr(request, "user", None): + request_user = request.user + is_currently_assigned = instance.users.filter( + id=request_user.id + ).exists() + will_be_assigned = any(u.id == request_user.id for u in users) + if is_currently_assigned and not will_be_assigned: + raise serializers.ValidationError( + {"users": "Users cannot remove their own role."} + ) + + # Safeguard MANAGE_ACCOUNT coverage when updating users of this role + if instance.manage_account: + # Existing MANAGE_ACCOUNT assignments on other roles within the tenant + other_ma_exists = ( + UserRoleRelationship.objects.filter( + tenant_id=tenant_id, role__manage_account=True + ) + .exclude(role_id=instance.id) + .exists() + ) + + if not other_ma_exists and len(users) == 0: + raise serializers.ValidationError( + { + "users": "At least one user in the tenant must retain MANAGE_ACCOUNT. " + "Assign this MANAGE_ACCOUNT role to at least one user or ensure another user has it." + } + ) instance.users.clear() through_model_instances = [ UserRoleRelationship( diff --git a/api/src/backend/api/v1/views.py b/api/src/backend/api/v1/views.py index e7d188bd03..3d95127400 100644 --- a/api/src/backend/api/v1/views.py +++ b/api/src/backend/api/v1/views.py @@ -902,7 +902,11 @@ class UserViewSet(BaseUserViewset): partial_update=extend_schema( tags=["User"], summary="Partially update a user-roles relationship", - description="Update the user-roles relationship information without affecting other fields.", + description=( + "Update the user-roles relationship information without affecting other fields. " + "If the update would remove MANAGE_ACCOUNT from the last remaining user in the " + "tenant, the API rejects the request with a 400 response." + ), responses={ 204: OpenApiResponse( response=None, description="Relationship updated successfully" @@ -912,7 +916,12 @@ class UserViewSet(BaseUserViewset): destroy=extend_schema( tags=["User"], summary="Delete a user-roles relationship", - description="Remove the user-roles relationship from the system by their ID.", + description=( + "Remove the user-roles relationship from the system by their ID. If removing " + "MANAGE_ACCOUNT would take it away from the last remaining user in the tenant, " + "the API rejects the request with a 400 response. Users also cannot delete their " + "own role assignments; attempting to do so returns a 400 response." + ), responses={ 204: OpenApiResponse( response=None, description="Relationship deleted successfully" @@ -932,6 +941,43 @@ class UserRoleRelationshipView(RelationshipView, BaseRLSViewSet): def get_queryset(self): return User.objects.filter(membership__tenant__id=self.request.tenant_id) + def destroy(self, request, *args, **kwargs): + """ + Prevent deleting role relationships if it would leave the tenant with no + users having MANAGE_ACCOUNT. Supports deleting specific roles via JSON:API + relationship payload or clearing all roles for the user when no payload. + """ + user = self.get_object() + # Disallow deleting own roles + if str(user.id) == str(request.user.id): + return Response( + data={ + "detail": "Users cannot delete the relationship with their role." + }, + status=status.HTTP_400_BAD_REQUEST, + ) + tenant_id = self.request.tenant_id + payload = request.data if isinstance(request.data, dict) else None + + # If a user has more than one role, we will delete the relationship with the roles in the payload + data = payload.get("data") if payload else None + if data: + try: + role_ids = [item["id"] for item in data] + except KeyError: + role_ids = [] + roles_to_remove = Role.objects.filter(id__in=role_ids, tenant_id=tenant_id) + else: + roles_to_remove = user.roles.filter(tenant_id=tenant_id) + + UserRoleRelationship.objects.filter( + user=user, + tenant_id=tenant_id, + role_id__in=roles_to_remove.values_list("id", flat=True), + ).delete() + + return Response(status=status.HTTP_204_NO_CONTENT) + def create(self, request, *args, **kwargs): user = self.get_object() @@ -970,12 +1016,6 @@ class UserRoleRelationshipView(RelationshipView, BaseRLSViewSet): serializer.save() return Response(status=status.HTTP_204_NO_CONTENT) - def destroy(self, request, *args, **kwargs): - user = self.get_object() - user.roles.clear() - - return Response(status=status.HTTP_204_NO_CONTENT) - @extend_schema_view( list=extend_schema( @@ -2880,13 +2920,11 @@ class InvitationAcceptViewSet(BaseRLSViewSet): partial_update=extend_schema( tags=["Role"], summary="Partially update a role", - description="Update certain fields of an existing role's information without affecting other fields.", responses={200: RoleSerializer}, ), destroy=extend_schema( tags=["Role"], summary="Delete a role", - description="Remove a role from the system by their ID.", ), ) class RoleViewSet(BaseRLSViewSet): @@ -2908,6 +2946,14 @@ class RoleViewSet(BaseRLSViewSet): return RoleUpdateSerializer return super().get_serializer_class() + @extend_schema( + description=( + "Update selected fields on an existing role. When changing the `users` " + "relationship of a role that grants MANAGE_ACCOUNT, the API blocks attempts " + "that would leave the tenant without any MANAGE_ACCOUNT assignees and prevents " + "callers from removing their own assignment to that role." + ) + ) def partial_update(self, request, *args, **kwargs): user_role = get_role(request.user) # If the user is the owner of the role, the manage_account field is not editable @@ -2915,6 +2961,12 @@ class RoleViewSet(BaseRLSViewSet): request.data["manage_account"] = str(user_role.manage_account).lower() return super().partial_update(request, *args, **kwargs) + @extend_schema( + description=( + "Delete the specified role. The API rejects deletion of the last role " + "in the tenant that grants MANAGE_ACCOUNT." + ) + ) def destroy(self, request, *args, **kwargs): instance = self.get_object() if ( @@ -2922,6 +2974,21 @@ class RoleViewSet(BaseRLSViewSet): ): # TODO: Move to a constant/enum (in case other roles are created by default) raise ValidationError(detail="The admin role cannot be deleted.") + # Prevent deleting the last MANAGE_ACCOUNT role in the tenant + if instance.manage_account: + has_other_ma = ( + Role.objects.filter(tenant_id=instance.tenant_id, manage_account=True) + .exclude(id=instance.id) + .exists() + ) + if not has_other_ma: + return Response( + data={ + "detail": "Cannot delete the only role with MANAGE_ACCOUNT in the tenant." + }, + status=status.HTTP_400_BAD_REQUEST, + ) + return super().destroy(request, *args, **kwargs)