From b6ec6cd679804e1c044778f624ecdcfc228d87d5 Mon Sep 17 00:00:00 2001 From: Ryan Cramer Date: Tue, 17 Jul 2018 11:41:33 -0400 Subject: [PATCH] Fix issue processwire/processwire-issues#629 where $config->pagefileSecure combined with non-superuser editing user profile didn't show images in image fields --- wire/core/User.php | 49 +++++++++++---- wire/modules/PagePermissions.module | 68 +++++++++++++++------ wire/modules/Process/ProcessPageView.module | 13 ++-- 3 files changed, 97 insertions(+), 33 deletions(-) diff --git a/wire/core/User.php b/wire/core/User.php index 6278d44e..8e06c4d9 100644 --- a/wire/core/User.php +++ b/wire/core/User.php @@ -27,7 +27,15 @@ * */ -class User extends Page { +class User extends Page { + + /** + * Cached value for $user->isSuperuser() checks + * + * @var null|bool + * + */ + protected $isSuperuser = null; /** * Create a new User page in memory. @@ -155,7 +163,7 @@ class User extends Page { * @param Page|Template|bool|string $context Page or Template... * - or specify boolean true to return if user has permission OR if it was added at any template * - or specify string "templates" to return array of Template objects where user has permission - * @return bool + * @return bool|array * */ public function hasPermission($name, $context = null) { @@ -233,7 +241,7 @@ class User extends Page { if(!$permission || !$permission->id) return false; - $roles = $this->get('roles'); + $roles = $this->getUnformatted('roles'); if(empty($roles) || !$roles instanceof PageArray) return false; $has = false; $accessTemplate = is_null($page) ? false : $page->getAccessTemplate($permission->name); @@ -390,17 +398,23 @@ class User extends Page { * */ public function isSuperuser() { + if(is_bool($this->isSuperuser)) return $this->isSuperuser; $config = $this->wire('config'); - if($this->id === $config->superUserPageID) return true; - if($this->id === $config->guestUserPageID) return false; - $superuserRoleID = (int) $config->superUserRolePageID; - $roles = $this->get('roles'); - if(empty($roles)) return false; - $is = false; - foreach($roles as $role) if(((int) $role->id) === $superuserRoleID) { + if($this->id === $config->superUserPageID) { $is = true; - break; + } else if($this->id === $config->guestUserPageID) { + $is = false; + } else { + $superuserRoleID = (int) $config->superUserRolePageID; + $roles = $this->getUnformatted('roles'); + if(empty($roles)) return false; // no cache intentional + $is = false; + foreach($roles as $role) if(((int) $role->id) === $superuserRoleID) { + $is = true; + break; + } } + $this->isSuperuser = $is; return $is; } @@ -485,4 +499,17 @@ class User extends Page { return $this->wire('users'); } + /** + * Hook called when field has changed + * + * @param string $what + * @param mixed $old + * @param mixed $new + * + */ + public function ___changed($what, $old = null, $new = null) { + if($what == 'roles' && is_bool($this->isSuperuser)) $this->isSuperuser = null; + parent::___changed($what, $old, $new); + } + } diff --git a/wire/modules/PagePermissions.module b/wire/modules/PagePermissions.module index 5e879b48..90ad1b53 100644 --- a/wire/modules/PagePermissions.module +++ b/wire/modules/PagePermissions.module @@ -239,27 +239,41 @@ class PagePermissions extends WireData implements Module { */ public function userEditable(Page $page, array $options = array()) { + /** @var User $user */ + $user = $this->wire('user'); + + /** @var Process|ProcessProfile|ProcessPageView|ProcessUser|ProcessPageList|ProcessPageLister $process */ + $process = $this->wire('process'); + + /** @var Config $config */ + $config = $this->wire('config'); + $defaults = array( 'viewable' => false, // specify true if method is being used to determine viewable state ); $options = count($options) ? array_merge($defaults, $options) : $defaults; - + + if(!$page->id) return false; if($page->className() != 'User') $page = $this->wire('users')->get($page->id); if(!$page || $page instanceof NullPage) return false; - - $user = $this->wire('user'); - - // if user is editing themselves in ProcessProfile, and they have permission to do so - if($user->id === $page->id) { - if($this->wire('page')->process == 'ProcessProfile' && $user->hasPermission('profile-edit')) { + + if($user->id === $page->id && !$user->isGuest() && $user->hasPermission('profile-edit')) { + // user is the same as the page being edited or viewed + if($process == 'ProcessProfile') { + // user editing themself in ProcssProfile return true; + } else if($this->wire('page') && $this->wire('page')->process == 'ProcessProfile') { + // user editing themself in ProcessProfile, when process not yet established + return true; + } else if($process == 'ProcessPageView' && $config->pagefileSecure && $options['viewable']) { + // user is viewing a file that is part of their User page when pagefileSecure mode active + return $process->getResponseType() == ProcessPageView::responseTypeFile; } } // if the current process is something other than ProcessUser, they don't have permission if(!$options['viewable']) { - $process = $this->wire('process'); if($process != 'ProcessUser' && (!$process instanceof ProcessPageList) && (!$process instanceof ProcessPageLister)) { return false; } @@ -270,7 +284,7 @@ class PagePermissions extends WireData implements Module { // if the user page being edited has a superuser role, and the current user doesn't, // never let them edit regardless of any other permissions - $superuserRole = $this->wire('roles')->get($this->wire('config')->superUserRolePageID); + $superuserRole = $this->wire('roles')->get($config->superUserRolePageID); if($page->roles->has($superuserRole) && !$user->roles->has($superuserRole)) return false; // if we reach this point then check if there are more granular user-admin permissions available @@ -285,7 +299,7 @@ class PagePermissions extends WireData implements Module { // there are role-specific permissions in the system, and user must have appropriate one to edit $userEditable = false; - $guestRoleID = $this->wire('config')->guestUserRolePageID; + $guestRoleID = $config->guestUserRolePageID; $n = 0; foreach($page->roles as $role) { $n++; @@ -302,6 +316,11 @@ class PagePermissions extends WireData implements Module { return false; } + + public function userViewable(Page $page, array $options = array()) { + $options['viewable'] = true; + return $this->userEditable($page, $options); + } /** * Can the current user add/remove the given role from other users? @@ -530,13 +549,28 @@ class PagePermissions extends WireData implements Module { if($status & Page::statusCorrupted) $status = $status & ~Page::statusCorrupted; // perform several viewable checks, in order - if($status >= Page::statusUnpublished) $viewable = false; - else if(!$page->template || ($checkFile && !$page->template->filenameExists())) $viewable = false; - else if($user->isSuperuser()) $viewable = true; - else if($page->process) $viewable = $this->processViewable($page); - else if($page instanceof User && $user->hasPermission('user-admin')) $viewable = $this->userEditable($page, array('viewable' => true)); - else if(!$user->hasPermission("page-view", $page)) $viewable = false; - else if($page->isTrash()) $viewable = false; + if($status >= Page::statusUnpublished) { + // unpublished pages are not viewable, but see override below this if/else statement + $viewable = false; + } else if(!$page->template || ($checkFile && !$page->template->filenameExists())) { + // template file does not exist + $viewable = false; + } else if($user->isSuperuser()) { + // superuser always allowed + $viewable = true; + } else if($page->process) { + // delegate access to permissions defined with Process module + $viewable = $this->processViewable($page); + } else if($page instanceof User && !$user->isGuest() && ($user->hasPermission('user-admin') || $page->id === $user->id)) { + // user administrator or user viewing themself + $viewable = $this->userViewable($page); + } else if(!$user->hasPermission("page-view", $page)) { + // user lacks basic view permission to page + $viewable = false; + } else if($page->isTrash()) { + // pages in trash are not viewable, except to superuser + $viewable = false; + } // if the page is editable by the current user, force it to be viewable (if not viewable due to being unpublished) if(!$viewable && !$user->isGuest() && ($status & Page::statusUnpublished)) { diff --git a/wire/modules/Process/ProcessPageView.module b/wire/modules/Process/ProcessPageView.module index 662bb52f..81540a83 100644 --- a/wire/modules/Process/ProcessPageView.module +++ b/wire/modules/Process/ProcessPageView.module @@ -344,7 +344,10 @@ class ProcessPageView extends Process { if($config->pagefileSecure) { $page = $this->checkRequestFile($it); - if(is_object($page)) return $page; // Page or NullPage + if(is_object($page)) { + $this->responseType = self::responseTypeFile; + return $page; // Page or NullPage + } } // optimization to filter out page numbers first @@ -602,14 +605,14 @@ class ProcessPageView extends Process { */ protected function checkAccess($page) { - if($page->viewable()) return $page; - if($this->requestFile) { // if a file was requested, we still allow view even if page doesn't have template file - // this is something that viewable() does not echeck - if($page->editable()) return $page; + if($page->viewable(false)) return $page; + // if($page->editable()) return $page; if($this->checkAccessDelegated($page)) return $page; if($page->status < Page::statusUnpublished && $this->wire('user')->hasPermission('page-view', $page)) return $page; + } else if($page->viewable()) { + return $page; } $accessTemplate = $page->getAccessTemplate();