From fd5d478a5ccbafdb32a38450b8cf819e79f332e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Fromme?= Date: Mon, 10 Aug 2026 16:24:41 +0200 Subject: [PATCH] feat: streamlined role-revocation logic --- src/BusProNet/Model/CrmAttributesResponse.php | 5 +- src/BusProNet/UserDataHandler.php | 50 +++++-- .../Admin/Teamer/DisableUserController.php | 8 +- src/Form/UserType.php | 27 +++- src/Repository/UserRepository.php | 10 ++ src/Security/BpnAuthenticator.php | 20 ++- src/Service/Cron/FeedbackReminderService.php | 2 +- templates/admin/system/user/_form.html.twig | 1 + tests/BusProNet/ResponseParserTest.php | 58 ++++++++ tests/BusProNet/UserDataHandlerTest.php | 53 +++++++- tests/Form/UserTypeTest.php | 30 +++++ tests/Resources/crm_attributes_granted.xml | 29 ++++ tests/Resources/crm_attributes_revoked.xml | 29 ++++ tests/Security/BpnAuthenticatorTest.php | 126 ++++++++++++++++++ 14 files changed, 426 insertions(+), 22 deletions(-) create mode 100644 tests/Resources/crm_attributes_granted.xml create mode 100644 tests/Resources/crm_attributes_revoked.xml create mode 100644 tests/Security/BpnAuthenticatorTest.php diff --git a/src/BusProNet/Model/CrmAttributesResponse.php b/src/BusProNet/Model/CrmAttributesResponse.php index cbb3c5e..f659ccf 100644 --- a/src/BusProNet/Model/CrmAttributesResponse.php +++ b/src/BusProNet/Model/CrmAttributesResponse.php @@ -87,12 +87,13 @@ class CrmAttributesResponse { $crmSelections = []; - foreach ($this->getAttributeGroups() as $group) { + // a response the CRM sent no attributes in still has to flatten to an empty set + foreach ($this->getAttributeGroups() ?? [] as $group) { /** @var CrmAttributeGroup $group */ if (false === isset($crmSelections[$group->getLabel()])) { $crmSelections[$group->getLabel()] = []; } - foreach ($group->getAttributes() as $attribute) { + foreach ($group->getAttributes() ?? [] as $attribute) { /** @var CrmAttribute $attribute */ if (false === $attribute->isSelected()) { continue; diff --git a/src/BusProNet/UserDataHandler.php b/src/BusProNet/UserDataHandler.php index 810e0e2..a635f64 100644 --- a/src/BusProNet/UserDataHandler.php +++ b/src/BusProNet/UserDataHandler.php @@ -165,10 +165,11 @@ class UserDataHandler /** * 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. + * Administrative roles and hotel codes are imported once on user creation only and are + * managed manually afterwards, so they are intentionally left untouched here. The + * exceptions are the privilege-free pending markers, which keep tracking the + * administrative roles claimed in the CRM, and ROLE_TEAMER, which needs no approval + * and is granted to whoever the CRM reports as a teamer. * * @param string[] $claimedRoles pending markers as returned by collectPendingRoles() */ @@ -188,6 +189,8 @@ class UserDataHandler $this->refreshPendingRoles($user, $claimedRoles); if (true === $isTeamer) { + $this->grantTeamerRole($user); + $address = Address::fromApiResponse($profileResponse); $communication = Communication::fromApiResponse($profileResponse); if (null === $user->getTeamer()) { @@ -221,15 +224,19 @@ class UserDataHandler /** * Blocks a user the CRM no longer grants anything in this application. * - * The granted roles are deliberately kept: they stay visible for review and are what - * makes the user reappear in the administrative list, where a super admin can unblock - * them. Only the privilege-free markers are dropped, as they no longer reflect the CRM. + * The granted roles are deliberately kept: they stay visible for review. Only the + * privilege-free markers are dropped, as they no longer reflect the CRM. What keeps the + * user reachable is the block itself, not the roles: a teamer stays in the teamer list, + * everyone else is listed by getAdministrativeUsers() whatever roles are left. * Regaining a CRM role does not unblock the account, that is a manual decision. */ public function disableForRevokedCrmRoles(User $user): void { - // an existing block may be a disciplinary one and must never be overwritten + // an existing block may be a disciplinary one and must never be overwritten, but + // findLocalUser() may have refreshed the BusPro ids and nothing else flushes here if (true === $user->isDisabled()) { + $this->entityManager->flush(); + return; } @@ -249,6 +256,33 @@ class UserDataHandler ]); } + /** + * Grants ROLE_TEAMER to a user the CRM reports as a teamer. + * + * Unlike the administrative roles this one needs no approval, so it may be granted on + * any login rather than on creation only: it carries no privileges beyond the teamer + * area, and a teamer without it would be left with a teamer record they cannot reach, + * or locked out entirely for holding no assignable role at all. + * + * It is never withdrawn here. Losing the CRM attribute while holding no other role + * blocks the account anyway, and a role handed out manually must survive a login. + */ + private function grantTeamerRole(User $user): void + { + $grantedRoles = $user->getAssignedRoles(); + + if (true === in_array('ROLE_TEAMER', $grantedRoles, true)) { + return; + } + + $user->setRoles([...$grantedRoles, 'ROLE_TEAMER', ...$user->getPendingRoles()]); + + $this->logger->info('Grant teamer role', [ + 'user_id' => $user->getId(), + 'user_email' => $user->getEmail(), + ]); + } + /** * 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 diff --git a/src/Controller/Admin/Teamer/DisableUserController.php b/src/Controller/Admin/Teamer/DisableUserController.php index aa9d331..1f39983 100644 --- a/src/Controller/Admin/Teamer/DisableUserController.php +++ b/src/Controller/Admin/Teamer/DisableUserController.php @@ -29,7 +29,7 @@ class DisableUserController extends AbstractController $form = $this->createForm(DisableUserType::class, $user, ['hx_post' => $request->getUri()]); $form->handleRequest($request); if ($form->isSubmitted() && $form->isValid()) { - $user->setDisabledAt(new \DateTimeImmutable()); + $user->setDisabled(true); $this->entityManager->flush(); @@ -56,11 +56,7 @@ class DisableUserController extends AbstractController $user = $teamer->getUser(); if (true === $request->isMethod(Request::METHOD_POST)) { - $user - ->setDisabledAt(null) - ->setDisabledReason(null) - ->setDisabledReasonInternal(null) - ; + $user->setDisabled(false); $this->entityManager->flush(); diff --git a/src/Form/UserType.php b/src/Form/UserType.php index c94c39e..9fbfd2f 100644 --- a/src/Form/UserType.php +++ b/src/Form/UserType.php @@ -33,8 +33,6 @@ class UserType extends AbstractType 'required' => false, 'help' => 'Setzt die Rolle Admin voraus.', ]) - // must stay ahead of the reason: properties are written in field order and - // unblocking clears the reasons ->add('disabled', CheckboxType::class, [ 'label' => 'Account gesperrt', 'required' => false, @@ -49,8 +47,33 @@ class UserType extends AbstractType 'data-action' => 'textarea-autosize#resize', ], ]) + ->add('disabledReasonInternal', TextareaType::class, [ + 'label' => 'Begründung intern', + 'required' => false, + 'help' => 'Wird der Benutzer:in nicht angezeigt.', + 'attr' => [ + 'data-controller' => 'textarea-autosize', + 'data-action' => 'textarea-autosize#resize', + ], + ]) ; + // the reasons only ever describe a block, so an unblocked account carries none. + // Done after mapping instead of relying on the field order, as the submitted text + // would otherwise be written back over the reset done by setDisabled() + $builder->addEventListener(FormEvents::POST_SUBMIT, static function (FormEvent $event): void { + $user = $event->getData(); + + if (false === $user instanceof User || true === $user->isDisabled()) { + return; + } + + $user + ->setDisabledReason(null) + ->setDisabledReasonInternal(null) + ; + }); + // 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 { diff --git a/src/Repository/UserRepository.php b/src/Repository/UserRepository.php index ba09843..016f8ac 100644 --- a/src/Repository/UserRepository.php +++ b/src/Repository/UserRepository.php @@ -25,6 +25,11 @@ class UserRepository extends ServiceEntityRepository } /** + * Users holding an administrative role or awaiting approval for one, plus blocked + * accounts without a teamer. A user demoted by the CRM may keep no role that would list + * them, and a block is only ever lifted from a list: teamers are unblocked from theirs, + * everyone else has none but this one. + * * @return User[] */ public function getAdministrativeUsers(): array @@ -41,6 +46,11 @@ class UserRepository extends ServiceEntityRepository ; } + $qb->orWhere($qb->expr()->andX( + $qb->expr()->isNotNull('u.disabledAt'), + $qb->expr()->isNull('u.teamer'), + )); + return $qb ->orderBy('u.lastName', 'ASC') ->getQuery() diff --git a/src/Security/BpnAuthenticator.php b/src/Security/BpnAuthenticator.php index c31b331..985e92f 100644 --- a/src/Security/BpnAuthenticator.php +++ b/src/Security/BpnAuthenticator.php @@ -112,12 +112,16 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent ): ?User { // Fetch CRM attributes, early return in case of an API error try { - /** @var CrmAttributesResponse $crmAttributes */ $crmAttributes = $this->apiClient->getCrmAttributes($email, $password); } catch (ApiClientException $e) { return null; } + // BusPro answers with a notification record instead of the data on its own errors + if (false === $crmAttributes instanceof CrmAttributesResponse) { + return null; + } + // Flatten selected CRM attributes $crmSelections = $crmAttributes->toArray(); @@ -140,6 +144,20 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent // are no user of it: never create an account, block an existing one. Returning // the blocked user lets the UserChecker explain why the login was refused. if ([] === $claimedRoles) { + // A response without any attribute group carries no roles either, so it looks + // exactly like a revocation while it really means the CRM told us nothing: + // an empty payload, a changed schema, a misconfigured attribute id. Blocking + // on that would lock out every user logging in, so refuse this single login + // instead and leave the account alone. + if ([] === ($crmAttributes->getAttributeGroups() ?? [])) { + $this->logger->warning('Skip demotion: CRM attributes response carries no attribute groups', [ + 'user_id' => $user?->getId(), + 'user_email' => $email, + ]); + + return null; + } + if (null === $user) { return null; } diff --git a/src/Service/Cron/FeedbackReminderService.php b/src/Service/Cron/FeedbackReminderService.php index 7f35095..1e39bfe 100644 --- a/src/Service/Cron/FeedbackReminderService.php +++ b/src/Service/Cron/FeedbackReminderService.php @@ -59,7 +59,7 @@ class FeedbackReminderService $hotelManagers = $this ->userRepository - ->getUsersByRoleAndHotelCode('ROLE_HOTEL_MANAGER', $hotelBaseCodes) + ->getUsersByRoleAndHotelCode('ROLE_HOUSE_MANAGER', $hotelBaseCodes) ; if (0 === count($hotelManagers)) { diff --git a/templates/admin/system/user/_form.html.twig b/templates/admin/system/user/_form.html.twig index f5a0af5..8554824 100644 --- a/templates/admin/system/user/_form.html.twig +++ b/templates/admin/system/user/_form.html.twig @@ -5,6 +5,7 @@ {{ form_row(form.hotelCodes) }} {{ form_row(form.disabled) }} {{ form_row(form.disabledReason) }} + {{ form_row(form.disabledReasonInternal) }}