diff --git a/config/services.yaml b/config/services.yaml index 23dc26d..e9e7df0 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -31,6 +31,12 @@ parameters: 10321389: 'Reisen-Alert Stubaital' 10554990: 'Reisen-Alert Ski & Boarderweek' + # Email domains whose accounts are members of staff. BusPro has no CRM selection expressing + # "works here", so ROLE_EMPLOYEE is derived from the address instead. Matched exactly on the + # domain part, never as a suffix. + employee_email_domains: + - 'ep-reisen.de' + # BusProNet "Hausleitung {CODE}" CRM selections, by selection id. # DEPLOYMENT-CRITICAL: roles and hotel codes are synced on every login, so an id missing # here does not merely fail to nominate a Hausleitung — it revokes the role and the hotel @@ -299,6 +305,10 @@ services: arguments: $houseManagerIds: '%bpn_crm_house_manager_ids%' + App\Security\EmployeeDomainMatcher: + arguments: + $domains: '%employee_email_domains%' + App\Service\DomainConfigProvider: arguments: $domainConfig: '%domain_config%' diff --git a/src/Security/BpnAuthenticator.php b/src/Security/BpnAuthenticator.php index f7a7f33..4657114 100644 --- a/src/Security/BpnAuthenticator.php +++ b/src/Security/BpnAuthenticator.php @@ -34,8 +34,10 @@ use Symfony\Component\Security\Http\Util\TargetPathTrait; * * The password is kept, RSA-encrypted, because every later BPN call needs it again. * - * BusPro owns the whole role set and the hotel codes: both are synced on every login, in both - * directions, so anything the CRM no longer reports is withdrawn here. What the CRM claims is + * BusPro owns the whole role set bar one, and the hotel codes: both are synced on every login, in + * both directions, so anything the CRM no longer reports is withdrawn here. The exception is + * ROLE_EMPLOYEE, which BusPro has no selection for and which is derived from the account's email + * domain — passed to Role::sync() as a claim, so it is granted and revoked by the same machinery. What the CRM claims is * not automatically granted, though — Role::sync() turns an administrative claim into a * nomination that an administrator has to approve in /admin/user, because BusPro backend users * can edit their own CRM selections and would otherwise make themselves administrators. @@ -51,6 +53,7 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent private readonly Crypt $crypt, private readonly ProfileCompletenessChecker $completenessChecker, private readonly LoggerInterface $authLogger, + private readonly EmployeeDomainMatcher $employeeDomainMatcher, ) { } @@ -152,8 +155,17 @@ class BpnAuthenticator extends AbstractLoginFormAuthenticator implements Authent } } + $claimedRoles = $crmAttributes->roles; + + // Not a CRM claim: BusPro has no selection for it, so the account's own address decides. + // Passing it in as a claim rather than setting it afterwards is what makes it revocable — + // Role::sync() strips every stored role the claim set does not contain. + if ($this->employeeDomainMatcher->isEmployee($user->getEmail())) { + $claimedRoles[] = Role::EMPLOYEE; + } + $user - ->setRoles(Role::sync($previousRoles, $crmAttributes->roles)) + ->setRoles(Role::sync($previousRoles, $claimedRoles)) ->setHotelCodes(array_values(array_unique($crmAttributes->hotelCodes))) ; diff --git a/src/Security/EmployeeDomainMatcher.php b/src/Security/EmployeeDomainMatcher.php new file mode 100644 index 0000000..72795fa --- /dev/null +++ b/src/Security/EmployeeDomainMatcher.php @@ -0,0 +1,58 @@ +domains = array_values(array_filter(array_map( + static fn (string $domain): string => strtolower(trim($domain, " \t\n\r\0\x0B.@")), + $domains, + ))); + } + + /** + * A malformed or missing identifier is simply not an employee. It must not throw: this runs + * inside the authentication path, where an exception would turn a bad address into a failed + * login rather than a login without the role. + */ + public function isEmployee(?string $email): bool + { + if (null === $email || [] === $this->domains) { + return false; + } + + $position = strrpos($email, '@'); + + if (false === $position) { + return false; + } + + $domain = strtolower(substr($email, $position + 1)); + + return \in_array($domain, $this->domains, true); + } +} diff --git a/src/Security/Role.php b/src/Security/Role.php index a3e28af..64dd885 100644 --- a/src/Security/Role.php +++ b/src/Security/Role.php @@ -16,6 +16,13 @@ namespace App\Security; * nothing until an administrator approves it in /admin/user. The CRM's word alone is enough to * take a role away, never to hand it out, and an administrator's word alone is enough for * neither. + * + * One role is not claimed by the CRM at all: ROLE_EMPLOYEE is derived from the account's own + * email domain, because BusPro has no selection expressing "works here". It is passed to sync() + * as a claim alongside the CRM's, so it is granted and revoked by exactly the same machinery. + * 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. */ final class Role { @@ -32,6 +39,7 @@ final class Role public const HOUSE_MANAGER = 'ROLE_HOUSE_MANAGER'; public const GROUPS_ADMIN = 'ROLE_GROUPS_ADMIN'; public const GROUPS_MANAGER = 'ROLE_GROUPS_MANAGER'; + public const EMPLOYEE = 'ROLE_EMPLOYEE'; /** * Appended to an administrative role to mark it as claimed by the CRM but not yet approved. @@ -55,17 +63,21 @@ final class Role self::HOUSE_MANAGER, self::GROUPS_ADMIN, self::GROUPS_MANAGER, + self::EMPLOYEE, ]; /** - * Roles the CRM grants outright. ROLE_CUSTOMER is never claimed by BusPro — it is the - * fallback for an account left without any effective role, and exclusive with the others. + * Roles granted outright, without an approval step. ROLE_CUSTOMER is never claimed by BusPro — + * it is the fallback for an account left without any effective role, and exclusive with the + * others. ROLE_EMPLOYEE is not claimed by BusPro either: it is derived from the account's own + * email domain (see EmployeeDomainMatcher) and, being effective, displaces that fallback. * * @var string[] */ public const UNCONDITIONAL = [ self::TEAMER, self::CUSTOMER, + self::EMPLOYEE, ]; /** @@ -180,7 +192,8 @@ final class Role * Nothing here can raise a privilege: step 3 only ever produces markers. * * @param string[] $storedRoles - * @param string[] $claimedRoles what the CRM reports + * @param string[] $claimedRoles what the CRM reports, plus the roles derived from the account + * itself (ROLE_EMPLOYEE); anything outside self::ALL is ignored * * @return string[] */ @@ -274,6 +287,7 @@ final class Role self::HOUSE_MANAGER => 'Hausleitung', self::GROUPS_ADMIN => 'Preisrechner Admin', self::GROUPS_MANAGER => 'Preisrechner', + self::EMPLOYEE => 'Mitarbeiter:in', ]; foreach (self::ADMINISTRATIVE as $role) { diff --git a/tests/Security/BpnAuthenticatorTest.php b/tests/Security/BpnAuthenticatorTest.php index df9b51e..202d6b6 100644 --- a/tests/Security/BpnAuthenticatorTest.php +++ b/tests/Security/BpnAuthenticatorTest.php @@ -11,6 +11,7 @@ use App\BusProNet\Model\PersonalData; use App\Entity\User; use App\Security\BpnAuthenticator; use App\Security\Crypt; +use App\Security\EmployeeDomainMatcher; use App\Security\Role; use App\Service\ProfileCompletenessChecker; use Doctrine\ORM\EntityManagerInterface; @@ -196,6 +197,38 @@ class BpnAuthenticatorTest extends TestCase return $attributes; } + public function testStaffEmailDomainGrantsTheEmployeeRole(): void + { + $persisted = null; + $authenticator = $this->authenticator($this->crmAttributes([], []), null, $persisted); + + $user = $this->loadUser($authenticator, 'someone@ep-reisen.de'); + + // Effective, so it displaces the customer fallback the same account would get otherwise. + self::assertSame(['ROLE_USER', Role::EMPLOYEE], $user->getRoles()); + } + + public function testAnotherEmailDomainStillFallsBackToCustomer(): void + { + $persisted = null; + $authenticator = $this->authenticator($this->crmAttributes([], []), null, $persisted); + + $user = $this->loadUser($authenticator, 'someone@example.org'); + + self::assertSame(['ROLE_USER', Role::CUSTOMER], $user->getRoles()); + } + + public function testEmployeeRoleIsWithdrawnWhenTheAddressIsNoLongerStaff(): void + { + $existing = (new User('someone@example.org'))->setRoles([Role::EMPLOYEE]); + $persisted = null; + $authenticator = $this->authenticator($this->crmAttributes([], []), $existing, $persisted); + + $user = $this->loadUser($authenticator, 'someone@example.org'); + + self::assertSame(['ROLE_USER', Role::CUSTOMER], $user->getRoles()); + } + private function authenticator( CrmAttributes $crmAttributes, ?User $existing, @@ -238,13 +271,14 @@ class BpnAuthenticatorTest extends TestCase $crypt, $completenessChecker, $this->createStub(LoggerInterface::class), + new EmployeeDomainMatcher(['ep-reisen.de']), ); } - private function loadUser(BpnAuthenticator $authenticator): User + private function loadUser(BpnAuthenticator $authenticator, string $email = 'teamer@example.org'): User { $request = new Request(); - $request->request->set('_username', 'teamer@example.org'); + $request->request->set('_username', $email); $request->request->set('_password', 'secret'); $badge = $authenticator->authenticate($request)->getBadge(UserBadge::class); diff --git a/tests/Security/EmployeeDomainMatcherTest.php b/tests/Security/EmployeeDomainMatcherTest.php new file mode 100644 index 0000000..241d419 --- /dev/null +++ b/tests/Security/EmployeeDomainMatcherTest.php @@ -0,0 +1,56 @@ +isEmployee($email)); + } + + /** + * @return iterable + */ + public static function addresses(): iterable + { + yield 'the configured domain' => ['someone@ep-reisen.de', true]; + yield 'mixed case is still the same domain' => ['Someone@EP-Reisen.DE', true]; + yield 'plus addressing does not touch the domain' => ['someone+booking@ep-reisen.de', true]; + yield 'an at sign in the local part' => ['"odd@name"@ep-reisen.de', true]; + + yield 'another domain' => ['someone@example.org', false]; + yield 'a subdomain is not the domain' => ['someone@mail.ep-reisen.de', false]; + yield 'a domain merely ending in it' => ['someone@notep-reisen.de', false]; + yield 'the domain as a prefix' => ['someone@ep-reisen.de.evil.test', false]; + yield 'no at sign at all' => ['ep-reisen.de', false]; + yield 'empty' => ['', false]; + yield 'null' => [null, false]; + } + + public function testWithoutConfiguredDomainsNobodyIsAnEmployee(): void + { + self::assertFalse((new EmployeeDomainMatcher([]))->isEmployee('someone@ep-reisen.de')); + } + + public function testConfiguredDomainsAreNormalised(): void + { + $matcher = new EmployeeDomainMatcher([' EP-Reisen.DE ', '@example.org']); + + self::assertTrue($matcher->isEmployee('someone@ep-reisen.de')); + self::assertTrue($matcher->isEmployee('someone@example.org')); + } +} diff --git a/tests/Security/RoleTest.php b/tests/Security/RoleTest.php index 2ee41cf..7795c31 100644 --- a/tests/Security/RoleTest.php +++ b/tests/Security/RoleTest.php @@ -93,6 +93,35 @@ class RoleTest extends TestCase self::assertSame([Role::ADMIN => 'Administration'], Role::nominatedFrom($roles)); } + public function testEmployeeIsGrantedOutrightAndDisplacesTheCustomerFallback(): void + { + // The claim does not come from the CRM, but it travels the same path as one. + self::assertSame([Role::EMPLOYEE], Role::sync([], [Role::EMPLOYEE])); + } + + public function testEmployeeIsRevokedOnceItIsNoLongerClaimed(): void + { + // Somebody whose address left the staff domain: no claim, so the role goes, and with no + // effective role left the fallback returns. + self::assertSame([Role::CUSTOMER], Role::sync([Role::EMPLOYEE], [])); + } + + public function testEmployeeDoesNotShortCircuitTheNominationOfAnAdministrativeRole(): void + { + self::assertSame( + [Role::EMPLOYEE, Role::pending(Role::ADMIN)], + Role::sync([], [Role::EMPLOYEE, Role::ADMIN]), + ); + } + + public function testEmployeeAndTeamerCoexist(): void + { + self::assertSame( + [Role::TEAMER, Role::EMPLOYEE], + Role::sync([], [Role::TEAMER, Role::EMPLOYEE]), + ); + } + public function testEveryRoleAndNominationHasALabel(): void { $labels = Role::labels();