Jump to content

$pages->find() not respecting my Pages::viewable() hook


thetuningspoon
 Share

Recommended Posts

I am hooking after Pages::viewable() to add a per-page permissions control based on the state of a field I've added to each page. The field is a Page field which lets you choose which role(s) the page will be viewable for.

The hook is setup in the init() method of one of my modules. 

In one of my templates I am using a $pages->find()->children to get some pages and looping through them to output a list. If I check for $item->viewable() before outputting each item, it only outputs the ones that are viewable by the current user (GOOD!). However, if I do not explicitly check with viewable(), all pages are shown (BAD :( )

I figured that $pages->find() must be using the viewable() method internally to determine which pages to return, so that my hook would be called (unless check_access=0 is used in the selector). But that doesn't seem to be the case.

Am I wrong about find() using the viewable() method? Or is my hook maybe getting added in the wrong place? 

Link to comment
Share on other sites

Thank you LostKobrakai, I guess it makes sense that PW would include the access restrictions in the SQL to avoid getting the pages from the database in the first place.

I didn't know about DynamicRoles (or maybe I did and then forgot). It looks very interesting. I cannot keep up with everything going on with PW any more (not sure if that's a good thing or a bad thing!)

I will probably just use the viewable() check manually for now. It does seem to work in every case (admin page tree, actually trying to access a page) other than find() operations.

Link to comment
Share on other sites

  • 2 years later...
On 1/18/2017 at 5:24 PM, thetuningspoon said:

I will probably just use the viewable() check manually for now. It does seem to work in every case (admin page tree, actually trying to access a page) other than find() operations.

Here is a discussion related to this issue, in which Ryan explains the API decisions: https://processwire.com/talk/topic/11736-get-requests-not-subject-to-access-control

To sum it up real quick (quotes): 

  • A $pages->find() or $pages->findOne() method that filters results is based on database-filtering, not runtime filtering.
  • The API is based around providing methods for the developer to control access the way they see fit. The viewable() method is the basis of that.
  • PW's access control model supports runtime hooks to Page::viewable.
  • One shouldn't skip a $page->viewable() call regardless of what method you used to retrieve the page.

When rendering frontend templates <?php if ($page->viewable()) : ?> (or $page->viewable('field_name'); ) is the way to go to check for default PW permissions. For example:

https://processwire.com/talk/topic/4834-simple-hooks-tutorial-turn-a-pagearray-into-a-list-of-links/

...
// loop through each item in the PageArray and render some links
foreach($event->object as $page) {
	$value = $page->get($property);
	if(!strlen($value)) continue; // skip empty values
	if(strlen($out)) $out .= $delimiter;
	if($page->viewable()) { 
		$out .= "<a href='$page->url'>$value</a>"; // if page is viewable, make it a link
	} else {
		$out .= $value; // if page is not viewable, just display the value
	}
}
...

In order to "extend" the access control logic, one can hook after Page::viewable eg.:
https://github.com/processwire/processwire-issues/issues/560#issuecomment-384656192
quote:

// goes into /site/ready.php
$wire->addHookAfter('Page::viewable', function($event) {
	$viewable = $event->return;
	if($viewable) return; // use PW's existing logic, since it said it was viewable
	$page = $event->object;
	if(!$page instanceof User) return; // this is an example for User templates only
	if(whatever logic you decide that $page is viewable) {
		$viewable = true;
		$event->return = $viewable;
	}
});

 

  • Like 2
Link to comment
Share on other sites

Hi Adrian,

Thanx for the reply! Actually, it is probably me who needs Dynamic Roles. So do I understand it right that the fork of @matjazp is ok to install in production?
This weekend I worked on implementing some sort of dynamic roles just to see how far I can get and also to study at least the basics in this area. However, if this fork is quite OK to use then it should be the way to go, I think.

Side-note: it's a pity that Ryan left Dynamic Roles in the dust ? 

 

Edited by szabesz
typo: added missing words :)
Link to comment
Share on other sites

@szabesz - I haven't actually used it in a long time. I thought the idea seemed cool, but I don't think the implementation is very efficient and I found another option for my needs - honestly right now I don't even remember the project I used it on. I trust @matjazp's work though and his fork fixes the main problems I was having with it (which is that new pages are not automatically added), so I think it's probably ok to use.

  • Like 1
Link to comment
Share on other sites

I'm also no longer using DynamicRoles in a production env. It was working well though with the fixes I proposed in the repo and on pw 2.7.3. 

An addition to @szabesz comments above: The thing to keep in mind with $page->viewable() as the "Solution for Access-Control" is that it's easy for it to not be enough. As soon as pagination is needed one needs access control at the database layer or pages will have different numbers of elements (even down to none at all). 

  • Like 2
Link to comment
Share on other sites

55 minutes ago, LostKobrakai said:

As soon as pagination is needed one needs access control at the database layer or pages will have different numbers of elements (even down to none at all). 

Yeap, I did realized that too. If one needs PW's out of the box pagination support, simply filtering with if($page->viewable()) is just not enough, it can only be used as a last resort to prevent information leakage but that does not necessarily mean issue-free code.

The approach I came up with in a nutshell is (besides using PW's built in access control):

  • Hooking after Page::viewable with custom access checks and using if($page->viewable()) where needed.
  • Hooking before Page::render with custom access checks, in order to block certain page access cases.
  • Added hook method (Pages::siteFind) which delegates its task to Pages::find but before doing so it modifies the selector string based on certain conditions in order to prevent fetching certain records in the first place.

However, my approach is rather crude and hardcoded into the site, so there is a lot of room for improvement here.

I do not know if I should spend the time on evaluating DynamicRoles as I do not have too much time to waste, but what makes it compelling is this bit I read in Ryans intro:

"This module directly affects the results of all page getting/finding operations by applying the access control directly to the database queries before pages are loaded. As a result, it is fast (regardless of scale), pagination friendly, and requires no further intervention by the developer other than configuring the dynamic roles as they see fit."

Anyway, if you guys no longer use DynamicRoles with current versions of PW, then may I ask what other approach you use? Any guidance would be appreciated.

Edited by szabesz
typos
  • Like 1
Link to comment
Share on other sites

For me it was a platform switch away from ProcessWire and even PHP. It had nothing to do with DynamicRoles at all. Without the issues mentioned/open prs on GitHub it should work fine. I've even looked into making it even more flexible once, but never finished the work.

  • Like 1
Link to comment
Share on other sites

I'm using DynamicRoles in production on two sites to allow users with specific roles to edit specific page(s). I started with PW 2.4.x, when I upgraded to PW 3, I had to modify the module by adding the namespace to the files. Then I made changes proposed by Benjamin (issues and PRs), I hope I did it well...

  • Like 1
Link to comment
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
 Share

  • Recently Browsing   0 members

    • No registered users viewing this page.
×
×
  • Create New...