Skip to content

Commit

Permalink
feat: check permissions only if ?include_perms
Browse files Browse the repository at this point in the history
* `can_<action>` field will be None unless ?include_perms requested
* adds tests to verify query counts are unchanged when perms requested
  • Loading branch information
pomegranited committed Jan 11, 2024
1 parent 2709d44 commit 7470904
Show file tree
Hide file tree
Showing 4 changed files with 174 additions and 64 deletions.
34 changes: 26 additions & 8 deletions openedx_tagging/core/tagging/rest_api/utils.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"""
Utilities for the API
"""
from typing import Type
from typing import Optional, Type

from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication # type: ignore[import]
from edx_rest_framework_extensions.auth.session.authentication import ( # type: ignore[import]
Expand Down Expand Up @@ -45,13 +45,31 @@ def _request(self) -> Request:
"""
raise NotImplementedError # pragma: no cover

def _can(self, action: str, instance=None) -> bool:
@property
def _include_perms(self) -> bool:
"""
Returns True if the current `request.user` may perform the given `action` on the `instance` object.
Are permission checks requested?
Returns True if ?include_perms found in the query string
Returns False by default.
"""
return 'include_perms' in self._request.query_params

def _can(self, action: str, instance=None) -> Optional[bool]:
"""
Can the current `request.user` perform the given `action` on the `instance` object?
Returns None if no permissions were requested.
Returns True if they may.
Returns False if they may not.
"""
assert action in ("add", "view", "change", "delete")
request = self._request
assert request and request.user
if not self._include_perms:
return None

assert action in ("add", "view", "change", "delete")

model = self._model
assert model

Expand All @@ -60,27 +78,27 @@ def _can(self, action: str, instance=None) -> bool:
perm_name = f'{app_label}.{action}_{model_name}'
return request.user.has_perm(perm_name, instance)

def get_can_add(self, _instance=None) -> bool:
def get_can_add(self, _instance=None) -> Optional[bool]:
"""
Returns True if the current user is allowed to add new instances.
Note: we omit the actual instance from the permissions check; most tagging models prefer this.
"""
return self._can('add')

def get_can_view(self, instance) -> bool:
def get_can_view(self, instance) -> Optional[bool]:
"""
Returns True if the current user is allowed to view/see this instance.
"""
return self._can('view', instance)

def get_can_change(self, instance) -> bool:
def get_can_change(self, instance) -> Optional[bool]:
"""
Returns True if the current user is allowed to edit/change this instance.
"""
return self._can('change', instance)

def get_can_delete(self, instance) -> bool:
def get_can_delete(self, instance) -> Optional[bool]:
"""
Returns True if the current user is allowed to delete this instance.
"""
Expand Down
12 changes: 7 additions & 5 deletions openedx_tagging/core/tagging/rest_api/v1/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"""
from __future__ import annotations

from typing import Any, Type
from typing import Any, Optional, Type
from urllib.parse import urlencode

from rest_framework import serializers
Expand Down Expand Up @@ -47,6 +47,8 @@ class UserPermissionsSerializerMixin(UserPermissionsHelper):
Notes:
* Assumes the serialized model should be used to check permissions (override _model to change).
* Requires the current request to be passed into the serializer context (override _request to change).
* Requires '?include_perms` in the request, otherwise no permission checks are run, and
the `can_<action>` fields return None.
"""
@property
def _model(self) -> Type:
Expand Down Expand Up @@ -235,25 +237,25 @@ def _model(self) -> Type:
"""
return Tag

def get_can_change(self, instance) -> bool:
def get_can_change(self, instance) -> Optional[bool]:
"""
Returns True if the current user is allowed to edit/change this Tag instance.
Returns False for all TagData instances.
"""
if isinstance(instance, Tag):
return super()._can('change', instance)
return False
return None

def get_can_delete(self, instance) -> bool:
def get_can_delete(self, instance) -> Optional[bool]:
"""
Returns True if the current user is allowed to delete this Tag instance.
Returns False for all TagData instances.
"""
if isinstance(instance, Tag):
return super()._can('delete', instance)
return False
return None

def to_representation(self, instance: TagData | Tag) -> dict:
"""
Expand Down
43 changes: 41 additions & 2 deletions openedx_tagging/core/tagging/rest_api/v1/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -366,8 +366,6 @@ class ObjectTagView(
**Retrieve Parameters**
* object_id (required): - The Object ID to retrieve ObjectTags for.
**Retrieve Query Parameters**
* taxonomy (optional) - PK of taxonomy to filter ObjectTags for.
**Retrieve Example Requests**
Expand All @@ -379,6 +377,28 @@ class ObjectTagView(
* 400 - Invalid query parameter
* 403 - Permission denied
Response:
{
${object_id}: {
taxonomies: [
{
taxonomy_id: str,
can_tag_object: bool,
tags: [
{
value: str,
lineage: list[str],
can_change: bool, // TODO: when/will free text tags are editable
can_delete: bool,
},
]
},
...
],
},
...
}
**Update Parameters**
* object_id (required): - The Object ID to add ObjectTags for.
Expand Down Expand Up @@ -587,6 +607,7 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView):
Using full_depth_threshold=1000 is recommended in general, but use lower values during development to ensure
compatibility with both large and small result sizes.
* include_counts (optional) - Include the count of how many times each tag has been used.
* include_perms (optional) - Run permission checks (can_change, can_delete) and include in response.
* page (optional) - Page number (default: 1)
* page_size (optional) - Number of items per page (default: 30). Ignored when there are fewer tags than
specified by ?full_depth_threshold.
Expand All @@ -601,6 +622,24 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView):
* 403 - Permission denied
* 404 - Taxonomy not found
Response:
[
{
value: str,
external_id: str,
child_count: int,
depth: int,
parent_value: str,
usage_count: int,
_id: int,
sub_tags_url: str,
can_change: bool|null,
can_delete: bool|null,
},
...
],
**Create Query Parameters**
* id (required) - The ID of the taxonomy to create a Tag for
Expand Down
Loading

0 comments on commit 7470904

Please sign in to comment.