diff --git a/config/services.yaml b/config/services.yaml index 8ac3c8a..35c423c 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -30,30 +30,33 @@ parameters: 10321389: 'Reisen-Alert Stubaital' 10554990: 'Reisen-Alert Ski & Boarderweek' - # Hausleitung hotel codes and their labels, assignable to users in /admin/user. - # Mirrors the "Hausleitung {CODE}" CRM selections published by BusProNet. - hotel_codes: - SSL: 'SSL' - SST: 'SST' - MVK: 'MVK' - ASB: 'ASB' - LPJ: 'LPJ' - DKS: 'DKS' - DGS: 'DGS' - DPW: 'DPW' - DKI: 'DKI' - DWW: 'DWW' - PMV: 'PMV' - ASG: 'ASG' - ASC: 'ASC' - SBW: 'SBW' - UCH: 'UCH' - SZO: 'SZO' - PCJ: 'PCJ' - KHH: 'KHH' - SVS: 'SVS' - SHM: 'SHM' - AGR: 'AGR' + # BusProNet "Hausleitung {CODE}" CRM selections, by selection id. + # DEPLOYMENT-CRITICAL: roles and hotel codes are synced on every login, so an id missing + # here does not merely fail to nominate a Hausleitung — it revokes the role and the hotel + # code from everyone holding it, one login at a time, with manual re-approval per user. + # Entries for houses that are not in use yet stay commented out. + bpn_crm_house_manager_ids: + 1299: 'SSL' + 1300: 'SST' + 1301: 'MVK' + # 1302: 'ASB' + 1303: 'LPJ' + 1304: 'DKS' + 1305: 'DGS' + 1306: 'DPW' + 1307: 'DKI' + 1308: 'DWW' + 1309: 'PMV' + # 1352: 'ASG' + 1371: 'ASC' + 1373: 'PCJ' + 1374: 'SBW' + # 1375: 'UCH' + # 1376: 'SZO' + 1377: 'KHH' + 1459: 'SVS' + 1461: 'SHM' + 1462: 'AGR' # MailJet contact metadata names for name synchronization mailjet_contact_metadata_fields: @@ -278,9 +281,9 @@ services: arguments: $mailjetLists: '%mailjet_lists%' - App\Form\Admin\UserType: + App\BusProNet\XmlParser\CrmAttributesResponseParser: arguments: - $hotelCodes: '%hotel_codes%' + $houseManagerIds: '%bpn_crm_house_manager_ids%' App\Service\DomainConfigProvider: arguments: diff --git a/src/BusProNet/XmlParser/CrmAttributesResponseParser.php b/src/BusProNet/XmlParser/CrmAttributesResponseParser.php index 18d4ce6..ce1d531 100644 --- a/src/BusProNet/XmlParser/CrmAttributesResponseParser.php +++ b/src/BusProNet/XmlParser/CrmAttributesResponseParser.php @@ -7,8 +7,21 @@ use App\BusProNet\Model\CrmAttributes; use App\BusProNet\Model\CrmSelection; use App\BusProNet\Model\CrmSelectionGroup; use App\BusProNet\Traits\TypeConversionTrait; +use App\Security\Role; use Symfony\Component\DomCrawler\Crawler; +/** + * Turns the SelektionCRM response into the roles and hotel codes the CRM claims for an account. + * + * This reports what BusPro says and nothing more: no fallback role, no default hotel code. What + * is made of the claims — which are granted outright and which only nominate — is Role::sync()'s + * business. + * + * The selection ids below are deployment-critical. BusPro always returns the full attribute tree + * and expresses membership through the `auswahl` flag, so a wrong or unset id yields a perfectly + * well-formed response in which nobody holds anything, and every user logging in is demoted one + * at a time. There is no signal inside the response that tells that apart from a real revocation. + */ class CrmAttributesResponseParser { use TypeConversionTrait; @@ -18,7 +31,13 @@ class CrmAttributesResponseParser private const BPN_CRM_ID_TEAMER = 1070; private const BPN_CRM_ID_GROUPS_MANAGER = 1477; private const BPN_CRM_ID_GROUPS_ADMIN = 1478; - private const BPN_DEFAULT_HOTEL_CODE = 'SSL'; + + /** + * @param array $houseManagerIds "Hausleitung" selection id => hotel code + */ + public function __construct(private readonly array $houseManagerIds = []) + { + } public function parse(Crawler $result): CrmAttributes { @@ -45,24 +64,28 @@ class CrmAttributesResponseParser $attribute->mutable = $this->stringToBool($node->attr('aenderbar')); $attribute->selected = $this->stringToBool($node->attr('auswahl')); - if (1 === preg_match('/^Hausleitung ([A-Z0-9]+)$/', $attribute->label, $matches) && true === $attribute->selected) { - $roles[] = 'ROLE_HOUSE_MANAGER'; - $hotelCodes[] = $matches[1]; - } - if (self::BPN_CRM_ID_ADMIN === $attribute->id && true === $attribute->selected) { - $roles[] = 'ROLE_ADMIN'; - } - if (self::BPN_CRM_ID_MANAGER === $attribute->id && true === $attribute->selected) { - $roles[] = 'ROLE_MANAGER'; - } - if (self::BPN_CRM_ID_TEAMER === $attribute->id && true === $attribute->selected) { - $roles[] = 'ROLE_TEAMER'; - } - if (self::BPN_CRM_ID_GROUPS_MANAGER === $attribute->id && true === $attribute->selected) { - $roles[] = 'ROLE_GROUPS_MANAGER'; - } - if (self::BPN_CRM_ID_GROUPS_ADMIN === $attribute->id && true === $attribute->selected) { - $roles[] = 'ROLE_GROUPS_ADMIN'; + if (true === $attribute->selected) { + $hotelCode = $this->houseManagerIds[$attribute->id] ?? null; + + if (null !== $hotelCode) { + $roles[] = Role::HOUSE_MANAGER; + $hotelCodes[] = $hotelCode; + } + if (self::BPN_CRM_ID_ADMIN === $attribute->id) { + $roles[] = Role::ADMIN; + } + if (self::BPN_CRM_ID_MANAGER === $attribute->id) { + $roles[] = Role::MANAGER; + } + if (self::BPN_CRM_ID_TEAMER === $attribute->id) { + $roles[] = Role::TEAMER; + } + if (self::BPN_CRM_ID_GROUPS_MANAGER === $attribute->id) { + $roles[] = Role::GROUPS_MANAGER; + } + if (self::BPN_CRM_ID_GROUPS_ADMIN === $attribute->id) { + $roles[] = Role::GROUPS_ADMIN; + } } $attributes[] = $attribute; @@ -74,13 +97,6 @@ class CrmAttributesResponseParser }) ; - $roles = array_unique($roles); - - // Assign default role if none could be resolved - if (0 === count($roles)) { - $roles = ['ROLE_CUSTOMER']; - } - $result ->filterXPath('//crmaktionen/crmaktion') ->each(function (Crawler $node) use (&$actions) { @@ -96,15 +112,11 @@ class CrmAttributesResponseParser }) ; - if (true === in_array('ROLE_ADMIN', $roles, true)) { - $hotelCodes[] = self::BPN_DEFAULT_HOTEL_CODE; - } - $response = new CrmAttributes(); $response->selectionGroups = $groups; $response->crmActions = $actions; - $response->roles = $roles; - $response->hotelCodes = $hotelCodes; + $response->roles = array_values(array_unique($roles)); + $response->hotelCodes = array_values(array_unique($hotelCodes)); return $response; } diff --git a/src/Controller/Admin/User/ApproveRoleController.php b/src/Controller/Admin/User/ApproveRoleController.php new file mode 100644 index 0000000..9f40a48 --- /dev/null +++ b/src/Controller/Admin/User/ApproveRoleController.php @@ -0,0 +1,106 @@ + 'ROLE_[A-Z_]+'])] + public function index(User $user, string $role, Request $request): Response + { + if ($user === $this->getUser() && \in_array($role, Role::SELF_APPROVAL_FORBIDDEN, true)) { + throw $this->createAccessDeniedException(sprintf('The role "%s" cannot be approved for your own account.', $role)); + } + + $nominated = Role::nominatedFrom($user->getRoles()); + + if (false === \array_key_exists($role, $nominated)) { + throw $this->createNotFoundException(sprintf('The account is not nominated for "%s".', $role)); + } + + $csrfTokenId = 'approve_user_role_'.$user->getId().'_'.$role; + + if ($request->isMethod(Request::METHOD_POST)) { + if (false === $this->isCsrfTokenValid($csrfTokenId, $request->request->getString('_token'))) { + throw $this->createAccessDeniedException('Invalid CSRF token.'); + } + + $user->setRoles(Role::approve($user->getRoles(), $role)); + + $this->entityManager->flush(); + + $this->reissueOwnSecurityToken($user); + + $this->addFlash('success', sprintf('Die Rolle %s wurde freigeschaltet', $nominated[$role])); + + $this->logger->info('Approved user role', [ + 'email' => $user->getEmail(), + 'role' => $role, + 'roles' => $user->getRoles(), + ]); + + return new HxRedirectResponse($this->getReturnUrl($request, 'app_admin_user')); + } + + return $this->render('admin/user/modal_approve_role.html.twig', [ + 'user' => $user, + 'role_label' => $nominated[$role], + 'csrf_token_id' => $csrfTokenId, + ]); + } + + /** + * Keeps the approver signed in when they just approved a role for themselves. + * + * A token carries the role names it was issued with, and ContextListener ends the session + * as soon as the stored user no longer matches them — the safeguard that makes a revocation + * take effect at once. Here the grant is deliberate and just happened under ROLE_ADMIN, so + * the token is re-issued with the new roles instead of the session being dropped. + */ + private function reissueOwnSecurityToken(User $user): void + { + $token = $this->tokenStorage->getToken(); + + if (false === $token instanceof PostAuthenticationToken || $token->getUser() !== $user) { + return; + } + + $this->tokenStorage->setToken( + new PostAuthenticationToken($user, $token->getFirewallName(), $user->getRoles()), + ); + } +} diff --git a/src/Controller/Admin/User/EditController.php b/src/Controller/Admin/User/EditController.php deleted file mode 100644 index fadba8d..0000000 --- a/src/Controller/Admin/User/EditController.php +++ /dev/null @@ -1,78 +0,0 @@ -getRoles(); - $previousHotelCodes = $user->getHotelCodes(); - - $form = $this->createForm(UserType::class, $user, ['hx_post' => $request->getRequestUri()]); - $form->handleRequest($request); - - if ($form->isSubmitted() && $form->isValid()) { - if ($this->locksOutSelf($user)) { - $form->get('roles')->addError(new FormError('Du kannst dir die Rolle Administration nicht selbst entziehen.')); - - return $this->render('admin/user/modal_edit.html.twig', [ - 'user' => $user, - 'form' => $form, - 'syncedRoles' => Role::syncedOnly($user->getRoles()), - ]); - } - - $this->entityManager->flush(); - - $this->addFlash('success', 'Der Benutzeraccount wurde aktualisiert'); - - $this->logger->info('Updated user permissions', [ - 'email' => $user->getEmail(), - 'previousRoles' => $previousRoles, - 'roles' => $user->getRoles(), - 'previousHotelCodes' => $previousHotelCodes, - 'hotelCodes' => $user->getHotelCodes(), - ]); - - return new HxRedirectResponse($this->getReturnUrl($request, 'app_admin_user')); - } - - return $this->render('admin/user/modal_edit.html.twig', [ - 'user' => $user, - 'form' => $form, - 'syncedRoles' => Role::syncedOnly($user->getRoles()), - ]); - } - - private function locksOutSelf(User $user): bool - { - return $user === $this->getUser() && false === \in_array(Role::ADMIN, $user->getRoles(), true); - } -} diff --git a/src/Controller/Admin/User/ShowController.php b/src/Controller/Admin/User/ShowController.php new file mode 100644 index 0000000..e8bd5a6 --- /dev/null +++ b/src/Controller/Admin/User/ShowController.php @@ -0,0 +1,57 @@ +getRoles()); + + // What ApproveRoleController would refuse for this account is not offered either, so + // nobody is sent into an access denied page. + $refused = $user === $this->getUser() + ? array_intersect_key($nominated, array_flip(Role::SELF_APPROVAL_FORBIDDEN)) + : []; + + return $this->render('admin/user/modal_permissions.html.twig', [ + 'user' => $user, + 'approvableRoles' => array_diff_key($nominated, $refused), + 'selfRefusedRoles' => $refused, + 'returnUrl' => $this->forwardedReturnUrl($request), + ]); + } + + /** + * Where an approval started from, still encoded the way return_url() handed it over. + * + * The approval links must forward what this modal was given rather than call return_url() + * themselves: that function answers with the *current* request URI, which here is the modal + * itself — and redirecting to it after the approval would render a bare modal as a page. + */ + private function forwardedReturnUrl(Request $request): string + { + $returnUrl = $request->query->getString('r'); + + return '' !== $returnUrl ? $returnUrl : rawurlencode($this->generateUrl('app_admin_user')); + } +} diff --git a/src/Controller/Api/UserinfoController.php b/src/Controller/Api/UserinfoController.php index 9981af7..e199a3e 100644 --- a/src/Controller/Api/UserinfoController.php +++ b/src/Controller/Api/UserinfoController.php @@ -55,7 +55,7 @@ class UserinfoController extends AbstractController // Patch current user's roles. The implicit ROLE_USER says nothing about the // account — every authenticated user holds it — and is not exported. - $data->roles = Role::assignedOnly($user->getRoles()); + $data->roles = Role::effectiveOnly($user->getRoles()); // Patch current user's hotel codes $data->hotelCodes = $user->getHotelCodes(); diff --git a/src/Form/Admin/UserType.php b/src/Form/Admin/UserType.php deleted file mode 100644 index 293b976..0000000 --- a/src/Form/Admin/UserType.php +++ /dev/null @@ -1,98 +0,0 @@ - - */ -class UserType extends AbstractType -{ - /** - * @param array $hotelCodes code => label - */ - public function __construct(private readonly array $hotelCodes = []) - { - } - - public function buildForm(FormBuilderInterface $builder, array $options): void - { - $user = $builder->getData(); - - $builder - ->add('roles', ChoiceType::class, [ - 'label' => 'Rollen', - 'choices' => $this->privilegedChoices(), - 'multiple' => true, - 'expanded' => true, - 'required' => false, - 'help' => 'Alle übrigen Rollen kommen bei jeder Anmeldung aus BusPro und lassen sich hier nicht ändern.', - // Only the administrator-granted half is editable; the synced half is preserved, - // as is the implicit ROLE_USER, which must never be written back. - 'getter' => static fn (User $user): array => Role::privilegedOnly($user->getRoles()), - 'setter' => static function (User $user, array $roles): void { - $user->setRoles(Role::combine($user->getRoles(), $roles)); - }, - ]) - ->add('hotelCodes', ChoiceType::class, [ - 'label' => 'Häuser', - 'choices' => $this->hotelCodeChoices($user), - 'multiple' => true, - 'expanded' => true, - 'required' => false, - 'setter' => static function (User $user, array $hotelCodes): void { - $user->setHotelCodes(array_values(array_unique($hotelCodes))); - }, - ]) - ; - } - - /** - * @return array label => role - */ - private function privilegedChoices(): array - { - return array_flip(array_intersect_key(Role::labels(), array_flip(Role::PRIVILEGED))); - } - - /** - * Codes already stored on the account are always offered, even when they are missing from - * the configured catalog — otherwise saving the form would silently drop them. - * - * @return array label => code - */ - private function hotelCodeChoices(?User $user): array - { - $codes = $this->hotelCodes; - - foreach ($user?->getHotelCodes() ?? [] as $code) { - $codes[$code] ??= $code; - } - - ksort($codes); - - return array_flip($codes); - } - - public function configureOptions(OptionsResolver $resolver): void - { - $resolver->setDefaults([ - 'data_class' => User::class, - ]); - } -} diff --git a/src/Security/BpnAuthenticator.php b/src/Security/BpnAuthenticator.php index 1a49219..63d4245 100644 --- a/src/Security/BpnAuthenticator.php +++ b/src/Security/BpnAuthenticator.php @@ -8,6 +8,7 @@ use App\BusProNet\ApiClient; use App\BusProNet\Exception\ApiClientException; use App\BusProNet\Exception\ImmediateConnectionCloseException; use App\BusProNet\Exception\TimeoutException; +use App\BusProNet\Model\CrmAttributes; use App\BusProNet\Model\PersonalData; use App\Entity\User; use App\Htmx\HxRedirectResponse; @@ -34,13 +35,11 @@ use Symfony\Component\Security\Http\Util\TargetPathTrait; * Validates credentials via BPN's getPersonalData endpoint and creates or updates * local User entities. Passwords are stored encrypted with RSA for subsequent API calls. * - * Roles have two owners. The non-privileged ones mirror the CRM selections on every login, in - * both directions, so somebody who becomes (or stops being) a Teamer in BusPro is granted (or - * loses) ROLE_TEAMER here — the sibling app myep-team gates on it. Role::PRIVILEGED is never - * imported: BusPro backend users can edit their own CRM selections, so honouring those would - * let anybody make themselves an administrator; they are granted in /admin/user only. - * - * Hotel codes still seed a *new* account only and are managed in /admin/user afterwards. + * BusPro owns the whole role set and the hotel codes: both are synced on every login, in both + * directions, so anything the CRM no longer reports is withdrawn here. What the CRM claims is + * not automatically granted, though — Role::sync() turns an administrative claim into a + * nomination that an administrator has to approve in /admin/user, because BusPro backend users + * can edit their own CRM selections and would otherwise make themselves administrators. */ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements AuthenticationEntryPointInterface { @@ -109,13 +108,13 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent if (null === $user = $userRepository->findOneBy(['email' => $email])) { $user = new User($email); - $user->setHotelCodes(array_values(array_unique($crmAttributes->hotelCodes))); $this->entityManager->persist($user); } + $this->syncFromCrm($user, $crmAttributes); + $user - ->setRoles($this->syncedRoles($email, $user->getRoles(), $crmAttributes->roles)) ->setPassword($encryptedPassword) ->setPersonId($personalData->personId) ->setAddressId($personalData->addressId) @@ -131,40 +130,43 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent } /** - * Merges the two halves of the role set: the non-privileged roles the BusPro CRM currently - * reports, and the privileged ones an administrator granted here. Anything the CRM no - * longer reports is dropped, so revoking a selection there revokes it here too. - * - * @param string[] $storedRoles - * @param string[] $crmRoles - * - * @return string[] + * Writes back what the CRM currently claims: the roles per Role::sync() and the hotel codes + * verbatim. Both replace what is stored, which is what makes BusPro the source of truth. */ - private function syncedRoles(string $email, array $storedRoles, array $crmRoles): array + private function syncFromCrm(User $user, CrmAttributes $crmAttributes): void { - return Role::combine($this->importableRoles($email, $crmRoles), $storedRoles); - } + $previousRoles = $user->getRoles(); - /** - * @param string[] $crmRoles - * - * @return string[] - */ - private function importableRoles(string $email, array $crmRoles): array - { - $roles = Role::filterImportable($crmRoles); - $dropped = array_values(array_intersect($crmRoles, Role::PRIVILEGED)); - - if ([] !== $dropped) { - // Somebody holds a privileged CRM selection in BusPro. We do not honour it, but it - // should stay visible: it either needs to be revoked there or granted in /admin/user. - $this->authLogger->warning('Ignored privileged roles from BPN CRM attributes', [ - 'email' => $email, - 'roles' => $dropped, + if ([] === $crmAttributes->selectionGroups) { + // BusPro always answers with the full attribute tree and expresses membership through + // the `auswahl` flag, so an empty one is a degraded payload rather than a revocation. + // Syncing it would strip the roles of every user who logs in. + $this->authLogger->warning('Skipped the role sync: the BPN CRM response carries no selection groups', [ + 'email' => $user->getEmail(), ]); + + // An existing account keeps everything it has. A brand new one still needs a role, + // and an empty claim set is exactly what Role::sync() answers with the fallback. + if ([] !== Role::assignedOnly($previousRoles)) { + return; + } } - return $roles; + $user + ->setRoles(Role::sync($previousRoles, $crmAttributes->roles)) + ->setHotelCodes(array_values(array_unique($crmAttributes->hotelCodes))) + ; + + $nominated = array_diff(Role::pendingOnly($user->getRoles()), Role::pendingOnly($previousRoles)); + + if ([] !== $nominated) { + // The CRM claims an administrative role for somebody who does not hold it. It grants + // nothing until an administrator approves it in /admin/user. + $this->authLogger->info('Nominated for administrative roles by the BPN CRM', [ + 'email' => $user->getEmail(), + 'roles' => array_values($nominated), + ]); + } } public function onAuthenticationSuccess(Request $request, TokenInterface $token, string $firewallName): ?Response diff --git a/src/Security/Role.php b/src/Security/Role.php index 46b9651..a3e28af 100644 --- a/src/Security/Role.php +++ b/src/Security/Role.php @@ -5,11 +5,17 @@ declare(strict_types=1); namespace App\Security; /** - * The roles this application knows about. + * The roles this application knows about, and the policy that assigns them. * * Roles are stored as plain strings in the User::$roles JSON column and consumed as strings * by #[IsGranted], the voters and security.yaml's role_hierarchy, so they are constants * rather than an enum. + * + * The governing rule: the BusPro CRM is the source of truth. It may nominate, but never grant, + * an administrative role — a nomination is stored as a marker (ROLE_X_PENDING) that grants + * nothing until an administrator approves it in /admin/user. The CRM's word alone is enough to + * take a role away, never to hand it out, and an administrator's word alone is enough for + * neither. */ final class Role { @@ -27,6 +33,14 @@ final class Role public const GROUPS_ADMIN = 'ROLE_GROUPS_ADMIN'; public const GROUPS_MANAGER = 'ROLE_GROUPS_MANAGER'; + /** + * Appended to an administrative role to mark it as claimed by the CRM but not yet approved. + * Markers live in the same column as the real roles and are handed to Symfony along with + * them, but nothing references them: no access_control rule, no role_hierarchy entry and no + * voter. Always ask effectiveOnly() when the question is what an account may actually do. + */ + public const PENDING_SUFFIX = '_PENDING'; + /** * The roles that are actually assigned to accounts. ROLE_USER is left out because every * account has it implicitly (see User::getRoles()) and it is never stored. @@ -44,37 +58,54 @@ final class Role ]; /** - * Roles that are never taken over from BusProNet. Many people can edit CRM selections in - * the BusPro backend, so these are granted by an administrator in /admin/user only. + * Roles the CRM grants outright. ROLE_CUSTOMER is never claimed by BusPro — it is the + * fallback for an account left without any effective role, and exclusive with the others. * * @var string[] */ - public const PRIVILEGED = [ + public const UNCONDITIONAL = [ + self::TEAMER, + self::CUSTOMER, + ]; + + /** + * Roles the CRM only nominates for. Many people can edit CRM selections in the BusPro + * backend, so honouring these directly would let anybody make themselves an administrator. + * + * @var string[] + */ + public const ADMINISTRATIVE = [ self::ADMIN, + self::MANAGER, + self::HOUSE_MANAGER, self::GROUPS_ADMIN, self::GROUPS_MANAGER, ]; /** - * Reduces the roles derived from BPN CRM attributes to the ones we accept from there. + * Roles nobody may approve for their own account. ROLE_ADMIN outranks every check in this + * application, including the approval surface itself, so it always takes a second + * administrator. The remaining administrative roles grant less than what an approver + * already holds, so requiring a second pair of eyes for them would only lock out the + * single-administrator case for no gain. * - * Falls back to ROLE_CUSTOMER the way CrmAttributesResponseParser does, so an account - * whose only selection was a privileged one does not end up without any role. - * - * @param string[] $roles - * - * @return string[] + * @var string[] */ - public static function filterImportable(array $roles): array - { - $importable = array_values(array_unique(array_diff($roles, self::PRIVILEGED))); + public const SELF_APPROVAL_FORBIDDEN = [ + self::ADMIN, + ]; - return [] === $importable ? [self::CUSTOMER] : $importable; + /** + * The marker standing for a role that the CRM claims but nobody has approved. + */ + public static function pending(string $role): string + { + return $role.self::PENDING_SUFFIX; } /** - * The roles actually assigned to an account — everything except the implicit ROLE_USER, - * which User::getRoles() prepends and which is never stored. + * The roles actually stored on an account — everything except the implicit ROLE_USER, + * which User::getRoles() prepends and which is never stored. Markers included. * * @param string[] $roles * @@ -86,41 +117,148 @@ final class Role } /** - * The non-privileged half of a role set: what BpnAuthenticator syncs from the BusPro CRM. + * The roles that actually grant something: no ROLE_USER, no markers. * * @param string[] $roles * * @return string[] */ - public static function syncedOnly(array $roles): array + public static function effectiveOnly(array $roles): array { - return array_values(array_diff(self::assignedOnly($roles), self::PRIVILEGED)); + return array_values(array_filter( + self::assignedOnly($roles), + static fn (string $role): bool => false === self::isPending($role), + )); } /** - * The privileged half: what an administrator granted in /admin/user. + * The markers on an account: administrative roles the CRM claims, awaiting approval. * * @param string[] $roles * * @return string[] */ - public static function privilegedOnly(array $roles): array + public static function pendingOnly(array $roles): array { - return array_values(array_intersect($roles, self::PRIVILEGED)); + return array_values(array_filter($roles, static fn (string $role): bool => self::isPending($role))); } /** - * Reassembles a complete role set from its two owners. Both sides are filtered, so a - * privileged role can never arrive through the synced half and vice versa. + * The roles behind those markers, labelled — what an approver acts on. * - * @param string[] $synced - * @param string[] $privileged + * @param string[] $roles + * + * @return array role => label + */ + public static function nominatedFrom(array $roles): array + { + $labels = self::labels(); + $nominated = []; + + foreach (self::pendingOnly($roles) as $marker) { + $role = self::realRole($marker); + + if (\in_array($role, self::ADMINISTRATIVE, true)) { + $nominated[$role] = $labels[$role]; + } + } + + return $nominated; + } + + /** + * The whole policy, applied on every login. + * + * 1. revoke everything the CRM no longer claims — granted roles and markers alike, which is + * what makes BusPro the source of truth; + * 2. grant the unconditional roles it claims; + * 3. mark every administrative role it claims that is not granted already. This runs after + * the revocation, so a role just revoked is not immediately marked again, and an approved + * role is never marked a second time; + * 4. fall back to ROLE_CUSTOMER when nothing effective is left. + * + * Nothing here can raise a privilege: step 3 only ever produces markers. + * + * @param string[] $storedRoles + * @param string[] $claimedRoles what the CRM reports * * @return string[] */ - public static function combine(array $synced, array $privileged): array + public static function sync(array $storedRoles, array $claimedRoles): array { - return array_values(array_unique([...self::syncedOnly($synced), ...self::privilegedOnly($privileged)])); + $claimed = array_values(array_intersect(self::ALL, array_unique($claimedRoles))); + + $roles = array_values(array_filter( + self::assignedOnly($storedRoles), + static fn (string $role): bool => \in_array(self::realRole($role), $claimed, true), + )); + + foreach (array_intersect($claimed, self::UNCONDITIONAL) as $role) { + $roles[] = $role; + } + + foreach (array_intersect($claimed, self::ADMINISTRATIVE) as $role) { + if (false === \in_array($role, $roles, true)) { + $roles[] = self::pending($role); + } + } + + return self::withCustomerFallback(array_values(array_unique($roles))); + } + + /** + * Turns a marker into the role it stands for. Refuses anything the account is not nominated + * for, so neither a hand-crafted request nor a claim revoked while the confirmation dialog + * was open can grant a role the CRM never reported. + * + * @param string[] $storedRoles + * + * @return string[] + * + * @throws \InvalidArgumentException when $role is not pending on this account + */ + public static function approve(array $storedRoles, string $role): array + { + $roles = self::assignedOnly($storedRoles); + + if (false === \in_array(self::pending($role), $roles, true)) { + throw new \InvalidArgumentException(sprintf('The role "%s" is not pending approval.', $role)); + } + + $roles = array_diff($roles, [self::pending($role)]); + $roles[] = $role; + + return self::withCustomerFallback(array_values(array_unique($roles))); + } + + /** + * Everybody without an effective role is a customer, and nobody with one is. The fallback is + * a fallback, not a baseline — ROLE_CUSTOMER and ROLE_TEAMER are mutually exclusive on + * purpose. + * + * @param string[] $roles + * + * @return string[] + */ + private static function withCustomerFallback(array $roles): array + { + if ([] === array_diff(self::effectiveOnly($roles), [self::CUSTOMER])) { + return array_values(array_unique([...$roles, self::CUSTOMER])); + } + + return array_values(array_diff($roles, [self::CUSTOMER])); + } + + private static function isPending(string $role): bool + { + return str_ends_with($role, self::PENDING_SUFFIX); + } + + private static function realRole(string $role): string + { + return self::isPending($role) + ? substr($role, 0, -\strlen(self::PENDING_SUFFIX)) + : $role; } /** @@ -128,7 +266,7 @@ final class Role */ public static function labels(): array { - return [ + $labels = [ self::ADMIN => 'Administration', self::MANAGER => 'Manager:in', self::TEAMER => 'Teamer:in', @@ -137,5 +275,11 @@ final class Role self::GROUPS_ADMIN => 'Preisrechner Admin', self::GROUPS_MANAGER => 'Preisrechner', ]; + + foreach (self::ADMINISTRATIVE as $role) { + $labels[self::pending($role)] = $labels[$role].' (nicht freigeschaltet)'; + } + + return $labels; } } diff --git a/src/Twig/AppExtension.php b/src/Twig/AppExtension.php index 0b4f2d3..a4c0ebd 100644 --- a/src/Twig/AppExtension.php +++ b/src/Twig/AppExtension.php @@ -27,7 +27,8 @@ class AppExtension extends AbstractExtension new TwigFilter('map_status', [AppRuntime::class, 'mapStatus']), new TwigFilter('map_country', [AppRuntime::class, 'mapCountry']), new TwigFilter('map_nationality', [AppRuntime::class, 'mapNationality']), - new TwigFilter('map_roles', [AppRuntime::class, 'mapRoles']), + new TwigFilter('effective_roles', [AppRuntime::class, 'effectiveRoles']), + new TwigFilter('nominated_roles', [AppRuntime::class, 'nominatedRoles']), ]; } diff --git a/src/Twig/AppRuntime.php b/src/Twig/AppRuntime.php index fd763c6..d6e5391 100644 --- a/src/Twig/AppRuntime.php +++ b/src/Twig/AppRuntime.php @@ -126,22 +126,44 @@ class AppRuntime implements RuntimeExtensionInterface } /** - * Turns the roles stored on a User into the labels the edit form uses. - * - * ROLE_USER is dropped because every account holds it implicitly; a role without a label - * is passed through unchanged so it stays visible instead of silently disappearing. + * The labels of the roles an account actually holds — no ROLE_USER, no nominations. * * @param string[] $roles * * @return string[] */ - public function mapRoles(array $roles): array + public function effectiveRoles(array $roles): array + { + return $this->roleLabels(Role::effectiveOnly($roles)); + } + + /** + * The labels of the roles the BusPro CRM claims for an account but nobody has approved. + * + * @param string[] $roles + * + * @return string[] + */ + public function nominatedRoles(array $roles): array + { + return array_values(Role::nominatedFrom($roles)); + } + + /** + * A role without a label is passed through unchanged so it stays visible instead of + * silently disappearing. + * + * @param string[] $roles + * + * @return string[] + */ + private function roleLabels(array $roles): array { $labels = Role::labels(); $mapped = []; - foreach (Role::assignedOnly($roles) as $role) { + foreach ($roles as $role) { $mapped[] = $labels[$role] ?? $role; } diff --git a/templates/admin/user/index.html.twig b/templates/admin/user/index.html.twig index c20549f..32b68c2 100644 --- a/templates/admin/user/index.html.twig +++ b/templates/admin/user/index.html.twig @@ -53,7 +53,14 @@ {{ user.personId | default('-') }} - {{ user.roles | map_roles | join(', ') | default('-') }} + {{ user.roles | effective_roles | join(', ') | default('-') }} + {% if user.roles | nominated_roles is not empty %} +
+ {% for label in user.roles | nominated_roles %} + {{ label }} + {% endfor %} +
+ {% endif %} {{ user.hotelCodes | join(', ') | default('-') }} @@ -63,8 +70,8 @@ - - Berechtigungen bearbeiten + + Berechtigungen diff --git a/templates/admin/user/modal_approve_role.html.twig b/templates/admin/user/modal_approve_role.html.twig new file mode 100644 index 0000000..b300ba0 --- /dev/null +++ b/templates/admin/user/modal_approve_role.html.twig @@ -0,0 +1,12 @@ +{% extends 'htmx_confirmation_modal.html.twig' %} + +{% block title %}Rolle freischalten{% endblock %} + +{% block content %} +
+ Möchtest du {{ user.email }} die Rolle {{ role_label }} freischalten? + Die Rolle bleibt bestehen, solange BusPro sie meldet, und kann hier nicht wieder entzogen werden. +
+{% endblock %} + +{% block button_confirm %}Freischalten{% endblock %} diff --git a/templates/admin/user/modal_edit.html.twig b/templates/admin/user/modal_edit.html.twig deleted file mode 100644 index 3342d39..0000000 --- a/templates/admin/user/modal_edit.html.twig +++ /dev/null @@ -1,27 +0,0 @@ -{% extends 'htmx_modal_admin.html.twig' %} -{% form_theme form 'forms_admin.html.twig' %} - -{% block title %} - Berechtigungen -{% endblock %} - -{% block content %} -
- {{ user.email }} -
-
- aus BusPro: {{ syncedRoles | map_roles | join(', ') | default('–') }} -
- {{ form_start(form) }} -
- {{ form_row(form.roles) }} - {{ form_row(form.hotelCodes) }} -
-
- -
- {{ form_rest(form) }} - {{ form_end(form) }} -{% endblock %} diff --git a/templates/admin/user/modal_permissions.html.twig b/templates/admin/user/modal_permissions.html.twig new file mode 100644 index 0000000..f7330e0 --- /dev/null +++ b/templates/admin/user/modal_permissions.html.twig @@ -0,0 +1,56 @@ +{% extends 'htmx_modal_admin.html.twig' %} + +{% block title %} + Berechtigungen +{% endblock %} + +{% block content %} +
+ {{ user.email }} +
+ +
Aus BusPro
+
+
+
Rollen
+
{{ user.roles | effective_roles | join(', ') | default('–') }}
+
Häuser
+
{{ user.hotelCodes | join(', ') | default('–') }}
+
Letzter Login
+
{{ user.lastLoginAt ? user.lastLoginAt | date('d.m.Y, H:i') : '–' }}
+
+

+ Rollen und Häuser werden bei jeder Anmeldung aus BusPro übernommen und lassen sich hier nicht ändern. +

+
+ +
Freischaltung
+ {% if approvableRoles is empty and selfRefusedRoles is empty %} +
+ Keine offenen Freischaltungen. +
+ {% else %} +
+ BusPro meldet diese Rollen für den Account. Sie sind erst nach der Freischaltung wirksam. +
+ {% if approvableRoles is not empty %} +
+ {% for role, label in approvableRoles %} + + {% endfor %} +
+ {% endif %} + {% if selfRefusedRoles is not empty %} +
+ {{ selfRefusedRoles | join(', ') }}: diese Rolle kannst du dir nicht selbst freischalten. + Bitte wende dich an eine:n andere:n Administrator:in. +
+ {% endif %} + {% endif %} +{% endblock %} diff --git a/tests/BusProNet/XmlParser/CrmAttributesResponseParserTest.php b/tests/BusProNet/XmlParser/CrmAttributesResponseParserTest.php index f76297a..aff1960 100644 --- a/tests/BusProNet/XmlParser/CrmAttributesResponseParserTest.php +++ b/tests/BusProNet/XmlParser/CrmAttributesResponseParserTest.php @@ -11,11 +11,17 @@ use Symfony\Component\DomCrawler\Crawler; class CrmAttributesResponseParserTest extends TestCase { + private const HOUSE_MANAGER_IDS = [ + 2001 => 'DKS', + 2002 => 'XYZ', + 2003 => 'ASB', + ]; + private CrmAttributesResponseParser $parser; protected function setUp(): void { - $this->parser = new CrmAttributesResponseParser(); + $this->parser = new CrmAttributesResponseParser(self::HOUSE_MANAGER_IDS); } public function testParseAssignsGroupsManagerRoleWhenSelected(): void @@ -38,9 +44,7 @@ class CrmAttributesResponseParserTest extends TestCase { $roles = $this->parseRoles($this->selectionXml(1477, false)); - self::assertNotContains('ROLE_GROUPS_MANAGER', $roles); - self::assertNotContains('ROLE_GROUPS_ADMIN', $roles); - self::assertSame(['ROLE_CUSTOMER'], $roles); + self::assertSame([], $roles, 'the parser reports what BusPro says and adds no fallback'); } public function testParseStillAssignsExistingAdminManagerTeamerRoles(): void @@ -73,11 +77,25 @@ class CrmAttributesResponseParserTest extends TestCase self::assertSame(['DKS', 'ASB'], $attributes->hotelCodes); } - public function testParseAddsTheDefaultHotelCodeForAdmins(): void + public function testParseIgnoresHausleitungSelectionsThatAreNotMapped(): void { + // A house that is deliberately left out of bpn_crm_house_manager_ids claims nothing — + // matching is by id, never by label. + $parser = new CrmAttributesResponseParser([]); + $attributes = $parser->parse((new Crawler($this->hausleitungXml()))->filterXPath('//ergebnis')); + + self::assertSame([], $attributes->roles); + self::assertSame([], $attributes->hotelCodes); + } + + public function testParseAddsNoDefaultHotelCodeForAdmins(): void + { + // Hotel codes are synced on every login now, so a default would permanently grant a + // house to every administrator. $attributes = $this->parse($this->selectionXml(1292, true)); - self::assertSame(['SSL'], $attributes->hotelCodes); + self::assertSame(['ROLE_ADMIN'], $attributes->roles); + self::assertSame([], $attributes->hotelCodes); } /** diff --git a/tests/Controller/Admin/User/ApproveRoleControllerTest.php b/tests/Controller/Admin/User/ApproveRoleControllerTest.php new file mode 100644 index 0000000..73c4712 --- /dev/null +++ b/tests/Controller/Admin/User/ApproveRoleControllerTest.php @@ -0,0 +1,211 @@ +nominatedUser(); + + $entityManager = $this->createMock(EntityManagerInterface::class); + $entityManager->expects(self::never())->method('flush'); + + $controller = new TestableApproveRoleController($entityManager, $this->createMock(LoggerInterface::class)); + + $response = $controller->index($user, Role::GROUPS_ADMIN, Request::create('/admin/user/1/approve/ROLE_GROUPS_ADMIN')); + + self::assertSame(Response::HTTP_OK, $response->getStatusCode()); + self::assertSame('admin/user/modal_approve_role.html.twig', $controller->renderedView); + self::assertSame([Role::TEAMER, Role::pending(Role::GROUPS_ADMIN)], Role::assignedOnly($user->getRoles())); + } + + public function testPostGrantsTheRoleAndRedirectsTheBrowser(): void + { + $user = $this->nominatedUser(); + + $entityManager = $this->createMock(EntityManagerInterface::class); + $entityManager->expects(self::once())->method('flush'); + + $controller = new TestableApproveRoleController($entityManager, $this->createMock(LoggerInterface::class)); + + $response = $controller->index($user, Role::GROUPS_ADMIN, Request::create('/admin/user/1/approve/ROLE_GROUPS_ADMIN', 'POST')); + + self::assertSame([Role::TEAMER, Role::GROUPS_ADMIN], Role::assignedOnly($user->getRoles())); + self::assertTrue($response->headers->has('HX-Redirect')); + self::assertSame(['success'], array_column($controller->flashes, 'type')); + } + + public function testARoleTheCrmNeverClaimedCannotBeApproved(): void + { + $entityManager = $this->createMock(EntityManagerInterface::class); + $entityManager->expects(self::never())->method('flush'); + + $controller = new TestableApproveRoleController($entityManager, $this->createMock(LoggerInterface::class)); + + $this->expectException(NotFoundHttpException::class); + + // ROLE_ADMIN is not nominated, so no hand-crafted request can grant it. + $controller->index($this->nominatedUser(), Role::ADMIN, Request::create('/admin/user/1/approve/ROLE_ADMIN', 'POST')); + } + + public function testApprovingRoleAdminForYourOwnAccountIsRefused(): void + { + $user = (new User('admin@example.org'))->setRoles([Role::ADMIN, Role::pending(Role::ADMIN)]); + + $entityManager = $this->createMock(EntityManagerInterface::class); + $entityManager->expects(self::never())->method('flush'); + + $controller = new TestableApproveRoleController($entityManager, $this->createMock(LoggerInterface::class), currentUser: $user); + + $this->expectException(AccessDeniedException::class); + + $controller->index($user, Role::ADMIN, Request::create('/admin/user/1/approve/ROLE_ADMIN', 'POST')); + } + + public function testApprovingALesserRoleForYourOwnAccountIsAllowed(): void + { + $user = $this->nominatedUser(); + + $entityManager = $this->createMock(EntityManagerInterface::class); + $entityManager->expects(self::once())->method('flush'); + + $controller = new TestableApproveRoleController($entityManager, $this->createMock(LoggerInterface::class), currentUser: $user); + + // Only ROLE_ADMIN needs a second pair of eyes — an approver already holds it, so the + // rest grant less than they could grant themselves anyway. + $controller->index($user, Role::GROUPS_ADMIN, Request::create('/admin/user/1/approve/ROLE_GROUPS_ADMIN', 'POST')); + + self::assertSame([Role::TEAMER, Role::GROUPS_ADMIN], Role::assignedOnly($user->getRoles())); + } + + public function testPostWithAnInvalidTokenIsDenied(): void + { + $entityManager = $this->createMock(EntityManagerInterface::class); + $entityManager->expects(self::never())->method('flush'); + + $controller = new TestableApproveRoleController($entityManager, $this->createMock(LoggerInterface::class), tokenValid: false); + + $this->expectException(AccessDeniedException::class); + + $controller->index($this->nominatedUser(), Role::GROUPS_ADMIN, Request::create('/admin/user/1/approve/ROLE_GROUPS_ADMIN', 'POST')); + } + + public function testApprovingForYourselfReissuesTheSecurityToken(): void + { + $user = $this->nominatedUser(); + + $tokenStorage = new TokenStorage(); + $tokenStorage->setToken(new PostAuthenticationToken($user, 'main', $user->getRoles())); + + $controller = new TestableApproveRoleController( + $this->createMock(EntityManagerInterface::class), + $this->createMock(LoggerInterface::class), + currentUser: $user, + tokenStorage: $tokenStorage, + ); + + $controller->index($user, Role::GROUPS_ADMIN, Request::create('/admin/user/1/approve/ROLE_GROUPS_ADMIN', 'POST')); + + // Without this the next request would find the stored roles out of step with the token + // and end the session, logging the approver out mid-action. + self::assertContains(Role::GROUPS_ADMIN, $tokenStorage->getToken()?->getRoleNames() ?? []); + self::assertNotContains(Role::pending(Role::GROUPS_ADMIN), $tokenStorage->getToken()?->getRoleNames() ?? []); + } + + public function testApprovingForSomebodyElseLeavesYourOwnTokenAlone(): void + { + $other = $this->nominatedUser(); + + $tokenStorage = new TokenStorage(); + $admin = (new User('admin@example.org'))->setRoles([Role::ADMIN]); + $tokenStorage->setToken($originalToken = new PostAuthenticationToken($admin, 'main', $admin->getRoles())); + + $controller = new TestableApproveRoleController( + $this->createMock(EntityManagerInterface::class), + $this->createMock(LoggerInterface::class), + currentUser: $admin, + tokenStorage: $tokenStorage, + ); + + $controller->index($other, Role::GROUPS_ADMIN, Request::create('/admin/user/1/approve/ROLE_GROUPS_ADMIN', 'POST')); + + self::assertSame($originalToken, $tokenStorage->getToken()); + } + + private function nominatedUser(): User + { + return (new User('teamer@example.org'))->setRoles([Role::TEAMER, Role::pending(Role::GROUPS_ADMIN)]); + } +} + +final class TestableApproveRoleController extends ApproveRoleController +{ + public ?string $renderedView = null; + + /** @var list */ + public array $flashes = []; + + public function __construct( + EntityManagerInterface $entityManager, + LoggerInterface $logger, + private readonly bool $tokenValid = true, + private readonly ?UserInterface $currentUser = null, + public readonly TokenStorageInterface $tokenStorage = new TokenStorage(), + ) { + parent::__construct($entityManager, $logger, $this->tokenStorage); + } + + protected function getUser(): ?UserInterface + { + return $this->currentUser; + } + + protected function isCsrfTokenValid(string $id, #[\SensitiveParameter] ?string $token): bool + { + return $this->tokenValid; + } + + /** + * @param array $parameters + */ + protected function render(string $view, array $parameters = [], ?Response $response = null): Response + { + $this->renderedView = $view; + + return new Response(); + } + + protected function addFlash(string $type, mixed $message): void + { + $this->flashes[] = ['type' => $type, 'message' => $message]; + } + + /** + * @param array $parameters + */ + public function generateUrl(string $route, array $parameters = [], int $referenceType = 1): string + { + return '/'.$route.'?'.http_build_query($parameters); + } +} diff --git a/tests/Controller/Admin/User/ShowControllerTest.php b/tests/Controller/Admin/User/ShowControllerTest.php new file mode 100644 index 0000000..02b4ad2 --- /dev/null +++ b/tests/Controller/Admin/User/ShowControllerTest.php @@ -0,0 +1,110 @@ +index($this->nominatedUser(), $this->permissionsRequest()); + + self::assertSame( + [Role::MANAGER => 'Manager:in', Role::GROUPS_ADMIN => 'Preisrechner Admin'], + $controller->parameters['approvableRoles'], + ); + self::assertSame([], $controller->parameters['selfRefusedRoles']); + } + + public function testWhatCannotBeSelfApprovedIsNotOffered(): void + { + $user = (new User('admin@example.org'))->setRoles([Role::ADMIN, Role::pending(Role::ADMIN), Role::pending(Role::MANAGER)]); + + $controller = new TestableShowController(currentUser: $user); + + $controller->index($user, $this->permissionsRequest()); + + // ROLE_ADMIN needs a second administrator, so no button leads into an access denied page. + self::assertSame([Role::MANAGER => 'Manager:in'], $controller->parameters['approvableRoles']); + self::assertSame([Role::ADMIN => 'Administration'], $controller->parameters['selfRefusedRoles']); + } + + public function testTheReturnUrlOfTheListIsForwardedUntouched(): void + { + $controller = new TestableShowController(); + + $controller->index($this->nominatedUser(), $this->permissionsRequest()); + + // Calling return_url() in the template instead would hand the approval this very modal + // and redirect the browser onto a bare modal fragment afterwards. + self::assertSame('%2Fadmin%2Fuser%3Fpage%3D2', $controller->parameters['returnUrl']); + } + + public function testWithoutAReturnUrlTheListIsUsed(): void + { + $controller = new TestableShowController(); + + $controller->index($this->nominatedUser(), Request::create('/admin/user/7/permissions')); + + self::assertSame(rawurlencode('/app_admin_user'), $controller->parameters['returnUrl']); + } + + private function nominatedUser(): User + { + return (new User('teamer@example.org')) + ->setRoles([Role::TEAMER, Role::pending(Role::MANAGER), Role::pending(Role::GROUPS_ADMIN)]); + } + + /** + * The list links the modal with r=return_url(), which rawurlencodes the URI, and path() + * encodes that again as a query value — so what arrives here is encoded exactly once. + */ + private function permissionsRequest(): Request + { + return Request::create('/admin/user/7/permissions?r='.rawurlencode(rawurlencode('/admin/user?page=2'))); + } +} + +final class TestableShowController extends ShowController +{ + /** @var array */ + public array $parameters = []; + + public function __construct(private readonly ?UserInterface $currentUser = null) + { + } + + protected function getUser(): ?UserInterface + { + return $this->currentUser; + } + + /** + * @param array $parameters + */ + protected function render(string $view, array $parameters = [], ?Response $response = null): Response + { + $this->parameters = $parameters; + + return new Response(); + } + + /** + * @param array $parameters + */ + protected function generateUrl(string $route, array $parameters = [], int $referenceType = 1): string + { + return '/'.$route; + } +} diff --git a/tests/Form/Admin/UserTypeTest.php b/tests/Form/Admin/UserTypeTest.php deleted file mode 100644 index 77bf37d..0000000 --- a/tests/Form/Admin/UserTypeTest.php +++ /dev/null @@ -1,89 +0,0 @@ -setRoles([Role::TEAMER, Role::GROUPS_MANAGER]); - - self::assertSame([Role::GROUPS_MANAGER], $this->createForm($user)->get('roles')->getData()); - } - - public function testOnlyPrivilegedRolesAreOffered(): void - { - $choices = $this->createForm(new User('teamer@example.org'))->get('roles')->getConfig()->getOption('choices'); - - // The rest is synced from BusPro on every login and would be overwritten right away. - self::assertSame(Role::PRIVILEGED, array_values($choices)); - } - - public function testSubmittingRolesKeepsTheSyncedOnesAndNotTheImplicitRoleUser(): void - { - $user = (new User('teamer@example.org'))->setRoles([Role::TEAMER]); - - $form = $this->createForm($user); - $form->submit(['roles' => [Role::GROUPS_MANAGER], 'hotelCodes' => []]); - - self::assertTrue($form->isSynchronized()); - // getRoles() prepends ROLE_USER; it must not have been persisted a second time. - self::assertSame(['ROLE_USER', Role::TEAMER, Role::GROUPS_MANAGER], $user->getRoles()); - } - - public function testClearingEveryCheckboxKeepsTheSyncedRoles(): void - { - $user = (new User('teamer@example.org')) - ->setRoles([Role::TEAMER, Role::GROUPS_MANAGER]) - ->setHotelCodes(['SSL']) - ; - - $form = $this->createForm($user); - $form->submit([]); - - self::assertTrue($form->isSynchronized()); - self::assertSame(['ROLE_USER', Role::TEAMER], $user->getRoles()); - self::assertSame([], $user->getHotelCodes()); - } - - public function testHotelCodeMissingFromTheCatalogSurvivesAnEdit(): void - { - $user = (new User('house@example.org'))->setHotelCodes(['SSL', 'XYZ']); - - $form = $this->createForm($user); - $form->submit(['roles' => [], 'hotelCodes' => ['SSL', 'XYZ']]); - - self::assertTrue($form->isSynchronized()); - self::assertSame(['SSL', 'XYZ'], $user->getHotelCodes()); - } - - public function testHotelCodesAreOfferedAlphabetically(): void - { - $user = (new User('house@example.org'))->setHotelCodes(['ASB']); - - $choices = $this->createForm($user)->get('hotelCodes')->getConfig()->getOption('choices'); - - self::assertSame(['ASB', 'DKS', 'SSL'], array_values($choices)); - } - - /** - * @return FormInterface - */ - private function createForm(User $user): FormInterface - { - return Forms::createFormFactoryBuilder() - ->addType(new UserType(['SSL' => 'SSL', 'DKS' => 'DKS'])) - ->getFormFactory() - ->create(UserType::class, $user) - ; - } -} diff --git a/tests/Security/BpnAuthenticatorTest.php b/tests/Security/BpnAuthenticatorTest.php index adc003e..961c938 100644 --- a/tests/Security/BpnAuthenticatorTest.php +++ b/tests/Security/BpnAuthenticatorTest.php @@ -6,11 +6,11 @@ namespace App\Tests\Security; use App\BusProNet\ApiClient; use App\BusProNet\Model\CrmAttributes; +use App\BusProNet\Model\CrmSelectionGroup; use App\BusProNet\Model\PersonalData; use App\Entity\User; use App\Security\BpnAuthenticator; use App\Security\Crypt; -use App\Security\DefaultRouteResolver; use App\Security\Role; use App\Service\ProfileCompletenessChecker; use Doctrine\ORM\EntityManagerInterface; @@ -22,16 +22,16 @@ use Symfony\Component\Routing\Generator\UrlGeneratorInterface; use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; /** - * Covers who may grant roles: BusPro backend users can edit their own CRM selections, so the - * import must not be a channel for privilege escalation. + * Covers what a login does to an account: BusPro owns the roles and the hotel codes, but a + * CRM claim must never grant an administrative role on its own. */ class BpnAuthenticatorTest extends TestCase { - public function testNewAccountIsSeededWithTheImportableRolesOnly(): void + public function testNewAccountIsSeededFromTheCrm(): void { $persisted = null; $authenticator = $this->authenticator( - $this->crmAttributes([Role::ADMIN, Role::TEAMER, Role::GROUPS_ADMIN], ['SSL', 'SSL']), + $this->crmAttributes([Role::ADMIN, Role::TEAMER], ['SSL', 'SSL']), null, $persisted, ); @@ -39,20 +39,17 @@ class BpnAuthenticatorTest extends TestCase $user = $this->loadUser($authenticator); self::assertSame($persisted, $user); - self::assertSame(['ROLE_USER', Role::TEAMER], $user->getRoles()); + self::assertSame(['ROLE_USER', Role::TEAMER, Role::pending(Role::ADMIN)], $user->getRoles()); self::assertSame(['SSL'], $user->getHotelCodes()); } - public function testExistingAccountKeepsThePrivilegedRolesAnAdministratorAssigned(): void + public function testAdministrativeClaimIsOnlyANominationUntilItIsApproved(): void { - $existing = (new User('teamer@example.org')) - ->setRoles([Role::TEAMER, Role::GROUPS_MANAGER]) - ->setHotelCodes(['DKS']) - ; + $existing = (new User('teamer@example.org'))->setRoles([Role::TEAMER]); $persisted = null; $authenticator = $this->authenticator( - $this->crmAttributes([Role::ADMIN, Role::CUSTOMER], ['SSL']), + $this->crmAttributes([Role::TEAMER, Role::GROUPS_ADMIN], []), $existing, $persisted, ); @@ -60,13 +57,42 @@ class BpnAuthenticatorTest extends TestCase $user = $this->loadUser($authenticator); self::assertNull($persisted, 'an existing account must not be persisted again'); - // ROLE_TEAMER is gone with its CRM selection, ROLE_ADMIN is still not honoured, and the - // administrator-granted ROLE_GROUPS_MANAGER survives. - self::assertSame(['ROLE_USER', Role::CUSTOMER, Role::GROUPS_MANAGER], $user->getRoles()); - self::assertSame(['DKS'], $user->getHotelCodes(), 'hotel codes stay administrator-managed'); + self::assertSame( + ['ROLE_USER', Role::TEAMER, Role::pending(Role::GROUPS_ADMIN)], + $user->getRoles(), + ); self::assertNotNull($user->getLastLoginAt(), 'the rest of the profile is still synced'); } + public function testApprovedRoleSurvivesTheNextLogin(): void + { + $existing = (new User('manager@example.org'))->setRoles([Role::TEAMER, Role::GROUPS_MANAGER]); + + $persisted = null; + $authenticator = $this->authenticator( + $this->crmAttributes([Role::TEAMER, Role::GROUPS_MANAGER], []), + $existing, + $persisted, + ); + + self::assertSame( + ['ROLE_USER', Role::TEAMER, Role::GROUPS_MANAGER], + $this->loadUser($authenticator)->getRoles(), + ); + } + + public function testRoleRevokedInBusProIsWithdrawnOnLogin(): void + { + $existing = (new User('manager@example.org'))->setRoles([Role::TEAMER, Role::GROUPS_MANAGER]); + + $persisted = null; + $authenticator = $this->authenticator($this->crmAttributes([], []), $existing, $persisted); + + // Nothing is claimed any more, so nothing is held — and an account without an effective + // role is a customer. + self::assertSame(['ROLE_USER', Role::CUSTOMER], $this->loadUser($authenticator)->getRoles()); + } + public function testRoleGainedInBusProIsGrantedOnLogin(): void { $existing = (new User('teamer@example.org'))->setRoles([Role::CUSTOMER]); @@ -82,29 +108,69 @@ class BpnAuthenticatorTest extends TestCase self::assertSame(['ROLE_USER', Role::TEAMER], $this->loadUser($authenticator)->getRoles()); } - public function testAccountWithoutAnyRoleIsHealedOnLogin(): void + public function testHotelCodesAreResyncedOnEveryLogin(): void { - $existing = new User('teamer@example.org'); + $existing = (new User('house@example.org')) + ->setRoles([Role::HOUSE_MANAGER]) + ->setHotelCodes(['DKS', 'SSL']) + ; $persisted = null; $authenticator = $this->authenticator( - $this->crmAttributes([Role::TEAMER], []), + $this->crmAttributes([Role::HOUSE_MANAGER], ['DKS']), $existing, $persisted, ); - self::assertSame(['ROLE_USER', Role::TEAMER], $this->loadUser($authenticator)->getRoles()); + self::assertSame(['DKS'], $this->loadUser($authenticator)->getHotelCodes()); + } + + public function testDegradedCrmResponseLeavesAnExistingAccountUntouched(): void + { + $existing = (new User('manager@example.org')) + ->setRoles([Role::TEAMER, Role::GROUPS_MANAGER]) + ->setHotelCodes(['DKS']) + ; + + $persisted = null; + // No selection groups at all: BusPro always answers with the full attribute tree, so + // this is a degraded payload and not a revocation of everything. + $authenticator = $this->authenticator( + $this->crmAttributes([], [], selectionGroups: []), + $existing, + $persisted, + ); + + $user = $this->loadUser($authenticator); + + self::assertSame(['ROLE_USER', Role::TEAMER, Role::GROUPS_MANAGER], $user->getRoles()); + self::assertSame(['DKS'], $user->getHotelCodes()); + } + + public function testDegradedCrmResponseStillGivesANewAccountTheFallbackRole(): void + { + $persisted = null; + $authenticator = $this->authenticator( + $this->crmAttributes([], [], selectionGroups: []), + null, + $persisted, + ); + + self::assertSame(['ROLE_USER', Role::CUSTOMER], $this->loadUser($authenticator)->getRoles()); } /** - * @param string[] $roles - * @param string[] $hotelCodes + * @param string[] $roles + * @param string[] $hotelCodes + * @param CrmSelectionGroup[] $selectionGroups only their presence matters here — an empty + * set is what marks a response as degraded */ - private function crmAttributes(array $roles, array $hotelCodes): CrmAttributes + private function crmAttributes(array $roles, array $hotelCodes, ?array $selectionGroups = null): CrmAttributes { $attributes = new CrmAttributes(); $attributes->roles = $roles; $attributes->hotelCodes = $hotelCodes; + $attributes->selectionGroups = $selectionGroups ?? [new CrmSelectionGroup()]; return $attributes; } @@ -144,7 +210,6 @@ class BpnAuthenticatorTest extends TestCase $crypt, $completenessChecker, $this->createMock(LoggerInterface::class), - $this->createMock(DefaultRouteResolver::class), ); } diff --git a/tests/Security/RoleTest.php b/tests/Security/RoleTest.php index 81c6064..2ee41cf 100644 --- a/tests/Security/RoleTest.php +++ b/tests/Security/RoleTest.php @@ -7,62 +7,102 @@ namespace App\Tests\Security; use App\Security\Role; use PHPUnit\Framework\TestCase; +/** + * Covers the role policy: BusPro backend users can edit their own CRM selections, so a claim + * must never grant an administrative role on its own. + */ class RoleTest extends TestCase { - public function testPrivilegedRolesAreNeverImported(): void + public function testAdministrativeClaimOnlyProducesANomination(): void { - $roles = Role::filterImportable([ - Role::TEAMER, - Role::ADMIN, - Role::GROUPS_ADMIN, - Role::GROUPS_MANAGER, - Role::HOUSE_MANAGER, - ]); + $roles = Role::sync([], [Role::ADMIN, Role::GROUPS_ADMIN, Role::TEAMER]); - self::assertSame([Role::TEAMER, Role::HOUSE_MANAGER], $roles); + self::assertSame( + [Role::TEAMER, Role::pending(Role::ADMIN), Role::pending(Role::GROUPS_ADMIN)], + $roles, + ); + self::assertSame([Role::TEAMER], Role::effectiveOnly($roles)); } - public function testResultIsADedupedList(): void + public function testApprovedRoleSurvivesTheNextSyncAndIsNotMarkedAgain(): void { - // CrmAttributesResponseParser applies array_unique(), which preserves keys — a - // non-list would be persisted as a JSON object instead of an array. - $roles = Role::filterImportable([0 => Role::ADMIN, 2 => Role::TEAMER, 5 => Role::TEAMER]); + $roles = Role::sync([Role::TEAMER, Role::GROUPS_ADMIN], [Role::GROUPS_ADMIN, Role::TEAMER]); + + self::assertSame([Role::TEAMER, Role::GROUPS_ADMIN], $roles); + } + + public function testRoleTheCrmNoLongerClaimsIsRevoked(): void + { + // Both halves go: BusPro is the source of truth for the granted role as much as for + // the nomination. + $roles = Role::sync([Role::TEAMER, Role::ADMIN, Role::pending(Role::MANAGER)], [Role::TEAMER]); self::assertSame([Role::TEAMER], $roles); - self::assertSame(array_keys($roles), range(0, \count($roles) - 1)); } - public function testAccountWithOnlyPrivilegedRolesFallsBackToCustomer(): void + public function testRevokedRoleIsNotImmediatelyNominatedAgain(): void { - self::assertSame([Role::CUSTOMER], Role::filterImportable([Role::ADMIN])); - self::assertSame([Role::CUSTOMER], Role::filterImportable([])); + self::assertSame([Role::CUSTOMER], Role::sync([Role::ADMIN], [])); } - public function testAssignedOnlyDropsTheImplicitRoleUser(): void + public function testAccountWithoutAnEffectiveRoleFallsBackToCustomer(): void { - $roles = Role::assignedOnly([Role::USER, Role::TEAMER, Role::GROUPS_ADMIN]); - - self::assertSame([Role::TEAMER, Role::GROUPS_ADMIN], $roles); - // The value is JSON-encoded into the userinfo response and must not become an object. - self::assertSame(array_keys($roles), range(0, \count($roles) - 1)); + // The nomination stays visible — it is what an approver acts on — but grants nothing, + // so the account is a customer in the meantime. + self::assertSame( + [Role::pending(Role::ADMIN), Role::CUSTOMER], + Role::sync([], [Role::ADMIN]), + ); + self::assertSame([Role::CUSTOMER], Role::sync([], [])); } - public function testCombineKeepsEachHalfInItsOwnLane(): void + public function testCustomerIsAFallbackAndNotABaseline(): void { - $roles = Role::combine([Role::TEAMER, Role::ADMIN], [Role::GROUPS_ADMIN, Role::TEAMER]); - - // The ADMIN from the synced half and the TEAMER from the privileged half are discarded. - self::assertSame([Role::TEAMER, Role::GROUPS_ADMIN], $roles); - self::assertSame(array_keys($roles), range(0, \count($roles) - 1)); + self::assertSame([Role::TEAMER], Role::sync([Role::CUSTOMER], [Role::TEAMER])); } - public function testCombineDropsTheImplicitRoleUser(): void + public function testUnknownClaimsAndTheImplicitRoleUserAreIgnored(): void { - self::assertSame([Role::TEAMER], Role::combine([Role::USER, Role::TEAMER], [])); + self::assertSame( + [Role::TEAMER], + Role::sync([Role::USER, Role::TEAMER], [Role::TEAMER, 'ROLE_SOMETHING_ELSE']), + ); } - public function testEveryRoleHasALabel(): void + public function testApprovalTurnsTheNominationIntoTheRole(): void { - self::assertSame(Role::ALL, array_keys(Role::labels())); + $roles = Role::approve([Role::pending(Role::ADMIN), Role::CUSTOMER], Role::ADMIN); + + // The customer fallback goes with it: the account now holds an effective role. + self::assertSame([Role::ADMIN], $roles); + } + + public function testApprovingARoleWithoutANominationIsRefused(): void + { + $this->expectException(\InvalidArgumentException::class); + + Role::approve([Role::TEAMER], Role::ADMIN); + } + + public function testEffectiveRolesExcludeNominationsAndTheImplicitRoleUser(): void + { + $roles = [Role::USER, Role::TEAMER, Role::pending(Role::ADMIN)]; + + self::assertSame([Role::TEAMER], Role::effectiveOnly($roles)); + self::assertSame([Role::pending(Role::ADMIN)], Role::pendingOnly($roles)); + self::assertSame([Role::ADMIN => 'Administration'], Role::nominatedFrom($roles)); + } + + public function testEveryRoleAndNominationHasALabel(): void + { + $labels = Role::labels(); + + foreach (Role::ALL as $role) { + self::assertArrayHasKey($role, $labels); + } + + foreach (Role::ADMINISTRATIVE as $role) { + self::assertArrayHasKey(Role::pending($role), $labels); + } } } diff --git a/tests/Twig/AppRuntimeMapRolesTest.php b/tests/Twig/AppRuntimeMapRolesTest.php deleted file mode 100644 index 9fcdec2..0000000 --- a/tests/Twig/AppRuntimeMapRolesTest.php +++ /dev/null @@ -1,36 +0,0 @@ -runtime()->mapRoles([Role::TEAMER, Role::GROUPS_MANAGER]), - ); - } - - public function testImplicitRoleUserIsNotListed(): void - { - self::assertSame(['Administration'], $this->runtime()->mapRoles(['ROLE_USER', Role::ADMIN])); - self::assertSame([], $this->runtime()->mapRoles(['ROLE_USER'])); - } - - public function testUnknownRoleStaysVisible(): void - { - self::assertSame(['ROLE_LEGACY'], $this->runtime()->mapRoles(['ROLE_LEGACY'])); - } - - private function runtime(): AppRuntime - { - return (new \ReflectionClass(AppRuntime::class))->newInstanceWithoutConstructor(); - } -} diff --git a/tests/Twig/AppRuntimeRoleLabelsTest.php b/tests/Twig/AppRuntimeRoleLabelsTest.php new file mode 100644 index 0000000..b31548d --- /dev/null +++ b/tests/Twig/AppRuntimeRoleLabelsTest.php @@ -0,0 +1,44 @@ +runtime()->effectiveRoles([Role::TEAMER, Role::GROUPS_MANAGER]), + ); + } + + public function testImplicitRoleUserIsNotListed(): void + { + self::assertSame(['Administration'], $this->runtime()->effectiveRoles([Role::USER, Role::ADMIN])); + self::assertSame([], $this->runtime()->effectiveRoles([Role::USER])); + } + + public function testNominationsAreListedApartFromTheEffectiveRoles(): void + { + $roles = [Role::TEAMER, Role::pending(Role::ADMIN)]; + + self::assertSame(['Teamer:in'], $this->runtime()->effectiveRoles($roles)); + self::assertSame(['Administration'], $this->runtime()->nominatedRoles($roles)); + } + + public function testUnknownRoleStaysVisible(): void + { + self::assertSame(['ROLE_LEGACY'], $this->runtime()->effectiveRoles(['ROLE_LEGACY'])); + } + + private function runtime(): AppRuntime + { + return (new \ReflectionClass(AppRuntime::class))->newInstanceWithoutConstructor(); + } +}