From 50c842cc9d8ed1937d9bcbdbd3b814f48ad9b8df Mon Sep 17 00:00:00 2001 From: Philippe Delodder <7259755+phdelodder@users.noreply.github.com> Date: Mon, 19 Oct 2026 14:35:50 +0200 Subject: [PATCH] Reconnecting an integration edits it, and a taken name is a form error (#217) The setup wizard built a new connection every time its Integrations step was saved, so saving it again collided with the unique key on (user, provider, name) and answered with a 500. It now edits the oldest existing connection for that service. The settings page had the same 500 for a second connection that kept the default name; a UniqueEntity constraint now reports it on the name field. Closes #216. Co-authored-by: Philippe Delodder Co-authored-by: Claude Opus 5.5 --- CHANGELOG.md | 8 + src/Entity/Integration/Integration.php | 6 + .../Step/IntegrationsStepHandler.php | 9 +- .../Integration/IntegrationNameTakenTest.php | 174 ++++++++++++++++++ translations/validators.de.yaml | 1 + translations/validators.en.yaml | 1 + translations/validators.en_PI.yaml | 1 + translations/validators.fr.yaml | 1 + 8 files changed, 200 insertions(+), 1 deletion(-) create mode 100644 tests/Controller/Integration/IntegrationNameTakenTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 159c4819..6d547b23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,14 @@ The published image tags: `latest` follows the most recent release below, ## Unreleased +**Reconnecting a service in the setup wizard no longer fails.** Opening the Integrations step for +a service that was already connected and saving it again answered with an error page, because the +step built a second connection with the same name instead of editing the first. It now edits the +existing one, and a blank secret still keeps the stored one. The settings page had the same error +when a second connection to a service (a work Nextcloud next to the home one) kept the default +name; it now asks for another name instead. No schema change. +[#216](https://github.com/karatektus/pl_mail/issues/216), by @phdelodder. + **Inline images in a message show up again.** Pictures that a mail carries inside itself, such as a logo or the icons in a booking confirmation, rendered as empty boxes: the message body is shown in a sandbox that sends no session cookie, so each image was answered with a redirect to the login diff --git a/src/Entity/Integration/Integration.php b/src/Entity/Integration/Integration.php index 0d0a359a..41417cbb 100644 --- a/src/Entity/Integration/Integration.php +++ b/src/Entity/Integration/Integration.php @@ -15,6 +15,7 @@ use App\Infrastructure\Doctrine\Type\EncryptedStringType; use App\Repository\Integration\IntegrationRepository; use DateTimeImmutable; use Doctrine\ORM\Mapping as ORM; +use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity; /** * One user's connection to one external file or photo service. @@ -37,6 +38,11 @@ use Doctrine\ORM\Mapping as ORM; #[ORM\Table(name: 'integration')] #[ORM\UniqueConstraint(name: 'uniq_integration_user_provider_name', columns: ['usr_id', 'provider', 'name'])] #[ORM\Index(name: 'idx_integration_user', columns: ['usr_id'])] +// The unique key above, checked before the INSERT so a form can say so. A +// second Nextcloud that kept the default name used to reach the database and +// come back as a 500; now it is an error on the name field, which is the one +// the user can change (#216). +#[UniqueEntity(fields: ['usr', 'provider', 'name'], message: 'integration.name_taken', errorPath: 'name')] #[ORM\HasLifecycleCallbacks] class Integration { diff --git a/src/Service/Onboarding/Step/IntegrationsStepHandler.php b/src/Service/Onboarding/Step/IntegrationsStepHandler.php index 36e4f95a..60f44eb1 100644 --- a/src/Service/Onboarding/Step/IntegrationsStepHandler.php +++ b/src/Service/Onboarding/Step/IntegrationsStepHandler.php @@ -105,9 +105,16 @@ final readonly class IntegrationsStepHandler implements OnboardingStepHandlerInt $config = $this->connector->requireConnectable($provider); + // Reconnecting edits the existing connection: (user, provider, name) is + // unique, so a fresh entity would collide on save. Someone with two + // (home and work, added in settings) gets the oldest, the one the + // wizard would have made, and the same one every time. + $integration = $this->integrations->findOneBy(['usr' => $user, 'provider' => $provider], ['id' => 'ASC']) + ?? new Integration($user, $provider, $provider->label()); + return $this->formFactory->create( IntegrationConnectType::class, - new Integration($user, $provider, $provider->label()), + $integration, [ 'integration_provider' => $provider, 'url_editable' => $this->connector->isUrlEditable($provider, $config), diff --git a/tests/Controller/Integration/IntegrationNameTakenTest.php b/tests/Controller/Integration/IntegrationNameTakenTest.php new file mode 100644 index 00000000..ab71ce86 --- /dev/null +++ b/tests/Controller/Integration/IntegrationNameTakenTest.php @@ -0,0 +1,174 @@ +client = static::createClient(); + $this->client->disableReboot(); + + $this->em = static::getContainer()->get(EntityManagerInterface::class); + $user = static::getContainer()->get(UserRepository::class)->findOneBy(['email' => self::ADMIN_EMAIL]); + + if (null === $user) { + self::markTestSkipped('run `app:test:seed-user --admin` first'); + } + + $this->user = $user; + $this->em->getConnection()->beginTransaction(); + + // Whatever the e2e suite left behind: one connectable Nextcloud, and + // no connections of this user's. + $this->em->getConnection()->executeStatement("DELETE FROM integration_provider_config WHERE provider = 'nextcloud'"); + $this->em->getConnection()->executeStatement('DELETE FROM integration WHERE usr_id = :u', ['u' => $user->id]); + + $config = new IntegrationProviderConfig(Provider::Nextcloud); + $config->isEnabled = true; + $this->em->persist($config); + $this->em->flush(); + + $this->client->loginUser($user); + } + + protected function tearDown(): void + { + if (true === $this->em->getConnection()->isTransactionActive()) { + $this->em->getConnection()->rollBack(); + } + + parent::tearDown(); + } + + public function testASecondConnectionWithTheSameNameIsAFieldErrorNotA500(): void + { + $this->connection('Nextcloud', 'home'); + + $this->submit('/settings/integrations/connect/nextcloud', ['username' => 'work', 'secret' => 'pw']); + + self::assertResponseStatusCodeSame(422); + self::assertStringContainsString( + 'You already have a connection to this service called "Nextcloud"', + $this->client->getCrawler()->filter('form[name="integration_connect"]')->text(), + ); + self::assertSame(['Nextcloud' => 'home'], $this->stored()); + } + + /** Two of the same service is allowed; only the name has to differ. */ + public function testASecondConnectionWithItsOwnNameIsSaved(): void + { + $this->connection('Nextcloud', 'home'); + + $this->submit('/settings/integrations/connect/nextcloud', ['name' => 'Work', 'username' => 'work', 'secret' => 'pw']); + + self::assertResponseIsSuccessful(); + self::assertSame(['Nextcloud' => 'home', 'Work' => 'work'], $this->stored()); + } + + public function testSavingTheWizardStepAgainEditsTheConnection(): void + { + $this->connection('Nextcloud', 'old'); + + $crawler = $this->client->request('GET', '/onboarding/integrations?connect=nextcloud'); + self::assertSame('old', $this->field($crawler, 'username'), 'the step opens on what is connected'); + + $this->submit('/onboarding/integrations?connect=nextcloud', ['username' => 'new']); + + self::assertResponseRedirects(); + self::assertSame(['Nextcloud' => 'new'], $this->stored()); + } + + /** Home and work, both added in settings: the wizard edits the older one, every time. */ + public function testTheWizardEditsTheOldestOfTwo(): void + { + $this->connection('Nextcloud', 'home'); + $this->connection('Work', 'work'); + + $crawler = $this->client->request('GET', '/onboarding/integrations?connect=nextcloud'); + + self::assertSame('Nextcloud', $this->field($crawler, 'name')); + self::assertSame('home', $this->field($crawler, 'username')); + } + + private function connection(string $name, string $username): void + { + $integration = new Integration($this->user, Provider::Nextcloud, $name); + $integration->baseUrl = self::NOWHERE; + $integration->username = $username; + $integration->secret = 'stored-secret'; + + $this->em->persist($integration); + $this->em->flush(); + } + + /** + * Opens the form at $url, fills in the server address and $fields, submits. + * + * @param array $fields field name inside `integration_connect` => value + */ + private function submit(string $url, array $fields): void + { + $form = $this->client->request('GET', $url)->filter('form[name="integration_connect"]')->form(); + + $form['integration_connect[baseUrl]'] = self::NOWHERE; + + foreach ($fields as $field => $value) { + $form['integration_connect[' . $field . ']'] = $value; + } + + $this->client->submit($form); + } + + private function field(Crawler $crawler, string $field): string + { + return (string) $crawler->filter('[name="integration_connect[' . $field . ']"]')->attr('value'); + } + + /** @return array name => username, for this user's Nextclouds */ + private function stored(): array + { + /** @var array */ + return $this->em->getConnection()->fetchAllKeyValue( + "SELECT name, username FROM integration WHERE usr_id = :u AND provider = 'nextcloud' ORDER BY name", + ['u' => $this->user->id], + ); + } +} diff --git a/translations/validators.de.yaml b/translations/validators.de.yaml index 93f1a686..a0a79ece 100644 --- a/translations/validators.de.yaml +++ b/translations/validators.de.yaml @@ -20,6 +20,7 @@ caldav.username_required: Gib den Benutzernamen ein, unter dem dich dein CalDAV- calendar.name_required: Gib dem Kalender einen Namen — daran erkennst du ihn zwischen den anderen. compose.recipient_invalid: "„{{ address }}“ ist keine gültige E-Mail-Adresse." compose.recipient_required: Füge mindestens einen Empfänger hinzu, bevor du sendest. +integration.name_taken: Du hast schon eine Verbindung zu diesem Dienst mit dem Namen {{ value }}. Gib dieser einen anderen Namen — etwa Privat und Arbeit. settings.password.current_required: Gib dein aktuelles Passwort ein — es verhindert, dass jemand an deinem offenen Bildschirm es einfach ändert. settings.password.current_wrong: Das ist nicht dein aktuelles Passwort. settings.password.mismatch: Die beiden neuen Passwörter stimmen nicht überein. diff --git a/translations/validators.en.yaml b/translations/validators.en.yaml index feb1a1ea..407f490b 100644 --- a/translations/validators.en.yaml +++ b/translations/validators.en.yaml @@ -21,6 +21,7 @@ caldav.username_required: Enter the username your CalDAV server knows you by. calendar.name_required: Give the calendar a name — it is what tells it apart from the others. compose.recipient_invalid: "“{{ address }}” is not a valid email address." compose.recipient_required: Add at least one recipient before sending. +integration.name_taken: You already have a connection to this service called {{ value }}. Give this one another name — home and work, say. settings.password.current_required: Enter your current password — it is what stops somebody at your unlocked screen changing it. settings.password.current_wrong: That is not your current password. settings.password.mismatch: The two new passwords don't match. diff --git a/translations/validators.en_PI.yaml b/translations/validators.en_PI.yaml index 1a760fb2..ca2dc8fb 100644 --- a/translations/validators.en_PI.yaml +++ b/translations/validators.en_PI.yaml @@ -19,6 +19,7 @@ caldav.username_required: Give the name yer CalDAV port knows ye by. calendar.name_required: Name the almanac, else ye'll not tell it from the rest. compose.recipient_invalid: "“{{ address }}” be no proper address, matey." compose.recipient_required: Name at least one soul afore ye cast off. +integration.name_taken: Ye already have a line to this port named {{ value }}. Christen this one somethin' else — home an' work, say. settings.password.current_required: Say yer current word — 'tis what keeps a passing hand from changing it at yer open desk. settings.password.current_wrong: That be not yer current word. settings.password.mismatch: Th' two new words don't match. diff --git a/translations/validators.fr.yaml b/translations/validators.fr.yaml index e812b187..edf9d614 100644 --- a/translations/validators.fr.yaml +++ b/translations/validators.fr.yaml @@ -20,6 +20,7 @@ caldav.username_required: Saisissez le nom d’utilisateur sous lequel votre ser calendar.name_required: Donnez un nom à l’agenda — c’est ce qui le distingue des autres. compose.recipient_invalid: "« {{ address }} » n’est pas une adresse e-mail valide." compose.recipient_required: Ajoutez au moins un destinataire avant d’envoyer. +integration.name_taken: Vous avez déjà une connexion à ce service nommée {{ value }}. Donnez un autre nom à celle-ci — maison et travail, par exemple. settings.password.current_required: Saisissez votre mot de passe actuel — c’est ce qui empêche quelqu’un devant votre écran déverrouillé de le changer. settings.password.current_wrong: Ce n’est pas votre mot de passe actuel. settings.password.mismatch: Les deux nouveaux mots de passe ne correspondent pas.