From 36703f1e521f3b6286e11e5e0b6926f66772eae8 Mon Sep 17 00:00:00 2001 From: Libor Pichler Date: Fri, 6 Dec 2024 13:01:10 +0100 Subject: [PATCH 1/4] Improve query to fetch related roles in access --- rbac/management/utils.py | 16 ++++---- tests/management/access/test_view.py | 57 +++++++++++++++++++++++++--- 2 files changed, 59 insertions(+), 14 deletions(-) diff --git a/rbac/management/utils.py b/rbac/management/utils.py index e934b18c7..519ce91d1 100644 --- a/rbac/management/utils.py +++ b/rbac/management/utils.py @@ -29,7 +29,7 @@ from rest_framework import serializers from rest_framework.request import Request -from api.models import CrossAccountRequest, Tenant +from api.models import Tenant USERNAME_KEY = "username" APPLICATION_KEY = "application" @@ -282,14 +282,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): diff --git a/tests/management/access/test_view.py b/tests/management/access/test_view.py index b6aa96da3..bb28f0eea 100644 --- a/tests/management/access/test_view.py +++ b/tests/management/access/test_view.py @@ -42,7 +42,7 @@ def setUp(self): user.account = self.customer_data["account_id"] user.org_id = self.customer_data["org_id"] request.user = user - public_tenant = Tenant.objects.get(tenant_name="public") + self.public_tenant = Tenant.objects.get(tenant_name="public") self.access_data = { "permission": "app:*:*", @@ -151,8 +151,8 @@ def create_platform_default_resource(self): ) default_group.policies.add(default_policy) - def create_role_and_permission(self, role_name, permission): - role = Role.objects.create(name=role_name, tenant=self.tenant) + def create_role_and_permission(self, role_name, permission, system=False): + role = Role.objects.create(name=role_name, tenant=self.tenant, system=system) assigned_permission = Permission.objects.create(permission=permission, tenant=self.tenant) access = Access.objects.create(role=role, permission=assigned_permission, tenant=self.tenant) return role @@ -227,6 +227,53 @@ def test_get_access_to_return_unique_records(self): response = client.get(url, **self.headers) self.assertEqual({"permission": "default:*:*", "resourceDefinitions": []}, response.data.get("data")[0]) + def test_access_for_cross_account_principal_return_permissions_based_on_assigned_system_role(self): + self.create_platform_default_resource() + client = APIClient() + url = "{}?application=".format(reverse("v1_management:access")) + account_id = self.customer_data["account_id"] + org_id = self.customer_data["org_id"] + user_id = "123456" + user_name = f"{account_id}-{user_id}" + + # create system role with access + system_role = Role.objects.create(name="Test Role one", tenant=self.public_tenant, system=True) + assigned_permission = Permission.objects.create( + permission="test:assigned:permission1", tenant=self.public_tenant + ) + Access.objects.create(role=system_role, permission=assigned_permission, tenant=self.tenant) + + cross_account_request = CrossAccountRequest.objects.create( + target_account=account_id, + user_id=user_id, + target_org=org_id, + end_date=timezone.now() + timedelta(10), + status="approved", + ) + cross_account_request.roles.add(system_role) + + # create non-system role with access and same name as system role + role = Role.objects.create(name="Test Role one", tenant=self.tenant, system=False) + assigned_permission = Permission.objects.create( + permission="test:assigned:permission2", tenant=self.public_tenant + ) + Access.objects.create(role=role, permission=assigned_permission, tenant=self.tenant) + + # Create cross_account principal and role, permission in the account + user_data = {"username": user_name, "email": "test@gmail.com"} + request_context = self._create_request_context(self.customer_data, user_data, is_org_admin=False) + request = request_context["request"] + self.test_headers = request.META + Principal.objects.create(username=user_name, cross_account=True, tenant=self.tenant) + + response = client.get(url, **self.test_headers) + + # only assigned role permissions without platform default permission + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(len(response.data.get("data")), 1) + + self.assertEqual(response.data.get("data")[0]["permission"], "test:assigned:permission1") + def test_access_for_cross_account_principal_return_permissions_based_on_assigned_role(self): """Test that the expected access for cross account principal return permissions based on assigned role.""" # setup default group/role @@ -240,7 +287,7 @@ def test_access_for_cross_account_principal_return_permissions_based_on_assigned # setup cross account request, role and permission in public schema ## This CAR will provide permission: "test:assigned:permission" - role = self.create_role_and_permission("Test Role one", "test:assigned:permission1") + role = self.create_role_and_permission("Test Role one", "test:assigned:permission1", True) cross_account_request = CrossAccountRequest.objects.create( target_account=account_id, user_id=user_id, @@ -250,7 +297,7 @@ def test_access_for_cross_account_principal_return_permissions_based_on_assigned ) cross_account_request.roles.add(role) ## CAR below will provide permission: "app:*:*" - role = self.create_role_and_permission("Test Role two", "test:assigned:permission2") + role = self.create_role_and_permission("Test Role two", "test:assigned:permission2", True) cross_account_request = CrossAccountRequest.objects.create( target_account=account_id, user_id=user_id, From 30b489fabdc1861b7fe09f1448b1d340644b35b9 Mon Sep 17 00:00:00 2001 From: Libor Pichler Date: Fri, 6 Dec 2024 13:17:31 +0100 Subject: [PATCH 2/4] Improve query to fetch only system roles in cross access request creation --- rbac/api/cross_access/serializer.py | 4 ++-- tests/api/cross_access/test_view.py | 25 +++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/rbac/api/cross_access/serializer.py b/rbac/api/cross_access/serializer.py index 4d07f4b09..faedf6a52 100644 --- a/rbac/api/cross_access/serializer.py +++ b/rbac/api/cross_access/serializer.py @@ -112,10 +112,10 @@ def create(self, validated_data): display_names = [role["display_name"] for role in role_data] request = CrossAccountRequest.objects.create(**validated_data) cross_account_access_handler(request, self.context["user"]) - - roles = Role.objects.filter(display_name__in=display_names) + roles = Role.objects.filter(display_name__in=display_names, system=True) for role in roles: request.roles.add(role) + return request @transaction.atomic diff --git a/tests/api/cross_access/test_view.py b/tests/api/cross_access/test_view.py index 8578026f2..89c29f0e3 100644 --- a/tests/api/cross_access/test_view.py +++ b/tests/api/cross_access/test_view.py @@ -307,6 +307,31 @@ def test_create_requests_success(self, notify_mock): self.data4create["target_org"], ) + @patch("management.notifications.notification_handlers.notify") + def test_create_requests_success_with_same_name_or_roles(self, notify_mock): + """Test the creation of cross account request success.""" + Role.objects.create(name="role_1", system=False, tenant=self.tenant) + client = APIClient() + with self.settings(NOTIFICATIONS_ENABLED=True): + response = client.post( + f"{URL_LIST}?", self.data4create, format="json", **self.associate_non_admin_request.META + ) + + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + self.assertEqual(response.data["target_account"], self.data4create["target_account"]) + self.assertEqual(response.data["status"], "pending") + self.assertEqual(response.data["start_date"], self.data4create["start_date"]) + self.assertEqual(response.data["end_date"], self.data4create["end_date"]) + self.assertEqual(len(response.data["roles"]), 2) + notify_mock.assert_called_once_with( + EVENT_TYPE_RH_TAM_REQUEST_CREATED, + { + "username": self.user_data["username"], + "request_id": response.data["request_id"], + }, + self.data4create["target_org"], + ) + def test_create_requests_fail_for_no_account(self): """Test the creation of cross account request fails when the account doesn't exist.""" From fc8bdb719be3b88d3ed8b41779c769923f18a7c6 Mon Sep 17 00:00:00 2001 From: Libor Pichler Date: Wed, 11 Dec 2024 16:40:01 +0100 Subject: [PATCH 3/4] Leverage query in validation to store role assigments for cross access account creation and update --- rbac/api/cross_access/serializer.py | 21 ++++++++---------- rbac/api/cross_access/view.py | 13 ++++++------ tests/api/cross_access/test_view.py | 33 ++++++++++++++++++++++++++++- 3 files changed, 48 insertions(+), 19 deletions(-) diff --git a/rbac/api/cross_access/serializer.py b/rbac/api/cross_access/serializer.py index faedf6a52..0f7cdc085 100644 --- a/rbac/api/cross_access/serializer.py +++ b/rbac/api/cross_access/serializer.py @@ -56,7 +56,8 @@ class Meta: class RoleSerializer(serializers.ModelSerializer): """Serializer for the roles of cross access request model.""" - display_name = serializers.CharField(max_length=150) + uuid = serializers.UUIDField(read_only=True) + display_name = serializers.CharField(max_length=150, read_only=True) description = serializers.CharField(max_length=150, read_only=True) permissions = serializers.SerializerMethodField(read_only=True) @@ -64,7 +65,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.""" @@ -108,25 +109,21 @@ def get_roles(self, obj): 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] + 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, system=True) - for role in roles: - request.roles.add(role) - + role_uuids = [role["uuid"] for role in self.context["request"].data["roles"]] + 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) + validated_data.pop("roles") + role_uuids = [role["uuid"] for role in self.context["request"].data["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)) diff --git a/rbac/api/cross_access/view.py b/rbac/api/cross_access/view.py index 47d1eea70..73e574f86 100644 --- a/rbac/api/cross_access/view.py +++ b/rbac/api/cross_access/view.py @@ -220,18 +220,19 @@ def validate_and_format_input(self, request_data): except Tenant.DoesNotExist: raise self.throw_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["roles"] = self.find_system_role_uuids_by_names(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")) + request_data["roles"] = self.find_system_role_uuids_by_names(request_data.get("roles")) - def format_roles(self, roles): + def find_system_role_uuids_by_names(self, role_names): """Format role list as expected for cross-account-request.""" public_tenant = Tenant.objects.get(tenant_name="public") - for role_name in roles: + system_role_uuids = [] + for role_name in role_names: try: role = Role.objects.get(tenant=public_tenant, display_name=role_name) if not role.system: @@ -240,8 +241,8 @@ def format_roles(self, roles): ) 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] + system_role_uuids.append({"uuid": role.uuid}) + return system_role_uuids def _with_dual_write_handler( self, diff --git a/tests/api/cross_access/test_view.py b/tests/api/cross_access/test_view.py index 89c29f0e3..7364dcf02 100644 --- a/tests/api/cross_access/test_view.py +++ b/tests/api/cross_access/test_view.py @@ -291,7 +291,6 @@ def test_create_requests_success(self, notify_mock): response = client.post( f"{URL_LIST}?", self.data4create, format="json", **self.associate_non_admin_request.META ) - self.assertEqual(response.status_code, status.HTTP_201_CREATED) self.assertEqual(response.data["target_account"], self.data4create["target_account"]) self.assertEqual(response.data["status"], "pending") @@ -489,6 +488,38 @@ def test_update_request_success_for_requestor(self): continue self.assertEqual(self.data4create.get(field), response.data.get(field)) + def test_update_request_success_for_requestor_when_custom_role_with_same_name_exist(self): + """Test updating an entire CAR.""" + Role.objects.create(name="role_8", system=False, tenant=self.tenant) + self.data4create["target_account"] = self.another_account + self.data4create["target_org"] = self.another_org_id + Tenant.objects.create( + tenant_name=f"acct{self.another_account}", account_id=self.another_account, org_id=self.another_org_id + ) + self.data4create["start_date"] = self.format_date(self.ref_time + timedelta(3)) + self.data4create["end_date"] = self.format_date(self.ref_time + timedelta(5)) + self.data4create["roles"] = ["role_8", "role_9"] + self.data4create["status"] = "pending" + + car_uuid = self.request_1.request_id + self.request_1.target_account = self.another_account + self.request_1.target_org = self.another_org_id + self.request_1.status = "pending" + self.request_1.save() + url = reverse("v1_api:cross-detail", kwargs={"pk": str(car_uuid)}) + + client = APIClient() + response = client.put(url, self.data4create, format="json", **self.associate_admin_request.META) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + for field in self.data4create: + if field == "roles": + for role in response.data.get("roles"): + self.assertIn(role.get("display_name"), self.data4create["roles"]) + continue + self.assertEqual(self.data4create.get(field), response.data.get(field)) + self.assertEqual(len(response.data.get("roles")), 2) + def test_update_request_fail_acct_for_requestor(self): """Test that updating the account of a CAR fails.""" Tenant.objects.create( From 787dda0962d089ba477ba8e0251aa0bf12d840fd Mon Sep 17 00:00:00 2001 From: Libor Pichler Date: Fri, 20 Dec 2024 13:06:41 +0100 Subject: [PATCH 4/4] WIP --- rbac/api/cross_access/serializer.py | 36 ++++++++++++++++++++++++----- rbac/api/cross_access/view.py | 12 ++++++++-- tests/api/cross_access/test_view.py | 1 - 3 files changed, 40 insertions(+), 9 deletions(-) diff --git a/rbac/api/cross_access/serializer.py b/rbac/api/cross_access/serializer.py index 0f7cdc085..65ea9c613 100644 --- a/rbac/api/cross_access/serializer.py +++ b/rbac/api/cross_access/serializer.py @@ -21,8 +21,9 @@ from management.notifications.notification_handlers import cross_account_access_handler from management.permission.serializer import PermissionSerializer from rest_framework import serializers +from rest_framework.serializers import ValidationError -from api.models import CrossAccountRequest +from api.models import CrossAccountRequest, Tenant class CrossAccountRequestSerializer(serializers.ModelSerializer): @@ -57,7 +58,7 @@ 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, 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) @@ -107,12 +108,36 @@ def get_roles(self, obj): serialized_roles = [RoleSerializer(role).data for role in obj.roles.all()] return serialized_roles + def throw_validation_error(self, source, message): + """Construct a validation error and raise the error.""" + error = {source: [message]} + raise ValidationError(error) + + def validate_roles(self, roles): + """Format role list as expected for cross-account-request.""" + public_tenant = Tenant.objects.get(tenant_name="public") + system_role_uuids = [] + for role in roles: + role_display_name = role["display_name"] + try: + role = Role.objects.get(tenant=public_tenant, display_name=role_display_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_display_name}' does not exist." + ) + system_role_uuids.append(role.uuid) + return system_role_uuids + def create(self, validated_data): """Override the create method to associate the roles to cross account request after it is created.""" - validated_data.pop("roles") + role_uuids = validated_data.pop("roles") request = CrossAccountRequest.objects.create(**validated_data) cross_account_access_handler(request, self.context["user"]) - role_uuids = [role["uuid"] for role in self.context["request"].data["roles"]] + # role_uuids = [role["uuid"] for role in self.context["request"].data["roles"]] request.roles.add(*role_uuids) return request @@ -120,8 +145,7 @@ def create(self, validated_data): 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: - validated_data.pop("roles") - role_uuids = [role["uuid"] for role in self.context["request"].data["roles"]] + role_uuids = validated_data.pop("roles") instance.roles.clear() instance.roles.add(*role_uuids) diff --git a/rbac/api/cross_access/view.py b/rbac/api/cross_access/view.py index 73e574f86..a7b654a4a 100644 --- a/rbac/api/cross_access/view.py +++ b/rbac/api/cross_access/view.py @@ -120,9 +120,15 @@ def get_serializer_context(self): context["user"] = self.request.user return context + def format_roles(self, role_names): + """Format array with role to json with display_name.""" + return [{"display_name": role_name} for role_name in role_names] + def create(self, request, *args, **kwargs): """Create cross account requests for associate.""" self.validate_and_format_input(request.data) + if "roles" in request.data: + request.data["roles"] = self.format_roles(request.data["roles"]) return super().create(request=request, args=args, kwargs=kwargs) def list(self, request, *args, **kwargs): @@ -141,6 +147,8 @@ def partial_update(self, request, *args, **kwargs): def update(self, request, *args, **kwargs): """Update a cross-account request. TAM requestor use it to update their requesters.""" validate_uuid(kwargs.get("pk"), "cross-account request uuid validation") + if "roles" in request.data: + request.data["roles"] = self.format_roles(request.data["roles"]) return super().update(request=request, *args, **kwargs) def perform_update(self, serializer): @@ -220,13 +228,13 @@ def validate_and_format_input(self, request_data): except Tenant.DoesNotExist: raise self.throw_validation_error("cross-account-request", f"Org ID '{target_org}' does not exist.") - request_data["roles"] = self.find_system_role_uuids_by_names(request_data.get("roles")) + # request_data["roles"] = self.find_system_role_uuids_by_names(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.find_system_role_uuids_by_names(request_data.get("roles")) + request_data["roles"] = self.format_roles(request_data.get("roles")) def find_system_role_uuids_by_names(self, role_names): """Format role list as expected for cross-account-request.""" diff --git a/tests/api/cross_access/test_view.py b/tests/api/cross_access/test_view.py index 7364dcf02..62ed62d47 100644 --- a/tests/api/cross_access/test_view.py +++ b/tests/api/cross_access/test_view.py @@ -479,7 +479,6 @@ def test_update_request_success_for_requestor(self): client = APIClient() response = client.put(url, self.data4create, format="json", **self.associate_admin_request.META) - self.assertEqual(response.status_code, status.HTTP_200_OK) for field in self.data4create: if field == "roles":