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/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 19ca9de7e..4401c2400 100644 --- a/src/Security/InheritedPermissionFlusher.php +++ b/src/Security/InheritedPermissionFlusher.php @@ -16,7 +16,7 @@ class InheritedPermissionFlusher extends DataExtension implements Flushable protected $services = []; /** - * Flush all CacheFlusher services + * Flush all MemberCacheFlusher services */ public static function flush() { @@ -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 b10014fa5..922047a42 100644 --- a/src/Security/InheritedPermissions.php +++ b/src/Security/InheritedPermissions.php @@ -8,6 +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\MemberCacheFlusher; /** * Calculates batch permissions for nested objects for: @@ -16,7 +18,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, MemberCacheFlusher { use Injectable; @@ -84,20 +86,70 @@ 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 $memberIDs A list of member IDs + */ + public function flushMemberCache($memberIDs = null) + { + if (!$this->cacheService) { + return; + } + + // Hard flush, e.g. flush=1 + if (!$memberIDs) { + $this->cacheService->clear(); + } + + if ($memberIDs && is_array($memberIDs)) { + foreach ([self::VIEW, self::EDIT, self::DELETE] as $type) { + foreach ($memberIDs as $memberID) { + $key = $this->generateCacheKey($type, $memberID); + $this->cacheService->delete($key); + } + } + } + } + /** * @param DefaultPermissionChecker $callback * @return $this @@ -212,12 +264,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 +294,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 +330,7 @@ class InheritedPermissions implements PermissionChecker if ($combinedStageResult) { $this->cachePermissions[$cacheKey] = $combinedStageResult + $this->cachePermissions[$cacheKey]; } + return $combinedStageResult; } @@ -367,6 +421,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 +438,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 +472,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 +524,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 +538,7 @@ class InheritedPermissions implements PermissionChecker // Regular canEdit logic is handled by canEditMultiple $results = $this->canDeleteMultiple( - [ $id ], + [$id], $member ); @@ -468,6 +546,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 +560,7 @@ class InheritedPermissions implements PermissionChecker // Regular canEdit logic is handled by canEditMultiple $results = $this->canEditMultiple( - [ $id ], + [$id], $member ); @@ -485,6 +568,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 +582,7 @@ class InheritedPermissions implements PermissionChecker // Regular canView logic is handled by canViewMultiple $results = $this->canViewMultiple( - [ $id ], + [$id], $member ); @@ -583,6 +671,9 @@ class InheritedPermissions implements PermissionChecker return $singleton->hasExtension(Versioned::class); } + /** + * @return $this + */ public function clearCache() { $this->cachePermissions = []; @@ -610,4 +701,43 @@ class InheritedPermissions implements PermissionChecker $table = DataObject::getSchema()->tableName($this->baseClass); return "{$table}_ViewerGroups"; } + + /** + * Gets the permission from cache + * + * @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) { + $result = $this->cacheService->get($cacheKey); + + // Warm local cache + if ($result) { + $this->cachePermissions[$cacheKey] = $result; + return $result; + } + } + + return null; + } + + /** + * Creates a cache key for a member and type + * + * @param string $type + * @param int $memberID + * @return string + */ + protected function generateCacheKey($type, $memberID) + { + return "{$type}-{$memberID}"; + } } diff --git a/tests/php/Security/InheritedPermissionsTest.php b/tests/php/Security/InheritedPermissionsTest.php index 0d4cbc7f6..c9b3754fc 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,112 @@ 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->flushMemberCache('dummy'); + foreach ([$editKey1, $editKey2, $viewKey1, $viewKey2] as $key) { + $this->assertNotNull($cache->get($key)); + } + + // Precision strike + $permissionChecker->flushMemberCache([$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->flushMemberCache(); + 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]); + } }