Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RHCLOUD-36627] Query only system roles for cross account request access and creation #1372

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+198 −31
Open

[RHCLOUD-36627] Query only system roles for cross account request access and creation #1372

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
927b7f5
Improve query to fetch related roles in access
lpichler Dec 6, 2024
1ac6e54
Improve query to fetch only system roles in cross access request crea…
lpichler Dec 6, 2024
c29fb82
Leverage query in validation to store role assigments for cross acces…
lpichler Dec 11, 2024
bd6e32e
WIP
lpichler Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 33 additions & 12 deletions rbac/api/cross_access/serializer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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)
Expand All @@ -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."""
Expand Down Expand Up @@ -106,27 +108,46 @@ 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."""
role_data = validated_data.pop("roles")
display_names = [role["display_name"] for role in role_data]
print("---")
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)
# role_uuids = [role["uuid"] for role in self.context["request"].data["roles"]]
request.roles.add(*role_uuids)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was talking with @astrozzc about this during collab. I wonder if we could move the validation and transformation to the serializer, if that would break things?

Something like this?

    def validate_roles(self, roles):
        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:
                    raise serializers.ValidationError(
                        "Only system roles may be assigned to a cross-account-request.", code="cross-account-request"
                    )
            except Role.DoesNotExist:
                raise serializers.ValidationError(f"Role '{role_name}' does not exist.", code="cross-account-request")
        return [{"uuid": role.uuid} for role in roles]

    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")
        role_uuids = [role["uuid"] for role in role_data]
        request = CrossAccountRequest.objects.create(**validated_data)
        cross_account_access_handler(request, self.context["user"])
        request.roles.add(*role_uuids)

And then you would take out the format / find role by display name stuff in the view and just leave it to the serializer.

This feels like what Django Rest Framework wants you to do, but dunno if it would change the existing behavior. Not tested!!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reference: https://www.django-rest-framework.org/api-guide/serializers/#field-level-validation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried this would work: the to_internal_value will call the role serializer validation, if we don't transform the display_name (strings) to list of {"uuid": "xxx"} before that, it would raise validation error.

    def to_internal_value(self, data):
        public_tenant = Tenant.objects.get(tenant_name="public")
        roles = data["roles"]
        role_objs = Role.objects.filter(tenant=public_tenant, display_name__in=roles)
        role_names = {role_obj.display_name for role_obj in role_objs}
        for role_name in roles:
            if role_name not in role_names:
                raise serializers.ValidationError(f"Role `'{role_name}'` does not exist.", code="cross-account-request")
        data["roles"] = [{"uuid": role.uuid} for role in role_objs]
        return super().to_internal_value(data)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder, why does returning the value from the validate role method not work? It looked like it used that value for the validated data from the docs & code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I spent sometime tracking it down, because it runs the validation of the RoleSerializer first before running the validate_roles of the CrossAccountDetailSerializer. The RoleSerializer requires it to be a dictionary like {"display_names": "xxx"} instead of list of display_names ["xxx"]

return request

@transaction.atomic
def update(self, instance, validated_data):
print("PATCH")
"""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))
Expand Down
22 changes: 17 additions & 5 deletions rbac/api/cross_access/view.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,14 @@ def get_serializer_context(self):
context["user"] = self.request.user
return context

def format_roles(self, role_names):
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):
Expand All @@ -143,10 +148,13 @@ def partial_update(self, request, *args, **kwargs):
"""Patch a cross-account request. Target account admin use it to update status of the request."""
return super().partial_update(request=request, *args, **kwargs)


@transaction.atomic
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):
Expand Down Expand Up @@ -226,18 +234,20 @@ 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."""
print("PATCHaaa")
if "roles" in request_data:
request_data["roles"] = self.format_roles(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:
Expand All @@ -246,8 +256,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,
Expand Down Expand Up @@ -311,6 +321,8 @@ def check_patch_permission(self, request, update_obj):
"""For requestors updating their requests, the request status may
only be updated from pending to cancelled.
"""
print(request.data.get("status"))
print(update_obj.status)
if update_obj.status != "pending" or request.data.get("status") != "cancelled":
self.throw_validation_error(
"cross-account partial update", "Request status may only be updated from pending to cancelled."
Expand Down
16 changes: 7 additions & 9 deletions rbac/management/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -283,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):
Expand Down
89 changes: 89 additions & 0 deletions tests/api/cross_access/test_view.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,32 @@ def test_retrieve_request_query_by_user_id_fail_if_not_associate(self):
@patch("management.notifications.notification_handlers.notify")
def test_create_requests_success(self, notify_mock):
"""Test the creation of cross account request success."""
print(Role.objects.all().values_list("display_name", "tenant_id"))
client = APIClient()
with self.settings(NOTIFICATIONS_ENABLED=True):
response = client.post(
f"{URL_LIST}?", self.data4create, format="json", **self.associate_non_admin_request.META
)
print(response.data)
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"],
)

@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(
Expand Down Expand Up @@ -434,6 +460,38 @@ def test_update_request_denied_for_approver(self):
response.data.get("errors")[0].get("detail"), "Only the requestor may update the cross access request."
)

def test_patch_request_success_for_requestor(self):
"""Test updating an entire CAR."""
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()
keys_to_exclude = ['target_account', 'target_org']
filtered_data = {k: v for k, v in self.data4create.items() if k not in keys_to_exclude}
print(filtered_data)
response = client.patch(url, filtered_data, format="json", **self.associate_admin_request.META)
print(response.data)
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))


def test_update_request_success_for_requestor(self):
"""Test updating an entire CAR."""
self.data4create["target_account"] = self.another_account
Expand All @@ -453,6 +511,36 @@ def test_update_request_success_for_requestor(self):
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))

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)

Expand All @@ -463,6 +551,7 @@ def test_update_request_success_for_requestor(self):
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."""
Expand Down
57 changes: 52 additions & 5 deletions tests/management/access/test_view.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:*:*",
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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": "[email protected]"}
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
Expand All @@ -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,
Expand All @@ -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,
Expand Down
Loading