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

[lpichler]

Link(s) to Jira

• RHCLOUD-36627

Description of Intent of Change(s)

This fixes that two cases:
Cross Account Request Creation and update

• Role names are passed in API request for Cross Account Request
• Query doesn't contain filter on system role in Cross Account Request Role assignment in code
  • if there is existing custom role with same name(display name) as system role - this custom role is also added in to Cross Account Request -> this is improper
    Access with Cross Account Request
• if there is existing custom role with same name as system role - this custom role's permissions are added to response in access endpoint for cross principal account

Impact on Dual Write and replication:
Dual Write for cross account request accept only system role, in this all custom role is passed which is causing error and 500 request.

Local Testing

• Creation
  • Create custom role with same display name as existing system role
  • Create cross account request with this name of role
  • Check in db there are records for both roles in api_requestsroles for created cross account request
• Access
  • same as creation
  • access call request

Checklist

• if API spec changes are required, is the spec updated?
• are there any pre/post merge actions required? if so, document here.
• are theses changes covered by unit tests?
• if warranted, are documentation changes accounted for?
• does this require migration changes?
  • if yes, are they backwards compatible?
• is there known, direct impact to dependent teams/components?
  • if yes, how will this be handled?

Secure Coding Practices Checklist Link

• https://github.com/RedHatInsights/secure-coding-checklist

Secure Coding Practices Checklist

• Input Validation
• Output Encoding
• Authentication and Password Management
• Session Management
• Access Control
• Cryptographic Practices
• Error Handling and Logging
• Data Protection
• Communication Security
• System Configuration
• Database Security
• File Management
• Memory Management
• General Coding Practices

[alechenninger]

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!!

[alechenninger]

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

[astrozzc]

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)

[alechenninger]

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

[astrozzc]

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"]

[alechenninger]

Ah I see. Maybe we could change the role fields validator then I wonder? Just curious now, not a comment on the implementation 😀 Sent with Shortwave <https://www.shortwave.com?utm_medium=email&utm_content=signature&utm_source=YWxlY2hlbm5pbmdlckBnbWFpbC5jb20=>

On Mon Dec 16, 2024, 01:25 PM GMT, Jay Z ***@***.***> wrote: @astrozzc commented on this pull request.

------------------------------

…

In rbac/api/cross_access/serializer.py <#1372 (comment)>: > 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) 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"] — Reply to this email directly, view it on GitHub <#1372 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2VU323K5W2QDOI4QQNKKT2F3IEPAVCNFSM6AAAAABTERS56OVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMBWGE4DIOBYHA>. You are receiving this because your review was requested.Message ID: ***@***.***>

[astrozzc]

Ah I see. Maybe we could change the role fields validator then I wonder? Just curious now, not a comment on the implementation 😀 Sent with Shortwave https://www.shortwave.com?utm_medium=email&utm_content=signature&utm_source=YWxlY2hlbm5pbmdlckBnbWFpbC5jb20=
On Mon Dec 16, 2024, 01:25 PM GMT, Jay Z @.***> wrote: @astrozzc commented on this pull request.

…
In rbac/api/cross_access/serializer.py <#1372 (comment)>: > 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) 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"] — Reply to this email directly, view it on GitHub <#1372 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA2VU323K5W2QDOI4QQNKKT2F3IEPAVCNFSM6AAAAABTERS56OVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMBWGE4DIOBYHA. You are receiving this because your review was requested.Message ID: @.>

Yea, we can also modify the to_internal_value method of the RoleSerializer to translate that display_name to {"uuid": "xxx"}