diff --git a/docs/user-roles.md b/docs/user-roles.md index eb1c1da..7587ac5 100644 --- a/docs/user-roles.md +++ b/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 -> admin turns a nomination into a privilege. The CRM's word alone is enough to *take away* -> access, never to hand it out. +> **BusPro is the source of truth for roles and hotel codes.** Both are synced on every +> login, so anything it no longer reports is withdrawn. +> +> **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_ADMIN` | Admin | super admin, manually | ⇒ `ROLE_ADMINISTRATIVE` | -| `ROLE_MANAGER` | Reisemanager | super admin, manually | ⇒ `ROLE_ADMINISTRATIVE` | -| `ROLE_HOUSE_MANAGER` | Hausleitung | super admin, manually | — | -| `ROLE_TEAMER` | Teamer | the CRM, automatically | — | +| Role | Label | Granted by | Revoked by | Hierarchy | +|------|-------|-----------|------------|-----------| +| `ROLE_ADMIN` | Admin | super admin, approving a CRM claim | the CRM, automatically | ⇒ `ROLE_ADMINISTRATIVE` | +| `ROLE_MANAGER` | Reisemanager | super admin, approving a CRM claim | the CRM, automatically | ⇒ `ROLE_ADMINISTRATIVE` | +| `ROLE_HOUSE_MANAGER` | Hausleitung | super admin, approving a CRM claim | 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 -`isHouseManager`, they are never both claimed, though the hotel codes of a Hausleitung are -imported eitherway. `collectRoles()` is that set plus a real `ROLE_TEAMER` when `isTeamer`. +`UserDataHandler::collectClaimedRoles()` turns those four booleans into plain role names - +what the CRM says. Two things are made of that list: `toPendingRoles()` produces one marker +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 - marker whose real role is already granted is dropped — an approved role is never marked - again. Granted roles are kept untouched. -- **`grantTeamerRole()`** adds `ROLE_TEAMER` if the CRM reports a teamer and the user does - not have it yet. This is deliberately *grant-only*: the role is never withdrawn here, - because it may have been handed out manually and must survive a login. Losing the CRM - teamer attribute while holding no other role blocks the account anyway (see 4). +1. **revoke** every granted role the CRM no longer claims. This is what makes BusPro the + source of truth, and it applies to `ROLE_TEAMER` as much as to the administrative roles. +2. **clear the super admin flag** when `ROLE_ADMIN` was among them — `ROLE_SUPER_ADMIN` is + synthesized from a separate column and would otherwise survive its own precondition. +3. **`refreshPendingRoles()`** recomputes the marker set from the current claims. A marker + whose real role is already granted is dropped — an approved role is never marked again. + 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 -afterwards. Nothing on this path can raise a privilege. +**`syncHotelCodes()`** then replaces the hotel codes with the ones the CRM reports, so a +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**. -Consequences to be aware of: +The page keeps three things apart, because they follow three different rules: -- Approving is "tick the real role and save". The marker disappears because the column is - rewritten from the ticked choices. -- Saving the form drops *every* marker, including ones you did not act on. -- A denial is not recorded anywhere. As long as the CRM keeps claiming the role, the marker - returns on that user's next login. +| Block | What it is | +|-------|-----------| +| **Aus BusPro** | information, not a form: granted roles, houses, last login. Synced on every login and editable nowhere in this application | +| **Freischaltung** | one action per nomination — a button, a confirmation dialog, its own route (`ApproveRoleController`). Only shown when the user carries a marker | +| **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 -this application, so they are not a user of it: +user is even loaded. If `collectClaimedRoles()` is empty, the CRM grants this person nothing +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; diff --git a/src/BusProNet/UserDataHandler.php b/src/BusProNet/UserDataHandler.php index 25480c9..30af5d6 100644 --- a/src/BusProNet/UserDataHandler.php +++ b/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 - * shared with the MyE&P SSO login (see App\Security\MyEpAuthenticator), so that no - * identity source can grant an administrative role this application would not. + * toPendingRoles() and everything syncRoles() composes - are deliberately shared with + * the MyE&P SSO login (see App\Security\MyEpAuthenticator), so that no identity source + * 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)) { - $roles[] = User::PENDING_ROLES['ROLE_ADMIN']; - } - - // 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']; + foreach (User::PENDING_ROLES as $role => $pendingRole) { + if (true === in_array($role, $claimedRoles, true)) { + $roles[] = $pendingRole; + } } 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 - * managed manually afterwards, so they are intentionally left untouched here. The - * 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. + * The CRM leads: roles and hotel codes are re-synced on every login, so a role or a + * house it no longer reports is gone. Granting still needs approval - see syncRoles(). * - * @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 - * blocks the account anyway, and a role handed out manually must survive a login. + * Grant-only in itself - withdrawing it is revokeUnclaimedRoles()' business, which + * syncRoles() runs first. * * Does not flush; the caller decides when to. */ diff --git a/src/Controller/Admin/System/User/ApproveRoleController.php b/src/Controller/Admin/System/User/ApproveRoleController.php new file mode 100644 index 0000000..5569903 --- /dev/null +++ b/src/Controller/Admin/System/User/ApproveRoleController.php @@ -0,0 +1,55 @@ +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], + ]); + } +} diff --git a/src/Controller/Admin/System/User/EditController.php b/src/Controller/Admin/System/User/EditController.php index 21abc73..ec2a9bd 100644 --- a/src/Controller/Admin/System/User/EditController.php +++ b/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, ]); } } diff --git a/src/Entity/User.php b/src/Entity/User.php index d22728a..fc82afa 100644 --- a/src/Entity/User.php +++ b/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 + */ + 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. */ diff --git a/src/Form/UserType.php b/src/Form/UserType.php index 9fbfd2f..41a695b 100644 --- a/src/Form/UserType.php +++ b/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 - // added as choices to keep them selectable instead of failing - $builder->addEventListener(FormEvents::PRE_SET_DATA, function (FormEvent $event): void { + // Super admin is an elevation of an existing role, never a grant of its own, so the + // field exists only for somebody who already holds ROLE_ADMIN - offering it to + // 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(); - foreach ($user instanceof User ? $user->getHotelCodes() : [] as $code) { - if (false === in_array($code, $choices, true)) { - $choices[$code] = $code; - } + if (false === $user instanceof User) { + return; } - $event->getForm()->add('hotelCodes', MultiselectType::class, [ - 'label' => 'Häuser', + if (false === in_array('ROLE_ADMIN', $user->getAssignedRoles(), true) && false === $user->isSuperAdmin()) { + return; + } + + $event->getForm()->add('superAdmin', CheckboxType::class, [ + 'label' => 'Superadmin', 'required' => false, - 'choices' => $choices, - 'empty_label' => 'keine Häuser', + 'help' => 'Darf Rollen freischalten und Accounts verwalten.', ]); }); } diff --git a/src/Security/BpnAuthenticator.php b/src/Security/BpnAuthenticator.php index f1c1975..b8b06db 100644 --- a/src/Security/BpnAuthenticator.php +++ b/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 - // codes alone: they are imported once on creation and managed manually after + // Update the existing user, re-syncing roles and hotel codes from the CRM: it + // 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(), diff --git a/src/Security/MyEpAuthenticator.php b/src/Security/MyEpAuthenticator.php index 60ec942..baee9f2 100644 --- a/src/Security/MyEpAuthenticator.php +++ b/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 - // imported once on creation and managed manually afterwards. Only the - // privilege-free markers and ROLE_TEAMER track MyE&P on every login. + // Update existing user. MyE&P leads here exactly as BusPro does on the other login + // path - the shared policy in UserDataHandler withdraws what is no longer claimed + // and marks administrative claims for approval rather than granting them. if (null !== $user) { $this->refreshBusProIds($user, $userinfo); - $this->userDataHandler->refreshPendingRoles($user, $pendingRoles); - - if (true === $isTeamer) { - $this->userDataHandler->grantTeamerRole($user); - } + $this->userDataHandler->syncRoles($user, $claimedRoles); + $this->userDataHandler->syncHotelCodes($user, $hotelCodes); $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 diff --git a/templates/admin/system/user/_form.html.twig b/templates/admin/system/user/_form.html.twig index 8554824..19110e3 100644 --- a/templates/admin/system/user/_form.html.twig +++ b/templates/admin/system/user/_form.html.twig @@ -1,8 +1,9 @@ {{ form_start(form) }}
- {{ form_row(form.roles) }} - {{ form_row(form.superAdmin) }} - {{ form_row(form.hotelCodes) }} + {# only present for somebody who already holds ROLE_ADMIN, see UserType #} + {% if form.superAdmin is defined %} + {{ form_row(form.superAdmin) }} + {% endif %} {{ form_row(form.disabled) }} {{ form_row(form.disabledReason) }} {{ form_row(form.disabledReasonInternal) }} diff --git a/templates/admin/system/user/edit.html.twig b/templates/admin/system/user/edit.html.twig index 538df25..72507a6 100644 --- a/templates/admin/system/user/edit.html.twig +++ b/templates/admin/system/user/edit.html.twig @@ -7,6 +7,85 @@

{{ user.fullName }} bearbeiten

+ +
+

+ Aus BusPro +

+

+ Wird bei jeder Anmeldung übernommen und kann hier nicht geändert werden. +

+
+
+
+ Zugewiesene Rollen +
+
+ {{ grantedRoles|join(', ')|default('keine Rolle') }} + {% if user.superAdmin %} +
+ Superadmin +
+ {% endif %} +
+
+
+
+ Zugewiesene Häuser +
+
+ {% for code, name in houses %} +
+ {{ name }} ({{ code }}) +
+ {% else %} + - + {% endfor %} +
+
+
+
+ Letzter Login +
+
+ {{ user.lastLoginAt ? user.lastLoginAt|date('d.m.Y, H:i') : '-' }} +
+
+
+
+ + {# 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 %} +
+

+ Freischaltung +

+

+ In BusPro beansprucht, aber noch nicht freigeschaltet. +

+
    + {% for role, label in user.nominatedRoles %} +
  • + + {{ label }} + + +
    • + {% endfor %} +
+
+ {% endif %} + +

+ Account +

{% include 'admin/system/user/_form.html.twig' %}
{% endblock %} diff --git a/templates/admin/system/user/index.html.twig b/templates/admin/system/user/index.html.twig index b4893cf..f32c83e 100644 --- a/templates/admin/system/user/index.html.twig +++ b/templates/admin/system/user/index.html.twig @@ -48,7 +48,13 @@ {{ user.email }} - {{ 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 %} + + {{ label }} angefordert + + {% endfor %} {% if user.superAdmin %} @@ -91,4 +97,4 @@ -{% endblock %} \ No newline at end of file +{% endblock %} diff --git a/templates/admin/system/user/modal_approve_role.html.twig b/templates/admin/system/user/modal_approve_role.html.twig new file mode 100644 index 0000000..6ea611b --- /dev/null +++ b/templates/admin/system/user/modal_approve_role.html.twig @@ -0,0 +1,12 @@ +{% extends 'htmx_confirmation_modal.html.twig' %} + +{% block content %} +
+ Möchtest du die Rolle {{ roleLabel }} für {{ user.fullName }} wirklich freischalten? +
+
+ Die Rolle wird damit sofort wirksam. Sie wird automatisch wieder entzogen, sobald sie in BusPro nicht mehr beansprucht wird. +
+{% endblock %} + +{% block button_confirm %}Freischalten{% endblock %} diff --git a/tests/BusProNet/UserDataHandlerTest.php b/tests/BusProNet/UserDataHandlerTest.php index e0e8aa9..fd37339 100644 --- a/tests/BusProNet/UserDataHandlerTest.php +++ b/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('new@example.com', $user->getEmail()); - // roles and hotel codes are imported on creation only and stay under manual control - $this->assertSame(['XYZ'], $user->getHotelCodes()); + // the CRM leads: the still claimed role survives, the houses are replaced by its own + $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' => [ - ['ROLE_ADMIN'], + yield 'a granted role is revoked once the CRM stops claiming it' => [ + ['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('user@example.com') + ->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('user@example.com') + ->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('user@example.com') + ->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('user@example.com') + ->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('user@example.com') + ->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()) diff --git a/tests/Entity/UserTest.php b/tests/Entity/UserTest.php index 7fc554f..fa27f0e 100644 --- a/tests/Entity/UserTest.php +++ b/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()); + } } diff --git a/tests/Form/UserTypeTest.php b/tests/Form/UserTypeTest.php index 83f3c3c..9c7ff5a 100644 --- a/tests/Form/UserTypeTest.php +++ b/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); - - // the marker is not an assignable choice and must not reach the field - $this->assertSame([], $form->createView()->children['roles']->vars['data']); + $this->assertTrue($form->has('superAdmin')); $form->submit([ - 'roles' => ['ROLE_ADMIN'], 'superAdmin' => null, - 'hotelCodes' => [], + 'disabled' => null, ]); $this->assertTrue($form->isValid()); - $this->assertSame(['ROLE_ADMIN'], $user->getAssignedRoles()); - $this->assertSame([], $user->getPendingRoles()); + $this->assertFalse($user->isSuperAdmin()); } - public function testRendersHotelCodesNotCoveredByTheConfiguredMap(): void + public function testSuperAdminIsAppointed(): void { - $user = (new User())->setHotelCodes(['XYZ']); - - $view = $this->createForm($user)->createView(); - - $this->assertSame(['XYZ'], $view->children['hotelCodes']->vars['data']); - } - - public function testSubmitStoresAssignedRolesOnly(): void - { - $user = (new User())->setRoles(['ROLE_TEAMER']); + $user = (new User())->setRoles(['ROLE_ADMIN']); $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.', diff --git a/tests/Security/BpnAuthenticatorTest.php b/tests/Security/BpnAuthenticatorTest.php index 4d83269..8c96fbb 100644 --- a/tests/Security/BpnAuthenticatorTest.php +++ b/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'); diff --git a/tests/Security/MyEpAuthenticatorTest.php b/tests/Security/MyEpAuthenticatorTest.php index 953432a..f6d673c 100644 --- a/tests/Security/MyEpAuthenticatorTest.php +++ b/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('user@example.com')->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('user@example.com')->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('user@example.com') + ->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('user@example.com')->setRoles([User::PENDING_ROLES['ROLE_MANAGER']]);