diff --git a/docs/api-consumer-guide.md b/docs/api-consumer-guide.md index a4b0fca..71000e6 100644 --- a/docs/api-consumer-guide.md +++ b/docs/api-consumer-guide.md @@ -379,7 +379,7 @@ Responses: OIDC-style claims for the authenticated user. The response contains only the claims covered by the granted scopes: -- always: `email` +- always: `sub` (string), `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/buspronet-schema/crm-selection-queries.md#from-claim-to-role`) - scope `profile`: `profile` object: @@ -394,7 +394,28 @@ OIDC-style claims for the authenticated user. The response contains only the cla } ``` -Note: `profile.communication.email` comes from the CRM address record and is **not necessarily** the login email — a single address can hold several contacts. Use the top-level `email` claim for identity. +##### Identifying the account: match on `sub` + +A BusPro address record holds several email addresses (private and business, say), and BusPro +accepts **any** of them as a login. Each one is a **separate MyE&P account with its own roles** — +that is intended, not a duplicate: administrative roles are reserved for the company address, so +the same person signing in privately is a Teamer:in or Kund:in and signing in with their +`@ep-reisen.de` address is an administrator. + +All of those accounts report the **same** `person_id` and `address_id`, and the same +`profile.communication.email` — that value is the first contact address on the BusPro record and +is **not necessarily** the one signed in with. So: + +- **`sub`** — opaque, stable, unique per MyE&P account. The only claim that identifies an account. + Match your local user on it and store it. +- **`email`** — the address this session actually authenticated with. Distinct per account, but + treat it as a display and contact value; it is not the account key. +- **`person_id` / `address_id`** — the BusPro person behind the account. Shared between that + person's accounts, so they identify a *human*, never an account. Do not match on them. + +Do not derive staff status from an email domain. MyE&P decides who is a member of staff and +exports the result as `ROLE_EMPLOYEE` in the `roles` claim; re-deriving it from `email` or from +`profile.communication.email` will disagree with MyE&P in both directions. `400 {"message": "...", "code": "..."}` when the upstream lookup fails or rejects the credentials. diff --git a/src/BusProNet/Model/PersonalData.php b/src/BusProNet/Model/PersonalData.php index 2e85da5..f6233b1 100644 --- a/src/BusProNet/Model/PersonalData.php +++ b/src/BusProNet/Model/PersonalData.php @@ -61,6 +61,13 @@ class PersonalData /** @var list */ public array $hotelCodes = []; + // The authenticated account, not BusPro's idea of this person. One BusPro person may sign in + // under any of the addresses on its record, and each of those is a separate local account with + // its own roles, so neither the BusPro ids nor the communication email identifies one. Patched + // on by the caller in the same way as the roles and hotel codes above. + public ?string $subject = null; + public ?string $loginEmail = null; + public function __construct() { $this->address = new Address(); @@ -132,14 +139,21 @@ class PersonalData * Creates a structured array containing user profile information * suitable for JWT claims or user session data. * + * `sub` and `email` describe the account that authenticated and are read from the patched-on + * fields, never from BusPro: `person_id`/`address_id` are shared by every account of the same + * person, and `profile.communication.email` is the first contact address on the BusPro record, + * which is not necessarily the one signed in with. Deliberately without a fallback to that + * address — a caller that forgets to patch them gets null rather than a wrong identity. + * * @return array The claims array with user profile data */ public function getClaims(): array { return [ + 'sub' => $this->subject, 'person_id' => $this->personId, 'address_id' => $this->addressId, - 'email' => $this->communication->email, + 'email' => $this->loginEmail, 'roles' => $this->roles, 'profile' => [ 'first_name' => $this->firstName, diff --git a/src/Controller/Api/UserinfoController.php b/src/Controller/Api/UserinfoController.php index e199a3e..077ca0e 100644 --- a/src/Controller/Api/UserinfoController.php +++ b/src/Controller/Api/UserinfoController.php @@ -26,8 +26,9 @@ class UserinfoController extends AbstractController #[Route('/userinfo', name: 'api_userinfo', methods: ['GET'])] public function index(): JsonResponse { - // basic scopes applicable to all authenticated users - $scopes = ['email']; + // basic scopes applicable to all authenticated users. `sub` is not gated on a scope of its + // own: it identifies the account every other claim describes, so it is always exported. + $scopes = ['sub', 'email']; // extend scopes depending on granted permissions if ($this->isGranted('ROLE_OAUTH2_ID')) { @@ -53,6 +54,13 @@ class UserinfoController extends AbstractController return new JsonResponse(['message' => $data->message, 'code' => $data->code], Response::HTTP_BAD_REQUEST); } + // Patch the identity of the account that authenticated. BusPro accepts any of the + // addresses on a person's record as a login and answers all of them with the same + // ids and the same first contact address, so only the local account tells the staff + // account and the private one apart — and they hold different roles. + $data->subject = (string) $user->getId(); + $data->loginEmail = $user->getEmail(); + // Patch current user's roles. The implicit ROLE_USER says nothing about the // account — every authenticated user holds it — and is not exported. $data->roles = Role::effectiveOnly($user->getRoles()); diff --git a/tests/Controller/Api/UserinfoControllerTest.php b/tests/Controller/Api/UserinfoControllerTest.php new file mode 100644 index 0000000..306928b --- /dev/null +++ b/tests/Controller/Api/UserinfoControllerTest.php @@ -0,0 +1,169 @@ +claims(); + + self::assertSame(['sub', 'email'], array_keys($claims)); + self::assertSame('42', $claims['sub']); + self::assertSame(self::LOGIN_EMAIL, $claims['email']); + } + + public function testSubIsTheLocalAccountAndNotTheBusProPerson(): void + { + $claims = $this->claims(['ROLE_OAUTH2_ID']); + + // The ids identify the human, the subject identifies the account. A second account of the + // same person reports these same two ids and must still be told apart. + self::assertSame('42', $claims['sub']); + self::assertSame(7, $claims['person_id']); + self::assertSame(9, $claims['address_id']); + } + + public function testEmailIsTheAuthenticatedAddressAndNotTheFirstContactOnTheBusProRecord(): void + { + $claims = $this->claims(['ROLE_OAUTH2_PROFILE']); + + self::assertSame(self::LOGIN_EMAIL, $claims['email']); + self::assertSame(self::CONTACT_EMAIL, $claims['profile']['communication']['email']); + } + + public function testRolesExportOnlyWhatIsEffective(): void + { + $claims = $this->claims(['ROLE_OAUTH2_ROLES'], [ + Role::USER, + Role::EMPLOYEE, + Role::TEAMER, + Role::pending(Role::MANAGER), + ]); + + // ROLE_EMPLOYEE is exported because consumers must read staff status from here rather than + // re-deriving it from an email domain — which, given the two addresses, would disagree. + self::assertSame([Role::EMPLOYEE, Role::TEAMER], $claims['roles']); + } + + public function testUpstreamFailureIsReportedRatherThanAnswered(): void + { + $apiClient = $this->createStub(ApiClient::class); + $apiClient->method('getPersonalData')->willThrowException(new \App\BusProNet\Exception\ApiClientException('nope')); + + $response = $this->call($apiClient, []); + + self::assertSame(Response::HTTP_BAD_REQUEST, $response->getStatusCode()); + } + + /** + * @param string[] $grantedScopeRoles + * @param string[] $roles + * + * @return array + */ + private function claims(array $grantedScopeRoles = [], array $roles = [Role::TEAMER]): array + { + $apiClient = $this->createStub(ApiClient::class); + $apiClient->method('getPersonalData')->willReturn($this->personalData()); + + $response = $this->call($apiClient, $grantedScopeRoles, $roles); + + self::assertSame(Response::HTTP_OK, $response->getStatusCode()); + + return json_decode((string) $response->getContent(), true); + } + + /** + * @param string[] $grantedScopeRoles + * @param string[] $roles + */ + private function call(ApiClient $apiClient, array $grantedScopeRoles, array $roles = [Role::TEAMER]): Response + { + $crypt = $this->createStub(Crypt::class); + $crypt->method('decrypt')->willReturn('md5-of-the-password'); + + $controller = new UserinfoController($apiClient, $crypt); + $controller->setContainer($this->container($this->user($roles), $grantedScopeRoles)); + + return $controller->index(); + } + + /** + * @param string[] $grantedScopeRoles + */ + private function container(User $user, array $grantedScopeRoles): Container + { + $tokenStorage = new TokenStorage(); + $tokenStorage->setToken(new UsernamePasswordToken($user, 'api', $user->getRoles())); + + $authorizationChecker = $this->createStub(AuthorizationCheckerInterface::class); + $authorizationChecker->method('isGranted')->willReturnCallback( + static fn (mixed $attribute): bool => \in_array($attribute, $grantedScopeRoles, true), + ); + + // No 'serializer' service registered, so AbstractController::json() falls back to + // JsonResponse — which is what this endpoint produces in production anyway. + $container = new Container(); + $container->set('security.token_storage', $tokenStorage); + $container->set('security.authorization_checker', $authorizationChecker); + + return $container; + } + + /** + * @param string[] $roles + */ + private function user(array $roles): User + { + $user = (new User(self::LOGIN_EMAIL)) + ->setPassword('encrypted') + ->setRoles($roles) + ; + + // The id is generated by Doctrine and has no setter, but it is what `sub` exports. + $property = new \ReflectionProperty(User::class, 'id'); + $property->setValue($user, 42); + + return $user; + } + + private function personalData(): PersonalData + { + $personalData = new PersonalData(); + $personalData->personId = 7; + $personalData->addressId = 9; + $personalData->firstName = 'Alex'; + $personalData->name = 'Beispiel'; + + // Deliberately not the login address: BusPro returns the first contact row on the record, + // which is whichever of the person's addresses happens to come first. + $personalData->communication->email = self::CONTACT_EMAIL; + + return $personalData; + } +} diff --git a/tests/Security/BpnAuthenticatorTest.php b/tests/Security/BpnAuthenticatorTest.php index 587ab2e..5429db0 100644 --- a/tests/Security/BpnAuthenticatorTest.php +++ b/tests/Security/BpnAuthenticatorTest.php @@ -240,6 +240,28 @@ class BpnAuthenticatorTest extends TestCase self::assertSame(['ROLE_USER', Role::CUSTOMER], $user->getRoles()); } + public function testAStaffContactAddressOnTheBusProRecordDoesNotMakeTheLoginStaff(): void + { + $persisted = null; + $persistedPassword = null; + $authenticator = $this->authenticator( + $this->crmAttributes([Role::ADMIN, Role::TEAMER], []), + null, + $persisted, + $persistedPassword, + 'someone@ep-reisen.de', + ); + + $user = $this->loadUser($authenticator, 'someone@example.org'); + + // BusPro accepts any address on the record as a login and answers with the first contact + // address regardless of which one was used, so only the typed address may decide. Reading + // the response instead would hand ROLE_EMPLOYEE — and every EMPLOYEE_ONLY role with it — + // to anyone who can add a staff address to their own BusPro record. + self::assertSame(['ROLE_USER', Role::TEAMER], $user->getRoles()); + self::assertSame([], $this->dispatched); + } + public function testEmployeeOnlyClaimFromAnotherDomainIsIgnored(): void { $persisted = null; @@ -321,10 +343,12 @@ class BpnAuthenticatorTest extends TestCase ?User $existing, ?User &$persisted, ?string &$persistedPassword = null, + ?string $contactEmail = null, ): BpnAuthenticator { $personalData = new PersonalData(); $personalData->personId = 42; $personalData->addressId = 4711; + $personalData->communication->email = $contactEmail; $apiClient = $this->createStub(ApiClient::class); $apiClient->method('getPersonalData')->willReturn($personalData);