From aefb0aeaa84728d581be3cb9e277f070291995c7 Mon Sep 17 00:00:00 2001 From: Aaron Carlino Date: Mon, 4 Dec 2017 14:23:14 +1300 Subject: [PATCH] Make InheritedPermissions use cache and implement cache flushing --- _config/cache.yml | 4 + _config/security.yml | 4 + src/Security/InheritedPermissions.php | 149 ++++++++++++++++-- .../php/Security/InheritedPermissionsTest.php | 112 +++++++++++++ 4 files changed, 254 insertions(+), 15 deletions(-) diff --git a/_config/cache.yml b/_config/cache.yml index 9bcb5dc47..7f7344132 100644 --- a/_config/cache.yml +++ b/_config/cache.yml @@ -22,3 +22,7 @@ SilverStripe\Core\Injector\Injector: factory: SilverStripe\Core\Cache\CacheFactory constructor: namespace: 'ratelimiter' + Psr\SimpleCache\CacheInterface.InheritedPermissions: + factory: SilverStripe\Core\Cache\CacheFactory + constructor: + namespace: "InheritedPermissions" \ No newline at end of file diff --git a/_config/security.yml b/_config/security.yml index 40273a393..c65f5a3c7 100644 --- a/_config/security.yml +++ b/_config/security.yml @@ -38,3 +38,7 @@ 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/InheritedPermissions.php b/src/Security/InheritedPermissions.php index b10014fa5..03c55ca70 100644 --- a/src/Security/InheritedPermissions.php +++ b/src/Security/InheritedPermissions.php @@ -8,7 +8,8 @@ use SilverStripe\ORM\DataList; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\Hierarchy\Hierarchy; use SilverStripe\Versioned\Versioned; - +use Psr\SimpleCache\CacheInterface; +use SilverStripe\Core\Cache\CacheFlusher; /** * Calculates batch permissions for nested objects for: * - canView: Supports 'Anyone' type @@ -16,7 +17,7 @@ use SilverStripe\Versioned\Versioned; * - canDelete: Includes special logic for ensuring parent objects can only be deleted if their children can * be deleted also. */ -class InheritedPermissions implements PermissionChecker +class InheritedPermissions implements PermissionChecker, CacheFlusher { use Injectable; @@ -84,20 +85,69 @@ class InheritedPermissions implements PermissionChecker */ protected $cachePermissions = []; + /** + * @var CacheInterface + */ + protected $cacheService; + /** * Construct new permissions object * * @param string $baseClass Base class + * @param CacheInterface $cache */ - public function __construct($baseClass) + public function __construct($baseClass, CacheInterface $cache = null) { if (!is_a($baseClass, DataObject::class, true)) { throw new InvalidArgumentException('Invalid DataObject class: ' . $baseClass); } + $this->baseClass = $baseClass; + $this->cacheService = $cache; + return $this; } + /** + * Commits the cache + */ + public function __destruct() + { + // Ensure back-end cache is updated + if (!empty($this->cachePermissions) && $this->cacheService) { + foreach ($this->cachePermissions as $key => $permissions) { + $this->cacheService->set($key, $permissions); + } + // Prevent double-destruct + $this->cachePermissions = []; + } + } + + /** + * Clear the cache for this instance only + * @param array $ids A list of member IDs + */ + public function flushCache($ids = null) + { + if (!$this->cacheService) { + return; + } + + // Hard flush, e.g. flush=1 + if (!$ids) { + $this->cacheService->clear(); + } + + if ($ids && is_array($ids)) { + foreach ([self::VIEW, self::EDIT, self::DELETE] as $type) { + foreach ($ids as $memberID) { + $key = $this->generateCacheKey($type, $memberID); + $this->cacheService->delete($key); + } + } + } + } + /** * @param DefaultPermissionChecker $callback * @return $this @@ -212,12 +262,13 @@ class InheritedPermissions implements PermissionChecker } // Look in the cache for values - $cacheKey = "{$type}-{$memberID}"; - if ($useCached && isset($this->cachePermissions[$cacheKey])) { - $cachedValues = array_intersect_key($this->cachePermissions[$cacheKey], $result); + $cacheKey = $this->generateCacheKey($type, $memberID); + $cachePermissions = $this->getCachePermissions($cacheKey); + if ($useCached && $cachePermissions) { + $cachedValues = array_intersect_key($cachePermissions, $result); // If we can't find everything in the cache, then look up the remainder separately - $uncachedIDs = array_keys(array_diff_key($result, $this->cachePermissions[$cacheKey])); + $uncachedIDs = array_keys(array_diff_key($result, $cachePermissions)); if ($uncachedIDs) { $uncachedValues = $this->batchPermissionCheck($type, $uncachedIDs, $member, $globalPermission, false); return $cachedValues + $uncachedValues; @@ -241,7 +292,7 @@ class InheritedPermissions implements PermissionChecker if ($this->isVersioned()) { // Check all records for each stage and merge $combinedStageResult = []; - foreach ([ Versioned::DRAFT, Versioned::LIVE ] as $stage) { + foreach ([Versioned::DRAFT, Versioned::LIVE] as $stage) { $stageRecords = Versioned::get_by_stage($this->getBaseClass(), $stage) ->byIDs($ids); // Exclude previously calculated records from later stage calculations @@ -277,6 +328,7 @@ class InheritedPermissions implements PermissionChecker if ($combinedStageResult) { $this->cachePermissions[$cacheKey] = $combinedStageResult + $this->cachePermissions[$cacheKey]; } + return $combinedStageResult; } @@ -367,6 +419,12 @@ class InheritedPermissions implements PermissionChecker return $result; } + /** + * @param array $ids + * @param Member|null $member + * @param bool $useCached + * @return array + */ public function canEditMultiple($ids, Member $member = null, $useCached = true) { return $this->batchPermissionCheck( @@ -378,11 +436,23 @@ class InheritedPermissions implements PermissionChecker ); } + /** + * @param array $ids + * @param Member|null $member + * @param bool $useCached + * @return array + */ public function canViewMultiple($ids, Member $member = null, $useCached = true) { return $this->batchPermissionCheck(self::VIEW, $ids, $member, [], $useCached); } + /** + * @param array $ids + * @param Member|null $member + * @param bool $useCached + * @return array + */ public function canDeleteMultiple($ids, Member $member = null, $useCached = true) { // Validate ids @@ -400,11 +470,12 @@ class InheritedPermissions implements PermissionChecker // Look in the cache for values $cacheKey = "delete-{$member->ID}"; - if ($useCached && isset($this->cachePermissions[$cacheKey])) { - $cachedValues = array_intersect_key($this->cachePermissions[$cacheKey], $result); + $cachePermissions = $this->getCachePermissions($cacheKey); + if ($useCached && $cachePermissions) { + $cachedValues = array_intersect_key($cachePermissions[$cacheKey], $result); // If we can't find everything in the cache, then look up the remainder separately - $uncachedIDs = array_keys(array_diff_key($result, $this->cachePermissions[$cacheKey])); + $uncachedIDs = array_keys(array_diff_key($result, $cachePermissions[$cacheKey])); if ($uncachedIDs) { $uncachedValues = $this->canDeleteMultiple($uncachedIDs, $member, false); return $cachedValues + $uncachedValues; @@ -451,6 +522,11 @@ class InheritedPermissions implements PermissionChecker return array_fill_keys($deletable, true) + array_fill_keys($ids, false); } + /** + * @param int $id + * @param Member|null $member + * @return bool|mixed + */ public function canDelete($id, Member $member = null) { // No ID: Check default permission @@ -460,7 +536,7 @@ class InheritedPermissions implements PermissionChecker // Regular canEdit logic is handled by canEditMultiple $results = $this->canDeleteMultiple( - [ $id ], + [$id], $member ); @@ -468,6 +544,11 @@ class InheritedPermissions implements PermissionChecker return isset($results[$id]) ? $results[$id] : false; } + /** + * @param int $id + * @param Member|null $member + * @return bool|mixed + */ public function canEdit($id, Member $member = null) { // No ID: Check default permission @@ -477,7 +558,7 @@ class InheritedPermissions implements PermissionChecker // Regular canEdit logic is handled by canEditMultiple $results = $this->canEditMultiple( - [ $id ], + [$id], $member ); @@ -485,6 +566,11 @@ class InheritedPermissions implements PermissionChecker return isset($results[$id]) ? $results[$id] : false; } + /** + * @param int $id + * @param Member|null $member + * @return bool|mixed + */ public function canView($id, Member $member = null) { // No ID: Check default permission @@ -494,7 +580,7 @@ class InheritedPermissions implements PermissionChecker // Regular canView logic is handled by canViewMultiple $results = $this->canViewMultiple( - [ $id ], + [$id], $member ); @@ -583,6 +669,9 @@ class InheritedPermissions implements PermissionChecker return $singleton->hasExtension(Versioned::class); } + /** + * @return $this + */ public function clearCache() { $this->cachePermissions = []; @@ -610,4 +699,34 @@ class InheritedPermissions implements PermissionChecker $table = DataObject::getSchema()->tableName($this->baseClass); return "{$table}_ViewerGroups"; } -} + + /** + * Gets the permission from cache + * + * @param $cacheKey + * @return mixed + */ + protected function getCachePermissions($cacheKey) + { + if (isset($this->cachePermissions[$cacheKey])) { + return $this->cachePermissions[$cacheKey]; + } + + if ($this->cacheService) { + return $this->cacheService->get($cacheKey); + } + + return null; + } + + /** + * Creates a cache key for a member and type + * @param $type + * @param $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 0d4cbc7f6..6e934920f 100644 --- a/tests/php/Security/InheritedPermissionsTest.php +++ b/tests/php/Security/InheritedPermissionsTest.php @@ -11,6 +11,8 @@ use SilverStripe\Security\PermissionChecker; use SilverStripe\Security\Test\InheritedPermissionsTest\TestPermissionNode; use SilverStripe\Security\Test\InheritedPermissionsTest\TestDefaultPermissionChecker; use SilverStripe\Versioned\Versioned; +use Psr\SimpleCache\CacheInterface; +use ReflectionClass; class InheritedPermissionsTest extends SapphireTest { @@ -266,4 +268,114 @@ class InheritedPermissionsTest extends SapphireTest $this->assertFalse($history->canView()); }); } + + public function testPermissionsPersistCache() + { + /* @var CacheInterface $cache */ + $cache = Injector::inst()->create(CacheInterface::class . '.InheritedPermissions'); + $cache->clear(); + + $member = $this->objFromFixture(Member::class, 'editor'); + + /** @var TestPermissionNode $history */ + $history = $this->objFromFixture(TestPermissionNode::class, 'history'); + /** @var TestPermissionNode $historyGallery */ + $historyGallery = $this->objFromFixture(TestPermissionNode::class, 'history-gallery'); + $permissionChecker = new InheritedPermissions(TestPermissionNode::class, $cache); + + $viewKey = $this->generateCacheKey($permissionChecker, InheritedPermissions::VIEW, $member->ID); + $editKey = $this->generateCacheKey($permissionChecker, InheritedPermissions::EDIT, $member->ID); + + $this->assertNull($cache->get($editKey)); + $this->assertNull($cache->get($viewKey)); + + $permissionChecker->canEditMultiple([$history->ID, $historyGallery->ID], $member); + $this->assertNull($cache->get($editKey)); + $this->assertNull($cache->get($viewKey)); + + unset($permissionChecker); + $this->assertTrue(is_array($cache->get($editKey))); + $this->assertNull($cache->get($viewKey)); + $this->assertArrayHasKey($history->ID, $cache->get($editKey)); + $this->assertArrayHasKey($historyGallery->ID, $cache->get($editKey)); + + $permissionChecker = new InheritedPermissions(TestPermissionNode::class, $cache); + $permissionChecker->canViewMultiple([$history->ID], $member); + $this->assertNotNull($cache->get($editKey)); + $this->assertNull($cache->get($viewKey)); + + unset($permissionChecker); + $this->assertTrue(is_array($cache->get($viewKey))); + $this->assertTrue(is_array($cache->get($editKey))); + $this->assertArrayHasKey($history->ID, $cache->get($viewKey)); + $this->assertArrayNotHasKey($historyGallery->ID, $cache->get($viewKey)); + } + + public function testPermissionsFlushCache() + { + /* @var CacheInterface $cache */ + $cache = Injector::inst()->create(CacheInterface::class . '.InheritedPermissions'); + $cache->clear(); + + $permissionChecker = new InheritedPermissions(TestPermissionNode::class, $cache); + $member1 = $this->objFromFixture(Member::class, 'editor'); + $member2 = $this->objFromFixture(Member::class, 'admin'); + $editKey1 = $this->generateCacheKey($permissionChecker, InheritedPermissions::EDIT, $member1->ID); + $editKey2 = $this->generateCacheKey($permissionChecker, InheritedPermissions::EDIT, $member2->ID); + $viewKey1 = $this->generateCacheKey($permissionChecker, InheritedPermissions::VIEW, $member1->ID); + $viewKey2 = $this->generateCacheKey($permissionChecker, InheritedPermissions::VIEW, $member2->ID); + + foreach([$editKey1, $editKey2, $viewKey1, $viewKey2] as $key) { + $this->assertNull($cache->get($key)); + } + + /** @var TestPermissionNode $history */ + $history = $this->objFromFixture(TestPermissionNode::class, 'history'); + /** @var TestPermissionNode $historyGallery */ + $historyGallery = $this->objFromFixture(TestPermissionNode::class, 'history-gallery'); + + $permissionChecker->canEditMultiple([$history->ID, $historyGallery->ID], $member1); + $permissionChecker->canViewMultiple([$history->ID, $historyGallery->ID], $member1); + $permissionChecker->canEditMultiple([$history->ID, $historyGallery->ID], $member2); + $permissionChecker->canViewMultiple([$history->ID, $historyGallery->ID], $member2); + + unset($permissionChecker); + + foreach([$editKey1, $editKey2, $viewKey1, $viewKey2] as $key) { + $this->assertNotNull($cache->get($key)); + } + $permissionChecker = new InheritedPermissions(TestPermissionNode::class, $cache); + + // Non existent ID + $permissionChecker->flushCache('dummy'); + foreach([$editKey1, $editKey2, $viewKey1, $viewKey2] as $key) { + $this->assertNotNull($cache->get($key)); + } + + // Precision strike + $permissionChecker->flushCache([$member1->ID]); + // Member1 should be clear + $this->assertNull($cache->get($editKey1)); + $this->assertNull($cache->get($viewKey1)); + // Member 2 is unaffected + $this->assertNotNull($cache->get($editKey2)); + $this->assertNotNull($cache->get($viewKey2)); + + // Nuclear + $permissionChecker->flushCache(); + foreach([$editKey1, $editKey2, $viewKey1, $viewKey2] as $key) { + $this->assertNull($cache->get($key)); + } + + } + + protected function generateCacheKey(InheritedPermissions $inst, $type, $memberID) + { + $reflection = new ReflectionClass(InheritedPermissions::class); + $method = $reflection->getMethod('generateCacheKey'); + $method->setAccessible(true); + + return $method->invokeArgs($inst, [$type, $memberID]); + } + }