From 5c832dd376615e3875bfd26b253be28e88d4592b Mon Sep 17 00:00:00 2001 From: Matias Griese Date: Thu, 28 Mar 2019 11:14:55 +0200 Subject: [PATCH] Fixed lost user access when saving user profile without super user permissions [#1639] --- CHANGELOG.md | 1 + classes/admin.php | 17 +- classes/adminbasecontroller.php | 5 +- classes/admincontroller.php | 294 ++++++++++++++++++-------------- 4 files changed, 177 insertions(+), 140 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 77dc826e..721a5be7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ 1. [](#bugfix) * Fixed user edit links if Flex Objects plugin is installed but user isn't Flex User * Fixed deprecated `sameas()` Twig test + * Regression: Fixed lost user access when saving user profile without super user permissions [#1639](https://github.com/getgrav/grav-plugin-admin/issues/1639) # v1.9.0-rc.4 ## 03/20/2019 diff --git a/classes/admin.php b/classes/admin.php index 1213d5ed..0e933b40 100644 --- a/classes/admin.php +++ b/classes/admin.php @@ -650,19 +650,11 @@ class Admin $obj->file($file); $data[$type] = $obj; - } elseif (preg_match('|users/|', $type)) { + } elseif (preg_match('|users?/|', $type)) { /** @var UserCollectionInterface $users */ $users = $this->grav['accounts']; - $obj = $users->load(preg_replace('|users/|', '', $type)); - $obj->update($this->cleanUserPost($post)); - - $data[$type] = $obj; - } elseif (preg_match('|user/|', $type)) { - /** @var UserCollectionInterface $users */ - $users = $this->grav['accounts']; - - $obj = $users->load(preg_replace('|user/|', '', $type)); + $obj = $users->load(preg_replace('|users?/|', '', $type)); $obj->update($this->cleanUserPost($post)); $data[$type] = $obj; @@ -714,15 +706,14 @@ class Admin * @param array $post * @return array */ - protected function cleanUserPost($post) + public function cleanUserPost($post) { // Clean fields for all users unset($post['hashed_password']); // Clean field for users who shouldn't be able to modify these fields if (!$this->authorize(['admin.user', 'admin.super'])) { - unset($post['access']); - unset($post['state']); + unset($post['access'], $post['state']); } return $post; diff --git a/classes/adminbasecontroller.php b/classes/adminbasecontroller.php index 542f7ee2..bee503f1 100644 --- a/classes/adminbasecontroller.php +++ b/classes/adminbasecontroller.php @@ -10,6 +10,7 @@ use Grav\Common\Media\Interfaces\MediaInterface; use Grav\Common\Page\Interfaces\PageInterface; use Grav\Common\Page\Media; use Grav\Common\Uri; +use Grav\Common\User\Interfaces\UserInterface; use Grav\Common\Utils; use Grav\Common\Plugin; use Grav\Common\Theme; @@ -727,9 +728,9 @@ class AdminBaseController } /** - * @param PageInterface|Data $obj + * @param PageInterface|UserInterface|Data $obj * - * @return PageInterface|Data + * @return PageInterface|UserInterface|Data */ protected function storeFiles($obj) { diff --git a/classes/admincontroller.php b/classes/admincontroller.php index 859be007..bd15a831 100644 --- a/classes/admincontroller.php +++ b/classes/admincontroller.php @@ -556,114 +556,92 @@ class AdminController extends AdminBaseController return false; } + $this->grav['twig']->twig_vars['current_form_data'] = (array)$this->data; + + switch ($this->view) { + case 'pages': + return $this->taskSavePage(); + case 'user': + return $this->taskSaveUser(); + default: + return $this->taskSaveDefault(); + } + } + + protected function taskSavePage() + { $reorder = true; - $data = (array)$this->data; - $this->grav['twig']->twig_vars['current_form_data'] = $data; + /** @var Pages $pages */ + $pages = $this->grav['pages']; - // Special handler for user data. - if ($this->view === 'user') { - if (!$this->grav['user']->exists()) { - $this->admin->setMessage($this->admin::translate('PLUGIN_ADMIN.NO_USER_EXISTS'),'error'); - return false; - } - if (!$this->admin->authorize(['admin.super', 'admin.users'])) { - // no user file or not admin.super or admin.users - if ($this->prepareData($data)->username !== $this->grav['user']->username) { - $this->admin->setMessage($this->admin::translate('PLUGIN_ADMIN.INSUFFICIENT_PERMISSIONS_FOR_TASK') . ' save.','error'); - return false; - } - } + // Find new parent page in order to build the path. + $route = $data['route'] ?? dirname($this->admin->route); + + /** @var PageInterface $obj */ + $obj = $this->admin->page(true); + + if (!isset($data['folder']) || !$data['folder']) { + $data['folder'] = $obj->slug(); + $this->data['folder'] = $obj->slug(); } - // Special handler for pages data. - if ($this->view === 'pages') { - /** @var Pages $pages */ - $pages = $this->grav['pages']; + // Ensure route is prefixed with a forward slash. + $route = '/' . ltrim($route, '/'); - // Find new parent page in order to build the path. - $route = !isset($data['route']) ? dirname($this->admin->route) : $data['route']; + // Check for valid frontmatter + if (isset($data['frontmatter']) && !$this->checkValidFrontmatter($data['frontmatter'])) { + $this->admin->setMessage($this->admin::translate('PLUGIN_ADMIN.INVALID_FRONTMATTER_COULD_NOT_SAVE'), + 'error'); + return false; + } - /** @var PageInterface $obj */ - $obj = $this->admin->page(true); - - if (!isset($data['folder']) || !$data['folder']) { - $data['folder'] = $obj->slug(); - $this->data['folder'] = $obj->slug(); - } - - // Ensure route is prefixed with a forward slash. - $route = '/' . ltrim($route, '/'); - - // Check for valid frontmatter - if (isset($data['frontmatter']) && !$this->checkValidFrontmatter($data['frontmatter'])) { - $this->admin->setMessage($this->admin::translate('PLUGIN_ADMIN.INVALID_FRONTMATTER_COULD_NOT_SAVE'), + // XSS Checks for page content + $xss_whitelist = $this->grav['config']->get('security.xss_whitelist', 'admin.super'); + if (!$this->admin->authorize($xss_whitelist)) { + $check_what = ['header' => $data['header'] ?? '', 'frontmatter' => $data['frontmatter'] ?? '', 'content' => $data['content'] ?? '']; + $results = Security::detectXssFromArray($check_what); + if (!empty($results)) { + $this->admin->setMessage(' ' . $this->admin::translate('PLUGIN_ADMIN.XSS_ONSAVE_ISSUE'), 'error'); return false; } + } - // XSS Checks for page content - $xss_whitelist = $this->grav['config']->get('security.xss_whitelist', 'admin.super'); - if (!$this->admin->authorize($xss_whitelist)) { - $check_what = ['header' => $data['header'] ?? '', 'frontmatter' => $data['frontmatter'] ?? '', 'content' => $data['content'] ?? '']; - $results = Security::detectXssFromArray($check_what); - if (!empty($results)) { - $this->admin->setMessage(' ' . $this->admin::translate('PLUGIN_ADMIN.XSS_ONSAVE_ISSUE'), - 'error'); - return false; - } + $parent = $route && $route !== '/' && $route !== '.' && $route !== '/.' ? $pages->dispatch($route, true) : $pages->root(); + $original_order = (int)trim($obj->order(), '.'); + + try { + // Change parent if needed and initialize move (might be needed also on ordering/folder change). + $obj = $obj->move($parent); + $this->preparePage($obj, false, $obj->language()); + $obj->validate(); + + } catch (\Exception $e) { + $this->admin->setMessage($e->getMessage(), 'error'); + + return false; + } + $obj->filter(); + + // rename folder based on visible + if ($original_order === 1000) { + // increment order to force reshuffle + $obj->order($original_order + 1); + } + + if (isset($data['order']) && !empty($data['order'])) { + $reorder = explode(',', $data['order']); + } + + // add or remove numeric prefix based on ordering value + if (isset($data['ordering'])) { + if ($data['ordering'] && !$obj->order()) { + $obj->order(static::getNextOrderInFolder($obj->parent()->path())); + $reorder = false; + } elseif (!$data['ordering'] && $obj->order()) { + $obj->folder($obj->slug()); } - - - $parent = $route && $route !== '/' && $route !== '.' && $route !== '/.' ? $pages->dispatch($route, true) : $pages->root(); - $original_order = (int)trim($obj->order(), '.'); - - try { - // Change parent if needed and initialize move (might be needed also on ordering/folder change). - $obj = $obj->move($parent); - $this->preparePage($obj, false, $obj->language()); - $obj->validate(); - - } catch (\Exception $e) { - $this->admin->setMessage($e->getMessage(), 'error'); - - return false; - } - $obj->filter(); - - // rename folder based on visible - if ($original_order === 1000) { - // increment order to force reshuffle - $obj->order($original_order + 1); - } - - if (isset($data['order']) && !empty($data['order'])) { - $reorder = explode(',', $data['order']); - } - - // add or remove numeric prefix based on ordering value - if (isset($data['ordering'])) { - if ($data['ordering'] && !$obj->order()) { - $obj->order(static::getNextOrderInFolder($obj->parent()->path())); - $reorder = false; - } elseif (!$data['ordering'] && $obj->order()) { - $obj->folder($obj->slug()); - } - } - - } else { - // Handle standard data types. - $obj = $this->prepareData($data); - - try { - $obj->validate(); - } catch (\Exception $e) { - $this->admin->setMessage($e->getMessage(), 'error'); - - return false; - } - - $obj->filter(); } $obj = $this->storeFiles($obj); @@ -676,44 +654,110 @@ class AdminController extends AdminBaseController $this->grav->fireEvent('onAdminAfterSave', new Event(['object' => $obj])); } - if ($this->view !== 'pages') { - // Force configuration reload. - /** @var Config $config */ - $config = $this->grav['config']; - $config->reload(); + if (method_exists($obj, 'unsetRouteSlug')) { + $obj->unsetRouteSlug(); + } - if ($this->view === 'user') { - if ($obj->username === $this->grav['user']->username) { - /** @var UserCollectionInterface $users */ - $users = $this->grav['accounts']; + $multilang = $this->isMultilang(); - //Editing current user. Reload user object - unset($this->grav['user']->avatar); - $this->grav['user']->merge($users->load($this->admin->route)->toArray()); - } + if ($multilang) { + if (!$obj->language()) { + $obj->language($this->grav['session']->admin_lang); + } + } + $admin_route = $this->admin->base; + + $route = $obj->rawRoute(); + $redirect_url = ($multilang ? '/' . $obj->language() : '') . $admin_route . '/' . $this->view . $route; + $this->setRedirect($redirect_url); + + return true; + } + + protected function taskSaveUser() + { + /** @var UserCollectionInterface $users */ + $users = $this->grav['accounts']; + + $user = $users->load($this->admin->route); + + if (!$this->admin->authorize(['admin.super', 'admin.users'])) { + // no user file or not admin.super or admin.users + if ($user->username !== $this->grav['user']->username) { + $this->admin->setMessage($this->admin::translate('PLUGIN_ADMIN.INSUFFICIENT_PERMISSIONS_FOR_TASK') . ' save.','error'); + + return false; } } - // Always redirect if a page route was changed, to refresh it - if ($obj instanceof PageInterface) { - if (method_exists($obj, 'unsetRouteSlug')) { - $obj->unsetRouteSlug(); - } + /** @var Data\Blueprint $blueprint */ + $blueprint = $user->blueprints(); + $data = $blueprint->processForm($this->admin->cleanUserPost((array)$this->data)); + $data = new Data\Data($data, $blueprint); - $multilang = $this->isMultilang(); + try { + $data->validate(); + $data->filter(); + } catch (\Exception $e) { + $this->admin->setMessage($e->getMessage(), 'error'); - if ($multilang) { - if (!$obj->language()) { - $obj->language($this->grav['session']->admin_lang); - } - } - $admin_route = $this->admin->base; - - $route = $obj->rawRoute(); - $redirect_url = ($multilang ? '/' . $obj->language() : '') . $admin_route . '/' . $this->view . $route; - $this->setRedirect($redirect_url); + return false; } + $user->update($data->toArray()); + + $user = $this->storeFiles($user); + + if ($user) { + // Event to manipulate data before saving the object + $this->grav->fireEvent('onAdminSave', new Event(['object' => &$user])); + $user->save(); + $this->admin->setMessage($this->admin::translate('PLUGIN_ADMIN.SUCCESSFULLY_SAVED'), 'info'); + $this->grav->fireEvent('onAdminAfterSave', new Event(['object' => $user])); + } + + if ($user->username === $this->grav['user']->username) { + /** @var UserCollectionInterface $users */ + $users = $this->grav['accounts']; + + //Editing current user. Reload user object + unset($this->grav['user']->avatar); + $this->grav['user']->merge($users->load($this->admin->route)->toArray()); + } + + return true; + } + + protected function taskSaveDefault() + { + // Handle standard data types. + $obj = $this->prepareData((array)$this->data); + + try { + $obj->validate(); + } catch (\Exception $e) { + $this->admin->setMessage($e->getMessage(), 'error'); + + return false; + } + + $obj->filter(); + + $obj = $this->storeFiles($obj); + + if ($obj) { + // Event to manipulate data before saving the object + $this->grav->fireEvent('onAdminSave', new Event(['object' => &$obj])); + $obj->save(); + $this->admin->setMessage($this->admin::translate('PLUGIN_ADMIN.SUCCESSFULLY_SAVED'), 'info'); + $this->grav->fireEvent('onAdminAfterSave', new Event(['object' => $obj])); + } + + // Force configuration reload. + /** @var Config $config */ + $config = $this->grav['config']; + $config->reload(); + return true; }