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

Jump to bottom

Lint, test, fix mongo helper partial permissions #4776

Merged
merged 2 commits into from
Dec 21, 2023
Merged

Lint, test, fix mongo helper partial permissions #4776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dbc05cf
Run linter on mongo helper and partial perm files
bufke Dec 20, 2023
995a714
Adjust linting rules for consistency. Prefer pytest skip to skip tests.
bufke Dec 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 14 additions & 12 deletions kpi/models/asset_user_partial_permission.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
# coding: utf-8
from __future__ import annotations

from collections import defaultdict

from django.db import models
Expand Down Expand Up @@ -33,16 +35,21 @@ class AssetUserPartialPermission(models.Model):
class Meta:
unique_together = [['asset', 'user']]

asset = models.ForeignKey('Asset', related_name='asset_partial_permissions',
on_delete=models.CASCADE)
user = models.ForeignKey('auth.User', related_name='user_partial_permissions',
on_delete=models.CASCADE)
asset = models.ForeignKey(
'Asset',
related_name='asset_partial_permissions',
on_delete=models.CASCADE,
)
user = models.ForeignKey(
'auth.User',
related_name='user_partial_permissions',
on_delete=models.CASCADE,
)
permissions = models.JSONField(default=dict)
date_created = models.DateTimeField(default=timezone.now)
date_modified = models.DateTimeField(default=timezone.now)

def save(self, *args, **kwargs):

if self.pk is not None:
self.date_modified = timezone.now()

Expand All @@ -52,20 +59,16 @@ def save(self, *args, **kwargs):
def update_partial_perms_to_include_implied(
asset: 'kpi.models.Asset', partial_perms: dict
) -> dict:
new_partial_perms = defaultdict(list)
new_partial_perms = defaultdict(list, partial_perms)
in_op = MongoHelper.IN_OPERATOR

for partial_perm, filters in partial_perms.items():

if partial_perm not in new_partial_perms:
new_partial_perms[partial_perm] = filters

# TODO: omit `add_submissions`? It's required at the asset
# level for any kind of editing (e.g. partial
# `change_submissions` requires asset-wide `add_submissions`),
# but it doesn't make sense to restrict adding submissions
# "only to those submissions that match some criteria"
implied_perms = [
implied_perms: list[str] = [
implied_perm
for implied_perm in asset.get_implied_perms(
partial_perm, for_instance=asset
Expand All @@ -74,7 +77,6 @@ def update_partial_perms_to_include_implied(
]

for implied_perm in implied_perms:

if (
implied_perm not in new_partial_perms
and implied_perm in partial_perms
Expand Down
88 changes: 49 additions & 39 deletions kpi/serializers/v2/asset_permission_assignment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
# coding: utf-8
from __future__ import annotations

import json
from collections import defaultdict
from dataclasses import dataclass
from typing import Optional

from django.db import transaction
from django.contrib.auth.models import Permission, User
from django.db import transaction
from django.urls import Resolver404
from django.utils.translation import gettext as t
from rest_framework import serializers
Expand All @@ -27,24 +28,22 @@
)
from kpi.utils.urls import absolute_resolve


ASSIGN_OWNER_ERROR_MESSAGE = "Owner's permissions cannot be assigned explicitly"


class AssetPermissionAssignmentSerializer(serializers.ModelSerializer):

Copy link
Contributor

Choose a reason for hiding this comment

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

💔 . Your linter is going to be crazy on all my code. I do add an extra line in purpose.
IMHO, It's more compliant with separated blocks of docstring and variables declaration.

Copy link
Contributor

Choose a reason for hiding this comment

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

It happens several times in this PR. I would keep the extra line at the beginning of classes (and methods).

url = serializers.SerializerMethodField()
user = RelativePrefixHyperlinkedRelatedField(
view_name='user-detail',
lookup_field='username',
queryset=User.objects.all(),
style={'base_template': 'input.html'} # Render as a simple text box
style={'base_template': 'input.html'}, # Render as a simple text box
)
permission = RelativePrefixHyperlinkedRelatedField(
view_name='permission-detail',
lookup_field='codename',
queryset=Permission.objects.all(),
style={'base_template': 'input.html'} # Render as a simple text box
style={'base_template': 'input.html'}, # Render as a simple text box
)
partial_permissions = serializers.SerializerMethodField()
label = serializers.SerializerMethodField()
Expand Down Expand Up @@ -99,26 +98,28 @@ def get_partial_permissions(self, object_permission):
# TODO: optimize `asset.get_partial_perms()` so it doesn't execute
# a new query for each assignment
partial_perms = asset.get_partial_perms(
object_permission.user_id, with_filters=True)
object_permission.user_id, with_filters=True
)
if not partial_perms:
return None

if partial_perms:
hyperlinked_partial_perms = []
for perm_codename, filters in partial_perms.items():
url = self.__get_permission_hyperlink(perm_codename)
hyperlinked_partial_perms.append({
'url': url,
'filters': filters
})
hyperlinked_partial_perms.append(
{'url': url, 'filters': filters}
)
return hyperlinked_partial_perms
return None

def get_url(self, object_permission):
asset_uid = self.context.get('asset_uid')
return reverse('asset-permission-assignment-detail',
args=(asset_uid, object_permission.uid),
request=self.context.get('request', None))
return reverse(
'asset-permission-assignment-detail',
args=(asset_uid, object_permission.uid),
request=self.context.get('request', None),
)

def validate(self, attrs):
# Because `partial_permissions` is a `SerializerMethodField`,
Expand All @@ -145,16 +146,16 @@ def validate_partial_permissions(self, attrs):
return attrs

def _invalid_partial_permissions(message):
raise serializers.ValidationError(
{'partial_permissions': message}
)
raise serializers.ValidationError({'partial_permissions': message})

request = self.context['request']
partial_permissions = None

if isinstance(request.data, dict): # for a single assignment
partial_permissions = request.data.get('partial_permissions')
elif self.context.get('partial_permissions'): # injected during bulk assignment
elif self.context.get(
'partial_permissions'
): # injected during bulk assignment
partial_permissions = self.context.get('partial_permissions')

if not partial_permissions:
Expand All @@ -166,12 +167,12 @@ def _invalid_partial_permissions(message):

partial_permissions_attr = defaultdict(list)

for partial_permission, filters_ in \
self.__get_partial_permissions_generator(partial_permissions):
for (
partial_permission,
filters_,
) in self.__get_partial_permissions_generator(partial_permissions):
try:
resolver_match = absolute_resolve(
partial_permission.get('url')
)
resolver_match = absolute_resolve(partial_permission.get('url'))
except (TypeError, Resolver404):
_invalid_partial_permissions(t('Invalid `url`'))

Expand All @@ -181,8 +182,9 @@ def _invalid_partial_permissions(message):
_invalid_partial_permissions(t('Invalid `url`'))

# Permission must valid and must be assignable.
if not self._validate_permission(codename,
SUFFIX_SUBMISSIONS_PERMS):
if not self._validate_permission(
codename, SUFFIX_SUBMISSIONS_PERMS
):
_invalid_partial_permissions(t('Invalid `url`'))

# No need to validate Mongo syntax, query will fail
Expand Down Expand Up @@ -249,11 +251,11 @@ def _validate_permission(self, codename, suffix=None):
"""
return (
# DONOTMERGE abusive to the database server?
codename in Asset.objects.only('asset_type').get(
uid=self.context['asset_uid']
).get_assignable_permissions(
with_partial=True
) and (suffix is None or codename.endswith(suffix))
codename
in Asset.objects.only('asset_type')
.get(uid=self.context['asset_uid'])
.get_assignable_permissions(with_partial=True)
and (suffix is None or codename.endswith(suffix))
)

def __get_partial_permissions_generator(self, partial_permissions):
Expand All @@ -275,13 +277,14 @@ def __get_permission_hyperlink(self, codename):
:param codename: str
:return: str. url
"""
return reverse('permission-detail',
args=(codename,),
request=self.context.get('request', None))
return reverse(
'permission-detail',
args=(codename,),
request=self.context.get('request', None),
)


class PartialPermissionField(serializers.Field):

Copy link
Contributor

Choose a reason for hiding this comment

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

💔 again.

default_error_messages = {
'invalid': t('Not a valid list.'),
'blank': t('This field may not be blank.'),
Expand All @@ -301,7 +304,6 @@ def to_representation(self, value):


class PermissionAssignmentSerializer(serializers.Serializer):

user = serializers.CharField()
permission = serializers.CharField()
partial_permissions = PartialPermissionField()
Expand All @@ -318,11 +320,15 @@ class AssetBulkInsertPermissionSerializer(serializers.Serializer):
Warning: If less queries are sent to DB, it consumes more CPU and memory.
The bigger the assignments are, the bigger the resources footprint will be.
"""

assignments = serializers.ListField(child=PermissionAssignmentSerializer())

@dataclass(frozen=True)
class PermissionAssignment:
""" A more-explicit alternative to a simple tuple """
"""
A more-explicit alternative to a simple tuple
"""

user_pk: int
permission_codename: str
partial_permissions_json: Optional[str] = None
Expand Down Expand Up @@ -355,7 +361,7 @@ def create(self, validated_data):
partial_perms = json.loads(addition.partial_permissions_json)
else:
partial_perms = None
perm = asset.assign_perm(
asset.assign_perm(
user_obj=user_pk_to_obj_cache[addition.user_pk],
perm=addition.permission_codename,
partial_perms=partial_perms,
Expand Down Expand Up @@ -448,8 +454,7 @@ def get_set_of_incoming_assignments(
# Expand to include implied partial permissions
if incoming_permission.codename == PERM_PARTIAL_SUBMISSIONS:
partial_permissions = json.dumps(
AssetUserPartialPermission\
.update_partial_perms_to_include_implied(
AssetUserPartialPermission.update_partial_perms_to_include_implied(
asset, incoming_assignment['partial_permissions']
),
sort_keys=True,
Expand Down Expand Up @@ -503,6 +508,11 @@ def validate(self, attrs):
username = self._get_arg_from_url('username', user_url)
username_to_url[username] = user_url
for partial_assignment in assignment.get('partial_permissions', []):
if 'filters' not in partial_assignment:
# Instead of this, we should validate using DRF
raise serializers.ValidationError(
'Permission assignment must contain filters'
)
partial_codename = self._get_arg_from_url(
'codename', partial_assignment['url']
)
Expand Down Expand Up @@ -562,7 +572,7 @@ def validate(self, attrs):
].codename
assignment_with_objects['partial_permissions'][
partial_codename
] = partial_assignment['filters']
] = partial_assignment.get('filters')
assignments_with_objects.append(assignment_with_objects)

attrs['assignments'] = assignments_with_objects
Expand Down