fix: make bpn response failures diagnosable

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.
 

src/BusProNet/ApiClient.php

@@ -602,10 +602,12 @@ class ApiClient
             'port' => $this->selectedPort,
         ]);
         $this->send($socket, $body);
-        $response = $this->receive($socket);
-        $this->disconnect($socket);
 
-        $responseXml = substr($response, 10);
+        try {
+            $responseXml = $this->receive($socket);
+        } finally {
+            $this->disconnect($socket);
+        }
 
         if (true === $debug || true === $this->config['debug']) {
             $this->dumpXmlToFile('response', $requestId, $responseXml);
@@ -714,18 +716,12 @@ class ApiClient
         ]);
 
         $this->send($socket, $body);
-        $response = $this->receive($socket);
-        $this->disconnect($socket);
 
-        // message length (10 bytes) is prepended to actual message
-        $xml = substr($response, 10);
-
-        if ('' === $xml) {
-            $this->logger->error('Empty response body received from API (header-only response)', [
-                'request_id' => $requestId,
-                'raw_length' => strlen($response),
-            ]);
-            throw new ApiClientException('Empty response body received from API');
+        try {
+            // the length header is consumed and verified by receive(), this is the payload
+            $xml = $this->receive($socket);
+        } finally {
+            $this->disconnect($socket);
         }
 
         if (true === $debug || true === $this->config['debug']) {
@@ -735,14 +731,19 @@ class ApiClient
         try {
             return $this->responseParser->parseXmlString($type, $xml, $additionalArgs);
         } catch (ResponseParserException $e) {
+            // Keep the evidence even outside debug mode: without the raw response a parse
+            // failure is not diagnosable after the fact. app:cleanup:xml-dumps prunes it.
             $this->dumpXmlToFile('response', $requestId, $xml);
+
+            $this->logger->error('Unable to parse response received from API', [
+                'request_id' => $requestId,
+                'type' => $type,
+                'response_length' => strlen($xml),
+                'error_message' => $e->getMessage(),
+            ]);
+
+            throw new ApiClientException('Unable to parse response received from API: '.$e->getMessage(), 0, $e);
         }
-
-        $this->logger->error('Unexpected response received from API', [
-            'request_id' => $requestId,
-        ]);
-
-        throw new ApiClientException('Unexpected response received from API');
     }
 
     /**
@@ -773,10 +774,12 @@ class ApiClient
         ]);
 
         $this->send($socket, $body);
-        $response = $this->receive($socket);
-        $this->disconnect($socket);
 
-        $xml = substr($response, 10);
+        try {
+            $xml = $this->receive($socket);
+        } finally {
+            $this->disconnect($socket);
+        }
 
         if (true === $this->config['debug']) {
             $this->dumpXmlToFile('response', $requestId, $xml);
@@ -898,8 +901,14 @@ class ApiClient
     }
 
     /**
+     * Reads a single response message and returns its payload. The protocol prepends the
+     * payload length as a 10 byte header, so the announced length is compared against what
+     * actually arrived: a peer closing mid-stream would otherwise yield a silently
+     * truncated body that only surfaces later as an unexplained parse failure.
+     *
      * @param resource $socket
      *
+     * @throws ApiClientException
      * @throws TimeoutException
      * @throws ImmediateConnectionCloseException
      */
@@ -963,7 +972,47 @@ class ApiClient
             throw new ImmediateConnectionCloseException('Server closed connection without sending data');
         }
 
-        return $response;
+        return $this->extractPayload($response);
+    }
+
+    /**
+     * @throws ApiClientException
+     */
+    private function extractPayload(string $response): string
+    {
+        if (10 > strlen($response)) {
+            $this->logger->error('Incomplete response header received from API', [
+                'bytes_received' => strlen($response),
+                'port' => $this->selectedPort,
+            ]);
+
+            throw new ApiClientException(sprintf('Incomplete response header, got %d of 10 bytes', strlen($response)));
+        }
+
+        $header = substr($response, 0, 10);
+        $announcedLength = (int) trim($header);
+        $payload = substr($response, 10);
+
+        if (1 > $announcedLength) {
+            $this->logger->error('Empty response body announced by API', [
+                'header' => trim($header),
+                'port' => $this->selectedPort,
+            ]);
+
+            throw new ApiClientException(sprintf('Response announced an empty body (header "%s")', trim($header)));
+        }
+
+        if (strlen($payload) !== $announcedLength) {
+            $this->logger->error('Truncated response received from API', [
+                'bytes_received' => strlen($payload),
+                'announced_length' => $announcedLength,
+                'port' => $this->selectedPort,
+            ]);
+
+            throw new ApiClientException(sprintf('Truncated response, got %d of %d announced bytes', strlen($payload), $announcedLength));
+        }
+
+        return $payload;
     }
 
     /** @param resource $socket */

src/BusProNet/Model/PersonalData.php

@@ -61,6 +61,13 @@ class PersonalData
     /** @var list<string> */
     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<string, mixed> 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,

src/BusProNet/XmlCrawlerFactory.php

@@ -20,4 +20,38 @@ final class XmlCrawlerFactory
 
         return $crawler;
     }
+
+    /**
+     * Explains why create() returned an empty Crawler. addXmlContent() discards the
+     * libxml errors, so an empty, a malformed and a truncated payload are
+     * indistinguishable afterwards; this re-parses to recover the reason. Only worth
+     * calling on the failure path.
+     */
+    public static function diagnose(string $xml): string
+    {
+        if ('' === trim($xml)) {
+            return sprintf('empty response (%d bytes)', strlen($xml));
+        }
+
+        $previousUseErrors = libxml_use_internal_errors(true);
+        libxml_clear_errors();
+
+        try {
+            simplexml_load_string($xml);
+
+            $messages = [];
+            foreach (libxml_get_errors() as $error) {
+                $messages[] = sprintf('%s (line %d, column %d)', trim($error->message), $error->line, $error->column);
+            }
+
+            if ([] === $messages) {
+                return sprintf('unknown XML error (%d bytes)', strlen($xml));
+            }
+
+            return sprintf('%s (%d bytes)', implode('; ', array_unique($messages)), strlen($xml));
+        } finally {
+            libxml_clear_errors();
+            libxml_use_internal_errors($previousUseErrors);
+        }
+    }
 }

src/BusProNet/XmlParser/ApiResponseParser.php

@@ -17,7 +17,7 @@ class ApiResponseParser extends AbstractParser
         $crawler = XmlCrawlerFactory::create($xml);
 
         if (0 === $crawler->count()) {
-            throw new ResponseParserException('Empty response received from server');
+            throw new ResponseParserException('Unable to parse XML response: '.XmlCrawlerFactory::diagnose($xml));
         }
 
         // Override type unless present in XML to catch error responses
@@ -56,7 +56,8 @@ class ApiResponseParser extends AbstractParser
                         case 'Vorgangdruck':
                             return (new DocumentsParser())->parseConfirmation($resultNode);
                     }
-                    break;
+
+                    throw new ResponseParserException(sprintf('Unrecognised customer data subtype "%s"', $subType));
                 case ApiClient::TYPE_BASE_DATA_COUNTRIES:
                     return (new CountriesParser())->parse($resultNode);
                 case ApiClient::TYPE_MUTABLE_DATA:
@@ -92,6 +93,6 @@ class ApiResponseParser extends AbstractParser
             throw new ResponseParserException('Invalid XML response structure: '.$e->getMessage(), 0, $e);
         }
 
-        throw new ResponseParserException('Unable to parse XML response');
+        throw new ResponseParserException(sprintf('Unrecognised BusProNet response type "%s"', $responseType));
     }
 }

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());

tests/BusProNet/ApiClientReceiveTest.php

@@ -6,6 +6,7 @@ namespace App\Tests\BusProNet;
 
 use App\BusProNet\ApiClient;
 use App\BusProNet\DataProcessor\BookingDataProcessor;
+use App\BusProNet\Exception\ApiClientException;
 use App\BusProNet\Exception\ImmediateConnectionCloseException;
 use App\BusProNet\XmlParser\ApiResponseParser;
 use App\Service\RequestIdGenerator;
@@ -70,7 +71,7 @@ class ApiClientReceiveTest extends TestCase
         $this->invokeReceive($stream);
     }
 
-    public function testNonEmptyResponseReturnsData(): void
+    public function testNonEmptyResponseReturnsPayloadWithoutTheLengthHeader(): void
     {
         $stream = fopen('php://memory', 'r+');
         fwrite($stream, '0000000005Hello');
@@ -80,7 +81,56 @@ class ApiClientReceiveTest extends TestCase
 
         $result = $this->invokeReceive($stream);
 
-        self::assertSame('0000000005Hello', $result);
+        self::assertSame('Hello', $result);
+    }
+
+    /**
+     * A peer closing mid-stream used to yield a silently shortened payload, which only
+     * surfaced later as an unexplained parse failure. The announced length catches it here.
+     */
+    public function testTruncatedResponseThrows(): void
+    {
+        $stream = fopen('php://memory', 'r+');
+        fwrite($stream, '0000000050Hello');
+        rewind($stream);
+
+        $this->setOperationStartTime();
+        $this->setSelectedPort(9000);
+
+        $this->expectException(ApiClientException::class);
+        $this->expectExceptionMessage('Truncated response, got 5 of 50 announced bytes');
+
+        $this->invokeReceive($stream);
+    }
+
+    public function testResponseShorterThanTheLengthHeaderThrows(): void
+    {
+        $stream = fopen('php://memory', 'r+');
+        fwrite($stream, '00000');
+        rewind($stream);
+
+        $this->setOperationStartTime();
+        $this->setSelectedPort(9000);
+
+        $this->expectException(ApiClientException::class);
+        $this->expectExceptionMessage('Incomplete response header, got 5 of 10 bytes');
+
+        $this->invokeReceive($stream);
+    }
+
+    public function testHeaderOnlyResponseThrows(): void
+    {
+        $stream = fopen('php://memory', 'r+');
+        fwrite($stream, '0000000000');
+        rewind($stream);
+
+        $this->setOperationStartTime();
+        $this->setSelectedPort(9000);
+
+        $this->expectException(ApiClientException::class);
+        $this->expectExceptionMessage('Response announced an empty body (header "0000000000")');
+
+        $this->invokeReceive($stream);
     }
 
     private function invokeReceive($socket): string

tests/BusProNet/XmlParser/ApiResponseParserFailureTest.php

@@ -0,0 +1,77 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\BusProNet\XmlParser;
+
+use App\BusProNet\ApiClient;
+use App\BusProNet\Exception\ResponseParserException;
+use App\BusProNet\XmlParser\ApiResponseParser;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * A broken response used to collapse into either "Empty response received from server" —
+ * which is wrong for a malformed body — or an opaque "Unable to parse XML response".
+ * Each shape must now name itself, otherwise a production parse failure is not
+ * diagnosable after the fact.
+ */
+class ApiResponseParserFailureTest extends TestCase
+{
+    public function testEmptyResponse(): void
+    {
+        $this->expectException(ResponseParserException::class);
+        $this->expectExceptionMessage('empty response (0 bytes)');
+
+        (new ApiResponseParser())->parseXmlString(ApiClient::TYPE_NOTIFICATION, '');
+    }
+
+    public function testTruncatedResponseNamesTheLibxmlReason(): void
+    {
+        $xml = '<?xml version="1.0" encoding="utf-8"?><ergebnis><satz typ="HINWEIS"></satz><hinweis';
+
+        try {
+            (new ApiResponseParser())->parseXmlString(ApiClient::TYPE_NOTIFICATION, $xml);
+            $this->fail('Expected a ResponseParserException');
+        } catch (ResponseParserException $e) {
+            self::assertStringContainsString('('.strlen($xml).' bytes)', $e->getMessage());
+            self::assertStringContainsString('line 1', $e->getMessage());
+        }
+    }
+
+    public function testWellFormedResponseOfAnUnknownType(): void
+    {
+        $xml = '<?xml version="1.0" encoding="utf-8"?><ergebnis><satz typ="IRGENDWAS"></satz></ergebnis>';
+
+        $this->expectException(ResponseParserException::class);
+        $this->expectExceptionMessage('Unrecognised BusProNet response type "IRGENDWAS"');
+
+        (new ApiResponseParser())->parseXmlString(ApiClient::TYPE_NOTIFICATION, $xml);
+    }
+
+    public function testUnknownCustomerDataSubtype(): void
+    {
+        $xml = '<?xml version="1.0" encoding="utf-8"?><ergebnis><satz typ="KUNDENKONTO"></satz><art>Irgendwas</art></ergebnis>';
+
+        $this->expectException(ResponseParserException::class);
+        $this->expectExceptionMessage('Unrecognised customer data subtype "Irgendwas"');
+
+        (new ApiResponseParser())->parseXmlString(ApiClient::TYPE_CUSTOMER_DATA, $xml);
+    }
+
+    public function testDiagnosingDoesNotLeakLibxmlErrorState(): void
+    {
+        $previous = libxml_use_internal_errors(false);
+
+        try {
+            try {
+                (new ApiResponseParser())->parseXmlString(ApiClient::TYPE_NOTIFICATION, '<ergebnis');
+            } catch (ResponseParserException) {
+            }
+
+            self::assertFalse(libxml_use_internal_errors(false));
+            self::assertSame([], libxml_get_errors());
+        } finally {
+            libxml_use_internal_errors($previous);
+        }
+    }
+}

tests/Controller/Api/UserinfoControllerTest.php

@@ -0,0 +1,169 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Controller\Api;
+
+use App\BusProNet\ApiClient;
+use App\BusProNet\Model\PersonalData;
+use App\Controller\Api\UserinfoController;
+use App\Entity\User;
+use App\Security\Crypt;
+use App\Security\Role;
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\DependencyInjection\Container;
+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage;
+use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
+use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
+
+/**
+ * The claims are a published contract (docs/api-consumer-guide.md), and the identity claims in
+ * particular: a BusPro person may sign in under any address on its record, every one of those is
+ * a separate account with its own roles, and BusPro answers all of them identically. So what
+ * separates the accounts is asserted here rather than left to the consumers to discover.
+ */
+class UserinfoControllerTest extends TestCase
+{
+    private const LOGIN_EMAIL = '[email protected]';
+    private const CONTACT_EMAIL = '[email protected]';
+
+    public function testSubAndEmailAreExportedWithoutAnyOptionalScope(): void
+    {
+        $claims = $this->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<string, mixed>
+     */
+    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;
+    }
+}

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,
+            '[email protected]',
+        );
+
+        $user = $this->loadUser($authenticator, '[email protected]');
+
+        // 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);