Merge pull request #7674 from open-sausages/pulls/4.0/permission-impossible

ENHANCEMENT: Add caching to InheritedPermissions

_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"

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)) {
+        $result = $result->forForeignID($familyIDs);
-            $groups = Group::get()->byIDs($familyIDs);
+        
-            $groupIDs = $groups->column('ID');
+        return $result->where($filter);
-            if (!empty($groupIDs)) {
-                $result = $result->forForeignID($groupIDs)->where($filter);
-            }
-        }
-
-        return $result;
     }
 
     /**
@@ -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);
                 }

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()
+            return $this->owner->Members()->column('ID');
-                ? $this->owner->Members()->column('ID')
-                : null;
         }
 
         return [$this->owner->ID];

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}";
+        $cacheKey = $this->generateCacheKey($type, $memberID);
-        if ($useCached && isset($this->cachePermissions[$cacheKey])) {
+        $cachePermissions = $this->getCachePermissions($cacheKey);
-            $cachedValues = array_intersect_key($this->cachePermissions[$cacheKey], $result);
+        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])) {
+        $cachePermissions = $this->getCachePermissions($cacheKey);
-            $cachedValues = array_intersect_key($this->cachePermissions[$cacheKey], $result);
+        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}";
+    }
 }

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]);
+    }
 }