Limiting Explorer to the current user's "explorable pages"

[coredumperror]

Last week, @tomdyson gave me a suggestion for how Wagtail might be enhanced to support completely separated tenants in a multi-tenant Wagtail instance: implement a "View in Admin" Permission. Possibly with a better name... I believe someone suggested "Choose" in one of the issues relating to this concept, but that's not really broad enough.

Applied to Pages, this permission would prevent unprivileged users from:

• seeing the Page in the Explorer or the fly-out menu in the sidebar
• going to the Explorer URL for that Page (e.g. typing /admin/pages/29/ into the address bar would issue a 403)
• seeing the Page in admin-side search results
• seeing the Page in a PageChooser

Applied to Collections, this permission would prevent unprivileged users from:

• seeing Images or Documents in Choosers
• uploading media to unprivileged Collections

Users already can't see or upload media in unprivileged Collections on the /admin/images/ and /admin/documents/ pages, so I'll hopefully be able to use the code from those pages as inspiration for implementing the View permission.

The purpose of this permission would be to completely isolate users belonging to one Site, so they can't tell that content belonging to any other Site exists (and also can't get to said content even if they guess the URL, since that would be a privacy violation). This is counter to the way that Collections and Pages currently work, though, so I suppose this functionality would have to be made optional, somehow? I'm not really sure how that would be done, though, so suggestions are welcome!

I intend to implement this myself over the coming days, and will be providing a PR for my changes as soon as they're in a presentable state. I opened this issue now, though, to get input from you guys on how best to go about this, and whether any of my current assumptions may be in need of an update to properly understand all the subtleties involved.

Since this will involve changing the choosers, it relates to #1645, which I also plan to enhance by making the new unified Choosers be Permission-aware.

[tomdyson]

Thanks, @coredumperror. I thought the 'someone' who suggested "Choose" was you! I still like it.

@timheap, see the reference to #1645.

[coredumperror]

I think "Choose" works for the choosers, but this permission affects the Explorer and the admin search as well, so it doesn't seem appropriate.

[coredumperror]

I ran into a spot of trouble while upgrading my development system from Yosemite + MacPorts to El Capitan + Homebrew, and thus have not had a chance to actually start on this project until just now. I'll be working on this over the next week or so, and will then integrate this new permission into #1645 as well.

I was thinking about naming this "Visible in Admin", which feels like a more descriptive name. But then it sounds unlike a permission granted to the user, and more like an attribute on the Page/Collection. So I'm not really sure it'd work.

[coredumperror]

I just ran into a conundrum that I'm not certain how best to solve:
Lets say Page A has subpages B, C, and D. The user is in a Group with "View in Admin" enabled on A, but that Group also has a permission set on D with "View in Admin" disabled. Should D and its subpages be hidden from them?

Based on reading the code in UserPagePermissionsProxy, all the other Page permissions appear to entirely ignore this situation of subpages having different permissions, which makes me think that View in Admin probably should, too. But that seems like a worthwhile feature for complex page tree visibility.

[coredumperror]

Had another thought: If View is Admin isn't granted, should that imply that all the other permissions are revoked as well? If you're not allowed to even see the page in the admin, presumably you shouldn't be allowed to edit it, publish it, delete it, or add subpages to it. Right?

[gasman]

Lets say Page A has subpages B, C, and D. The user is in a Group with "View in Admin" enabled on A, but that Group also has a permission set on D with "View in Admin" disabled. Should D and its subpages be hidden from them?

By design, all permissions are additive as you go down the tree - so it's not possible to have a subpage with less permissions than the parent. Unless you have a specific scenario in mind that requires this, I'd suggest that this new permission type behaves the same way as the existing ones.

If you're not allowed to even see the page in the admin, presumably you shouldn't be allowed to edit it, publish it, delete it, or add subpages to it. Right?

I'd actually see it the other way round: if you don't have add/edit/publish permission on a page (or any of its children), there's actually no reason for you to see it in the explorer, and the fact that you currently do see it is arguably a usability bug in Wagtail. With that in mind, I would re-propose what I proposed back in #359 (comment) - we use the existing permissions to determine what gets shown in the explorer (and left menu), so there's no need to create a new 'view in admin' permission for this case.

To formalise what I mean a bit more: if you have a site structure like

Fooville University
  |- Contact Us
  |- Departments
      |- Economics
      |- Philosophy
      |- History

and a user only has edit permission over the 'Economics' section, then that's the point at which their page tree will begin - they won't see 'Fooville University' or 'Departments' at all. On the other hand, if they have edit permission over 'Economics' and 'Philosophy' then their page tree would have to start from 'Departments', as that's the deepest level that encloses all their editable pages. They would therefore see 'Departments' as a read-only page, and Economics and Philosophy as editable pages under it. (They would not see History, since again there's no reason for them to be able explore that subsection.)

Separately to the above change, we then introduce a 'choose' permission that takes effect specifically on the 'page chooser' popups. In this case it does make sense to have this as a distinct permission level, to distinguish between this kind of multi-user site (where users should be able to create links to History in the Economics section) and true multi-homed setups where users have no knowledge of other sites.

This also addresses the main reservation I have with "View in admin" as a permission type, namely that it doesn't really represent an 'action'. Ideally, permissions should indicate what actions a user is allowed to perform, and the admin UI should configure itself in whatever way is most appropriate to achieve that - rather than the permissions being directly tied to the admin behaviour. It's a subtle distinction, but it means that the site owner only has to think in terms of access restrictions, and not 'how will the admin behave for this other user' - and also means that if we ever implement alternative ways of editing that don't go through the admin (like, say, a mobile Wagtail client app...) the permission rules will still be meaningful.

[coredumperror]

I like this idea! It solves the awkwardness of the new permission's name and gets around the issue of differentiation between multi-tenant and multi-user sites that I was worried about in the back of my mind, but couldn't quite grasp well enough to articulate.

However, this does it make it more complex to implement the Explorer and sidemenu. I'm not really sure how to do this:

if they have edit permission over 'Economics' and 'Philosophy' then their page tree would have to start from 'Departments', as that's the deepest level that encloses all their editable pages.

Have you got any advice on how to implement such a filter? Does treebeard have an API for "given a set of nodes, find the closest common ancestor"?

I'll get to work on what I can right away. This week is going to be nearly full-time wagtail work for me.

[gasman]

Have you got any advice on how to implement such a filter? Does treebeard have an API for "given a set of nodes, find the closest common ancestor"?

I suspect there's no official API for it, but the internal path representation (a string like '000100020001', where each group of steplen characters, 4 by default, indicates the position at that level) should hopefully make it fairly easy to hack one up - it just needs to find the longest common prefix (trimmed to a multiple of steplen characters).

[coredumperror]

Ah, I thought that big numeric string in the DB rows was probably related to the tree format. Now I know! Thanks for the hint on stoplen, too. I had assumed it was just hardcoded at 4.

[coredumperror]

OK, I have a working solution in place for limiting the Explorer to only those pages for which a user has at least one administrative permission. This is a function I added to the Page model, and the wagtailadmin.view.pages.index view calls it instead of parent_page.get_children():

    def get_administrable_children(self, user):
        """
        Returns a queryset of all the node's children that the given user is permitted to administer.
        """
        query = self.get_children()
        # Superusers see everything.
        if user.is_superuser:
            return query

        # Everyone else sees only the pages for which they have any non-Choose permission.
        permissions = (GroupPagePermission.objects
            .filter(group__user=user).exclude(permission_type='choose').select_related('page')
        )
        permitted_paths = set(perm.page.path for perm in permissions)
        path_Qs = reduce(operator.or_, (Q(path__startswith=permitted_path) for permitted_path in permitted_paths))
        query = query.filter(path_Qs)

        return query

After getting the base query from MP_Node.get_children(), Page.get_administrable_children(user) consults the GroupPagePermission model to determine the paths of each Page that the given user is permitted to administer. The "Choose" permission is excluded because it's only relevant for Choosers, not the Explorer.

I'm wary of this approach, though, because it issues 2 queries against the DB. I'd like to reduce that down to 1, but I only have an intermediate understanding of SQL, so I'm not even sure if it would be possible. My best guess is that using a subquery which consults GroupPagePermission in place of children_query.filter(path_Qs) might work, but I don't know how to construct one that would accomplish what the code that builds path_Qs does.

[coredumperror]

Ah dang, I just realized that this isn't actually sufficient regardless of performance. If a user is limited to only pages that start below depth = 2, going to /admin/pages displays nothing, and you can't go anywhere. Which is exactly what @gasman said 2 comments ago... ugh, I completely forgot.

I'll work on tweaking this algorithm to include all pages up to the closest common ancestor, and then change /admin/pages to display the page tree starting there.

[coredumperror]

I've pushed my changes for this feature up in PR #2463. I'm pretty sure I got all the necessary functionality working for Pages, though it's going to definitely need some iteration to refine it.

I'm going to start implementing the missing cache clear functionality (#2 in my list of work to still be done) right now.

[coredumperror]

Have you guys got any feedback on my implementation? Am I going about this in a reasonable way? Have I missed any essential features that should be getting limited by the "explorable pages" functionality?

I haven't started on the chooser or collection limiting code, yet. Besides the testing (which I'm working on now), it feels like the Explorer itself is done.

[tomdyson]

Sorry, we've been focussing on the modeladmin integration (which is now done). @m1kola do you have time to look at this?

[m1kola]

I will be able to start review tomorrow. Is it ok?

[coredumperror]

Sounds good, thanks! I should be done writing the tests by then, as well.

[coredumperror]

I've got an iMac (27-inch, Late 2013) as my dev box, but I'm getting much lower performance than I would expect when I run ./runtests. Is it just expected that it'll take upwards of a full minute for the Creating test database for alias 'default'... step? That makes developing my tests pretty annoying, so I really hope I'm doing something wrong.

[m1kola]

I really hope I'm doing something wrong.

I don't think so. I tried to run tests on master and on your branch and Creating test database for alias 'default'... step took almost same time on both branches (20-30 seconds on my laptop).

I agree that it's annoying. Now I can only suggest you to run only those tests that you need at the moment. For example:

./runtests.py wagtail.wagtailadmin.tests.test_pages_views.TestExplorablePageVisibility

[coredumperror]

Yeah, that's what I've been doing. That doesn't speed up the database setup process, though. :(

Oh well.

[coredumperror]

Is the {% explorer_nav %} template tag documented anywhere? It doesn't appear to be used by anything within wagtail itself. I ask because it doesn't accept any arguments, which means that any existing code that might use it won't be passing the current user to it, which means we can't determine who's permissions to check for explorability.

The easiest solution would be to change the tag so it accepts a required argument of the current user. That would be backward incompatible, though, and I'm not sure what the policy is for that.

[coredumperror]

I just ran into a real pickle of an issue.

When determining how to respond when a non-superuser requests a certain page, there seem to be two ways to go about it:

1. Check if the User has permission to explore the Page. If he does, return it. If he doesn't, throw a 403 if the page is on the current Site, or a 404 if it's on another Site. This is how I originally designed the system.
2. Check if the page is on the current Site, and throw a 404 if it's not, regardless of permission. For Pages that are on the current Site, if the User has permission, return the Page, otherwise throw a 403. I recently changed the algorithm to this form.

The first one allows a User who has permission to see Pages only on Site A to access them from Site B, which seems wrong.

But the second prevents a User who has permission on Pages from Site A and Site B from seeing Pages that aren't on the current Site, forcing him to log in to another domain to explore the other Site's pages, even though they're on the same server.

Neither of these seem desirable, and I don't know what to do about it. I suppose #2 is slightly more desirable for true multi-tenanting purposes, but an admin who doesn't want to keep his Sites' pages quite so separated might find that irritating.

[gasman]

Hi @coredumperror,

I'd very much like to stick to a model where the admin behaves the same regardless of the domain they happen to be logged in on. In other words, there's a single entry point through which the user can edit everything across the installation that they have permission for - there's no concept of "the admin backend for site A".

In the past, we've had odd bits of logic in the admin that triggered based on the domain the user was logged in on, and that's always turned out to be a bad idea (see #1461 for example). Indeed, we'd like to allow for the possibility of logging in to the admin through a domain like admin.example.com that doesn't correspond to a site at all - this doesn't quite work at the moment (see #1807).

As far as I can see, the only scenario where this wouldn't be desirable would be if you had users administering multiple sites, and didn't want those users to know that they were all running from the same Wagtail installation. That seems like a pretty esoteric requirement, and if you really needed that, you could always give users multiple login accounts, one per site.

[coredumperror]

Yeah, that makes sense. I'll go change my algorithm back to the old method.

[coredumperror]

With the major work on this PR winding down to completion, I'm now suffering a conundrum of ignorance.

The second part of the work I've been doing to restrict unprivileged users is going to depend upon a not-yet-complete PR (#1645): I need to update the choosers to restrict users to their permitted Pages and Collections.

What I don't know is how to start the PR, since it will be dependent upon my own #2463 and upon #1645. What does one do in such a situation? I have no idea which git commands to run to create the starting point for adding new commits, since they'll depend on changes from both PRs.

[coredumperror]

So... have you guys got anything to say about this feature, now that it's finished? I'd really like to get started on updating the choosers to have the same permission limits, but I can't really do so unless I'm sure this implementation is what you're going to go with.

[ravxl]

Folks,

I saw that this "error" was successfully implemented in explorer tab. Is there any open issue or plan to fix it in root pages view and in all documents, images?

thanks.

[gasman]

@ravxl Yes - that's why this issue (#2401) is still open, and #2869 was described as a partial fix.

[gasman]

Closing this to reduce duplication of tickets - I'm now treating #2463 as the "master" issue for tracking multi-tenancy support (even if much of the work is happening on other PRs).