From 6fd2255e2e9b5f820687658900837845487e1929 Mon Sep 17 00:00:00 2001 From: "Nek (Maxime Veber)" Date: Mon, 17 Aug 2020 15:44:01 +0200 Subject: [PATCH] feat(crud): security is optionnal This commit removes a mandatory dependency for CRUD controllers. An exception will be throw in case the user didn't configure the security component but security for Melodiia. It comes with a huge BC break because it required to change the order of the properties while those controllers are designed to be extendable. I allowed this break to happen because we break on many things for the rebranding. --- CHANGELOG.md | 2 ++ docs/Crud.md | 13 +++++++++++- src/Bridge/Symfony/Resources/config/crud.yaml | 10 ++++----- src/Crud/Controller/Create.php | 10 +++------ src/Crud/Controller/CrudControllerTrait.php | 21 +++++++++++++++++++ src/Crud/Controller/Delete.php | 12 ++++------- src/Crud/Controller/Get.php | 12 ++++------- src/Crud/Controller/GetAll.php | 11 +++------- src/Crud/Controller/Update.php | 8 ++----- tests/Melodiia/Crud/Controller/DeleteTest.php | 4 ++-- tests/Melodiia/Crud/Controller/GetAllTest.php | 4 ++-- tests/Melodiia/Crud/Controller/GetTest.php | 2 +- tests/Melodiia/Crud/Controller/UpdateTest.php | 4 ++-- 13 files changed, 63 insertions(+), 50 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1dd320b..874bcdb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Huge BC Break on namespaces. You need to rename all classes used to SwagIndustries instead of Biig +- BC Break on CRUD classes. It's big changes time. So we made the security optional for crud controllers, this has a + consequence on their constructor ## [0.6.0] 2020-06-01 ### Added diff --git a/docs/Crud.md b/docs/Crud.md index 3f0c184..c974ac4 100644 --- a/docs/Crud.md +++ b/docs/Crud.md @@ -1,6 +1,18 @@ CRUDs with Melodiia =================== +Requirements +------------ + +The CRUD of Melodiia is based on the following components of Symfony. This means you need to use install & enable them: +- **The Serializer Component** + + Melodiia uses the serializer of Symfony to render responses. +- **The Form Component** + + Melodiia uses the form component to manage input. + + Register your first CRUD ------------------------ @@ -36,7 +48,6 @@ acme_article_get: defaults: melodiia_model: App\Entity\Article melodiia_security_check: 'some content runnable in AuthorizationChecker class' - ``` #### Collection diff --git a/src/Bridge/Symfony/Resources/config/crud.yaml b/src/Bridge/Symfony/Resources/config/crud.yaml index a17e747..aede679 100644 --- a/src/Bridge/Symfony/Resources/config/crud.yaml +++ b/src/Bridge/Symfony/Resources/config/crud.yaml @@ -27,7 +27,7 @@ services: $dataStore: '@melodiia.doctrine.data_provider' $formFactory: '@form.factory' $dispatcher: '@event_dispatcher' - $checker: '@security.authorization_checker' + $checker: '@?security.authorization_checker' tags: [ controller.service_arguments ] melodiia.crud.controller.update: @@ -36,32 +36,32 @@ services: $dataStore: '@melodiia.doctrine.data_provider' $formFactory: '@form.factory' $dispatcher: '@event_dispatcher' - $checker: '@security.authorization_checker' $idResolver: '@melodiia.crud.id_resolver' + $checker: '@?security.authorization_checker' tags: [ controller.service_arguments ] melodiia.crud.controller.get_all: class: SwagIndustries\Melodiia\Crud\Controller\GetAll arguments: $dataStore: '@melodiia.doctrine.data_provider' - $checker: '@security.authorization_checker' $collectionFactory: '@melodiia.crud.filters.filter_collection_factory' $pagesRequestFactory: '@SwagIndustries\Melodiia\Crud\Pagination\PaginationRequestFactoryInterface' + $checker: '@?security.authorization_checker' tags: [ controller.service_arguments ] melodiia.crud.controller.get: class: SwagIndustries\Melodiia\Crud\Controller\Get arguments: $dataStore: '@melodiia.doctrine.data_provider' - $checker: '@security.authorization_checker' $idResolver: '@melodiia.crud.id_resolver' + $checker: '@?security.authorization_checker' tags: [ controller.service_arguments ] melodiia.crud.controller.delete: class: SwagIndustries\Melodiia\Crud\Controller\Delete arguments: $dataStore: '@melodiia.doctrine.data_provider' - $checker: '@security.authorization_checker' $dispatcher: '@event_dispatcher' $idResolver: '@melodiia.crud.id_resolver' + $checker: '@?security.authorization_checker' tags: [ controller.service_arguments ] diff --git a/src/Crud/Controller/Create.php b/src/Crud/Controller/Create.php index b0ab144..b7679d6 100644 --- a/src/Crud/Controller/Create.php +++ b/src/Crud/Controller/Create.php @@ -12,7 +12,6 @@ use Symfony\Component\Form\FormFactoryInterface; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; -use Symfony\Component\Security\Core\Exception\AccessDeniedException; /** * Crud controller that create data model with the data from the request using a form. @@ -30,10 +29,10 @@ final class Create extends BaseCrudController /** @var FormFactoryInterface */ private $formFactory; - /** @var AuthorizationCheckerInterface */ + /** @var AuthorizationCheckerInterface|null */ private $checker; - public function __construct(DataStoreInterface $dataStore, FormFactoryInterface $formFactory, EventDispatcherInterface $dispatcher, AuthorizationCheckerInterface $checker) + public function __construct(DataStoreInterface $dataStore, FormFactoryInterface $formFactory, EventDispatcherInterface $dispatcher, AuthorizationCheckerInterface $checker = null) { parent::__construct($dispatcher); $this->dataStore = $dataStore; @@ -46,14 +45,11 @@ public function __invoke(Request $request): ApiResponse // Metadata you can specify in routing definition $modelClass = $request->attributes->get(self::MODEL_ATTRIBUTE); $form = $request->attributes->get(self::FORM_ATTRIBUTE); - $securityCheck = $request->attributes->get(self::SECURITY_CHECK, null); $clearMissing = $request->attributes->getBoolean(self::FORM_CLEAR_MISSING, true); $this->assertModelClassInvalid($modelClass); - if ($securityCheck && !$this->checker->isGranted($securityCheck)) { - throw new AccessDeniedException(\sprintf('Access denied to data of type "%s".', $modelClass)); - } + $this->assertResourceRights($request); if (empty($form) || !class_exists($form)) { throw new MelodiiaLogicException('If you use melodiia CRUD classes, you need to specify a model.'); diff --git a/src/Crud/Controller/CrudControllerTrait.php b/src/Crud/Controller/CrudControllerTrait.php index 2969e51..f79d704 100644 --- a/src/Crud/Controller/CrudControllerTrait.php +++ b/src/Crud/Controller/CrudControllerTrait.php @@ -5,7 +5,10 @@ namespace SwagIndustries\Melodiia\Crud\Controller; use SwagIndustries\Melodiia\Crud\CrudableModelInterface; +use SwagIndustries\Melodiia\Crud\CrudControllerInterface; use SwagIndustries\Melodiia\Exception\MelodiiaLogicException; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\Security\Core\Exception\AccessDeniedException; trait CrudControllerTrait { @@ -20,4 +23,22 @@ public function assertModelClassInvalid(string $modelClass): void throw new MelodiiaLogicException('If you use melodiia CRUD classes, you need to specify a model.'); } } + + private function assertResourceRights(Request $request, $data = null) + { + $securityCheck = $request->attributes->get(CrudControllerInterface::SECURITY_CHECK, null); + $modelClass = $request->attributes->get(CrudControllerInterface::MODEL_ATTRIBUTE); + + if (null === $securityCheck) { + return; + } + + if (!$this->checker) { + throw new MelodiiaLogicException('The security component of Symfony seems to not be enabled'); + } + + if (($data && !$this->checker->isGranted($securityCheck, $data)) || !$this->checker->isGranted($securityCheck)) { + throw new AccessDeniedException(\sprintf('Access denied to data of type "%s".', $modelClass)); + } + } } diff --git a/src/Crud/Controller/Delete.php b/src/Crud/Controller/Delete.php index 4eacc18..1342767 100644 --- a/src/Crud/Controller/Delete.php +++ b/src/Crud/Controller/Delete.php @@ -14,7 +14,6 @@ use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; -use Symfony\Component\Security\Core\Exception\AccessDeniedException; final class Delete extends BaseCrudController implements CrudControllerInterface { @@ -26,24 +25,23 @@ final class Delete extends BaseCrudController implements CrudControllerInterface /** @var DataStoreInterface */ private $dataStore; - /** @var AuthorizationCheckerInterface */ + /** @var AuthorizationCheckerInterface|null */ private $checker; /** @var IdResolverInterface */ private $idResolver; - public function __construct(DataStoreInterface $dataStore, AuthorizationCheckerInterface $checker, EventDispatcherInterface $dispatcher, IdResolverInterface $idResolver = null) + public function __construct(DataStoreInterface $dataStore, EventDispatcherInterface $dispatcher, IdResolverInterface $idResolver = null, AuthorizationCheckerInterface $checker = null) { parent::__construct($dispatcher); $this->dataStore = $dataStore; - $this->checker = $checker; $this->idResolver = $idResolver ?? new SimpleIdResolver(); + $this->checker = $checker; } public function __invoke(Request $request) { $modelClass = $request->attributes->get(self::MODEL_ATTRIBUTE); - $securityCheck = $request->attributes->get(self::SECURITY_CHECK, null); try { $id = $this->idResolver->resolveId($request, $modelClass); @@ -59,9 +57,7 @@ public function __invoke(Request $request) throw new NotFoundHttpException(\sprintf('resource item "%s" with id "%s" can not be found', $modelClass, $id)); } - if ($securityCheck && !$this->checker->isGranted($securityCheck, $data)) { - throw new AccessDeniedException(\sprintf('You can\'t perform a delete operation of the resource item "%s" with id "%s"', $modelClass, $id)); - } + $this->assertResourceRights($request, $data); $this->dispatch(new CrudEvent($data), self::EVENT_PRE_DELETE); $this->dataStore->remove($data); diff --git a/src/Crud/Controller/Get.php b/src/Crud/Controller/Get.php index 47a9fb4..fc4396a 100644 --- a/src/Crud/Controller/Get.php +++ b/src/Crud/Controller/Get.php @@ -13,7 +13,6 @@ use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; -use Symfony\Component\Security\Core\Exception\AccessDeniedException; final class Get implements CrudControllerInterface { @@ -22,24 +21,23 @@ final class Get implements CrudControllerInterface /** @var DataStoreInterface */ private $dataStore; - /** @var AuthorizationCheckerInterface */ + /** @var AuthorizationCheckerInterface|null */ private $checker; /** @var IdResolverInterface */ private $idResolver; - public function __construct(DataStoreInterface $dataStore, AuthorizationCheckerInterface $checker, IdResolverInterface $idResolver = null) + public function __construct(DataStoreInterface $dataStore, IdResolverInterface $idResolver = null, AuthorizationCheckerInterface $checker = null) { $this->dataStore = $dataStore; - $this->checker = $checker; $this->idResolver = $idResolver ?? new SimpleIdResolver(); + $this->checker = $checker; } public function __invoke(Request $request): ApiResponse { $modelClass = $request->attributes->get(self::MODEL_ATTRIBUTE); $groups = $request->attributes->get(self::SERIALIZATION_GROUP, []); - $securityCheck = $request->attributes->get(self::SECURITY_CHECK, null); try { $id = $this->idResolver->resolveId($request, $modelClass); } catch (IdMissingException $e) { @@ -52,9 +50,7 @@ public function __invoke(Request $request): ApiResponse return new NotFound(); } - if ($securityCheck && !$this->checker->isGranted($securityCheck, $data)) { - throw new AccessDeniedException(\sprintf('Access denied to data of type "%s".', get_class($data))); - } + $this->assertResourceRights($request, $data); return new OkContent($data, $groups); } diff --git a/src/Crud/Controller/GetAll.php b/src/Crud/Controller/GetAll.php index d89a132..c39db2d 100644 --- a/src/Crud/Controller/GetAll.php +++ b/src/Crud/Controller/GetAll.php @@ -10,7 +10,6 @@ use SwagIndustries\Melodiia\Response\OkContent; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; -use Symfony\Component\Security\Core\Exception\AccessDeniedException; class GetAll implements CrudControllerInterface { @@ -30,9 +29,9 @@ class GetAll implements CrudControllerInterface public function __construct( DataStoreInterface $dataStore, - AuthorizationCheckerInterface $checker, FilterCollectionFactoryInterface $collectionFactory, - PaginationRequestFactoryInterface $pagesRequestFactory + PaginationRequestFactoryInterface $pagesRequestFactory, + AuthorizationCheckerInterface $checker = null ) { $this->dataStore = $dataStore; $this->checker = $checker; @@ -44,14 +43,10 @@ public function __invoke(Request $request) { // Metadata you can specify in routing definition $modelClass = $request->attributes->get(self::MODEL_ATTRIBUTE); - $securityCheck = $request->attributes->get(self::SECURITY_CHECK, null); $groups = $request->attributes->get(self::SERIALIZATION_GROUP, []); $this->assertModelClassInvalid($modelClass); - - if ($securityCheck && !$this->checker->isGranted($securityCheck)) { - throw new AccessDeniedException(\sprintf('Access denied to data of type "%s".', $modelClass)); - } + $this->assertResourceRights($request); $filters = $this->filterCollectionFactory->createCollection($modelClass); $form = $filters->getForm(); diff --git a/src/Crud/Controller/Update.php b/src/Crud/Controller/Update.php index e357cc7..7ec0562 100644 --- a/src/Crud/Controller/Update.php +++ b/src/Crud/Controller/Update.php @@ -16,7 +16,6 @@ use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; -use Symfony\Component\Security\Core\Exception\AccessDeniedException; /** * Crud controller that update data model with the data from the request using a form. @@ -40,7 +39,7 @@ final class Update extends BaseCrudController /** @var IdResolverInterface */ private $idResolver; - public function __construct(DataStoreInterface $dataStore, FormFactoryInterface $formFactory, EventDispatcherInterface $dispatcher, AuthorizationCheckerInterface $checker, IdResolverInterface $idResolver = null) + public function __construct(DataStoreInterface $dataStore, FormFactoryInterface $formFactory, EventDispatcherInterface $dispatcher, IdResolverInterface $idResolver = null, AuthorizationCheckerInterface $checker = null) { parent::__construct($dispatcher); $this->dataStore = $dataStore; @@ -53,7 +52,6 @@ public function __invoke(Request $request): ApiResponse { $modelClass = $request->attributes->get(self::MODEL_ATTRIBUTE); $form = $request->attributes->get(self::FORM_ATTRIBUTE); - $securityCheck = $request->attributes->get(self::SECURITY_CHECK, null); $clearMissing = $request->attributes->has(self::FORM_CLEAR_MISSING) ? $request->attributes->getBoolean(self::FORM_CLEAR_MISSING) : null @@ -69,9 +67,7 @@ public function __invoke(Request $request): ApiResponse $data = $this->dataStore->find($modelClass, $id); - if ($securityCheck && !$this->checker->isGranted($securityCheck, $data)) { - throw new AccessDeniedException(\sprintf('Access denied to data of type "%s".', $modelClass)); - } + $this->assertResourceRights($request, $data); if (empty($form) || !class_exists($form)) { throw new MelodiiaLogicException('If you use melodiia CRUD classes, you need to specify a model.'); diff --git a/tests/Melodiia/Crud/Controller/DeleteTest.php b/tests/Melodiia/Crud/Controller/DeleteTest.php index 05c69ab..12c3163 100644 --- a/tests/Melodiia/Crud/Controller/DeleteTest.php +++ b/tests/Melodiia/Crud/Controller/DeleteTest.php @@ -59,9 +59,9 @@ protected function setUp() $this->controller = new Delete( $this->dataStore->reveal(), - $this->checker->reveal(), $this->dispatcher->reveal(), - $idResolver->reveal() + $idResolver->reveal(), + $this->checker->reveal() ); } diff --git a/tests/Melodiia/Crud/Controller/GetAllTest.php b/tests/Melodiia/Crud/Controller/GetAllTest.php index c6d7de5..9cbc33b 100644 --- a/tests/Melodiia/Crud/Controller/GetAllTest.php +++ b/tests/Melodiia/Crud/Controller/GetAllTest.php @@ -82,9 +82,9 @@ public function setUp() $this->controller = new GetAll( $this->dataStore->reveal(), - $this->authorizationChecker->reveal(), $this->filtersFactory->reveal(), - $paginationFactory->reveal() + $paginationFactory->reveal(), + $this->authorizationChecker->reveal() ); } diff --git a/tests/Melodiia/Crud/Controller/GetTest.php b/tests/Melodiia/Crud/Controller/GetTest.php index afb80b6..f647315 100644 --- a/tests/Melodiia/Crud/Controller/GetTest.php +++ b/tests/Melodiia/Crud/Controller/GetTest.php @@ -35,7 +35,7 @@ public function setUp() $idResolver = $this->prophesize(IdResolverInterface::class); $idResolver->resolveId(Argument::type(Request::class), Argument::type('string'))->willReturn('id'); - $this->controller = new Get($this->dataStore->reveal(), $this->authorizationChecker->reveal(), $idResolver->reveal()); + $this->controller = new Get($this->dataStore->reveal(), $idResolver->reveal(), $this->authorizationChecker->reveal()); } public function testItIsIntanceOfMelodiiaController() diff --git a/tests/Melodiia/Crud/Controller/UpdateTest.php b/tests/Melodiia/Crud/Controller/UpdateTest.php index a165035..57f024c 100644 --- a/tests/Melodiia/Crud/Controller/UpdateTest.php +++ b/tests/Melodiia/Crud/Controller/UpdateTest.php @@ -82,8 +82,8 @@ public function setUp() $this->dataStore->reveal(), $this->formFactory->reveal(), $this->dispatcher->reveal(), - $this->checker->reveal(), - $idResolver->reveal() + $idResolver->reveal(), + $this->checker->reveal() ); }