Skip to content

Commit dfd52e1

Browse files
ioigoumeioigoume
authored
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
1 parent 7904539 commit dfd52e1

11 files changed

+390
-262
lines changed

‎composer.json

+2-3
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,7 @@
2323
"simplesamlphp/simplesamlphp-test-framework": "^1.7",
2424
"phpunit/phpunit": "^10",
2525
"psalm/plugin-phpunit": "^0.19.0",
26-
"squizlabs/php_codesniffer": "^3.7",
27-
"dg/bypass-finals": "^1.8"
26+
"squizlabs/php_codesniffer": "^3.7"
2827
},
2928
"autoload": {
3029
"psr-4": {
@@ -49,7 +48,7 @@
4948
},
5049
"scripts": {
5150
"validate": [
52-
"vendor/bin/phpunit --no-coverage",
51+
"vendor/bin/phpunit --no-coverage --testdox",
5352
"vendor/bin/phpcs -p",
5453
"vendor/bin/psalm --no-cache"
5554
],

‎docs/APPLE.md

+5-1
Original file line numberDiff line numberDiff line change
@@ -77,4 +77,8 @@ docker run --name ssp-apple-oidc \
7777

7878
Edit your `/etc/hosts` file to make `apple.test.idpproxy.illinois.edu` route to local host and then visit
7979
`https://apple.test.idpproxy.illinois.edu/simplesaml/module.php/core/authenticate.php?as=appleTest` to
80-
initiate a login to Apple. Non-secret values such as keyId and teamId
80+
initiate a login to Apple. Non-secret values such as keyId and teamId
81+
82+
# Documentation
83+
84+
* [TN3107: Resolving Sign in with Apple response errors](https://developer.apple.com/documentation/technotes/tn3107-resolving-sign-in-with-apple-response-errors)

‎phpunit.xml

-4
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,6 @@
88
displayDetailsOnTestsThatTriggerDeprecations="true"
99
backupGlobals="false"
1010
>
11-
<extensions>
12-
<bootstrap class="DG\BypassFinals\PHPUnitExtension"/>
13-
</extensions>
14-
1511
<testsuites>
1612
<testsuite name="The project's test suite">
1713
<directory>./tests</directory>

‎src/Controller/OIDCLogoutController.php

+10-6
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@
1212
use SimpleSAML\Logger;
1313
use SimpleSAML\Module\authoauth2\Auth\Source\OAuth2;
1414
use SimpleSAML\Module\authoauth2\Auth\Source\OpenIDConnect;
15+
use SimpleSAML\Module\authoauth2\Codebooks\LegacyRoutesEnum;
16+
use SimpleSAML\Module\authoauth2\Codebooks\RoutesEnum;
1517
use SimpleSAML\Module\authoauth2\Controller\Traits\RequestTrait;
1618
use SimpleSAML\Module\authoauth2\locators\SourceServiceLocator;
1719
use Symfony\Component\HttpFoundation\Request;
@@ -55,9 +57,8 @@ public function __construct(Configuration $config = null)
5557
*/
5658
public function loggedout(Request $request): void
5759
{
58-
Logger::debug('authoauth2: logout request=' . var_export($request->request->all(), true));
59-
6060
$this->parseRequest($request);
61+
Logger::debug('authoauth2: logout request=' . var_export($this->requestParams, true));
6162

6263
\assert(\is_array($this->state));
6364

@@ -72,19 +73,22 @@ public function loggedout(Request $request): void
7273
*/
7374
public function logout(Request $request): void
7475
{
75-
Logger::debug('authoauth2: logout request=' . var_export($request->request->all(), true));
76+
$this->parseRequestParamsSingleton($request);
77+
Logger::debug('authoauth2: logout request=' . var_export($this->requestParams, true));
7678
// Find the authentication source
77-
if (!$request->query->has('authSource')) {
79+
if (!isset($this->requestParams['authSource'])) {
7880
throw new BadRequest('No authsource in the request');
7981
}
80-
$sourceId = $request->query->get('authSource');
82+
$sourceId = $this->requestParams['authSource'];
8183
if (empty($sourceId) || !\is_string($sourceId)) {
8284
throw new BadRequest('Authsource ID invalid');
8385
}
86+
$logoutRoute = $this->config->getOptionalBoolean('useLegacyRoutes', false) ?
87+
LegacyRoutesEnum::LegacyLogout->value : RoutesEnum::Logout->value;
8488
$this->getAuthSource($sourceId)
8589
->logout([
8690
'oidc:localLogout' => true,
87-
'ReturnTo' => $this->config->getBasePath() . 'logout.php',
91+
'ReturnTo' => $this->config->getBasePath() . $logoutRoute,
8892
]);
8993
}
9094

‎src/Controller/Oauth2Controller.php

+5-4
Original file line numberDiff line numberDiff line change
@@ -51,22 +51,23 @@ public function __construct()
5151
*/
5252
public function linkback(Request $request): void
5353
{
54-
Logger::debug('authoauth2: linkback request=' . var_export($request->query->all(), true));
55-
5654
$this->parseRequest($request);
55+
Logger::debug('authoauth2: linkback request=' . var_export($this->requestParams, true));
5756

5857
// Required for psalm
5958
\assert($this->source instanceof OAuth2);
6059
\assert(\is_array($this->state));
6160
\assert(\is_string($this->sourceId));
6261

6362
// Handle Identify Provider error
64-
if (!$request->query->has('code') || empty($request->query->get('code'))) {
63+
if (empty($this->requestParams['code'])) {
6564
$this->handleError($this->source, $this->state, $request);
65+
// Used to facilitate testing
66+
return;
6667
}
6768

6869
try {
69-
$this->source->finalStep($this->state, (string)$request->query->get('code'));
70+
$this->source->finalStep($this->state, (string)$this->requestParams['code']);
7071
} catch (IdentityProviderException $e) {
7172
// phpcs:ignore Generic.Files.LineLength.TooLong
7273
Logger::error("authoauth2: error in '$this->sourceId' msg '{$e->getMessage()}' body '" . var_export($e->getResponseBody(), true) . "'");

‎src/Controller/Traits/ErrorTrait.php

+6-4
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
namespace SimpleSAML\Module\authoauth2\Controller\Traits;
66

77
use Symfony\Component\HttpFoundation\Request;
8+
use SimpleSAML\Module\authoauth2\Lib\RequestUtilities;
89

910
trait ErrorTrait
1011
{
@@ -15,16 +16,17 @@ trait ErrorTrait
1516
*/
1617
public function parseError(Request $request): array
1718
{
19+
$requestParams = RequestUtilities::getRequestParams($request);
1820
// Do not throw if errors are suppressed by @ operator
1921
// error_reporting() value for suppressed errors changed in PHP 8.0.0
2022
$error = '';
2123
$error_description = '';
22-
if ($request->query->has('error')) {
23-
$error = (string)$request->query->get('error');
24+
if (isset($requestParams['error'])) {
25+
$error = (string)$requestParams['error'];
2426
}
2527

26-
if ($request->query->has('error_description')) {
27-
$error_description = (string)$request->query->get('error_description');
28+
if (isset($requestParams['error_description'])) {
29+
$error_description = (string)$requestParams['error_description'];
2830
}
2931

3032
return [

‎src/Controller/Traits/RequestTrait.php

+20-4
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,9 @@
99
use SimpleSAML\Error\BadRequest;
1010
use SimpleSAML\Error\NoState;
1111
use SimpleSAML\Module\authoauth2\Auth\Source\OAuth2;
12-
use SimpleSAML\Module\authoauth2\Codebooks\RoutesEnum;
1312
use SimpleSAML\Module\authoauth2\Codebooks\LegacyRoutesEnum;
13+
use SimpleSAML\Module\authoauth2\Codebooks\RoutesEnum;
14+
use SimpleSAML\Module\authoauth2\Lib\RequestUtilities;
1415
use SimpleSAML\Module\authoauth2\locators\SourceServiceLocator;
1516
use Symfony\Component\HttpFoundation\Request;
1617

@@ -36,18 +37,23 @@ trait RequestTrait
3637
*/
3738
protected string $expectedStateAuthId = OAuth2::AUTHID;
3839

40+
/** @var array */
41+
protected array $requestParams = [];
42+
3943
/**
4044
* @param Request $request
4145
*
4246
* @return bool
4347
*/
4448
public function stateIsValid(Request $request): bool
4549
{
46-
if (!$request->query->has('state')) {
50+
// Parse the request parameters
51+
$this->parseRequestParamsSingleton($request);
52+
if (!isset($this->requestParams['state'])) {
4753
return false;
4854
}
4955
/** @var ?string $stateId */
50-
$stateId = $request->query->get('state');
56+
$stateId = $this->requestParams['state'];
5157
if (empty($stateId)) {
5258
return false;
5359
}
@@ -72,7 +78,7 @@ public function parseRequest(Request $request): void
7278
};
7379
throw new BadRequest($message);
7480
}
75-
$stateIdWithPrefix = (string)($request->query->get('state') ?? '');
81+
$stateIdWithPrefix = (string)($this->requestParams['state'] ?? '');
7682
$stateId = substr($stateIdWithPrefix, \strlen($this->expectedPrefix));
7783

7884
$this->state = $this->loadState($stateId, $this->expectedStageState);
@@ -110,4 +116,14 @@ public function loadState(string $id, string $stage, bool $allowMissing = false)
110116
{
111117
return State::loadState($id, $stage, $allowMissing);
112118
}
119+
120+
/**
121+
* @param Request $request
122+
*/
123+
public function parseRequestParamsSingleton(Request $request): void
124+
{
125+
if (empty($this->requestParams)) {
126+
$this->requestParams = RequestUtilities::getRequestParams($request);
127+
}
128+
}
113129
}

‎src/Lib/RequestUtilities.php

+27
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace SimpleSAML\Module\authoauth2\Lib;
6+
7+
use Symfony\Component\HttpFoundation\Request;
8+
9+
class RequestUtilities
10+
{
11+
/**
12+
* @param Request $request
13+
*
14+
* @return array
15+
*/
16+
public static function getRequestParams(Request $request): array
17+
{
18+
$params = [];
19+
if ($request->isMethod('GET')) {
20+
$params = $request->query->all();
21+
} elseif ($request->isMethod('POST')) {
22+
$params = $request->request->all();
23+
}
24+
25+
return $params;
26+
}
27+
}

0 commit comments

Comments
 (0)