processwire / processwire-issues

ProcessWire issue reports.
44 stars 2 forks source link

Inconsistency between PagesLoader::findOne() and PagesLoader::find() using options to override access control #1897

Closed kixe closed 6 months ago

kixe commented 6 months ago

Short description of the issue

PagesLoader::findOne() does not take into account settings in options to override access control, whereas PagesLoader::find() does.

Consistency is given if access is controlled via the selector.

// EXAMPLE:
// Given an access controlled page with ID 1234 and a user not allowed to view this page

// expected results – will return the access controlled page
$accessControlledPage = wire('pages')->find(1234, ['findAll' => true])->first();
$accessControlledPage = wire('pages')->find(1234, ['include' =>'all'])->first();
$accessControlledPage = wire('pages')->findOne("id=1234,include=all");
$accessControlledPage = wire('pages')->findOne("id=1234,check_access=0");

// unexpected results – will return NullPage
$accessControlledPage = wire('pages')->findOne(1234, ['findAll' => true]);
$accessControlledPage = wire('pages')->findOne(1234, ['include' =>'all']);

Maybe related: https://github.com/processwire/processwire-issues/issues/1552 https://github.com/processwire/processwire-issues/issues/1507

Setup/Environment

ryancramerdesign commented 6 months ago

Thanks @kixe Since the options are possible, I agree they should be supported. I've pushed a fix for this. Though for this use case, I'd suggest using the $pages->get() method instead as include=all is implied by the get(), and the reason for the existence of findOne() is to have the include=all excluded.

teppokoivula commented 6 months ago

Though for this use case, I'd suggest using the $pages->get() method instead as include=all is implied by the get(), and the reason for the existence of findOne() is to have the include=all excluded.

Slightly off-topic, but I've taken up the habit of always using — and recommending the use of — findOne() instead of get(). The reason is simply that I've seen way too many cases where get() was mistakenly assumed to restrict permissions etc.

Thus from my point of view it makes a lot of sense to combine findOne() with include=all. This way the code is clearly stating that "yes, we really wanted to bypass all checks" :)

kixe commented 6 months ago

@ryancramerdesign Thanks