feat: bpn as single source of truth for role and hotel code assignments

docs/user-roles.md

@@ -3,11 +3,23 @@
 Reference for how a user account comes into existence, how it gains and loses roles, and
 how it gets blocked. Describes the behaviour as implemented — not a plan.
 
-The governing rule, from which most of the rest follows:
+The governing rules, from which the rest follows:
 
-> **The BusPro CRM may nominate, but never grant, an administrative role.** Only a super
+> **BusPro is the source of truth for roles and hotel codes.** Both are synced on every
-> admin turns a nomination into a privilege. The CRM's word alone is enough to *take away*
+> login, so anything it no longer reports is withdrawn.
-> access, never to hand it out.
+>
+> **The CRM may nominate, but never grant, an administrative role.** Only a super admin
+> turns a nomination into a privilege. The CRM's word alone is enough to take away access,
+> never to hand it out - and an administrator's word alone is enough for neither.
+
+Two corollaries that surprise people:
+
+- **A role cannot be handed out by hand.** The edit form approves what BusPro claims; it
+  cannot add a role BusPro is silent about, nor remove one it reports. Same for hotel codes,
+  which are display-only there.
+- **Every role stands on its own.** `ROLE_TEAMER` is not a base role others build on, and a
+  Reisemanager is not a superset of a Hausleitung - somebody claiming both is nominated for
+  both, and approved for each separately.
 
 ---
 
@@ -16,12 +28,12 @@ The governing rule, from which most of the rest follows:
 Defined in `User::ROLES` (`src/Entity/User.php`), these four are the only roles a human can
 assign:
 
-| Role | Label | Granted by | Hierarchy |
+| Role | Label | Granted by | Revoked by | Hierarchy |
-|------|-------|-----------|-----------|
+|------|-------|-----------|------------|-----------|
-| `ROLE_ADMIN` | Admin | super admin, manually | ⇒ `ROLE_ADMINISTRATIVE` |
+| `ROLE_ADMIN` | Admin | super admin, approving a CRM claim | the CRM, automatically | ⇒ `ROLE_ADMINISTRATIVE` |
-| `ROLE_MANAGER` | Reisemanager | super admin, manually | ⇒ `ROLE_ADMINISTRATIVE` |
+| `ROLE_MANAGER` | Reisemanager | super admin, approving a CRM claim | the CRM, automatically | ⇒ `ROLE_ADMINISTRATIVE` |
-| `ROLE_HOUSE_MANAGER` | Hausleitung | super admin, manually | — |
+| `ROLE_HOUSE_MANAGER` | Hausleitung | super admin, approving a CRM claim | the CRM, automatically | — |
-| `ROLE_TEAMER` | Teamer | the CRM, automatically | — |
+| `ROLE_TEAMER` | Teamer | the CRM, automatically | the CRM, automatically | — |
 
 `User::PENDING_ROLES` holds a marker for each of the three administrative roles, keyed by
 the role it stands for:
@@ -40,7 +52,9 @@ put the user on the approval list). `ROLE_TEAMER` has no marker: it needs no app
 Two further roles are synthesized by `User::getRoles()` and never stored: `ROLE_USER` for
 everybody, and `ROLE_SUPER_ADMIN` when the separate `superAdmin` boolean column is set. A
 validation callback (`User::validateSuperAdmin()`) refuses `superAdmin` without
-`ROLE_ADMIN` alongside it — super admin is an elevation, never a standalone grant.
+`ROLE_ADMIN` alongside it — super admin is an elevation, never a standalone grant. The sync
+enforces the same rule from the other side: revoking `ROLE_ADMIN` clears the flag, or the one
+role that outranks every check in the application would outlive the role it depends on.
 
 ### Storage and accessors
 
@@ -50,8 +64,9 @@ slice it, and picking the right one matters:
 | Accessor | Returns |
 |----------|---------|
 | `getRoles()` | the column **plus** synthesized `ROLE_USER` / `ROLE_SUPER_ADMIN` — what Symfony authorises against |
-| `getAssignedRoles()` | only the four real roles from the column — what the edit form binds to |
+| `getAssignedRoles()` | only the four real roles from the column — what the sync works on |
 | `getPendingRoles()` | only the markers |
+| `getNominatedRoles()` | the roles behind those markers, as `role => label` — what an approver acts on |
 
 ---
 
@@ -71,9 +86,11 @@ slice it, and picking the right one matters:
 code. Entries whose code is not a house in the `houses` parameter are kept commented out:
 such a house manager could log in but would see no assignments and no dispositions at all.
 
-`UserDataHandler::collectPendingRoles()` turns those into markers — `isManager` wins over
+`UserDataHandler::collectClaimedRoles()` turns those four booleans into plain role names -
-`isHouseManager`, they are never both claimed, though the hotel codes of a Hausleitung are
+what the CRM says. Two things are made of that list: `toPendingRoles()` produces one marker
-imported eitherway. `collectRoles()` is that set plus a real `ROLE_TEAMER` when `isTeamer`.
+per claimed administrative role (the roles are independent, so a Reisemanager who is also a
+Hausleitung gets both), and `collectRoles()` produces the markers plus a real `ROLE_TEAMER`,
+which is what a brand new account starts with.
 
 ### The shape of a BusPro response (important)
 
@@ -100,6 +117,14 @@ roles were revoked.
 >
 > Matching is by id and never by label, so `Preisrechner Admin` does not trip the admin flag.
 >
+> `bpn_crm_house_manager_ids` (`config/services.yaml`) is deployment-critical for the same
+> reason, and more sharply so: since roles are synced, an id missing from that map does not
+> merely fail to nominate a Hausleitung, it **revokes** the role from everyone holding it, one
+> login at a time, with re-approval manual per user. Entries whose hotel code is not a house
+> in the `houses` parameter are deliberately commented out there — a person holding only such
+> a Hausleitung claims nothing at all and is blocked (see 4) rather than left with a role that
+> shows them no data.
+>
 > `APP_BPN_DEFAULT_HOTEL_CODE` is a testing affordance: when set, **every** admin also
 > becomes a house manager for that hotel. It must stay empty outside local development.
 
@@ -107,7 +132,7 @@ roles were revoked.
 
 ## Lifecycle
 
-### 1. First login — the only automatic grant of anything
+### 1. First login
 
 `BpnAuthenticator::getOrCreateLocalUser()` → `UserDataHandler::createLocalUser()` writes
 `collectRoles()` verbatim, together with the hotel codes from the Hausleitung attributes.
@@ -116,40 +141,70 @@ A CRM admin who is not also a teamer therefore starts with `['ROLE_ADMIN_PENDING
 privileges at all: they can authenticate, but `UserChecker` refuses the session until a
 super admin approves them.
 
-### 2. Every subsequent login
+### 2. Every subsequent login — the sync
 
-`UserDataHandler::updateLocalUser()` refreshes name, email and the teamer record, and then:
+`UserDataHandler::updateLocalUser()` refreshes name, email and the teamer record, and hands
+the roles to **`syncRoles()`**, which is the whole policy in four steps:
 
-- **`refreshPendingRoles()`** recomputes the marker set from the current CRM claims. A
+1. **revoke** every granted role the CRM no longer claims. This is what makes BusPro the
-  marker whose real role is already granted is dropped — an approved role is never marked
+   source of truth, and it applies to `ROLE_TEAMER` as much as to the administrative roles.
-  again. Granted roles are kept untouched.
+2. **clear the super admin flag** when `ROLE_ADMIN` was among them — `ROLE_SUPER_ADMIN` is
-- **`grantTeamerRole()`** adds `ROLE_TEAMER` if the CRM reports a teamer and the user does
+   synthesized from a separate column and would otherwise survive its own precondition.
-  not have it yet. This is deliberately *grant-only*: the role is never withdrawn here,
+3. **`refreshPendingRoles()`** recomputes the marker set from the current claims. A marker
-  because it may have been handed out manually and must survive a login. Losing the CRM
+   whose real role is already granted is dropped — an approved role is never marked again.
-  teamer attribute while holding no other role blocks the account anyway (see 4).
+   It runs *after* the revocation, so a role just revoked is not immediately marked again.
+4. **`grantTeamerRole()`** adds `ROLE_TEAMER` when the CRM claims it. Grant-only in itself;
+   withdrawing it is step 1's business.
 
-Administrative roles and hotel codes are imported at creation and are managed by hand
+**`syncHotelCodes()`** then replaces the hotel codes with the ones the CRM reports, so a
-afterwards. Nothing on this path can raise a privilege.
+Hausleitung who moves house is not left seeing the old one.
+
+Nothing on this path can raise a privilege: step 3 only ever produces markers.
+
+The same two methods run on the MyE&P SSO path (`MyEpAuthenticator`), against the eligible
+roles it reports — one policy, two identity sources.
 
 ### 3. Approval — turning a marker into a role
 
 `/admin/system/user/edit/{id}`, `UserType`, gated by `UserVoter::EDIT`: super admin only,
 never yourself, never while impersonating.
 
-The `roles` field binds to `assignedRoles`, whose setter **replaces the whole column**.
+The page keeps three things apart, because they follow three different rules:
-Consequences to be aware of:
 
-- Approving is "tick the real role and save". The marker disappears because the column is
+| Block | What it is |
-  rewritten from the ticked choices.
+|-------|-----------|
-- Saving the form drops *every* marker, including ones you did not act on.
+| **Aus BusPro** | information, not a form: granted roles, houses, last login. Synced on every login and editable nowhere in this application |
-- A denial is not recorded anywhere. As long as the CRM keeps claiming the role, the marker
+| **Freischaltung** | one action per nomination — a button, a confirmation dialog, its own route (`ApproveRoleController`). Only shown when the user carries a marker |
-  returns on that user's next login.
+| **Account** | the actual form (`UserType`): super admin, block, block reasons |
 
-### 4. Revocation — demotion by the CRM
+None of this is a disabled form field. Roles and hotel codes are simply not fields, so there
+is nothing to submit and nothing that looks editable but is not.
+
+**Approval is its own act**, deliberately not a checkbox on the form: it grants a privilege,
+so it is confirmed on its own, logged on its own, and cannot happen as a side effect of
+saving an unrelated setting. `UserDataHandler::approveRole()` refuses any role the user has
+no marker for, so a hand-crafted URL cannot grant one the CRM never claimed, and the check
+runs again on submit to catch a sync that revoked the claim while the dialog was open.
+
+A denial is not recorded anywhere: as long as the CRM keeps claiming the role, the
+nomination is back on the next login.
+
+**Super admin** is only offered to somebody who already holds `ROLE_ADMIN` — approve first,
+elevate afterwards. The one exception is a flag that outlived its role, which stays editable
+so the account can be saved at all while `User::validateSuperAdmin()` is violated; the sync
+clears it (see 2), so it should never occur in practice.
+
+The user list marks nominations with their own badge — it is the only place an approver would
+look for them.
+
+### 4. Losing everything — the block
+
+Revocation of an *individual* role is step 1 of the sync above. This section is the stronger
+case: the CRM claims **nothing at all**, which is not a demotion but an exit.
 
 Evaluated on every login, in `BpnAuthenticator::getOrCreateLocalUser()`, before the local
-user is even loaded. If `collectRoles()` is empty, the CRM grants this person nothing in
+user is even loaded. If `collectClaimedRoles()` is empty, the CRM grants this person nothing
-this application, so they are not a user of it:
+in this application, so they are not a user of it:
 
 | Situation | Outcome |
 |-----------|---------|
@@ -163,7 +218,9 @@ pending markers — they no longer reflect the CRM. **Granted roles are kept**,
 stays reviewable.
 
 Two preconditions guard this branch, because "no roles" is otherwise indistinguishable from
-"the CRM told us nothing":
+"the CRM told us nothing". They now protect the sync as well: a degraded response that got
+past them would not merely block one account, it would strip the roles of every user logging
+in.
 
 1. the response must be a `CrmAttributesResponse` — BusPro answers with a notification
    record on its own errors;

src/BusProNet/UserDataHandler.php

@@ -14,9 +14,17 @@ use Psr\Log\LoggerInterface;
 /**
  * Owns the role policy for local user accounts, whatever identity source reports them.
  * The importing of BusPro profile data is specific to the CRM, but the role rules -
- * toPendingRoles(), refreshPendingRoles() and grantTeamerRole() - are deliberately
+ * toPendingRoles() and everything syncRoles() composes - are deliberately shared with
- * shared with the MyE&P SSO login (see App\Security\MyEpAuthenticator), so that no
+ * the MyE&P SSO login (see App\Security\MyEpAuthenticator), so that no identity source
- * identity source can grant an administrative role this application would not.
+ * can grant an administrative role this application would not.
+ *
+ * The policy in one paragraph: the identity source leads. Every role it no longer claims
+ * is revoked on the next login, and the hotel codes are re-imported with them. What it
+ * claims is not granted, though: an administrative role arrives as a privilege-free
+ * marker and only a super admin turns it into the real role. ROLE_TEAMER is the exception
+ * that needs no approval - it carries no privileges of its own - and is granted outright.
+ * Nobody can therefore escalate through the CRM, and nobody keeps an access the CRM has
+ * taken away.
  */
 class UserDataHandler
 {
@@ -27,7 +35,7 @@ class UserDataHandler
     }
 
     /**
-     * Collects the roles to grant on initial user creation.
+     * Collects the roles to store on initial user creation.
      *
      * Administrative roles are deliberately not importable: they may only be granted
      * manually by a super admin, so that nobody can escalate their own privileges via
@@ -48,9 +56,15 @@ class UserDataHandler
     }
 
     /**
-     * Collects the pending markers for the administrative roles claimed in the CRM.
+     * The roles the CRM claims for this person, as plain role names.
+     *
+     * This is what the CRM says, not what the application grants for it - see syncRoles()
+     * for that. Named after MyEpAuthenticator::collectClaimedRoles(), which produces the
+     * same shape from the SSO claims, so that one policy can serve both identity sources.
+     *
+     * @return string[]
      */
-    public function collectPendingRoles(CrmAttributesResponse $crmAttributes): array
+    public function collectClaimedRoles(CrmAttributesResponse $crmAttributes): array
     {
         $claimedRoles = [];
 
@@ -66,7 +80,19 @@ class UserDataHandler
             $claimedRoles[] = 'ROLE_HOUSE_MANAGER';
         }
 
-        return $this->toPendingRoles($claimedRoles);
+        if ($crmAttributes->isTeamer()) {
+            $claimedRoles[] = 'ROLE_TEAMER';
+        }
+
+        return $claimedRoles;
+    }
+
+    /**
+     * Collects the pending markers for the administrative roles claimed in the CRM.
+     */
+    public function collectPendingRoles(CrmAttributesResponse $crmAttributes): array
+    {
+        return $this->toPendingRoles($this->collectClaimedRoles($crmAttributes));
     }
 
     /**
@@ -75,6 +101,10 @@ class UserDataHandler
      * attributes, expressed over plain role names so that any identity source can use
      * it. ROLE_TEAMER has no marker and is dropped here: it needs no approval.
      *
+     * Every claimed role gets its own marker. The roles stand on their own - a manager is
+     * not a super-set of a house manager - so somebody claiming both is approved for both,
+     * one decision at a time.
+     *
      * @param string[] $claimedRoles
      *
      * @return string[]
@@ -83,15 +113,10 @@ class UserDataHandler
     {
         $roles = [];
 
-        if (true === in_array('ROLE_ADMIN', $claimedRoles, true)) {
+        foreach (User::PENDING_ROLES as $role => $pendingRole) {
-            $roles[] = User::PENDING_ROLES['ROLE_ADMIN'];
+            if (true === in_array($role, $claimedRoles, true)) {
-        }
+                $roles[] = $pendingRole;
-
+            }
-        // a manager outranks a house manager, so only the higher marker is kept
-        if (true === in_array('ROLE_MANAGER', $claimedRoles, true)) {
-            $roles[] = User::PENDING_ROLES['ROLE_MANAGER'];
-        } elseif (true === in_array('ROLE_HOUSE_MANAGER', $claimedRoles, true)) {
-            $roles[] = User::PENDING_ROLES['ROLE_HOUSE_MANAGER'];
         }
 
         return $roles;
@@ -209,13 +234,11 @@ class UserDataHandler
     /**
      * Updates an existing user from BusPro data.
      *
-     * Administrative roles and hotel codes are imported once on user creation only and are
+     * The CRM leads: roles and hotel codes are re-synced on every login, so a role or a
-     * managed manually afterwards, so they are intentionally left untouched here. The
+     * house it no longer reports is gone. Granting still needs approval - see syncRoles().
-     * 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()
+     * @param string[] $claimedRoles plain role names as returned by collectClaimedRoles()
+     * @param string[] $hotelCodes   the houses the CRM reports for this person
      */
     public function updateLocalUser(
         User $user,
@@ -223,6 +246,7 @@ class UserDataHandler
         bool $isTeamer = false,
         array $crmSelections = [],
         array $claimedRoles = [],
+        array $hotelCodes = [],
     ): void {
         $user
             ->setFirstName($profileResponse->getFirstName())
@@ -230,11 +254,10 @@ class UserDataHandler
             ->setEmail($profileResponse->getCommunication()->getEmail())
         ;
 
-        $this->refreshPendingRoles($user, $claimedRoles);
+        $this->syncRoles($user, $claimedRoles);
+        $this->syncHotelCodes($user, $hotelCodes);
 
         if (true === $isTeamer) {
-            $this->grantTeamerRole($user);
-
             $address = Address::fromApiResponse($profileResponse);
             $communication = Communication::fromApiResponse($profileResponse);
             if (null === $user->getTeamer()) {
@@ -306,6 +329,148 @@ class UserDataHandler
         ]);
     }
 
+    /**
+     * Turns a nomination into the real role - the one role decision left to a human.
+     *
+     * Only a role the identity source currently claims can be approved, and the marker is
+     * what says so: no marker, nothing to approve, whoever asked for it. Everything else
+     * the user holds is left exactly as it is, markers for other roles included.
+     *
+     * Returns false when there is nothing to approve, so the caller can refuse the request
+     * rather than silently granting a role off a hand-crafted URL.
+     */
+    public function approveRole(User $user, string $role): bool
+    {
+        $pendingRole = User::PENDING_ROLES[$role] ?? null;
+
+        if (null === $pendingRole || false === in_array($pendingRole, $user->getPendingRoles(), true)) {
+            return false;
+        }
+
+        $user->setRoles([
+            ...$user->getAssignedRoles(),
+            $role,
+            ...array_values(array_diff($user->getPendingRoles(), [$pendingRole])),
+        ]);
+
+        $this->entityManager->flush();
+
+        $this->logger->info('Approve role', [
+            'user_id' => $user->getId(),
+            'user_email' => $user->getEmail(),
+            'role' => $role,
+        ]);
+
+        return true;
+    }
+
+    /**
+     * Brings a user's roles in line with what the identity source claims for them.
+     *
+     * The whole policy, in the order it has to run:
+     *
+     * 1. revoke what is no longer claimed - the identity source leads;
+     * 2. drop the super admin flag along with ROLE_ADMIN, or the highest privilege in the
+     *    application would outlive the role it depends on;
+     * 3. refresh the pending markers, after the revocation so that a role just revoked is
+     *    not immediately marked again - it is unclaimed in both steps;
+     * 4. grant ROLE_TEAMER, the one role that needs no approval.
+     *
+     * Nothing here grants an administrative role: step 3 only ever produces markers, and
+     * turning one into the real role is a super admin's decision.
+     *
+     * Does not flush; the caller decides when to.
+     *
+     * @param string[] $claimedRoles plain role names, e.g. from collectClaimedRoles()
+     */
+    public function syncRoles(User $user, array $claimedRoles): void
+    {
+        $this->revokeUnclaimedRoles($user, $claimedRoles);
+        $this->refreshPendingRoles($user, $this->toPendingRoles($claimedRoles));
+
+        if (true === in_array('ROLE_TEAMER', $claimedRoles, true)) {
+            $this->grantTeamerRole($user);
+        }
+    }
+
+    /**
+     * Replaces the houses a user is responsible for with the ones the identity source
+     * reports. They are led by the CRM just like the roles are, so a house manager who
+     * moves house is not left seeing their old one.
+     *
+     * Does not flush; the caller decides when to.
+     *
+     * @param string[] $hotelCodes
+     */
+    public function syncHotelCodes(User $user, array $hotelCodes): void
+    {
+        $hotelCodes = array_values($hotelCodes);
+
+        if ($hotelCodes === $user->getHotelCodes()) {
+            return;
+        }
+
+        $user->setHotelCodes($hotelCodes);
+
+        $this->logger->info('Sync hotel codes', [
+            'user_id' => $user->getId(),
+            'user_email' => $user->getEmail(),
+            'hotel_codes' => $hotelCodes,
+        ]);
+    }
+
+    /**
+     * Withdraws every granted role the identity source no longer claims.
+     *
+     * Only the roles of User::ROLES are touched: getAssignedRoles() excludes the pending
+     * markers as well as the implicit ROLE_USER, and the markers are dealt with by
+     * refreshPendingRoles(). ROLE_SUPER_ADMIN is not a stored role at all but a flag, so
+     * it is handled separately below.
+     *
+     * @param string[] $claimedRoles
+     */
+    private function revokeUnclaimedRoles(User $user, array $claimedRoles): void
+    {
+        $grantedRoles = $user->getAssignedRoles();
+        $keptRoles = array_values(array_intersect($grantedRoles, $claimedRoles));
+
+        if ($keptRoles === $grantedRoles) {
+            return;
+        }
+
+        $user->setRoles([...$keptRoles, ...$user->getPendingRoles()]);
+
+        $this->logger->info('Revoke roles no longer claimed', [
+            'user_id' => $user->getId(),
+            'user_email' => $user->getEmail(),
+            'revoked_roles' => array_values(array_diff($grantedRoles, $keptRoles)),
+        ]);
+
+        $this->revokeSuperAdminWithoutRoleAdmin($user);
+    }
+
+    /**
+     * Takes the super admin flag down with ROLE_ADMIN.
+     *
+     * The flag is stored on its own and getRoles() turns it into ROLE_SUPER_ADMIN whatever
+     * else the user holds, so without this a person the CRM no longer calls an admin would
+     * keep the one role that outranks every check in the application. User::validateSuperAdmin()
+     * enforces the same rule on the edit form, but only there.
+     */
+    private function revokeSuperAdminWithoutRoleAdmin(User $user): void
+    {
+        if (false === $user->isSuperAdmin() || true === $user->hasRole('ROLE_ADMIN')) {
+            return;
+        }
+
+        $user->setSuperAdmin(false);
+
+        $this->logger->info('Revoke super admin flag along with ROLE_ADMIN', [
+            'user_id' => $user->getId(),
+            'user_email' => $user->getEmail(),
+        ]);
+    }
+
     /**
      * Grants ROLE_TEAMER to a user the CRM reports as a teamer.
      *
@@ -314,8 +479,8 @@ class UserDataHandler
      * 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
+     * Grant-only in itself - withdrawing it is revokeUnclaimedRoles()' business, which
-     * blocks the account anyway, and a role handed out manually must survive a login.
+     * syncRoles() runs first.
      *
      * Does not flush; the caller decides when to.
      */

src/Controller/Admin/System/User/ApproveRoleController.php

@@ -0,0 +1,55 @@
+<?php
+
+namespace App\Controller\Admin\System\User;
+
+use App\BusProNet\UserDataHandler;
+use App\Entity\User;
+use App\Htmx\HxRedirectResponse;
+use App\Security\Voter\UserVoter;
+use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\Routing\Attribute\Route;
+use Symfony\Component\Security\Http\Attribute\IsGranted;
+
+/**
+ * Granting a privilege is its own act, deliberately not a checkbox on the user edit form:
+ * it is confirmed on its own, logged on its own, and cannot happen as a side effect of
+ * saving an unrelated setting. It is also the only way a role is ever granted at all -
+ * everything else about roles is synced from BusPro (see docs/user-roles.md).
+ */
+class ApproveRoleController extends AbstractController
+{
+    public function __construct(private readonly UserDataHandler $userDataHandler)
+    {
+    }
+
+    #[Route('/admin/system/user/approve-role/{id}/{role}', name: 'app_admin_system_user_approve_role')]
+    #[IsGranted(UserVoter::EDIT, subject: 'user')]
+    public function index(User $user, string $role, Request $request): Response
+    {
+        // nothing but a role this user is actually nominated for, so a hand-crafted URL
+        // cannot grant one BusPro never claimed
+        $pendingRole = User::PENDING_ROLES[$role] ?? null;
+        if (null === $pendingRole || false === in_array($pendingRole, $user->getPendingRoles(), true)) {
+            throw $this->createNotFoundException('Für diese Rolle liegt keine Freischaltung vor');
+        }
+
+        if (true === $request->isMethod(Request::METHOD_POST)) {
+            // checked again by the handler, which is what catches a sync revoking the claim
+            // between opening the dialog and confirming it
+            if (false === $this->userDataHandler->approveRole($user, $role)) {
+                throw $this->createNotFoundException('Für diese Rolle liegt keine Freischaltung vor');
+            }
+
+            $this->addFlash('success', sprintf('Die Rolle %s wurde freigeschaltet', User::ROLES[$role]));
+
+            return new HxRedirectResponse($this->generateUrl('app_admin_system_user_edit', ['id' => $user->getId()]));
+        }
+
+        return $this->render('admin/system/user/modal_approve_role.html.twig', [
+            'user' => $user,
+            'roleLabel' => User::ROLES[$role],
+        ]);
+    }
+}

src/Controller/Admin/System/User/EditController.php

@@ -2,6 +2,7 @@
 
 namespace App\Controller\Admin\System\User;
 
+use App\Config\HouseCatalog;
 use App\Entity\User;
 use App\Form\UserType;
 use App\Security\Voter\UserVoter;
@@ -18,6 +19,7 @@ class EditController extends AbstractController
     public function __construct(
         private readonly EntityManagerInterface $entityManager,
         private readonly LoggerInterface $logger,
+        private readonly HouseCatalog $houseCatalog,
     ) {
     }
 
@@ -44,9 +46,21 @@ class EditController extends AbstractController
             return $this->redirectToRoute('app_admin_system_user_index');
         }
 
+        // a code with no house behind it stays visible under its own name rather than
+        // vanishing from the page - it is what the user is actually restricted to
+        $houses = [];
+        foreach ($user->getHotelCodes() as $code) {
+            $houses[$code] = $this->houseCatalog->getName($code) ?? $code;
+        }
+
         return $this->render('admin/system/user/edit.html.twig', [
             'form' => $form,
             'user' => $user,
+            'grantedRoles' => array_map(
+                static fn (string $role): string => User::ROLES[$role],
+                $user->getAssignedRoles(),
+            ),
+            'houses' => $houses,
         ]);
     }
 }

src/Entity/User.php

@@ -231,6 +231,27 @@ class User implements UserInterface, TimestampableEntityInterface, SoftDeletable
         return array_values(array_intersect($this->roles, array_keys(self::ROLES)));
     }
 
+    /**
+     * The roles this user is nominated for but does not hold, as role => label.
+     *
+     * The markers are storage; this is what an approver acts on, so it is keyed by the real
+     * role rather than by the marker standing in for it.
+     *
+     * @return array<string, string>
+     */
+    public function getNominatedRoles(): array
+    {
+        $roles = [];
+
+        foreach (self::PENDING_ROLES as $role => $pendingRole) {
+            if (true === in_array($pendingRole, $this->roles, true)) {
+                $roles[$role] = self::ROLES[$role];
+            }
+        }
+
+        return $roles;
+    }
+
     /**
      * The pending markers currently held by the user.
      */

src/Form/UserType.php

@@ -2,7 +2,6 @@
 
 namespace App\Form;
 
-use App\Config\HouseCatalog;
 use App\Entity\User;
 use Symfony\Component\Form\AbstractType;
 use Symfony\Component\Form\Extension\Core\Type\CheckboxType;
@@ -12,27 +11,20 @@ use Symfony\Component\Form\FormEvent;
 use Symfony\Component\Form\FormEvents;
 use Symfony\Component\OptionsResolver\OptionsResolver;
 
+/**
+ * The account settings of an administrative user - and nothing else.
+ *
+ * Roles and hotel codes are synced from BusPro on every login and are not editable here or
+ * anywhere else, so they are not fields: the edit page renders them as information. The one
+ * role decision left to a human, approving a nomination, is its own action with its own
+ * confirmation (see App\Controller\Admin\System\User\ApproveRoleController). Keeping all
+ * three apart is deliberate - a form field that cannot be submitted only looks broken.
+ */
 class UserType extends AbstractType
 {
-    public function __construct(private readonly HouseCatalog $houseCatalog)
-    {
-    }
-
     public function buildForm(FormBuilderInterface $builder, array $options): void
     {
         $builder
-            ->add('roles', MultiselectType::class, [
-                'label' => 'Rollen',
-                'required' => false,
-                'property_path' => 'assignedRoles',
-                'choices' => array_flip(User::ROLES),
-                'empty_label' => 'keine Rolle',
-            ])
-            ->add('superAdmin', CheckboxType::class, [
-                'label' => 'Superadmin',
-                'required' => false,
-                'help' => 'Setzt die Rolle Admin voraus.',
-            ])
             ->add('disabled', CheckboxType::class, [
                 'label' => 'Account gesperrt',
                 'required' => false,
@@ -74,23 +66,26 @@ class UserType extends AbstractType
             ;
         });
 
-        // hotel codes already assigned to the user may predate the catalog, so they are
+        // Super admin is an elevation of an existing role, never a grant of its own, so the
-        // added as choices to keep them selectable instead of failing
+        // field exists only for somebody who already holds ROLE_ADMIN - offering it to
-        $builder->addEventListener(FormEvents::PRE_SET_DATA, function (FormEvent $event): void {
+        // anyone else would only produce the violation from User::validateSuperAdmin().
+        // A flag already set without the role is the exception: it has to stay editable, or
+        // that user could not be saved at all until BusPro claims them an admin again.
+        $builder->addEventListener(FormEvents::PRE_SET_DATA, static function (FormEvent $event): void {
             $user = $event->getData();
 
-            $choices = $this->houseCatalog->getCodeChoices();
+            if (false === $user instanceof User) {
-            foreach ($user instanceof User ? $user->getHotelCodes() : [] as $code) {
+                return;
-                if (false === in_array($code, $choices, true)) {
-                    $choices[$code] = $code;
-                }
             }
 
-            $event->getForm()->add('hotelCodes', MultiselectType::class, [
+            if (false === in_array('ROLE_ADMIN', $user->getAssignedRoles(), true) && false === $user->isSuperAdmin()) {
-                'label' => 'Häuser',
+                return;
+            }
+
+            $event->getForm()->add('superAdmin', CheckboxType::class, [
+                'label' => 'Superadmin',
                 'required' => false,
-                'choices' => $choices,
+                'help' => 'Darf Rollen freischalten und Accounts verwalten.',
-                'empty_label' => 'keine Häuser',
             ]);
         });
     }

src/Security/BpnAuthenticator.php

@@ -128,8 +128,15 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent
         // Determine teamer status from CRM attributes
         $isTeamer = $crmAttributes->isTeamer();
 
-        // Everything the CRM grants this person here: pending markers plus ROLE_TEAMER
+        // What the CRM claims, as plain role names, and what this application makes of it:
+        // pending markers plus ROLE_TEAMER. The first drives the sync, the second is what
+        // a new account starts with.
         $claimedRoles = $this
+            ->userDataHandler
+            ->collectClaimedRoles($crmAttributes)
+        ;
+
+        $grantedRoles = $this
             ->userDataHandler
             ->collectRoles($crmAttributes)
         ;
@@ -180,8 +187,8 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent
             return $user;
         }
 
-        // Update existing user's teamer data and return it, leaving roles and hotel
+        // Update the existing user, re-syncing roles and hotel codes from the CRM: it
-        // codes alone: they are imported once on creation and managed manually after
+        // leads, so a role or a house it no longer reports is withdrawn here
         if (null !== $user) {
             $this
                 ->userDataHandler
@@ -190,7 +197,8 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent
                     $profileResponse,
                     $isTeamer,
                     $crmSelections,
-                    $this->userDataHandler->collectPendingRoles($crmAttributes),
+                    $claimedRoles,
+                    $crmAttributes->getHotelCodes(),
                 )
             ;
 
@@ -202,7 +210,7 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent
             ->userDataHandler
             ->createLocalUser(
                 $profileResponse,
-                $claimedRoles,
+                $grantedRoles,
                 $isTeamer,
                 $crmSelections,
                 $crmAttributes->getHotelCodes(),

src/Security/MyEpAuthenticator.php

@@ -161,6 +161,7 @@ class MyEpAuthenticator extends AbstractAuthenticator
 
         $isTeamer = in_array('ROLE_TEAMER', $claimedRoles, true);
         $pendingRoles = $this->userDataHandler->toPendingRoles($claimedRoles);
+        $hotelCodes = $this->collectHotelCodes($userinfo);
 
         $user = $this->findLocalUser($userinfo);
 
@@ -177,23 +178,20 @@ class MyEpAuthenticator extends AbstractAuthenticator
             return $user;
         }
 
-        // Update existing user, leaving granted roles and hotel codes alone: they are
+        // Update existing user. MyE&P leads here exactly as BusPro does on the other login
-        // imported once on creation and managed manually afterwards. Only the
+        // path - the shared policy in UserDataHandler withdraws what is no longer claimed
-        // privilege-free markers and ROLE_TEAMER track MyE&P on every login.
+        // and marks administrative claims for approval rather than granting them.
         if (null !== $user) {
             $this->refreshBusProIds($user, $userinfo);
-            $this->userDataHandler->refreshPendingRoles($user, $pendingRoles);
+            $this->userDataHandler->syncRoles($user, $claimedRoles);
-
+            $this->userDataHandler->syncHotelCodes($user, $hotelCodes);
-            if (true === $isTeamer) {
-                $this->userDataHandler->grantTeamerRole($user);
-            }
 
             $this->entityManager->flush();
 
             return $user;
         }
 
-        return $this->createLocalUser($userinfo, $pendingRoles, $isTeamer);
+        return $this->createLocalUser($userinfo, $pendingRoles, $isTeamer, $hotelCodes);
     }
 
     /**
@@ -233,6 +231,19 @@ class MyEpAuthenticator extends AbstractAuthenticator
         return $claimedRoles;
     }
 
+    /**
+     * The houses MyE&P reports for this person. Read on both the create and the update
+     * path, since they are led by the identity provider just like the roles are.
+     *
+     * @return string[]
+     */
+    private function collectHotelCodes(array $userinfo): array
+    {
+        $profile = is_array($userinfo['profile'] ?? null) ? $userinfo['profile'] : [];
+
+        return is_array($profile['hotel_codes'] ?? null) ? array_values($profile['hotel_codes']) : [];
+    }
+
     /**
      * Matches the local account on the BusPro ids first and falls back to the email, the
      * same precedence UserDataHandler::findLocalUser() applies to a BusPro login, so that
@@ -300,8 +311,9 @@ class MyEpAuthenticator extends AbstractAuthenticator
 
     /**
      * @param string[] $pendingRoles
+     * @param string[] $hotelCodes
      */
-    private function createLocalUser(array $userinfo, array $pendingRoles, bool $isTeamer): ?User
+    private function createLocalUser(array $userinfo, array $pendingRoles, bool $isTeamer, array $hotelCodes): ?User
     {
         $addressId = $userinfo['address_id'] ?? null;
         $personId = $userinfo['person_id'] ?? null;
@@ -318,7 +330,6 @@ class MyEpAuthenticator extends AbstractAuthenticator
         }
 
         $profile = is_array($userinfo['profile'] ?? null) ? $userinfo['profile'] : [];
-        $hotelCodes = is_array($profile['hotel_codes'] ?? null) ? $profile['hotel_codes'] : [];
 
         $user = new User();
         $user

templates/admin/system/user/_form.html.twig

@@ -1,8 +1,9 @@
 {{ form_start(form) }}
 <div class="flex flex-col space-y-4 pb-8">
-    {{ form_row(form.roles) }}
+    {# only present for somebody who already holds ROLE_ADMIN, see UserType #}
-    {{ form_row(form.superAdmin) }}
+    {% if form.superAdmin is defined %}
-    {{ form_row(form.hotelCodes) }}
+        {{ form_row(form.superAdmin) }}
+    {% endif %}
     {{ form_row(form.disabled) }}
     {{ form_row(form.disabledReason) }}
     {{ form_row(form.disabledReasonInternal) }}

templates/admin/system/user/edit.html.twig

@@ -7,6 +7,85 @@
         <h1 class="text-2xl font-bold pb-8">
             {{ user.fullName }} bearbeiten
         </h1>
+
+        <div class="pb-10">
+            <h2 class="text-lg font-bold pb-1">
+                Aus BusPro
+            </h2>
+            <p class="text-sm text-gray-500 pb-2">
+                Wird bei jeder Anmeldung übernommen und kann hier nicht geändert werden.
+            </p>
+            <dl class="divide-y divide-gray-100 border-t border-gray-100">
+                <div class="py-4 sm:grid sm:grid-cols-3 sm:gap-4">
+                    <dt class="text-sm font-bold leading-6 text-gray-900">
+                        Zugewiesene Rollen
+                    </dt>
+                    <dd class="mt-1 text-sm leading-6 text-gray-700 sm:col-span-2 sm:mt-0">
+                        {{ grantedRoles|join(', ')|default('keine Rolle') }}
+                        {% if user.superAdmin %}
+                            <div class="text-gray-500">
+                                Superadmin
+                            </div>
+                        {% endif %}
+                    </dd>
+                </div>
+                <div class="py-4 sm:grid sm:grid-cols-3 sm:gap-4">
+                    <dt class="text-sm font-bold leading-6 text-gray-900">
+                        Zugewiesene Häuser
+                    </dt>
+                    <dd class="mt-1 text-sm leading-6 text-gray-700 sm:col-span-2 sm:mt-0">
+                        {% for code, name in houses %}
+                            <div>
+                                {{ name }} <span class="text-gray-500">({{ code }})</span>
+                            </div>
+                        {% else %}
+                            -
+                        {% endfor %}
+                    </dd>
+                </div>
+                <div class="py-4 sm:grid sm:grid-cols-3 sm:gap-4">
+                    <dt class="text-sm font-bold leading-6 text-gray-900">
+                        Letzter Login
+                    </dt>
+                    <dd class="mt-1 text-sm leading-6 text-gray-700 sm:col-span-2 sm:mt-0">
+                        {{ user.lastLoginAt ? user.lastLoginAt|date('d.m.Y, H:i') : '-' }}
+                    </dd>
+                </div>
+            </dl>
+        </div>
+
+        {# Its own act, with its own confirmation: granting a privilege must not happen as a
+           side effect of saving something else on this page. #}
+        {% if user.nominatedRoles is not empty %}
+            <div class="pb-10">
+                <h2 class="text-lg font-bold pb-1">
+                    Freischaltung
+                </h2>
+                <p class="text-sm text-gray-500 pb-2">
+                    In BusPro beansprucht, aber noch nicht freigeschaltet.
+                </p>
+                <ul class="divide-y divide-gray-100 border-t border-gray-100">
+                    {% for role, label in user.nominatedRoles %}
+                        <li class="py-3 flex items-center justify-between">
+                            <span class="text-sm">
+                                {{ label }}
+                            </span>
+                            <button type="button"
+                                    class="btn btn--secondary"
+                                    hx-get="{{ path('app_admin_system_user_approve_role', { 'id': user.id, 'role': role }) }}"
+                                    hx-target="body"
+                                    hx-swap="beforeend">
+                                Freischalten
+                            </button>
+                        </li>
+                    {% endfor %}
+                </ul>
+            </div>
+        {% endif %}
+
+        <h2 class="text-lg font-bold pb-2">
+            Account
+        </h2>
         {% include 'admin/system/user/_form.html.twig' %}
     </div>
 {% endblock %}

templates/admin/system/user/index.html.twig

@@ -48,7 +48,13 @@
                             {{ user.email }}
                         </td>
                         <td>
-                            {{ user.rolesLabels|join(', ') }}
+                            {% set grantedRoles = user.assignedRoles|map(role => constant('App\\Entity\\User::ROLES')[role]) %}
+                            {{ grantedRoles is empty ? '-' : grantedRoles|join(', ') }}
+                            {% for label in user.nominatedRoles %}
+                                <span class="inline-block mt-1 py-1 px-2 text-xs bg-amber-500 text-gray-700 rounded-md">
+                                    {{ label }} angefordert
+                                </span>
+                            {% endfor %}
                         </td>
                         <td class="text-center">
                             {% if user.superAdmin %}
@@ -91,4 +97,4 @@
             </table>
         </div>
     </div>
-{% endblock %}
+{% endblock %}

templates/admin/system/user/modal_approve_role.html.twig

@@ -0,0 +1,12 @@
+{% extends 'htmx_confirmation_modal.html.twig' %}
+
+{% block content %}
+    <div class="pb-4">
+        Möchtest du die Rolle <em>{{ roleLabel }}</em> für <em>{{ user.fullName }}</em> wirklich freischalten?
+    </div>
+    <div>
+        Die Rolle wird damit sofort wirksam. Sie wird automatisch wieder entzogen, sobald sie in BusPro nicht mehr beansprucht wird.
+    </div>
+{% endblock %}
+
+{% block button_confirm %}Freischalten{% endblock %}

tests/BusProNet/UserDataHandlerTest.php

@@ -72,9 +72,9 @@ class UserDataHandlerTest extends TestCase
             [User::PENDING_ROLES['ROLE_ADMIN'], User::PENDING_ROLES['ROLE_MANAGER']],
         ];
 
-        yield 'manager takes precedence over house manager' => [
+        yield 'manager and house manager yield both markers, the roles stand on their own' => [
             (new CrmAttributesResponse())->setManager(true)->setHouseManager(true),
-            [User::PENDING_ROLES['ROLE_MANAGER']],
+            [User::PENDING_ROLES['ROLE_MANAGER'], User::PENDING_ROLES['ROLE_HOUSE_MANAGER']],
         ];
 
         yield 'house manager and teamer' => [
@@ -105,10 +105,10 @@ class UserDataHandlerTest extends TestCase
             [User::PENDING_ROLES['ROLE_ADMIN']],
         ];
 
-        yield 'manager takes precedence over house manager' => [
+        yield 'manager and house manager are marked independently' => [
             ['ROLE_HOUSE_MANAGER', 'ROLE_MANAGER'],
             (new CrmAttributesResponse())->setManager(true)->setHouseManager(true),
-            [User::PENDING_ROLES['ROLE_MANAGER']],
+            [User::PENDING_ROLES['ROLE_MANAGER'], User::PENDING_ROLES['ROLE_HOUSE_MANAGER']],
         ];
 
         yield 'house manager' => [
@@ -172,14 +172,16 @@ class UserDataHandlerTest extends TestCase
             $profileResponse,
             true,
             ['team' => ['selected' => true]],
+            ['ROLE_ADMIN', 'ROLE_TEAMER'],
+            ['DKS'],
         );
 
         $this->assertSame('New', $user->getFirstName());
         $this->assertSame('Lastname', $user->getLastName());
         $this->assertSame('[email protected]', $user->getEmail());
 
-        // roles and hotel codes are imported on creation only and stay under manual control
+        // the CRM leads: the still claimed role survives, the houses are replaced by its own
-        $this->assertSame(['XYZ'], $user->getHotelCodes());
+        $this->assertSame(['DKS'], $user->getHotelCodes());
         $this->assertTrue($user->hasRole('ROLE_ADMIN'));
 
         $this->assertSame('New', $teamer->getFirstName());
@@ -225,44 +227,51 @@ class UserDataHandlerTest extends TestCase
     {
         yield 'marker is added when the CRM claims a manager' => [
             ['ROLE_TEAMER'],
-            [User::PENDING_ROLES['ROLE_MANAGER']],
+            ['ROLE_MANAGER', 'ROLE_TEAMER'],
             ['ROLE_TEAMER'],
             [User::PENDING_ROLES['ROLE_MANAGER']],
         ];
 
         yield 'marker is dropped when the CRM attribute is gone' => [
             [User::PENDING_ROLES['ROLE_HOUSE_MANAGER'], 'ROLE_TEAMER'],
-            [],
+            ['ROLE_TEAMER'],
             ['ROLE_TEAMER'],
             [],
         ];
 
         yield 'an approved role is never marked again' => [
             ['ROLE_MANAGER'],
-            [User::PENDING_ROLES['ROLE_MANAGER']],
+            ['ROLE_MANAGER'],
             ['ROLE_MANAGER'],
             [],
         ];
 
         yield 'a claim beyond the approved role stays pending' => [
             ['ROLE_MANAGER'],
-            [User::PENDING_ROLES['ROLE_ADMIN'], User::PENDING_ROLES['ROLE_MANAGER']],
+            ['ROLE_ADMIN', 'ROLE_MANAGER'],
             ['ROLE_MANAGER'],
             [User::PENDING_ROLES['ROLE_ADMIN']],
         ];
 
         yield 'the claimed role changes' => [
             [User::PENDING_ROLES['ROLE_HOUSE_MANAGER']],
-            [User::PENDING_ROLES['ROLE_MANAGER']],
+            ['ROLE_MANAGER'],
             [],
             [User::PENDING_ROLES['ROLE_MANAGER']],
         ];
 
-        yield 'granted roles are untouched without any claim' => [
+        yield 'a granted role is revoked once the CRM stops claiming it' => [
-            ['ROLE_ADMIN'],
+            ['ROLE_ADMIN', 'ROLE_TEAMER'],
+            ['ROLE_TEAMER'],
+            ['ROLE_TEAMER'],
             [],
-            ['ROLE_ADMIN'],
+        ];
+
+        yield 'a manager who is also a house manager is marked for both' => [
             [],
+            ['ROLE_MANAGER', 'ROLE_HOUSE_MANAGER'],
+            [],
+            [User::PENDING_ROLES['ROLE_MANAGER'], User::PENDING_ROLES['ROLE_HOUSE_MANAGER']],
         ];
     }
 
@@ -277,15 +286,18 @@ class UserDataHandlerTest extends TestCase
         ;
 
         $handler = new UserDataHandler($this->entityManager, $this->logger);
-        $handler->updateLocalUser($user, $this->createProfileResponse(), true, [], [User::PENDING_ROLES['ROLE_ADMIN']]);
+        $handler->updateLocalUser($user, $this->createProfileResponse(), true, [], ['ROLE_ADMIN', 'ROLE_TEAMER']);
 
         $this->assertSame(['ROLE_TEAMER'], $user->getAssignedRoles());
         $this->assertSame([User::PENDING_ROLES['ROLE_ADMIN']], $user->getPendingRoles());
     }
 
-    public function testUpdateLocalUserKeepsTheTeamerRoleOfSomebodyTheCrmNoLongerReportsAsTeamer(): void
+    /**
+     * ROLE_TEAMER needs no approval, but it is not exempt from the sync either: the CRM
+     * leads, so the role goes when the attribute does.
+     */
+    public function testUpdateLocalUserWithdrawsTheTeamerRoleOfSomebodyTheCrmNoLongerReportsAsTeamer(): void
     {
-        // the role may have been granted manually and must survive a login
         $user = (new User())
             ->setFirstName('First')
             ->setLastName('Last')
@@ -294,9 +306,9 @@ class UserDataHandlerTest extends TestCase
         ;
 
         $handler = new UserDataHandler($this->entityManager, $this->logger);
-        $handler->updateLocalUser($user, $this->createProfileResponse(), false, [], []);
+        $handler->updateLocalUser($user, $this->createProfileResponse(), false, [], ['ROLE_ADMIN']);
 
-        $this->assertSame(['ROLE_ADMIN', 'ROLE_TEAMER'], $user->getAssignedRoles());
+        $this->assertSame(['ROLE_ADMIN'], $user->getAssignedRoles());
     }
 
     public function testUpdateLocalUserGrantsTheTeamerRoleOnlyOnce(): void
@@ -309,11 +321,131 @@ class UserDataHandlerTest extends TestCase
         ;
 
         $handler = new UserDataHandler($this->entityManager, $this->logger);
-        $handler->updateLocalUser($user, $this->createProfileResponse(), true, [], []);
+        $handler->updateLocalUser($user, $this->createProfileResponse(), true, [], ['ROLE_TEAMER']);
 
         $this->assertSame(['ROLE_TEAMER'], $user->getAssignedRoles());
     }
 
+    /**
+     * ROLE_SUPER_ADMIN is not a stored role but a flag getRoles() turns into one, so
+     * revoking ROLE_ADMIN has to take it down explicitly - otherwise the highest privilege
+     * in the application would outlive the role it depends on.
+     */
+    public function testUpdateLocalUserTakesTheSuperAdminFlagDownWithRoleAdmin(): void
+    {
+        $user = (new User())
+            ->setFirstName('First')
+            ->setLastName('Last')
+            ->setEmail('[email protected]')
+            ->setRoles(['ROLE_ADMIN', 'ROLE_TEAMER'])
+            ->setSuperAdmin(true)
+        ;
+
+        $handler = new UserDataHandler($this->entityManager, $this->logger);
+        $handler->updateLocalUser($user, $this->createProfileResponse(), true, [], ['ROLE_TEAMER']);
+
+        $this->assertSame(['ROLE_TEAMER'], $user->getAssignedRoles());
+        $this->assertFalse($user->isSuperAdmin());
+        $this->assertFalse($user->hasRole('ROLE_SUPER_ADMIN'));
+    }
+
+    public function testUpdateLocalUserKeepsTheSuperAdminFlagOfAStillClaimedAdmin(): void
+    {
+        $user = (new User())
+            ->setFirstName('First')
+            ->setLastName('Last')
+            ->setEmail('[email protected]')
+            ->setRoles(['ROLE_ADMIN', 'ROLE_TEAMER'])
+            ->setSuperAdmin(true)
+        ;
+
+        $handler = new UserDataHandler($this->entityManager, $this->logger);
+        $handler->updateLocalUser($user, $this->createProfileResponse(), false, [], ['ROLE_ADMIN']);
+
+        $this->assertTrue($user->isSuperAdmin());
+        $this->assertTrue($user->hasRole('ROLE_SUPER_ADMIN'));
+    }
+
+    public function testUpdateLocalUserReplacesTheHotelCodesWithTheOnesTheCrmReports(): void
+    {
+        $user = (new User())
+            ->setFirstName('First')
+            ->setLastName('Last')
+            ->setEmail('[email protected]')
+            ->setRoles(['ROLE_HOUSE_MANAGER'])
+            ->setHotelCodes(['SSL'])
+        ;
+
+        $handler = new UserDataHandler($this->entityManager, $this->logger);
+        $handler->updateLocalUser($user, $this->createProfileResponse(), false, [], ['ROLE_HOUSE_MANAGER'], ['DKS']);
+
+        $this->assertSame(['DKS'], $user->getHotelCodes());
+    }
+
+    /**
+     * Approval is the one place a role is granted at all, and it can only ever grant a role
+     * the CRM already claims - the marker is what says so.
+     */
+    public function testApproveRoleGrantsTheRoleAndClearsItsMarker(): void
+    {
+        $user = (new User())
+            ->setFirstName('First')
+            ->setLastName('Last')
+            ->setEmail('[email protected]')
+            ->setRoles([
+                'ROLE_TEAMER',
+                User::PENDING_ROLES['ROLE_ADMIN'],
+                User::PENDING_ROLES['ROLE_HOUSE_MANAGER'],
+            ])
+        ;
+
+        $this->entityManager->expects($this->once())->method('flush');
+
+        $handler = new UserDataHandler($this->entityManager, $this->logger);
+
+        $this->assertTrue($handler->approveRole($user, 'ROLE_ADMIN'));
+        $this->assertSame(['ROLE_TEAMER', 'ROLE_ADMIN'], $user->getAssignedRoles());
+
+        // the other nomination is untouched: one decision at a time
+        $this->assertSame([User::PENDING_ROLES['ROLE_HOUSE_MANAGER']], $user->getPendingRoles());
+    }
+
+    /**
+     * @dataProvider unapprovableRoleProvider
+     */
+    public function testApproveRoleRefusesARoleWithoutANomination(array $roles, string $role): void
+    {
+        $user = (new User())
+            ->setFirstName('First')
+            ->setLastName('Last')
+            ->setEmail('[email protected]')
+            ->setRoles($roles)
+        ;
+
+        $this->entityManager->expects($this->never())->method('flush');
+
+        $handler = new UserDataHandler($this->entityManager, $this->logger);
+
+        $this->assertFalse($handler->approveRole($user, $role));
+        $this->assertEqualsCanonicalizing(
+            $roles,
+            [...$user->getAssignedRoles(), ...$user->getPendingRoles()],
+        );
+    }
+
+    public static function unapprovableRoleProvider(): iterable
+    {
+        yield 'the CRM never claimed it' => [['ROLE_TEAMER'], 'ROLE_MANAGER'];
+
+        yield 'a different role is nominated' => [[User::PENDING_ROLES['ROLE_MANAGER']], 'ROLE_ADMIN'];
+
+        yield 'already granted, so there is no marker left' => [['ROLE_ADMIN'], 'ROLE_ADMIN'];
+
+        yield 'teamer has no nomination to approve' => [['ROLE_TEAMER'], 'ROLE_TEAMER'];
+
+        yield 'not a role at all' => [[User::PENDING_ROLES['ROLE_ADMIN']], 'ROLE_SUPER_ADMIN'];
+    }
+
     public function testDisableForRevokedCrmRolesBlocksTheUserAndDropsThePendingMarkers(): void
     {
         $user = (new User())

tests/Entity/UserTest.php

@@ -165,4 +165,25 @@ class UserTest extends TestCase
             ->validate($user)
         ;
     }
+
+    public function testNominatedRolesAreKeyedByTheRoleAndNotByItsMarker(): void
+    {
+        $user = (new User())->setRoles([
+            'ROLE_TEAMER',
+            User::PENDING_ROLES['ROLE_ADMIN'],
+            User::PENDING_ROLES['ROLE_HOUSE_MANAGER'],
+        ]);
+
+        $this->assertSame(
+            ['ROLE_ADMIN' => 'Admin', 'ROLE_HOUSE_MANAGER' => 'Hausleitung'],
+            $user->getNominatedRoles(),
+        );
+    }
+
+    public function testAGrantedRoleIsNotNominated(): void
+    {
+        $user = (new User())->setRoles(['ROLE_ADMIN']);
+
+        $this->assertSame([], $user->getNominatedRoles());
+    }
 }

tests/Form/UserTypeTest.php

@@ -11,88 +11,98 @@ use Symfony\Component\Form\FormFactoryInterface;
 
 class UserTypeTest extends KernelTestCase
 {
-    public function testRendersRolesWithoutSynthesizedRoles(): void
+    /**
+     * The form is the account settings and nothing else. Roles and hotel codes are synced
+     * from BusPro and editable nowhere, so they are not fields at all - a disabled field
+     * that cannot be submitted only reads as broken.
+     */
+    public function testRolesAndHotelCodesAreNotFields(): void
     {
         $user = (new User())
             ->setRoles(['ROLE_ADMIN'])
+            ->setHotelCodes(['DKS'])
+        ;
+
+        $form = $this->createForm($user);
+
+        $this->assertFalse($form->has('roles'));
+        $this->assertFalse($form->has('hotelCodes'));
+        $this->assertFalse($form->has('approvedRoles'));
+    }
+
+    public function testNeitherRolesNorHotelCodesCanBeSubmitted(): void
+    {
+        $user = (new User())
+            ->setRoles(['ROLE_TEAMER'])
+            ->setHotelCodes(['SSL'])
+        ;
+
+        $form = $this->createForm($user);
+        $form->submit([
+            'roles' => ['ROLE_ADMIN'],
+            'hotelCodes' => ['DKS'],
+            'disabled' => null,
+        ]);
+
+        $this->assertSame(['ROLE_TEAMER'], $user->getAssignedRoles());
+        $this->assertSame(['SSL'], $user->getHotelCodes());
+    }
+
+    /**
+     * Super admin is an elevation of ROLE_ADMIN, so it is not on offer for anybody else -
+     * offering it would only ever produce the violation from User::validateSuperAdmin().
+     */
+    public function testSuperAdminIsOnlyOfferedToAnAdmin(): void
+    {
+        $this->assertTrue($this->createForm((new User())->setRoles(['ROLE_ADMIN']))->has('superAdmin'));
+        $this->assertFalse($this->createForm((new User())->setRoles(['ROLE_MANAGER']))->has('superAdmin'));
+        $this->assertFalse($this->createForm((new User())->setRoles([User::PENDING_ROLES['ROLE_ADMIN']]))->has('superAdmin'));
+    }
+
+    /**
+     * The exception: a flag left over from before its role was revoked has to stay editable,
+     * or that account could not be saved at all while the violation stands.
+     */
+    public function testSuperAdminStaysEditableWhenTheFlagOutlivedTheRole(): void
+    {
+        $user = (new User())
+            ->setRoles(['ROLE_MANAGER'])
             ->setSuperAdmin(true)
         ;
 
-        $view = $this->createForm($user)->createView();
-
-        // ROLE_USER and ROLE_SUPER_ADMIN are synthesized by getRoles() and must not leak in
-        $this->assertSame(['ROLE_ADMIN'], $view->children['roles']->vars['data']);
-    }
-
-    public function testApprovingAPendingRoleClearsTheMarker(): void
-    {
-        $user = (new User())->setRoles([User::PENDING_ROLES['ROLE_ADMIN']]);
-
         $form = $this->createForm($user);
-
+        $this->assertTrue($form->has('superAdmin'));
-        // the marker is not an assignable choice and must not reach the field
-        $this->assertSame([], $form->createView()->children['roles']->vars['data']);
 
         $form->submit([
-            'roles' => ['ROLE_ADMIN'],
             'superAdmin' => null,
-            'hotelCodes' => [],
+            'disabled' => null,
         ]);
 
         $this->assertTrue($form->isValid());
-        $this->assertSame(['ROLE_ADMIN'], $user->getAssignedRoles());
+        $this->assertFalse($user->isSuperAdmin());
-        $this->assertSame([], $user->getPendingRoles());
     }
 
-    public function testRendersHotelCodesNotCoveredByTheConfiguredMap(): void
+    public function testSuperAdminIsAppointed(): void
     {
-        $user = (new User())->setHotelCodes(['XYZ']);
+        $user = (new User())->setRoles(['ROLE_ADMIN']);
-
-        $view = $this->createForm($user)->createView();
-
-        $this->assertSame(['XYZ'], $view->children['hotelCodes']->vars['data']);
-    }
-
-    public function testSubmitStoresAssignedRolesOnly(): void
-    {
-        $user = (new User())->setRoles(['ROLE_TEAMER']);
 
         $form = $this->createForm($user);
         $form->submit([
-            'roles' => ['ROLE_ADMIN'],
             'superAdmin' => '1',
-            'hotelCodes' => [],
+            'disabled' => null,
         ]);
 
         $this->assertTrue($form->isValid());
-        $this->assertSame(['ROLE_ADMIN'], $user->getAssignedRoles());
         $this->assertTrue($user->isSuperAdmin());
     }
 
-    public function testSubmitRejectsSuperAdminWithoutRoleAdmin(): void
-    {
-        $user = new User();
-
-        $form = $this->createForm($user);
-        $form->submit([
-            'roles' => ['ROLE_MANAGER'],
-            'superAdmin' => '1',
-            'hotelCodes' => [],
-        ]);
-
-        $this->assertFalse($form->isValid());
-        $this->assertCount(1, $form->get('superAdmin')->getErrors());
-    }
-
     public function testSubmitBlocksTheAccountWithAReason(): void
     {
         $user = (new User())->setRoles(['ROLE_ADMIN']);
 
         $form = $this->createForm($user);
         $form->submit([
-            'roles' => ['ROLE_ADMIN'],
             'superAdmin' => null,
-            'hotelCodes' => [],
             'disabled' => '1',
             'disabledReason' => 'Wegen Fehlverhaltens gesperrt.',
             'disabledReasonInternal' => 'Siehe Vorgang 4711.',
@@ -115,9 +125,7 @@ class UserTypeTest extends KernelTestCase
 
         $form = $this->createForm($user);
         $form->submit([
-            'roles' => ['ROLE_ADMIN'],
             'superAdmin' => null,
-            'hotelCodes' => [],
             'disabled' => null,
             'disabledReason' => null,
             'disabledReasonInternal' => null,
@@ -143,9 +151,7 @@ class UserTypeTest extends KernelTestCase
 
         // the textareas are prefilled, so unchecking the box alone submits the old reasons
         $form->submit([
-            'roles' => ['ROLE_ADMIN'],
             'superAdmin' => null,
-            'hotelCodes' => [],
             'disabled' => null,
             'disabledReason' => 'Für deinen Account liegt in BusPro keine Berechtigung mehr vor.',
             'disabledReasonInternal' => 'Automatisch gesperrt: keine Rollen in BusPro.',

tests/Security/BpnAuthenticatorTest.php

@@ -38,7 +38,7 @@ class BpnAuthenticatorTest extends TestCase
     {
         $this->stubApiClient($this->createCrmAttributes());
 
-        $this->userDataHandler->method('collectRoles')->willReturn([]);
+        $this->userDataHandler->method('collectClaimedRoles')->willReturn([]);
         $this->userDataHandler->method('findLocalUser')->willReturn(null);
 
         $this->userDataHandler->expects($this->never())->method('createLocalUser');
@@ -55,7 +55,7 @@ class BpnAuthenticatorTest extends TestCase
 
         $this->stubApiClient($this->createCrmAttributes());
 
-        $this->userDataHandler->method('collectRoles')->willReturn([]);
+        $this->userDataHandler->method('collectClaimedRoles')->willReturn([]);
         $this->userDataHandler->method('findLocalUser')->willReturn($user);
 
         $this->userDataHandler->expects($this->never())->method('updateLocalUser');
@@ -69,6 +69,11 @@ class BpnAuthenticatorTest extends TestCase
         $this->assertSame($user, $this->loadUser());
     }
 
+    /**
+     * This guard is what stands between a degraded response and a mass revocation: with the
+     * roles led by the CRM, a login that reached updateLocalUser() on an empty payload would
+     * strip every role of every user logging in, one at a time.
+     */
     public function testResponseWithoutAttributeGroupsRefusesTheLoginWithoutBlocking(): void
     {
         $user = (new User())->setRoles(['ROLE_ADMIN']);
@@ -76,14 +81,19 @@ class BpnAuthenticatorTest extends TestCase
         // an empty payload carries no roles either and must not read as a revocation
         $this->stubApiClient(new CrmAttributesResponse());
 
-        $this->userDataHandler->method('collectRoles')->willReturn([]);
+        $this->userDataHandler->method('collectClaimedRoles')->willReturn([]);
         $this->userDataHandler->method('findLocalUser')->willReturn($user);
 
         $this->userDataHandler->expects($this->never())->method('disableForRevokedCrmRoles');
+        $this->userDataHandler->expects($this->never())->method('updateLocalUser');
 
-        $this->expectException(UserNotFoundException::class);
+        try {
+            $this->loadUser();
+            $this->fail('Expected the login to be refused');
+        } catch (UserNotFoundException) {
+        }
 
-        $this->loadUser();
+        $this->assertSame(['ROLE_ADMIN'], $user->getAssignedRoles());
     }
 
     /**
@@ -97,7 +107,7 @@ class BpnAuthenticatorTest extends TestCase
 
         $this->stubApiClient($this->createCrmAttributes());
 
-        $this->userDataHandler->method('collectRoles')->willReturn(['ROLE_TEAMER']);
+        $this->userDataHandler->method('collectClaimedRoles')->willReturn(['ROLE_TEAMER']);
         $this->userDataHandler->method('findLocalUser')->willReturn($user);
 
         $this->userDataHandler->expects($this->never())->method('updateLocalUser');
@@ -115,7 +125,7 @@ class BpnAuthenticatorTest extends TestCase
 
         $this->stubApiClient($this->createCrmAttributes());
 
-        $this->userDataHandler->method('collectRoles')->willReturn([]);
+        $this->userDataHandler->method('collectClaimedRoles')->willReturn([]);
         $this->userDataHandler->method('findLocalUser')->willReturn($user);
 
         $this->userDataHandler->expects($this->never())->method('disableForRevokedCrmRoles');

tests/Security/MyEpAuthenticatorTest.php

@@ -110,9 +110,10 @@ class MyEpAuthenticatorTest extends TestCase
     }
 
     /**
-     * A super admin's manual demotion has to survive the user's next SSO login.
+     * A claimed administrative role is marked for approval, never granted - not even when
+     * the identity provider reports it outright.
      */
-    public function testExistingGrantedRolesAreNeverOverwritten(): void
+    public function testClaimedAdministrativeRolesAreNeverGrantedOnLogin(): void
     {
         $user = (new User())->setEmail('[email protected]')->setRoles(['ROLE_TEAMER']);
         $this->repository->method('findOneBy')->willReturn($user);
@@ -130,7 +131,7 @@ class MyEpAuthenticatorTest extends TestCase
     /**
      * An already approved role must not be demoted back to a marker on the next login.
      */
-    public function testAnAlreadyGrantedAdministrativeRoleIsKept(): void
+    public function testAnAlreadyGrantedAdministrativeRoleIsKeptWhileStillClaimed(): void
     {
         $user = (new User())->setEmail('[email protected]')->setRoles(['ROLE_ADMIN', 'ROLE_TEAMER']);
         $this->repository->method('findOneBy')->willReturn($user);
@@ -141,6 +142,32 @@ class MyEpAuthenticatorTest extends TestCase
         $this->assertSame([], $user->getPendingRoles());
     }
 
+    /**
+     * MyE&P leads exactly as BusPro does: a role it stops reporting is withdrawn on the
+     * next login, and the hotel codes are re-imported with it.
+     */
+    public function testGrantedRolesNoLongerClaimedAreRevoked(): void
+    {
+        $user = (new User())
+            ->setEmail('[email protected]')
+            ->setRoles(['ROLE_ADMIN', 'ROLE_TEAMER'])
+            ->setSuperAdmin(true)
+            ->setHotelCodes(['SSL'])
+        ;
+        $this->repository->method('findOneBy')->willReturn($user);
+
+        $this->loadUser($this->createUserinfo(['ROLE_TEAMER']));
+
+        $this->assertSame(['ROLE_TEAMER'], $user->getAssignedRoles());
+        $this->assertSame([], $user->getPendingRoles());
+
+        // the flag would otherwise outlive the role it depends on
+        $this->assertFalse($user->isSuperAdmin());
+        $this->assertNotContains('ROLE_SUPER_ADMIN', $user->getRoles());
+
+        $this->assertSame(['HOTEL'], $user->getHotelCodes());
+    }
+
     public function testTeamerRoleIsGrantedToAnExistingUser(): void
     {
         $user = (new User())->setEmail('[email protected]')->setRoles([User::PENDING_ROLES['ROLE_MANAGER']]);