Fix request parameters for actions. Refactored oidc and oauth2 controller tests.

Author: ioigoume

composer.json

@@ -23,8 +23,7 @@
     "simplesamlphp/simplesamlphp-test-framework": "^1.7",
     "phpunit/phpunit": "^10",
     "psalm/plugin-phpunit": "^0.19.0",
-    "squizlabs/php_codesniffer": "^3.7",
-    "dg/bypass-finals": "^1.8"
+    "squizlabs/php_codesniffer": "^3.7"
   },
   "autoload": {
     "psr-4": {
@@ -49,7 +48,7 @@
   },
   "scripts": {
     "validate": [
-      "vendor/bin/phpunit --no-coverage",
+      "vendor/bin/phpunit --no-coverage --testdox",
       "vendor/bin/phpcs -p",
       "vendor/bin/psalm --no-cache"
     ],

phpunit.xml

@@ -8,10 +8,6 @@
          displayDetailsOnTestsThatTriggerDeprecations="true"
          backupGlobals="false"
 >
-    <extensions>
-        <bootstrap class="DG\BypassFinals\PHPUnitExtension"/>
-    </extensions>
-
     <testsuites>
         <testsuite name="The project's test suite">
             <directory>./tests</directory>

src/Controller/OIDCLogoutController.php

@@ -12,6 +12,8 @@
 use SimpleSAML\Logger;
 use SimpleSAML\Module\authoauth2\Auth\Source\OAuth2;
 use SimpleSAML\Module\authoauth2\Auth\Source\OpenIDConnect;
+use SimpleSAML\Module\authoauth2\Codebooks\LegacyRoutesEnum;
+use SimpleSAML\Module\authoauth2\Codebooks\RoutesEnum;
 use SimpleSAML\Module\authoauth2\Controller\Traits\RequestTrait;
 use SimpleSAML\Module\authoauth2\locators\SourceServiceLocator;
 use Symfony\Component\HttpFoundation\Request;
@@ -55,9 +57,8 @@ public function __construct(Configuration $config = null)
      */
     public function loggedout(Request $request): void
     {
-        Logger::debug('authoauth2: logout request=' . var_export($request->request->all(), true));
-
         $this->parseRequest($request);
+        Logger::debug('authoauth2: logout request=' . var_export($this->requestParams, true));
 
         \assert(\is_array($this->state));
 
@@ -72,19 +73,22 @@ public function loggedout(Request $request): void
      */
     public function logout(Request $request): void
     {
-        Logger::debug('authoauth2: logout request=' . var_export($request->request->all(), true));
+        $this->parseRequestParamsSingleton($request);
+        Logger::debug('authoauth2: logout request=' . var_export($this->requestParams, true));
         // Find the authentication source
-        if (!$request->query->has('authSource')) {
+        if (!isset($this->requestParams['authSource'])) {
             throw new BadRequest('No authsource in the request');
         }
-        $sourceId = $request->query->get('authSource');
+        $sourceId = $this->requestParams['authSource'];
         if (empty($sourceId) || !\is_string($sourceId)) {
             throw new BadRequest('Authsource ID invalid');
         }
+        $logoutRoute = $this->config->getOptionalBoolean('useLegacyRoutes', false) ?
+            LegacyRoutesEnum::LegacyLogout->value : RoutesEnum::Logout->value;
         $this->getAuthSource($sourceId)
             ->logout([
                          'oidc:localLogout' => true,
-                         'ReturnTo' => $this->config->getBasePath() . 'logout.php',
+                         'ReturnTo' => $this->config->getBasePath() . $logoutRoute,
                      ]);
     }
 

src/Controller/Oauth2Controller.php

@@ -51,22 +51,23 @@ public function __construct()
      */
     public function linkback(Request $request): void
     {
-        Logger::debug('authoauth2: linkback request=' . var_export($request->query->all(), true));
-
         $this->parseRequest($request);
+        Logger::debug('authoauth2: linkback request=' . var_export($this->requestParams, true));
 
         // Required for psalm
         \assert($this->source instanceof  OAuth2);
         \assert(\is_array($this->state));
         \assert(\is_string($this->sourceId));
 
         // Handle Identify Provider error
-        if (!$request->query->has('code') || empty($request->query->get('code'))) {
+        if (empty($this->requestParams['code'])) {
             $this->handleError($this->source, $this->state, $request);
+            // Used to facilitate testing
+            return;
         }
 
         try {
-            $this->source->finalStep($this->state, (string)$request->query->get('code'));
+            $this->source->finalStep($this->state, (string)$this->requestParams['code']);
         } catch (IdentityProviderException $e) {
             // phpcs:ignore Generic.Files.LineLength.TooLong
             Logger::error("authoauth2: error in '$this->sourceId' msg '{$e->getMessage()}' body '" . var_export($e->getResponseBody(), true) . "'");

src/Controller/Traits/RequestTrait.php

@@ -37,8 +37,8 @@ trait RequestTrait
      */
     protected string $expectedStateAuthId = OAuth2::AUTHID;
 
-    /** @var array|null  */
-    private ?array $requestParams;
+    /** @var array  */
+    protected array $requestParams = [];
 
     /**
      * @param   Request  $request
@@ -47,12 +47,13 @@ trait RequestTrait
      */
     public function stateIsValid(Request $request): bool
     {
-        $requestParams = $this->parseRequestParamsSingleton($request);
-        if (!isset($requestParams['state'])) {
+        // Parse the request parameters
+        $this->parseRequestParamsSingleton($request);
+        if (!isset($this->requestParams['state'])) {
             return false;
         }
         /** @var ?string $stateId */
-        $stateId = $requestParams['state'];
+        $stateId = $this->requestParams['state'];
         if (empty($stateId)) {
             return false;
         }
@@ -77,8 +78,7 @@ public function parseRequest(Request $request): void
             };
             throw new BadRequest($message);
         }
-        $requestParams = $this->parseRequestParamsSingleton($request);
-        $stateIdWithPrefix = (string)($requestParams['state'] ?? '');
+        $stateIdWithPrefix = (string)($this->requestParams['state'] ?? '');
         $stateId = substr($stateIdWithPrefix, \strlen($this->expectedPrefix));
 
         $this->state = $this->loadState($stateId, $this->expectedStageState);
@@ -119,15 +119,11 @@ public function loadState(string $id, string $stage, bool $allowMissing = false)
 
     /**
      * @param   Request  $request
-     *
-     * @return array
      */
-    public function parseRequestParamsSingleton(Request $request): array
+    public function parseRequestParamsSingleton(Request $request): void
     {
-        if (!empty($this->requestParams)) {
-            return $this->requestParams;
+        if (empty($this->requestParams)) {
+            $this->requestParams = RequestUtilities::getRequestParams($request);
         }
-
-        return RequestUtilities::getRequestParams($request);
     }
 }

src/Lib/RequestUtilities.php

@@ -4,32 +4,10 @@
 
 namespace SimpleSAML\Module\authoauth2\Lib;
 
-use Generator;
 use Symfony\Component\HttpFoundation\Request;
 
 class RequestUtilities
 {
-    /**
-     * @param   Request  $request
-     *
-     * @return Generator
-     */
-    public static function parseRequestGenerator(Request $request): Generator
-    {
-        if ($request->isMethod('GET')) {
-            foreach ($request->query->all() as $key => $value) {
-                yield $key => $value;
-            }
-        } elseif ($request->isMethod('POST')) {
-            foreach ($request->request->all() as $key => $value) {
-                yield $key => $value;
-            }
-        } else {
-            // If it is neither a GET or a POST then yield nothing
-            yield;
-        }
-    }
-
     /**
      * @param   Request  $request
      *