[RHCLOUD-36627] Query only system roles for cross account request acc…

…ess and creation (RedHatInsights#1372)

* Improve query to fetch related roles in access

* Improve query to fetch only system roles in cross access request creation

* Leverage query in validation to store role assigments for cross access account creation and update

* Use validate_roles and to_internal value on RoleSerializer

This allowed also to simplify patch method and remove validation
method on view.

* Rename and put method raise_validation_error into utils

rbac/api/cross_access/access_control.py

@@ -60,6 +60,5 @@ def has_object_permission(self, request, view, obj):
             view.validate_and_format_input(request.data)
         elif request.method == "PATCH":
             view.check_patch_permission(request, obj)
-            view.validate_and_format_patch_input(request.data)
 
         return True

rbac/api/cross_access/serializer.py

@@ -20,9 +20,10 @@
 from management.models import Role
 from management.notifications.notification_handlers import cross_account_access_handler
 from management.permission.serializer import PermissionSerializer
+from management.utils import raise_validation_error
 from rest_framework import serializers
 
-from api.models import CrossAccountRequest
+from api.models import CrossAccountRequest, Tenant
 
 
 class CrossAccountRequestSerializer(serializers.ModelSerializer):
@@ -56,6 +57,7 @@ class Meta:
 class RoleSerializer(serializers.ModelSerializer):
     """Serializer for the roles of cross access request model."""
 
+    uuid = serializers.UUIDField(read_only=True)
     display_name = serializers.CharField(max_length=150)
     description = serializers.CharField(max_length=150, read_only=True)
     permissions = serializers.SerializerMethodField(read_only=True)
@@ -64,7 +66,7 @@ class Meta:
         """Metadata for the serializer."""
 
         model = Role
-        fields = ("display_name", "description", "permissions")
+        fields = ("uuid", "display_name", "description", "permissions")
 
     def get_permissions(self, obj):
         """Permissions constructor for the serializer."""
@@ -106,27 +108,52 @@ def get_roles(self, obj):
         serialized_roles = [RoleSerializer(role).data for role in obj.roles.all()]
         return serialized_roles
 
+    def validate_roles(self, roles):
+        """Format role list as expected for cross-account-request."""
+        public_tenant = Tenant.objects.get(tenant_name="public")
+
+        role_display_names = [role["display_name"] for role in roles]
+        roles_queryset = Role.objects.filter(tenant=public_tenant, display_name__in=role_display_names)
+        role_dict = {role.display_name: role for role in roles_queryset}
+
+        system_role_uuids = []
+        for role in roles:
+            role_display_name = role["display_name"]
+
+            if role_display_name not in role_dict:
+                raise raise_validation_error("cross-account-request", f"Role '{role_display_name}' does not exist.")
+
+            role = role_dict[role_display_name]
+            if not role.system:
+                raise_validation_error(
+                    "cross-account-request", "Only system roles may be assigned to a cross-account-request."
+                )
+
+            system_role_uuids.append(role.uuid)
+
+        return system_role_uuids
+
+    def to_internal_value(self, data):
+        """Convert the incoming 'roles' data into the expected format."""
+        if "roles" in data:
+            data["roles"] = [{"display_name": role_name} for role_name in data["roles"]]
+        return super().to_internal_value(data)
+
     def create(self, validated_data):
         """Override the create method to associate the roles to cross account request after it is created."""
-        role_data = validated_data.pop("roles")
-        display_names = [role["display_name"] for role in role_data]
+        role_uuids = validated_data.pop("roles")
         request = CrossAccountRequest.objects.create(**validated_data)
         cross_account_access_handler(request, self.context["user"])
-
-        roles = Role.objects.filter(display_name__in=display_names)
-        for role in roles:
-            request.roles.add(role)
+        request.roles.add(*role_uuids)
         return request
 
     @transaction.atomic
     def update(self, instance, validated_data):
         """Override the update method to associate the roles to cross account request after it is updated."""
         if "roles" in validated_data:
-            role_data = validated_data.pop("roles")
-            display_names = [role["display_name"] for role in role_data]
-            roles = Role.objects.filter(display_name__in=display_names)
+            role_uuids = validated_data.pop("roles")
             instance.roles.clear()
-            instance.roles.add(*roles)
+            instance.roles.add(*role_uuids)
 
         for field in validated_data:
             setattr(instance, field, validated_data.get(field))

rbac/api/cross_access/view.py

@@ -23,13 +23,11 @@
 from django.utils import timezone
 from django_filters import rest_framework as filters
 from management.filters import CommonFilters
-from management.models import Role
 from management.principal.proxy import PrincipalProxy
 from management.relation_replicator.relation_replicator import ReplicationEventType
-from management.utils import validate_and_get_key, validate_uuid
+from management.utils import raise_validation_error, validate_and_get_key, validate_uuid
 from rest_framework import mixins, viewsets
 from rest_framework.filters import OrderingFilter
-from rest_framework.serializers import ValidationError
 
 from api.cross_access.access_control import CrossAccountRequestAccessPermission
 from api.cross_access.relation_api_dual_write_cross_access_handler import RelationApiDualWriteCrossAccessHandler
@@ -149,8 +147,7 @@ def perform_update(self, serializer):
         request = self.request
         if serializer.partial and request.data.get("status"):
             self.update_status(current, request.data.get("status"))
-        else:
-            serializer.save()
+        serializer.save()
 
     def retrieve(self, request, *args, **kwargs):
         """Retrieve cross account requests by request_id."""
@@ -198,51 +195,25 @@ def replace_user_id_with_info(self, result):
 
         return result
 
-    def throw_validation_error(self, source, message):
-        """Construct a validation error and raise the error."""
-        error = {source: [message]}
-        raise ValidationError(error)
-
     def validate_and_format_input(self, request_data):
         """Validate the create api input."""
         for field in PARAMS_FOR_CREATION:
             if not request_data.__contains__(field):
-                self.throw_validation_error("cross-account-request", f"Field {field} must be specified.")
+                raise_validation_error("cross-account-request", f"Field {field} must be specified.")
 
         target_org = request_data.get("target_org")
         if target_org == self.request.user.org_id:
-            self.throw_validation_error(
+            raise_validation_error(
                 "cross-account-request", "Creating a cross access request for your own org id is not allowed."
             )
 
         try:
             Tenant.objects.get(org_id=target_org)
         except Tenant.DoesNotExist:
-            raise self.throw_validation_error("cross-account-request", f"Org ID '{target_org}' does not exist.")
+            raise raise_validation_error("cross-account-request", f"Org ID '{target_org}' does not exist.")
 
-        request_data["roles"] = self.format_roles(request_data.get("roles"))
         request_data["user_id"] = self.request.user.user_id
 
-    def validate_and_format_patch_input(self, request_data):
-        """Validate the create api input."""
-        if "roles" in request_data:
-            request_data["roles"] = self.format_roles(request_data.get("roles"))
-
-    def format_roles(self, roles):
-        """Format role list as expected for cross-account-request."""
-        public_tenant = Tenant.objects.get(tenant_name="public")
-        for role_name in roles:
-            try:
-                role = Role.objects.get(tenant=public_tenant, display_name=role_name)
-                if not role.system:
-                    self.throw_validation_error(
-                        "cross-account-request", "Only system roles may be assigned to a cross-account-request."
-                    )
-            except Role.DoesNotExist:
-                raise self.throw_validation_error("cross-account-request", f"Role '{role_name}' does not exist.")
-
-        return [{"display_name": role} for role in roles]
-
     def _with_dual_write_handler(
         self,
         car: CrossAccountRequest,
@@ -282,7 +253,6 @@ def update_status(self, car, status):
                     cross_account_roles
                 ),
             )
-        car.save()
 
     def check_patch_permission(self, request, update_obj):
         """Check if user has right to patch cross access request."""
@@ -291,56 +261,54 @@ def check_patch_permission(self, request, update_obj):
             may update status from pending/approved/denied to approved/denied.
             """
             if not request.user.admin:
-                self.throw_validation_error("cross-account partial update", "Only org admins may update status.")
+                raise_validation_error("cross-account partial update", "Only org admins may update status.")
             if update_obj.status not in ["pending", "approved", "denied"]:
-                self.throw_validation_error(
+                raise_validation_error(
                     "cross-account partial update", "Only pending/approved/denied requests may be updated."
                 )
             if request.data.get("status") not in ["approved", "denied"]:
-                self.throw_validation_error(
+                raise_validation_error(
                     "cross-account partial update", "Request status may only be updated to approved/denied."
                 )
             if len(request.data.keys()) > 1 or next(iter(request.data)) != "status":
-                self.throw_validation_error("cross-account partial update", "Only status may be updated.")
+                raise_validation_error("cross-account partial update", "Only status may be updated.")
         elif request.user.user_id == update_obj.user_id:
             """For requestors updating their requests, the request status may
             only be updated from pending to cancelled.
             """
             if update_obj.status != "pending" or request.data.get("status") != "cancelled":
-                self.throw_validation_error(
+                raise_validation_error(
                     "cross-account partial update", "Request status may only be updated from pending to cancelled."
                 )
             for field in request.data:
                 if field not in VALID_PATCH_FIELDS:
-                    self.throw_validation_error(
+                    raise_validation_error(
                         "cross-account partial update",
                         f"Field '{field}' is not supported. Please use one or more of: {VALID_PATCH_FIELDS}",
                     )
         else:
-            self.throw_validation_error(
+            raise_validation_error(
                 "cross-account partial update", "User does not have permission to update the request."
             )
 
     def check_update_permission(self, request, update_obj):
         """Check if user has permission to update cross access request."""
         # Only requestors could update the cross access request.
         if request.user.user_id != update_obj.user_id:
-            self.throw_validation_error(
-                "cross-account update", "Only the requestor may update the cross access request."
-            )
+            raise_validation_error("cross-account update", "Only the requestor may update the cross access request.")
 
         # Only pending request could be updated.
         if update_obj.status != "pending":
-            self.throw_validation_error("cross-account update", "Only pending requests may be updated.")
+            raise_validation_error("cross-account update", "Only pending requests may be updated.")
 
         # Do not allow updating the status:
         if request.data.get("status") and str(request.data.get("status")) != "pending":
-            self.throw_validation_error(
+            raise_validation_error(
                 "cross-account update",
                 "The status may not be updated through PUT endpoint. "
                 "Please use PATCH to update the status of the request.",
             )
 
         # Do not allow updating the target_org.
         if request.data.get("target_org") and str(request.data.get("target_org")) != update_obj.target_org:
-            self.throw_validation_error("cross-account-update", "Target org must stay the same.")
+            raise_validation_error("cross-account-update", "Target org must stay the same.")

rbac/management/utils.py

@@ -28,8 +28,9 @@
 from management.principal.proxy import PrincipalProxy
 from rest_framework import serializers
 from rest_framework.request import Request
+from rest_framework.serializers import ValidationError
 
-from api.models import CrossAccountRequest, Tenant
+from api.models import Tenant
 
 USERNAME_KEY = "username"
 APPLICATION_KEY = "application"
@@ -282,14 +283,12 @@ def roles_for_cross_account_principal(principal):
     """Return roles for cross account principals."""
     _, user_id = principal.username.split("-")
     target_org = principal.tenant.org_id
-    role_names = (
-        CrossAccountRequest.objects.filter(target_org=target_org, user_id=user_id, status="approved")
-        .values_list("roles__name", flat=True)
-        .distinct()
-    )
-
-    role_names_list = list(role_names)
-    return Role.objects.filter(name__in=role_names_list)
+    return Role.objects.filter(
+        crossaccountrequest__target_org=target_org,
+        crossaccountrequest__user_id=user_id,
+        crossaccountrequest__status="approved",
+        system=True,
+    ).distinct()
 
 
 def clear_pk(entry):
@@ -353,3 +352,9 @@ def v2response_error_from_errors(errors, exc=None, context=None):
         response["instance"] = context.get("request").path
 
     return response
+
+
+def raise_validation_error(source, message):
+    """Construct a validation error and raise the error."""
+    error = {source: [message]}
+    raise ValidationError(error)