From 211a80e9beba3ee1f97a6c0b719edb696f230277 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julien=20Fastr=C3=A9?= Date: Thu, 8 Sep 2022 13:46:24 +0200 Subject: [PATCH 1/6] deprecate chill prophecy train in favor of prophecy-phpunit bridge --- src/Bundle/ChillMainBundle/Test/ProphecyTrait.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Bundle/ChillMainBundle/Test/ProphecyTrait.php b/src/Bundle/ChillMainBundle/Test/ProphecyTrait.php index a25624719..ae274ca1d 100644 --- a/src/Bundle/ChillMainBundle/Test/ProphecyTrait.php +++ b/src/Bundle/ChillMainBundle/Test/ProphecyTrait.php @@ -18,6 +18,7 @@ namespace Chill\MainBundle\Test; * and use tearDownTrait after usage. * * @codeCoverageIgnore + * @deprecated use @class{Prophecy\PhpUnit\ProphecyTrait} instead */ trait ProphecyTrait { From e379d8adb5c94eb617cff0cd3fa6a2c19ef558bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julien=20Fastr=C3=A9?= Date: Thu, 8 Sep 2022 13:47:35 +0200 Subject: [PATCH 2/6] [feature] use internal services to check for acl on exports --- .../Controller/ExportController.php | 5 +- .../ChillMainBundle/Export/ExportManager.php | 56 ++++++------------- .../Repository/CenterRepository.php | 12 ++++ .../Authorization/ChillExportVoter.php | 17 +++--- .../ChillMainBundle/config/services.yaml | 8 +-- 5 files changed, 42 insertions(+), 56 deletions(-) diff --git a/src/Bundle/ChillMainBundle/Controller/ExportController.php b/src/Bundle/ChillMainBundle/Controller/ExportController.php index 1893a64b3..ffd73b777 100644 --- a/src/Bundle/ChillMainBundle/Controller/ExportController.php +++ b/src/Bundle/ChillMainBundle/Controller/ExportController.php @@ -23,6 +23,7 @@ use Symfony\Component\Form\Extension\Core\Type\FormType; use Symfony\Component\Form\Extension\Core\Type\SubmitType; use Symfony\Component\Form\FormFactoryInterface; use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpFoundation\Session\SessionInterface; use Symfony\Contracts\Translation\TranslatorInterface; @@ -142,10 +143,8 @@ class ExportController extends AbstractController /** * Render the list of available exports. - * - * @return \Symfony\Component\HttpFoundation\Response */ - public function indexAction(Request $request) + public function indexAction(): Response { $exportManager = $this->exportManager; diff --git a/src/Bundle/ChillMainBundle/Export/ExportManager.php b/src/Bundle/ChillMainBundle/Export/ExportManager.php index e2d099ba8..c1384a3b8 100644 --- a/src/Bundle/ChillMainBundle/Export/ExportManager.php +++ b/src/Bundle/ChillMainBundle/Export/ExportManager.php @@ -14,6 +14,7 @@ namespace Chill\MainBundle\Export; use Chill\MainBundle\Form\Type\Export\ExportType; use Chill\MainBundle\Form\Type\Export\PickCenterType; use Chill\MainBundle\Security\Authorization\AuthorizationHelper; +use Chill\MainBundle\Security\Authorization\AuthorizationHelperInterface; use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\QueryBuilder; use Generator; @@ -42,52 +43,38 @@ class ExportManager /** * The collected aggregators, injected by DI. * - * @var AggregatorInterface[] + * @var array|AggregatorInterface[] */ - private $aggregators = []; + private array $aggregators = []; - /** - * @var AuthorizationChecker - */ - private $authorizationChecker; + private AuthorizationCheckerInterface $authorizationChecker; - /** - * @var AuthorizationHelper - */ - private $authorizationHelper; + private AuthorizationHelperInterface $authorizationHelper; - /** - * @var EntityManagerInterface - */ - private $em; + private EntityManagerInterface $em; /** * Collected Exports, injected by DI. * - * @var ExportInterface[] + * @var array|ExportInterface[] */ - private $exports = []; + private array $exports = []; /** * The collected filters, injected by DI. * - * @var FilterInterface[] + * @var array|FilterInterface[] */ - private $filters = []; + private array $filters = []; /** * Collected Formatters, injected by DI. * - * @var FormatterInterface[] + * @var array|FormatterInterface[] */ - private $formatters = []; + private array $formatters = []; - /** - * a logger. - * - * @var LoggerInterface - */ - private $logger; + private LoggerInterface $logger; /** * @var \Symfony\Component\Security\Core\User\UserInterface @@ -98,7 +85,7 @@ class ExportManager LoggerInterface $logger, EntityManagerInterface $em, AuthorizationCheckerInterface $authorizationChecker, - AuthorizationHelper $authorizationHelper, + AuthorizationHelperInterface $authorizationHelper, TokenStorageInterface $tokenStorage ) { $this->logger = $logger; @@ -547,19 +534,16 @@ class ExportManager . 'an ExportInterface.'); } - if (null === $centers) { - $centers = $this->authorizationHelper->getReachableCenters( + if (null === $centers || [] === $centers) { + // we want to try if at least one center is reachable + return [] !== $this->authorizationHelper->getReachableCenters( $this->user, $role ); } - if (count($centers) === 0) { - return false; - } - foreach ($centers as $center) { - if ($this->authorizationChecker->isGranted($role, $center) === false) { + if (false === $this->authorizationChecker->isGranted($role, $center)) { //debugging $this->logger->debug('user has no access to element', [ 'method' => __METHOD__, @@ -568,10 +552,6 @@ class ExportManager 'role' => $role, ]); - ///// Bypasse les autorisations qui empêche d'afficher les nouveaux exports - return true; - ///// TODO supprimer le return true - return false; } } diff --git a/src/Bundle/ChillMainBundle/Repository/CenterRepository.php b/src/Bundle/ChillMainBundle/Repository/CenterRepository.php index 554f39880..e8f6f4fa3 100644 --- a/src/Bundle/ChillMainBundle/Repository/CenterRepository.php +++ b/src/Bundle/ChillMainBundle/Repository/CenterRepository.php @@ -30,6 +30,18 @@ final class CenterRepository implements ObjectRepository return $this->repository->find($id, $lockMode, $lockVersion); } + /** + * Return all active centers + * + * Note: this is a teaser: active will comes later on center entity + * + * @return Center[] + */ + public function findActive(): array + { + return $this->findAll(); + } + /** * @return Center[] */ diff --git a/src/Bundle/ChillMainBundle/Security/Authorization/ChillExportVoter.php b/src/Bundle/ChillMainBundle/Security/Authorization/ChillExportVoter.php index ec1a0479d..b98564adf 100644 --- a/src/Bundle/ChillMainBundle/Security/Authorization/ChillExportVoter.php +++ b/src/Bundle/ChillMainBundle/Security/Authorization/ChillExportVoter.php @@ -19,24 +19,23 @@ class ChillExportVoter extends Voter { public const EXPORT = 'chill_export'; - protected AuthorizationHelperInterface $authorizationHelper; + private VoterHelperInterface $helper; - public function __construct(AuthorizationHelperInterface $authorizationHelper) + public function __construct(VoterHelperFactoryInterface $voterHelperFactory) { - $this->authorizationHelper = $authorizationHelper; + $this->helper = $voterHelperFactory + ->generate(self::class) + ->addCheckFor(null, [self::EXPORT]) + ->build(); } protected function supports($attribute, $subject): bool { - return self::EXPORT === $attribute; + return $this->helper->supports($attribute, $subject); } protected function voteOnAttribute($attribute, $subject, TokenInterface $token): bool { - if (!$token->getUser() instanceof User) { - return false; - } - - return [] !== $this->authorizationHelper->getReachableCenters($token->getUser(), $attribute); + return $this->helper->voteOnAttribute($attribute, $subject, $token); } } diff --git a/src/Bundle/ChillMainBundle/config/services.yaml b/src/Bundle/ChillMainBundle/config/services.yaml index 6d55532a6..697fd62aa 100644 --- a/src/Bundle/ChillMainBundle/config/services.yaml +++ b/src/Bundle/ChillMainBundle/config/services.yaml @@ -88,12 +88,8 @@ services: - { name: validator.constraint_validator, alias: 'role_scope_scope_presence' } Chill\MainBundle\Export\ExportManager: - arguments: - - "@logger" - - "@doctrine.orm.entity_manager" - - "@security.authorization_checker" - - "@chill.main.security.authorization.helper" - - "@security.token_storage" + autoconfigure: true + autowire: true Chill\MainBundle\Security\Resolver\CenterResolverDispatcherInterface: '@Chill\MainBundle\Security\Resolver\CenterResolverDispatcher' From 38cb1fe35711c75a9a749eb58e7b37cee010ef93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julien=20Fastr=C3=A9?= Date: Fri, 9 Sep 2022 10:54:21 +0200 Subject: [PATCH 3/6] [dev-feature] use an interface for describing CenterRepository (allow mocking in tests --- .../Repository/CenterRepository.php | 9 +-------- .../Repository/CenterRepositoryInterface.php | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 8 deletions(-) create mode 100644 src/Bundle/ChillMainBundle/Repository/CenterRepositoryInterface.php diff --git a/src/Bundle/ChillMainBundle/Repository/CenterRepository.php b/src/Bundle/ChillMainBundle/Repository/CenterRepository.php index e8f6f4fa3..d8e54d1c4 100644 --- a/src/Bundle/ChillMainBundle/Repository/CenterRepository.php +++ b/src/Bundle/ChillMainBundle/Repository/CenterRepository.php @@ -16,7 +16,7 @@ use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\EntityRepository; use Doctrine\Persistence\ObjectRepository; -final class CenterRepository implements ObjectRepository +final class CenterRepository implements CenterRepositoryInterface { private EntityRepository $repository; @@ -30,13 +30,6 @@ final class CenterRepository implements ObjectRepository return $this->repository->find($id, $lockMode, $lockVersion); } - /** - * Return all active centers - * - * Note: this is a teaser: active will comes later on center entity - * - * @return Center[] - */ public function findActive(): array { return $this->findAll(); diff --git a/src/Bundle/ChillMainBundle/Repository/CenterRepositoryInterface.php b/src/Bundle/ChillMainBundle/Repository/CenterRepositoryInterface.php new file mode 100644 index 000000000..27ba64caf --- /dev/null +++ b/src/Bundle/ChillMainBundle/Repository/CenterRepositoryInterface.php @@ -0,0 +1,18 @@ + Date: Fri, 9 Sep 2022 18:36:13 +0200 Subject: [PATCH 4/6] [FIX] use AuthorizationHelperInterface instead of implementation in PickCenterType --- .../Form/Type/Export/PickCenterType.php | 36 ++++++------------- .../ChillMainBundle/config/services/form.yaml | 8 ++--- 2 files changed, 13 insertions(+), 31 deletions(-) diff --git a/src/Bundle/ChillMainBundle/Form/Type/Export/PickCenterType.php b/src/Bundle/ChillMainBundle/Form/Type/Export/PickCenterType.php index c73402dd3..07776cb71 100644 --- a/src/Bundle/ChillMainBundle/Form/Type/Export/PickCenterType.php +++ b/src/Bundle/ChillMainBundle/Form/Type/Export/PickCenterType.php @@ -15,6 +15,7 @@ use Chill\MainBundle\Center\GroupingCenterInterface; use Chill\MainBundle\Entity\Center; use Chill\MainBundle\Export\ExportManager; use Chill\MainBundle\Security\Authorization\AuthorizationHelper; +use Chill\MainBundle\Security\Authorization\AuthorizationHelperInterface; use Doctrine\ORM\EntityRepository; use Symfony\Bridge\Doctrine\Form\Type\EntityType; use Symfony\Component\Form\AbstractType; @@ -24,6 +25,7 @@ use Symfony\Component\Form\FormBuilderInterface; use Symfony\Component\OptionsResolver\OptionsResolver; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; +use Symfony\Component\Security\Core\User\UserInterface; use function array_intersect; use function array_key_exists; use function array_merge; @@ -38,30 +40,24 @@ class PickCenterType extends AbstractType { public const CENTERS_IDENTIFIERS = 'c'; - /** - * @var AuthorizationHelper - */ - protected $authorizationHelper; + protected AuthorizationHelperInterface $authorizationHelper; + + protected ExportManager $exportManager; /** - * @var ExportManager + * @var array|GroupingCenterInterface[] */ - protected $exportManager; - - /** - * @var GroupingCenterInterface[] - */ - protected $groupingCenters = []; + protected array $groupingCenters = []; /** * @var \Symfony\Component\Security\Core\User\UserInterface */ - protected $user; + protected UserInterface $user; public function __construct( TokenStorageInterface $tokenStorage, ExportManager $exportManager, - AuthorizationHelper $authorizationHelper + AuthorizationHelperInterface $authorizationHelper ) { $this->exportManager = $exportManager; $this->user = $tokenStorage->getToken()->getUser(); @@ -78,22 +74,12 @@ class PickCenterType extends AbstractType $export = $this->exportManager->getExport($options['export_alias']); $centers = $this->authorizationHelper->getReachableCenters( $this->user, - (string) $export->requiredRole() + $export->requiredRole() ); $builder->add(self::CENTERS_IDENTIFIERS, EntityType::class, [ 'class' => Center::class, - 'query_builder' => static function (EntityRepository $er) use ($centers) { - $qb = $er->createQueryBuilder('c'); - $ids = array_map( - static function (Center $el) { - return $el->getId(); - }, - $centers - ); - - return $qb->where($qb->expr()->in('c.id', $ids)); - }, + 'choices' => $centers, 'multiple' => true, 'expanded' => true, 'choice_label' => static function (Center $c) { diff --git a/src/Bundle/ChillMainBundle/config/services/form.yaml b/src/Bundle/ChillMainBundle/config/services/form.yaml index 407a1b6af..0a757a8db 100644 --- a/src/Bundle/ChillMainBundle/config/services/form.yaml +++ b/src/Bundle/ChillMainBundle/config/services/form.yaml @@ -81,12 +81,8 @@ services: chill.main.form.pick_centers_type: class: Chill\MainBundle\Form\Type\Export\PickCenterType - arguments: - - "@security.token_storage" - - '@Chill\MainBundle\Export\ExportManager' - - "@chill.main.security.authorization.helper" - tags: - - { name: form.type } + autowire: true + autoconfigure: true chill.main.form.formatter_type: class: Chill\MainBundle\Form\Type\Export\FormatterType From 78ea990189d8229150194c36dfbce55ee2708a1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julien=20Fastr=C3=A9?= Date: Fri, 9 Sep 2022 18:36:46 +0200 Subject: [PATCH 5/6] allow voter to handle export about Accompanying periods on Center --- .../Security/Authorization/AccompanyingPeriodVoter.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Bundle/ChillPersonBundle/Security/Authorization/AccompanyingPeriodVoter.php b/src/Bundle/ChillPersonBundle/Security/Authorization/AccompanyingPeriodVoter.php index 8da2036d7..b9c9fb219 100644 --- a/src/Bundle/ChillPersonBundle/Security/Authorization/AccompanyingPeriodVoter.php +++ b/src/Bundle/ChillPersonBundle/Security/Authorization/AccompanyingPeriodVoter.php @@ -11,6 +11,7 @@ declare(strict_types=1); namespace Chill\PersonBundle\Security\Authorization; +use Chill\MainBundle\Entity\Center; use Chill\MainBundle\Entity\User; use Chill\MainBundle\Security\Authorization\AbstractChillVoter; use Chill\MainBundle\Security\Authorization\VoterHelperFactoryInterface; @@ -119,6 +120,7 @@ class AccompanyingPeriodVoter extends AbstractChillVoter implements ProvideRoleH ->addCheckFor(null, [self::CREATE, self::REASSIGN_BULK]) ->addCheckFor(AccompanyingPeriod::class, [self::TOGGLE_CONFIDENTIAL, ...self::ALL]) ->addCheckFor(Person::class, [self::SEE, self::CREATE]) + ->addCheckFor(Center::class, [self::STATS]) ->build(); } From d716e0c2c276463c7042a11cbc2bbf2c516d1615 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julien=20Fastr=C3=A9?= Date: Sun, 11 Sep 2022 22:44:09 +0200 Subject: [PATCH 6/6] add missing roles and adapt role voter for exports houshold and activity --- .../Authorization/ActivityStatsVoter.php | 40 ++++++------------- .../Security/Authorization/HouseholdVoter.php | 38 ++++++++++++++++-- .../translations/messages.fr.yml | 1 + 3 files changed, 47 insertions(+), 32 deletions(-) diff --git a/src/Bundle/ChillActivityBundle/Security/Authorization/ActivityStatsVoter.php b/src/Bundle/ChillActivityBundle/Security/Authorization/ActivityStatsVoter.php index 789e634e5..48448d5e3 100644 --- a/src/Bundle/ChillActivityBundle/Security/Authorization/ActivityStatsVoter.php +++ b/src/Bundle/ChillActivityBundle/Security/Authorization/ActivityStatsVoter.php @@ -13,10 +13,10 @@ namespace Chill\ActivityBundle\Security\Authorization; use Chill\MainBundle\Entity\Center; use Chill\MainBundle\Security\Authorization\AbstractChillVoter; -use Chill\MainBundle\Security\Authorization\AuthorizationHelper; +use Chill\MainBundle\Security\Authorization\VoterHelperFactoryInterface; +use Chill\MainBundle\Security\Authorization\VoterHelperInterface; use Chill\MainBundle\Security\ProvideRoleHierarchyInterface; - -use function in_array; +use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; class ActivityStatsVoter extends AbstractChillVoter implements ProvideRoleHierarchyInterface { @@ -24,14 +24,14 @@ class ActivityStatsVoter extends AbstractChillVoter implements ProvideRoleHierar public const STATS = 'CHILL_ACTIVITY_STATS'; - /** - * @var AuthorizationHelper - */ - protected $helper; + protected VoterHelperInterface $helper; - public function __construct(AuthorizationHelper $helper) + public function __construct(VoterHelperFactoryInterface $voterHelperFactory) { - $this->helper = $helper; + $this->helper = $voterHelperFactory + ->generate(self::class) + ->addCheckFor(Center::class, [self::STATS, self::LISTS]) + ->build(); } public function getRoles(): array @@ -49,30 +49,14 @@ class ActivityStatsVoter extends AbstractChillVoter implements ProvideRoleHierar return $this->getAttributes(); } - protected function getSupportedClasses() + protected function voteOnAttribute($attribute, $subject, TokenInterface $token) { - return [Center::class]; - } - - protected function isGranted($attribute, $object, $user = null) - { - if (!$user instanceof \Symfony\Component\Security\Core\User\UserInterface) { - return false; - } - - return $this->helper->userHasAccess($user, $object, $attribute); + return $this->helper->voteOnAttribute($attribute, $subject, $token); } protected function supports($attribute, $subject) { - if ( - $subject instanceof Center - && in_array($attribute, $this->getAttributes(), true) - ) { - return true; - } - - return false; + return $this->helper->supports($attribute, $subject); } private function getAttributes() diff --git a/src/Bundle/ChillPersonBundle/Security/Authorization/HouseholdVoter.php b/src/Bundle/ChillPersonBundle/Security/Authorization/HouseholdVoter.php index ca956db63..0288c9e61 100644 --- a/src/Bundle/ChillPersonBundle/Security/Authorization/HouseholdVoter.php +++ b/src/Bundle/ChillPersonBundle/Security/Authorization/HouseholdVoter.php @@ -11,6 +11,10 @@ declare(strict_types=1); namespace Chill\PersonBundle\Security\Authorization; +use Chill\MainBundle\Entity\Center; +use Chill\MainBundle\Security\Authorization\VoterHelperFactoryInterface; +use Chill\MainBundle\Security\Authorization\VoterHelperInterface; +use Chill\MainBundle\Security\ProvideRoleHierarchyInterface; use Chill\PersonBundle\Entity\Household\Household; use Chill\PersonBundle\Entity\Household\HouseholdMember; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; @@ -19,7 +23,7 @@ use Symfony\Component\Security\Core\Security; use UnexpectedValueException; use function in_array; -class HouseholdVoter extends Voter +class HouseholdVoter extends Voter implements ProvideRoleHierarchyInterface { public const EDIT = 'CHILL_PERSON_HOUSEHOLD_EDIT'; @@ -36,17 +40,40 @@ class HouseholdVoter extends Voter self::EDIT, self::SEE, ]; + private VoterHelperInterface $helper; + private Security $security; - public function __construct(Security $security) + public function __construct(Security $security, VoterHelperFactoryInterface $voterHelperFactory) { $this->security = $security; + $this->helper = $voterHelperFactory + ->generate(self::class) + ->addCheckFor(Center::class, [self::STATS]) + ->build(); + } + + public function getRolesWithHierarchy(): array + { + return [ 'Person' => $this->getRoles() ]; + } + + public function getRoles(): array + { + return [self::STATS]; + } + + public function getRolesWithoutScope(): array + { + return $this->getRoles(); } protected function supports($attribute, $subject) { - return $subject instanceof Household - && in_array($attribute, self::ALL, true); + return ($subject instanceof Household + && in_array($attribute, self::ALL, true)) + || $this->helper->supports($attribute, $subject) + ; } protected function voteOnAttribute($attribute, $subject, TokenInterface $token): bool @@ -58,6 +85,9 @@ class HouseholdVoter extends Voter case self::EDIT: return $this->checkAssociatedMembersRole($subject, PersonVoter::UPDATE); + case self::STATS: + return $this->voteOnAttribute($attribute, $subject, $token); + default: throw new UnexpectedValueException('attribute not supported'); } diff --git a/src/Bundle/ChillPersonBundle/translations/messages.fr.yml b/src/Bundle/ChillPersonBundle/translations/messages.fr.yml index a81fb7e86..dc53f4839 100644 --- a/src/Bundle/ChillPersonBundle/translations/messages.fr.yml +++ b/src/Bundle/ChillPersonBundle/translations/messages.fr.yml @@ -318,6 +318,7 @@ CHILL_PERSON_ACCOMPANYING_PERIOD_FULL: Voir les détails, créer, supprimer et m CHILL_PERSON_ACCOMPANYING_COURSE_REASSIGN_BULK: Réassigner les parcours en lot CHILL_PERSON_ACCOMPANYING_PERIOD_SEE_DETAILS: Voir les détails d'une période d'accompagnement CHILL_PERSON_ACCOMPANYING_PERIOD_STATS: Statistiques sur les parcours d'accompagnement +CHILL_PERSON_HOUSEHOLD_STATS: Statistiques sur les ménages #period Period closed!: Période clôturée!