diff --git a/config/packages/messenger.yaml b/config/packages/messenger.yaml index 215cce7..813656c 100644 --- a/config/packages/messenger.yaml +++ b/config/packages/messenger.yaml @@ -26,6 +26,7 @@ framework: Symfony\Component\Notifier\Message\SmsMessage: async App\Message\MailjetNewsletterEventMessage: async App\Message\CreateClickUpBookingTaskMessage: async + App\Message\RoleNominationMessage: async # Route your messages to the transports # 'App\Message\YourMessage': async diff --git a/config/packages/routing.yaml b/config/packages/routing.yaml index 4b766ce..86911d0 100644 --- a/config/packages/routing.yaml +++ b/config/packages/routing.yaml @@ -2,9 +2,12 @@ framework: router: utf8: true - # Configure how to generate URLs in non-HTTP contexts, such as CLI commands. - # See https://symfony.com/doc/current/routing.html#generating-urls-in-commands - #default_uri: http://localhost + # How URLs are generated where there is no request to take the host from: scheduled + # commands and, notably, the Messenger workers that send mail off the request path. + # Without it those links would point at http://localhost. Only ever consulted in a + # non-HTTP context, so web requests keep using their own host — which matters here, + # since one codebase serves four brands. + default_uri: '%env(APP_BASE_URL)%' when@prod: framework: diff --git a/src/Dashboard/Widget/PendingRoleApprovalsWidgetProvider.php b/src/Dashboard/Widget/PendingRoleApprovalsWidgetProvider.php index 6d4e7cd..6555523 100644 --- a/src/Dashboard/Widget/PendingRoleApprovalsWidgetProvider.php +++ b/src/Dashboard/Widget/PendingRoleApprovalsWidgetProvider.php @@ -6,19 +6,20 @@ namespace App\Dashboard\Widget; use App\Dashboard\Contract\DashboardWidgetProviderInterface; use App\Entity\User; -use App\Form\Model\Filter\AbstractListFilterDto; use App\Model\DashboardWidget; use App\Model\DashboardWidgetEntry; use App\Repository\UserRepository; use App\Security\Role; +use App\Service\RoleApprovalUrlGenerator; use Symfony\Component\Routing\Generator\UrlGeneratorInterface; /** * The accounts waiting on an administrator to act. * - * A nomination grants nothing until it is approved, and nothing else in the application - * announces that one is waiting — without this widget an administrator only finds out by - * opening the user list. + * A nomination grants nothing until it is approved. RoleNominationHandler emails the + * administrators the moment one appears, but that fires once, on the login that produced it — + * this widget is the standing list, and the only thing that still shows a nomination somebody + * has left sitting. */ class PendingRoleApprovalsWidgetProvider implements DashboardWidgetProviderInterface { @@ -27,6 +28,7 @@ class PendingRoleApprovalsWidgetProvider implements DashboardWidgetProviderInter public function __construct( private readonly UserRepository $userRepository, private readonly UrlGeneratorInterface $urlGenerator, + private readonly RoleApprovalUrlGenerator $approvalUrlGenerator, ) { } @@ -46,7 +48,7 @@ class PendingRoleApprovalsWidgetProvider implements DashboardWidgetProviderInter 'Offene Rollenfreigaben', array_map(fn (User $user): DashboardWidgetEntry => new DashboardWidgetEntry( $this->label($user), - $this->approvalUrl($user), + $this->approvalUrlGenerator->forUser($user), 'user', ), $this->userRepository->findWithPendingRoles(self::LIMIT)), 'Keine offenen Rollenfreigaben.', @@ -65,20 +67,4 @@ class PendingRoleApprovalsWidgetProvider implements DashboardWidgetProviderInter return sprintf('%s: %s', $user->getDisplayName(), implode(', ', $nominated)); } - - /** - * The user list, filtered down to this account. - * - * Approving happens in a modal that app_admin_user_permissions renders as a bare fragment, - * so it cannot be linked to directly. The filtered list is one click away from it and shows - * the nomination badge on the way. The query string is flat because the list filter forms - * declare no block prefix. - */ - private function approvalUrl(User $user): string - { - return $this->urlGenerator->generate('app_admin_user', [ - AbstractListFilterDto::MARKER => 1, - 'q' => $user->getEmail(), - ]); - } } diff --git a/src/Message/RoleNominationMessage.php b/src/Message/RoleNominationMessage.php new file mode 100644 index 0000000..cd9e5ff --- /dev/null +++ b/src/Message/RoleNominationMessage.php @@ -0,0 +1,25 @@ +userRepository->find($message->userId); + + if (null === $user) { + // The account is gone — a retry cannot bring it back. + $this->authLogger->warning('Nominated user not found, skipping the role nomination email', [ + 'userId' => $message->userId, + ]); + + return; + } + + $recipients = array_values(array_filter(array_map( + static fn ($admin): ?string => $admin->getEmail(), + $this->userRepository->findAdministrators(), + ))); + + if ([] === $recipients) { + // Worth a warning rather than a silent return: nobody can approve the nomination, and + // without this line nobody would find out that the notification goes nowhere. + $this->authLogger->warning('No administrator to notify about a role nomination', [ + 'userId' => $message->userId, + 'roles' => $message->roles, + ]); + + return; + } + + $labels = Role::labels(); + + $this->mailer->createAndSendEmail( + [ + 'user' => $user, + 'roles' => array_values(array_map( + static fn (string $role): string => $labels[$role] ?? $role, + $message->roles, + )), + // Absolute: generated in a worker, where there is no request to borrow a host + // from (see framework.router.default_uri). + 'approvalUrl' => $this->approvalUrlGenerator->forUser($user, UrlGeneratorInterface::ABSOLUTE_URL), + ], + [ + 'template' => 'email/role_nomination.html.twig', + 'subject' => 'Neue Rollen-Freischaltung angefordert', + 'to' => $recipients, + ], + ); + } +} diff --git a/src/Repository/UserRepository.php b/src/Repository/UserRepository.php index 2d4ed57..2bdfb3a 100644 --- a/src/Repository/UserRepository.php +++ b/src/Repository/UserRepository.php @@ -71,6 +71,32 @@ class UserRepository extends ServiceEntityRepository ->getResult(); } + /** + * The administrators who can approve a role nomination. + * + * The quote-anchored needle is load-bearing. ROLE_ADMIN_PENDING lives in the same JSON column, + * and an unanchored '%ROLE_ADMIN%' would match it — which would mail the very people whose own + * nomination is unapproved about other people's nominations. The result is filtered through + * Role::effectiveOnly() as well, so the guarantee does not rest on the LIKE alone. + * + * @return User[] + */ + public function findAdministrators(): array + { + /** @var User[] $candidates */ + $candidates = $this->createQueryBuilder('u') + ->andWhere('u.roles LIKE :admin') + ->setParameter('admin', '%"'.Role::ADMIN.'"%') + ->orderBy('u.email', 'ASC') + ->getQuery() + ->getResult(); + + return array_values(array_filter( + $candidates, + static fn (User $user): bool => \in_array(Role::ADMIN, Role::effectiveOnly($user->getRoles()), true), + )); + } + /** * Everyone worth filtering an accommodation booking by: current groups staff, plus whoever * a booking is still assigned to even after losing the role — otherwise a booking assigned diff --git a/src/Security/BpnAuthenticator.php b/src/Security/BpnAuthenticator.php index 4657114..e09bde9 100644 --- a/src/Security/BpnAuthenticator.php +++ b/src/Security/BpnAuthenticator.php @@ -12,12 +12,14 @@ use App\BusProNet\Model\CrmAttributes; use App\BusProNet\Model\PersonalData; use App\Entity\User; use App\Htmx\HxRedirectResponse; +use App\Message\RoleNominationMessage; use App\Service\ProfileCompletenessChecker; use Doctrine\ORM\EntityManagerInterface; use Psr\Log\LoggerInterface; use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; +use Symfony\Component\Messenger\MessageBusInterface; use Symfony\Component\Routing\Generator\UrlGeneratorInterface; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; use Symfony\Component\Security\Core\Exception\CustomUserMessageAuthenticationException; @@ -54,6 +56,7 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent private readonly ProfileCompletenessChecker $completenessChecker, private readonly LoggerInterface $authLogger, private readonly EmployeeDomainMatcher $employeeDomainMatcher, + private readonly MessageBusInterface $messageBus, ) { } @@ -120,7 +123,7 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent ->setProfileComplete($this->completenessChecker->isComplete($personalData)) ; - $this->syncFromCrm($user, $crmAttributes); + $nominated = $this->syncFromCrm($user, $crmAttributes); // Registered only once it is fully populated: syncFromCrm() logs on a channel that writes // to the database, and an account already managed at that point would be flushed @@ -129,14 +132,24 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent $this->entityManager->persist($user); $this->entityManager->flush(); + // After the flush, deliberately: a first login has no id before it, and the transport is + // Doctrine-backed, so a message queued ahead of a failing flush would announce a + // nomination that was never stored. + if ([] !== $nominated) { + $this->messageBus->dispatch(new RoleNominationMessage((int) $user->getId(), $nominated)); + } + return $user; } /** * 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. + * + * @return string[] the roles this login newly nominated the account for — the roles + * themselves, not their markers, and empty whenever nothing changed */ - private function syncFromCrm(User $user, CrmAttributes $crmAttributes): void + private function syncFromCrm(User $user, CrmAttributes $crmAttributes): array { $previousRoles = $user->getRoles(); @@ -151,7 +164,7 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent // 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 []; } } @@ -169,16 +182,25 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent ->setHotelCodes(array_values(array_unique($crmAttributes->hotelCodes))) ; - $nominated = array_diff(Role::pendingOnly($user->getRoles()), Role::pendingOnly($previousRoles)); + $nominated = array_values(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), - ]); + if ([] === $nominated) { + return []; } + + // 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' => $nominated, + ]); + + // Only the newly appeared markers reach this point, so a repeat login with a nomination + // still standing announces nothing. That difference is the whole de-duplication. + return array_keys(Role::nominatedFrom($nominated)); } public function onAuthenticationSuccess(Request $request, TokenInterface $token, string $firewallName): ?Response diff --git a/src/Service/RoleApprovalUrlGenerator.php b/src/Service/RoleApprovalUrlGenerator.php new file mode 100644 index 0000000..126c96a --- /dev/null +++ b/src/Service/RoleApprovalUrlGenerator.php @@ -0,0 +1,41 @@ + with no page chrome, meant to be swapped + * into a page that is already open. Following it as a normal navigation, which is what a link + * in an email or a dashboard does, yields an unstyled fragment. + * + * The user list filtered down to the one account is one click away from the modal and shows the + * nomination badge on the way. The query string is flat because the list filter forms declare no + * block prefix. + */ +class RoleApprovalUrlGenerator +{ + public function __construct( + private readonly UrlGeneratorInterface $urlGenerator, + ) { + } + + public function forUser(User $user, int $referenceType = UrlGeneratorInterface::ABSOLUTE_PATH): string + { + return $this->urlGenerator->generate( + 'app_admin_user', + [ + AbstractListFilterDto::MARKER => 1, + 'q' => $user->getEmail(), + ], + $referenceType, + ); + } +} diff --git a/templates/email/role_nomination.html.twig b/templates/email/role_nomination.html.twig new file mode 100644 index 0000000..639a27f --- /dev/null +++ b/templates/email/role_nomination.html.twig @@ -0,0 +1,33 @@ +{% extends 'email/layout.html.twig' %} + +{% block body %} +

+ Neue Rollen-Freischaltung +

+

+ Für ein MyE&P-Konto wurde im BusPro-CRM eine administrative Rolle hinterlegt. Sie muss + freigeschaltet werden, bevor sie gilt. +

+

+ {{ user.displayName }} +
+ {{ user.email }} +

+

+ Beantragt {{ roles|length > 1 ? 'wurden folgende Rollen' : 'wurde folgende Rolle' }}: + {% for role in roles %} +
+ - {{ role }} + {% endfor %} +

+

+ + Freischaltung prüfen + +

+

+ Solange die Freischaltung aussteht, hat die Rolle keinerlei Wirkung — das Konto kann damit + nichts tun. Möchtest du sie nicht erteilen, ignoriere diese E-Mail einfach. Die Rolle wird + erst entfernt, wenn die Auswahl im CRM entfernt wird. +

+{% endblock %} diff --git a/tests/Dashboard/PendingRoleApprovalsWidgetProviderTest.php b/tests/Dashboard/PendingRoleApprovalsWidgetProviderTest.php index 596aa22..ab37df6 100644 --- a/tests/Dashboard/PendingRoleApprovalsWidgetProviderTest.php +++ b/tests/Dashboard/PendingRoleApprovalsWidgetProviderTest.php @@ -8,6 +8,7 @@ use App\Dashboard\Widget\PendingRoleApprovalsWidgetProvider; use App\Entity\User; use App\Repository\UserRepository; use App\Security\Role; +use App\Service\RoleApprovalUrlGenerator; use PHPUnit\Framework\TestCase; use Symfony\Component\Routing\Generator\UrlGeneratorInterface; @@ -76,6 +77,10 @@ class PendingRoleApprovalsWidgetProviderTest extends TestCase }, ); - return new PendingRoleApprovalsWidgetProvider($repository, $urlGenerator); + return new PendingRoleApprovalsWidgetProvider( + $repository, + $urlGenerator, + new RoleApprovalUrlGenerator($urlGenerator), + ); } } diff --git a/tests/MessageHandler/RoleNominationHandlerTest.php b/tests/MessageHandler/RoleNominationHandlerTest.php new file mode 100644 index 0000000..ba2be39 --- /dev/null +++ b/tests/MessageHandler/RoleNominationHandlerTest.php @@ -0,0 +1,128 @@ +|null */ + private ?array $sentOptions = null; + + /** @var array|null */ + private ?array $sentContext = null; + + private ?string $generatedRoute = null; + + private ?int $generatedReferenceType = null; + + public function testMailsEveryAdministrator(): void + { + $handler = $this->handler( + $this->user(7, 'nominee@example.org'), + [ + $this->user(1, 'first@ep-reisen.de', [Role::ADMIN]), + $this->user(2, 'second@ep-reisen.de', [Role::ADMIN]), + ], + ); + + $handler(new RoleNominationMessage(7, [Role::ADMIN])); + + self::assertSame(['first@ep-reisen.de', 'second@ep-reisen.de'], $this->sentOptions['to']); + self::assertSame('email/role_nomination.html.twig', $this->sentOptions['template']); + // Labelled for a human, not the raw role string. + self::assertSame(['Administration'], $this->sentContext['roles']); + // The filtered user list, not app_admin_user_permissions: that route renders a bare htmx + // fragment, which a mail client following the link would show unstyled. + self::assertSame('app_admin_user', $this->generatedRoute); + // Absolute, because a worker has no request to borrow a host from. + self::assertSame(UrlGeneratorInterface::ABSOLUTE_URL, $this->generatedReferenceType); + self::assertSame('https://my.ep-reisen.de/generated', $this->sentContext['approvalUrl']); + } + + public function testAMissingAccountIsANoOp(): void + { + $handler = $this->handler(null, [$this->user(1, 'admin@ep-reisen.de', [Role::ADMIN])]); + + $handler(new RoleNominationMessage(7, [Role::ADMIN])); + + self::assertNull($this->sentOptions); + } + + public function testWithoutAnAdministratorNothingIsSent(): void + { + $handler = $this->handler($this->user(7, 'nominee@example.org'), []); + + $handler(new RoleNominationMessage(7, [Role::ADMIN])); + + self::assertNull($this->sentOptions); + } + + /** + * @param string[] $roles + */ + private function user(int $id, string $email, array $roles = []): User + { + $user = (new User($email))->setRoles($roles); + + // The id is generated by Doctrine and has no setter, but the notification links to it. + $property = new \ReflectionProperty(User::class, 'id'); + $property->setValue($user, $id); + + return $user; + } + + /** + * @param User[] $administrators + */ + private function handler(?User $nominee, array $administrators): RoleNominationHandler + { + $userRepository = $this->createStub(UserRepository::class); + $userRepository->method('find')->willReturn($nominee); + $userRepository->method('findAdministrators')->willReturn($administrators); + + $mailer = $this->createStub(Mailer::class); + $mailer + ->method('createAndSendEmail') + ->willReturnCallback(function (array $context, array $options): void { + $this->sentContext = $context; + $this->sentOptions = $options; + }) + ; + + // Records what was asked of the router rather than imitating its output: what matters is + // which route the mail points at and that it is absolute, not how a query string is escaped. + $urlGenerator = $this->createStub(UrlGeneratorInterface::class); + $urlGenerator + ->method('generate') + ->willReturnCallback(function (string $route, array $parameters, int $referenceType): string { + $this->generatedRoute = $route; + $this->generatedReferenceType = $referenceType; + + return 'https://my.ep-reisen.de/generated'; + }) + ; + + return new RoleNominationHandler( + $userRepository, + $mailer, + new RoleApprovalUrlGenerator($urlGenerator), + $this->createStub(LoggerInterface::class), + ); + } +} diff --git a/tests/Security/BpnAuthenticatorTest.php b/tests/Security/BpnAuthenticatorTest.php index 202d6b6..63fa43b 100644 --- a/tests/Security/BpnAuthenticatorTest.php +++ b/tests/Security/BpnAuthenticatorTest.php @@ -9,6 +9,7 @@ use App\BusProNet\Model\CrmAttributes; use App\BusProNet\Model\CrmSelectionGroup; use App\BusProNet\Model\PersonalData; use App\Entity\User; +use App\Message\RoleNominationMessage; use App\Security\BpnAuthenticator; use App\Security\Crypt; use App\Security\EmployeeDomainMatcher; @@ -19,6 +20,8 @@ use Doctrine\ORM\EntityRepository; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\Messenger\Envelope; +use Symfony\Component\Messenger\MessageBusInterface; use Symfony\Component\Routing\Generator\UrlGeneratorInterface; use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; @@ -28,6 +31,14 @@ use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; */ class BpnAuthenticatorTest extends TestCase { + /** @var object[] messages the authenticator dispatched during the login under test */ + private array $dispatched = []; + + protected function setUp(): void + { + $this->dispatched = []; + } + public function testNewAccountIsSeededFromTheCrm(): void { $persisted = null; @@ -229,6 +240,54 @@ class BpnAuthenticatorTest extends TestCase self::assertSame(['ROLE_USER', Role::CUSTOMER], $user->getRoles()); } + public function testANewNominationIsAnnouncedOnce(): void + { + $persisted = null; + $authenticator = $this->authenticator($this->crmAttributes([Role::ADMIN], []), null, $persisted); + + $user = $this->loadUser($authenticator); + + self::assertCount(1, $this->dispatched); + $message = $this->dispatched[0]; + self::assertInstanceOf(RoleNominationMessage::class, $message); + // The role itself, not its marker: the marker is an internal bookkeeping detail. + self::assertSame([Role::ADMIN], $message->roles); + self::assertContains(Role::pending(Role::ADMIN), $user->getRoles()); + } + + public function testAStandingNominationIsNotAnnouncedAgain(): void + { + $existing = (new User('teamer@example.org'))->setRoles([Role::pending(Role::ADMIN)]); + $persisted = null; + $authenticator = $this->authenticator($this->crmAttributes([Role::ADMIN], []), $existing, $persisted); + + $this->loadUser($authenticator); + + // The nomination has not changed, so there is nothing new to tell an administrator about. + self::assertSame([], $this->dispatched); + } + + public function testAnApprovedRoleIsNotAnnouncedAsANomination(): void + { + $existing = (new User('teamer@example.org'))->setRoles([Role::ADMIN]); + $persisted = null; + $authenticator = $this->authenticator($this->crmAttributes([Role::ADMIN], []), $existing, $persisted); + + $this->loadUser($authenticator); + + self::assertSame([], $this->dispatched); + } + + public function testALoginWithoutANominationAnnouncesNothing(): void + { + $persisted = null; + $authenticator = $this->authenticator($this->crmAttributes([Role::TEAMER], []), null, $persisted); + + $this->loadUser($authenticator); + + self::assertSame([], $this->dispatched); + } + private function authenticator( CrmAttributes $crmAttributes, ?User $existing, @@ -264,6 +323,16 @@ class BpnAuthenticatorTest extends TestCase $completenessChecker = $this->createStub(ProfileCompletenessChecker::class); $completenessChecker->method('isComplete')->willReturn(true); + $messageBus = $this->createStub(MessageBusInterface::class); + $messageBus + ->method('dispatch') + ->willReturnCallback(function (object $message): Envelope { + $this->dispatched[] = $message; + + return new Envelope($message); + }) + ; + return new BpnAuthenticator( $this->createStub(UrlGeneratorInterface::class), $apiClient, @@ -272,6 +341,7 @@ class BpnAuthenticatorTest extends TestCase $completenessChecker, $this->createStub(LoggerInterface::class), new EmployeeDomainMatcher(['ep-reisen.de']), + $messageBus, ); }