From 33b2d50d59b6534f512af09ad2d976a5b3afef07 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 11 Dec 2017 17:49:23 +1300 Subject: [PATCH] Cache warming in InheritedPermissions::getCachePermissions() Simplify Group::Members() code Remove cms-only config --- _config/security.yml | 4 ---- src/Security/Group.php | 19 +++++++-------- src/Security/InheritedPermissionFlusher.php | 4 +--- src/Security/InheritedPermissions.php | 23 ++++++++++++++----- .../php/Security/InheritedPermissionsTest.php | 10 ++++---- 5 files changed, 30 insertions(+), 30 deletions(-) diff --git a/_config/security.yml b/_config/security.yml index c65f5a3c7..40273a393 100644 --- a/_config/security.yml +++ b/_config/security.yml @@ -38,7 +38,3 @@ SilverStripe\Core\Injector\Injector: Authenticators: cms: '%$SilverStripe\Security\MemberAuthenticator\CMSMemberAuthenticator' SilverStripe\Security\IdentityStore: '%$SilverStripe\Security\AuthenticationHandler' - SilverStripe\Security\InheritedPermissionFlusher: - properties: - Services: - - '%$SilverStripe\Security\PermissionChecker.sitetree' \ No newline at end of file diff --git a/src/Security/Group.php b/src/Security/Group.php index 6e49644d9..6e3313dff 100755 --- a/src/Security/Group.php +++ b/src/Security/Group.php @@ -305,7 +305,7 @@ class Group extends DataObject * including all members which are "inherited" from children groups of this record. * See {@link DirectMembers()} for retrieving members without any inheritance. * - * @param String $filter + * @param string $filter * @return ManyManyList */ public function Members($filter = '') @@ -330,15 +330,9 @@ class Group extends DataObject // Now set all children groups as a new foreign key $familyIDs = $this->collateFamilyIDs(); - if (!empty($familyIDs)) { - $groups = Group::get()->byIDs($familyIDs); - $groupIDs = $groups->column('ID'); - if (!empty($groupIDs)) { - $result = $result->forForeignID($groupIDs)->where($filter); - } - } - - return $result; + $result = $result->forForeignID($familyIDs); + + return $result->where($filter); } /** @@ -450,7 +444,7 @@ class Group extends DataObject } /** - * This isn't a decendant of SiteTree, but needs this in case + * This isn't a descendant of SiteTree, but needs this in case * the group is "reorganised"; */ public function cmsCleanup_parentChanged() @@ -626,6 +620,8 @@ class Group extends DataObject /** * Returns all of the children for the CMS Tree. * Filters to only those groups that the current user can edit + * + * @return ArrayList */ public function AllChildrenIncludingDeleted() { @@ -635,6 +631,7 @@ class Group extends DataObject if ($children) { foreach ($children as $child) { + /** @var DataObject $child */ if ($child->canView()) { $filteredChildren->push($child); } diff --git a/src/Security/InheritedPermissionFlusher.php b/src/Security/InheritedPermissionFlusher.php index 4a4f20e26..4401c2400 100644 --- a/src/Security/InheritedPermissionFlusher.php +++ b/src/Security/InheritedPermissionFlusher.php @@ -94,9 +94,7 @@ class InheritedPermissionFlusher extends DataExtension implements Flushable } if ($this->owner instanceof Group) { - return $this->owner->Members()->exists() - ? $this->owner->Members()->column('ID') - : null; + return $this->owner->Members()->column('ID'); } return [$this->owner->ID]; diff --git a/src/Security/InheritedPermissions.php b/src/Security/InheritedPermissions.php index fb8f4b8fe..922047a42 100644 --- a/src/Security/InheritedPermissions.php +++ b/src/Security/InheritedPermissions.php @@ -10,6 +10,7 @@ use SilverStripe\ORM\Hierarchy\Hierarchy; use SilverStripe\Versioned\Versioned; use Psr\SimpleCache\CacheInterface; use SilverStripe\Core\Cache\MemberCacheFlusher; + /** * Calculates batch permissions for nested objects for: * - canView: Supports 'Anyone' type @@ -125,7 +126,8 @@ class InheritedPermissions implements PermissionChecker, MemberCacheFlusher /** * Clear the cache for this instance only - * @param array $ids A list of member IDs + * + * @param array $memberIDs A list of member IDs */ public function flushMemberCache($memberIDs = null) { @@ -703,17 +705,25 @@ class InheritedPermissions implements PermissionChecker, MemberCacheFlusher /** * Gets the permission from cache * - * @param $cacheKey + * @param string $cacheKey * @return mixed */ protected function getCachePermissions($cacheKey) { + // Check local cache if (isset($this->cachePermissions[$cacheKey])) { return $this->cachePermissions[$cacheKey]; } + // Check persistent cache if ($this->cacheService) { - return $this->cacheService->get($cacheKey); + $result = $this->cacheService->get($cacheKey); + + // Warm local cache + if ($result) { + $this->cachePermissions[$cacheKey] = $result; + return $result; + } } return null; @@ -721,12 +731,13 @@ class InheritedPermissions implements PermissionChecker, MemberCacheFlusher /** * Creates a cache key for a member and type - * @param $type - * @param $memberID + * + * @param string $type + * @param int $memberID * @return string */ protected function generateCacheKey($type, $memberID) { return "{$type}-{$memberID}"; } -} \ No newline at end of file +} diff --git a/tests/php/Security/InheritedPermissionsTest.php b/tests/php/Security/InheritedPermissionsTest.php index 551f672a5..c9b3754fc 100644 --- a/tests/php/Security/InheritedPermissionsTest.php +++ b/tests/php/Security/InheritedPermissionsTest.php @@ -325,7 +325,7 @@ class InheritedPermissionsTest extends SapphireTest $viewKey1 = $this->generateCacheKey($permissionChecker, InheritedPermissions::VIEW, $member1->ID); $viewKey2 = $this->generateCacheKey($permissionChecker, InheritedPermissions::VIEW, $member2->ID); - foreach([$editKey1, $editKey2, $viewKey1, $viewKey2] as $key) { + foreach ([$editKey1, $editKey2, $viewKey1, $viewKey2] as $key) { $this->assertNull($cache->get($key)); } @@ -341,14 +341,14 @@ class InheritedPermissionsTest extends SapphireTest unset($permissionChecker); - foreach([$editKey1, $editKey2, $viewKey1, $viewKey2] as $key) { + foreach ([$editKey1, $editKey2, $viewKey1, $viewKey2] as $key) { $this->assertNotNull($cache->get($key)); } $permissionChecker = new InheritedPermissions(TestPermissionNode::class, $cache); // Non existent ID $permissionChecker->flushMemberCache('dummy'); - foreach([$editKey1, $editKey2, $viewKey1, $viewKey2] as $key) { + foreach ([$editKey1, $editKey2, $viewKey1, $viewKey2] as $key) { $this->assertNotNull($cache->get($key)); } @@ -363,10 +363,9 @@ class InheritedPermissionsTest extends SapphireTest // Nuclear $permissionChecker->flushMemberCache(); - foreach([$editKey1, $editKey2, $viewKey1, $viewKey2] as $key) { + foreach ([$editKey1, $editKey2, $viewKey1, $viewKey2] as $key) { $this->assertNull($cache->get($key)); } - } protected function generateCacheKey(InheritedPermissions $inst, $type, $memberID) @@ -377,5 +376,4 @@ class InheritedPermissionsTest extends SapphireTest return $method->invokeArgs($inst, [$type, $memberID]); } - }