Skip to content

Commit

Permalink
Merge pull request #3847 from bolt/feature/backend-optimize-22
Browse files Browse the repository at this point in the history 
Optimisation for backend by caching requests for users. [2.2]
  • Loading branch information
@GwendolenLynch
GwendolenLynch committed Jul 21, 2015
2 parents f31ac65 + 7f4f3ef commit b1cb966
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 55 deletions.
98 changes: 50 additions & 48 deletions src/Permissions.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -462,27 +462,29 @@ public function getEffectiveRolesForUser($user)
*
* "contenttype:$contenttype:edit or contenttype:$contenttype:view"
*
* @param string $what The desired permission, as elaborated upon above.
* @param mixed $user The user to check permissions against.
* @param string $contenttype Optional: Content type slug. If specified,
* $what is taken to be a relative permission (e.g. 'edit')
* rather than an absolute one (e.g. 'contenttype:pages:edit').
* @param integer $contentid Only used if $contenttype is given, to further
* specifiy the content item.
* @param string $what The desired permission, as elaborated upon above.
* @param mixed $user The user to check permissions against.
* @param string|array|Content $content Optional: Content object/array or ContentType slug.
* If specified, $what is taken to be a relative permission (e.g. 'edit')
* rather than an absolute one (e.g. 'contenttype:pages:edit').
* @param integer $contentId Only used if $content is given, to further specifiy the content item.
*
* @return boolean TRUE if the permission is granted, FALSE if denied.
*/
public function isAllowed($what, $user, $contenttype = null, $contentid = null)
public function isAllowed($what, $user, $content = null, $contentId = null)
{
// $contenttype must be a string, not an array.
if (is_array($contenttype)) {
$contenttype = $contenttype['slug'];
if (is_array($content)) {
$contenttypeSlug = $content['slug'];
} elseif ($content instanceof \Bolt\Content) {
$contenttypeSlug = $content->contenttype['slug'];
} else {
$contenttypeSlug = $content;
}

$this->audit("Checking permission query '$what' for user '{$user['username']}' with contenttype '$contenttype' and contentid '$contentid'");
$this->audit("Checking permission query '$what' for user '{$user['username']}' with contenttype '$contenttypeSlug' and contentid '$contentId'");

// First, let's see if we have the check in the per-request cache.
$rqCacheKey = $user['id'] . '//' . $what . '//' . $contenttype . '//' . $contentid;
$rqCacheKey = $user['id'] . '//' . $what . '//' . $contenttypeSlug . '//' . $contentId;
if (isset($this->rqcache[$rqCacheKey])) {
return $this->rqcache[$rqCacheKey];
}
Expand All @@ -496,7 +498,7 @@ public function isAllowed($what, $user, $contenttype = null, $contentid = null)
$this->app['cache']->save($cacheKey, json_encode($rule));
}
$userRoles = $this->getEffectiveRolesForUser($user);
$isAllowed = $this->isAllowedRule($rule, $user, $userRoles, $contenttype, $contentid);
$isAllowed = $this->isAllowedRule($rule, $user, $userRoles, $content, $contenttypeSlug, $contentId);

// Cache for the current request
$this->rqcache[$rqCacheKey] = $isAllowed;
Expand All @@ -507,36 +509,37 @@ public function isAllowed($what, $user, $contenttype = null, $contentid = null)
/**
* Check if a user is allowed a rule 'type'.
*
* @param array $rule
* @param array $user
* @param array $userRoles
* @param string $contenttype
* @param integer $contentid
* @param array $rule
* @param array $user
* @param array $userRoles
* @param string|array|Content $content
* @param string $contenttype
* @param integer $contentid
*
* @throws \Exception
*
* @return boolean
*/
private function isAllowedRule($rule, $user, $userRoles, $contenttype, $contentid)
private function isAllowedRule($rule, $user, $userRoles, $content, $contenttypeSlug, $contentid)
{
switch ($rule['type']) {
case PermissionParser::P_TRUE:
return true;
case PermissionParser::P_FALSE:
return false;
case PermissionParser::P_SIMPLE:
return $this->isAllowedSingle($rule['value'], $user, $userRoles, $contenttype, $contentid);
return $this->isAllowedSingle($rule['value'], $user, $userRoles, $content, $contenttypeSlug, $contentid);
case PermissionParser::P_OR:
foreach ($rule['value'] as $subrule) {
if ($this->isAllowedRule($subrule, $user, $userRoles, $contenttype, $contentid)) {
if ($this->isAllowedRule($subrule, $user, $userRoles, $content, $contenttypeSlug, $contentid)) {
return true;
}
}

return false;
case PermissionParser::P_AND:
foreach ($rule['value'] as $subrule) {
if (!$this->isAllowedRule($subrule, $user, $userRoles, $contenttype, $contentid)) {
if (!$this->isAllowedRule($subrule, $user, $userRoles, $content, $contenttypeSlug, $contentid)) {
return false;
}
}
Expand All @@ -550,22 +553,23 @@ private function isAllowedRule($rule, $user, $userRoles, $contenttype, $contenti
/**
* Check if a user has a specific role.
*
* @param string $what
* @param array $user
* @param array $userRoles
* @param string $contenttype
* @param integer $contentid
* @param string $what
* @param array $user
* @param array $userRoles
* @param string|array|Content $content
* @param string $contenttype
* @param integer $contentId
*
* @return boolean
*/
private function isAllowedSingle($what, $user, $userRoles, $contenttype = null, $contentid = null)
private function isAllowedSingle($what, $user, $userRoles, $content = null, $contenttypeSlug = null, $contentId = null)
{
if ($contenttype !== null) {
if ($content !== null) {
$parts = array(
'contenttype',
$contenttype,
$contenttypeSlug,
$what,
$contentid,
$contentId,
);
} else {
$parts = explode(':', $what);
Expand Down Expand Up @@ -605,31 +609,29 @@ private function isAllowedSingle($what, $user, $userRoles, $contenttype = null,

case 'contenttype':
$contenttype = $parts[1];
$permission = $contentid = null;
$permission = $contentId = null;
if (isset($parts[2])) {
$permission = $parts[2];
}
if (isset($parts[3])) {
$contentid = $parts[3];
$contentId = $parts[3];
}
if (empty($permission)) {
$permission = 'view';
}

// Handle special case for owner.
// It's a bit unfortunate that we have to fetch the content
// item for this, but since we're in the back-end, we probably
// won't see a lot of traffic here, so it's probably
// forgivable.
if (!empty($contentid)) {
// $contenttype must be a string, not an array.
if (is_array($contenttype)) {
$contenttype = $contenttype['slug'];
}
$content = $this->app['storage']->getContent("$contenttype/$contentid", array('hydrate' => false));
if (intval($content['ownerid']) &&
(intval($content['ownerid']) === intval($user['id']))) {
$userRoles[] = Permissions::ROLE_OWNER;
}
if ($contentId === null) {
break;
}

// If $content was passed in as a string, fetch the Content object
if (is_string($content)) {
$content = $this->app['storage']->getContent("$contenttypeSlug/$contentId", array('hydrate' => false));
}

if (intval($content['ownerid']) && (intval($content['ownerid']) === intval($user['id']))) {
$userRoles[] = Permissions::ROLE_OWNER;
}
break;

Expand Down
2 changes: 1 addition & 1 deletion src/Twig/Handler/UserHandler.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public function isAllowed($what, $content = null)
$contentid = null;
if ($content instanceof \Bolt\Content) {
// It's a content record
$contenttype = $content->contenttype;
$contenttype = $content;
$contentid = $content['id'];
} elseif (is_array($content)) {
// It's a contenttype
Expand Down
10 changes: 9 additions & 1 deletion src/Users.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -841,7 +841,7 @@ public function getUsers()
$tempusers = $queryBuilder->execute()->fetchAll();

foreach ($tempusers as $user) {
$key = $user['username'];
$key = $user['id'];
$this->users[$key] = $user;
$this->users[$key]['password'] = '**dontchange**';

Expand Down Expand Up @@ -897,6 +897,14 @@ public function getUser($id)
}
}

// Make sure users have been 'got' already.
$this->getUsers();

// In most cases by far, we'll request an ID, and we can return it here.
if (array_key_exists($id, $this->users)) {
return $this->users[$id];
}

/** @var \Doctrine\DBAL\Query\QueryBuilder $queryBuilder */
$queryBuilder = $this->app['db']->createQueryBuilder()
->select('*')
Expand Down
2 changes: 1 addition & 1 deletion tests/phpunit/unit/Storage/StorageTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public function testGetContentObject()
public function testPreFill()
{
$app = $this->getApp();
$app['users']->users = array(1 => $this->addDefaultUser($app));
$this->addDefaultUser($app);
$prefillMock = new LoripsumMock();
$app['prefill'] = $prefillMock;

Expand Down
4 changes: 0 additions & 4 deletions tests/phpunit/unit/Users/UsersTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ public function testGetUserById()
{
// Setup test
$users = $this->getMock('Bolt\Users', array('getUsers'), array($this->getApp()));
$users->users = array($this->user);

// Run test
$result = $users->getUser(2);
Expand All @@ -53,7 +52,6 @@ public function testGetUserByUnknownId()
{
// Setup test
$users = $this->getMock('Bolt\Users', array('getUsers'), array($this->getApp()));
$users->users = array($this->user);

// Run test
$result = $users->getUser(0);
Expand All @@ -69,7 +67,6 @@ public function testGetUserByUsername()
{
// Setup test
$users = $this->getMock('Bolt\Users', array('getUsers'), array($this->getApp()));
$users->users = array($this->user);

// Run test
$result = $users->getUser('editor');
Expand All @@ -87,7 +84,6 @@ public function testGetUserByUnknownUsername()
{
// Setup test
$users = $this->getMock('Bolt\Users', array('getUsers'), array($this->getApp()));
$users->users = array($this->user);

// Run test
$result = $users->getUser('anotheruser');
Expand Down

0 comments on commit b1cb966

Please sign in to comment.