fix: make email readonly in personal data form to prevent user lockout

src/Controller/Account/PersonalDataController.php

@@ -84,11 +84,20 @@ class PersonalDataController extends AbstractController
         $newsletterSubscribed = $this->newsletterManager->hasConfirmedOptIn($email);
         $newsletterPendingConfirmation = $this->newsletterManager->hasPendingConfirmation($email);
 
+        // The e-mail is deliberately locked here: BPN stores it on the address, where it doubles as
+        // the login identity of any person on that address. Accepting a submitted value would let
+        // one household member overwrite another's login. It is only ever echoed back to BPN as read.
         $personalDataForm = $this->createForm(PersonalDataType::class, $personalData, [
             'attr' => ['novalidate' => 'novalidate'],
             'validation_groups' => ['personal_data'],
+            'email_editable' => false,
         ]);
+
+        // A failed load yields an empty PersonalData. Updating from it would send blank values for
+        // fields we never read - including the e-mail - so the form must not accept a submission.
+        if (null !== $personalData->addressId) {
             $personalDataForm->handleRequest($request);
+        }
 
         if ($personalDataForm->isSubmitted() && $personalDataForm->isValid()) {
             try {

src/Form/PersonalDataType.php

@@ -11,10 +11,13 @@ use Symfony\Component\Form\Extension\Core\Type\ChoiceType;
 use Symfony\Component\Form\Extension\Core\Type\EmailType;
 use Symfony\Component\Form\Extension\Core\Type\TextType;
 use Symfony\Component\Form\FormBuilderInterface;
+use Symfony\Component\OptionsResolver\OptionsResolver;
 
 /** @extends AbstractType<mixed> */
 class PersonalDataType extends AbstractType
 {
+    public const string LOCKED_EMAIL_HINT = 'Deine E-Mail-Adresse ist auch dein Benutzername. Bitte wende dich an den Kundenservice, wenn du sie ändern möchtest.';
+
     public function buildForm(FormBuilderInterface $builder, array $options): void
     {
         $builder
@@ -61,9 +64,18 @@ class PersonalDataType extends AbstractType
                 'placeholder' => 'Bitte auswählen...',
                 'preferred_choices' => ['D', 'A', 'CH'],
             ])
+            // The address level e-mail doubles as the BPN login identity, and a single BPN address
+            // can carry several of them for different persons. Editing it here would let one
+            // household member overwrite another's login, so forms that only change an existing
+            // address lock the field: Symfony ignores submitted data for disabled fields, which
+            // keeps the value BPN handed us on its way back into the update payload.
             ->add('email', EmailType::class, [
                 'label' => 'E-Mail',
                 'property_path' => 'communication.email',
+                'disabled' => false === $options['email_editable'],
+                'attr' => false === $options['email_editable'] ? [
+                    'title' => self::LOCKED_EMAIL_HINT,
+                ] : [],
             ])
             ->add('phone', TextType::class, [
                 'label' => 'Telefon',
@@ -78,4 +90,12 @@ class PersonalDataType extends AbstractType
             ])
         ;
     }
+
+    public function configureOptions(OptionsResolver $resolver): void
+    {
+        $resolver
+            ->setDefault('email_editable', true)
+            ->setAllowedTypes('email_editable', 'bool')
+        ;
+    }
 }

templates/account/personal_data.html.twig

@@ -45,6 +45,7 @@
             </div>
             <div class="pt-8">
                 {{ form_start(personalDataForm) }}
+                {{ form_errors(personalDataForm) }}
                 <div class="grid md:grid-cols-2 gap-x-8 gap-y-4 pb-4">
                     <div>
                         <h3 class="text-white">
@@ -56,7 +57,10 @@
                         <h3 class="text-white">
                             Kontakt
                         </h3>
-                        {{ form_row(personalDataForm.email, { 'label_attr': { 'class': 'text-white' } }) }}
+                        {% set locked_email_hint = constant('App\\Form\\PersonalDataType::LOCKED_EMAIL_HINT') %}
+                        {{ form_row(personalDataForm.email, {
+                            'label_attr': { 'class': 'text-white cursor-help', 'title': locked_email_hint },
+                        }) }}
                         {{ form_row(personalDataForm.mobile, { 'label_attr': { 'class': 'text-white' } }) }}
                         {{ form_row(personalDataForm.phone, { 'label_attr': { 'class': 'text-white' } }) }}
                     </div>

templates/forms.html.twig

@@ -86,9 +86,6 @@
     {%- endif -%}
     <div{% if tooltip is not null %} {{ stimulus_controller('tooltip', { 'content': tooltip }) }}{% endif %}>
         {{ parent() }}
-        {%- if disabled is defined and disabled == true -%}
-            <input type="hidden" name="{{ form.vars.full_name }}" value="{{ form.vars.value }}">
-        {%- endif -%}
     </div>
 {%- endblock form_widget_simple -%}
 
@@ -107,9 +104,6 @@
     {%- endif -%}
     <div{% if tooltip is not null %} {{ stimulus_controller('tooltip', { 'content': tooltip }) }}{% endif %}>
         {{ parent() }}
-        {%- if disabled is defined and disabled == true -%}
-            <input type="hidden" name="{{ form.vars.full_name }}" value="{{ form.vars.value }}">
-        {%- endif -%}
     </div>
 {%- endblock textarea_widget -%}
 

templates/forms_admin.html.twig

@@ -46,9 +46,6 @@
         {%- endif -%}
     {%- endif -%}
     {{ parent() }}
-    {%- if disabled is defined and disabled == true -%}
-        <input type="hidden" name="{{ form.vars.full_name }}" value="{{ form.vars.value }}">
-    {%- endif -%}
 {%- endblock form_widget_simple -%}
 
 {%- block textarea_widget -%}
@@ -62,9 +59,6 @@
         {%- set attr = attr|merge({'class': attr.class|default('') ~ ' cursor-not-allowed' }) -%}
     {%- endif -%}
     {{ parent() }}
-    {%- if disabled is defined and disabled == true -%}
-        <input type="hidden" name="{{ form.vars.full_name }}" value="{{ form.vars.value }}">
-    {%- endif -%}
 {%- endblock textarea_widget -%}
 
 {%- block checkbox_widget -%}
@@ -105,9 +99,6 @@
         {%- set attr = attr|merge({'class': attr.class|default('') ~ ' cursor-not-allowed' }) -%}
     {%- endif -%}
     {{ parent() }}
-    {%- if disabled is defined and disabled == true -%}
-        <input type="hidden" name="{{ form.vars.full_name }}" value="{{ form.vars.value }}">
-    {%- endif -%}
 {%- endblock choice_widget_collapsed -%}
 
 {%- block choice_widget_expanded -%}

tests/BusProNet/XmlParser/PersonalDataParserTest.php

@@ -0,0 +1,87 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\BusProNet\XmlParser;
+
+use App\BusProNet\XmlParser\PersonalDataParser;
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\DomCrawler\Crawler;
+
+class PersonalDataParserTest extends TestCase
+{
+    private PersonalDataParser $parser;
+
+    protected function setUp(): void
+    {
+        $this->parser = new PersonalDataParser();
+    }
+
+    public function testParseReadsCommunicationOfASinglePerson(): void
+    {
+        $personalData = $this->parse($this->responseXml(['[email protected]']));
+
+        self::assertSame('[email protected]', $personalData->communication->email);
+    }
+
+    /**
+     * A BPN address carries its communication entries 1:N, and the e-mail there doubles as a login
+     * identity. The parser only ever surfaces the first entry, so anything writing the parsed model
+     * back to BPN replaces the whole set with that one value. Pinned here because the personal data
+     * form used to do exactly that and locked out the person owning the second entry.
+     */
+    public function testParseOnlySurfacesTheFirstOfSeveralCommunicationEntries(): void
+    {
+        $personalData = $this->parse($this->responseXml(['[email protected]', '[email protected]']));
+
+        self::assertSame('[email protected]', $personalData->communication->email);
+    }
+
+    public function testParseLeavesEmailNullWithoutCommunicationEntry(): void
+    {
+        $personalData = $this->parse($this->responseXml([]));
+
+        self::assertNull($personalData->communication->email);
+    }
+
+    private function parse(string $xml): \App\BusProNet\Model\PersonalData
+    {
+        return $this->parser->parse(new Crawler($xml));
+    }
+
+    /**
+     * @param list<string> $emails
+     */
+    private function responseXml(array $emails): string
+    {
+        $communication = '';
+
+        foreach ($emails as $email) {
+            $communication .= sprintf(
+                '<kommunikation><email>%s</email><telefonmobil>0170 1234567</telefonmobil></kommunikation>',
+                $email,
+            );
+        }
+
+        return <<<XML
+            <?xml version="1.0" encoding="UTF-8"?>
+            <antwort>
+                <idadresse>4711</idadresse>
+                <idperson>815</idperson>
+                <adressdaten>
+                    <vorname>Mia</vorname>
+                    <name>Muster</name>
+                    <geschlecht>w</geschlecht>
+                    <nationalitaet>d</nationalitaet>
+                    <anschrift>
+                        <strasse>Musterweg 1</strasse>
+                        <plz>12345</plz>
+                        <ort>Musterstadt</ort>
+                        <land>D</land>
+                    </anschrift>
+                    {$communication}
+                </adressdaten>
+            </antwort>
+            XML;
+    }
+}

tests/Controller/Account/PersonalDataControllerTest.php

@@ -5,6 +5,7 @@ declare(strict_types=1);
 namespace App\Tests\Controller\Account;
 
 use App\BusProNet\ApiClient;
+use App\BusProNet\Model\Notification;
 use App\BusProNet\Model\PersonalData;
 use App\Controller\Account\PersonalDataController;
 use App\Entity\User;
@@ -150,6 +151,49 @@ class PersonalDataControllerTest extends TestCase
         ], $controller->flashes);
     }
 
+    /**
+     * The e-mail lives on the BPN address and doubles as the login identity of every person on it,
+     * so this form must never accept a submitted value for it.
+     */
+    public function testIndexLocksTheEmailField(): void
+    {
+        $controller = $this->createController();
+        $controller->apiClient
+            ->method('getPersonalData')
+            ->willReturn($this->createPersonalData('Mia', 'Muster'));
+
+        $controller->index(Request::create('/personal-data', 'GET'));
+
+        self::assertFalse($controller->createFormOptions['email_editable']);
+    }
+
+    /**
+     * A failed load yields an empty PersonalData. Handing that to BPN would blank every field we
+     * never read - including the e-mail, which would lock the customer out of the portal.
+     */
+    public function testIndexDoesNotSubmitTheFormWhenPersonalDataCouldNotBeLoaded(): void
+    {
+        $controller = $this->createController();
+        $controller->apiClient
+            ->expects(self::once())
+            ->method('getPersonalData')
+            ->willReturn(new Notification(500, 'no such customer'));
+        $controller->apiClient
+            ->expects(self::never())
+            ->method('updatePersonalData');
+
+        $controller->form
+            ->expects(self::never())
+            ->method('handleRequest');
+
+        $response = $controller->index(Request::create('/personal-data', 'POST', [
+            'personal_data' => ['email' => '[email protected]'],
+        ]));
+
+        self::assertSame(200, $response->getStatusCode());
+        self::assertSame('account/personal_data.html.twig', $controller->renderedView);
+    }
+
     private function createController(): TestablePersonalDataController
     {
         $user = new User('[email protected]');
@@ -187,8 +231,11 @@ class PersonalDataControllerTest extends TestCase
     private function createPersonalData(string $firstName, string $lastName): PersonalData
     {
         $personalData = new PersonalData();
+        $personalData->addressId = 4711;
+        $personalData->personId = 815;
         $personalData->firstName = $firstName;
         $personalData->name = $lastName;
+        $personalData->communication->email = '[email protected]';
 
         return $personalData;
     }
@@ -208,6 +255,11 @@ final class TestablePersonalDataController extends PersonalDataController
 
     public string $renderedView = '';
 
+    /**
+     * @var array<string, mixed>
+     */
+    public array $createFormOptions = [];
+
     public function __construct(
         public readonly ApiClient $apiClient,
         Crypt $crypt,
@@ -217,7 +269,7 @@ final class TestablePersonalDataController extends PersonalDataController
         public readonly NewsletterManager $newsletterManager,
         LoggerInterface $logger,
         private readonly User $user,
-        private readonly FormInterface $form,
+        public readonly FormInterface $form,
     ) {
         parent::__construct(
             $apiClient,
@@ -232,6 +284,8 @@ final class TestablePersonalDataController extends PersonalDataController
 
     public function createForm(string $type, mixed $data = null, array $options = []): FormInterface
     {
+        $this->createFormOptions = $options;
+
         return $this->form;
     }
 

tests/Form/PersonalDataTypeTest.php

@@ -0,0 +1,119 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Form;
+
+use App\BusProNet\DataProvider\CountryDataProvider;
+use App\BusProNet\Form\CountryType;
+use App\BusProNet\Model\PersonalData;
+use App\Form\PersonalDataType;
+use PHPUnit\Framework\TestCase;
+use Psr\Container\ContainerInterface;
+use Symfony\Component\Form\Extension\HtmlSanitizer\Type\TextTypeHtmlSanitizerExtension;
+use Symfony\Component\Form\FormInterface;
+use Symfony\Component\Form\Forms;
+use Symfony\Component\HtmlSanitizer\HtmlSanitizer;
+use Symfony\Component\HtmlSanitizer\HtmlSanitizerConfig;
+
+class PersonalDataTypeTest extends TestCase
+{
+    public function testEmailIsEditableByDefault(): void
+    {
+        $form = $this->createForm(new PersonalData());
+
+        self::assertFalse($form->get('email')->getConfig()->getOption('disabled'));
+    }
+
+    public function testEmailIsRenderedLockedWithATooltipWhenNotEditable(): void
+    {
+        $form = $this->createForm(new PersonalData(), ['email_editable' => false]);
+        $config = $form->get('email')->getConfig();
+
+        self::assertTrue($config->getOption('disabled'));
+        self::assertSame(
+            PersonalDataType::LOCKED_EMAIL_HINT,
+            $config->getOption('attr')['title'] ?? null,
+        );
+    }
+
+    /**
+     * The BPN address level e-mail is a login identity shared across every person on the address.
+     * A submitted value must not be able to reach the model - otherwise saving the form would
+     * overwrite somebody else's login.
+     */
+    public function testSubmittedEmailIsIgnoredWhenNotEditable(): void
+    {
+        $personalData = new PersonalData();
+        $personalData->communication->email = '[email protected]';
+
+        $form = $this->createForm($personalData, ['email_editable' => false]);
+        $form->submit($this->submittedValues(['email' => '[email protected]']));
+
+        self::assertTrue($form->isSynchronized());
+        self::assertSame('[email protected]', $personalData->communication->email);
+    }
+
+    public function testSubmittedEmailIsAppliedByDefault(): void
+    {
+        $personalData = new PersonalData();
+        $personalData->communication->email = '[email protected]';
+
+        $form = $this->createForm($personalData);
+        $form->submit($this->submittedValues(['email' => '[email protected]']));
+
+        self::assertTrue($form->isSynchronized());
+        self::assertSame('[email protected]', $personalData->communication->email);
+    }
+
+    /**
+     * @param array<string, mixed> $overrides
+     *
+     * @return array<string, mixed>
+     */
+    private function submittedValues(array $overrides = []): array
+    {
+        return array_merge([
+            'gender' => 'W',
+            'dateOfBirth' => '01.01.1990',
+            'street' => 'Musterweg 1',
+            'postCode' => '12345',
+            'city' => 'Musterstadt',
+            'country' => 'D',
+            'nationality' => 'D',
+            'phone' => '',
+            'mobile' => '0170 1234567',
+        ], $overrides);
+    }
+
+    /**
+     * @param array<string, mixed> $options
+     */
+    private function createForm(PersonalData $personalData, array $options = []): FormInterface
+    {
+        $countries = $this->createMock(CountryDataProvider::class);
+        $countries->method('getAll')->willReturn([]);
+
+        return Forms::createFormFactoryBuilder()
+            ->addType(new CountryType($countries))
+            ->addTypeExtension(new TextTypeHtmlSanitizerExtension($this->sanitizers()))
+            ->getFormFactory()
+            ->create(PersonalDataType::class, $personalData, $options)
+        ;
+    }
+
+    private function sanitizers(): ContainerInterface
+    {
+        return new class implements ContainerInterface {
+            public function get(string $id): HtmlSanitizer
+            {
+                return new HtmlSanitizer(new HtmlSanitizerConfig());
+            }
+
+            public function has(string $id): bool
+            {
+                return true;
+            }
+        };
+    }
+}