feat: sync non-privileged BusPro roles on every login

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
      *

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<string, string> role => label
      */

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('[email protected]'))
             ->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('[email protected]'))->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('[email protected]');
+
+        $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

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()));