fix: drop the implicit ROLE_USER from the userinfo roles claim

src/BusProNet/Model/PersonalData.php

@@ -137,7 +137,8 @@ class PersonalData
     public function getClaims(): array
     {
         return [
-            'id' => $this->personId,
+            'person_id' => $this->personId,
+            'address_id' => $this->addressId,
             'email' => $this->communication->email,
             'roles' => $this->roles,
             'profile' => [

src/BusProNet/XmlParser/CrmAttributesResponseParser.php

@@ -29,7 +29,7 @@ class CrmAttributesResponseParser
 
         $result
             ->filterXPath('//selektionsmerkmale/selektionsgruppe')
-            ->each(function (Crawler $node) use (&$groups, &$roles) {
+            ->each(function (Crawler $node) use (&$groups, &$roles, &$hotelCodes) {
                 $group = new CrmSelectionGroup();
                 $group->id = (int) $node->attr('id');
                 $group->label = $node->attr('bezeichnung');

src/Controller/Admin/AccommodationBooking/EditController.php

@@ -71,7 +71,8 @@ class EditController extends AbstractController
             ));
         }
 
-        // ROLE_ADMIN inherits ROLE_GROUPS_ADMIN, so this single check covers both.
+        // ROLE_GROUPS_ADMIN is granted explicitly since 6c1073e4 removed it from ROLE_ADMIN's
+        // role_hierarchy — an administrator who needs this has to hold it in their own right.
         $assignableManagers = [];
         if ($this->isGranted('ROLE_GROUPS_ADMIN')) {
             $assignableManagers = $this->userRepo->findGroupsStaff();

src/Controller/Admin/AccommodationBooking/FilterController.php

@@ -34,7 +34,8 @@ class FilterController extends AbstractController
     #[Route('/admin/accommodation-booking/filter', name: 'app_admin_accommodationbooking_filter', methods: ['GET'])]
     public function filter(Request $request): Response
     {
-        // ROLE_ADMIN inherits ROLE_GROUPS_ADMIN, so this single check covers both.
+        // ROLE_GROUPS_ADMIN is granted explicitly since 6c1073e4 removed it from ROLE_ADMIN's
+        // role_hierarchy — an administrator who needs this has to hold it in their own right.
         $seesAllBookings = $this->isGranted('ROLE_GROUPS_ADMIN');
         $user = $this->getUser();
 

src/Controller/Admin/AccommodationBooking/IndexController.php

@@ -32,7 +32,8 @@ class IndexController extends AbstractController
     #[Route('/admin/accommodation-booking', name: 'app_admin_accommodationbooking')]
     public function index(Request $request): Response
     {
-        // ROLE_ADMIN inherits ROLE_GROUPS_ADMIN, so this single check covers both.
+        // ROLE_GROUPS_ADMIN is granted explicitly since 6c1073e4 removed it from ROLE_ADMIN's
+        // role_hierarchy — an administrator who needs this has to hold it in their own right.
         $seesAllBookings = $this->isGranted('ROLE_GROUPS_ADMIN');
         $user = $this->getUser();
         $user = $user instanceof User ? $user : null;

src/Controller/Admin/User/EditController.php

@@ -45,6 +45,7 @@ class EditController extends AbstractController
                 return $this->render('admin/user/modal_edit.html.twig', [
                     'user' => $user,
                     'form' => $form,
+                    'syncedRoles' => Role::syncedOnly($user->getRoles()),
                 ]);
             }
 
@@ -66,6 +67,7 @@ class EditController extends AbstractController
         return $this->render('admin/user/modal_edit.html.twig', [
             'user' => $user,
             'form' => $form,
+            'syncedRoles' => Role::syncedOnly($user->getRoles()),
         ]);
     }
 

src/Controller/Api/UserinfoController.php

@@ -8,6 +8,7 @@ use App\BusProNet\Model\Notification;
 use App\BusProNet\Model\PersonalData;
 use App\Entity\User;
 use App\Security\Crypt;
+use App\Security\Role;
 use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
 use Symfony\Component\HttpFoundation\JsonResponse;
 use Symfony\Component\HttpFoundation\Response;
@@ -30,7 +31,8 @@ class UserinfoController extends AbstractController
 
         // extend scopes depending on granted permissions
         if ($this->isGranted('ROLE_OAUTH2_ID')) {
-            $scopes[] = 'id';
+            $scopes[] = 'person_id';
+            $scopes[] = 'address_id';
         }
         if ($this->isGranted('ROLE_OAUTH2_PROFILE')) {
             $scopes[] = 'profile';
@@ -51,8 +53,9 @@ class UserinfoController extends AbstractController
                 return new JsonResponse(['message' => $data->message, 'code' => $data->code], Response::HTTP_BAD_REQUEST);
             }
 
-            // Patch current user's roles
-            $data->roles = $user->getRoles();
+            // 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());
 
             // Patch current user's hotel codes
             $data->hotelCodes = $user->getHotelCodes();

src/Form/Admin/UserType.php

@@ -12,10 +12,12 @@ use Symfony\Component\Form\FormBuilderInterface;
 use Symfony\Component\OptionsResolver\OptionsResolver;
 
 /**
- * Lets an administrator assign roles and hotel codes to an existing account.
+ * Lets an administrator assign the privileged roles and the hotel codes of an existing account.
  *
- * This is the only way those two get changed after the account was created — BpnAuthenticator
- * imports them once and never touches them again.
+ * Only Role::PRIVILEGED is offered: the remaining roles are synced from the BusPro CRM on every
+ * login (see BpnAuthenticator), so editing them here would be undone at the user's next login.
+ * Hotel codes are seeded once at account creation, which makes this form the only way to change
+ * them afterwards.
  *
  * @extends AbstractType<User>
  */
@@ -35,14 +37,16 @@ class UserType extends AbstractType
         $builder
             ->add('roles', ChoiceType::class, [
                 'label' => 'Rollen',
-                'choices' => array_flip(Role::labels()),
+                'choices' => $this->privilegedChoices(),
                 'multiple' => true,
                 'expanded' => true,
                 'required' => false,
-                // User::getRoles() prepends the implicit ROLE_USER, which must not be written back.
-                'getter' => static fn (User $user): array => array_values(array_diff($user->getRoles(), ['ROLE_USER'])),
+                '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(array_values(array_unique($roles)));
+                    $user->setRoles(Role::combine($user->getRoles(), $roles));
                 },
             ])
             ->add('hotelCodes', ChoiceType::class, [
@@ -58,6 +62,14 @@ class UserType extends AbstractType
         ;
     }
 
+    /**
+     * @return array<string, string> 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.

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,57 @@ final class Role
         return [] === $importable ? [self::CUSTOMER] : $importable;
     }
 
+    /**
+     * The roles actually assigned to an account — everything except the implicit ROLE_USER,
+     * which User::getRoles() prepends and which is never stored.
+     *
+     * @param string[] $roles
+     *
+     * @return string[]
+     */
+    public static function assignedOnly(array $roles): array
+    {
+        return array_values(array_diff($roles, [self::USER]));
+    }
+
+    /**
+     * 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(self::assignedOnly($roles), self::PRIVILEGED));
+    }
+
+    /**
+     * 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
      */

src/Twig/AppRuntime.php

@@ -141,11 +141,7 @@ class AppRuntime implements RuntimeExtensionInterface
 
         $mapped = [];
 
-        foreach ($roles as $role) {
-            if ('ROLE_USER' === $role) {
-                continue;
-            }
-
+        foreach (Role::assignedOnly($roles) as $role) {
             $mapped[] = $labels[$role] ?? $role;
         }
 

templates/admin/user/modal_edit.html.twig

@@ -9,6 +9,9 @@
     <div class="pb-4 text-sm text-gray-500">
         {{ user.email }}
     </div>
+    <div class="pb-4 text-sm text-gray-500">
+        aus BusPro: {{ syncedRoles | map_roles | join(', ') | default('–') }}
+    </div>
     {{ form_start(form) }}
     <div class="grid lg:grid-cols-2 gap-y-4 lg:gap-x-8">
         {{ form_row(form.roles) }}

tests/BusProNet/XmlParser/CrmAttributesResponseParserTest.php

@@ -4,6 +4,7 @@ declare(strict_types=1);
 
 namespace App\Tests\BusProNet\XmlParser;
 
+use App\BusProNet\Model\CrmAttributes;
 use App\BusProNet\XmlParser\CrmAttributesResponseParser;
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\DomCrawler\Crawler;
@@ -64,15 +65,54 @@ class CrmAttributesResponseParserTest extends TestCase
         self::assertContains('ROLE_TEAMER', $roles);
     }
 
+    public function testParseCollectsTheHotelCodesOfEveryHausleitungSelection(): void
+    {
+        $attributes = $this->parse($this->hausleitungXml());
+
+        self::assertSame(['ROLE_HOUSE_MANAGER'], $attributes->roles);
+        self::assertSame(['DKS', 'ASB'], $attributes->hotelCodes);
+    }
+
+    public function testParseAddsTheDefaultHotelCodeForAdmins(): void
+    {
+        $attributes = $this->parse($this->selectionXml(1292, true));
+
+        self::assertSame(['SSL'], $attributes->hotelCodes);
+    }
+
     /**
      * @return string[]
      */
     private function parseRoles(string $xmlContent): array
+    {
+        return $this->parse($xmlContent)->roles;
+    }
+
+    private function parse(string $xmlContent): CrmAttributes
     {
         $crawler = new Crawler($xmlContent);
         $resultNode = $crawler->filterXPath('//ergebnis');
 
-        return $this->parser->parse($resultNode)->roles;
+        return $this->parser->parse($resultNode);
+    }
+
+    private function hausleitungXml(): string
+    {
+        // Two groups, so the codes have to survive both closure levels of parse().
+        return '<?xml version="1.0" encoding="utf-8"?>
+<ergebnis>
+    <satz typ="KUNDENDATEN" />
+    <selektionsmerkmale>
+        <selektionsgruppe id="1" bezeichnung="Rollen">
+            <selektion id="2001" bezeichnung="Hausleitung DKS" aenderbar="0" auswahl="True" />
+            <selektion id="2002" bezeichnung="Hausleitung XYZ" aenderbar="0" auswahl="False" />
+        </selektionsgruppe>
+        <selektionsgruppe id="2" bezeichnung="Häuser">
+            <selektion id="2003" bezeichnung="Hausleitung ASB" aenderbar="0" auswahl="True" />
+        </selektionsgruppe>
+    </selektionsmerkmale>
+    <crmaktionen></crmaktionen>
+</ergebnis>';
     }
 
     private function selectionXml(int $id, bool $selected): string

tests/Form/Admin/UserTypeTest.php

@@ -13,29 +13,37 @@ use Symfony\Component\Form\Forms;
 
 class UserTypeTest extends TestCase
 {
-    public function testStoredRolesArePreselectedWithoutTheImplicitRoleUser(): void
+    public function testOnlyThePrivilegedRolesArePreselected(): void
     {
-        $user = (new User('[email protected]'))->setRoles([Role::TEAMER]);
+        $user = (new User('[email protected]'))->setRoles([Role::TEAMER, Role::GROUPS_MANAGER]);
 
-        self::assertSame([Role::TEAMER], $this->createForm($user)->get('roles')->getData());
+        self::assertSame([Role::GROUPS_MANAGER], $this->createForm($user)->get('roles')->getData());
     }
 
-    public function testSubmittingRolesDoesNotStoreTheImplicitRoleUser(): void
+    public function testOnlyPrivilegedRolesAreOffered(): void
+    {
+        $choices = $this->createForm(new User('[email protected]'))->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('[email protected]'))->setRoles([Role::TEAMER]);
 
         $form = $this->createForm($user);
-        $form->submit(['roles' => [Role::TEAMER, Role::GROUPS_MANAGER], 'hotelCodes' => []]);
+        $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 testClearingEveryCheckboxEmptiesTheAssignment(): void
+    public function testClearingEveryCheckboxKeepsTheSyncedRoles(): void
     {
         $user = (new User('[email protected]'))
-            ->setRoles([Role::TEAMER])
+            ->setRoles([Role::TEAMER, Role::GROUPS_MANAGER])
             ->setHotelCodes(['SSL'])
         ;
 
@@ -43,7 +51,7 @@ class UserTypeTest extends TestCase
         $form->submit([]);
 
         self::assertTrue($form->isSynchronized());
-        self::assertSame(['ROLE_USER'], $user->getRoles());
+        self::assertSame(['ROLE_USER', Role::TEAMER], $user->getRoles());
         self::assertSame([], $user->getHotelCodes());
     }
 

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,29 @@ class RoleTest extends TestCase
         self::assertSame([Role::CUSTOMER], Role::filterImportable([]));
     }
 
+    public function testAssignedOnlyDropsTheImplicitRoleUser(): 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));
+    }
+
+    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()));