diff --git a/config/services.yaml b/config/services.yaml index 41e07cf..623d0d1 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -9,26 +9,28 @@ parameters: teamer_inactive_period: '-2 years' - destinations: - - Schwendi - - Waldschlössli - - Spinabad - - Schweizerhaus - - Weißfluh - - Victoria - - Klein Tirol - - Jenatsch - - Lederer - - Steinachhof - - Val Thorens - - Les Deux Alpes - - Ranalt - - Jolimont - - Christiler - - Heuberghaus - - Silvana - - Rotbach - - Mitterlengau + # Houses, hotel code: name. The name is used both as a label and to match + # destination.hotel, the code to match destination.hotelCode. + houses: + AGR: 'Rotbach' + ASC: 'Christiler' + DGS: 'Spinabad' + DKI: 'Schwendi' + DKS: 'Schweizerhaus' + DPW: 'Waldschlössli' + DWW: 'Weißfluh' + KHH: 'Heuberghaus' + L2A: 'Les Deux Alpes' + LPJ: 'Jenatsch' + MVK: 'Klein Tirol' + PCJ: 'Jolimont' + PMV: 'Victoria' + SBW: 'Val Thorens' + SHM: 'Mitterlengau' + SRR: 'Ranalt' + SSL: 'Lederer' + SST: 'Steinachhof' + SVS: 'Silvana' services: _defaults: @@ -37,7 +39,7 @@ services: bind: $tempDir: '%kernel.project_dir%/temp' $logger: '@monolog.logger.myep' - $destinations: '%destinations%' + $houses: '%houses%' $xmlExport: '@xml_export.storage' $xmlDump: '@xml_dump.storage' $teamerInactivePeriod: '%teamer_inactive_period%' diff --git a/src/BusProNet/UserDataHandler.php b/src/BusProNet/UserDataHandler.php index 70f923a..be70d13 100644 --- a/src/BusProNet/UserDataHandler.php +++ b/src/BusProNet/UserDataHandler.php @@ -19,18 +19,19 @@ class UserDataHandler ) { } - public function collectRoles(CrmAttributesResponse $crmAttributes, ?string $preferredRole): array + /** + * Collects the roles to grant on initial user creation. + * + * Administrative roles are deliberately not importable: they may only be granted + * manually by a super admin, so that nobody can escalate their own privileges via + * the BusPro CRM. They are imported as pending markers instead, which grant nothing + * but mark the user for approval. ROLE_TEAMER carries no privileges of its own and + * is granted directly. + */ + public function collectRoles(CrmAttributesResponse $crmAttributes): array { // Collect user's roles from CRM attributes - $roles = []; - - if ($crmAttributes->isAdmin() && (null === $preferredRole || 'admin' === $preferredRole)) { - $roles[] = 'ROLE_ADMIN'; - } elseif ($crmAttributes->isManager() && (null === $preferredRole || 'manager' === $preferredRole)) { - $roles[] = 'ROLE_MANAGER'; - } elseif ($crmAttributes->isHouseManager() && (null === $preferredRole || 'house_manager' === $preferredRole)) { - $roles[] = 'ROLE_HOUSE_MANAGER'; - } + $roles = $this->collectPendingRoles($crmAttributes); if ($crmAttributes->isTeamer()) { $roles[] = 'ROLE_TEAMER'; @@ -39,6 +40,26 @@ class UserDataHandler return $roles; } + /** + * Collects the pending markers for the administrative roles claimed in the CRM. + */ + public function collectPendingRoles(CrmAttributesResponse $crmAttributes): array + { + $roles = []; + + if ($crmAttributes->isAdmin()) { + $roles[] = User::PENDING_ROLES['ROLE_ADMIN']; + } + + if ($crmAttributes->isManager()) { + $roles[] = User::PENDING_ROLES['ROLE_MANAGER']; + } elseif ($crmAttributes->isHouseManager()) { + $roles[] = User::PENDING_ROLES['ROLE_HOUSE_MANAGER']; + } + + return $roles; + } + public function findLocalUser(ProfileResponse $profileResponse): ?User { // Check if user is already present in local database @@ -141,22 +162,31 @@ class UserDataHandler return $user; } + /** + * Updates an existing user from BusPro data. + * + * Roles and hotel codes are imported once on user creation only and are managed + * manually afterwards, so they are intentionally left untouched here. The only + * exception are the privilege-free pending markers, which keep tracking the + * administrative roles claimed in the CRM. + * + * @param string[] $claimedRoles pending markers as returned by collectPendingRoles() + */ public function updateLocalUser( User $user, ProfileResponse $profileResponse, - array $roles, bool $isTeamer = false, array $crmSelections = [], - array $hotelCodes = [], + array $claimedRoles = [], ): void { $user ->setFirstName($profileResponse->getFirstName()) ->setLastName($profileResponse->getName()) ->setEmail($profileResponse->getCommunication()->getEmail()) - ->setRoles($roles) - ->setHotelCodes($hotelCodes) ; + $this->refreshPendingRoles($user, $claimedRoles); + if (true === $isTeamer) { $address = Address::fromApiResponse($profileResponse); $communication = Communication::fromApiResponse($profileResponse); @@ -187,4 +217,39 @@ class UserDataHandler $this->entityManager->flush(); } + + /** + * Keeps the pending markers in sync with the administrative roles claimed in the CRM. + * The markers grant no privileges, so tracking them on every login is safe: only a + * super admin can turn one into an actual role, and an already granted role is never + * marked as pending again. + * + * @param string[] $claimedRoles + */ + private function refreshPendingRoles(User $user, array $claimedRoles): void + { + // an approved role needs no marker anymore + $grantedRoles = $user->getAssignedRoles(); + $pendingRoles = array_values(array_filter( + $claimedRoles, + static fn (string $pendingRole): bool => false === in_array( + array_search($pendingRole, User::PENDING_ROLES, true), + $grantedRoles, + true, + ), + )); + + if ($pendingRoles === $user->getPendingRoles()) { + return; + } + + // getAssignedRoles() returns the manually assignable roles without the markers + $user->setRoles([...$grantedRoles, ...$pendingRoles]); + + $this->logger->info('Refresh pending roles', [ + 'user_id' => $user->getId(), + 'user_email' => $user->getEmail(), + 'pending_roles' => $pendingRoles, + ]); + } } diff --git a/src/Config/HouseCatalog.php b/src/Config/HouseCatalog.php new file mode 100644 index 0000000..d30d104 --- /dev/null +++ b/src/Config/HouseCatalog.php @@ -0,0 +1,55 @@ + $houses hotel code => name + */ + public function __construct(private readonly array $houses) + { + } + + /** + * Choices for filtering destinations by house name: label => name. + * + * @return array + */ + public function getNameChoices(): array + { + $names = array_values($this->houses); + sort($names); + + return array_combine($names, $names); + } + + /** + * Choices for assigning hotel codes to a user: label => code. + * + * @return array + */ + public function getCodeChoices(): array + { + $codes = $this->houses; + asort($codes); + + return array_flip($codes); + } + + public function getName(string $code): ?string + { + return $this->houses[$code] ?? null; + } +} diff --git a/src/Controller/Admin/System/User/EditController.php b/src/Controller/Admin/System/User/EditController.php new file mode 100644 index 0000000..f7accdd --- /dev/null +++ b/src/Controller/Admin/System/User/EditController.php @@ -0,0 +1,51 @@ +createForm(UserType::class, $user); + $form->handleRequest($request); + + if ($form->isSubmitted() && $form->isValid()) { + $this->entityManager->flush(); + + $this->addFlash('success', 'Die Benutzer:in wurde aktualisiert'); + $this->logger->info('Edit user', [ + 'user_id' => $user->getId(), + 'user_email' => $user->getEmail(), + 'roles' => $user->getRoles(), + 'super_admin' => $user->isSuperAdmin(), + 'hotel_codes' => $user->getHotelCodes(), + ]); + + return $this->redirectToRoute('app_admin_system_user_index'); + } + + return $this->render('admin/system/user/edit.html.twig', [ + 'form' => $form, + 'user' => $user, + ]); + } +} diff --git a/src/Controller/Security/LoginController.php b/src/Controller/Security/LoginController.php index fe5b8a9..5847cc6 100644 --- a/src/Controller/Security/LoginController.php +++ b/src/Controller/Security/LoginController.php @@ -4,7 +4,6 @@ namespace App\Controller\Security; use App\Entity\User; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; -use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Routing\Attribute\Route; use Symfony\Component\Security\Http\Authentication\AuthenticationUtils; @@ -12,7 +11,7 @@ use Symfony\Component\Security\Http\Authentication\AuthenticationUtils; class LoginController extends AbstractController { #[Route('/login', name: 'app_security_login')] - public function login(AuthenticationUtils $authenticationUtils, Request $request): Response + public function login(AuthenticationUtils $authenticationUtils): Response { // redirect to default route in case of active session if (null !== $user = $this->getUser()) { @@ -26,12 +25,9 @@ class LoginController extends AbstractController // last username entered by the user $lastUsername = $authenticationUtils->getLastUsername(); - $role = $request->query->get('role'); - return $this->render('security/login.html.twig', [ 'last_username' => $lastUsername, 'error' => $error, - 'role' => $role, ]); } diff --git a/src/Entity/User.php b/src/Entity/User.php index 385eff1..6333b4a 100644 --- a/src/Entity/User.php +++ b/src/Entity/User.php @@ -8,12 +8,36 @@ use Doctrine\DBAL\Types\Types; use Doctrine\ORM\Mapping as ORM; use Symfony\Component\Security\Core\User\UserInterface; use Symfony\Component\Uid\Uuid; +use Symfony\Component\Validator\Constraints as Assert; +use Symfony\Component\Validator\Context\ExecutionContextInterface; #[ORM\Entity(repositoryClass: UserRepository::class)] class User implements UserInterface, TimestampableEntityInterface { use TimestampableEntity; + /** + * Assignable roles and their labels. + */ + public const ROLES = [ + 'ROLE_ADMIN' => 'Admin', + 'ROLE_MANAGER' => 'Reisemanager', + 'ROLE_HOUSE_MANAGER' => 'Hausleitung', + 'ROLE_TEAMER' => 'Teamer', + ]; + + /** + * Markers for administrative roles a user holds in the BusPro CRM, keyed by the role + * they stand for. They grant no privileges whatsoever and merely make the user show + * up for approval, because administrative roles may only ever be granted manually by + * a super admin. + */ + public const PENDING_ROLES = [ + 'ROLE_ADMIN' => 'ROLE_ADMIN_PENDING', + 'ROLE_MANAGER' => 'ROLE_MANAGER_PENDING', + 'ROLE_HOUSE_MANAGER' => 'ROLE_HOUSE_MANAGER_PENDING', + ]; + #[ORM\Id] #[ORM\GeneratedValue] #[ORM\Column] @@ -176,19 +200,16 @@ class User implements UserInterface, TimestampableEntityInterface public function getRolesLabels(): array { - $labels = []; + $labels = self::ROLES; - foreach ($this->roles as $role) { - $labels[] = match ($role) { - 'ROLE_ADMIN' => 'Admin', - 'ROLE_MANAGER' => 'Reisemanager', - 'ROLE_HOUSE_MANAGER' => 'Hausleitung', - 'ROLE_TEAMER' => 'Teamer', - default => $role, - }; + foreach (self::PENDING_ROLES as $role => $pendingRole) { + $labels[$pendingRole] = self::ROLES[$role].' (nicht freigeschaltet)'; } - return $labels; + return array_map( + static fn (string $role): string => $labels[$role] ?? $role, + $this->roles, + ); } public function setRoles(array $roles): static @@ -198,6 +219,29 @@ class User implements UserInterface, TimestampableEntityInterface return $this; } + /** + * The manually assignable roles held by the user, i.e. without the implicit ROLE_USER + * and ROLE_SUPER_ADMIN added by getRoles() and without any pending marker. Used to + * edit role assignments: saving them resolves the pending approvals. + */ + public function getAssignedRoles(): array + { + return array_values(array_intersect($this->roles, array_keys(self::ROLES))); + } + + /** + * The pending markers currently held by the user. + */ + public function getPendingRoles(): array + { + return array_values(array_intersect($this->roles, array_values(self::PENDING_ROLES))); + } + + public function setAssignedRoles(array $roles): static + { + return $this->setRoles($roles); + } + public function hasRole(string $role): bool { return in_array($role, $this->getRoles()); @@ -215,6 +259,21 @@ class User implements UserInterface, TimestampableEntityInterface return $this; } + /** + * Super admin is an elevation of ROLE_ADMIN, never a standalone grant. + */ + #[Assert\Callback] + public function validateSuperAdmin(ExecutionContextInterface $context): void + { + if (true === $this->superAdmin && false === in_array('ROLE_ADMIN', $this->roles, true)) { + $context + ->buildViolation('Nur Admins können zu Superadmins ernannt werden.') + ->atPath('superAdmin') + ->addViolation() + ; + } + } + public function getLastLoginAt(): ?\DateTimeImmutable { return $this->lastLoginAt; @@ -235,9 +294,9 @@ class User implements UserInterface, TimestampableEntityInterface return 'app_manager_index'; } elseif ($this->hasRole('ROLE_HOUSE_MANAGER')) { return 'app_house_manager_index'; - } else { - return 'app_teamer_index'; } + + return 'app_teamer_index'; } public function eraseCredentials(): void diff --git a/src/Form/ApplicationFilterType.php b/src/Form/ApplicationFilterType.php index 048a2a5..871167b 100644 --- a/src/Form/ApplicationFilterType.php +++ b/src/Form/ApplicationFilterType.php @@ -2,6 +2,7 @@ namespace App\Form; +use App\Config\HouseCatalog; use App\Entity\Assignment; use App\Entity\JobProfile; use App\Model\ApplicationFilterDto; @@ -17,15 +18,8 @@ use Symfony\Component\OptionsResolver\OptionsResolver; class ApplicationFilterType extends AbstractType { - private array $hotelChoices = []; - - public function __construct(private readonly Security $security, private readonly array $destinations) + public function __construct(private readonly Security $security, private readonly HouseCatalog $houseCatalog) { - $destinations = $this->destinations; - sort($destinations); - foreach ($destinations as $item) { - $this->hotelChoices[$item] = $item; - } } public function buildForm(FormBuilderInterface $builder, array $options): void @@ -126,7 +120,7 @@ class ApplicationFilterType extends AbstractType 'label' => 'Haus/Destination', 'required' => false, 'empty_label' => 'nicht filtern', - 'choices' => $this->hotelChoices, + 'choices' => $this->houseCatalog->getNameChoices(), ]) ; } diff --git a/src/Form/AssignmentFilterType.php b/src/Form/AssignmentFilterType.php index 4b730f1..e96bffe 100644 --- a/src/Form/AssignmentFilterType.php +++ b/src/Form/AssignmentFilterType.php @@ -2,6 +2,7 @@ namespace App\Form; +use App\Config\HouseCatalog; use App\Entity\Assignment; use App\Entity\JobProfile; use App\Model\AssignmentFilterDto; @@ -17,15 +18,8 @@ use Symfony\Component\OptionsResolver\OptionsResolver; class AssignmentFilterType extends AbstractType { - private array $hotelChoices = []; - - public function __construct(private readonly Security $security, private readonly array $destinations) + public function __construct(private readonly Security $security, private readonly HouseCatalog $houseCatalog) { - $destinations = $this->destinations; - sort($destinations); - foreach ($destinations as $item) { - $this->hotelChoices[$item] = $item; - } } public function buildForm(FormBuilderInterface $builder, array $options): void @@ -130,7 +124,7 @@ class AssignmentFilterType extends AbstractType 'label' => 'Haus/Destination', 'required' => false, 'empty_label' => 'nicht filtern', - 'choices' => $this->hotelChoices, + 'choices' => $this->houseCatalog->getNameChoices(), ]) ; } diff --git a/src/Form/DestinationFilterType.php b/src/Form/DestinationFilterType.php index 7d2b40e..e604e8f 100644 --- a/src/Form/DestinationFilterType.php +++ b/src/Form/DestinationFilterType.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace App\Form; +use App\Config\HouseCatalog; use App\Model\DestinationFilterDto; use Symfony\Component\Form\AbstractType; use Symfony\Component\Form\Extension\Core\Type\SubmitType; @@ -12,15 +13,8 @@ use Symfony\Component\OptionsResolver\OptionsResolver; class DestinationFilterType extends AbstractType { - private array $hotelChoices = []; - - public function __construct(private readonly array $destinations) + public function __construct(private readonly HouseCatalog $houseCatalog) { - $destinations = $this->destinations; - sort($destinations); - foreach ($destinations as $item) { - $this->hotelChoices[$item] = $item; - } } public function buildForm(FormBuilderInterface $builder, array $options): void @@ -30,7 +24,7 @@ class DestinationFilterType extends AbstractType 'label' => 'Haus/Destination', 'required' => false, 'empty_label' => 'nicht filtern', - 'choices' => $this->hotelChoices, + 'choices' => $this->houseCatalog->getNameChoices(), ]) ->add('apply', SubmitType::class, [ 'label' => 'filtern', diff --git a/src/Form/DocumentFilterType.php b/src/Form/DocumentFilterType.php index 655810d..f8cde49 100644 --- a/src/Form/DocumentFilterType.php +++ b/src/Form/DocumentFilterType.php @@ -2,6 +2,7 @@ namespace App\Form; +use App\Config\HouseCatalog; use App\Entity\JobProfile; use App\Entity\Teamer; use App\Entity\Upload; @@ -14,15 +15,8 @@ use Symfony\Component\OptionsResolver\OptionsResolver; class DocumentFilterType extends AbstractType { - private array $hotelChoices = []; - - public function __construct(private readonly array $destinations) + public function __construct(private readonly HouseCatalog $houseCatalog) { - $destinations = $this->destinations; - sort($destinations); - foreach ($destinations as $item) { - $this->hotelChoices[$item] = $item; - } } public function buildForm(FormBuilderInterface $builder, array $options): void @@ -56,7 +50,7 @@ class DocumentFilterType extends AbstractType 'label' => 'Haus/Destination', 'required' => false, 'empty_label' => 'nicht filtern', - 'choices' => $this->hotelChoices, + 'choices' => $this->houseCatalog->getNameChoices(), ]) ->add('dateFrom', DatepickerType::class, [ 'label' => 'Einsatzzeitraum von', diff --git a/src/Form/TimelineFilterType.php b/src/Form/TimelineFilterType.php index 8d932e0..064a221 100644 --- a/src/Form/TimelineFilterType.php +++ b/src/Form/TimelineFilterType.php @@ -2,6 +2,7 @@ namespace App\Form; +use App\Config\HouseCatalog; use App\Model\TimelineFilterDto; use App\Service\Assignment\TimelineService; use Symfony\Bundle\SecurityBundle\Security; @@ -15,7 +16,7 @@ class TimelineFilterType extends AbstractType public function __construct( private readonly TimelineService $timelineService, private readonly Security $security, - private readonly array $destinations, + private readonly HouseCatalog $houseCatalog, ) { } @@ -53,19 +54,12 @@ class TimelineFilterType extends AbstractType ; if (false === $this->security->isGranted('ROLE_HOUSE_MANAGER')) { - $destinations = $this->destinations; - $hotelChoices = []; - sort($destinations); - foreach ($destinations as $item) { - $hotelChoices[$item] = $item; - } - $builder ->add('hotels', MultiselectType::class, [ 'label' => 'Haus/Destination', 'required' => false, 'empty_label' => 'nicht filtern', - 'choices' => $hotelChoices, + 'choices' => $this->houseCatalog->getNameChoices(), ]) ; } diff --git a/src/Form/UserType.php b/src/Form/UserType.php new file mode 100644 index 0000000..6af4b63 --- /dev/null +++ b/src/Form/UserType.php @@ -0,0 +1,64 @@ +add('roles', MultiselectType::class, [ + 'label' => 'Rollen', + 'required' => false, + 'property_path' => 'assignedRoles', + 'choices' => array_flip(User::ROLES), + 'empty_label' => 'keine Rolle', + ]) + ->add('superAdmin', CheckboxType::class, [ + 'label' => 'Superadmin', + 'required' => false, + 'help' => 'Setzt die Rolle Admin voraus.', + ]) + ; + + // hotel codes already assigned to the user may predate the catalog, so they are + // added as choices to keep them selectable instead of failing + $builder->addEventListener(FormEvents::PRE_SET_DATA, function (FormEvent $event): void { + $user = $event->getData(); + + $choices = $this->houseCatalog->getCodeChoices(); + foreach ($user instanceof User ? $user->getHotelCodes() : [] as $code) { + if (false === in_array($code, $choices, true)) { + $choices[$code] = $code; + } + } + + $event->getForm()->add('hotelCodes', MultiselectType::class, [ + 'label' => 'Häuser', + 'required' => false, + 'choices' => $choices, + 'empty_label' => 'keine Häuser', + ]); + }); + } + + public function configureOptions(OptionsResolver $resolver): void + { + $resolver->setDefaults([ + 'data_class' => User::class, + ]); + } +} diff --git a/src/Menu/AdminMenuBuilder.php b/src/Menu/AdminMenuBuilder.php index 7ba35cf..25a136e 100644 --- a/src/Menu/AdminMenuBuilder.php +++ b/src/Menu/AdminMenuBuilder.php @@ -259,6 +259,11 @@ class AdminMenuBuilder extends AbstractMenuBuilder 'linkAttributes' => [ 'title' => 'Administrative Benutzer:innen', ], + 'extras' => [ + 'routes' => [ + ['pattern' => '/^app_admin_system_user_/'], + ], + ], ]); $menu->addChild('Logs', [ 'route' => 'app_admin_log_index', diff --git a/src/Repository/UserRepository.php b/src/Repository/UserRepository.php index 861fd30..ba09843 100644 --- a/src/Repository/UserRepository.php +++ b/src/Repository/UserRepository.php @@ -31,16 +31,18 @@ class UserRepository extends ServiceEntityRepository { $qb = $this->createQueryBuilder('u'); + // administrative roles, plus the pending markers standing in for them + $roles = [...array_keys(User::PENDING_ROLES), ...array_values(User::PENDING_ROLES)]; + + foreach ($roles as $index => $role) { + $qb + ->orWhere('JSON_CONTAINS(u.roles, :role'.$index.') = 1') + ->setParameter('role'.$index, json_encode($role)) + ; + } + return $qb - ->where('JSON_CONTAINS(u.roles, :role_admin) = 1') - ->orWhere('JSON_CONTAINS(u.roles, :role_manager) = 1') - ->orWhere('JSON_CONTAINS(u.roles, :role_house_manager) = 1') ->orderBy('u.lastName', 'ASC') - ->setParameters([ - 'role_admin' => json_encode('ROLE_ADMIN'), - 'role_manager' => json_encode('ROLE_MANAGER'), - 'role_house_manager' => json_encode('ROLE_HOUSE_MANAGER'), - ]) ->getQuery() ->getResult() ; diff --git a/src/Security/BpnAuthenticator.php b/src/Security/BpnAuthenticator.php index f01be9a..47c39e8 100644 --- a/src/Security/BpnAuthenticator.php +++ b/src/Security/BpnAuthenticator.php @@ -64,8 +64,7 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent } // Final checks and local user loading/creation - $preferredRole = $request->request->get('_role'); - $user = $this->getOrCreateLocalUser($response, $email, $password, $preferredRole); + $user = $this->getOrCreateLocalUser($response, $email, $password); if (null === $user) { return null; @@ -110,7 +109,6 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent ProfileResponse $profileResponse, string $email, string $password, - ?string $preferredRole, ): ?User { // Fetch CRM attributes, early return in case of an API error try { @@ -120,24 +118,6 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent return null; } - // Collect user's roles from CRM attributes - $roles = $this - ->userDataHandler - ->collectRoles($crmAttributes, $preferredRole) - ; - - // User is expected to have at least one role - if (0 === count($roles)) { - // Revoke roles on existing local user to invalidate any active session - $existingUser = $this->userDataHandler->findLocalUser($profileResponse); - if (null !== $existingUser) { - $existingUser->setRoles([]); - $this->entityManager->flush(); - } - - return null; - } - // Flatten selected CRM attributes $crmSelections = $crmAttributes->toArray(); @@ -150,19 +130,33 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent ->findLocalUser($profileResponse) ; - // Update existing user's roles and teamer data and return it + // Update existing user's teamer data and return it, leaving roles and hotel + // codes alone: they are imported once on creation and managed manually after if (null !== $user) { $this ->userDataHandler - ->updateLocalUser($user, $profileResponse, $roles, $isTeamer, $crmSelections, $crmAttributes->getHotelCodes()) + ->updateLocalUser( + $user, + $profileResponse, + $isTeamer, + $crmSelections, + $this->userDataHandler->collectPendingRoles($crmAttributes), + ) ; return $user; } + // Initial import of roles and hotel codes on user creation return $this ->userDataHandler - ->createLocalUser($profileResponse, $roles, $isTeamer, $crmSelections, $crmAttributes->getHotelCodes()) + ->createLocalUser( + $profileResponse, + $this->userDataHandler->collectRoles($crmAttributes), + $isTeamer, + $crmSelections, + $crmAttributes->getHotelCodes(), + ) ; } } diff --git a/src/Security/UserChecker.php b/src/Security/UserChecker.php index 6a7a29b..d2fd6df 100644 --- a/src/Security/UserChecker.php +++ b/src/Security/UserChecker.php @@ -9,13 +9,6 @@ use Symfony\Component\Security\Core\User\UserInterface; class UserChecker implements UserCheckerInterface { - private const APPLICATION_ROLES = [ - 'ROLE_ADMIN', - 'ROLE_MANAGER', - 'ROLE_HOUSE_MANAGER', - 'ROLE_TEAMER', - ]; - public function checkPreAuth(UserInterface $user): void { if (!$user instanceof User) { @@ -26,7 +19,11 @@ class UserChecker implements UserCheckerInterface throw new CustomUserMessageAccountStatusException('Dein Account wurde gesperrt: '.$user->getDisabledReason()); } - if ([] === array_intersect($user->getRoles(), self::APPLICATION_ROLES)) { + if ([] === array_intersect($user->getRoles(), array_keys(User::ROLES))) { + if ([] !== $user->getPendingRoles()) { + throw new CustomUserMessageAccountStatusException('Deine Rolle wurde noch nicht freigeschaltet.'); + } + throw new CustomUserMessageAccountStatusException('Keine gültige Rolle zugewiesen.'); } } diff --git a/src/Security/Voter/UserVoter.php b/src/Security/Voter/UserVoter.php new file mode 100644 index 0000000..09aa9f7 --- /dev/null +++ b/src/Security/Voter/UserVoter.php @@ -0,0 +1,44 @@ +getUser(); + $targetUser = $subject; + + if (!$currentUser instanceof User || !$targetUser instanceof User) { + return false; + } + + // editing your own roles or super admin flag is not allowed + if ($currentUser === $targetUser) { + return false; + } + + // if the current user is impersonating, do not grant access + if ($this->security->isGranted('IS_IMPERSONATOR')) { + return false; + } + + return $currentUser->isSuperAdmin(); + } +} diff --git a/templates/admin/system/user/_form.html.twig b/templates/admin/system/user/_form.html.twig new file mode 100644 index 0000000..cc42ff5 --- /dev/null +++ b/templates/admin/system/user/_form.html.twig @@ -0,0 +1,16 @@ +{{ form_start(form) }} +
+ {{ form_row(form.roles) }} + {{ form_row(form.superAdmin) }} + {{ form_row(form.hotelCodes) }} +
+
+ + + Abbrechen + +
+{{ form_rest(form) }} +{{ form_end(form) }} diff --git a/templates/admin/system/user/edit.html.twig b/templates/admin/system/user/edit.html.twig new file mode 100644 index 0000000..538df25 --- /dev/null +++ b/templates/admin/system/user/edit.html.twig @@ -0,0 +1,12 @@ +{% extends 'admin/layout.html.twig' %} + +{% block title %}Einstellungen - Benutzer:in bearbeiten{% endblock %} + +{% block content %} +
+

+ {{ user.fullName }} bearbeiten +

+ {% include 'admin/system/user/_form.html.twig' %} +
+{% endblock %} diff --git a/templates/admin/system/user/index.html.twig b/templates/admin/system/user/index.html.twig index 5cb78d4..ba98976 100644 --- a/templates/admin/system/user/index.html.twig +++ b/templates/admin/system/user/index.html.twig @@ -56,16 +56,23 @@ {{ user.lastLoginAt ? user.lastLoginAt|date('d.m.Y, H:i') : '-' }} - {% if is_granted('CAN_IMPERSONATE', user) %} - - {{ icon('mask', 'w-4 h-4') }} - - {% endif %} +
+ {% if is_granted('CAN_IMPERSONATE', user) %} + + {{ icon('mask', 'w-4 h-4') }} + + {% endif %} + {% if is_granted('CAN_EDIT_USER', user) %} + + {{ icon('edit') }} + + {% endif %} +
{% else %} - + Keine Daten... diff --git a/templates/security/login.html.twig b/templates/security/login.html.twig index 0c322dd..fec040e 100644 --- a/templates/security/login.html.twig +++ b/templates/security/login.html.twig @@ -44,9 +44,6 @@ Login - {% if role is not null %} - - {% endif %}
diff --git a/tests/BusProNet/UserDataHandlerTest.php b/tests/BusProNet/UserDataHandlerTest.php index 7009c58..9eaed23 100644 --- a/tests/BusProNet/UserDataHandlerTest.php +++ b/tests/BusProNet/UserDataHandlerTest.php @@ -6,6 +6,7 @@ namespace App\Tests\BusProNet; use App\BusProNet\Model\Address as BusProAddress; use App\BusProNet\Model\Communication as BusProCommunication; +use App\BusProNet\Model\CrmAttributesResponse; use App\BusProNet\Model\ProfileResponse; use App\BusProNet\UserDataHandler; use App\Entity\Embeddable\Address; @@ -29,6 +30,59 @@ class UserDataHandlerTest extends TestCase $this->logger = $this->createMock(LoggerInterface::class); } + /** + * @dataProvider collectRolesProvider + */ + public function testCollectRolesImportsAdministrativeRolesAsPendingOnly(CrmAttributesResponse $crmAttributes, array $expectedRoles): void + { + $handler = new UserDataHandler($this->entityManager, $this->logger); + + $this->assertSame($expectedRoles, $handler->collectRoles($crmAttributes)); + } + + public static function collectRolesProvider(): iterable + { + yield 'admin only yields the pending marker' => [ + (new CrmAttributesResponse())->setAdmin(true), + [User::PENDING_ROLES['ROLE_ADMIN']], + ]; + + yield 'manager only yields the pending marker' => [ + (new CrmAttributesResponse())->setManager(true), + [User::PENDING_ROLES['ROLE_MANAGER']], + ]; + + yield 'house manager only yields the pending marker' => [ + (new CrmAttributesResponse())->setHouseManager(true), + [User::PENDING_ROLES['ROLE_HOUSE_MANAGER']], + ]; + + yield 'teamer is granted directly' => [ + (new CrmAttributesResponse())->setTeamer(true), + ['ROLE_TEAMER'], + ]; + + yield 'admin and teamer' => [ + (new CrmAttributesResponse())->setAdmin(true)->setTeamer(true), + [User::PENDING_ROLES['ROLE_ADMIN'], 'ROLE_TEAMER'], + ]; + + yield 'admin and manager yield both markers' => [ + (new CrmAttributesResponse())->setAdmin(true)->setManager(true), + [User::PENDING_ROLES['ROLE_ADMIN'], User::PENDING_ROLES['ROLE_MANAGER']], + ]; + + yield 'manager takes precedence over house manager' => [ + (new CrmAttributesResponse())->setManager(true)->setHouseManager(true), + [User::PENDING_ROLES['ROLE_MANAGER']], + ]; + + yield 'house manager and teamer' => [ + (new CrmAttributesResponse())->setHouseManager(true)->setTeamer(true), + [User::PENDING_ROLES['ROLE_HOUSE_MANAGER'], 'ROLE_TEAMER'], + ]; + } + public function testUpdateLocalUserSyncsUserAndTeamerDataFromBusPro(): void { $user = (new User()) @@ -37,6 +91,8 @@ class UserDataHandlerTest extends TestCase ->setEmail('old@example.com') ->setBusProAddressId(1) ->setBusProPersonId(2) + ->setRoles(['ROLE_ADMIN']) + ->setHotelCodes(['XYZ']) ; $teamer = (new Teamer()) @@ -61,17 +117,17 @@ class UserDataHandlerTest extends TestCase $handler->updateLocalUser( $user, $profileResponse, - ['ROLE_TEAMER'], true, ['team' => ['selected' => true]], - ['ABC'], ); $this->assertSame('New', $user->getFirstName()); $this->assertSame('Lastname', $user->getLastName()); $this->assertSame('new@example.com', $user->getEmail()); - $this->assertSame(['ABC'], $user->getHotelCodes()); - $this->assertTrue($user->hasRole('ROLE_TEAMER')); + + // roles and hotel codes are imported on creation only and stay under manual control + $this->assertSame(['XYZ'], $user->getHotelCodes()); + $this->assertTrue($user->hasRole('ROLE_ADMIN')); $this->assertSame('New', $teamer->getFirstName()); $this->assertSame('Lastname', $teamer->getLastName()); @@ -89,6 +145,74 @@ class UserDataHandlerTest extends TestCase $this->assertSame(['team' => ['selected' => true]], $teamer->getCrmSelections()); } + /** + * @dataProvider pendingRolesProvider + */ + public function testUpdateLocalUserRefreshesThePendingRoles( + array $roles, + array $claimedRoles, + array $expectedAssignedRoles, + array $expectedPendingRoles, + ): void { + $user = (new User()) + ->setFirstName('Old') + ->setLastName('Name') + ->setEmail('old@example.com') + ->setRoles($roles) + ; + + $handler = new UserDataHandler($this->entityManager, $this->logger); + $handler->updateLocalUser($user, $this->createProfileResponse(), false, [], $claimedRoles); + + $this->assertSame($expectedAssignedRoles, $user->getAssignedRoles()); + $this->assertSame($expectedPendingRoles, $user->getPendingRoles()); + } + + public static function pendingRolesProvider(): iterable + { + yield 'marker is added when the CRM claims a manager' => [ + ['ROLE_TEAMER'], + [User::PENDING_ROLES['ROLE_MANAGER']], + ['ROLE_TEAMER'], + [User::PENDING_ROLES['ROLE_MANAGER']], + ]; + + yield 'marker is dropped when the CRM attribute is gone' => [ + [User::PENDING_ROLES['ROLE_HOUSE_MANAGER'], 'ROLE_TEAMER'], + [], + ['ROLE_TEAMER'], + [], + ]; + + yield 'an approved role is never marked again' => [ + ['ROLE_MANAGER'], + [User::PENDING_ROLES['ROLE_MANAGER']], + ['ROLE_MANAGER'], + [], + ]; + + yield 'a claim beyond the approved role stays pending' => [ + ['ROLE_MANAGER'], + [User::PENDING_ROLES['ROLE_ADMIN'], User::PENDING_ROLES['ROLE_MANAGER']], + ['ROLE_MANAGER'], + [User::PENDING_ROLES['ROLE_ADMIN']], + ]; + + yield 'the claimed role changes' => [ + [User::PENDING_ROLES['ROLE_HOUSE_MANAGER']], + [User::PENDING_ROLES['ROLE_MANAGER']], + [], + [User::PENDING_ROLES['ROLE_MANAGER']], + ]; + + yield 'granted roles are untouched without any claim' => [ + ['ROLE_ADMIN'], + [], + ['ROLE_ADMIN'], + [], + ]; + } + public function testFindLocalUserFallsBackToUniqueEmailAndRefreshesBusProIds(): void { $user = (new User()) diff --git a/tests/Config/HouseCatalogTest.php b/tests/Config/HouseCatalogTest.php new file mode 100644 index 0000000..e4b6cdc --- /dev/null +++ b/tests/Config/HouseCatalogTest.php @@ -0,0 +1,55 @@ + 'Silvana', + 'AGR' => 'Rotbach', + 'DPW' => 'Waldschlössli', + ]; + + public function testNameChoicesAreSortedAndMapLabelToName(): void + { + $catalog = new HouseCatalog(self::HOUSES); + + $this->assertSame([ + 'Rotbach' => 'Rotbach', + 'Silvana' => 'Silvana', + 'Waldschlössli' => 'Waldschlössli', + ], $catalog->getNameChoices()); + } + + public function testCodeChoicesAreSortedByLabelAndMapLabelToCode(): void + { + $catalog = new HouseCatalog(self::HOUSES); + + $this->assertSame([ + 'Rotbach' => 'AGR', + 'Silvana' => 'SVS', + 'Waldschlössli' => 'DPW', + ], $catalog->getCodeChoices()); + } + + public function testGetName(): void + { + $catalog = new HouseCatalog(self::HOUSES); + + $this->assertSame('Silvana', $catalog->getName('SVS')); + $this->assertNull($catalog->getName('XYZ')); + } + + public function testEmptyCatalog(): void + { + $catalog = new HouseCatalog([]); + + $this->assertSame([], $catalog->getNameChoices()); + $this->assertSame([], $catalog->getCodeChoices()); + } +} diff --git a/tests/Entity/UserTest.php b/tests/Entity/UserTest.php index 50f9096..b41b717 100644 --- a/tests/Entity/UserTest.php +++ b/tests/Entity/UserTest.php @@ -6,6 +6,8 @@ namespace App\Tests\Entity; use App\Entity\User; use PHPUnit\Framework\TestCase; +use Symfony\Component\Validator\ConstraintViolationListInterface; +use Symfony\Component\Validator\Validation; class UserTest extends TestCase { @@ -23,4 +25,64 @@ class UserTest extends TestCase $isMatch = $user->hasHotelCodeMatch('XXDEFXX'); $this->assertFalse($isMatch); } + + public function testPendingMarkersAreLabelledButNotAssignable(): void + { + $user = (new User())->setRoles([ + User::PENDING_ROLES['ROLE_ADMIN'], + User::PENDING_ROLES['ROLE_HOUSE_MANAGER'], + 'ROLE_TEAMER', + ]); + + $this->assertSame( + ['Admin (nicht freigeschaltet)', 'Hausleitung (nicht freigeschaltet)', 'Teamer'], + $user->getRolesLabels(), + ); + $this->assertSame(['ROLE_TEAMER'], $user->getAssignedRoles()); + $this->assertSame( + [User::PENDING_ROLES['ROLE_ADMIN'], User::PENDING_ROLES['ROLE_HOUSE_MANAGER']], + $user->getPendingRoles(), + ); + } + + public function testSuperAdminRequiresRoleAdmin(): void + { + $user = (new User()) + ->setRoles(['ROLE_MANAGER']) + ->setSuperAdmin(true) + ; + + $violations = $this->validate($user); + + $this->assertCount(1, $violations); + $this->assertSame('superAdmin', $violations[0]->getPropertyPath()); + } + + public function testSuperAdminWithRoleAdminIsValid(): void + { + $user = (new User()) + ->setRoles(['ROLE_ADMIN']) + ->setSuperAdmin(true) + ; + + $this->assertCount(0, $this->validate($user)); + } + + public function testNonSuperAdminWithoutRoleAdminIsValid(): void + { + $user = (new User()) + ->setRoles(['ROLE_TEAMER']) + ; + + $this->assertCount(0, $this->validate($user)); + } + + private function validate(User $user): ConstraintViolationListInterface + { + return Validation::createValidatorBuilder() + ->enableAttributeMapping() + ->getValidator() + ->validate($user) + ; + } } diff --git a/tests/Form/UserTypeTest.php b/tests/Form/UserTypeTest.php new file mode 100644 index 0000000..524c129 --- /dev/null +++ b/tests/Form/UserTypeTest.php @@ -0,0 +1,98 @@ +setRoles(['ROLE_ADMIN']) + ->setSuperAdmin(true) + ; + + $view = $this->createForm($user)->createView(); + + // ROLE_USER and ROLE_SUPER_ADMIN are synthesized by getRoles() and must not leak in + $this->assertSame(['ROLE_ADMIN'], $view->children['roles']->vars['data']); + } + + public function testApprovingAPendingRoleClearsTheMarker(): void + { + $user = (new User())->setRoles([User::PENDING_ROLES['ROLE_ADMIN']]); + + $form = $this->createForm($user); + + // the marker is not an assignable choice and must not reach the field + $this->assertSame([], $form->createView()->children['roles']->vars['data']); + + $form->submit([ + 'roles' => ['ROLE_ADMIN'], + 'superAdmin' => null, + 'hotelCodes' => [], + ]); + + $this->assertTrue($form->isValid()); + $this->assertSame(['ROLE_ADMIN'], $user->getAssignedRoles()); + $this->assertSame([], $user->getPendingRoles()); + } + + public function testRendersHotelCodesNotCoveredByTheConfiguredMap(): void + { + $user = (new User())->setHotelCodes(['XYZ']); + + $view = $this->createForm($user)->createView(); + + $this->assertSame(['XYZ'], $view->children['hotelCodes']->vars['data']); + } + + public function testSubmitStoresAssignedRolesOnly(): void + { + $user = (new User())->setRoles(['ROLE_TEAMER']); + + $form = $this->createForm($user); + $form->submit([ + 'roles' => ['ROLE_ADMIN'], + 'superAdmin' => '1', + 'hotelCodes' => [], + ]); + + $this->assertTrue($form->isValid()); + $this->assertSame(['ROLE_ADMIN'], $user->getAssignedRoles()); + $this->assertTrue($user->isSuperAdmin()); + } + + public function testSubmitRejectsSuperAdminWithoutRoleAdmin(): void + { + $user = new User(); + + $form = $this->createForm($user); + $form->submit([ + 'roles' => ['ROLE_MANAGER'], + 'superAdmin' => '1', + 'hotelCodes' => [], + ]); + + $this->assertFalse($form->isValid()); + $this->assertCount(1, $form->get('superAdmin')->getErrors()); + } + + private function createForm(User $user): \Symfony\Component\Form\FormInterface + { + self::bootKernel(); + + /** @var FormFactoryInterface $formFactory */ + $formFactory = self::getContainer()->get(FormFactoryInterface::class); + + return $formFactory->create(UserType::class, $user, [ + 'csrf_protection' => false, + ]); + } +} diff --git a/tests/Security/Voter/UserVoterTest.php b/tests/Security/Voter/UserVoterTest.php new file mode 100644 index 0000000..6dffb60 --- /dev/null +++ b/tests/Security/Voter/UserVoterTest.php @@ -0,0 +1,82 @@ +security = $this->createMock(Security::class); + } + + public function testSuperAdminMayEditOtherUsers(): void + { + $this->assertSame( + VoterInterface::ACCESS_GRANTED, + $this->vote($this->createUser(true), $this->createUser(false)), + ); + } + + public function testPlainAdminMayNotEdit(): void + { + $this->assertSame( + VoterInterface::ACCESS_DENIED, + $this->vote($this->createUser(false), $this->createUser(false)), + ); + } + + public function testSuperAdminMayNotEditThemselves(): void + { + $currentUser = $this->createUser(true); + + $this->assertSame( + VoterInterface::ACCESS_DENIED, + $this->vote($currentUser, $currentUser), + ); + } + + public function testImpersonatorMayNotEdit(): void + { + $this->assertSame( + VoterInterface::ACCESS_DENIED, + $this->vote($this->createUser(true), $this->createUser(false), true), + ); + } + + private function vote(User $currentUser, User $targetUser, bool $isImpersonator = false): int + { + $this->security + ->method('isGranted') + ->with('IS_IMPERSONATOR') + ->willReturn($isImpersonator); + + $token = $this->createMock(TokenInterface::class); + $token + ->method('getUser') + ->willReturn($currentUser); + + $voter = new UserVoter($this->security); + + return $voter->vote($token, $targetUser, [UserVoter::EDIT]); + } + + private function createUser(bool $superAdmin): User + { + return (new User()) + ->setRoles(['ROLE_ADMIN']) + ->setSuperAdmin($superAdmin) + ; + } +}