From 054a45e649ed3463e199d5125d13d81a257a2052 Mon Sep 17 00:00:00 2001 From: "John N. Milner" Date: Tue, 12 Jan 2021 02:07:22 -0500 Subject: [PATCH 1/3] Add new permission for managing sharing * Search out and destroy any code that still allowed editors to change permission assignments * Consistently enforce the rule that users with explicitly-assigned permissions see only their own permissions and the owner's permissions, not other users' permissions * Add new `manage_asset` permission that allows non-owners to change the permissions of assets; managing sharing of collections remains the purview of the owner only (until #2580) * Add `manage_asset` to the UI * Remove obsolete `share_asset` and `share_submissions` permissions * Remove obsolete `editors_can_change_permissions` fields from the database * Allow the asset type label to vary based on the permission, i.e. `survey` is usually labeled 'form', but it is called 'project' for `manage_asset` * Add rudimenary filtering of assignable permissions by asset type; this needs work and currently hamstrings the front end's `PermValidator` * Expand the back-end permissions test suite, e.g. by fixing the API v2 copy of `ApiAssignedPermissionsTestCase` * Grant the new `manage_asset` permission to all existing owners using a data migration. This isn't strictly needed so long as we short-circuit permission checks for owners, but we might not do that always (and it's good hygiene) * Remove the sad remnants of my sacred cow --- jsapp/js/actions.es6 | 3 + .../js/components/permissions/permParser.es6 | 8 + .../components/permissions/permValidator.es6 | 5 + .../permissions/permissionsMocks.es6 | 19 +- .../permissions/userAssetPermsEditor.es6 | 35 ++ jsapp/js/constants.es6 | 1 + kpi/constants.py | 4 +- kpi/filters.py | 38 ++- kpi/fixtures/conflicting_versions.json | 1 - kpi/fixtures/test_data.json | 8 +- .../0027_add_manage_asset_permission.py | 57 ++++ .../0028_assign_manage_asset_permissions.py | 74 +++++ kpi/models/asset.py | 68 ++-- kpi/models/collection.py | 12 +- kpi/models/object_permission.py | 106 ++++--- kpi/permissions.py | 11 +- kpi/tests/api/v1/test_api_permissions.py | 25 +- .../test_api_asset_permission_assignment.py | 287 +++++++++++++---- kpi/tests/api/v2/test_api_assets.py | 8 +- ...st_api_collection_permission_assignment.py | 11 +- kpi/tests/api/v2/test_api_permissions.py | 299 ++++++++++-------- kpi/tests/base_test_case.py | 4 + kpi/tests/kpi_test_case.py | 38 ++- kpi/tests/test_assets.py | 28 +- kpi/tests/test_collections.py | 31 -- kpi/tests/test_permissions.py | 84 ++--- kpi/utils/object_permission_helper.py | 26 +- kpi/views/v1/asset.py | 4 +- kpi/views/v1/object_permission.py | 18 +- kpi/views/v2/asset_permission_assignment.py | 6 +- .../v2/collection_permission_assignment.py | 5 +- 31 files changed, 879 insertions(+), 445 deletions(-) create mode 100644 kpi/migrations/0027_add_manage_asset_permission.py create mode 100644 kpi/migrations/0028_assign_manage_asset_permissions.py diff --git a/jsapp/js/actions.es6 b/jsapp/js/actions.es6 index 5ccdf57dd4..a904b0f12c 100644 --- a/jsapp/js/actions.es6 +++ b/jsapp/js/actions.es6 @@ -255,6 +255,9 @@ permissionsActions.assignAssetPermission.failed.listen(() => { permissionsActions.removeAssetPermission.failed.listen(() => { notify(t('Failed to remove permissions'), 'error'); }); +permissionsActions.bulkSetAssetPermissions.failed.listen(() => { + notify(t('Failed to update permissions'), 'error'); +}); permissionsActions.assignCollectionPermission.failed.listen(() => { notify(t('Failed to update permissions'), 'error'); }); diff --git a/jsapp/js/components/permissions/permParser.es6 b/jsapp/js/components/permissions/permParser.es6 index 4bfa9b5e38..31d219788b 100644 --- a/jsapp/js/components/permissions/permParser.es6 +++ b/jsapp/js/components/permissions/permParser.es6 @@ -19,6 +19,7 @@ import { * @property {string} data.username - Who give permissions to. * @property {boolean} data.formView - Is able to view forms. * @property {boolean} data.formEdit - Is able to edit forms. + * @property {boolean} data.formManage - Is able to change form permissions (and future stuff TBD). * @property {boolean} data.submissionsView - Is able to view submissions. * @property {boolean} data.submissionsViewPartial - If true, then able to view submissions only of some users. * @property {string[]} data.submissionsViewPartialUsers - Users mentioned in the above line. @@ -60,6 +61,10 @@ function parseFormData(data) { parsed.push(buildBackendPerm(data.username, PERMISSIONS_CODENAMES.get('change_asset'))); } + if (data.formManage) { + parsed.push(buildBackendPerm(data.username, PERMISSIONS_CODENAMES.get('manage_asset'))); + } + if (data.submissionsViewPartial) { let permObj = buildBackendPerm(data.username, PERMISSIONS_CODENAMES.get('partial_submissions')); permObj.partial_permissions = [{ @@ -159,6 +164,9 @@ function buildFormData(permissions) { if (perm.permission === permConfig.getPermissionByCodename(PERMISSIONS_CODENAMES.get('change_asset')).url) { formData.formEdit = true; } + if (perm.permission === permConfig.getPermissionByCodename(PERMISSIONS_CODENAMES.get('manage_asset')).url) { + formData.formManage = true; + } if (perm.permission === permConfig.getPermissionByCodename(PERMISSIONS_CODENAMES.get('partial_submissions')).url) { formData.submissionsView = true; formData.submissionsViewPartial = true; diff --git a/jsapp/js/components/permissions/permValidator.es6 b/jsapp/js/components/permissions/permValidator.es6 index d9a21bde44..3210406875 100644 --- a/jsapp/js/components/permissions/permValidator.es6 +++ b/jsapp/js/components/permissions/permValidator.es6 @@ -38,6 +38,10 @@ class PermValidator extends React.Component { }); let hasAllImplied = true; + // FIXME: `manage_asset` implies all the `*_submission` permissions, but + // those are assignable *only* when the asset type is 'survey'. We need to + // design a way to pass that nuance from the back end to the front end + /* allImplied.forEach((implied) => { let isFound = false; permissionAssignments.forEach((assignment) => { @@ -50,6 +54,7 @@ class PermValidator extends React.Component { hasAllImplied = false; } }); + */ let hasAnyContradictory = false; allContradictory.forEach((contradictory) => { diff --git a/jsapp/js/components/permissions/permissionsMocks.es6 b/jsapp/js/components/permissions/permissionsMocks.es6 index 163e5d2f9c..49a01dfce5 100644 --- a/jsapp/js/components/permissions/permissionsMocks.es6 +++ b/jsapp/js/components/permissions/permissionsMocks.es6 @@ -6,7 +6,7 @@ // /api/v2/permissions/ const permissions = { - 'count': 10, + 'count': 11, 'next': null, 'previous': null, 'results': [ @@ -52,6 +52,23 @@ const permissions = { ], 'name': 'Can delete submitted data for asset' }, + { + "url": "/api/v2/permissions/manage_asset.json", + "codename": "manage_asset", + "implied": [ + "/api/v2/permissions/delete_submissions/", + "/api/v2/permissions/change_submissions/", + "/api/v2/permissions/validate_submissions/", + "/api/v2/permissions/view_asset/", + "/api/v2/permissions/change_asset/", + "/api/v2/permissions/view_submissions/", + "/api/v2/permissions/add_submissions/" + ], + 'contradictory': [ + '/api/v2/permissions/partial_submissions/' + ], + "name": "Can manage all aspects of asset" + }, { 'url': '/api/v2/permissions/partial_submissions/', 'codename': 'partial_submissions', diff --git a/jsapp/js/components/permissions/userAssetPermsEditor.es6 b/jsapp/js/components/permissions/userAssetPermsEditor.es6 index fb7a383cf3..5acb10c3a4 100644 --- a/jsapp/js/components/permissions/userAssetPermsEditor.es6 +++ b/jsapp/js/components/permissions/userAssetPermsEditor.es6 @@ -47,6 +47,7 @@ class UserAssetPermsEditor extends React.Component { username: '', formView: false, formViewDisabled: false, + formEditDisabled: false, formEdit: false, submissionsView: false, submissionsViewDisabled: false, @@ -54,6 +55,7 @@ class UserAssetPermsEditor extends React.Component { submissionsViewPartialDisabled: false, submissionsViewPartialUsers: [], submissionsAdd: false, + submissionsAddDisabled: false, submissionsEdit: false, submissionsEditDisabled: false, submissionsDelete: false, @@ -137,8 +139,10 @@ class UserAssetPermsEditor extends React.Component { applyValidityRules(stateObj) { // reset disabling before checks stateObj.formViewDisabled = false; + stateObj.formEditDisabled = false; stateObj.submissionsViewDisabled = false; stateObj.submissionsViewPartialDisabled = false; + stateObj.submissionsAddDisabled = false; stateObj.submissionsDeleteDisabled = false; stateObj.submissionsEditDisabled = false; stateObj.submissionsValidateDisabled = false; @@ -187,6 +191,26 @@ class UserAssetPermsEditor extends React.Component { stateObj.submissionsViewPartialUsers = []; } + // `formManage` implies every other permission (except parial permissions) + if (stateObj.formManage) { + stateObj.formView = true; + stateObj.formEdit = true; + stateObj.submissionsAdd = true; + stateObj.submissionsView = true; + stateObj.submissionsDelete = true; + stateObj.submissionsEdit = true; + stateObj.submissionsValidate = true; + + stateObj.formViewDisabled = true; + stateObj.formEditDisabled = true; + stateObj.submissionsViewDisabled = true; + stateObj.submissionsViewPartialDisabled = true; + stateObj.submissionsAddDisabled = true; + stateObj.submissionsDeleteDisabled = true; + stateObj.submissionsEditDisabled = true; + stateObj.submissionsValidateDisabled = true; + } + return stateObj; } @@ -339,6 +363,7 @@ class UserAssetPermsEditor extends React.Component { if (this.isAssignable('view_asset')) {output.formView = this.state.formView;} if (this.isAssignable('change_asset')) {output.formEdit = this.state.formEdit;} + if (this.isAssignable('manage_asset')) {output.formManage = this.state.formManage;} if (this.isAssignable('add_submissions')) {output.submissionsAdd = this.state.submissionsAdd;} if (this.isAssignable('view_submissions')) {output.submissionsView = this.state.submissionsView;} if (this.isAssignable('partial_submissions')) { @@ -421,6 +446,7 @@ class UserAssetPermsEditor extends React.Component { {this.isAssignable('change_asset') && @@ -459,6 +485,7 @@ class UserAssetPermsEditor extends React.Component { {this.isAssignable('add_submissions') && @@ -490,6 +517,14 @@ class UserAssetPermsEditor extends React.Component { label={this.getLabel('validate_submissions')} /> } + + {this.isAssignable('manage_asset') && + + }
diff --git a/jsapp/js/constants.es6 b/jsapp/js/constants.es6 index 5bb926cabc..2458e309de 100644 --- a/jsapp/js/constants.es6 +++ b/jsapp/js/constants.es6 @@ -27,6 +27,7 @@ export const PERMISSIONS_CODENAMES = new Map(); new Set([ 'view_asset', 'change_asset', + 'manage_asset', 'add_submissions', 'view_submissions', 'partial_submissions', diff --git a/kpi/constants.py b/kpi/constants.py index 8c73969ba2..3899edd21d 100644 --- a/kpi/constants.py +++ b/kpi/constants.py @@ -52,6 +52,7 @@ # ASSIGNABLE_PERMISSIONS PERM_VIEW_ASSET = 'view_asset' PERM_CHANGE_ASSET = 'change_asset' +PERM_MANAGE_ASSET = 'manage_asset' PERM_ADD_SUBMISSIONS = 'add_submissions' PERM_DELETE_SUBMISSIONS = 'delete_submissions' PERM_VIEW_SUBMISSIONS = 'view_submissions' @@ -62,11 +63,8 @@ PERM_CHANGE_COLLECTION = 'change_collection' # CALCULATED_PERMISSIONS -PERM_SHARE_ASSET = 'share_asset' PERM_DELETE_ASSET = 'delete_asset' -PERM_SHARE_SUBMISSIONS = 'share_submissions' PERM_DELETE_SUBMISSIONS = 'delete_submissions' -PERM_SHARE_COLLECTION = 'share_collection' PERM_DELETE_COLLECTION = 'delete_collection' # KC INTERNAL diff --git a/kpi/filters.py b/kpi/filters.py index ba4192a93e..fc332897ff 100644 --- a/kpi/filters.py +++ b/kpi/filters.py @@ -159,6 +159,10 @@ def filter_queryset(self, request, queryset, view): class KpiAssignedObjectPermissionsFilter(filters.BaseFilterBackend): + """ + Used by kpi.views.v1.object_permission.ObjectPermissionViewSet only + """ + def filter_queryset(self, request, queryset, view): # TODO: omit objects for which the user has only a deny permission user = request.user @@ -171,10 +175,11 @@ def filter_queryset(self, request, queryset, view): # Hide permissions from anonymous users return queryset.none() """ - A regular user sees permissions for objects to which they have access. - For example, if Alana has view access to an object owned by Richard, - she should see all permissions for that object, including those - assigned to other users. + A regular user sees their own permissions and the owner's permissions + for objects to which they have access. For example, if Alana and John + have view access to an object owned by Richard, John should see all of + his own permissions and Richard's permissions, but not any of Alana's + permissions. """ possible_content_types = ContentType.objects.get_for_models( *get_models_with_object_permissions() @@ -187,11 +192,22 @@ def filter_queryset(self, request, queryset, view): user=user, ) # Find all the objects associated with those permissions, and then - # find all the permissions applied to all of those objects - result |= ObjectPermission.objects.filter( - content_type=content_type, - object_id__in=permissions_assigned_to_user.values( - 'object_id' - ).distinct() - ) + # find all the owners' permissions applied to all of those objects + for object_id, owner_id in ( + content_type.model_class() + .objects.filter( + pk__in=permissions_assigned_to_user.values_list( + 'object_id', flat=True + ).distinct() + ) + .values_list('pk', 'owner_id') + ): + criteria = dict( + content_type=content_type, + object_id=object_id, + ) + # The owner sees all permission assignments, but others don't + if user.pk != owner_id: + criteria['user_id__in'] = (user.pk, owner_id) + result |= ObjectPermission.objects.filter(**criteria) return result diff --git a/kpi/fixtures/conflicting_versions.json b/kpi/fixtures/conflicting_versions.json index 0b3923f809..afbfb696ec 100644 --- a/kpi/fixtures/conflicting_versions.json +++ b/kpi/fixtures/conflicting_versions.json @@ -604,7 +604,6 @@ } ] }, - "editors_can_change_permissions": true, "owner": 2, "date_created": "2017-12-18T22:42:59.233Z", "asset_type": "survey", diff --git a/kpi/fixtures/test_data.json b/kpi/fixtures/test_data.json index 1bcfcd66fc..cc0fb7e6d1 100644 --- a/kpi/fixtures/test_data.json +++ b/kpi/fixtures/test_data.json @@ -34,18 +34,16 @@ ["add_collection", "kpi", "collection"], ["change_collection", "kpi", "collection"], ["delete_collection", "kpi", "collection"], - ["share_collection", "kpi", "collection"], ["view_collection", "kpi", "collection"], ["add_asset", "kpi", "asset"], ["change_asset", "kpi", "asset"], ["delete_asset", "kpi", "asset"], - ["share_asset", "kpi", "asset"], + ["manage_asset", "kpi", "asset"], ["view_asset", "kpi", "asset"], ["add_submissions", "kpi", "asset"], ["change_submissions", "kpi", "asset"], ["validate_submissions", "kpi", "asset"], ["delete_submissions", "kpi", "asset"], - ["share_submissions", "kpi", "asset"], ["view_submissions", "kpi", "asset"] ] }, @@ -69,18 +67,16 @@ ["add_collection", "kpi", "collection"], ["change_collection", "kpi", "collection"], ["delete_collection", "kpi", "collection"], - ["share_collection", "kpi", "collection"], ["view_collection", "kpi", "collection"], ["add_asset", "kpi", "asset"], ["change_asset", "kpi", "asset"], ["delete_asset", "kpi", "asset"], - ["share_asset", "kpi", "asset"], + ["manage_asset", "kpi", "asset"], ["view_asset", "kpi", "asset"], ["add_submissions", "kpi", "asset"], ["change_submissions", "kpi", "asset"], ["validate_submissions", "kpi", "asset"], ["delete_submissions", "kpi", "asset"], - ["share_submissions", "kpi", "asset"], ["view_submissions", "kpi", "asset"] ] }, diff --git a/kpi/migrations/0027_add_manage_asset_permission.py b/kpi/migrations/0027_add_manage_asset_permission.py new file mode 100644 index 0000000000..a4aafe922e --- /dev/null +++ b/kpi/migrations/0027_add_manage_asset_permission.py @@ -0,0 +1,57 @@ +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('kpi', '0026_disable_editors_can_change_permissions'), + ] + + operations = [ + migrations.AlterModelOptions( + name='asset', + options={ + 'default_permissions': ('add', 'change', 'delete'), + 'ordering': ('-date_modified',), + 'permissions': ( + ('view_asset', 'Can view asset'), + ('manage_asset', 'Can manage all aspects of asset'), + ('add_submissions', 'Can submit data to asset'), + ('view_submissions', 'Can view submitted data for asset'), + ( + 'partial_submissions', + 'Can make partial actions on submitted data for asset for specific users', + ), + ( + 'change_submissions', + 'Can modify submitted data for asset', + ), + ( + 'delete_submissions', + 'Can delete submitted data for asset', + ), + ( + 'validate_submissions', + 'Can validate submitted data asset', + ), + ('from_kc_only', 'INTERNAL USE ONLY; DO NOT ASSIGN'), + ), + }, + ), + migrations.AlterModelOptions( + name='collection', + options={ + 'default_permissions': ('add', 'change', 'delete'), + 'ordering': ('-date_modified',), + 'permissions': (('view_collection', 'Can view collection'),), + }, + ), + migrations.RemoveField( + model_name='asset', + name='editors_can_change_permissions', + ), + migrations.RemoveField( + model_name='collection', + name='editors_can_change_permissions', + ), + ] diff --git a/kpi/migrations/0028_assign_manage_asset_permissions.py b/kpi/migrations/0028_assign_manage_asset_permissions.py new file mode 100644 index 0000000000..1f971725e2 --- /dev/null +++ b/kpi/migrations/0028_assign_manage_asset_permissions.py @@ -0,0 +1,74 @@ +import sys + +from django.contrib.auth.management import create_permissions +from django.contrib.auth.models import AnonymousUser +from django.db import migrations + + +def grant_object_level_perms(apps): + """ + Grant `manage_asset` to the owner of every asset + """ + ContentType = apps.get_model('contenttypes', 'ContentType') # noqa + User = apps.get_model('auth', 'User') # noqa + Permission = apps.get_model('auth', 'Permission') # noqa + Asset = apps.get_model('kpi', 'Asset') # noqa + ObjectPermission = apps.get_model('kpi', 'ObjectPermission') # noqa + + new_perm = Permission.objects.get( + content_type__app_label='kpi', codename='manage_asset' + ) + content_type = ContentType.objects.get_for_model(Asset) + new_perm_objects = [] + for asset in Asset.objects.only('owner_id'): + new_perm_assignment = ObjectPermission( + permission_id=new_perm.pk, + user_id=asset.owner_id, + object_id=asset.pk, + content_type_id=content_type.pk, + ) + new_perm_objects.append(new_perm_assignment) + sys.stderr.write( + 'Creating {} object-level permission assignments...\n'.format( + len(new_perm_objects) + ) + ) + sys.stderr.flush() + ObjectPermission.objects.bulk_create( + new_perm_objects, ignore_conflicts=True + ) + + +def remove_object_level_perms(apps): + """ + Remove all object-level 'manage_asset' permission assignments + """ + Permission = apps.get_model('auth', 'Permission') # noqa + ObjectPermission = apps.get_model('kpi', 'ObjectPermission') # noqa + perm = Permission.objects.get( + content_type__app_label='kpi', codename='manage_asset' + ) + ObjectPermission.objects.filter(permission=perm).delete() + + +def forwards_func(apps, schema_editor): + sys.stderr.write( + 'Granting `manage_asset` to all asset owners. This may take several ' + 'minutes on large databases...\n' + ) + sys.stderr.flush() + grant_object_level_perms(apps) + + +def reverse_func(apps, schema_editor): + remove_object_level_perms(apps) + + +class Migration(migrations.Migration): + dependencies = [ + ('kpi', '0027_add_manage_asset_permission'), + ] + operations = [ + migrations.RunPython(forwards_func, reverse_func), + ] + diff --git a/kpi/models/asset.py b/kpi/models/asset.py index 9d9e56a2bb..dc5af526e5 100644 --- a/kpi/models/asset.py +++ b/kpi/models/asset.py @@ -39,9 +39,8 @@ PERM_DELETE_ASSET, PERM_DELETE_SUBMISSIONS, PERM_FROM_KC_ONLY, + PERM_MANAGE_ASSET, PERM_PARTIAL_SUBMISSIONS, - PERM_SHARE_ASSET, - PERM_SHARE_SUBMISSIONS, PERM_VALIDATE_SUBMISSIONS, PERM_VIEW_ASSET, PERM_VIEW_COLLECTION, @@ -487,9 +486,6 @@ class Asset(ObjectPermissionMixin, null=True, blank=True, on_delete=models.CASCADE) owner = models.ForeignKey('auth.User', related_name='assets', null=True, on_delete=models.CASCADE) - # TODO: remove this flag; support for it has been removed from - # ObjectPermissionMixin - editors_can_change_permissions = models.BooleanField(default=False) uid = KpiUidField(uid_prefix='a') tags = TaggableManager(manager=KpiTaggableManager) settings = JSONBField(default=dict) @@ -513,7 +509,7 @@ class Meta: # change_, add_, and delete_asset are provided automatically # by Django (PERM_VIEW_ASSET, _('Can view asset')), - (PERM_SHARE_ASSET, _("Can change asset's sharing settings")), + (PERM_MANAGE_ASSET, _('Can manage all aspects of asset')), # Permissions for collected data, i.e. submissions (PERM_ADD_SUBMISSIONS, _('Can submit data to asset')), (PERM_VIEW_SUBMISSIONS, _('Can view submitted data for asset')), @@ -522,8 +518,6 @@ class Meta: 'for specific users')), (PERM_CHANGE_SUBMISSIONS, _('Can modify submitted data for asset')), (PERM_DELETE_SUBMISSIONS, _('Can delete submitted data for asset')), - (PERM_SHARE_SUBMISSIONS, _("Can change sharing settings for " - "asset's submitted data")), (PERM_VALIDATE_SUBMISSIONS, _("Can validate submitted data asset")), # TEMPORARY Issue #1161: A flag to indicate that permissions came # solely from `sync_kobocat_xforms` and not from any user @@ -542,15 +536,19 @@ class Meta: # The simplest way to fix this is to keep old behaviour default_permissions = ('add', 'change', 'delete') - # Labels for each `asset_type` as they should be presented to users - ASSET_TYPE_LABELS = { - ASSET_TYPE_SURVEY: _('form'), + # Labels for each `asset_type` as they should be presented to users. Can be + # strings or callables if special logic is needed. Callables receive the + # codename of the permission for which a label is being created + ASSET_TYPE_LABELS_FOR_PERMISSIONS = { + ASSET_TYPE_SURVEY: ( + lambda p: _('project') if p == PERM_MANAGE_ASSET else _('form') + ), ASSET_TYPE_TEMPLATE: _('template'), ASSET_TYPE_BLOCK: _('block'), ASSET_TYPE_QUESTION: _('question'), ASSET_TYPE_TEXT: _('text'), # unused? ASSET_TYPE_EMPTY: _('empty'), # unused? - #ASSET_TYPE_COLLECTION: _('collection'), + # ASSET_TYPE_COLLECTION: _('collection'), } # Assignable permissions that are stored in the database. @@ -559,6 +557,7 @@ class Meta: ASSIGNABLE_PERMISSIONS_WITH_LABELS = { PERM_VIEW_ASSET: _('View ##asset_type_label##'), PERM_CHANGE_ASSET: _('Edit ##asset_type_label##'), + PERM_MANAGE_ASSET: _('Manage ##asset_type_label##'), PERM_ADD_SUBMISSIONS: _('Add submissions'), PERM_VIEW_SUBMISSIONS: _('View submissions'), PERM_PARTIAL_SUBMISSIONS: _('View submissions only from specific users'), @@ -569,21 +568,31 @@ class Meta: ASSIGNABLE_PERMISSIONS = tuple(ASSIGNABLE_PERMISSIONS_WITH_LABELS.keys()) # Depending on our `asset_type`, only some permissions might be applicable ASSIGNABLE_PERMISSIONS_BY_TYPE = { - ASSET_TYPE_SURVEY: ASSIGNABLE_PERMISSIONS, # all of them - ASSET_TYPE_TEMPLATE: (PERM_VIEW_ASSET, PERM_CHANGE_ASSET), - ASSET_TYPE_BLOCK: (PERM_VIEW_ASSET, PERM_CHANGE_ASSET), - ASSET_TYPE_QUESTION: (PERM_VIEW_ASSET, PERM_CHANGE_ASSET), + ASSET_TYPE_SURVEY: ASSIGNABLE_PERMISSIONS, # all of them + ASSET_TYPE_TEMPLATE: ( + PERM_VIEW_ASSET, + PERM_CHANGE_ASSET, + PERM_MANAGE_ASSET, + ), + ASSET_TYPE_BLOCK: ( + PERM_VIEW_ASSET, + PERM_CHANGE_ASSET, + PERM_MANAGE_ASSET, + ), + ASSET_TYPE_QUESTION: ( + PERM_VIEW_ASSET, + PERM_CHANGE_ASSET, + PERM_MANAGE_ASSET, + ), ASSET_TYPE_TEXT: (), # unused? ASSET_TYPE_EMPTY: (), # unused? - #ASSET_TYPE_COLLECTION: # tbd + # ASSET_TYPE_COLLECTION: # tbd } # Calculated permissions that are neither directly assignable nor stored # in the database, but instead implied by assignable permissions CALCULATED_PERMISSIONS = ( - PERM_SHARE_ASSET, PERM_DELETE_ASSET, - PERM_SHARE_SUBMISSIONS ) # Certain Collection permissions carry over to Asset MAPPED_PARENT_PERMISSIONS = { @@ -594,12 +603,19 @@ class Meta: IMPLIED_PERMISSIONS = { # Format: explicit: (implied, implied, ...) PERM_CHANGE_ASSET: (PERM_VIEW_ASSET,), + PERM_MANAGE_ASSET: tuple( + ( + p + for p in ASSIGNABLE_PERMISSIONS + if p != PERM_MANAGE_ASSET and p != PERM_PARTIAL_SUBMISSIONS + ) + ), PERM_ADD_SUBMISSIONS: (PERM_VIEW_ASSET,), PERM_VIEW_SUBMISSIONS: (PERM_VIEW_ASSET,), PERM_PARTIAL_SUBMISSIONS: (PERM_VIEW_ASSET,), PERM_CHANGE_SUBMISSIONS: (PERM_VIEW_SUBMISSIONS,), PERM_DELETE_SUBMISSIONS: (PERM_VIEW_SUBMISSIONS,), - PERM_VALIDATE_SUBMISSIONS: (PERM_VIEW_SUBMISSIONS,) + PERM_VALIDATE_SUBMISSIONS: (PERM_VIEW_SUBMISSIONS,), } CONTRADICTORY_PERMISSIONS = { @@ -608,6 +624,7 @@ class Meta: PERM_CHANGE_SUBMISSIONS, PERM_DELETE_SUBMISSIONS, PERM_VALIDATE_SUBMISSIONS, + PERM_MANAGE_ASSET, ), PERM_VIEW_SUBMISSIONS: (PERM_PARTIAL_SUBMISSIONS,), PERM_CHANGE_SUBMISSIONS: (PERM_PARTIAL_SUBMISSIONS,), @@ -718,10 +735,19 @@ def get_label_for_permission(self, permission_or_codename): codename=codename ) label = permission.name + asset_type_label = self.ASSET_TYPE_LABELS_FOR_PERMISSIONS[ + self.asset_type + ] + try: + # Some labels may be callables + asset_type_label = asset_type_label(codename) + except TypeError: + # Others are just strings + pass label = label.replace( '##asset_type_label##', # Raises TypeError if not coerced explicitly - str(self.ASSET_TYPE_LABELS[self.asset_type]) + str(asset_type_label) ) return label diff --git a/kpi/models/collection.py b/kpi/models/collection.py index d138f87d29..bfe6e16a73 100644 --- a/kpi/models/collection.py +++ b/kpi/models/collection.py @@ -9,8 +9,11 @@ from taggit.managers import TaggableManager from taggit.models import Tag -from kpi.constants import PERM_VIEW_COLLECTION, PERM_CHANGE_COLLECTION, \ - PERM_DELETE_COLLECTION, PERM_SHARE_COLLECTION +from kpi.constants import ( + PERM_CHANGE_COLLECTION, + PERM_DELETE_COLLECTION, + PERM_VIEW_COLLECTION, +) from kpi.fields import KpiUidField from .asset import ( Asset, @@ -52,7 +55,6 @@ class Collection(ObjectPermissionMixin, TagStringMixin, MPTTModel): related_name='children', on_delete=models.CASCADE) owner = models.ForeignKey('auth.User', related_name='owned_collections', on_delete=models.CASCADE) - editors_can_change_permissions = models.BooleanField(default=False) discoverable_when_public = models.BooleanField(default=False) uid = KpiUidField(uid_prefix='c') date_created = models.DateTimeField(auto_now_add=True) @@ -71,8 +73,6 @@ class Meta: # change_, add_, and delete_collection are provided automatically # by Django (PERM_VIEW_COLLECTION, 'Can view collection'), - (PERM_SHARE_COLLECTION, - "Can change this collection's sharing settings"), ) # Since Django 2.1, 4 permissions are added for each registered model: @@ -90,7 +90,7 @@ class Meta: ASSIGNABLE_PERMISSIONS = (PERM_VIEW_COLLECTION, PERM_CHANGE_COLLECTION) # Calculated permissions that are neither directly assignable nor stored # in the database, but instead implied by assignable permissions - CALCULATED_PERMISSIONS = (PERM_SHARE_COLLECTION, PERM_DELETE_COLLECTION) + CALCULATED_PERMISSIONS = (PERM_DELETE_COLLECTION) # Granting some permissions implies also granting other permissions IMPLIED_PERMISSIONS = { # Format: explicit: (implied, implied, ...) diff --git a/kpi/models/object_permission.py b/kpi/models/object_permission.py index cd57c749cf..b950720653 100644 --- a/kpi/models/object_permission.py +++ b/kpi/models/object_permission.py @@ -261,7 +261,7 @@ class Meta: @void_cache_for_request(keys=('__get_all_object_permissions', '__get_all_user_permissions',)) def save(self, *args, **kwargs): - if self.permission.content_type_id is not self.content_type_id: + if self.permission.content_type_id != self.content_type_id: raise ValidationError('The content type of the permission does ' 'not match that of the object.') super().save(*args, **kwargs) @@ -299,7 +299,9 @@ class MyAwesomeModel(ObjectPermissionMixin, models.Model) CONTRADICTORY_PERMISSIONS = {} @classmethod - def get_assignable_permissions(cls, with_partial=True): + def get_assignable_permissions( + cls, with_partial: bool = True, instance: 'kpi.models.Asset' = None + ) -> tuple: """ The "versioned app registry" used during migrations apparently does not store non-database attributes, so this awful workaround is needed @@ -307,18 +309,27 @@ def get_assignable_permissions(cls, with_partial=True): Returns assignable permissions including permissions prefixed by `PREFIX_PARTIAL_PERMS` if `with_partial` is True. + Attempts to return only permissions relevant to the asset type when + `instance` is provided. This feels like a bit of a hack; sorry. + It can be useful to remove the partial permissions when assigning permissions to owner of the object. - - :param with_partial: bool. - :return: tuple """ - try: - assignable_permissions = cls.ASSIGNABLE_PERMISSIONS - except AttributeError: - assignable_permissions = apps.get_model( - cls._meta.app_label, cls._meta.model_name - ).ASSIGNABLE_PERMISSIONS + assignable_permissions = None + if instance: + try: + assignable_permissions = ( + instance.ASSIGNABLE_PERMISSIONS_BY_TYPE[instance.asset_type] + ) + except AttributeError: + pass + if not assignable_permissions: + try: + assignable_permissions = cls.ASSIGNABLE_PERMISSIONS + except AttributeError: + assignable_permissions = apps.get_model( + cls._meta.app_label, cls._meta.model_name + ).ASSIGNABLE_PERMISSIONS if with_partial is False: assignable_permissions = tuple(ap for ap in assignable_permissions @@ -410,12 +421,7 @@ def _get_effective_perms( if user is not None: kwargs['user'] = user if codename is not None: - # share_ requires loading change_ from the database - if codename.startswith('share_'): - kwargs['codename'] = re.sub( - '^share_', 'change_', codename, 1) - else: - kwargs['codename'] = codename + kwargs['codename'] = codename grant_perms = self.__get_object_permissions(deny=False, **kwargs) deny_perms = self.__get_object_permissions(deny=True, **kwargs) @@ -433,33 +439,23 @@ def _get_effective_perms( # Anonymous users weren't considered; no filtering is necessary return effective_perms - # Add on the calculated permissions + # Add the calculated `delete_` permission for the owner content_type = ContentType.objects.get_for_model(self) - if codename in self.CALCULATED_PERMISSIONS: - # A specific query for a calculated permission should not return - # any explicitly assigned permissions, e.g. share_ should not - # include change_ - effective_perms_copy = effective_perms - effective_perms = set() - else: - effective_perms_copy = copy.copy(effective_perms) - # The owner has the share_ and delete_ permission - for codename_prefix in ('share_', 'delete_'): - if self.owner is not None and ( - user is None or user.pk == self.owner.pk) and ( - codename is None or codename.startswith(codename_prefix) - ): - matching_permissions = self.__get_permissions_for_content_type( - content_type.pk, codename__startswith=codename_prefix) - for perm_pk, perm_codename in matching_permissions: - if (codename is not None and - perm_codename != codename - ): - # If the caller specified `codename`, skip anything that - # doesn't match exactly. Necessary because `Asset` has - # `delete_submissions` in addition to `delete_asset` - continue - effective_perms.add((self.owner.pk, perm_pk)) + if self.owner is not None and ( + user is None or user.pk == self.owner.pk) and ( + codename is None or codename.startswith('delete_') + ): + matching_permissions = self.__get_permissions_for_content_type( + content_type.pk, codename__startswith='delete_') + for perm_pk, perm_codename in matching_permissions: + if (codename is not None and + perm_codename != codename + ): + # If the caller specified `codename`, skip anything that + # doesn't match exactly. Necessary because `Asset` has + # `delete_submissions` in addition to `delete_asset` + continue + effective_perms.add((self.owner.pk, perm_pk)) # We may have calculated more permissions for anonymous users # than they are allowed to have. Remove them. if user is None or user.pk == settings.ANONYMOUS_USER_ID: @@ -570,7 +566,9 @@ def _recalculate_inherited_perms( if self.owner is not None: for perm in Permission.objects.filter( content_type=content_type, - codename__in=self.get_assignable_permissions(with_partial=False) + codename__in=self.get_assignable_permissions( + with_partial=False, instance=self + ), ): new_permission = ObjectPermission() new_permission.content_object = self @@ -669,10 +667,7 @@ def get_implied_perms(cls, explicit_perm, reverse=False): implied_perms = implied_perms_dict[this_explicit_perm] except KeyError: continue - if result.intersection(implied_perms): - raise ImproperlyConfigured( - 'Loop in IMPLIED_PERMISSIONS for {}'.format(cls)) - perms_to_process.extend(implied_perms) + perms_to_process.extend(set(implied_perms).difference(result)) result.update(implied_perms) return result @@ -727,11 +722,13 @@ def assign_perm(self, user_obj, perm, deny=False, defer_recalc=False, partial permissions """ app_label, codename = perm_parse(perm, self) - if codename not in self.get_assignable_permissions(): + assignable_permissions = self.get_assignable_permissions(instance=self) + if codename not in assignable_permissions: # Some permissions are calculated and not stored in the database raise ValidationError( - '{} cannot be assigned explicitly to {} objects.'.format( - codename, self._meta.model_name) + '{} cannot be assigned explicitly to this object.'.format( + codename + ) ) if isinstance(user_obj, AnonymousUser) or ( user_obj.pk == settings.ANONYMOUS_USER_ID @@ -804,7 +801,9 @@ def assign_perm(self, user_obj, perm, deny=False, defer_recalc=False, assign_applicable_kc_permissions(self, user_obj, codename) # Resolve implied permissions, e.g. granting change implies granting # view - implied_perms = self.get_implied_perms(codename, reverse=deny) + implied_perms = self.get_implied_perms( + codename, reverse=deny + ).intersection(assignable_permissions) for implied_perm in implied_perms: self.assign_perm( user_obj, implied_perm, deny=deny, defer_recalc=True) @@ -918,6 +917,9 @@ def remove_perm(self, user_obj, perm, defer_recalc=False, skip_kc=False): # Get the User database representation for AnonymousUser user_obj = get_anonymous_user() app_label, codename = perm_parse(perm, self) + # Unlike `assign_perm()`, do not pass `instance` to + # `get_assignable_permissions()`. That way, we can allow invalid + # permissions to be removed if codename not in self.get_assignable_permissions(): # Some permissions are calculated and not stored in the database raise ValidationError('{} cannot be removed explicitly.'.format( diff --git a/kpi/permissions.py b/kpi/permissions.py index 59dbd8fb20..64ac90dab0 100644 --- a/kpi/permissions.py +++ b/kpi/permissions.py @@ -148,7 +148,7 @@ class AssetNestedObjectPermission(BaseAssetNestedObjectPermission): perms_map = { 'GET': ['%(app_label)s.view_asset'], - 'POST': ['%(app_label)s.change_asset'], + 'POST': ['%(app_label)s.manage_asset'], } perms_map['OPTIONS'] = perms_map['GET'] @@ -182,7 +182,11 @@ def has_permission(self, request, view): else: raise Http404 - has_perm = set(required_permissions).issubset(user_permissions) + if user == parent_object.owner: + # The owner can always manage permission assignments + has_perm = True + else: + has_perm = set(required_permissions).issubset(user_permissions) if has_perm: # Access granted! @@ -225,7 +229,8 @@ class CollectionNestedObjectPermission(BaseCollectionNestedObjectPermission, perms_map = { 'GET': ['%(app_label)s.view_collection'], - 'POST': ['%(app_label)s.change_collection'], + # TODO: trash this once Collection is a type of Asset + 'POST': ['%(app_label)s.INTENTIONALLY IMPOSSIBLE TO MATCH'], } perms_map['OPTIONS'] = perms_map['GET'] diff --git a/kpi/tests/api/v1/test_api_permissions.py b/kpi/tests/api/v1/test_api_permissions.py index 4b3a5db8c7..ecbd9165e7 100644 --- a/kpi/tests/api/v1/test_api_permissions.py +++ b/kpi/tests/api/v1/test_api_permissions.py @@ -31,8 +31,8 @@ class ApiAssignedPermissionsTestCase(KpiTestCase): * Superusers see it all (thank goodness for pagination) * Anonymous users see nothing * Regular users see everything that concerns them, namely all - permissions for all objects to which they have been assigned any - permission + their own permissions and all the owners' permissions for all objects + to which they have been assigned any permission See also `kpi.filters.KpiAssignedObjectPermissionsFilter` """ @@ -87,10 +87,16 @@ def test_anon_cannot_list_permissions(self): self.asset.remove_perm(self.anon, 'view_asset') self.assertFalse(self.anon.has_perm('view_asset', self.asset)) - def test_user_sees_all_permissions_on_assigned_objects(self): + def test_user_sees_relevant_permissions_on_assigned_objects(self): + # A user with explicitly-assigned permissions should see their + # own permissions and the owner's permissions, but not permissions + # assigned to other users self.asset.assign_perm(self.anotheruser, 'view_asset') self.assertTrue(self.anotheruser.has_perm('view_asset', self.asset)) + irrelevant_user = User.objects.create(username='mindyourown') + self.asset.assign_perm(irrelevant_user, 'view_asset') + self.client.login(username=self.anotheruser.username, password=self.anotheruser_password) @@ -100,11 +106,16 @@ def test_user_sees_all_permissions_on_assigned_objects(self): returned_uids = [r['uid'] for r in response.data['results']] all_obj_perms = ObjectPermission.objects.filter_for_object(self.asset) + relevant_obj_perms = all_obj_perms.filter( + user__in=(self.asset.owner, self.anotheruser), + permission__codename__in=self.asset.ASSIGNABLE_PERMISSIONS_BY_TYPE[ + self.asset.asset_type + ], + ) - self.assertTrue( - set(returned_uids).issuperset( - all_obj_perms.values_list('uid', flat=True) - ) + self.assertListEqual( + sorted(returned_uids), + sorted(relevant_obj_perms.values_list('uid', flat=True)), ) self.asset.remove_perm(self.anotheruser, 'view_asset') diff --git a/kpi/tests/api/v2/test_api_asset_permission_assignment.py b/kpi/tests/api/v2/test_api_asset_permission_assignment.py index d7d114c32b..bb8640cb5c 100644 --- a/kpi/tests/api/v2/test_api_asset_permission_assignment.py +++ b/kpi/tests/api/v2/test_api_asset_permission_assignment.py @@ -1,10 +1,21 @@ # coding: utf-8 -from django.contrib.auth.models import User +from django.contrib.auth.models import User, Permission +from django.core.exceptions import ValidationError from django.urls import reverse from rest_framework import status -from kpi.constants import PERM_VIEW_ASSET, PERM_CHANGE_ASSET -from kpi.models import Asset +from kpi.constants import ( + ASSET_TYPE_TEMPLATE, + PERM_VIEW_ASSET, + PERM_CHANGE_ASSET, + PERM_MANAGE_ASSET, + PERM_ADD_SUBMISSIONS, + PERM_DELETE_SUBMISSIONS, + PERM_VIEW_SUBMISSIONS, + PERM_CHANGE_SUBMISSIONS, + PERM_VALIDATE_SUBMISSIONS, +) +from kpi.models import Asset, ObjectPermission from kpi.models.object_permission import get_anonymous_user from kpi.tests.kpi_test_case import KpiTestCase from kpi.urls.router_api_v2 import URL_NAMESPACE as ROUTER_URL_NAMESPACE @@ -24,41 +35,23 @@ def setUp(self): self.client.login(username='admin', password='pass') self.asset = self.create_asset('An asset to be shared') - self.admin_detail_url = reverse( - self._get_endpoint('user-detail'), - kwargs={'username': self.admin.username}) - - self.someuser_detail_url = reverse( - self._get_endpoint('user-detail'), - kwargs={'username': self.someuser.username}) - - self.anotheruser_detail_url = reverse( - self._get_endpoint('user-detail'), - kwargs={'username': self.anotheruser.username}) - - self.view_asset_permission_detail_url = reverse( - self._get_endpoint('permission-detail'), - kwargs={'codename': PERM_VIEW_ASSET}) - - self.change_asset_permission_detail_url = reverse( - self._get_endpoint('permission-detail'), - kwargs={'codename': PERM_CHANGE_ASSET}) - - self.asset_permissions_list_url = reverse( - self._get_endpoint('asset-permission-assignment-list'), - kwargs={'parent_lookup_asset': self.asset.uid} - ) - - def _logged_user_gives_permission(self, username, permission): + def _grant_perm_as_logged_in_user(self, username, codename): """ - Uses the API to grant `permission` to `username` + Uses the API to grant the permission identified by `codename` on + `self.asset` to the user identified by `username`. Does not attempt any + authentication. """ data = { - 'user': getattr(self, '{}_detail_url'.format(username)), - 'permission': getattr(self, '{}_permission_detail_url'.format(permission)) + 'user': self.obj_to_url(User.objects.get(username=username)), + 'permission': self.obj_to_url( + Permission.objects.get(codename=codename) + ), } - response = self.client.post(self.asset_permissions_list_url, - data, format='json') + response = self.client.post( + self.get_asset_perm_assignment_list_url(self.asset), + data, + format='json', + ) return response @@ -66,28 +59,49 @@ class ApiAssetPermissionTestCase(BaseApiAssetPermissionTestCase): def test_owner_can_give_permissions(self): # Current user is `self.admin` - response = self._logged_user_gives_permission('someuser', PERM_VIEW_ASSET) + response = self._grant_perm_as_logged_in_user('someuser', PERM_VIEW_ASSET) self.assertEqual(response.status_code, status.HTTP_201_CREATED) - def test_viewers_can_not_give_permissions(self): - self._logged_user_gives_permission('someuser', PERM_VIEW_ASSET) + def test_viewers_cannot_give_permissions(self): + self._grant_perm_as_logged_in_user('someuser', PERM_VIEW_ASSET) + self.assertTrue(self.asset.has_perm(self.someuser, PERM_VIEW_ASSET)) self.client.login(username='someuser', password='someuser') # Current user is now: `self.someuser` - response = self._logged_user_gives_permission('anotheruser', PERM_VIEW_ASSET) + response = self._grant_perm_as_logged_in_user('anotheruser', PERM_VIEW_ASSET) self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) - def test_editors_can_give_permissions(self): - self._logged_user_gives_permission('someuser', PERM_CHANGE_ASSET) + def test_editors_cannot_give_permissions(self): + self._grant_perm_as_logged_in_user('someuser', PERM_CHANGE_ASSET) + self.assertTrue(self.asset.has_perm(self.someuser, PERM_CHANGE_ASSET)) self.client.login(username='someuser', password='someuser') # Current user is now: `self.someuser` - response = self._logged_user_gives_permission('anotheruser', PERM_VIEW_ASSET) - self.assertEqual(response.status_code, status.HTTP_201_CREATED) + response = self._grant_perm_as_logged_in_user('anotheruser', PERM_VIEW_ASSET) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) - def test_anonymous_can_not_give_permissions(self): + def test_anonymous_cannot_give_permissions(self): self.client.logout() - response = self._logged_user_gives_permission('someuser', PERM_VIEW_ASSET) + response = self._grant_perm_as_logged_in_user('someuser', PERM_VIEW_ASSET) self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + def test_managers_can_give_permissions(self): + self._grant_perm_as_logged_in_user('someuser', PERM_MANAGE_ASSET) + self.assertTrue(self.asset.has_perm(self.someuser, PERM_MANAGE_ASSET)) + self.client.login(username='someuser', password='someuser') + response = self._grant_perm_as_logged_in_user('anotheruser', PERM_VIEW_ASSET) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + + def test_submission_assignments_ignored_for_non_survey_assets(self): + self.asset.asset_type = ASSET_TYPE_TEMPLATE + self.asset.save() + # FIXME: should handle ValidationError somewhere and return HTTP 405 + self.assertRaises( + ValidationError, + self._grant_perm_as_logged_in_user, + 'someuser', + PERM_VIEW_SUBMISSIONS, + ) + self.assertFalse(self.asset.has_perm(self.someuser, PERM_VIEW_SUBMISSIONS)) + class ApiAssetPermissionListTestCase(BaseApiAssetPermissionTestCase): """ @@ -108,7 +122,7 @@ def test_viewers_see_only_self_anon_and_owner_assignments(self): self.client.login(username='anotheruser', password='anotheruser') permission_list_response = self.client.get( - self.asset_permissions_list_url, format='json' + self.get_asset_perm_assignment_list_url(self.asset), format='json' ) self.assertEqual( permission_list_response.status_code, status.HTTP_200_OK @@ -145,12 +159,40 @@ def test_viewers_see_only_self_anon_and_owner_assignments(self): self.assertEqual(expected_perms, obj_perms) + def test_managers_see_all_assignments(self): + manager = User(username='businessfish') + manager.set_password('manage this!') + manager.save() + self.asset.assign_perm(manager, PERM_MANAGE_ASSET) + + self.client.login(username='businessfish', password='manage this!') + response = self.client.get( + self.get_asset_perm_assignment_list_url(self.asset) + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + returned_urls = [r['url'] for r in response.data] + all_obj_perms = ObjectPermission.objects.filter_for_object(self.asset) + assigned_obj_perms = all_obj_perms.filter( + permission__codename__in=self.asset.get_assignable_permissions( + with_partial=False + ), + ) + + self.assertListEqual( + sorted(returned_urls), + sorted( + self.get_urls_for_asset_perm_assignment_objs( + assigned_obj_perms, asset=self.asset + ) + ), + ) def test_editors_see_only_self_anon_and_owner_assignments(self): self.client.login(username='someuser', password='someuser') permission_list_response = self.client.get( - self.asset_permissions_list_url, format='json' + self.get_asset_perm_assignment_list_url(self.asset), format='json' ) self.assertEqual( permission_list_response.status_code, status.HTTP_200_OK @@ -192,8 +234,9 @@ def test_editors_see_only_self_anon_and_owner_assignments(self): def test_anonymous_get_only_owner_s_assignments(self): self.client.logout() - permission_list_response = self.client.get(self.asset_permissions_list_url, - format='json') + permission_list_response = self.client.get( + self.get_asset_perm_assignment_list_url(self.asset), format='json' + ) self.assertEqual(permission_list_response.status_code, status.HTTP_200_OK) admin_perms = self.asset.get_perms(self.admin) results = permission_list_response.data @@ -219,51 +262,155 @@ def test_anonymous_get_only_owner_s_assignments(self): class ApiBulkAssetPermissionTestCase(BaseApiAssetPermissionTestCase): - def _logged_user_gives_permissions(self, assignments): + def _assign_perms_as_logged_in_user(self, assignments): """ - Uses the API to grant `permission` to `username` + Uses the bulk API to replace the permission assignments of `self.asset` + with `assignments`. Does not attempt any authentication. """ - url = '{}bulk/'.format(self.asset_permissions_list_url) + url = reverse( + # this view name is a bit... bulky + self._get_endpoint('asset-permission-assignment-bulk-assignments'), + kwargs={'parent_lookup_asset': self.asset.uid} + ) - def get_data_template(username_, permission_): + def get_data_template(username_, codename_): return { - 'user': getattr(self, '{}_detail_url'.format(username_)), - 'permission': getattr(self, '{}_permission_detail_url'.format( - permission_)) + 'user': self.obj_to_url(User.objects.get(username=username)), + 'permission': self.obj_to_url( + Permission.objects.get(codename=codename_) + ), } data = [] - for username, permission in assignments: - data.append(get_data_template(username, permission)) + for username, codename in assignments: + data.append(get_data_template(username, codename)) response = self.client.post(url, data, format='json') return response def test_cannot_assign_permissions_to_owner(self): - self._logged_user_gives_permission('someuser', PERM_CHANGE_ASSET) + self._grant_perm_as_logged_in_user('someuser', PERM_MANAGE_ASSET) self.client.login(username='someuser', password='someuser') - response = self._logged_user_gives_permissions([ + response = self._assign_perms_as_logged_in_user([ ('admin', PERM_VIEW_ASSET), ('admin', PERM_CHANGE_ASSET) ]) self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - def test_bulk_assign_permissions(self): - # TODO Improve this test - permission_list_response = self.client.get(self.asset_permissions_list_url, - format='json') + def test_owner_can_assign_permissions(self): + permission_list_response = self.client.get( + self.get_asset_perm_assignment_list_url(self.asset), format='json' + ) self.assertEqual(permission_list_response.status_code, status.HTTP_200_OK) - total = len(permission_list_response.data) - # Add number of permissions added with 'view_asset' - total += len(Asset.get_implied_perms(PERM_VIEW_ASSET)) + 1 - # Add number of permissions added with 'change_asset' - total += len(Asset.get_implied_perms(PERM_CHANGE_ASSET)) + 1 - response = self._logged_user_gives_permissions([ + response = self._assign_perms_as_logged_in_user([ ('someuser', PERM_VIEW_ASSET), ('someuser', PERM_VIEW_ASSET), # Add a duplicate which should not count ('anotheruser', PERM_CHANGE_ASSET) ]) + self.assertEqual(response.status_code, status.HTTP_200_OK) + returned_urls = [r['url'] for r in response.data] + all_obj_perms = ObjectPermission.objects.filter_for_object(self.asset) + assigned_obj_perms = all_obj_perms.filter( + permission__codename__in=self.asset.get_assignable_permissions( + with_partial=False + ) + ) + self.assertListEqual( + sorted( + assigned_obj_perms.values_list( + 'user__username', 'permission__codename' + ) + ), + sorted([ + ('admin', PERM_VIEW_ASSET), + ('admin', PERM_CHANGE_ASSET), + ('admin', PERM_MANAGE_ASSET), + ('admin', PERM_ADD_SUBMISSIONS), + ('admin', PERM_DELETE_SUBMISSIONS), + ('admin', PERM_VIEW_SUBMISSIONS), + ('admin', PERM_CHANGE_SUBMISSIONS), + ('admin', PERM_VALIDATE_SUBMISSIONS), + ('someuser', PERM_VIEW_ASSET), + ('anotheruser', PERM_VIEW_ASSET), + ('anotheruser', PERM_CHANGE_ASSET), + ]), + ) + self.assertListEqual( + sorted(returned_urls), + sorted( + self.get_urls_for_asset_perm_assignment_objs( + assigned_obj_perms, asset=self.asset + ) + ), + ) + + def test_assignment_removes_old_permissions(self): + self.asset.assign_perm(self.someuser, PERM_CHANGE_ASSET) + self.assertTrue(self.asset.has_perm(self.someuser, PERM_CHANGE_ASSET)) + response = self._assign_perms_as_logged_in_user([ + ('someuser', PERM_VIEW_ASSET), + ('anotheruser', PERM_CHANGE_ASSET) + ]) self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual(len(response.data), total) + self.assertFalse(self.asset.has_perm(self.someuser, PERM_CHANGE_ASSET)) + def test_viewers_cannot_give_permissions(self): + self.asset.assign_perm(self.someuser, PERM_VIEW_ASSET) + self.assertTrue(self.asset.has_perm(self.someuser, PERM_VIEW_ASSET)) + self.client.login(username='someuser', password='someuser') + response = self._assign_perms_as_logged_in_user([ + ('anotheruser', PERM_CHANGE_ASSET), + ('anotheruser', PERM_CHANGE_SUBMISSIONS) + ]) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + # If they don't have `view_asset`, they don't have anything, and we're + # in good shape + self.assertFalse(self.asset.has_perm(self.anotheruser, PERM_VIEW_ASSET)) + + def test_editors_cannot_give_permissions(self): + self._grant_perm_as_logged_in_user('someuser', PERM_CHANGE_ASSET) + self.assertTrue(self.asset.has_perm(self.someuser, PERM_CHANGE_ASSET)) + self.client.login(username='someuser', password='someuser') + response = self._assign_perms_as_logged_in_user([ + ('anotheruser', PERM_CHANGE_ASSET), + ('anotheruser', PERM_CHANGE_SUBMISSIONS) + ]) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + self.assertFalse(self.asset.has_perm(self.anotheruser, PERM_VIEW_ASSET)) + + def test_anonymous_cannot_give_permissions(self): + self.client.logout() + response = self._assign_perms_as_logged_in_user([ + ('anotheruser', PERM_CHANGE_ASSET), + ('anotheruser', PERM_CHANGE_SUBMISSIONS) + ]) + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + self.assertFalse(self.asset.has_perm(self.anotheruser, PERM_VIEW_ASSET)) + + def test_managers_can_give_permissions(self): + self._grant_perm_as_logged_in_user('someuser', PERM_MANAGE_ASSET) + self.assertTrue(self.asset.has_perm(self.someuser, PERM_MANAGE_ASSET)) + self.client.login(username='someuser', password='someuser') + response = self._assign_perms_as_logged_in_user([ + ('anotheruser', PERM_CHANGE_ASSET), + ('anotheruser', PERM_CHANGE_SUBMISSIONS) + ]) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertTrue(self.asset.has_perm(self.anotheruser, PERM_CHANGE_ASSET)) + self.assertTrue(self.asset.has_perm(self.anotheruser, PERM_CHANGE_SUBMISSIONS)) + + def test_submission_assignments_ignored_for_non_survey_assets(self): + self.asset.asset_type = ASSET_TYPE_TEMPLATE + self.asset.save() + # FIXME: should handle ValidationError somewhere and return HTTP 405 + self.assertRaises( + ValidationError, + self._assign_perms_as_logged_in_user, + [ + ('someuser', PERM_VIEW_SUBMISSIONS), + ('anotheruser', PERM_VALIDATE_SUBMISSIONS), + ], + ) + self.assertFalse(self.asset.has_perm(self.someuser, PERM_VIEW_SUBMISSIONS)) + self.assertFalse(self.asset.has_perm(self.anotheruser, PERM_VALIDATE_SUBMISSIONS)) diff --git a/kpi/tests/api/v2/test_api_assets.py b/kpi/tests/api/v2/test_api_assets.py index 4342b2a04d..fc87ba8791 100644 --- a/kpi/tests/api/v2/test_api_assets.py +++ b/kpi/tests/api/v2/test_api_assets.py @@ -422,6 +422,8 @@ def test_assignable_permissions(self): 'url': 'http://testserver/api/v2/permissions/change_asset/'}, {'label': 'Edit submissions', 'url': 'http://testserver/api/v2/permissions/change_submissions/'}, + {'label': 'Manage project', + 'url': 'http://testserver/api/v2/permissions/manage_asset/'}, {'label': 'Validate submissions', 'url': 'http://testserver/api/v2/permissions/validate_submissions/'}, {'label': 'View form', @@ -451,6 +453,8 @@ def test_assignable_permissions(self): expected_response = [ {'label': 'Edit question', 'url': 'http://testserver/api/v2/permissions/change_asset/'}, + {'label': 'Manage question', + 'url': 'http://testserver/api/v2/permissions/manage_asset/'}, {'label': 'View question', 'url': 'http://testserver/api/v2/permissions/view_asset/'}, ] @@ -527,10 +531,6 @@ def setUp(self): Asset.CALCULATED_PERMISSIONS)) ) - @staticmethod - def absolute_reverse(*args, **kwargs): - return 'http://testserver/' + reverse(*args, **kwargs).lstrip('/') - def get_asset_file_content(self, url): response = self.client.get(url) streaming_content = [to_str(chunk) for chunk in diff --git a/kpi/tests/api/v2/test_api_collection_permission_assignment.py b/kpi/tests/api/v2/test_api_collection_permission_assignment.py index 8c73bab65d..e217d4a56f 100644 --- a/kpi/tests/api/v2/test_api_collection_permission_assignment.py +++ b/kpi/tests/api/v2/test_api_collection_permission_assignment.py @@ -1,4 +1,6 @@ # coding: utf-8 +import unittest + from django.contrib.auth.models import User from django.urls import reverse from rest_framework import status @@ -66,21 +68,21 @@ def test_owner_can_give_permissions(self): response = self._logged_user_gives_permission('someuser', PERM_VIEW_COLLECTION) self.assertEqual(response.status_code, status.HTTP_201_CREATED) - def test_viewers_can_not_give_permissions(self): + def test_viewers_cannot_give_permissions(self): self._logged_user_gives_permission('someuser', PERM_VIEW_COLLECTION) self.client.login(username='someuser', password='someuser') # Current user is now: `self.someuser` response = self._logged_user_gives_permission('anotheruser', PERM_VIEW_COLLECTION) self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) - def test_editors_can_give_permissions(self): + def test_editors_cannot_give_permissions(self): self._logged_user_gives_permission('someuser', PERM_CHANGE_COLLECTION) self.client.login(username='someuser', password='someuser') # Current user is now: `self.someuser` response = self._logged_user_gives_permission('anotheruser', PERM_VIEW_COLLECTION) - self.assertEqual(response.status_code, status.HTTP_201_CREATED) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) - def test_anonymous_can_not_give_permissions(self): + def test_anonymous_cannot_give_permissions(self): self.client.logout() response = self._logged_user_gives_permission('someuser', PERM_VIEW_COLLECTION) self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) @@ -215,6 +217,7 @@ def get_data_template(username_, permission_): response = self.client.post(url, data, format='json') return response + @unittest.skip('TODO: bring this back once we can assign manage_asset to someuser') def test_cannot_assign_permissions_to_owner(self): self._logged_user_gives_permission('someuser', PERM_CHANGE_COLLECTION) self.client.login(username='someuser', password='someuser') diff --git a/kpi/tests/api/v2/test_api_permissions.py b/kpi/tests/api/v2/test_api_permissions.py index f3fd8083c5..2f04231844 100644 --- a/kpi/tests/api/v2/test_api_permissions.py +++ b/kpi/tests/api/v2/test_api_permissions.py @@ -1,10 +1,11 @@ # coding: utf-8 from django.contrib.auth.models import User, Permission from django.urls import reverse +from django.utils import timezone from rest_framework import status -from kpi.constants import PERM_VIEW_ASSET, PERM_CHANGE_ASSET, \ - PERM_SHARE_ASSET +from kpi.constants import PERM_VIEW_ASSET, PERM_CHANGE_ASSET +from kpi.models import Asset, Collection, ObjectPermission from kpi.models.object_permission import get_anonymous_user from kpi.tests.kpi_test_case import KpiTestCase from kpi.urls.router_api_v2 import URL_NAMESPACE as ROUTER_URL_NAMESPACE @@ -510,136 +511,164 @@ def test_inherited_viewable_collection_not_deletable(self): self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) -# TODO Fix permissions tests -# class ApiAssignedPermissionsTestCase(KpiTestCase): -# """ -# An obnoxiously large amount of code to test that the endpoint for listing -# assigned permissions complies with the following rules: -# -# * Superusers see it all (thank goodness for pagination) -# * Anonymous users see nothing -# * Regular users see everything that concerns them, namely all -# permissions for all objects to which they have been assigned any -# permission -# -# See also `kpi.filters.KpiAssignedObjectPermissionsFilter` -# """ -# -# URL_NAMESPACE = ROUTER_URL_NAMESPACE -# -# def setUp(self): -# super().setUp() -# self.anon = get_anonymous_user() -# self.super = User.objects.get(username='admin') -# self.super_password = 'pass' -# self.someuser = User.objects.get(username='someuser') -# self.someuser_password = 'someuser' -# self.anotheruser = User.objects.get(username='anotheruser') -# self.anotheruser_password = 'anotheruser' -# -# # Find an unused, common PK for both Asset and Collection--useful for -# # catching bugs related to content types like -# # https://github.com/kobotoolbox/kpi/issues/2270 -# last_asset = Asset.objects.order_by('pk').last() -# last_collection = Collection.objects.order_by('pk').last() -# available_pk = 1 + max(last_asset.pk if last_asset else 1, -# last_collection.pk if last_collection else 1) -# -# def create_object_with_specific_pk(model, pk, **kwargs): -# obj = model() -# obj.pk = pk -# for k, v in kwargs.items(): -# setattr(obj, k, v) -# obj.save() -# return obj -# -# self.collection = create_object_with_specific_pk( -# Collection, -# available_pk, -# owner=self.someuser, -# ) -# self.asset = create_object_with_specific_pk( -# Asset, available_pk, owner=self.someuser, -# # perenially evil `auto_now_add` leaves the field NULL if a pk is -# # specified, leading to `IntegrityError` unless we set it manually -# date_created=timezone.now(), -# ) -# -# def test_anon_cannot_list_permissions(self): -# self.asset.assign_perm(self.anon, 'view_asset') -# self.assertTrue(self.anon.has_perm('view_asset', self.asset)) -# -# url = reverse(self._get_endpoint('objectpermission-list')) -# response = self.client.get(url) -# self.assertEqual(response.status_code, status.HTTP_200_OK) -# self.assertListEqual(response.data['results'], []) -# -# self.asset.remove_perm(self.anon, 'view_asset') -# self.assertFalse(self.anon.has_perm('view_asset', self.asset)) -# -# def test_user_sees_all_permissions_on_assigned_objects(self): -# self.asset.assign_perm(self.anotheruser, 'view_asset') -# self.assertTrue(self.anotheruser.has_perm('view_asset', self.asset)) -# -# self.client.login(username=self.anotheruser.username, -# password=self.anotheruser_password) -# -# url = reverse(self._get_endpoint('objectpermission-list')) -# response = self.client.get(url) -# self.assertEqual(response.status_code, status.HTTP_200_OK) -# -# returned_uids = [r['uid'] for r in response.data['results']] -# all_obj_perms = ObjectPermission.objects.filter_for_object(self.asset) -# -# self.assertTrue( -# set(returned_uids).issuperset( -# all_obj_perms.values_list('uid', flat=True) -# ) -# ) -# -# self.asset.remove_perm(self.anotheruser, 'view_asset') -# self.assertFalse(self.anotheruser.has_perm('view_asset', self.asset)) -# -# def test_user_cannot_see_permissions_on_unassigned_objects(self): -# self.asset.assign_perm(self.anotheruser, 'view_asset') -# self.assertTrue(self.anotheruser.has_perm('view_asset', self.asset)) -# -# self.client.login(username=self.anotheruser.username, -# password=self.anotheruser_password) -# -# url = reverse(self._get_endpoint('objectpermission-list')) -# response = self.client.get(url) -# self.assertEqual(response.status_code, status.HTTP_200_OK) -# -# returned_uids = [r['uid'] for r in response.data['results']] -# other_obj_perms = ObjectPermission.objects.filter_for_object( -# self.collection) -# -# self.assertFalse( -# set(returned_uids).intersection( -# other_obj_perms.values_list('uid', flat=True) -# ) -# ) -# -# self.asset.remove_perm(self.anotheruser, 'view_asset') -# self.assertFalse(self.anotheruser.has_perm('view_asset', self.asset)) -# -# def test_superuser_sees_all_permissions(self): -# self.asset.assign_perm(self.anotheruser, 'view_asset') -# self.assertTrue(self.anotheruser.has_perm('view_asset', self.asset)) -# -# self.client.login(username=self.super.username, -# password=self.super_password) -# -# url = reverse(self._get_endpoint('objectpermission-list')) -# response = self.client.get(url) -# self.assertEqual(response.status_code, status.HTTP_200_OK) -# -# returned_uids = [r['uid'] for r in response.data['results']] -# self.assertListEqual( -# sorted(returned_uids), -# sorted(ObjectPermission.objects.values_list('uid', flat=True)) -# ) -# -# self.asset.remove_perm(self.anotheruser, 'view_asset') -# self.assertFalse(self.anotheruser.has_perm('view_asset', self.asset)) +class ApiAssignedPermissionsTestCase(KpiTestCase): + """ + An obnoxiously large amount of code to test that the endpoint for listing + assigned permissions complies with the following rules: + + * Superusers see it all (and there is *no* pagination) + * Anonymous users see nothing + * Regular users see everything that concerns them, namely all + their own permissions and all the owners' permissions for all objects + to which they have been assigned any permission + + See also + kpi.utils.object_permission_helper.ObjectPermissionHelper.get_user_permission_assignments_queryset + """ + + # TODO: does this duplicate stuff in + # test_api_asset_permission_assignment.py / should it be moved there? + + URL_NAMESPACE = ROUTER_URL_NAMESPACE + + def setUp(self): + super().setUp() + self.anon = get_anonymous_user() + self.super = User.objects.get(username='admin') + self.super_password = 'pass' + self.someuser = User.objects.get(username='someuser') + self.someuser_password = 'someuser' + self.anotheruser = User.objects.get(username='anotheruser') + self.anotheruser_password = 'anotheruser' + + # Find an unused, common PK for both Asset and Collection--useful for + # catching bugs related to content types like + # https://github.com/kobotoolbox/kpi/issues/2270 + last_asset = Asset.objects.order_by('pk').last() + last_collection = Collection.objects.order_by('pk').last() + available_pk = 1 + max(last_asset.pk if last_asset else 1, + last_collection.pk if last_collection else 1) + + def create_object_with_specific_pk(model, pk, **kwargs): + obj = model() + obj.pk = pk + for k, v in kwargs.items(): + setattr(obj, k, v) + obj.save() + return obj + + self.collection = create_object_with_specific_pk( + Collection, + available_pk, + owner=self.someuser, + ) + self.asset = create_object_with_specific_pk( + Asset, available_pk, owner=self.someuser, + # perenially evil `auto_now_add` leaves the field NULL if a pk is + # specified, leading to `IntegrityError` unless we set it manually + date_created=timezone.now(), + ) + + def get_collection_perm_assignment_url(self, collection): + # TODO: REMOVE this when Collection is a type of Asset + return reverse( + self._get_endpoint('collection-permission-assignment-list'), + kwargs={'parent_lookup_collection': collection.uid} + ) + + def get_urls_for_collection_perm_assignment_objs( + self, perm_assignments, collection + ): + # TODO: REMOVE this when Collection is a type of Asset + return [ + self.absolute_reverse( + self._get_endpoint('collection-permission-assignment-detail'), + kwargs={'uid': uid, 'parent_lookup_collection': collection.uid}, + ) + for uid in perm_assignments.values_list('uid', flat=True) + ] + + def test_anon_only_sees_owner_permissions(self): + self.asset.assign_perm(self.anon, 'view_asset') + self.assertTrue(self.anon.has_perm('view_asset', self.asset)) + + url = self.get_asset_perm_assignment_list_url(self.asset) + response = self.client.get(url) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + owner_url = self.absolute_reverse( + self._get_endpoint('user-detail'), + kwargs={'username': self.asset.owner.username}, + ) + self.assertSetEqual( + set((a['user'] for a in response.data)), set((owner_url,)) + ) + + def test_user_sees_relevant_permissions_on_assigned_objects(self): + # A user with explicitly-assigned permissions should see their + # own permissions and the owner's permissions, but not permissions + # assigned to other users + self.asset.assign_perm(self.anotheruser, 'view_asset') + self.assertTrue(self.anotheruser.has_perm('view_asset', self.asset)) + + irrelevant_user = User.objects.create(username='mindyourown') + self.asset.assign_perm(irrelevant_user, 'view_asset') + + self.client.login(username=self.anotheruser.username, + password=self.anotheruser_password) + + url = self.get_asset_perm_assignment_list_url(self.asset) + response = self.client.get(url) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + returned_urls = [r['url'] for r in response.data] + all_obj_perms = ObjectPermission.objects.filter_for_object(self.asset) + relevant_obj_perms = all_obj_perms.filter( + user__in=(self.asset.owner, self.anotheruser), + permission__codename__in=self.asset.get_assignable_permissions( + with_partial=False + ), + ) + + self.assertListEqual( + sorted(returned_urls), + sorted( + self.get_urls_for_asset_perm_assignment_objs( + relevant_obj_perms, asset=self.asset + ) + ), + ) + + def test_user_cannot_see_permissions_on_unassigned_objects(self): + self.asset.assign_perm(self.anotheruser, 'view_asset') + self.assertTrue(self.anotheruser.has_perm('view_asset', self.asset)) + + self.client.login(username=self.anotheruser.username, + password=self.anotheruser_password) + + url = self.get_collection_perm_assignment_url(self.collection) + response = self.client.get(url) + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + + def test_superuser_sees_all_permissions(self): + self.asset.assign_perm(self.anotheruser, 'view_asset') + self.assertTrue(self.anotheruser.has_perm('view_asset', self.asset)) + + self.client.login(username=self.super.username, + password=self.super_password) + + url = self.get_asset_perm_assignment_list_url(self.asset) + response = self.client.get(url) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + returned_urls = [r['url'] for r in response.data] + all_obj_perms = ObjectPermission.objects.filter_for_object(self.asset) + + self.assertListEqual( + sorted(returned_urls), + sorted( + self.get_urls_for_asset_perm_assignment_objs( + all_obj_perms, asset=self.asset + ) + ), + ) diff --git a/kpi/tests/base_test_case.py b/kpi/tests/base_test_case.py index 4db363e6a4..4219445778 100644 --- a/kpi/tests/base_test_case.py +++ b/kpi/tests/base_test_case.py @@ -17,6 +17,10 @@ def _get_endpoint(self, endpoint): if self.URL_NAMESPACE else endpoint return endpoint + @staticmethod + def absolute_reverse(*args, **kwargs): + return 'http://testserver/' + reverse(*args, **kwargs).lstrip('/') + class BaseAssetTestCase(BaseTestCase): diff --git a/kpi/tests/kpi_test_case.py b/kpi/tests/kpi_test_case.py index 705416ec49..2cad9a9050 100644 --- a/kpi/tests/kpi_test_case.py +++ b/kpi/tests/kpi_test_case.py @@ -6,13 +6,15 @@ """ import re +from django.contrib.auth import get_user_model +from django.contrib.auth.models import Permission from django.urls import reverse from rest_framework import status # FIXME: Remove the following line when the permissions API is in place. from .base_test_case import BaseTestCase from .test_permissions import BasePermissionsTestCase -from ..models.asset import Asset +from ..models.asset import Asset, ObjectPermission from ..models.collection import Collection from ..models.object_permission import ObjectPermission @@ -51,6 +53,40 @@ def url_to_obj(self, url): obj = klass.objects.get(uid=uid) return obj + def obj_to_url(self, obj): + # Add more types as you need them + if isinstance(obj, ObjectPermission): + return reverse( + self._get_endpoint('asset-permission-assignment-detail'), + kwargs={'parent_lookup_asset': obj.asset.uid, 'uid': obj.uid}, + ) + if isinstance(obj, Permission): + return reverse( + self._get_endpoint('permission-detail'), + kwargs={'codename': obj.codename}, + ) + elif isinstance(obj, get_user_model()): + return reverse( + self._get_endpoint('user-detail'), + kwargs={'username': obj.username}, + ) + raise NotImplementedError + + def get_asset_perm_assignment_list_url(self, asset): + return reverse( + self._get_endpoint('asset-permission-assignment-list'), + kwargs={'parent_lookup_asset': asset.uid} + ) + + def get_urls_for_asset_perm_assignment_objs(self, perm_assignments, asset): + return [ + self.absolute_reverse( + self._get_endpoint('asset-permission-assignment-detail'), + kwargs={'uid': uid, 'parent_lookup_asset': asset.uid}, + ) + for uid in perm_assignments.values_list('uid', flat=True) + ] + def create_collection(self, name, owner=None, owner_password=None, **kwargs): if owner and owner_password: diff --git a/kpi/tests/test_assets.py b/kpi/tests/test_assets.py index 16143b1539..fb4e9d1fd5 100644 --- a/kpi/tests/test_assets.py +++ b/kpi/tests/test_assets.py @@ -9,8 +9,13 @@ from django.core.exceptions import ValidationError from django.test import TestCase -from kpi.constants import PERM_VIEW_ASSET, PERM_CHANGE_ASSET, PERM_SHARE_ASSET, \ - PERM_VIEW_COLLECTION, PERM_CHANGE_COLLECTION +from kpi.constants import ( + PERM_CHANGE_ASSET, + PERM_CHANGE_COLLECTION, + PERM_MANAGE_ASSET, + PERM_VIEW_ASSET, + PERM_VIEW_COLLECTION, +) from kpi.models import Asset from kpi.models import Collection from kpi.models.object_permission import get_all_objects_for_user @@ -650,33 +655,20 @@ def test_query_all_assets_user_can_access(self): def test_owner_can_edit_permissions(self): self.assertTrue(self.asset.owner.has_perm( - PERM_SHARE_ASSET, + PERM_MANAGE_ASSET, self.asset )) - def test_share_asset_permission_is_not_inherited(self): + def test_manage_asset_permission_is_not_inherited(self): # Give the child asset a different owner self.asset_in_coll.owner = User.objects.get(username='anotheruser') - # The change permission is inherited; prevent it from allowing - # users to edit permissions - self.asset_in_coll.editors_can_change_permissions = False self.asset_in_coll.save() # Ensure the parent's owner can't change permissions on the child self.assertFalse(self.coll.owner.has_perm( - PERM_SHARE_ASSET, + PERM_MANAGE_ASSET, self.asset_in_coll )) - def test_change_permission_does_not_provide_share_permission(self): - anotheruser = User.objects.get(username='anotheruser') - self.assertFalse(anotheruser.has_perm( - PERM_CHANGE_ASSET, self.asset)) - # Grant the change permission and make sure it provides - # share_asset - self.asset.assign_perm(anotheruser, PERM_CHANGE_ASSET) - self.assertFalse(anotheruser.has_perm( - PERM_SHARE_ASSET, self.asset)) - def test_anonymous_view_permission_on_standalone_asset(self): # Grant self.assertFalse(AnonymousUser().has_perm( diff --git a/kpi/tests/test_collections.py b/kpi/tests/test_collections.py index e182c07631..be06cd2b3c 100644 --- a/kpi/tests/test_collections.py +++ b/kpi/tests/test_collections.py @@ -408,37 +408,6 @@ def test_object_permissions_are_deleted_with_collection(self): 0 ) - def test_owner_can_edit_permissions(self): - self.assertTrue(self.standalone_coll.owner.has_perm( - 'share_collection', - self.standalone_coll - )) - - def test_share_collection_permission_is_not_inherited(self): - # Make a child collection whose owner is different than its parent's - coll = Collection.objects.create( - name="anotheruser's collection", - owner=self.anotheruser, - parent=self.standalone_coll, - # The change permission is inherited; prevent it from allowing - # users to edit permissions - editors_can_change_permissions=False - ) - # Ensure the parent's owner can't change permissions on the child - self.assertFalse(self.standalone_coll.owner.has_perm( - 'share_collection', - coll - )) - - def test_change_permission_does_not_provide_share_permission(self): - self.assertFalse(self.someuser.has_perm( - PERM_CHANGE_COLLECTION, self.standalone_coll)) - # Grant the change permission and make sure it does not provide - # share_collection - self.standalone_coll.assign_perm(self.someuser, PERM_CHANGE_COLLECTION) - self.assertFalse(self.someuser.has_perm( - 'share_collection', self.standalone_coll)) - def test_anonymous_view_permission_on_standalone_collection(self): # Grant self.assertFalse(AnonymousUser().has_perm( diff --git a/kpi/tests/test_permissions.py b/kpi/tests/test_permissions.py index 3f629172a3..98c88d2df1 100644 --- a/kpi/tests/test_permissions.py +++ b/kpi/tests/test_permissions.py @@ -2,11 +2,20 @@ from django.contrib.auth.models import User, AnonymousUser from django.test import TestCase -from kpi.constants import PERM_VIEW_ASSET, PERM_CHANGE_ASSET, PERM_ADD_SUBMISSIONS, \ - PERM_VIEW_SUBMISSIONS, PERM_CHANGE_SUBMISSIONS, PERM_VALIDATE_SUBMISSIONS, \ - PERM_SHARE_ASSET, PERM_DELETE_ASSET, PERM_SHARE_SUBMISSIONS, \ - PERM_DELETE_SUBMISSIONS, PERM_VIEW_COLLECTION, PERM_CHANGE_COLLECTION, \ - PERM_PARTIAL_SUBMISSIONS +from kpi.constants import ( + PERM_VIEW_ASSET, + PERM_CHANGE_ASSET, + PERM_MANAGE_ASSET, + PERM_ADD_SUBMISSIONS, + PERM_VIEW_SUBMISSIONS, + PERM_CHANGE_SUBMISSIONS, + PERM_VALIDATE_SUBMISSIONS, + PERM_DELETE_ASSET, + PERM_DELETE_SUBMISSIONS, + PERM_VIEW_COLLECTION, + PERM_CHANGE_COLLECTION, + PERM_PARTIAL_SUBMISSIONS, +) from kpi.exceptions import BadPermissionsException from ..models.asset import Asset from ..models.collection import Collection @@ -162,8 +171,7 @@ def setUp(self): PERM_CHANGE_SUBMISSIONS, PERM_DELETE_ASSET, PERM_DELETE_SUBMISSIONS, - PERM_SHARE_ASSET, - PERM_SHARE_SUBMISSIONS, + PERM_MANAGE_ASSET, PERM_VALIDATE_SUBMISSIONS, PERM_VIEW_ASSET, PERM_VIEW_SUBMISSIONS, @@ -171,7 +179,6 @@ def setUp(self): self.collection_owner_permissions = [ PERM_CHANGE_COLLECTION, 'delete_collection', - 'share_collection', PERM_VIEW_COLLECTION ] @@ -247,9 +254,6 @@ def test_implied_asset_grant_permissions(self): asset = self.admin_asset grantee = self.someuser - # Prevent extra `share_` permissions from being assigned - asset.editors_can_change_permissions = False - for explicit, implied in implications.items(): # Make sure the slate is clean self.assertListEqual(list(asset.get_perms(grantee)), []) @@ -274,33 +278,31 @@ def test_remove_implied_asset_permissions(self): """ asset = self.admin_asset grantee = self.someuser + self.assertListEqual(list(asset.get_perms(grantee)), []) - # Prevent extra `share_` permissions from being assigned - asset.editors_can_change_permissions = False - - common_expected_lineage = [ - # (___) - # (o o)___________________________________________/ - # @@ ` \ - # \ __________________________________________, / - # // // - # ^^ ^^ - PERM_VIEW_ASSET, PERM_VIEW_SUBMISSIONS] - implied_permissions = [PERM_CHANGE_SUBMISSIONS, PERM_VALIDATE_SUBMISSIONS] - - for implied_permission in implied_permissions: - # Copy common lineage before appending submissions. - expected_lineage = list(common_expected_lineage) - expected_lineage.append(implied_permission) + asset.assign_perm(grantee, PERM_CHANGE_SUBMISSIONS) + expected_perms = [ + PERM_VIEW_ASSET, + PERM_VIEW_SUBMISSIONS, + PERM_CHANGE_SUBMISSIONS, + ] + self.assertListEqual( + sorted(asset.get_perms(grantee)), sorted(expected_perms) + ) + asset.remove_perm(grantee, PERM_VIEW_ASSET) + self.assertListEqual(list(asset.get_perms(grantee)), []) - self.assertListEqual(list(asset.get_perms(grantee)), []) - # Assigning the tail should bring the head and body along - asset.assign_perm(grantee, expected_lineage[-1]) - self.assertListEqual( - sorted(asset.get_perms(grantee)), sorted(expected_lineage)) - # Removing the head should remove the body and tail as well - asset.remove_perm(grantee, expected_lineage[0]) - self.assertListEqual(list(asset.get_perms(grantee)), []) + asset.assign_perm(grantee, PERM_VALIDATE_SUBMISSIONS) + expected_perms = [ + PERM_VIEW_ASSET, + PERM_VIEW_SUBMISSIONS, + PERM_VALIDATE_SUBMISSIONS, + ] + self.assertListEqual( + sorted(asset.get_perms(grantee)), sorted(expected_perms) + ) + asset.remove_perm(grantee, PERM_VIEW_ASSET) + self.assertListEqual(list(asset.get_perms(grantee)), []) def test_implied_asset_deny_permissions(self): """ @@ -314,9 +316,6 @@ def test_implied_asset_deny_permissions(self): collection = self.admin_collection grantee = self.someuser - # Prevent extra `share_` permissions from being assigned - asset.editors_can_change_permissions = False - collection.assets.add(asset) self.assertListEqual(list(collection.get_perms(grantee)), []) self.assertListEqual(list(asset.get_perms(grantee)), []) @@ -341,6 +340,7 @@ def test_implied_asset_deny_permissions(self): PERM_CHANGE_ASSET, PERM_CHANGE_SUBMISSIONS, PERM_DELETE_SUBMISSIONS, + PERM_MANAGE_ASSET, PERM_PARTIAL_SUBMISSIONS, PERM_VALIDATE_SUBMISSIONS, PERM_VIEW_ASSET, @@ -368,6 +368,7 @@ def test_contradict_implied_asset_deny_permissions(self): PERM_CHANGE_ASSET, PERM_CHANGE_SUBMISSIONS, PERM_DELETE_SUBMISSIONS, + PERM_MANAGE_ASSET, PERM_PARTIAL_SUBMISSIONS, PERM_VALIDATE_SUBMISSIONS, PERM_VIEW_ASSET, @@ -389,6 +390,7 @@ def test_contradict_implied_asset_deny_permissions(self): (PERM_CHANGE_ASSET, True), (PERM_CHANGE_SUBMISSIONS, False), (PERM_DELETE_SUBMISSIONS, True), + (PERM_MANAGE_ASSET, True), (PERM_VALIDATE_SUBMISSIONS, True), (PERM_VIEW_ASSET, False), (PERM_VIEW_SUBMISSIONS, False) @@ -399,9 +401,6 @@ def test_implied_collection_permissions(self): grantee = self.someuser collection = self.admin_collection - # Prevent extra `share_` permissions from being assigned - collection.editors_can_change_permissions = False - self.assertListEqual(list(collection.get_perms(grantee)), []) collection.assign_perm(grantee, PERM_CHANGE_COLLECTION) self.assertListEqual( @@ -558,6 +557,7 @@ def test_copy_permissions_between_objects_different_owner(self): PERM_CHANGE_ASSET, PERM_CHANGE_SUBMISSIONS, PERM_DELETE_SUBMISSIONS, + PERM_MANAGE_ASSET, PERM_VALIDATE_SUBMISSIONS, PERM_VIEW_ASSET, PERM_VIEW_SUBMISSIONS diff --git a/kpi/utils/object_permission_helper.py b/kpi/utils/object_permission_helper.py index fc9044f0ea..32751a5a38 100644 --- a/kpi/utils/object_permission_helper.py +++ b/kpi/utils/object_permission_helper.py @@ -1,34 +1,32 @@ # coding: utf-8 from django.conf import settings -from kpi.constants import PERM_SHARE_SUBMISSIONS, PERM_FROM_KC_ONLY +from kpi.constants import PERM_MANAGE_ASSET, PERM_FROM_KC_ONLY class ObjectPermissionHelper: @staticmethod - def user_can_share(affected_object, user_object, codename=''): + def user_can_share(affected_object, user_object): """ - Return `True` if `user` is allowed to grant and revoke - `codename` on `affected_object`. For `Collection`, this is always - the same as checking that `user` has the - `share_collection` permission on `affected_object`. For `Asset`, - the result is determined by either `share_asset` or - `share_submissions`, depending on the `codename`. + TODO: trash this method once Collection is merged into Asset + + Return `True` if `user_object` is the owner of `affected_object` (to + support Collection) or if `user_object` has the `manage_asset` + permission :type affected_object: :py:class:Asset or :py:class:Collection :type user_object: auth.User - :type codename: str :rtype bool """ # affected_object can be deferred which doesn't return the expected # model_name. Using `concrete_model` does. model_name = affected_object._meta.concrete_model._meta.model_name - if model_name == 'asset' and codename.endswith('_submissions'): - share_permission = PERM_SHARE_SUBMISSIONS - else: - share_permission = 'share_{}'.format(model_name) - return affected_object.has_perm(user_object, share_permission) + if model_name not in ('asset', 'collection'): + raise NotImplementedError + return affected_object.owner == user_object or affected_object.has_perm( + user_object, PERM_MANAGE_ASSET + ) @classmethod def get_user_permission_assignments_queryset(cls, affected_object, user): diff --git a/kpi/views/v1/asset.py b/kpi/views/v1/asset.py index 8d07f229bc..13d91b2c53 100644 --- a/kpi/views/v1/asset.py +++ b/kpi/views/v1/asset.py @@ -4,7 +4,7 @@ from rest_framework.decorators import action from rest_framework.response import Response -from kpi.constants import CLONE_ARG_NAME, PERM_SHARE_ASSET, PERM_VIEW_ASSET +from kpi.constants import CLONE_ARG_NAME, PERM_MANAGE_ASSET, PERM_VIEW_ASSET from kpi.models import Asset from kpi.serializers.v1.asset import AssetSerializer, AssetListSerializer from kpi.views.v2.asset import AssetViewSet as AssetViewSetV2 @@ -173,7 +173,7 @@ def permissions(self, request, uid): response = {} http_status = status.HTTP_204_NO_CONTENT - if user.has_perm(PERM_SHARE_ASSET, target_asset) and \ + if user.has_perm(PERM_MANAGE_ASSET, target_asset) and \ user.has_perm(PERM_VIEW_ASSET, source_asset): if not target_asset.copy_permissions_from(source_asset): http_status = status.HTTP_400_BAD_REQUEST diff --git a/kpi/views/v1/object_permission.py b/kpi/views/v1/object_permission.py index f753d0ae47..45f435dd92 100644 --- a/kpi/views/v1/object_permission.py +++ b/kpi/views/v1/object_permission.py @@ -16,14 +16,15 @@ class ObjectPermissionViewSet(NoUpdateModelViewSet): filter_backends = (KpiAssignedObjectPermissionsFilter, ) def perform_create(self, serializer): - # Make sure the requesting user has the share_ permission on + # Make sure the requesting user has the manage_ permission on # the affected object with transaction.atomic(): affected_object = serializer.validated_data['content_object'] codename = serializer.validated_data['permission'].codename - if not ObjectPermissionHelper.user_can_share(affected_object, - self.request.user, - codename): + if not ObjectPermissionHelper.user_can_share( + affected_object, + self.request.user, + ): raise exceptions.PermissionDenied() serializer.save() @@ -35,14 +36,15 @@ def perform_destroy(self, instance): self.request.method, detail='Cannot delete inherited permissions.' ) - # Make sure the requesting user has the share_ permission on + # Make sure the requesting user has the manage_ permission on # the affected object with transaction.atomic(): affected_object = instance.content_object codename = instance.permission.codename - if not ObjectPermissionHelper.user_can_share(affected_object, - self.request.user, - codename): + if not ObjectPermissionHelper.user_can_share( + affected_object, + self.request.user, + ): raise exceptions.PermissionDenied() instance.content_object.remove_perm( instance.user, diff --git a/kpi/views/v2/asset_permission_assignment.py b/kpi/views/v2/asset_permission_assignment.py index 76241be398..21e9b83963 100644 --- a/kpi/views/v2/asset_permission_assignment.py +++ b/kpi/views/v2/asset_permission_assignment.py @@ -11,7 +11,7 @@ from kpi.constants import ( CLONE_ARG_NAME, - PERM_SHARE_ASSET, + PERM_MANAGE_ASSET, PERM_VIEW_ASSET, ) from kpi.deployment_backends.kc_access.utils import \ @@ -160,6 +160,8 @@ class AssetPermissionAssignmentViewSet(AssetNestedObjectViewsetMixin, serializer_class = AssetPermissionAssignmentSerializer permission_classes = (AssetNestedObjectPermission,) pagination_class = None + # filter_backends = Just kidding! Look at this instead: + # kpi.utils.object_permission_helper.ObjectPermissionHelper.get_user_permission_assignments_queryset @action(detail=False, methods=['POST'], renderer_classes=[renderers.JSONRenderer], url_path='bulk') @@ -215,7 +217,7 @@ def clone(self, request, *args, **kwargs): source_asset = get_object_or_404(Asset, uid=source_asset_uid) user = request.user - if user.has_perm(PERM_SHARE_ASSET, self.asset) and \ + if user.has_perm(PERM_MANAGE_ASSET, self.asset) and \ user.has_perm(PERM_VIEW_ASSET, source_asset): if not self.asset.copy_permissions_from(source_asset): http_status = status.HTTP_400_BAD_REQUEST diff --git a/kpi/views/v2/collection_permission_assignment.py b/kpi/views/v2/collection_permission_assignment.py index 0fe21022d9..ac0c1bebda 100644 --- a/kpi/views/v2/collection_permission_assignment.py +++ b/kpi/views/v2/collection_permission_assignment.py @@ -9,8 +9,7 @@ from rest_framework.response import Response from rest_framework_extensions.mixins import NestedViewSetMixin -from kpi.constants import CLONE_ARG_NAME, PERM_SHARE_COLLECTION, \ - PERM_VIEW_COLLECTION +from kpi.constants import CLONE_ARG_NAME, PERM_VIEW_COLLECTION from kpi.models.collection import Collection from kpi.models.object_permission import ObjectPermission from kpi.permissions import CollectionNestedObjectPermission @@ -184,7 +183,7 @@ def clone(self, request, *args, **kwargs): source_collection = get_object_or_404(Collection, uid=source_collection_uid) user = request.user - if user.has_perm(PERM_SHARE_COLLECTION, self.collection) and \ + if ObjectPermissionHelper.user_can_share(self.collection, user) and \ user.has_perm(PERM_VIEW_COLLECTION, source_collection): if not self.collection.copy_permissions_from(source_collection): http_status = status.HTTP_400_BAD_REQUEST From ed48170de329e3714354d6a83e47bb5a6b7222a8 Mon Sep 17 00:00:00 2001 From: "John N. Milner" Date: Wed, 13 Jan 2021 04:43:59 -0500 Subject: [PATCH 2/3] Yes, you really do need to create the permission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …manually inside the data migration --- .../0028_assign_manage_asset_permissions.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/kpi/migrations/0028_assign_manage_asset_permissions.py b/kpi/migrations/0028_assign_manage_asset_permissions.py index 1f971725e2..afd97ae018 100644 --- a/kpi/migrations/0028_assign_manage_asset_permissions.py +++ b/kpi/migrations/0028_assign_manage_asset_permissions.py @@ -5,6 +5,20 @@ from django.db import migrations +def create_new_perms(apps): + """ + The new `delete_submissions` permission does not exist when running this + migration for the first time. Django runs migrations in a transaction and + new permissions are not created until after the transaction is completed. + + See https://stackoverflow.com/a/40092780/1141214 + """ + for app_config in apps.get_app_configs(): + app_config.models_module = True + create_permissions(app_config, apps=apps, verbosity=0) + app_config.models_module = None + + def grant_object_level_perms(apps): """ Grant `manage_asset` to the owner of every asset @@ -57,6 +71,7 @@ def forwards_func(apps, schema_editor): 'minutes on large databases...\n' ) sys.stderr.flush() + create_new_perms(apps) grant_object_level_perms(apps) From 2c8b2ed64e3fc86e69355d3262f2cdb0ad7f117a Mon Sep 17 00:00:00 2001 From: "John N. Milner" Date: Wed, 20 Jan 2021 19:03:21 -0500 Subject: [PATCH 3/3] Fix typos and code style issues --- .../permissions/userAssetPermsEditor.es6 | 2 +- .../0028_assign_manage_asset_permissions.py | 2 +- kpi/models/asset.py | 2 +- kpi/models/object_permission.py | 14 +++++------ kpi/views/v1/asset.py | 19 +++++++++++---- .../v2/collection_permission_assignment.py | 24 +++++++++++++------ 6 files changed, 41 insertions(+), 22 deletions(-) diff --git a/jsapp/js/components/permissions/userAssetPermsEditor.es6 b/jsapp/js/components/permissions/userAssetPermsEditor.es6 index 5acb10c3a4..2535aae362 100644 --- a/jsapp/js/components/permissions/userAssetPermsEditor.es6 +++ b/jsapp/js/components/permissions/userAssetPermsEditor.es6 @@ -191,7 +191,7 @@ class UserAssetPermsEditor extends React.Component { stateObj.submissionsViewPartialUsers = []; } - // `formManage` implies every other permission (except parial permissions) + // `formManage` implies every other permission (except partial permissions) if (stateObj.formManage) { stateObj.formView = true; stateObj.formEdit = true; diff --git a/kpi/migrations/0028_assign_manage_asset_permissions.py b/kpi/migrations/0028_assign_manage_asset_permissions.py index afd97ae018..f7f19a82cf 100644 --- a/kpi/migrations/0028_assign_manage_asset_permissions.py +++ b/kpi/migrations/0028_assign_manage_asset_permissions.py @@ -7,7 +7,7 @@ def create_new_perms(apps): """ - The new `delete_submissions` permission does not exist when running this + The new `manage_asset` permission does not exist when running this migration for the first time. Django runs migrations in a transaction and new permissions are not created until after the transaction is completed. diff --git a/kpi/models/asset.py b/kpi/models/asset.py index dc5af526e5..c8ae4e18a6 100644 --- a/kpi/models/asset.py +++ b/kpi/models/asset.py @@ -607,7 +607,7 @@ class Meta: ( p for p in ASSIGNABLE_PERMISSIONS - if p != PERM_MANAGE_ASSET and p != PERM_PARTIAL_SUBMISSIONS + if p not in (PERM_MANAGE_ASSET, PERM_PARTIAL_SUBMISSIONS) ) ), PERM_ADD_SUBMISSIONS: (PERM_VIEW_ASSET,), diff --git a/kpi/models/object_permission.py b/kpi/models/object_permission.py index b950720653..67d9e58cde 100644 --- a/kpi/models/object_permission.py +++ b/kpi/models/object_permission.py @@ -441,16 +441,16 @@ def _get_effective_perms( # Add the calculated `delete_` permission for the owner content_type = ContentType.objects.get_for_model(self) - if self.owner is not None and ( - user is None or user.pk == self.owner.pk) and ( - codename is None or codename.startswith('delete_') + if ( + self.owner is not None + and (user is None or user.pk == self.owner.pk) + and (codename is None or codename.startswith('delete_')) ): matching_permissions = self.__get_permissions_for_content_type( - content_type.pk, codename__startswith='delete_') + content_type.pk, codename__startswith='delete_' + ) for perm_pk, perm_codename in matching_permissions: - if (codename is not None and - perm_codename != codename - ): + if codename is not None and perm_codename != codename: # If the caller specified `codename`, skip anything that # doesn't match exactly. Necessary because `Asset` has # `delete_submissions` in addition to `delete_asset` diff --git a/kpi/views/v1/asset.py b/kpi/views/v1/asset.py index 13d91b2c53..584a216fcb 100644 --- a/kpi/views/v1/asset.py +++ b/kpi/views/v1/asset.py @@ -165,19 +165,28 @@ def get_serializer_class(self): def get_serializer_context(self): return super(AssetViewSetV2, self).get_serializer_context() - @action(detail=True, methods=["PATCH"], renderer_classes=[renderers.JSONRenderer]) + @action( + detail=True, + methods=["PATCH"], + renderer_classes=[renderers.JSONRenderer], + ) def permissions(self, request, uid): target_asset = self.get_object() - source_asset = get_object_or_404(Asset, uid=request.data.get(CLONE_ARG_NAME)) + source_asset = get_object_or_404( + Asset, uid=request.data.get(CLONE_ARG_NAME) + ) user = request.user response = {} http_status = status.HTTP_204_NO_CONTENT - if user.has_perm(PERM_MANAGE_ASSET, target_asset) and \ - user.has_perm(PERM_VIEW_ASSET, source_asset): + if user.has_perm(PERM_MANAGE_ASSET, target_asset) and user.has_perm( + PERM_VIEW_ASSET, source_asset + ): if not target_asset.copy_permissions_from(source_asset): http_status = status.HTTP_400_BAD_REQUEST - response = {"detail": "Source and destination objects don't seem to have the same type"} + response = { + "detail": "Source and destination objects don't seem to have the same type" + } else: raise exceptions.PermissionDenied() diff --git a/kpi/views/v2/collection_permission_assignment.py b/kpi/views/v2/collection_permission_assignment.py index ac0c1bebda..2c64deaf3e 100644 --- a/kpi/views/v2/collection_permission_assignment.py +++ b/kpi/views/v2/collection_permission_assignment.py @@ -175,20 +175,30 @@ def bulk_assignments(self, request, *args, **kwargs): # see all permissions. return self.list(request, *args, **kwargs) - @action(detail=False, methods=['PATCH'], - renderer_classes=[renderers.JSONRenderer]) + @action( + detail=False, + methods=['PATCH'], + renderer_classes=[renderers.JSONRenderer], + ) def clone(self, request, *args, **kwargs): source_collection_uid = self.request.data[CLONE_ARG_NAME] - source_collection = get_object_or_404(Collection, uid=source_collection_uid) + source_collection = get_object_or_404( + Collection, uid=source_collection_uid + ) user = request.user - if ObjectPermissionHelper.user_can_share(self.collection, user) and \ - user.has_perm(PERM_VIEW_COLLECTION, source_collection): + if ObjectPermissionHelper.user_can_share( + self.collection, user + ) and user.has_perm(PERM_VIEW_COLLECTION, source_collection): if not self.collection.copy_permissions_from(source_collection): http_status = status.HTTP_400_BAD_REQUEST - response = {'detail': _("Source and destination objects don't " - "seem to have the same type")} + response = { + 'detail': _( + "Source and destination objects don't " + "seem to have the same type" + ) + } return Response(response, status=http_status) else: raise exceptions.PermissionDenied()