Don't override already set language

cicnavi · cicnavi · commit c5faa099f0f3 · 2026-07-06T18:19:36.000+02:00
diff --git a/docs/6-oidc-upgrade.md b/docs/6-oidc-upgrade.md
@@ -185,11 +185,16 @@ per-field metadata policy (honored / validated / rejected) is documented in
 endpoints (previously ignored). The parameter carries the End-User's preferred
 UI languages as a space-separated list of BCP47 language tags, ordered by
 preference. The most preferred requested language which is also available in
-SimpleSAMLphp (per the `language.available` config option) is applied using the
-standard SimpleSAMLphp language cookie — the same mechanism as when the user
-picks a language on any SimpleSAMLphp page — so subsequent screens shown during
-the flow (login page, consent, logout page...) are rendered in the requested
-language. Matching includes a fallback to the primary language subtag (for
+SimpleSAMLphp (per the `language.available` config option) is used to render
+the screens shown during the flow. On the authorization endpoint, where the
+login page and consent are rendered by SimpleSAMLphp in subsequent requests,
+this is done via the standard SimpleSAMLphp language cookie (the same mechanism
+as when the user picks a language on any SimpleSAMLphp page); to avoid a
+client-supplied parameter silently overwriting a persistent, instance-wide
+preference, an already present language cookie (an explicit choice the user
+made) is not overridden. On the end session endpoint, the logout page is
+rendered directly in the requested language, without setting the language
+cookie. Matching includes a fallback to the primary language subtag (for
 example, requested `fr-CA` matches available `fr`). Per specification this is
 best-effort: if none of the requested languages are available, the parameter is
 ignored without raising an error. The available languages are also advertised
diff --git a/src/Bridges/SspBridge/Locale/Language.php b/src/Bridges/SspBridge/Locale/Language.php
@@ -14,6 +14,11 @@ public function setLanguageCookie(string $language): void
         SspLanguage::setLanguageCookie($language);
     }
 
+    public function getLanguageCookie(): ?string
+    {
+        return SspLanguage::getLanguageCookie();
+    }
+
     /**
      * Get the languages available in SimpleSAMLphp (configured in language.available and known to the
      * translation system), as computed by SimpleSAMLphp itself. The Language instance is created without
diff --git a/src/Controllers/AuthorizationController.php b/src/Controllers/AuthorizationController.php
@@ -136,6 +136,10 @@ public function authorization(Request $request): Response
      * page), so subsequent screens shown during the authentication flow (login page, consent...) are
      * rendered in the requested language. Per specification this is best-effort, so no error is raised
      * if none of the requested languages are available.
+     *
+     * The SimpleSAMLphp language cookie is a persistent, instance-wide preference, so an already present
+     * cookie (an explicit language choice the user, or a previous interaction, has made) is not
+     * overridden by the client-supplied ui_locales value.
      */
     protected function setUiLanguage(OAuth2AuthorizationRequestInterface $authorizationRequest): void
     {
@@ -149,6 +153,14 @@ protected function setUiLanguage(OAuth2AuthorizationRequestInterface $authorizat
             return;
         }
 
+        if ($this->sspBridge->locale()->language()->getLanguageCookie() !== null) {
+            $this->loggerService->debug(
+                'AuthorizationController: not overriding existing language cookie with ui_locales parameter.',
+                ['uiLocales' => $authorizationRequest->getUiLocales(), 'language' => $language],
+            );
+            return;
+        }
+
         $this->loggerService->debug(
             'AuthorizationController: setting UI language based on ui_locales parameter.',
             ['uiLocales' => $authorizationRequest->getUiLocales(), 'language' => $language],
diff --git a/src/Controllers/EndSessionController.php b/src/Controllers/EndSessionController.php
@@ -7,7 +7,6 @@
 use League\OAuth2\Server\Exception\OAuthServerException;
 use Psr\Http\Message\ServerRequestInterface;
 use SimpleSAML\Module\oidc\Bridges\PsrHttpBridge;
-use SimpleSAML\Module\oidc\Bridges\SspBridge;
 use SimpleSAML\Module\oidc\Factories\TemplateFactory;
 use SimpleSAML\Module\oidc\Server\AuthorizationServer;
 use SimpleSAML\Module\oidc\Server\LogoutHandlers\BackChannelLogoutHandler;
@@ -35,7 +34,6 @@ public function __construct(
         protected PsrHttpBridge $psrHttpBridge,
         protected ErrorResponder $errorResponder,
         protected UiLocalesResolver $uiLocalesResolver,
-        protected SspBridge $sspBridge,
     ) {
     }
 
@@ -55,7 +53,7 @@ public function __invoke(ServerRequestInterface $request): Response
 
         $logoutRequest = $this->authorizationServer->validateLogoutRequest($request);
 
-        $uiLanguage = $this->setUiLanguage($logoutRequest);
+        $uiLanguage = $this->resolveUiLanguage($logoutRequest);
 
         // Set indication that the logout is initiated using OIDC protocol. This will be checked in the
         // logoutHandler() method.
@@ -144,14 +142,16 @@ public function __invoke(ServerRequestInterface $request): Response
     }
 
     /**
-     * Set the UI language for the current user agent based on the ui_locales logout request parameter, if any of
-     * the requested languages are available in SimpleSAMLphp. This is done using the standard SimpleSAMLphp
-     * language cookie (same mechanism as when the user picks a language on any SimpleSAMLphp page). Per
-     * specification this is best-effort, so no error is raised if none of the requested languages are
-     * available. Returns the resolved language, so it can also be applied when rendering the logout
-     * page in the current request (the language cookie only affects subsequent requests).
+     * Resolve the UI language to use for the logout page based on the ui_locales logout request parameter, if
+     * any of the requested languages are available in SimpleSAMLphp. The resolved language is applied only when
+     * rendering the logout page in the current request (see resolveResponse()); unlike the authorization flow,
+     * the logout flow renders at most a single page and has no subsequent SimpleSAMLphp screens, so the
+     * persistent, instance-wide language cookie is intentionally not set here (it would be a side effect
+     * affecting later unrelated screens, including when a post_logout_redirect_uri is used and no page is
+     * rendered at all). Per specification this is best-effort, so no error is raised if none of the
+     * requested languages are available.
      */
-    protected function setUiLanguage(LogoutRequest $logoutRequest): ?string
+    protected function resolveUiLanguage(LogoutRequest $logoutRequest): ?string
     {
         $language = $this->uiLocalesResolver->resolve($logoutRequest->getUiLocales());
 
@@ -160,12 +160,10 @@ protected function setUiLanguage(LogoutRequest $logoutRequest): ?string
         }
 
         $this->loggerService->debug(
-            'EndSessionController: setting UI language based on ui_locales parameter.',
+            'EndSessionController: resolved UI language based on ui_locales parameter.',
             ['uiLocales' => $logoutRequest->getUiLocales(), 'language' => $language],
         );
 
-        $this->sspBridge->locale()->language()->setLanguageCookie($language);
-
         return $language;
     }
 
diff --git a/tests/unit/src/Controllers/AuthorizationControllerTest.php b/tests/unit/src/Controllers/AuthorizationControllerTest.php
@@ -563,4 +563,35 @@ public function testDoesNotSetUiLanguageWhenNoRequestedLanguageIsAvailable(): vo
 
         ($this->mock())($this->serverRequestStub);
     }
+
+    /**
+     * @throws \Throwable
+     */
+    public function testDoesNotOverrideExistingLanguageCookieWithUiLocales(): void
+    {
+        $this->authorizationRequestMock->method('getUiLocales')->willReturn('hr en');
+        $this->uiLocalesResolverStub->method('resolve')->willReturn('hr');
+        // An explicit language choice is already stored in the language cookie.
+        $this->sspBridgeLocaleLanguageMock->method('getLanguageCookie')->willReturn('de');
+
+        $this->authorizationServerStub
+            ->method('validateAuthorizationRequest')
+            ->willReturn($this->authorizationRequestMock);
+        $this->authorizationServerStub
+            ->method('completeAuthorizationRequest')
+            ->willReturn($this->responseStub);
+
+        $this->serverRequestStub->method('getQueryParams')->willReturn([]);
+
+        $this->authenticationServiceStub->method('manageState')->willReturn($this->state);
+        $this->authenticationServiceStub->method('getAuthenticateUser')->willReturn($this->userEntityStub);
+        $this->authenticationServiceStub
+            ->method('getAuthorizationRequestFromState')
+            ->willReturn($this->authorizationRequestMock);
+
+        $this->sspBridgeLocaleLanguageMock->expects($this->never())
+            ->method('setLanguageCookie');
+
+        ($this->mock())($this->serverRequestStub);
+    }
 }
diff --git a/tests/unit/src/Controllers/EndSessionControllerTest.php b/tests/unit/src/Controllers/EndSessionControllerTest.php
@@ -11,7 +11,6 @@
 use PHPUnit\Framework\TestCase;
 use SimpleSAML\Error\BadRequest;
 use SimpleSAML\Module\oidc\Bridges\PsrHttpBridge;
-use SimpleSAML\Module\oidc\Bridges\SspBridge;
 use SimpleSAML\Module\oidc\Controllers\EndSessionController;
 use SimpleSAML\Module\oidc\Factories\TemplateFactory;
 use SimpleSAML\Module\oidc\Server\AuthorizationServer;
@@ -25,6 +24,7 @@
 use SimpleSAML\OpenID\Codebooks\ClaimsEnum;
 use SimpleSAML\OpenID\Core\IdToken;
 use SimpleSAML\Session;
+use SimpleSAML\XHTML\Template;
 use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpFoundation\Response;
 
@@ -51,9 +51,6 @@ class EndSessionControllerTest extends TestCase
     protected MockObject $psrHttpBridgeMock;
     protected MockObject $errorResponderMock;
     protected Stub $uiLocalesResolverStub;
-    protected MockObject $sspBridgeMock;
-    protected MockObject $sspBridgeLocaleMock;
-    protected MockObject $sspBridgeLocaleLanguageMock;
 
     /**
      * @throws \PHPUnit\Framework\MockObject\Exception
@@ -76,25 +73,19 @@ public function setUp(): void
         $this->errorResponderMock = $this->createMock(ErrorResponder::class);
 
         $this->uiLocalesResolverStub = $this->createStub(UiLocalesResolver::class);
-        $this->sspBridgeMock = $this->createMock(SspBridge::class);
-        $this->sspBridgeLocaleMock = $this->createMock(SspBridge\Locale::class);
-        $this->sspBridgeLocaleLanguageMock = $this->createMock(SspBridge\Locale\Language::class);
-        $this->sspBridgeMock->method('locale')->willReturn($this->sspBridgeLocaleMock);
-        $this->sspBridgeLocaleMock->method('language')->willReturn($this->sspBridgeLocaleLanguageMock);
     }
 
-    protected function mock(): EndSessionController
+    protected function mock(?TemplateFactory $templateFactory = null): EndSessionController
     {
         return new EndSessionController(
             $this->authorizationServerStub,
             $this->sessionServiceStub,
             $this->sessionLogoutTicketStoreBuilderStub,
             $this->loggerServiceMock,
-            $this->templateFactoryStub,
+            $templateFactory ?? $this->templateFactoryStub,
             $this->psrHttpBridgeMock,
             $this->errorResponderMock,
             $this->uiLocalesResolverStub,
-            $this->sspBridgeMock,
         );
     }
 
@@ -232,37 +223,65 @@ public function testLogoutHandler(): never
      * @throws \SimpleSAML\Error\BadRequest
      * @throws \SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException
      */
-    public function testSetsUiLanguageBasedOnUiLocales(): void
+    public function testRendersLogoutPageInResolvedUiLanguage(): void
     {
         $this->currentSessionMock->method('getAuthorities')->willReturn([]);
         $this->sessionServiceStub->method('getCurrentSession')->willReturn($this->currentSessionMock);
         $this->logoutRequestStub->method('getUiLocales')->willReturn('hr en');
         $this->authorizationServerStub->method('validateLogoutRequest')->willReturn($this->logoutRequestStub);
         $this->uiLocalesResolverStub->method('resolve')->willReturn('hr');
 
-        $this->sspBridgeLocaleLanguageMock->expects($this->once())
-            ->method('setLanguageCookie')
-            ->with('hr');
-
-        $this->mock()->__invoke($this->serverRequestStub);
+        $this->assertSame('hr', $this->captureRenderedTemplateLanguage());
     }
 
     /**
      * @throws \Throwable
      * @throws \SimpleSAML\Error\BadRequest
      * @throws \SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException
      */
-    public function testDoesNotSetUiLanguageWhenNoRequestedLanguageIsAvailable(): void
+    public function testRendersLogoutPageWithoutLanguageWhenNoneResolved(): void
     {
         $this->currentSessionMock->method('getAuthorities')->willReturn([]);
         $this->sessionServiceStub->method('getCurrentSession')->willReturn($this->currentSessionMock);
         $this->logoutRequestStub->method('getUiLocales')->willReturn('de');
         $this->authorizationServerStub->method('validateLogoutRequest')->willReturn($this->logoutRequestStub);
         $this->uiLocalesResolverStub->method('resolve')->willReturn(null);
 
-        $this->sspBridgeLocaleLanguageMock->expects($this->never())
-            ->method('setLanguageCookie');
+        $this->assertNull($this->captureRenderedTemplateLanguage());
+    }
 
-        $this->mock()->__invoke($this->serverRequestStub);
+    /**
+     * Invoke the controller with a TemplateFactory mock and return the language passed to the rendered
+     * logout template.
+     *
+     * @throws \Throwable
+     * @throws \SimpleSAML\Error\BadRequest
+     * @throws \SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException
+     */
+    private function captureRenderedTemplateLanguage(): ?string
+    {
+        $templateStub = $this->createStub(Template::class);
+        $capturedLanguage = null;
+
+        $templateFactoryMock = $this->createMock(TemplateFactory::class);
+        $templateFactoryMock->method('build')->willReturnCallback(
+            function (
+                string $templateName,
+                array $data = [],
+                ?string $activeHrefPath = null,
+                ?bool $includeDefaultMenuItems = null,
+                ?bool $showMenu = null,
+                ?bool $showModuleName = null,
+                ?bool $showSubPageTitle = null,
+                ?string $language = null,
+            ) use (&$capturedLanguage, $templateStub): Template {
+                $capturedLanguage = $language;
+                return $templateStub;
+            },
+        );
+
+        $this->mock($templateFactoryMock)->__invoke($this->serverRequestStub);
+
+        return $capturedLanguage;
     }
 }
