diff --git a/src/Controller/Account/PersonalDataController.php b/src/Controller/Account/PersonalDataController.php index 0e3ae87..fb65892 100644 --- a/src/Controller/Account/PersonalDataController.php +++ b/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, ]); - $personalDataForm->handleRequest($request); + + // 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 { diff --git a/src/Form/PersonalDataType.php b/src/Form/PersonalDataType.php index 231a529..4b94943 100644 --- a/src/Form/PersonalDataType.php +++ b/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 */ 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') + ; + } } diff --git a/templates/account/personal_data.html.twig b/templates/account/personal_data.html.twig index b62e9fe..895a123 100644 --- a/templates/account/personal_data.html.twig +++ b/templates/account/personal_data.html.twig @@ -45,6 +45,7 @@
{{ form_start(personalDataForm) }} + {{ form_errors(personalDataForm) }}

@@ -56,7 +57,10 @@

Kontakt

- {{ 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' } }) }}
diff --git a/templates/forms.html.twig b/templates/forms.html.twig index 54dff11..0c73735 100644 --- a/templates/forms.html.twig +++ b/templates/forms.html.twig @@ -86,9 +86,6 @@ {%- endif -%} {{ parent() }} - {%- if disabled is defined and disabled == true -%} - - {%- endif -%}
{%- endblock form_widget_simple -%} @@ -107,9 +104,6 @@ {%- endif -%} {{ parent() }} - {%- if disabled is defined and disabled == true -%} - - {%- endif -%}
{%- endblock textarea_widget -%} diff --git a/templates/forms_admin.html.twig b/templates/forms_admin.html.twig index 02d8682..b38d04a 100644 --- a/templates/forms_admin.html.twig +++ b/templates/forms_admin.html.twig @@ -46,9 +46,6 @@ {%- endif -%} {%- endif -%} {{ parent() }} - {%- if disabled is defined and disabled == true -%} - - {%- 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 -%} - - {%- 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 -%} - - {%- endif -%} {%- endblock choice_widget_collapsed -%} {%- block choice_widget_expanded -%} diff --git a/tests/BusProNet/XmlParser/PersonalDataParserTest.php b/tests/BusProNet/XmlParser/PersonalDataParserTest.php new file mode 100644 index 0000000..0a73834 --- /dev/null +++ b/tests/BusProNet/XmlParser/PersonalDataParserTest.php @@ -0,0 +1,87 @@ +parser = new PersonalDataParser(); + } + + public function testParseReadsCommunicationOfASinglePerson(): void + { + $personalData = $this->parse($this->responseXml(['mia@example.com'])); + + self::assertSame('mia@example.com', $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(['mia@example.com', 'tom@example.com'])); + + self::assertSame('mia@example.com', $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 $emails + */ + private function responseXml(array $emails): string + { + $communication = ''; + + foreach ($emails as $email) { + $communication .= sprintf( + '%s0170 1234567', + $email, + ); + } + + return << + + 4711 + 815 + + Mia + Muster + w + d + + Musterweg 1 + 12345 + Musterstadt + D + + {$communication} + + + XML; + } +} diff --git a/tests/Controller/Account/PersonalDataControllerTest.php b/tests/Controller/Account/PersonalDataControllerTest.php index 33a63bd..68edd7c 100644 --- a/tests/Controller/Account/PersonalDataControllerTest.php +++ b/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' => 'attacker@example.com'], + ])); + + self::assertSame(200, $response->getStatusCode()); + self::assertSame('account/personal_data.html.twig', $controller->renderedView); + } + private function createController(): TestablePersonalDataController { $user = new User('customer@example.com'); @@ -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 = 'customer@example.com'; return $personalData; } @@ -208,6 +255,11 @@ final class TestablePersonalDataController extends PersonalDataController public string $renderedView = ''; + /** + * @var array + */ + 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; } diff --git a/tests/Form/PersonalDataTypeTest.php b/tests/Form/PersonalDataTypeTest.php new file mode 100644 index 0000000..e798be5 --- /dev/null +++ b/tests/Form/PersonalDataTypeTest.php @@ -0,0 +1,119 @@ +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 = 'household@example.com'; + + $form = $this->createForm($personalData, ['email_editable' => false]); + $form->submit($this->submittedValues(['email' => 'attacker@example.com'])); + + self::assertTrue($form->isSynchronized()); + self::assertSame('household@example.com', $personalData->communication->email); + } + + public function testSubmittedEmailIsAppliedByDefault(): void + { + $personalData = new PersonalData(); + $personalData->communication->email = 'household@example.com'; + + $form = $this->createForm($personalData); + $form->submit($this->submittedValues(['email' => 'new@example.com'])); + + self::assertTrue($form->isSynchronized()); + self::assertSame('new@example.com', $personalData->communication->email); + } + + /** + * @param array $overrides + * + * @return array + */ + 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 $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; + } + }; + } +}