Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC 32: Extensible view restrictions #32

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

RFC 32: Extensible view restrictions #32

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ccbdb6
RFC: Extensible view restrictions
gasman Jan 22, 2019
8a4de76
Fill in RFC number 32 (Extensible view restrictions)
gasman Jan 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions text/032-extensible-view-restrictions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
# RFC 32: Extensible view restrictions

* RFC: 32
* Author: Matt Westcott
* Created: 2019-01-22
* Last Modified: 2019-01-22

## Abstract

Wagtail provides the ability to place view restrictions on pages and documents (at the collection level), requiring users to pass an authentication step before they are allowed to view that resource. As of Wagtail 2.4, three types of restriction are supported:

* Resource is password-protected by an ad-hoc shared password
* Resource is only available to logged-in users
* Resource is only available to users in specified groups

The logic for these restriction types is currently hard-coded into the Wagtail core and admin interface, and there is no supported way to add new restriction types. This RFC proposes an extension mechanism so that new restriction types can be defined within user code or third-party extensions, with no change to Wagtail required.

The primary motivation for this feature is to support permission rules that are too complex to express in Django's user / group model, such as: "this resource is only available to users who are members of an organisation that has an active subscription to a product with features X or Y". However, it could equally be used to implement restrictions that are unrelated to login accounts, such as:

* Resource is only available to users in a given IP range
* Resource is only available between 9am and 5pm each day
* Resource is only available over HTTPS

## Specification

### Models

`wagtail.core.models` provides two models `PageViewRestriction` and `CollectionViewRestriction`, both descending from the abstract model `BaseViewRestriction`, which represent a view restriction on a page and on a collection respectively. Currently, these models include a `restriction_type` field (one of `'password'`, `'login'` or `'groups'`) and `password` and `groups` fields to store additional parameters for password-based and group-based restrictions.

In the updated implementation, each restriction type will be implemented as a subclass of PageViewRestriction or CollectionViewRestriction via multi-table inheritance, with type-specific parameters stored in the subclass's specific table and a `content_type` parameter on the base table to identify the restriction type. Any concrete subclass of PageViewRestriction or CollectionViewRestriction will be automatically 'discovered' by Wagtail's view restrictions system, and exposed in the admin interface where appropriate - e.g. offered as an available restriction type when setting up a new view restriction.

The existing built-in restriction types (password, login and groups) will be reimplemented as subclasses of `PageViewRestriction` and `CollectionViewRestriction`, giving the following inheritance tree:

BaseViewRestriction (abstract)
|
|- PageViewRestriction
| |
| |- PasswordPageViewRestriction
| |- LoginPageViewRestriction
| |- GroupPageViewRestriction
| '- (any custom view restriction types defined for pages)
|
'- CollectionViewRestriction
|
|- PasswordCollectionViewRestriction
|- LoginCollectionViewRestriction
|- GroupCollectionViewRestriction
'- (any custom view restriction types defined for collections)

It is expected that the corresponding restriction type models for pages and collections (e.g. `PasswordPageViewRestriction` and `PasswordCollectionViewRestriction`) will have a considerable amount of shared logic and be implemented through a common abstract / mixin class, not shown in the above hierarchy.

> Q: This means that any external code that defines a new restriction type will generally have to define two classes - one for pages and one for collections. Furthermore, if a future version of Wagtail introduces a new kind of permissionable resource alongside pages and collections, the restriction type won't be available for that kind of resource until the external code is explicitly updated to support it. Wouldn't it be neater to define a single (possibly-abstract) class for the restriction type, and have it automatically apply to pages, collections and any other kinds of resources?
>
> A: No, it's better to have explicit handling for each kind of resource. Since the definition of what a restriction type can do has been left somewhat open-ended, we can't assume that the implementation will look the same for both pages and documents, or that a particular restriction type is meaningful for both pages and documents. For example, PasswordPageViewRestriction will support serving up a custom variant of the 'real' page as the password prompt view - e.g. displaying a preview paragraph in place of the full text - which can't be done for documents.

### Front-end behaviour

Subclasses of `PageViewRestriction` should implement a method `check(self, request, page)` which will be called just before serving a page which is covered by that view restriction, passing the `HttpRequest` and the `Page` instance. This method shall return either `None` (to allow the page to be served as normal) or an `HttpResponse` (which is served immediately to the user, cancelling the rest of the page serve workflow). This differs from the current implementation, where the `accept_request` method returns a boolean and the calling code (the `before_serve_page` hook in `wagtail.core.wagtail_hooks`) has to generate a suitable response in the case of `False` being returned.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would there be a way to filter a page QuerySet to only contain pages that the request can view? This is what accept_request was originally developed for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! The problem of filtering a queryset to the pages accessible to a given visitor came up recently here: https://stackoverflow.com/questions/54629835/wagtail-filter-page-childs-elements-on-the-basis-of-logged-in-users-permissions/54655495#54655495

I wonder if we'll be creating problems down the line by tying the filtering logic to a request object, though... it's possible that we'll want to apply the filtering in a situation where a request isn't available, or where the active request isn't representative of the request for the actual page. For example, a headless site might make an API request to fetch a listing of pages accessible to the current user, and we can't assume that the HTTP request corresponds to an active session. (Judging from https://www.django-rest-framework.org/api-guide/authentication/ we probably can rely on request.user to be set appropriately, but not request.session['passed_page_view_restrictions'] as used by password-based restrictions.) It seems to me that we might want to introduce a formal concept of "credentials", i.e. variables that might have a bearing on the "can this visitor view page X or not" decision, and pass those to the filtering / decision methods rather than letting them dig around in a request object. Might get a bit over-engineered though...

On a separate note, it occurs to me that user-based auth is probably going to be the most common use-case here - and if the *ViewRestriction API is going to grow like this, then there'll almost certainly be some recurring patterns ("does the user satisfy condition X? If not, redirect them to the login page, unless they are logged in, in which case send them to a dead end you-don't-have-permission page") that would be best handled in a new abstract class, say UserPageViewRestriction. Hopefully, a developer subclassing UserPageViewRestriction would then only have to provide the minimal model-level logic, and leave all the request/redirection stuff to Wagtail.


Similarly, subclasses of `CollectionViewRestriction` should implement a method `check_document(self, request, document)` which will be called just before serving a document, and which returns either `None` or an `HttpResponse`.

> This method is named `check_document` rather than `check` to allow for images or other collection-based resources to support view restrictions as a possible future development; these would warrant a distinct "check" function on `CollectionViewRestriction`.

A number of helper methods / functions exist to support the built-in restriction types: `BaseViewRestriction.mark_as_passed` (which updates a field inside the user's session to keep track of password restrictions that have been cleared) and `require_wagtail_login` (which generates a redirect response to the configured front-end login page). As these are potentially useful in implementing custom view restriction types, they will be made available either as methods on one of the `BaseViewRestriction` / `PageViewRestriction` / `CollectionViewRestriction` classes, or a mixin class.

### Form handling

Within the 'Page Privacy' modal (accessible from the Privacy button in the top right of the page editor and explorer views) and the 'Collection Privacy' modal (accessible from Settings -> Collections -> Edit -> Privacy), each subclass of `PageViewRestriction` / `CollectionViewRestriction` will contribute an radio button to the "visibility" chooser, along with a (possibly empty) set of form fields to be shown when that radio button is selected. This requires each subclass to define the following attributes / methods:

* `option_label`: a class-level attribute containing the label for the radio button, as a string or lazy translation object, e.g. `_("Private, accessible with the following password")`
* `get_form_class()`: a class method which takes no arguments and returns a ModelForm subclass containing the custom fields specific to this restriction type
* `render_form(form)`: a class method which accepts an instance of this form class and returns an HTML rendering of it. `BaseViewRestriction` provides a default implementation of `render_form` which renders the form using [a generic Wagtail admin form styling](https://github.com/wagtail/wagtail/blob/8fa7a41011810696a7cfb83255091e91c441b23e/wagtail/admin/templates/wagtailadmin/generic/edit.html#L15-L32)
* `template`: a class-level attribute that specifies a template path for `render_form` to use instead of the default template

The view logic in `wagtail.admin.views.page_privacy` / `wagtail.admin.views.collection_privacy` will be responsible for constructing a form instance for each restriction type, populated or unpopulated as appropriate, with a distinct prefix for each form, and rendering each form in a container element so that it can be shown / hidden via Javascript when the appropriate radio button is checked. When the form is submitted, the view logic will instantiate the appropriate form for the currently selected radio button and call its `is_valid` / `save` methods so that the appropriate view restriction object is written back to the database.


## Open Questions