diff --git a/docs/api-consumer-guide.md b/docs/api-consumer-guide.md index 7df06f6..a4b0fca 100644 --- a/docs/api-consumer-guide.md +++ b/docs/api-consumer-guide.md @@ -381,7 +381,7 @@ OIDC-style claims for the authenticated user. The response contains only the cla - always: `email` - scope `id`: `person_id`, `address_id` -- scope `roles`: `roles` (array; only the roles that actually grant something are exported — the implicit baseline role is stripped, and so are the `*_PENDING` markers of roles the BusPro CRM claims but nobody has approved yet, see `docs/user-roles.md`) +- scope `roles`: `roles` (array; only the roles that actually grant something are exported — the implicit baseline role is stripped, and so are the `*_PENDING` markers of roles the BusPro CRM claims but nobody has approved yet, see `docs/buspronet-schema/crm-selection-queries.md#from-claim-to-role`) - scope `profile`: `profile` object: ```json diff --git a/docs/bpn-connect-query-reference.md b/docs/bpn-connect-query-reference.md index 7d35e1b..6a3cc46 100644 --- a/docs/bpn-connect-query-reference.md +++ b/docs/bpn-connect-query-reference.md @@ -577,6 +577,10 @@ Known role mapping from the XML parser: - admin users also receive default hotel code `SSL` - no matched role means `ROLE_CUSTOMER` +These are the CRM's claims. The portal filters them before storing anything (staff-only +roles, approval markers), so a proxy must return the claims unfiltered — see +`docs/buspronet-schema/crm-selection-queries.md#from-claim-to-role`. + Candidate SQL for selected customer selections: ```sql diff --git a/docs/buspronet-schema/crm-selection-queries.md b/docs/buspronet-schema/crm-selection-queries.md index caf384f..8747022 100644 --- a/docs/buspronet-schema/crm-selection-queries.md +++ b/docs/buspronet-schema/crm-selection-queries.md @@ -29,6 +29,35 @@ Symfony roles: Only selections with `auswahl="True"` count. Every `Admin/*` controller is gated on one of these roles. +These are *claims*, not grants. What an account ends up holding is decided by +`Role::sync()` (`src/Security/Role.php`) on every login — see +[From claim to role](#from-claim-to-role). + +--- + +## From claim to role + +BusPro backend users can edit their own CRM selections, so the claims above are +filtered before anything is stored: + +| Role | Claimed without a staff address | Claimed with a staff address | +| --- | --- | --- | +| `ROLE_TEAMER` | granted | granted | +| `ROLE_HOUSE_MANAGER` | nominated | nominated | +| `ROLE_ADMIN`, `ROLE_TEAM_ADMIN`, `ROLE_MANAGER`, `ROLE_GROUPS_ADMIN`, `ROLE_GROUPS_MANAGER`, `ROLE_CUSTOMER_EXPERT` (`Role::EMPLOYEE_ONLY`) | **ignored** | nominated | + +- **Staff address**: the login email's domain is listed in `employee_email_domains` + (`config/services.yaml`, exact match, never a suffix). Such an account also gets + `ROLE_EMPLOYEE`, which is derived from the address and not from the CRM. +- **Nominated**: stored as the marker `ROLE_X_PENDING`, which grants nothing until an + administrator approves it in `/admin/user`. Nominations are announced to + `role_nomination_notification_emails`. +- **Ignored**: treated exactly like a selection that is not set. No marker is stored, + and a role approved earlier is **revoked** on the next login. A leftover marker from + before the rule cannot be approved and disappears on that login too. +- Anything the CRM stops claiming is revoked, approved or not. +- An account left without any effective role gets `ROLE_CUSTOMER`. + --- ## The four tables @@ -398,11 +427,11 @@ BusProNet host is the fix, and no schema change is needed. ## Portal side: a correct XML is not enough -`BpnAuthenticator::createOrUpdateLocalUser()` snapshots the roles onto the local `User` -entity at login (`setRoles($crmAttributes->roles)` / -`setHotelCodes($crmAttributes->hotelCodes)`). Roles are **not** re-read per request. So -after fixing BusPro the affected user must **log out and log in again** before -`ROLE_GROUPS_ADMIN` takes effect — an active session keeps the stale role set. +`BpnAuthenticator::syncFromCrm()` writes the roles onto the local `User` entity at login, +through `Role::sync()` (see [From claim to role](#from-claim-to-role)), and the hotel codes +verbatim. Roles are **not** re-read per request. So after fixing BusPro the affected user +must **log out and log in again**, and even then `ROLE_GROUPS_ADMIN` only takes effect once +the account has a staff address and an administrator has approved the nomination. --- diff --git a/src/Security/BpnAuthenticator.php b/src/Security/BpnAuthenticator.php index e09bde9..b93110b 100644 --- a/src/Security/BpnAuthenticator.php +++ b/src/Security/BpnAuthenticator.php @@ -182,9 +182,11 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent ->setHotelCodes(array_values(array_unique($crmAttributes->hotelCodes))) ; + // Compared on the full role sets rather than on the markers alone: nominatedFrom() needs + // to see ROLE_EMPLOYEE to know whether an EMPLOYEE_ONLY marker counts. $nominated = array_values(array_diff( - Role::pendingOnly($user->getRoles()), - Role::pendingOnly($previousRoles), + array_keys(Role::nominatedFrom($user->getRoles())), + array_keys(Role::nominatedFrom($previousRoles)), )); if ([] === $nominated) { @@ -198,9 +200,9 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent 'roles' => $nominated, ]); - // Only the newly appeared markers reach this point, so a repeat login with a nomination - // still standing announces nothing. That difference is the whole de-duplication. - return array_keys(Role::nominatedFrom($nominated)); + // Only the newly appeared nominations reach this point, so a repeat login with a + // nomination still standing announces nothing. That difference is the whole de-duplication. + return $nominated; } public function onAuthenticationSuccess(Request $request, TokenInterface $token, string $firewallName): ?Response diff --git a/src/Security/Role.php b/src/Security/Role.php index f0f951f..e5a7ffe 100644 --- a/src/Security/Role.php +++ b/src/Security/Role.php @@ -23,6 +23,10 @@ namespace App\Security; * That widens a claim from "what the CRM reports" to "what the CRM reports plus what the account * itself implies", and nothing more: ROLE_EMPLOYEE is not administrative, so it cannot reach the * nomination path, and the CRM remains the only source for every role that grants privileges. + * + * ROLE_EMPLOYEE does gate a few administrative roles, though (see EMPLOYEE_ONLY): for those, a + * CRM claim only counts when the account is staff. It narrows what the CRM can nominate and + * never widens it — the approval step still applies on top. */ final class Role { @@ -33,6 +37,7 @@ final class Role public const USER = 'ROLE_USER'; public const ADMIN = 'ROLE_ADMIN'; + public const TEAM_ADMIN = 'ROLE_TEAM_ADMIN'; public const MANAGER = 'ROLE_MANAGER'; public const TEAMER = 'ROLE_TEAMER'; public const CUSTOMER = 'ROLE_CUSTOMER'; @@ -58,6 +63,7 @@ final class Role */ public const ALL = [ self::ADMIN, + self::TEAM_ADMIN, self::MANAGER, self::TEAMER, self::CUSTOMER, @@ -90,6 +96,7 @@ final class Role */ public const ADMINISTRATIVE = [ self::ADMIN, + self::TEAM_ADMIN, self::MANAGER, self::HOUSE_MANAGER, self::GROUPS_ADMIN, @@ -97,6 +104,25 @@ final class Role self::CUSTOMER_EXPERT, ]; + /** + * Administrative roles reserved for staff accounts. A CRM claim for one of these is ignored + * unless the account is also claimed as ROLE_EMPLOYEE, so it neither nominates nor keeps a + * role approved earlier. BusPro backend users outside the company can edit their own CRM + * selections too, and these roles reach far enough that the email domain has to agree. + * ROLE_HOUSE_MANAGER is the one administrative role left out: a Hausleitung signs in with + * the hotel's own address. + * + * @var string[] + */ + public const EMPLOYEE_ONLY = [ + self::ADMIN, + self::TEAM_ADMIN, + self::MANAGER, + self::GROUPS_ADMIN, + self::GROUPS_MANAGER, + self::CUSTOMER_EXPERT, + ]; + /** * Roles nobody may approve for their own account. ROLE_ADMIN outranks every check in this * application, including the approval surface itself, so it always takes a second @@ -159,7 +185,9 @@ final class Role } /** - * The roles behind those markers, labelled — what an approver acts on. + * The roles behind those markers, labelled — what an approver acts on. A marker for an + * EMPLOYEE_ONLY role on an account that is not staff is left out: it predates the rule and + * goes on that account's next login, and until then it must not be approvable. * * @param string[] $roles * @@ -170,9 +198,15 @@ final class Role $labels = self::labels(); $nominated = []; + $isEmployee = \in_array(self::EMPLOYEE, $roles, true); + foreach (self::pendingOnly($roles) as $marker) { $role = self::realRole($marker); + if (false === $isEmployee && \in_array($role, self::EMPLOYEE_ONLY, true)) { + continue; + } + if (\in_array($role, self::ADMINISTRATIVE, true)) { $nominated[$role] = $labels[$role]; } @@ -184,6 +218,8 @@ final class Role /** * The whole policy, applied on every login. * + * 0. drop the claims for EMPLOYEE_ONLY roles unless ROLE_EMPLOYEE is claimed as well — they + * then count as not made, so the steps below revoke and never mark them; * 1. revoke everything the CRM no longer claims — granted roles and markers alike, which is * what makes BusPro the source of truth; * 2. grant the unconditional roles it claims; @@ -204,6 +240,10 @@ final class Role { $claimed = array_values(array_intersect(self::ALL, array_unique($claimedRoles))); + if (false === \in_array(self::EMPLOYEE, $claimed, true)) { + $claimed = array_values(array_diff($claimed, self::EMPLOYEE_ONLY)); + } + $roles = array_values(array_filter( self::assignedOnly($storedRoles), static fn (string $role): bool => \in_array(self::realRole($role), $claimed, true), @@ -224,8 +264,8 @@ final class Role /** * Turns a marker into the role it stands for. Refuses anything the account is not nominated - * for, so neither a hand-crafted request nor a claim revoked while the confirmation dialog - * was open can grant a role the CRM never reported. + * for (see nominatedFrom()), so neither a hand-crafted request nor a claim revoked while the + * confirmation dialog was open can grant a role the CRM never reported. * * @param string[] $storedRoles * @@ -237,7 +277,7 @@ final class Role { $roles = self::assignedOnly($storedRoles); - if (false === \in_array(self::pending($role), $roles, true)) { + if (false === \array_key_exists($role, self::nominatedFrom($roles))) { throw new \InvalidArgumentException(sprintf('The role "%s" is not pending approval.', $role)); } @@ -284,6 +324,7 @@ final class Role { $labels = [ self::ADMIN => 'Administration', + self::TEAM_ADMIN => 'Team Administration', self::MANAGER => 'Manager:in', self::TEAMER => 'Teamer:in', self::CUSTOMER => 'Kund:in', diff --git a/tests/Controller/Admin/User/ApproveRoleControllerTest.php b/tests/Controller/Admin/User/ApproveRoleControllerTest.php index 86ce260..9e8a18c 100644 --- a/tests/Controller/Admin/User/ApproveRoleControllerTest.php +++ b/tests/Controller/Admin/User/ApproveRoleControllerTest.php @@ -33,11 +33,11 @@ class ApproveRoleControllerTest extends TestCase $controller = new TestableApproveRoleController($entityManager, $this->createStub(LoggerInterface::class)); - $response = $controller->index($user, Role::GROUPS_ADMIN, Request::create('/admin/user/1/approve/ROLE_GROUPS_ADMIN')); + $response = $controller->index($user, Role::HOUSE_MANAGER, Request::create('/admin/user/1/approve/ROLE_HOUSE_MANAGER')); self::assertSame(Response::HTTP_OK, $response->getStatusCode()); self::assertSame('admin/user/modal_approve_role.html.twig', $controller->renderedView); - self::assertSame([Role::TEAMER, Role::pending(Role::GROUPS_ADMIN)], Role::assignedOnly($user->getRoles())); + self::assertSame([Role::TEAMER, Role::pending(Role::HOUSE_MANAGER)], Role::assignedOnly($user->getRoles())); } public function testPostGrantsTheRoleAndRedirectsTheBrowser(): void @@ -49,9 +49,9 @@ class ApproveRoleControllerTest extends TestCase $controller = new TestableApproveRoleController($entityManager, $this->createStub(LoggerInterface::class)); - $response = $controller->index($user, Role::GROUPS_ADMIN, Request::create('/admin/user/1/approve/ROLE_GROUPS_ADMIN', 'POST')); + $response = $controller->index($user, Role::HOUSE_MANAGER, Request::create('/admin/user/1/approve/ROLE_HOUSE_MANAGER', 'POST')); - self::assertSame([Role::TEAMER, Role::GROUPS_ADMIN], Role::assignedOnly($user->getRoles())); + self::assertSame([Role::TEAMER, Role::HOUSE_MANAGER], Role::assignedOnly($user->getRoles())); self::assertTrue($response->headers->has('HX-Redirect')); self::assertSame(['success'], array_column($controller->flashes, 'type')); } @@ -94,9 +94,9 @@ class ApproveRoleControllerTest extends TestCase // Only ROLE_ADMIN needs a second pair of eyes — an approver already holds it, so the // rest grant less than they could grant themselves anyway. - $controller->index($user, Role::GROUPS_ADMIN, Request::create('/admin/user/1/approve/ROLE_GROUPS_ADMIN', 'POST')); + $controller->index($user, Role::HOUSE_MANAGER, Request::create('/admin/user/1/approve/ROLE_HOUSE_MANAGER', 'POST')); - self::assertSame([Role::TEAMER, Role::GROUPS_ADMIN], Role::assignedOnly($user->getRoles())); + self::assertSame([Role::TEAMER, Role::HOUSE_MANAGER], Role::assignedOnly($user->getRoles())); } public function testPostWithAnInvalidTokenIsDenied(): void @@ -108,7 +108,7 @@ class ApproveRoleControllerTest extends TestCase $this->expectException(AccessDeniedException::class); - $controller->index($this->nominatedUser(), Role::GROUPS_ADMIN, Request::create('/admin/user/1/approve/ROLE_GROUPS_ADMIN', 'POST')); + $controller->index($this->nominatedUser(), Role::HOUSE_MANAGER, Request::create('/admin/user/1/approve/ROLE_HOUSE_MANAGER', 'POST')); } public function testApprovingForYourselfReissuesTheSecurityToken(): void @@ -125,12 +125,12 @@ class ApproveRoleControllerTest extends TestCase tokenStorage: $tokenStorage, ); - $controller->index($user, Role::GROUPS_ADMIN, Request::create('/admin/user/1/approve/ROLE_GROUPS_ADMIN', 'POST')); + $controller->index($user, Role::HOUSE_MANAGER, Request::create('/admin/user/1/approve/ROLE_HOUSE_MANAGER', 'POST')); // Without this the next request would find the stored roles out of step with the token // and end the session, logging the approver out mid-action. - self::assertContains(Role::GROUPS_ADMIN, $tokenStorage->getToken()?->getRoleNames() ?? []); - self::assertNotContains(Role::pending(Role::GROUPS_ADMIN), $tokenStorage->getToken()?->getRoleNames() ?? []); + self::assertContains(Role::HOUSE_MANAGER, $tokenStorage->getToken()?->getRoleNames() ?? []); + self::assertNotContains(Role::pending(Role::HOUSE_MANAGER), $tokenStorage->getToken()?->getRoleNames() ?? []); } public function testApprovingForSomebodyElseLeavesYourOwnTokenAlone(): void @@ -148,14 +148,14 @@ class ApproveRoleControllerTest extends TestCase tokenStorage: $tokenStorage, ); - $controller->index($other, Role::GROUPS_ADMIN, Request::create('/admin/user/1/approve/ROLE_GROUPS_ADMIN', 'POST')); + $controller->index($other, Role::HOUSE_MANAGER, Request::create('/admin/user/1/approve/ROLE_HOUSE_MANAGER', 'POST')); self::assertSame($originalToken, $tokenStorage->getToken()); } private function nominatedUser(): User { - return (new User('teamer@example.org'))->setRoles([Role::TEAMER, Role::pending(Role::GROUPS_ADMIN)]); + return (new User('teamer@example.org'))->setRoles([Role::TEAMER, Role::pending(Role::HOUSE_MANAGER)]); } } diff --git a/tests/Controller/Admin/User/ShowControllerTest.php b/tests/Controller/Admin/User/ShowControllerTest.php index 02b4ad2..a50eaf1 100644 --- a/tests/Controller/Admin/User/ShowControllerTest.php +++ b/tests/Controller/Admin/User/ShowControllerTest.php @@ -21,7 +21,7 @@ class ShowControllerTest extends TestCase $controller->index($this->nominatedUser(), $this->permissionsRequest()); self::assertSame( - [Role::MANAGER => 'Manager:in', Role::GROUPS_ADMIN => 'Preisrechner Admin'], + [Role::HOUSE_MANAGER => 'Hausleitung', Role::GROUPS_ADMIN => 'Preisrechner Admin'], $controller->parameters['approvableRoles'], ); self::assertSame([], $controller->parameters['selfRefusedRoles']); @@ -29,7 +29,7 @@ class ShowControllerTest extends TestCase public function testWhatCannotBeSelfApprovedIsNotOffered(): void { - $user = (new User('admin@example.org'))->setRoles([Role::ADMIN, Role::pending(Role::ADMIN), Role::pending(Role::MANAGER)]); + $user = (new User('admin@ep-reisen.de'))->setRoles([Role::EMPLOYEE, Role::ADMIN, Role::pending(Role::ADMIN), Role::pending(Role::MANAGER)]); $controller = new TestableShowController(currentUser: $user); @@ -62,8 +62,8 @@ class ShowControllerTest extends TestCase private function nominatedUser(): User { - return (new User('teamer@example.org')) - ->setRoles([Role::TEAMER, Role::pending(Role::MANAGER), Role::pending(Role::GROUPS_ADMIN)]); + return (new User('teamer@ep-reisen.de')) + ->setRoles([Role::TEAMER, Role::EMPLOYEE, Role::pending(Role::HOUSE_MANAGER), Role::pending(Role::GROUPS_ADMIN)]); } /** diff --git a/tests/Dashboard/PendingRoleApprovalsWidgetProviderTest.php b/tests/Dashboard/PendingRoleApprovalsWidgetProviderTest.php index ab37df6..13533cb 100644 --- a/tests/Dashboard/PendingRoleApprovalsWidgetProviderTest.php +++ b/tests/Dashboard/PendingRoleApprovalsWidgetProviderTest.php @@ -19,7 +19,7 @@ class PendingRoleApprovalsWidgetProviderTest extends TestCase $user = (new User('nominee@example.com')) ->setFirstName('Rita') ->setLastName('Vorschlag') - ->setRoles([Role::TEAMER, Role::pending(Role::GROUPS_ADMIN)]) + ->setRoles([Role::TEAMER, Role::pending(Role::HOUSE_MANAGER)]) ; $widget = $this->provider([$user])->build(); @@ -27,7 +27,7 @@ class PendingRoleApprovalsWidgetProviderTest extends TestCase $this->assertNotNull($widget); $this->assertCount(1, $widget->entries); $this->assertStringContainsString('Rita Vorschlag', $widget->entries[0]->label); - $this->assertStringContainsString(Role::labels()[Role::GROUPS_ADMIN], $widget->entries[0]->label); + $this->assertStringContainsString(Role::labels()[Role::HOUSE_MANAGER], $widget->entries[0]->label); } /** diff --git a/tests/Security/BpnAuthenticatorTest.php b/tests/Security/BpnAuthenticatorTest.php index 63fa43b..587ab2e 100644 --- a/tests/Security/BpnAuthenticatorTest.php +++ b/tests/Security/BpnAuthenticatorTest.php @@ -43,7 +43,7 @@ class BpnAuthenticatorTest extends TestCase { $persisted = null; $authenticator = $this->authenticator( - $this->crmAttributes([Role::ADMIN, Role::TEAMER], ['SSL', 'SSL']), + $this->crmAttributes([Role::HOUSE_MANAGER, Role::TEAMER], ['SSL', 'SSL']), null, $persisted, ); @@ -51,7 +51,7 @@ class BpnAuthenticatorTest extends TestCase $user = $this->loadUser($authenticator); self::assertSame($persisted, $user); - self::assertSame(['ROLE_USER', Role::TEAMER, Role::pending(Role::ADMIN)], $user->getRoles()); + self::assertSame(['ROLE_USER', Role::TEAMER, Role::pending(Role::HOUSE_MANAGER)], $user->getRoles()); self::assertSame(['SSL'], $user->getHotelCodes()); } @@ -61,7 +61,7 @@ class BpnAuthenticatorTest extends TestCase $persisted = null; $authenticator = $this->authenticator( - $this->crmAttributes([Role::TEAMER, Role::GROUPS_ADMIN], []), + $this->crmAttributes([Role::TEAMER, Role::HOUSE_MANAGER], []), $existing, $persisted, ); @@ -70,7 +70,7 @@ class BpnAuthenticatorTest extends TestCase self::assertSame($existing, $persisted, 'a login must not create a second account'); self::assertSame( - ['ROLE_USER', Role::TEAMER, Role::pending(Role::GROUPS_ADMIN)], + ['ROLE_USER', Role::TEAMER, Role::pending(Role::HOUSE_MANAGER)], $user->getRoles(), ); self::assertNotNull($user->getLastLoginAt(), 'the rest of the profile is still synced'); @@ -78,24 +78,24 @@ class BpnAuthenticatorTest extends TestCase public function testApprovedRoleSurvivesTheNextLogin(): void { - $existing = (new User('manager@example.org'))->setRoles([Role::TEAMER, Role::GROUPS_MANAGER]); + $existing = (new User('manager@example.org'))->setRoles([Role::TEAMER, Role::HOUSE_MANAGER]); $persisted = null; $authenticator = $this->authenticator( - $this->crmAttributes([Role::TEAMER, Role::GROUPS_MANAGER], []), + $this->crmAttributes([Role::TEAMER, Role::HOUSE_MANAGER], []), $existing, $persisted, ); self::assertSame( - ['ROLE_USER', Role::TEAMER, Role::GROUPS_MANAGER], + ['ROLE_USER', Role::TEAMER, Role::HOUSE_MANAGER], $this->loadUser($authenticator)->getRoles(), ); } public function testRoleRevokedInBusProIsWithdrawnOnLogin(): void { - $existing = (new User('manager@example.org'))->setRoles([Role::TEAMER, Role::GROUPS_MANAGER]); + $existing = (new User('manager@example.org'))->setRoles([Role::TEAMER, Role::HOUSE_MANAGER]); $persisted = null; $authenticator = $this->authenticator($this->crmAttributes([], []), $existing, $persisted); @@ -140,7 +140,7 @@ class BpnAuthenticatorTest extends TestCase public function testDegradedCrmResponseLeavesAnExistingAccountUntouched(): void { $existing = (new User('manager@example.org')) - ->setRoles([Role::TEAMER, Role::GROUPS_MANAGER]) + ->setRoles([Role::TEAMER, Role::HOUSE_MANAGER]) ->setHotelCodes(['DKS']) ; @@ -155,7 +155,7 @@ class BpnAuthenticatorTest extends TestCase $user = $this->loadUser($authenticator); - self::assertSame(['ROLE_USER', Role::TEAMER, Role::GROUPS_MANAGER], $user->getRoles()); + self::assertSame(['ROLE_USER', Role::TEAMER, Role::HOUSE_MANAGER], $user->getRoles()); self::assertSame(['DKS'], $user->getHotelCodes()); } @@ -240,12 +240,40 @@ class BpnAuthenticatorTest extends TestCase self::assertSame(['ROLE_USER', Role::CUSTOMER], $user->getRoles()); } + public function testEmployeeOnlyClaimFromAnotherDomainIsIgnored(): void + { + $persisted = null; + $authenticator = $this->authenticator($this->crmAttributes([Role::ADMIN, Role::TEAMER], []), null, $persisted); + + $user = $this->loadUser($authenticator, 'someone@example.org'); + + // Not even a nomination, so there is nothing for an administrator to be told about. + self::assertSame(['ROLE_USER', Role::TEAMER], $user->getRoles()); + self::assertSame([], $this->dispatched); + } + + public function testEmployeeOnlyRoleIsRevokedWhenTheAddressIsNotStaff(): void + { + $existing = (new User('someone@example.org'))->setRoles([Role::TEAMER, Role::ADMIN, Role::HOUSE_MANAGER]); + $persisted = null; + $authenticator = $this->authenticator( + $this->crmAttributes([Role::TEAMER, Role::ADMIN, Role::HOUSE_MANAGER], []), + $existing, + $persisted, + ); + + $user = $this->loadUser($authenticator, 'someone@example.org'); + + // Only the EMPLOYEE_ONLY role goes; a Hausleitung does not need a staff address. + self::assertSame(['ROLE_USER', Role::TEAMER, Role::HOUSE_MANAGER], $user->getRoles()); + } + public function testANewNominationIsAnnouncedOnce(): void { $persisted = null; $authenticator = $this->authenticator($this->crmAttributes([Role::ADMIN], []), null, $persisted); - $user = $this->loadUser($authenticator); + $user = $this->loadUser($authenticator, 'someone@ep-reisen.de'); self::assertCount(1, $this->dispatched); $message = $this->dispatched[0]; @@ -257,11 +285,11 @@ class BpnAuthenticatorTest extends TestCase public function testAStandingNominationIsNotAnnouncedAgain(): void { - $existing = (new User('teamer@example.org'))->setRoles([Role::pending(Role::ADMIN)]); + $existing = (new User('someone@ep-reisen.de'))->setRoles([Role::EMPLOYEE, Role::pending(Role::ADMIN)]); $persisted = null; $authenticator = $this->authenticator($this->crmAttributes([Role::ADMIN], []), $existing, $persisted); - $this->loadUser($authenticator); + $this->loadUser($authenticator, 'someone@ep-reisen.de'); // The nomination has not changed, so there is nothing new to tell an administrator about. self::assertSame([], $this->dispatched); @@ -269,11 +297,11 @@ class BpnAuthenticatorTest extends TestCase public function testAnApprovedRoleIsNotAnnouncedAsANomination(): void { - $existing = (new User('teamer@example.org'))->setRoles([Role::ADMIN]); + $existing = (new User('someone@ep-reisen.de'))->setRoles([Role::EMPLOYEE, Role::ADMIN]); $persisted = null; $authenticator = $this->authenticator($this->crmAttributes([Role::ADMIN], []), $existing, $persisted); - $this->loadUser($authenticator); + $this->loadUser($authenticator, 'someone@ep-reisen.de'); self::assertSame([], $this->dispatched); } diff --git a/tests/Security/RoleTest.php b/tests/Security/RoleTest.php index be821b3..318f9f3 100644 --- a/tests/Security/RoleTest.php +++ b/tests/Security/RoleTest.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace App\Tests\Security; use App\Security\Role; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; /** @@ -15,46 +16,46 @@ class RoleTest extends TestCase { public function testAdministrativeClaimOnlyProducesANomination(): void { - $roles = Role::sync([], [Role::ADMIN, Role::GROUPS_ADMIN, Role::TEAMER]); + $roles = Role::sync([], [Role::HOUSE_MANAGER, Role::GROUPS_ADMIN, Role::TEAMER, Role::EMPLOYEE]); self::assertSame( - [Role::TEAMER, Role::pending(Role::ADMIN), Role::pending(Role::GROUPS_ADMIN)], + [Role::TEAMER, Role::EMPLOYEE, Role::pending(Role::HOUSE_MANAGER), Role::pending(Role::GROUPS_ADMIN)], $roles, ); - self::assertSame([Role::TEAMER], Role::effectiveOnly($roles)); + self::assertSame([Role::TEAMER, Role::EMPLOYEE], Role::effectiveOnly($roles)); } public function testCustomerExpertClaimOnlyProducesANomination(): void { - $roles = Role::sync([], [Role::CUSTOMER_EXPERT]); + $roles = Role::sync([], [Role::EMPLOYEE, Role::CUSTOMER_EXPERT]); - self::assertSame([Role::pending(Role::CUSTOMER_EXPERT), Role::CUSTOMER], $roles); - self::assertSame([Role::CUSTOMER], Role::effectiveOnly($roles)); + self::assertSame([Role::EMPLOYEE, Role::pending(Role::CUSTOMER_EXPERT)], $roles); + self::assertSame([Role::EMPLOYEE], Role::effectiveOnly($roles)); self::assertSame( - [Role::CUSTOMER_EXPERT => 'KO-Experte'], + [Role::CUSTOMER_EXPERT => 'KO-Expert:in'], Role::nominatedFrom($roles), ); } - public function testApprovedCustomerExpertDisplacesTheCustomerFallback(): void + public function testCustomerExpertCanBeApprovedForStaff(): void { - $roles = Role::approve([Role::pending(Role::CUSTOMER_EXPERT), Role::CUSTOMER], Role::CUSTOMER_EXPERT); + $roles = Role::approve([Role::EMPLOYEE, Role::pending(Role::CUSTOMER_EXPERT)], Role::CUSTOMER_EXPERT); - self::assertSame([Role::CUSTOMER_EXPERT], $roles); + self::assertSame([Role::EMPLOYEE, Role::CUSTOMER_EXPERT], $roles); } public function testApprovedRoleSurvivesTheNextSyncAndIsNotMarkedAgain(): void { - $roles = Role::sync([Role::TEAMER, Role::GROUPS_ADMIN], [Role::GROUPS_ADMIN, Role::TEAMER]); + $roles = Role::sync([Role::TEAMER, Role::HOUSE_MANAGER], [Role::HOUSE_MANAGER, Role::TEAMER]); - self::assertSame([Role::TEAMER, Role::GROUPS_ADMIN], $roles); + self::assertSame([Role::TEAMER, Role::HOUSE_MANAGER], $roles); } public function testRoleTheCrmNoLongerClaimsIsRevoked(): void { // Both halves go: BusPro is the source of truth for the granted role as much as for // the nomination. - $roles = Role::sync([Role::TEAMER, Role::ADMIN, Role::pending(Role::MANAGER)], [Role::TEAMER]); + $roles = Role::sync([Role::TEAMER, Role::GROUPS_ADMIN, Role::pending(Role::HOUSE_MANAGER)], [Role::TEAMER]); self::assertSame([Role::TEAMER], $roles); } @@ -69,8 +70,8 @@ class RoleTest extends TestCase // The nomination stays visible — it is what an approver acts on — but grants nothing, // so the account is a customer in the meantime. self::assertSame( - [Role::pending(Role::ADMIN), Role::CUSTOMER], - Role::sync([], [Role::ADMIN]), + [Role::pending(Role::HOUSE_MANAGER), Role::CUSTOMER], + Role::sync([], [Role::HOUSE_MANAGER]), ); self::assertSame([Role::CUSTOMER], Role::sync([], [])); } @@ -90,10 +91,10 @@ class RoleTest extends TestCase public function testApprovalTurnsTheNominationIntoTheRole(): void { - $roles = Role::approve([Role::pending(Role::ADMIN), Role::CUSTOMER], Role::ADMIN); + $roles = Role::approve([Role::pending(Role::HOUSE_MANAGER), Role::CUSTOMER], Role::HOUSE_MANAGER); // The customer fallback goes with it: the account now holds an effective role. - self::assertSame([Role::ADMIN], $roles); + self::assertSame([Role::HOUSE_MANAGER], $roles); } public function testApprovingARoleWithoutANominationIsRefused(): void @@ -105,11 +106,11 @@ class RoleTest extends TestCase public function testEffectiveRolesExcludeNominationsAndTheImplicitRoleUser(): void { - $roles = [Role::USER, Role::TEAMER, Role::pending(Role::ADMIN)]; + $roles = [Role::USER, Role::TEAMER, Role::pending(Role::HOUSE_MANAGER)]; self::assertSame([Role::TEAMER], Role::effectiveOnly($roles)); - self::assertSame([Role::pending(Role::ADMIN)], Role::pendingOnly($roles)); - self::assertSame([Role::ADMIN => 'Administration'], Role::nominatedFrom($roles)); + self::assertSame([Role::pending(Role::HOUSE_MANAGER)], Role::pendingOnly($roles)); + self::assertSame([Role::HOUSE_MANAGER => 'Hausleitung'], Role::nominatedFrom($roles)); } public function testEmployeeIsGrantedOutrightAndDisplacesTheCustomerFallback(): void @@ -133,6 +134,61 @@ class RoleTest extends TestCase ); } + /** + * @return iterable + */ + public static function employeeOnlyRoles(): iterable + { + foreach (Role::EMPLOYEE_ONLY as $role) { + yield $role => [$role]; + } + } + + #[DataProvider('employeeOnlyRoles')] + public function testEmployeeOnlyClaimWithoutEmployeeIsNotNominated(string $role): void + { + self::assertSame([Role::TEAMER], Role::sync([], [Role::TEAMER, $role])); + } + + #[DataProvider('employeeOnlyRoles')] + public function testEmployeeOnlyRoleIsRevokedWithoutEmployee(string $role): void + { + // The CRM still claims it, but the account is no longer staff: the claim counts as not + // made, and a role approved back when it was staff goes with it. + self::assertSame( + [Role::TEAMER], + Role::sync([Role::TEAMER, Role::EMPLOYEE, $role], [Role::TEAMER, $role]), + ); + } + + #[DataProvider('employeeOnlyRoles')] + public function testEmployeeOnlyRoleSurvivesTheNextSyncForStaff(string $role): void + { + self::assertSame( + [Role::EMPLOYEE, $role], + Role::sync([Role::EMPLOYEE, $role], [Role::EMPLOYEE, $role]), + ); + } + + public function testHouseManagerDoesNotNeedEmployee(): void + { + // A Hausleitung signs in with the hotel's own address. + self::assertSame([Role::pending(Role::HOUSE_MANAGER), Role::CUSTOMER], Role::sync([], [Role::HOUSE_MANAGER])); + self::assertSame([Role::HOUSE_MANAGER], Role::sync([Role::HOUSE_MANAGER], [Role::HOUSE_MANAGER])); + } + + public function testStaleEmployeeOnlyNominationWithoutEmployeeCannotBeApproved(): void + { + // A marker from before the rule, still stored until the account's next login. + $roles = [Role::TEAMER, Role::pending(Role::ADMIN)]; + + self::assertSame([], Role::nominatedFrom($roles)); + + $this->expectException(\InvalidArgumentException::class); + + Role::approve($roles, Role::ADMIN); + } + public function testEmployeeAndTeamerCoexist(): void { self::assertSame( diff --git a/tests/Twig/AppRuntimeRoleLabelsTest.php b/tests/Twig/AppRuntimeRoleLabelsTest.php index b31548d..268f5c2 100644 --- a/tests/Twig/AppRuntimeRoleLabelsTest.php +++ b/tests/Twig/AppRuntimeRoleLabelsTest.php @@ -26,10 +26,10 @@ class AppRuntimeRoleLabelsTest extends TestCase public function testNominationsAreListedApartFromTheEffectiveRoles(): void { - $roles = [Role::TEAMER, Role::pending(Role::ADMIN)]; + $roles = [Role::TEAMER, Role::pending(Role::HOUSE_MANAGER)]; self::assertSame(['Teamer:in'], $this->runtime()->effectiveRoles($roles)); - self::assertSame(['Administration'], $this->runtime()->nominatedRoles($roles)); + self::assertSame(['Hausleitung'], $this->runtime()->nominatedRoles($roles)); } public function testUnknownRoleStaysVisible(): void