Abstract post body and get parameters parsing (#102)

* Abstract post body and get parameters parsing * Add tests for post+get parameters request handling * Fix request parameters for actions. Refactored oidc and oauth2 controller tests. * run request tests both for POST and GET method
cirrusidentity · Oct 30, 2024 · dfd52e1 · dfd52e1
1 parent 7904539
commit dfd52e1
Show file tree

Hide file tree

Showing 11 changed files with 390 additions and 262 deletions.
diff --git a/composer.json b/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"
     ],

diff --git a/docs/APPLE.md b/docs/APPLE.md
@@ -77,4 +77,8 @@ docker run --name ssp-apple-oidc \
 
 Edit your `/etc/hosts` file to make `apple.test.idpproxy.illinois.edu` route to local host and then visit
  `https://apple.test.idpproxy.illinois.edu/simplesaml/module.php/core/authenticate.php?as=appleTest` to
-initiate a login to Apple. Non-secret values such as keyId and teamId
+initiate a login to Apple. Non-secret values such as keyId and teamId
+
+# Documentation
+
+* [TN3107: Resolving Sign in with Apple response errors](https://developer.apple.com/documentation/technotes/tn3107-resolving-sign-in-with-apple-response-errors)
diff --git a/phpunit.xml b/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>

diff --git a/src/Controller/OIDCLogoutController.php b/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,
                      ]);
     }
 

diff --git a/src/Controller/Oauth2Controller.php b/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) . "'");

diff --git a/src/Controller/Traits/ErrorTrait.php b/src/Controller/Traits/ErrorTrait.php
@@ -5,6 +5,7 @@
 namespace SimpleSAML\Module\authoauth2\Controller\Traits;
 
 use Symfony\Component\HttpFoundation\Request;
+use SimpleSAML\Module\authoauth2\Lib\RequestUtilities;
 
 trait ErrorTrait
 {
@@ -15,16 +16,17 @@ trait ErrorTrait
      */
     public function parseError(Request $request): array
     {
+        $requestParams = RequestUtilities::getRequestParams($request);
         // Do not throw if errors are suppressed by @ operator
         // error_reporting() value for suppressed errors changed in PHP 8.0.0
         $error = '';
         $error_description = '';
-        if ($request->query->has('error')) {
-            $error = (string)$request->query->get('error');
+        if (isset($requestParams['error'])) {
+            $error = (string)$requestParams['error'];
         }
 
-        if ($request->query->has('error_description')) {
-            $error_description = (string)$request->query->get('error_description');
+        if (isset($requestParams['error_description'])) {
+            $error_description = (string)$requestParams['error_description'];
         }
 
         return [

diff --git a/src/Controller/Traits/RequestTrait.php b/src/Controller/Traits/RequestTrait.php
@@ -9,8 +9,9 @@
 use SimpleSAML\Error\BadRequest;
 use SimpleSAML\Error\NoState;
 use SimpleSAML\Module\authoauth2\Auth\Source\OAuth2;
-use SimpleSAML\Module\authoauth2\Codebooks\RoutesEnum;
 use SimpleSAML\Module\authoauth2\Codebooks\LegacyRoutesEnum;
+use SimpleSAML\Module\authoauth2\Codebooks\RoutesEnum;
+use SimpleSAML\Module\authoauth2\Lib\RequestUtilities;
 use SimpleSAML\Module\authoauth2\locators\SourceServiceLocator;
 use Symfony\Component\HttpFoundation\Request;
 
@@ -36,18 +37,23 @@ trait RequestTrait
      */
     protected string $expectedStateAuthId = OAuth2::AUTHID;
 
+    /** @var array  */
+    protected array $requestParams = [];
+
     /**
      * @param   Request  $request
      *
      * @return bool
      */
     public function stateIsValid(Request $request): bool
     {
-        if (!$request->query->has('state')) {
+        // Parse the request parameters
+        $this->parseRequestParamsSingleton($request);
+        if (!isset($this->requestParams['state'])) {
             return false;
         }
         /** @var ?string $stateId */
-        $stateId = $request->query->get('state');
+        $stateId = $this->requestParams['state'];
         if (empty($stateId)) {
             return false;
         }
@@ -72,7 +78,7 @@ public function parseRequest(Request $request): void
             };
             throw new BadRequest($message);
         }
-        $stateIdWithPrefix = (string)($request->query->get('state') ?? '');
+        $stateIdWithPrefix = (string)($this->requestParams['state'] ?? '');
         $stateId = substr($stateIdWithPrefix, \strlen($this->expectedPrefix));
 
         $this->state = $this->loadState($stateId, $this->expectedStageState);
@@ -110,4 +116,14 @@ public function loadState(string $id, string $stage, bool $allowMissing = false)
     {
         return State::loadState($id, $stage, $allowMissing);
     }
+
+    /**
+     * @param   Request  $request
+     */
+    public function parseRequestParamsSingleton(Request $request): void
+    {
+        if (empty($this->requestParams)) {
+            $this->requestParams = RequestUtilities::getRequestParams($request);
+        }
+    }
 }
diff --git a/src/Lib/RequestUtilities.php b/src/Lib/RequestUtilities.php
@@ -0,0 +1,27 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SimpleSAML\Module\authoauth2\Lib;
+
+use Symfony\Component\HttpFoundation\Request;
+
+class RequestUtilities
+{
+    /**
+     * @param   Request  $request
+     *
+     * @return array
+     */
+    public static function getRequestParams(Request $request): array
+    {
+        $params = [];
+        if ($request->isMethod('GET')) {
+            $params = $request->query->all();
+        } elseif ($request->isMethod('POST')) {
+            $params = $request->request->all();
+        }
+
+        return $params;
+    }
+}