diff --git a/src/Controller/SecurityController.php b/src/Controller/SecurityController.php index e6b85d26..82952878 100644 --- a/src/Controller/SecurityController.php +++ b/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 diff --git a/templates/_partials/_header.html.twig b/templates/_partials/_header.html.twig index 6c1a697d..9e0dbfb4 100644 --- a/templates/_partials/_header.html.twig +++ b/templates/_partials/_header.html.twig @@ -1,18 +1,6 @@
- {# 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 %} - - {% elseif booking_theme() == 'sbw' %} - - {% elseif booking_theme() == 'ser' %} - - {% elseif booking_theme() == 'snz' %} - - {% else %} - - {% endif %} + {% include '_partials/_logo.html.twig' %}
{% if is_granted('ROLE_USER') %} diff --git a/templates/_partials/_login_form.html.twig b/templates/_partials/_login_form.html.twig new file mode 100644 index 00000000..256dc2c1 --- /dev/null +++ b/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 %} +
+
+
+ + +
+
+ +
+ + +
+
+
+ +
+ +
+
diff --git a/templates/_partials/_logo.html.twig b/templates/_partials/_logo.html.twig new file mode 100644 index 00000000..741fb4e1 --- /dev/null +++ b/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 %} + +{% elseif booking_theme() == 'sbw' %} + +{% elseif booking_theme() == 'ser' %} + +{% elseif booking_theme() == 'snz' %} + +{% else %} + +{% endif %} diff --git a/templates/layout_oauth2.html.twig b/templates/layout_oauth2.html.twig new file mode 100644 index 00000000..299d239e --- /dev/null +++ b/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 %} diff --git a/templates/security/login.html.twig b/templates/security/login.html.twig index 50f86a45..61d982ef 100644 --- a/templates/security/login.html.twig +++ b/templates/security/login.html.twig @@ -7,50 +7,7 @@ Login
MyE&P {% include '_partials/_flashes.html.twig' %} - {% if error %} - {% include '_partials/_alert.html.twig' with { 'level': 'error', messages: [ error.messageKey|trans(error.messageData, 'security') ] } %} - {% endif %} -
-
-
- - -
-
- -
- - -
-
-
- -
- -
-
+ {% include '_partials/_login_form.html.twig' %}
diff --git a/templates/security/login_oauth2.html.twig b/templates/security/login_oauth2.html.twig new file mode 100644 index 00000000..cadad728 --- /dev/null +++ b/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 %} +
+
+

+ {% if client_name %} + Anmelden für
{{ client_name }} + {% else %} + Login
MyE&P + {% endif %} +

+ +

+ Melde dich mit deinem MyE&P-Konto an. Anschließend wirst du automatisch + {% if client_name %}zu {{ client_name }} {% endif %}zurückgeleitet. +

+ + {% include '_partials/_login_form.html.twig' %} + +
+ + Passwort vergessen? + + + Jetzt registrieren + +
+
+
+{% endblock %} diff --git a/tests/Controller/SecurityControllerTest.php b/tests/Controller/SecurityControllerTest.php new file mode 100644 index 00000000..08d8a971 --- /dev/null +++ b/tests/Controller/SecurityControllerTest.php @@ -0,0 +1,208 @@ +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 + */ + 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'; + } +}