Prevent modifying shared resources when using platform ingress

[fosterseth]

SUMMARY

List of endpoints affected

teams/
teams/users/
teams/<id>/
organization/
organization/users/
organization/admins/
organization/teams/
organization/<id>/
users/
users/<id>/
users/<id>/roles/
roles/<id>/users

Preventing CRUD operations on organizations, teams, and users

Adds a class decorator to prevent modifying shared resources when the platform ingress is being used.

AWX_DIRECT_SHARED_RESOURCE_MANAGEMENT_ENABLED is the setting to enable/disable this feature.

Works by overriding these view methods:

• create
• delete
• perform_update

create and delete are overridden to raise a PermissionDenied exception.

perform_update is overridden to check if any shared fields are being modified, and raise a PermissionDenied exception if so.

Preventing modifying member_role and admin_role of organizations and teams

For this, we prevent any POST to the /api/v2/users/4/roles endpoint if

• AWX_DIRECT_SHARED_RESOURCE_MANAGEMENT_ENABLED is False
• role content type is organization or team
• role_field is member_role or admin_role

Preventing external auth

Don't register any of the external auth settings if AWX_DIRECT_SHARED_RESOURCE_MANAGEMENT_ENABLED is False.

Right now this leads to 404 in the UI if you hit these links

Todo

• extend role assignment to other role assignment endpoints, e.g. reverse of role-users-list

ISSUE TYPE

• New or Enhanced Feature

COMPONENT NAME

• API

AWX VERSION

awx: 24.3.1.dev33+ge2fab15d58

[TheRealHaoLiu]

did we check if this breaks the UI?

[fosterseth]

the external auth settings links are still present, and lead to 404s

[AlanCoding]

What about adding/removing users from organization/team members/admins?

[AlanCoding]

The setting in the eda-server patch is EDA_DIRECT_SHARED_RESOURCE_MANAGEMENT_ENABLED and the setting here is DIRECT_SHARED_RESOURCE_MANAGEMENT_ENABLED.

We will have a need to use either the same setting in DAB, or a different setting that does the same thing DAB-specific. I don't care what things are called as long as they are plausibly consistent in all the systems.

Given the current state of things, the only way this makes sense is to rename the setting here to AWX_DIRECT_SHARED_RESOURCE_MANAGEMENT_ENABLED. Then we could have ANSIBLE_BASE_DIRECT_SHARED_RESOURCE_MANAGEMENT_ENABLED. I don't love the verbosity of that, but I put a higher value on consistency.

[fosterseth]

went ahead and renamed the setting to AWX_DIRECT_SHARED_RESOURCE_MANAGEMENT_ENABLED

[AlanCoding]

Fairly strong preference to replace this line with instance = serializer.instance

https://github.com/encode/django-rest-framework/blob/36d5c0e74f562cbe3055f0d20818bd48d3c32359/rest_framework/mixins.py#L66-L68

If you read the origination of serializer here, you can see instance is already passed into it. The get_object method is very non-trivial and expensive and we should avoid re-calling.

[fosterseth]

okay I tried this and it gets tricky
instance = serializer.instance
this returns an instance populated with new data, not the original data. So I can't compare attrs[field] and instance.field

I can access _prior_values_store, but those dictionary keys don't match one to one with the shared_fields

for example, for a Team

 '_prior_values_store': {'description': 'sbf',
                         'name': 'teamA',
                         'organization_id': 1},

but the shared_field is called organization, not organization_id

I could hard-code organization to organization_id..but then this code path becomes less generalized.

do you have a suggestion for this? I do agree that re-running a get_object() is expensive

[AlanCoding]

instance = serializer.instance this returns an instance populated with new data

In what scenario does this happen? Digging back into DRF, I believe update is called for both PATCH and PUT, with the partial being True/False depending on which. That partial kwarg gets passed to the serializer, but in either case the serializer itself is passed instance for the existing instance.

[fosterseth]

sadly we are doing this..

awx/awx/api/serializers.py

Line 645 in 389a729

setattr(obj, k, v)

so in is_valid() we set these attributes

here is the drf update for our views

    def update(self, request, *args, **kwargs):
        partial = kwargs.pop('partial', False)
        instance = self.get_object()
        serializer = self.get_serializer(instance, data=request.data, partial=partial)
        serializer.is_valid(raise_exception=True)
        self.perform_update(serializer)

so by the time we get to perform_update our serializer.instance already has new values

[fosterseth]

kind of gross but we could considering something like

diff --git a/awx/api/views/__init__.py b/awx/api/views/__init__.py
index 4ed8f8b2f6..17829b3698 100644
--- a/awx/api/views/__init__.py
+++ b/awx/api/views/__init__.py
@@ -748,6 +748,15 @@ def immutablesharedfields(cls):
 
         cls.delete = delete_wrapper
 
+    cls.original_get_object = cls.get_object
+    def get_object_wrapper(*args, **kwargs):
+        view = args[0]
+        obj = cls.original_get_object(*args, **kwargs)
+        import copy
+        view._original_obj = copy.deepcopy(obj)
+        return obj
+    cls.get_object = get_object_wrapper
+
     if hasattr(cls, 'perform_update'):
         cls.original_perform_update = cls.perform_update
 
@@ -755,7 +764,7 @@ def immutablesharedfields(cls):
         def update_wrapper(*args, **kwargs):
             if not settings.AWX_DIRECT_SHARED_RESOURCE_MANAGEMENT_ENABLED:
                 view, serializer = args
-                instance = view.get_object()
+                instance = view._original_obj
                 if instance:
                     if isinstance(instance, models.Organization):
                         shared_fields = OrganizationType._declared_fields.keys()

[AlanCoding]

You're right, that's grosser than just calling .get_object, it's just hamstrung by our legacy serializer. Normal DRF doesn't have this problem of getting a diff, and it's just AWX that's unorthodox.

[AlanCoding]

Trying to wrap up this review, what's the status of adding validation for /api/v2/roles/:id/users/ and /api/v2/users/:id/roles/ sublists? Those need some added validation too right?

[AlanCoding]

Oh I see the RoleUsersList and its reverse now.