From beda5d38202d3822f2f149c3aebf45c245bcbc34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Burnichon?= Date: Wed, 27 Jan 2016 17:16:49 +0100 Subject: [PATCH] Getting collection calls access_restriction which is not properly cached. Remove use of cache in AccessRestriction and use instance memory cache instead. Beware static keyword declares variable static for the class, not the instance --- .../Core/Configuration/AccessRestriction.php | 89 ++++++++++++------- .../Provider/ConfigurationServiceProvider.php | 2 +- lib/classes/ACL.php | 6 +- lib/classes/appbox.php | 2 +- lib/classes/collection.php | 83 ++++++++++------- .../Configuration/AccessRestrictionTest.php | 2 +- 6 files changed, 111 insertions(+), 73 deletions(-) diff --git a/lib/Alchemy/Phrasea/Core/Configuration/AccessRestriction.php b/lib/Alchemy/Phrasea/Core/Configuration/AccessRestriction.php index ef76784639..ff5ccd6a8a 100644 --- a/lib/Alchemy/Phrasea/Core/Configuration/AccessRestriction.php +++ b/lib/Alchemy/Phrasea/Core/Configuration/AccessRestriction.php @@ -1,9 +1,8 @@ conf = $conf; + $this->propertyAccess = $propertyAccess; $this->appbox = $appbox; $this->logger = $logger; - $this->cache = $cache; } /** * Returns true if a configuration is set. * - * @return Boolean + * @return bool */ public function isRestricted() { $this->load(); - return $this->cache->fetch('restricted'); + return $this->restricted; } /** @@ -49,7 +74,7 @@ class AccessRestriction * * @param \databox $databox * - * @return Boolean + * @return bool */ public function isDataboxAvailable(\databox $databox) { @@ -57,7 +82,7 @@ class AccessRestriction return true; } - return in_array($databox->get_sbas_id(), $this->cache->fetch('available_databoxes'), true); + return in_array($databox->get_sbas_id(), $this->availableDataboxes, true); } /** @@ -72,7 +97,7 @@ class AccessRestriction return $databoxes; } - $available = array_flip($this->cache->fetch('available_databoxes')); + $available = array_flip($this->availableDataboxes); return array_filter($databoxes, function (\databox $databox) use ($available) { return isset($available[$databox->get_sbas_id()]); @@ -84,7 +109,7 @@ class AccessRestriction * * @param \collection $collection * - * @return Boolean + * @return bool */ public function isCollectionAvailable(\collection $collection) { @@ -92,30 +117,31 @@ class AccessRestriction return true; } - $availableCollections = $this->cache->fetch('available_collections_'.$collection->get_databox()->get_sbas_id()) ?: []; + $availableCollections = isset($this->availableCollections[$collection->get_sbas_id()]) + ? $this->availableCollections[$collection->get_sbas_id()] : []; return in_array($collection->get_base_id(), $availableCollections, true); } private function load() { - if ($this->cache->fetch('loaded')) { + if ($this->loaded) { return; } - $this->cache->save('loaded', true); + $this->loaded = true; $allowedDataboxIds = array_map(function ($dbConf) { return $dbConf['id']; - }, $this->conf->get('databoxes', [])); + }, $this->propertyAccess->get('databoxes', [])); if (count($allowedDataboxIds) === 0) { - $this->cache->save('restricted', false); + $this->restricted = false; return; } - $this->cache->save('restricted', true); + $this->restricted = true; $databoxIds = array_map(function (\databox $databox) { return $databox->get_sbas_id(); }, $this->appbox->get_databoxes()); $errors = array_diff($allowedDataboxIds, $databoxIds); @@ -124,18 +150,15 @@ class AccessRestriction $this->logger->error(sprintf('Misconfiguration for allowed databoxes : ids %s do not exist', implode(', ', $errors))); } - $allowedDataboxIds = array_intersect($allowedDataboxIds, $databoxIds); - $this->cache->save('available_databoxes', $allowedDataboxIds); + $this->availableDataboxes = array_intersect($allowedDataboxIds, $databoxIds); $this->loadCollections(); } private function loadCollections() { - $allowedDataboxIds = $this->cache->fetch('available_databoxes'); - - foreach ($this->conf->get('databoxes') as $databox) { - if (!in_array($databox['id'], $allowedDataboxIds, true)) { + foreach ($this->propertyAccess->get('databoxes') as $databox) { + if (!in_array($databox['id'], $this->availableDataboxes, true)) { continue; } @@ -148,9 +171,7 @@ class AccessRestriction $this->logger->error(sprintf('Misconfiguration for allowed collections : ids %s do not exist', implode(', ', $errors))); } - $collections = array_intersect($collections, $availableBaseIds); - - $this->cache->save('available_collections_'.$databox['id'], $collections); + $this->availableCollections[$databox['id']] = array_intersect($collections, $availableBaseIds); } } } diff --git a/lib/Alchemy/Phrasea/Core/Provider/ConfigurationServiceProvider.php b/lib/Alchemy/Phrasea/Core/Provider/ConfigurationServiceProvider.php index a684fa6412..55623e0081 100644 --- a/lib/Alchemy/Phrasea/Core/Provider/ConfigurationServiceProvider.php +++ b/lib/Alchemy/Phrasea/Core/Provider/ConfigurationServiceProvider.php @@ -69,7 +69,7 @@ class ConfigurationServiceProvider implements ServiceProviderInterface }); $app['conf.restrictions'] = $app->share(function (SilexApplication $app) { - return new AccessRestriction($app['cache'], $app['conf'], $app->getApplicationBox(), $app['monolog']); + return new AccessRestriction($app['conf'], $app->getApplicationBox(), $app['monolog']); }); } diff --git a/lib/classes/ACL.php b/lib/classes/ACL.php index ae3534f7ec..b28c134af6 100644 --- a/lib/classes/ACL.php +++ b/lib/classes/ACL.php @@ -730,11 +730,7 @@ class ACL implements cache_cacheableInterface continue; } - try { - $ret[$base_id] = collection::getByBaseId($this->app, $base_id); - } catch (\Exception $e) { - - } + $ret[$base_id] = $collection; } } diff --git a/lib/classes/appbox.php b/lib/classes/appbox.php index b8ade4ce38..6309532b82 100644 --- a/lib/classes/appbox.php +++ b/lib/classes/appbox.php @@ -335,7 +335,7 @@ class appbox extends base /** * @return AccessRestriction */ - private function getAccessRestriction() + public function getAccessRestriction() { return $this->app['conf.restrictions']; } diff --git a/lib/classes/collection.php b/lib/classes/collection.php index ff8aa96f2c..7ac76177bb 100644 --- a/lib/classes/collection.php +++ b/lib/classes/collection.php @@ -188,52 +188,73 @@ class collection implements ThumbnailedElement, cache_cacheableInterface )); } - $repository = self::getRepository($app, $reference->getDataboxId()); - $collection = $repository->find($reference->getCollectionId()); + return self::getAvailableCollection($app, $reference->getDataboxId(), $reference->getCollectionId()); + } - if (!$collection) { - throw new Exception_Databox_CollectionNotFound(sprintf( - "Collection with base_id %s could not be found", - $base_id - )); - } + /** + * @param Application $app + * @param databox $databox + * @param int $collectionId + * @return collection + */ + public static function getByCollectionId(Application $app, databox $databox, $collectionId) + { + assert(is_int($collectionId)); - if (!$app['conf.restrictions']->isCollectionAvailable($collection)) { + return self::getAvailableCollection($app, $databox->get_sbas_id(), $collectionId); + } + + /** + * @param Application $app + * @return \Alchemy\Phrasea\Core\Configuration\AccessRestriction + */ + private static function getAccessRestriction(Application $app) + { + return $app['conf.restrictions']; + } + + private static function assertCollectionIsAvailable(Application $app, collection $collection) + { + if (!self::getAccessRestriction($app)->isCollectionAvailable($collection)) { throw new Exception_Databox_CollectionNotFound(sprintf( 'Collection `%d` is not available here.', $collection->get_base_id() )); } + } + + /** + * @param Application $app + * @param int $databoxId + * @param int $collectionId + * @return collection + */ + private static function getByDataboxIdAndCollectionId(Application $app, $databoxId, $collectionId) + { + $repository = self::getRepository($app, $databoxId); + $collection = $repository->find($collectionId); + + if (!$collection) { + throw new Exception_Databox_CollectionNotFound(sprintf( + "Collection '%d' on databox '%d' could not be found", + $collectionId, + $databoxId + )); + } return $collection; } /** - * @param Application $app - * @param databox $databox - * @param int $coll_id + * @param Application $app + * @param int $databoxId + * @param int $collectionId * @return collection */ - public static function getByCollectionId(Application $app, databox $databox, $coll_id) + private static function getAvailableCollection(Application $app, $databoxId, $collectionId) { - assert(is_int($coll_id)); - - $repository = self::getRepository($app, $databox->get_sbas_id()); - $collection = $repository->find($coll_id); - - if (!$collection) { - throw new Exception_Databox_CollectionNotFound(sprintf( - "Collection with collection ID %d could not be found", - $coll_id - )); - } - - if (!$app['conf.restrictions']->isCollectionAvailable($collection)) { - throw new Exception_Databox_CollectionNotFound(sprintf( - 'Collection `%d` is not available here.', - $collection->get_base_id() - )); - } + $collection = self::getByDataboxIdAndCollectionId($app, $databoxId, $collectionId); + self::assertCollectionIsAvailable($app, $collection); return $collection; } diff --git a/tests/Alchemy/Tests/Phrasea/Core/Configuration/AccessRestrictionTest.php b/tests/Alchemy/Tests/Phrasea/Core/Configuration/AccessRestrictionTest.php index d105663d78..41955b2dc4 100644 --- a/tests/Alchemy/Tests/Phrasea/Core/Configuration/AccessRestrictionTest.php +++ b/tests/Alchemy/Tests/Phrasea/Core/Configuration/AccessRestrictionTest.php @@ -21,7 +21,7 @@ class AccessRestrictionTest extends \PhraseanetTestCase $conf = new MockArrayConf($conf); $logger = $this->createLoggerMock(); - $restriction = new AccessRestriction(new ArrayCache(), new PropertyAccess($conf), self::$DI['app']['phraseanet.appbox'], $logger); + $restriction = new AccessRestriction(new PropertyAccess($conf), self::$DI['app']['phraseanet.appbox'], $logger); $this->assertEquals($restricted, $restriction->isRestricted()); foreach ($collAccess as $coll) {