From 7a8212749488166f8e0bb0543bbea50507e999c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Fromme?= Date: Wed, 12 Aug 2026 17:42:20 +0200 Subject: [PATCH] feat: sync non-privileged BusPro roles on every login --- src/Security/BpnAuthenticator.php | 32 +++++++++++++----- src/Security/Role.php | 44 +++++++++++++++++++++++++ tests/Security/BpnAuthenticatorTest.php | 37 +++++++++++++++++++-- tests/Security/RoleTest.php | 14 ++++++++ 4 files changed, 116 insertions(+), 11 deletions(-) diff --git a/src/Security/BpnAuthenticator.php b/src/Security/BpnAuthenticator.php index 2207705..a4a5997 100644 --- a/src/Security/BpnAuthenticator.php +++ b/src/Security/BpnAuthenticator.php @@ -34,10 +34,13 @@ 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. * - * CRM attributes (roles, hotel codes) seed a *new* account only: BusPro backend users can - * edit their own CRM selections, so taking roles over on every login would let anybody grant - * themselves Role::PRIVILEGED here. Privileged roles are never imported at all, and from the - * second login on both roles and hotel codes are managed by an administrator in /admin/user. + * 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. */ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements AuthenticationEntryPointInterface { @@ -106,15 +109,13 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent if (null === $user = $userRepository->findOneBy(['email' => $email])) { $user = new User($email); - $user - ->setRoles($this->importableRoles($email, $crmAttributes->roles)) - ->setHotelCodes(array_values(array_unique($crmAttributes->hotelCodes))) - ; + $user->setHotelCodes(array_values(array_unique($crmAttributes->hotelCodes))); $this->entityManager->persist($user); } $user + ->setRoles($this->syncedRoles($email, $user->getRoles(), $crmAttributes->roles)) ->setPassword($encryptedPassword) ->setPersonId($personalData->personId) ->setAddressId($personalData->addressId) @@ -127,6 +128,21 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent return $user; } + /** + * 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[] + */ + private function syncedRoles(string $email, array $storedRoles, array $crmRoles): array + { + return Role::combine($this->importableRoles($email, $crmRoles), $storedRoles); + } + /** * @param string[] $crmRoles * diff --git a/src/Security/Role.php b/src/Security/Role.php index 4d0b3c7..836cbc9 100644 --- a/src/Security/Role.php +++ b/src/Security/Role.php @@ -13,6 +13,12 @@ namespace App\Security; */ final class Role { + /** + * The role every account holds implicitly. User::getRoles() prepends it and it is never + * stored, so it has to be filtered out wherever a role set is written back. + */ + public const USER = 'ROLE_USER'; + public const ADMIN = 'ROLE_ADMIN'; public const MANAGER = 'ROLE_MANAGER'; public const TEAMER = 'ROLE_TEAMER'; @@ -66,6 +72,44 @@ final class Role return [] === $importable ? [self::CUSTOMER] : $importable; } + /** + * The non-privileged half of a role set: what BpnAuthenticator syncs from the BusPro CRM. + * + * @param string[] $roles + * + * @return string[] + */ + public static function syncedOnly(array $roles): array + { + return array_values(array_diff($roles, self::PRIVILEGED, [self::USER])); + } + + /** + * The privileged half: what an administrator granted in /admin/user. + * + * @param string[] $roles + * + * @return string[] + */ + public static function privilegedOnly(array $roles): array + { + return array_values(array_intersect($roles, self::PRIVILEGED)); + } + + /** + * 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. + * + * @param string[] $synced + * @param string[] $privileged + * + * @return string[] + */ + public static function combine(array $synced, array $privileged): array + { + return array_values(array_unique([...self::syncedOnly($synced), ...self::privilegedOnly($privileged)])); + } + /** * @return array role => label */ diff --git a/tests/Security/BpnAuthenticatorTest.php b/tests/Security/BpnAuthenticatorTest.php index bed40bf..55638c5 100644 --- a/tests/Security/BpnAuthenticatorTest.php +++ b/tests/Security/BpnAuthenticatorTest.php @@ -42,7 +42,7 @@ class BpnAuthenticatorTest extends TestCase self::assertSame(['SSL'], $user->getHotelCodes()); } - public function testExistingAccountKeepsTheRolesAnAdministratorAssigned(): void + public function testExistingAccountKeepsThePrivilegedRolesAnAdministratorAssigned(): void { $existing = (new User('teamer@example.org')) ->setRoles([Role::TEAMER, Role::GROUPS_MANAGER]) @@ -59,11 +59,42 @@ class BpnAuthenticatorTest extends TestCase $user = $this->loadUser($authenticator); self::assertNull($persisted, 'an existing account must not be persisted again'); - self::assertSame(['ROLE_USER', Role::TEAMER, Role::GROUPS_MANAGER], $user->getRoles()); - self::assertSame(['DKS'], $user->getHotelCodes()); + // 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::assertNotNull($user->getLastLoginAt(), 'the rest of the profile is still synced'); } + public function testRoleGainedInBusProIsGrantedOnLogin(): void + { + $existing = (new User('teamer@example.org'))->setRoles([Role::CUSTOMER]); + + $persisted = null; + $authenticator = $this->authenticator( + $this->crmAttributes([Role::TEAMER], []), + $existing, + $persisted, + ); + + // The case myep-team depends on: somebody becomes a Teamer after their account exists. + self::assertSame(['ROLE_USER', Role::TEAMER], $this->loadUser($authenticator)->getRoles()); + } + + public function testAccountWithoutAnyRoleIsHealedOnLogin(): void + { + $existing = new User('teamer@example.org'); + + $persisted = null; + $authenticator = $this->authenticator( + $this->crmAttributes([Role::TEAMER], []), + $existing, + $persisted, + ); + + self::assertSame(['ROLE_USER', Role::TEAMER], $this->loadUser($authenticator)->getRoles()); + } + /** * @param string[] $roles * @param string[] $hotelCodes diff --git a/tests/Security/RoleTest.php b/tests/Security/RoleTest.php index 83ef168..a38b508 100644 --- a/tests/Security/RoleTest.php +++ b/tests/Security/RoleTest.php @@ -38,6 +38,20 @@ class RoleTest extends TestCase self::assertSame([Role::CUSTOMER], Role::filterImportable([])); } + public function testCombineKeepsEachHalfInItsOwnLane(): 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)); + } + + public function testCombineDropsTheImplicitRoleUser(): void + { + self::assertSame([Role::TEAMER], Role::combine([Role::USER, Role::TEAMER], [])); + } + public function testEveryRoleHasALabel(): void { self::assertSame(Role::ALL, array_keys(Role::labels()));