WIP: Permission management #216

cutwater · 2021-12-13T14:24:32Z

No description provided.

mkanoor · 2021-12-13T19:04:39Z

ansible_catalog/main/catalog/views.py

+        "list": {"type": "queryset", "permission": "read"},
+        "create": {"type": "wildcard", "permission": "create"},
+        "retrieve": {"type": "object", "permission": "read"},
+        "update": {"type": "object", "permission": "edit"},


@cutwater I think these 'edit' should be 'update'

mkanoor · 2021-12-13T19:23:09Z

ansible_catalog/common/auth/keycloak_django/permissions.py

+            scope=scope,
+        )
+        object_permission = models.AuthzPermission(
+            resource=make_resource_name(view.keycloak_resource_type, obj.pk),


@cutwater This line raises an exception
Exception Value: | 'Job' object has no attribute 'pk'

The response from Share is a Job object which is going thru the permission model.

The response doesn't go thru permission model. Can you please give more details on a request that ended up with this error.

mkanoor · 2021-12-13T19:33:03Z

ansible_catalog/main/catalog/views.py

    ordering = ("-id",)
    filterset_fields = ("name", "description", "created_at", "updated_at")
    search_fields = ("name", "description")

+    keycloak_resource_type = "catalog:portfolio"


@cutwater The PortfolioItem inherits all of its permission from the Portfolio object, so for PortfolioItem would be keycloak_resource_type be set to "catalog:portfolio" or would it be ansible_catalog.main.catalog.models.Portfolio?

For PortfolioItem the idea was to use keycloak_resource_type = "catalog:portfolio and override check_object_permissions method for the PortfolioItemViewSet:

class PortfolioItemViewSet(...): keycloak_resource_type = "catalog:portfolio" keycloak_access_policies = [...] def check_object_permissions(self, request, obj): return super().check_object_permissions(self, request, obj.portfolio)

But this will not work for list views. As they must check access for the individual portfolio as well.

mkanoor · 2021-12-13T20:07:52Z

ansible_catalog/common/auth/keycloak_django/permissions.py

+            scope=scope,
+        )
+        object_permission = models.AuthzPermission(
+            resource=make_resource_name(view.keycloak_resource_type, obj.pk),


@cutwater instead of obj.pk we should be able to specify the attribute name to use. If the PermissionAttribute is not defined we can use pk. This relates to how we model Parent/Child Permissions.

Portfolio is the parent of PortfolioItem

Class PortfolioItem:
PermissionClass: Portfolio
PermissionAttribute: portfolio_id
KeycloakAccessPolicies: #we should be able to override the parents access policies

Class Portfolio:
PermissionClass: undefined defaults to class name
PermissionAttribute: undefined defaults to pk
KeycloakResourceType: catalog:portfolios
KeycloakAccessPolicies: ....

In the query set we have model class name as either Portfolio or PortfolioItem access_token = request.keycloak_user.extra_data["access_token"] client = get_authz_client(access_token) permissions = client.get_permissions( models.AuthzPermission( scope=make_scope_name(PermissionClass.KeycloakResourceType, permission)) ) keycloak_ids = ... collect resource ids from permissions.... key = f"{PermissionAttribute}__in" <<model_class_name>>.objects.filter(key=keycloak_ids) So for portfolio items the call would look like PortfolioItem.objects.filter(portfolio_id__in=keycloak_ids) For Portfolio the call would look like Portfolio.objects.filter(pk__in=keycloak_ids)

The permission class is different from the Model class

Portfolio is the parent of PortfolioItem

Not really. They share one-to-many relationship in terms of models.

PortfolioItem.objects.filter(portfolio_id__in=keycloak_ids)

If user has access to Portfolio, he has access to all portfolio items, doesn't he?

@cutwater We have an endpoint called /portfolio_items/ which lists all the visible PortfolioItem which is different from /portfolio/10/portfolio_items. And there are other direct access to portfolio_items which would have to know about the permissions on the Portfolio before granting access to users.

Makes sense. Thank you.

mkanoor · 2021-12-13T20:10:12Z

ansible_catalog/common/auth/keycloak_django/permissions.py

+            # FIXME(cutwater): Support for non integer primary keys needed
+            resource_ids.append(int(resource_id))
+
+        return qs.filter(pk__in=resource_ids)


@cutwater The pk__in should be customizable by the model class attributes.

mkanoor · 2021-12-13T20:28:35Z

ansible_catalog/common/auth/keycloak_django/permissions.py

+
+class KeycloakPermission(BasePermission):
+    def has_permission(self, request: Request, view: Any):
+        if view.action is None and request.method in SAFE_METHODS:


@cutwater The SAFE_METHODS have GET in them so should we be allowing a GET call to return True if there is no action, shouldn't it be False.

This is needed mainly to support DRF generated html representations of the API. I think we may limit it to DEBUG=True.

mkanoor · 2021-12-16T20:57:43Z

ansible_catalog/common/auth/keycloak_django/permissions.py

+            scope=scope,
+        )
+        object_permission = models.AuthzPermission(
+            resource=make_resource_name(resource_type, obj.pk),


@cutwater Shouldn't this be portfolio_id when checking for PortfolioItem permissions. The attribute name would have to be either an attribute or it would have to be passed in. If we can define these as class level attributes it would simplify the code.

It is already a portfolio_id, as portfolio object is passed to check_object_permissions() in the PortfolioItemViewSet, however I want to split this class into two and handle nestedviewsets in a separate permission class.

mkanoor · 2021-12-16T21:00:26Z

ansible_catalog/main/catalog/views.py

+        "update": {"type": "object", "permission": "update"},
+        "destroy": {"type": "object", "permission": "update"},
+    }
+


@cutwater If we can define the attribute name to use here. It might simplify the code it can be used in single object permission check as well as in the query set calls.

mkanoor · 2021-12-17T19:38:06Z

ansible_catalog/common/auth/keycloak_django/permissions.py

+    def _get_policy(view):
+        if view.action is None:
+            return None
+        return view.get_keycloak_access_policies().get(view.action)


@cutwater Should there be a check here if the action is not specified in the access_policies. So a developer could add it in if its missing.

At the moment the default behavior is to deny access or return an empty queryset for a policy that is missing. How would you like to change that behavior?

mkanoor · 2021-12-17T19:40:17Z

ansible_catalog/common/auth/keycloak_django/permissions.py

+        if policy is None:
+            return False
+        if policy["type"] != OBJECT_PERMISSION:
+            return True


@cutwater Should this be returning False instead of True. Is this to prevent misconfigured access policies from giving access to object.

No, it shouldn't. This is to apply the policy only for a specific type.

mkanoor · 2021-12-17T19:55:51Z

ansible_catalog/common/auth/keycloak_django/views.py

@@ -0,0 +1,7 @@
+class KeycloakPermissionMixin:
+
+    keycloak_resource_name = None


@cutwater Should this be keycloak_resource_type instead of keycloak_resource_name?

Yes, indeed.

mkanoor · 2021-12-18T16:42:59Z

ansible_catalog/main/catalog/views.py

+        "destroy": {"type": "object", "permission": "delete"},
+        "share": {"type": "object", "permission": "update"},
+        "unshare": {"type": "object", "permission": "update"},
+        "share_info": {"type": "object", "permission": "update"},


@cutwater I think the permission should be an array instead of a single value so we can explicitly state
share: :"type": "object", "permissions": ["read", "update"]

Otherwise the external package has to know that update means you have read permission. We are missing the copy action here, it order to copy you need read permission on the portfolio being copied and create permission to create the new portfolio object if you don't have either of these permissions it should get rejected.

"copy" : {"type": "object", permission: ["read","create"]}

I think the permission should be an array instead of a single value so we can explicitly state
share: :"type": "object", "permissions": ["read", "update"]

Can you please provide an example where update permission does not imply read permission?

Otherwise the external package has to know that update means you have read permission.

What external "package"?

@cutwater I mean your code that is in ansible_catalog/common/auth/keycloak/authz.py at some point it would have to become an external package to be shared. The get_object call needs a "read" permission, if we don't list the read permission we are tying permissions together (update => read + update)

For the copy action we need read permission on the object we are trying to copy and then we need a create permission.

"copy" : {"type": "object", permission: ["read","create"]}

mkanoor · 2021-12-18T16:44:40Z

ansible_catalog/main/catalog/views.py

+    def get_keycloak_access_policies(self):
+        if "portfolio_id" in self.kwargs:
+            return {
+                "list": {"type": "object", "permission": "read"},


@cutwater wouldn't this be resolved if permissions is an array and for object as well as query set we added "read"

Nope, because the permission is the same, the stage when permission check is applied is different.

mkanoor · 2021-12-18T17:28:40Z

ansible_catalog/main/catalog/views.py

@@ -313,6 +336,46 @@ class PortfolioItemViewSet(
    search_fields = ("name", "description")
    parent_field_names = ("portfolio",)

+    keycloak_resource_type = "catalog:portfolio"
+    keycloak_access_policies = {
+        "create": {"type": "object", "permission": "update"},


@cutwater Should this be using the constants defined

WILDCARD_PERMISSION = "wildcard" OBJECT_PERMISSION = "object" QUERYSET_PERMISSION = "queryset"

mkanoor · 2021-12-18T17:30:36Z

ansible_catalog/main/catalog/views.py

+                **self.keycloak_access_policies,
+            }
+
+    def check_permissions(self, request):


@cutwater Just an FYI chibisov/drf-extensions#142

mkanoor · 2021-12-18T17:40:05Z

ansible_catalog/main/catalog/views.py

+        "retrieve": {"type": "object", "permission": "read"},
+        "update": {"type": "object", "permission": "update"},
+        "destroy": {"type": "object", "permission": "update"},
+    }


@cutwater Don't we need a list action here since we can have /portfolio_items or /portfolio/nnn/portfolio_items/ we tweak the keycloak_access_polices based on the parent_id in the self.kwargs wouldn't it be better to have the list action be defined here. If we just fetched the parent object wouldn't that trigger its permission check to run automatically when its get_object is called. For nested routes we could just call the get_object on the parent if it exists. We are going to encounter this for order_items and progress_messages. /orders/nnn/order_items, /order_items/nnn/progress_messages

The base get_object call does a permission check
https://github.com/encode/django-rest-framework/blob/45082b39368729caa70534dde11b0788ef186a37/rest_framework/generics.py#L99

We can move all of the code that figures out the parentage call into a mixin and use a generic logic like here
https://github.com/chibisov/drf-extensions/blob/ecdf3a95d7f18ccf9cffa55809635c3715179605/rest_framework_extensions/mixins.py#L70

and make a get_object call on all the parent objects in the URL path and the get_object will call the appropriate permission checks. The get_object is defined in a View not sure how we would create a parent view to call the get_object. Can our keycloak_access_policies be moved to the model instead of views?

We have added this mixin for nested path which works for scenarios with and without parent_ids.
https://github.com/ansible/ansible-catalog/blob/1d7baa94e3d83f22d205c010b80da4fcf2a8de31/ansible_catalog/common/queryset_mixin.py#L26

For starters we can add the get_object call in this code and have it validate the permissions and this will work for all our uses cases of orders, order_items, progress messages etc.

@cutwater we can get the parent object here and check the permissions
https://github.com/ansible/ansible-catalog/blob/1d7baa94e3d83f22d205c010b80da4fcf2a8de31/ansible_catalog/common/queryset_mixin.py#L25

related_descriptor= getattr(queryset.model, parent_lookup_key) related_model= related_descriptor.field.related_model parent_obj = related_model.objects.get(pk=self.kwargs[parent_lookup_key]) ... check permissions on parent object...

Can our keycloak_access_policies be moved to the model instead of views?

Nope

mkanoor · 2021-12-19T22:52:51Z

ansible_catalog/main/catalog/views.py

    ordering = ("-id",)
    filterset_fields = ("name", "description", "created_at", "updated_at")
    search_fields = ("name", "description")

+    keycloak_resource_type = "catalog:portfolio"
+    keycloak_access_policies = {
+        "list": {"type": "queryset", "permission": "read"},


@cutwater The type seems to be redundant can we do that based on the action. If we have 2 special actions list and create this dictionaries becomes much simpler. List is the only time we need to call query_set otherwise it is a retrieve on a single object.

Some ViewSets require queryset filtering (Actually only Portfolio and related ones), some of them don't as only wildcard check is performed.

It is not true for custom actions, some actions have detail=True and some of them don't.

It is very different for nested query sets.

I was thinking by determining type of policy based on view.detail property, but at the moment my preference is to be explicit on the policy type.

mkanoor · 2021-12-19T22:58:00Z

ansible_catalog/common/auth/keycloak_django/permissions.py

+        request: Request,
+        view: Any,
+        qs: QuerySet,
+        filter_field: str = "pk",


@cutwater If we can use something like this
https://github.com/encode/django-rest-framework/blob/45082b39368729caa70534dde11b0788ef186a37/rest_framework/generics.py#L39

For portfolio we use
keycloak_lookup_field: pk
for portfolio_item we use
keycloak_lookup_field: portfolio_id

Then we don't have to pass the attribute name around it can be defined in the KeycloakPermissionMixin and be overriden by each class.

mkanoor · 2021-12-19T22:59:24Z

ansible_catalog/common/auth/keycloak_django/views.py

@@ -0,0 +1,7 @@
+class KeycloakPermissionMixin:
+


@cutwater we could add the keycloak_lookup_field here and default it to pk
PortfolioItem would override it to portfolio_id

mkanoor · 2021-12-21T16:03:36Z

ansible_catalog/common/auth/keycloak_django/permissions.py

+            obj = get_object_or_404(
+                parent_model, **{lookup_field: lookup_value}
+            )
+            return _check_resource_permission(


@cutwater Should this raise an exception exceptions.PermissionDenied instead of doing a return, because here we are only checking the permission on the parent. We might have to check permission on parent as well as the current object.

Not sure I'm following the issue here. Can you please elaborate?
The has_permission and has_object_permission methods return a boolean. By default if a permission check call returns True, the subsequent permission checks are executed, otherwise the permission denied response is returned to the client.

mkanoor · 2021-12-21T19:15:48Z

ansible_catalog/main/catalog/views.py

+        "list": KeycloakPolicy("read", KeycloakPolicyType.QUERYSET),
+        "share": KeycloakPolicy("update", KeycloakPolicyType.OBJECT),
+        "unshare": KeycloakPolicy("update", KeycloakPolicyType.OBJECT),
+        "share_info": KeycloakPolicy("update", KeycloakPolicyType.OBJECT),


@cutwater we are missing copy here with ["read","create"] permissions

mkanoor · 2021-12-21T19:21:23Z

ansible_catalog/main/catalog/views.py

+        "retrieve": KeycloakPolicy("read", KeycloakPolicyType.OBJECT),
+        "create": KeycloakPolicy("update", KeycloakPolicyType.OBJECT),
+        "update": KeycloakPolicy("update", KeycloakPolicyType.OBJECT),
+        "destroy": KeycloakPolicy("update", KeycloakPolicyType.OBJECT),


@cutwater We are missing copy action here with ["read","update"] permissions

mkanoor · 2021-12-21T19:28:41Z

ansible_catalog/main/catalog/views.py

+    }
+
+    def get_keycloak_access_policies(self):
+        if self.keycloak_lookup_field in self.kwargs:


@cutwater This function is confusing. Do we need this now since we are checking for parent object in has_object_permission and in has_permissions?

mkanoor reviewed Dec 13, 2021

View reviewed changes

mkanoor reviewed Dec 16, 2021

View reviewed changes

cutwater added 2 commits December 17, 2021 16:55

WIP: Permission management

9ba7025

WIP: Nested views permission checks

ce56b90

mkanoor reviewed Dec 17, 2021

View reviewed changes

mkanoor reviewed Dec 18, 2021

View reviewed changes

mkanoor reviewed Dec 19, 2021

View reviewed changes

cutwater added 3 commits December 21, 2021 13:24

WIP: Fix mixin class attribute name

eb93124

WIP: Move util functions outside of the permission class

7767a5e

WIP: Continue refactoring

bf8e1be

mkanoor reviewed Dec 21, 2021

View reviewed changes

WIP: Continue

5805953

cutwater closed this Jan 31, 2022

cutwater deleted the feature/permissions branch March 10, 2022 10:02

		@@ -0,0 +1,7 @@
		class KeycloakPermissionMixin:

		keycloak_resource_name = None

WIP: Permission management #216

WIP: Permission management #216

Conversation

cutwater commented Dec 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkanoor Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkanoor Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkanoor Dec 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkanoor Dec 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkanoor Dec 13, 2021 •

edited

Loading

mkanoor Dec 14, 2021 •

edited

Loading

mkanoor Dec 18, 2021 •

edited

Loading

mkanoor Dec 18, 2021 •

edited

Loading