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 1 commit
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: 11 additions & 15 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,17 @@ 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 +55,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,14 +73,11 @@ 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
):
new_partial_perms[implied_perm] = partial_perms[
implied_perm
]
new_partial_perms[implied_perm] = partial_perms[implied_perm]

new_partial_perm = new_partial_perms[implied_perm]
# Trivial case, i.e.: permissions are built with front end.
Expand Down
121 changes: 53 additions & 68 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 All @@ -65,9 +64,7 @@ def create(self, validated_data):
user = validated_data['user']
asset = validated_data['asset']
if asset.owner_id == user.id:
raise serializers.ValidationError(
{'user': t(ASSIGN_OWNER_ERROR_MESSAGE)}
)
raise serializers.ValidationError({'user': t(ASSIGN_OWNER_ERROR_MESSAGE)})
Copy link
Contributor

Choose a reason for hiding this comment

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

It's going over the 80 characters limit (but below 90*). Did you do this in purpose or your linter is configured this way?

* Old Kobo Python coding convention discussion: We usually break at 80 but allow up to 90 when it's only for few characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It appears screwed up my local config to use default 88 when it was already set to 80. Using the formatter with 80 is OK right? I will update. Note the formatter greatly helps in the (soon to be) large permission dict (") and json (') that we need in docs and tests.

permission = validated_data['permission']
partial_permissions = validated_data.get('partial_permissions', None)

Expand All @@ -85,9 +82,7 @@ def get_label(self, object_permission):
except KeyError:
return object_permission.label
else:
return asset.get_label_for_permission(
object_permission.permission.codename
)
return asset.get_label_for_permission(object_permission.permission.codename)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as #4776 (comment)


def get_partial_permissions(self, object_permission):
codename = object_permission.permission.codename
Expand All @@ -99,26 +94,26 @@ 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})
Copy link
Contributor

Choose a reason for hiding this comment

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

#4776 (comment)

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,9 +140,7 @@ 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
Expand All @@ -166,12 +159,11 @@ 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
):
Copy link
Contributor

Choose a reason for hiding this comment

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

What about

        for (
            partial_permission,
            filters_,
        ) in self.__get_partial_permissions_generator(partial_permissions):

instead. It's Black compliant and does not go over the 80 characters limit.

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 +173,7 @@ 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 +240,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 +266,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 +293,6 @@ def to_representation(self, value):


class PermissionAssignmentSerializer(serializers.Serializer):

user = serializers.CharField()
permission = serializers.CharField()
partial_permissions = PartialPermissionField()
Expand All @@ -318,11 +309,13 @@ 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"""
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 really don't like when the code breaths a little :-P
I would leave the extra space alone or even better I would put the docstrings on separated lines.

"""
A more-explicit alternative to a simple tuple
"""

I prefer this way even if the sentence is short. Moreover if there is a diff at a later time, it does not involve the triple quotes.


user_pk: int
permission_codename: str
partial_permissions_json: Optional[str] = None
Expand Down Expand Up @@ -355,7 +348,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 @@ -392,9 +385,7 @@ def get_set_of_existing_assignments(
):
# Expand the stupid cache to include any users present in the
# existing assignments but not in the incoming assignments
user_pk_to_obj_cache[
assignment_in_db.user_id
] = assignment_in_db.user
user_pk_to_obj_cache[assignment_in_db.user_id] = assignment_in_db.user

if assignment_in_db.permission.codename == PERM_PARTIAL_SUBMISSIONS:
partial_permissions_json = json.dumps(
Expand Down Expand Up @@ -430,9 +421,9 @@ def get_set_of_incoming_assignments(
# always require a fully-fledged `User` object? Until then, keep a
# stupid object cache thing because `assign_perm()` and
# `remove_perm()` REQUIRE user objects
user_pk_to_obj_cache[
incoming_assignment['user'].pk
] = incoming_assignment['user']
user_pk_to_obj_cache[incoming_assignment['user'].pk] = incoming_assignment[
'user'
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as #4776 (comment)


# Expand to include implied permissions
for implied_codename in asset.get_implied_perms(
Expand All @@ -448,8 +439,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 All @@ -475,22 +465,18 @@ def validate(self, attrs):
# …for looking up by API permission URLs by codename
codename_to_url = dict()

assignable_permissions = self.context[
'asset'
].get_assignable_permissions(with_partial=True)
assignable_permissions = self.context['asset'].get_assignable_permissions(
with_partial=True
)

# Perhaps not the best error messages, but they're what DRF was already
# returning
INVALID_PERMISSION_ERROR = {
'permission': t('Invalid hyperlink - Object does not exist.')
}
INVALID_USER_ERROR = {
'user': t('Invalid hyperlink - Object does not exist.')
}
INVALID_USER_ERROR = {'user': t('Invalid hyperlink - Object does not exist.')}
# This matches the behavior of `AssetPermissionAssignmentSerializer`
INVALID_PARTIAL_PERMISSION_ERROR = {
'partial_permissions': t('Invalid `url`')
}
INVALID_PARTIAL_PERMISSION_ERROR = {'partial_permissions': t('Invalid `url`')}

# Fill in the dictionaries by parsing the incoming assignments
for assignment in attrs['assignments']:
Expand All @@ -503,16 +489,19 @@ 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']
)
if not (
partial_codename in assignable_permissions
and partial_codename.endswith(SUFFIX_SUBMISSIONS_PERMS)
):
raise serializers.ValidationError(
INVALID_PARTIAL_PERMISSION_ERROR
)
raise serializers.ValidationError(INVALID_PARTIAL_PERMISSION_ERROR)
codename_to_url[partial_codename] = partial_assignment['url']

# Create a dictionary of API user URLs to `User` objects
Expand All @@ -537,9 +526,7 @@ def validate(self, attrs):
if len(url_to_permission) != len(codename_to_url):
# This should never happen since all codenames were found within
# `assignable_permissions`
raise RuntimeError(
'Unexpected mismatch while processing permissions'
)
raise RuntimeError('Unexpected mismatch while processing permissions')

# Rewrite the incoming assignments, replacing user and permission URLs
# with their corresponding model instance objects
Expand All @@ -553,16 +540,14 @@ def validate(self, attrs):
assignment_with_objects['permission'].codename
== PERM_PARTIAL_SUBMISSIONS
):
assignment_with_objects['partial_permissions'] = defaultdict(
list
)
assignment_with_objects['partial_permissions'] = defaultdict(list)
for partial_assignment in assignment['partial_permissions']:
partial_codename = url_to_permission[
partial_assignment['url']
].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