Cache warming in InheritedPermissions::getCachePermissions()

Simplify Group::Members() code
Remove cms-only config

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

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

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];

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}";
     }
-}
+}

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