feat: dedicated login page for oauth2 logins

src/Controller/SecurityController.php

@@ -5,6 +5,7 @@ namespace App\Controller;
 use App\Form\Model\BookingDto;
 use App\Service\BookingSessionManager;
 use App\Service\BookingSummaryAssembler;
+use League\Bundle\OAuth2ServerBundle\Manager\ClientManagerInterface;
 use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
@@ -21,13 +22,28 @@ class SecurityController extends AbstractController
         Request $request,
         BookingSessionManager $bookingSessionService,
         BookingSummaryAssembler $summaryDataService,
+        ClientManagerInterface $clientManager,
     ): Response {
+        // flag this request in the session as external OAuth2 request if
+        // applicable to immediately logout the current user after successful
+        // authentication (see App\EventListener\AuthorizationCodeListener).
+        // Read before the booking flow is decided, because it overrules it.
+        $session = $request->getSession();
+        $targetPath = $session->get('_security.main.target_path');
+        $isOauth2 = \is_string($targetPath) && str_contains($targetPath, '/authorize');
+        $session->set('_oauth2', $isOauth2);
+
         // Check if this is a booking flow (BookingDto exists in session)
         $bookingDto = $bookingSessionService->getBookingDto($request, BookingDto::MODE_CREATE);
-        $isBookingFlow = null !== $bookingDto;
+        $hasPendingBooking = null !== $bookingDto;
+
+        // A pending authorization wins over a half-finished booking. Otherwise the target path
+        // below is overwritten with booking step 1, the user is sent there after logging in, and
+        // the application that asked for the authorization never receives its code.
+        $isBookingFlow = $hasPendingBooking && false === $isOauth2;
 
         // If authenticated and in booking flow, proceed to Step 1
-        if (null !== $this->getUser() && true === $isBookingFlow) {
+        if (null !== $this->getUser() && true === $hasPendingBooking) {
             return $this->redirectToRoute('app_booking_create_step_1');
         }
 
@@ -39,21 +55,18 @@ class SecurityController extends AbstractController
         $error = $authenticationUtils->getLastAuthenticationError();
         $lastUsername = $authenticationUtils->getLastUsername();
 
-        // flag this request in the session as external OAuth2 request if
-        // applicable to immediately logout the current user after successful
-        // authentication (see App\EventListener\AuthorizationCodeListener).
-        $session = $request->getSession();
-        $targetPath = $session->get('_security.main.target_path');
-        $isOauth2 = null !== $targetPath && str_contains($targetPath, '/authorize');
-        $session->set('_oauth2', $isOauth2);
-
         // For booking flow, set target path to Step 1 (after successful auth, redirect there)
         if (true === $isBookingFlow) {
             $session->set('_security.main.target_path', $this->generateUrl('app_booking_create_step_1'));
         }
 
-        // Render booking login template if in booking flow, otherwise standard login
-        $template = true === $isBookingFlow ? 'booking/create/authenticate.html.twig' : 'security/login.html.twig';
+        // Render booking login template if in booking flow, the self-explaining one when another
+        // application sent them here to authorize it, otherwise standard login
+        $template = match (true) {
+            $isBookingFlow => 'booking/create/authenticate.html.twig',
+            $isOauth2 => 'security/login_oauth2.html.twig',
+            default => 'security/login.html.twig',
+        };
 
         // Fetch CMS data for booking flow (uses cache warmed in IndexController)
         $cmsData = null;
@@ -72,9 +85,37 @@ class SecurityController extends AbstractController
             'travel_date_to' => $bookingDto?->travel->dateTo,
             'cmsData' => $cmsData,
             'oauth2' => $isOauth2,
+            'client_name' => $isOauth2 ? $this->authorizingClientName($clientManager, $targetPath) : null,
         ]);
     }
 
+    /**
+     * The display name of the application that sent the user here to authorize it, so the login page
+     * can name it instead of leaving the user wondering why the site changed.
+     *
+     * Derived from the pending target path on every render rather than stashed on the session: a
+     * failed login redirects back to this route and the target path survives, so the name comes back
+     * with it. Returns null when the client cannot be resolved, and the page falls back to neutral
+     * wording rather than failing -- this runs before authentication.
+     */
+    private function authorizingClientName(ClientManagerInterface $clientManager, ?string $targetPath): ?string
+    {
+        $query = null !== $targetPath ? parse_url($targetPath, \PHP_URL_QUERY) : null;
+        if (!\is_string($query) || '' === $query) {
+            return null;
+        }
+
+        parse_str($query, $params);
+        $clientId = $params['client_id'] ?? null;
+        if (!\is_string($clientId) || '' === $clientId) {
+            return null;
+        }
+
+        $name = $clientManager->find($clientId)?->getName() ?? '';
+
+        return '' !== $name ? $name : null;
+    }
+
     #[Route('/logout', name: 'app_logout')]
     #[IsGranted('ROLE_USER')]
     public function logout(): void

templates/_partials/_header.html.twig

@@ -1,18 +1,6 @@
 <div class="flex items-center justify-between">
     <a href="{{ path('app_account') }}" title="zu meinem Account">
-        {# The operating entity depends on where the accommodation is, so its logo wins over the host's #}
-        {% set country_logo = accommodation|default(null) and accommodation.country ? accommodation.country.logo : null %}
-        {% if country_logo %}
-            <img src="{{ asset('build/images/' ~ country_logo) }}" alt="" class="h-14 my-1 w-auto">
-        {% elseif booking_theme() == 'sbw' %}
-            <img src="{{ asset('build/images/logo_sbw.svg') }}" alt="" class="h-10 my-2 w-auto">
-        {% elseif booking_theme() == 'ser' %}
-            <img src="{{ asset('build/images/logo_ser.svg') }}" alt="" class="h-10 my-2 w-auto">
-        {% elseif booking_theme() == 'snz' %}
-            <img src="{{ asset('build/images/logo_snz.svg') }}" alt="" class="h-10 my-2 w-auto">
-        {% else %}
-            <img src="{{ asset('build/images/logo_ep.svg') }}" alt="" class="h-10 my-2 w-auto">
-        {% endif %}
+        {% include '_partials/_logo.html.twig' %}
     </a>
     <div class="flex items-center space-x-2">
         {% if is_granted('ROLE_USER') %}

templates/_partials/_login_form.html.twig

@@ -0,0 +1,47 @@
+{# The login form itself, shared by security/login.html.twig and security/login_oauth2.html.twig so
+   the field names, the CSRF token id and the error rendering cannot drift between the two pages.
+   Expects `error` and `last_username` from the including template's context. #}
+{% if error %}
+    {% include '_partials/_alert.html.twig' with { 'level': 'error', messages: [ error.messageKey|trans(error.messageData, 'security') ] } %}
+{% endif %}
+<div class="pb-4">
+    <form action="{{ path('app_login') }}" method="post">
+        <div class="mb-4">
+            <label for="username" class="mb-1 font-semibold text-white">
+                E-Mail:
+            </label>
+            <input type="email"
+                   id="username"
+                   name="_username"
+                   value="{{ last_username }}"
+                   class="form-field"
+                   required
+                   autocomplete="username">
+        </div>
+        <div class="mb-4" {{ stimulus_controller('password-reveal') }}>
+            <label for="password" class="mb-1 font-semibold text-white">
+                Passwort:
+            </label>
+            <div class="relative">
+                <input type="password"
+                       id="password"
+                       name="_password"
+                       class="form-field"
+                        {{ stimulus_target('password-reveal', 'field') }}
+                       required
+                       autocomplete="current-password">
+                <button type="button"
+                        {{ stimulus_action('password-reveal', 'toggle') }}
+                        class="absolute top-0 right-0 mt-2 mr-2">
+                    {{ icon('eye', 'w-6 h-6 text-gray-500') }}
+                </button>
+            </div>
+        </div>
+        <div class="flex flex-col lg:flex-row space-y-2 lg:space-y-0 lg:space-x-2 mt-4">
+            <button type="submit" class="button button--primary">
+                Login
+            </button>
+        </div>
+        <input type="hidden" name="_csrf_token" value="{{ csrf_token('authenticate') }}">
+    </form>
+</div>

templates/_partials/_logo.html.twig

@@ -0,0 +1,15 @@
+{# The brand logo, extracted from _header.html.twig so the OAuth2 login chrome can show the same
+   logo without inheriting the account links that go with it. Takes an optional `accommodation`. #}
+{# The operating entity depends on where the accommodation is, so its logo wins over the host's #}
+{% set country_logo = accommodation|default(null) and accommodation.country ? accommodation.country.logo : null %}
+{% if country_logo %}
+    <img src="{{ asset('build/images/' ~ country_logo) }}" alt="" class="h-14 my-1 w-auto">
+{% elseif booking_theme() == 'sbw' %}
+    <img src="{{ asset('build/images/logo_sbw.svg') }}" alt="" class="h-10 my-2 w-auto">
+{% elseif booking_theme() == 'ser' %}
+    <img src="{{ asset('build/images/logo_ser.svg') }}" alt="" class="h-10 my-2 w-auto">
+{% elseif booking_theme() == 'snz' %}
+    <img src="{{ asset('build/images/logo_snz.svg') }}" alt="" class="h-10 my-2 w-auto">
+{% else %}
+    <img src="{{ asset('build/images/logo_ep.svg') }}" alt="" class="h-10 my-2 w-auto">
+{% endif %}

templates/layout_oauth2.html.twig

@@ -0,0 +1,14 @@
+{# Chrome for a login that is serving an external OAuth2 authorization.
+
+   Only the logo survives from the normal header: the account and logout links are dead ends in the
+   middle of an authorization, and the marketing column of the standard login page is noise when the
+   user did not come here to read about MyE&P. The footer's legal links stay -- this page collects
+   credentials, so dropping them would be the wrong direction.
+
+   The background is inherited unchanged, which is why the shared form partial's `text-white`
+   styling still works here. #}
+{% extends 'layout.html.twig' %}
+
+{% block header %}
+    {% include '_partials/_logo.html.twig' %}
+{% endblock %}

templates/security/login.html.twig

@@ -7,50 +7,7 @@
                 Login<br>MyE&amp;P
             </h1>
             {% include '_partials/_flashes.html.twig' %}
-            {% if error %}
-                {% include '_partials/_alert.html.twig' with { 'level': 'error', messages: [ error.messageKey|trans(error.messageData, 'security') ] } %}
-            {% endif %}
-            <div class="pb-4">
-                <form action="{{ path('app_login') }}" method="post" hx-history="false">
-                    <div class="mb-4">
-                        <label for="username" class="mb-1 font-semibold text-white">
-                            E-Mail:
-                        </label>
-                        <input type="email"
-                               id="username"
-                               name="_username"
-                               value="{{ last_username }}"
-                               class="form-field"
-                               required
-                               autocomplete="username">
-                    </div>
-                    <div class="mb-4" {{ stimulus_controller('password-reveal') }}>
-                        <label for="password" class="mb-1 font-semibold text-white">
-                            Passwort:
-                        </label>
-                        <div class="relative">
-                            <input type="password"
-                                   id="password"
-                                   name="_password"
-                                   class="form-field"
-                                    {{ stimulus_target('password-reveal', 'field') }}
-                                   required
-                                   autocomplete="current-password">
-                            <button type="button"
-                                    {{ stimulus_action('password-reveal', 'toggle') }}
-                                    class="absolute top-0 right-0 mt-2 mr-2">
-                                {{ icon('eye', 'w-6 h-6 text-gray-500') }}
-                            </button>
-                        </div>
-                    </div>
-                    <div class="flex flex-col lg:flex-row space-y-2 lg:space-y-0 lg:space-x-2 mt-4">
-                        <button type="submit" class="button button--primary">
-                            Login
-                        </button>
-                    </div>
-                    <input type="hidden" name="_csrf_token" value="{{ csrf_token('authenticate') }}">
-                </form>
-            </div>
+            {% include '_partials/_login_form.html.twig' %}
             <div class="flex space-x-2">
                 <a href="{{ path('app_reset_password') }}"
                    class="text-white underline">

templates/security/login_oauth2.html.twig

@@ -0,0 +1,37 @@
+{# The login page as seen by someone who was sent here by another E&P application (ephub, myep-team)
+   to authorize it. Its job is to explain why they are suddenly on a different website and that they
+   will be sent back, which the standard login page never said.
+
+   The grid is the one from security/login.html.twig, minus its marketing column: that keeps the
+   form column at exactly the width it has on the standard login page. #}
+{% extends 'layout_oauth2.html.twig' %}
+
+{% block content %}
+    <div class="grid grid-cols-1 md:grid-cols-2 gap-8 px-4 lg:px-8 py-8 lg:py-16">
+        <div>
+            <h1 class="text-white uppercase pb-4 mb-8 border-b border-primary-bg/40">
+                {% if client_name %}
+                    Anmelden für<br>{{ client_name }}
+                {% else %}
+                    Login<br>MyE&amp;P
+                {% endif %}
+            </h1>
+
+            <p class="mb-6 text-white">
+                Melde dich mit deinem MyE&amp;P-Konto an. Anschließend wirst du automatisch
+                {% if client_name %}zu {{ client_name }} {% endif %}zurückgeleitet.
+            </p>
+
+            {% include '_partials/_login_form.html.twig' %}
+
+            <div class="flex space-x-2">
+                <a href="{{ path('app_reset_password') }}" class="text-white underline">
+                    Passwort vergessen?
+                </a>
+                <a href="{{ path('app_registration') }}" class="text-white underline">
+                    Jetzt registrieren
+                </a>
+            </div>
+        </div>
+    </div>
+{% endblock %}

tests/Controller/SecurityControllerTest.php

@@ -0,0 +1,208 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Controller;
+
+use App\BusProNet\Model\Travel;
+use App\Controller\SecurityController;
+use App\Form\Model\BookingDto;
+use App\Service\BookingSessionManager;
+use App\Service\BookingSummaryAssembler;
+use League\Bundle\OAuth2ServerBundle\Manager\ClientManagerInterface;
+use League\Bundle\OAuth2ServerBundle\Model\Client;
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\HttpFoundation\Session\Session;
+use Symfony\Component\HttpFoundation\Session\Storage\MockArraySessionStorage;
+use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
+use Symfony\Component\Security\Core\User\UserInterface;
+use Symfony\Component\Security\Http\Authentication\AuthenticationUtils;
+
+class SecurityControllerTest extends TestCase
+{
+    private const AUTHORIZE_PATH = '/authorize?response_type=code&client_id=abc123&redirect_uri=https%3A%2F%2Fephub.example%2Fcallback';
+
+    public function testAnAuthorizeTargetPathRendersTheSelfExplainingLoginPageNamingTheClient(): void
+    {
+        $controller = $this->createController();
+        $controller->clientManager
+            ->expects(self::once())
+            ->method('find')
+            ->with('abc123')
+            ->willReturn(new Client('EPHub', 'abc123', null));
+
+        $response = $controller->login(...$this->arguments($controller, self::AUTHORIZE_PATH));
+
+        self::assertSame(200, $response->getStatusCode());
+        self::assertSame('security/login_oauth2.html.twig', $controller->renderedView);
+        self::assertTrue($controller->renderedParameters['oauth2']);
+        self::assertSame('EPHub', $controller->renderedParameters['client_name']);
+    }
+
+    public function testWithoutAnAuthorizeTargetPathTheStandardLoginPageIsRendered(): void
+    {
+        $controller = $this->createController();
+        $controller->clientManager->expects(self::never())->method('find');
+
+        $response = $controller->login(...$this->arguments($controller, null));
+
+        self::assertSame(200, $response->getStatusCode());
+        self::assertSame('security/login.html.twig', $controller->renderedView);
+        self::assertFalse($controller->renderedParameters['oauth2']);
+        self::assertNull($controller->renderedParameters['client_name']);
+    }
+
+    /**
+     * The name is cosmetic and this runs before authentication, so an unknown client must degrade
+     * to neutral wording rather than break the login.
+     */
+    public function testAnUnresolvableClientIdFallsBackToNeutralWording(): void
+    {
+        $controller = $this->createController();
+        $controller->clientManager
+            ->expects(self::once())
+            ->method('find')
+            ->willReturn(null);
+
+        $response = $controller->login(...$this->arguments($controller, self::AUTHORIZE_PATH));
+
+        self::assertSame(200, $response->getStatusCode());
+        self::assertSame('security/login_oauth2.html.twig', $controller->renderedView);
+        self::assertNull($controller->renderedParameters['client_name']);
+    }
+
+    public function testAnAuthorizeTargetPathWithoutAClientIdFallsBackToNeutralWording(): void
+    {
+        $controller = $this->createController();
+        $controller->clientManager->expects(self::never())->method('find');
+
+        $response = $controller->login(...$this->arguments($controller, '/authorize'));
+
+        self::assertSame(200, $response->getStatusCode());
+        self::assertSame('security/login_oauth2.html.twig', $controller->renderedView);
+        self::assertNull($controller->renderedParameters['client_name']);
+    }
+
+    /**
+     * A booking left half-finished in the same browser used to overwrite the pending target path
+     * with booking step 1, so the application that asked for the authorization never got its code.
+     */
+    public function testAPendingAuthorizationWinsOverAHalfFinishedBooking(): void
+    {
+        $controller = $this->createController(bookingDto: $this->createBookingDto());
+        $controller->clientManager
+            ->expects(self::once())
+            ->method('find')
+            ->willReturn(new Client('EPHub', 'abc123', null));
+
+        $session = $this->createSession(self::AUTHORIZE_PATH);
+        $response = $controller->login(...$this->arguments($controller, self::AUTHORIZE_PATH, $session));
+
+        self::assertSame(200, $response->getStatusCode());
+        self::assertSame('security/login_oauth2.html.twig', $controller->renderedView);
+        self::assertSame(self::AUTHORIZE_PATH, $session->get('_security.main.target_path'));
+    }
+
+    public function testAHalfFinishedBookingStillWinsWhenThereIsNoAuthorization(): void
+    {
+        $controller = $this->createController(bookingDto: $this->createBookingDto());
+        $controller->clientManager->expects(self::never())->method('find');
+
+        $session = $this->createSession(null);
+        $response = $controller->login(...$this->arguments($controller, null, $session));
+
+        self::assertSame(200, $response->getStatusCode());
+        self::assertSame('booking/create/authenticate.html.twig', $controller->renderedView);
+        self::assertSame('/booking/create/step-1', $session->get('_security.main.target_path'));
+    }
+
+    /**
+     * @return array{AuthenticationUtils, Request, BookingSessionManager, BookingSummaryAssembler, ClientManagerInterface}
+     */
+    private function arguments(TestableSecurityController $controller, ?string $targetPath, ?Session $session = null): array
+    {
+        $request = Request::create('/', 'GET');
+        $request->setSession($session ?? $this->createSession($targetPath));
+
+        return [
+            $controller->authenticationUtils,
+            $request,
+            $controller->bookingSessionManager,
+            $controller->summaryAssembler,
+            $controller->clientManager,
+        ];
+    }
+
+    private function createSession(?string $targetPath): Session
+    {
+        $session = new Session(new MockArraySessionStorage());
+        if (null !== $targetPath) {
+            $session->set('_security.main.target_path', $targetPath);
+        }
+
+        return $session;
+    }
+
+    private function createBookingDto(): BookingDto
+    {
+        $travel = new Travel();
+        $travel->id = 123;
+
+        return new BookingDto($travel, 77);
+    }
+
+    private function createController(?BookingDto $bookingDto = null): TestableSecurityController
+    {
+        $authenticationUtils = $this->createStub(AuthenticationUtils::class);
+        $authenticationUtils->method('getLastAuthenticationError')->willReturn(null);
+        $authenticationUtils->method('getLastUsername')->willReturn('');
+
+        $bookingSessionManager = $this->createStub(BookingSessionManager::class);
+        $bookingSessionManager->method('getBookingDto')->willReturn($bookingDto);
+
+        return new TestableSecurityController(
+            $authenticationUtils,
+            $bookingSessionManager,
+            $this->createStub(BookingSummaryAssembler::class),
+            $this->createMock(ClientManagerInterface::class),
+        );
+    }
+}
+
+final class TestableSecurityController extends SecurityController
+{
+    /**
+     * @var array<string, mixed>
+     */
+    public array $renderedParameters = [];
+
+    public string $renderedView = '';
+
+    public function __construct(
+        public readonly AuthenticationUtils $authenticationUtils,
+        public readonly BookingSessionManager $bookingSessionManager,
+        public readonly BookingSummaryAssembler $summaryAssembler,
+        public readonly ClientManagerInterface $clientManager,
+    ) {
+    }
+
+    protected function getUser(): ?UserInterface
+    {
+        return null;
+    }
+
+    protected function render(string $view, array $parameters = [], ?Response $response = null): Response
+    {
+        $this->renderedView = $view;
+        $this->renderedParameters = $parameters;
+
+        return new Response('ok');
+    }
+
+    protected function generateUrl(string $route, array $parameters = [], int $referenceType = UrlGeneratorInterface::ABSOLUTE_PATH): string
+    {
+        return '/booking/create/step-1';
+    }
+}