Add generic ModelPermissionTester based on PagePermissionTester

[laymonage]

Our recent discoveries seem to suggest that we won't get rid of PagePermissionTester. If that's the case, it's likely that we want to use the same approach for non-page models.

This PR adds a generic ModelPermissionTester that works like PagePermissionTester, which does permission checks that also take into account the current circumstances of the object (e.g. workflows, lock state, etc.) and not just the Permissions.

Marking as draft since I'm still not convinced of the approach. A few thoughts:

• Page views do not use permission policies (at least not directly) to guard the view. Instead, it relies on PagePermissionTester, which now in turn relies on PagePermissionPolicy. PagePermissionTester is used for permission checks both at the beginning of a view (e.g. raise PermissionDenied at the start of dispatch() if the permissions do not satisfy) and for "action"-specific checks for the object, e.g. whether the user can lock, unschedule, etc. which may or may not map directly to a Permission.
• For snippets (and non-page models in general), the permission checks are done using ModelPermissionPolicy via PermissionCheckedMixin. This combo basically only checks whether the user has the specified permission codename for the model, without taking into account the object's circumstances.
• To allow for more-informed permission checks based on the object, I refactored PermissionCheckedMixin to have separate user_has_permission() method in 0f0ecb4. Then, it is overridden in CreateEditViewOptionalFeaturesMixin to allow similar logic from PagePermissionTester to short-circuit/build upon the checks done by the permission policy:

  wagtail/wagtail/admin/views/generic/mixins.py

  Lines 269 to 304 in 61f0f4d

  	def user_has_permission(self, permission):
  	user = self.request.user
  	if user.is_superuser:
  	return True
  	# Workflow lock/unlock methods take precedence before the base
  	# "lock" and "unlock" permissions -- see PagePermissionTester for reference
  	if permission == "lock" and self.current_workflow_task:
  	return self.current_workflow_task.user_can_lock(self.object, user)
  	if permission == "unlock":
  	# Allow unlocking even if the user does not have the 'unlock' permission
  	# if they are the user who locked the object
  	if self.object.locked_by_id == user.pk:
  	return True
  	if self.current_workflow_task:
  	return self.current_workflow_task.user_can_unlock(self.object, user)
  	# Check with base PermissionCheckedMixin logic
  	has_base_permission = super().user_has_permission(permission)
  	if has_base_permission:
  	return True
  	# Allow access to the editor if the current workflow task allows it,
  	# even if the user does not normally have edit access. Users with edit
  	# permissions can always edit regardless what this method returns --
  	# see Task.user_can_access_editor() for reference
  	if (
  	permission == "change"
  	and self.current_workflow_task
  	and self.current_workflow_task.user_can_access_editor(
  	self.object, self.request.user
  	)
  	):
  	return True
  	return False

• If we were to add this generic ModelPermissionTester, where and how does it fit into PermissionCheckedMixin (or any other view class)? Also, note that PermissionCheckedMixin rely on strings that map directly to the Permission's action part of the codename (e.g. permission_required = "change", while {Foo}PermissionTester relies on the method names, e.g. can_edit().
• As a follow-up, I feel like the bigger question is, where does the {Foo}PermissionTester fit in the grand scheme of things, now that we have permission policies for all Wagtail-managed models? Is PermissionTester really the name we want to call this kind of classes?
  • I'm thinking it should live somewhere close to the permission policy. Maybe as a child class within the permission policy, e.g. maybe you could do like

    class PagePermissionPolicy(OwnershipPermissionPolicy):
        tester_class = PagePermissionTester
    
        def user_tester_for_instance(self, user, instance)
            tester = self.tester_class(user, instance)
            # or change the tester's constructor signature to accept a permission policy too
            tester.permission_policy = self
            return tester

• The {Foo}PermissionTester currently accept the user and the object instance in the __init__. I encountered a problem with the "create" view, where self.object is None, so the permission tester can't work properly.
  • For pages, what happens is usually a PagePermissionTester instance is created using the parent page, and then a method like tester.can_add_subpage() is used.
  • For snippets, the correct logic would be to fall back to just check based on the model and permissions, but this isn't possible as the tester doesn't have access to the model (if you pass None as the object).
  • The above idea of encapsulating the tester within the policy would allow the tester to access the model via self.permission_policy.model (or the policy can also supply it during instantiation). This can be used for fallback logic when the object is None.
  • That said, allowing {Foo}PermissionTester to work with None as the object seems like an anti-pattern. Perhaps the view should decide to use the plain policy when self.object is None, and use the tester when the self.object is available? But this means that the object needs to be fetched first... maybe just separate the logic/check based on what the view is (create vs. edit)?
• Page has a permissions_for_user() method that returns the PagePermissionTester object for the page and user, which is convenient. We can't do the same for non-page models (unless we patch the method to the model, which seems hacky). Using ModelPermissionTester(user, object) works though, and I'd say it isn't so bad.

Please check the following:

• Do the tests still pass?¹
• Does the code comply with the style guide?
  • Run make lint from the Wagtail root.
• For Python changes: Have you added tests to cover the new/fixed behaviour?
• For front-end changes: Did you test on all of Wagtail’s supported environments?²
  • Please list the exact browser and operating system versions you tested:
  • Please list which assistive technologies ³ you tested:
• For new features: Has the documentation been updated accordingly?

Please describe additional details for testing this change.

Footnotes

1. Development Testing ↩

2. Browser and device support ↩

3. Accessibility Target ↩

[squash-labs]

Manage this branch in Squash

Test this branch here: https://laymonagegeneric-tester-4njs4.squash.io