Start testing with SSP 2.3 (#268)

cicnavi · web-flow · commit 4cdf7a4305f6 · 2024-12-06T12:36:54.000+01:00
* Change error code as per OIDF draft 41

* Explicitly mark nullable parameters

* Add PHP v8.4 to GH PHP version matrix

* Skip PHP v8.4 GH action check for now as psalm is not ready

* Start testing with SSP v2.3

---------

Co-authored-by: Marko Ivančić &lt;marko.ivancic@srce.hr&gt;
diff --git a/README.md b/README.md
@@ -26,7 +26,7 @@ PHP version requirement changes in minor releases for SimpleSAMLphp.
 
 | OIDC module | Tested SimpleSAMLphp |  PHP   | Note                        |
 |:------------|:---------------------|:------:|-----------------------------|
-| v6.\*       | v2.2.\*              | \>=8.2 | Recommended                 |
+| v6.\*       | v2.3.\*              | \>=8.2 | Recommended                 |
 | v5.\*       | v2.1.\*              | \>=8.1 |                             |
 | v4.\*       | v2.0.\*              | \>=8.0 |                             |
 | v3.\*       | v2.0.\*              | \>=7.4 | Abandoned from August 2023. |
@@ -329,7 +329,7 @@ docker run --name ssp-oidc-dev \
   --mount type=bind,source="$(pwd)/docker/ssp/oidc_module.crt",target=/var/simplesamlphp/cert/oidc_module.crt,readonly \
   --mount type=bind,source="$(pwd)/docker/ssp/oidc_module.key",target=/var/simplesamlphp/cert/oidc_module.key,readonly \
   --mount type=bind,source="$(pwd)/docker/apache-override.cf",target=/etc/apache2/sites-enabled/ssp-override.cf,readonly \
-   -p 443:443 cirrusid/simplesamlphp:v2.2.2
+   -p 443:443 cirrusid/simplesamlphp:v2.3.5
 ```
 
 Visit https://localhost/simplesaml/ and confirm you get the default page.
diff --git a/UPGRADE.md b/UPGRADE.md
@@ -91,7 +91,7 @@ has been refactored:
 
 - upgraded to v5 of lcobucci/jwt https://github.com/lcobucci/jwt
 - upgraded to v3 of laminas/laminas-diactoros https://github.com/laminas/laminas-diactoros
-- SimpleSAMLphp version used during development was bumped to v2.2
+- SimpleSAMLphp version used during development was bumped to v2.3
 - In Authorization Code Flow, a new validation was added which checks for 'openid' value in 'scope' parameter. Up to
 now, 'openid' value was dynamically added if not present. In Implicit Code Flow this validation was already present. 
 
diff --git a/composer.json b/composer.json
@@ -43,7 +43,7 @@
         "friendsofphp/php-cs-fixer": "^3",
         "phpunit/phpunit": "^10",
         "rector/rector": "^0.18.3",
-        "simplesamlphp/simplesamlphp": "2.2.*",
+        "simplesamlphp/simplesamlphp": "2.3.*",
         "simplesamlphp/simplesamlphp-test-framework": "^1.5",
         "squizlabs/php_codesniffer": "^3",
         "vimeo/psalm": "^5",
@@ -56,9 +56,10 @@
         },
         "sort-packages": true,
         "allow-plugins": {
-            "simplesamlphp/composer-module-installer": true,
             "dealerdirect/phpcodesniffer-composer-installer": true,
-            "phpstan/extension-installer": true
+            "phpstan/extension-installer": true,
+            "simplesamlphp/composer-module-installer": true,
+            "simplesamlphp/composer-xmlprovider-installer": true
         },
         "cache-dir": "build/composer"
     },
diff --git a/docker/Dockerfile b/docker/Dockerfile
@@ -1,5 +1,5 @@
-#FROM cirrusid/simplesamlphp:v2.2.2
-FROM cicnavi/simplesamlphp:dev
+FROM cirrusid/simplesamlphp:v2.3.5
+#FROM cicnavi/simplesamlphp:dev
 
 RUN apt-get update && apt-get install -y sqlite3
 # Prepopulate the DB with items needed for testing
diff --git a/src/Admin/Menu.php b/src/Admin/Menu.php
@@ -20,7 +20,7 @@ public function __construct(Item ...$items)
         array_push($this->items, ...$items);
     }
 
-    public function addItem(Item $menuItem, int $offset = null): void
+    public function addItem(Item $menuItem, ?int $offset = null): void
     {
         $offset ??= count($this->items);
 
diff --git a/src/Entities/AccessTokenEntity.php b/src/Entities/AccessTokenEntity.php
@@ -65,11 +65,11 @@ public function __construct(
         DateTimeImmutable $expiryDateTime,
         CryptKey $privateKey,
         protected JsonWebTokenBuilderService $jsonWebTokenBuilderService,
-        int|string $userIdentifier = null,
-        string $authCodeId = null,
-        array $requestedClaims = null,
-        bool $isRevoked = false,
-        Configuration $jwtConfiguration = null,
+        int|string|null $userIdentifier = null,
+        ?string $authCodeId = null,
+        ?array $requestedClaims = null,
+        ?bool $isRevoked = false,
+        ?Configuration $jwtConfiguration = null,
     ) {
         $this->setIdentifier($id);
         $this->setClient($clientEntity);
diff --git a/src/Entities/AuthCodeEntity.php b/src/Entities/AuthCodeEntity.php
@@ -40,9 +40,9 @@ public function __construct(
         OAuth2ClientEntityInterface $client,
         array $scopes,
         DateTimeImmutable $expiryDateTime,
-        string $userIdentifier = null,
-        string $redirectUri = null,
-        string $nonce = null,
+        ?string $userIdentifier = null,
+        ?string $redirectUri = null,
+        ?string $nonce = null,
         bool $isRevoked = false,
     ) {
         $this->identifier = $id;
diff --git a/src/Factories/Entities/AccessTokenEntityFactory.php b/src/Factories/Entities/AccessTokenEntityFactory.php
@@ -31,10 +31,10 @@ public function fromData(
         OAuth2ClientEntityInterface $clientEntity,
         array $scopes,
         DateTimeImmutable $expiryDateTime,
-        int|string $userIdentifier = null,
-        string $authCodeId = null,
-        array $requestedClaims = null,
-        bool $isRevoked = false,
+        int|string|null $userIdentifier = null,
+        ?string $authCodeId = null,
+        ?array $requestedClaims = null,
+        ?bool $isRevoked = false,
     ): AccessTokenEntity {
         return new AccessTokenEntity(
             $id,
diff --git a/src/Factories/Entities/AuthCodeEntityFactory.php b/src/Factories/Entities/AuthCodeEntityFactory.php
@@ -27,9 +27,9 @@ public function fromData(
         OAuth2ClientEntityInterface $client,
         array $scopes,
         DateTimeImmutable $expiryDateTime,
-        string $userIdentifier = null,
-        string $redirectUri = null,
-        string $nonce = null,
+        ?string $userIdentifier = null,
+        ?string $redirectUri = null,
+        ?string $nonce = null,
         bool $isRevoked = false,
     ): AuthCodeEntity {
         return new AuthCodeEntity(
diff --git a/src/Factories/Entities/ScopeEntityFactory.php b/src/Factories/Entities/ScopeEntityFactory.php
@@ -13,8 +13,8 @@ class ScopeEntityFactory
      */
     public function fromData(
         string $identifier,
-        string $description = null,
-        string $icon = null,
+        ?string $description = null,
+        ?string $icon = null,
         array $claims = [],
     ): ScopeEntity {
         return new ScopeEntity(
diff --git a/src/Factories/TemplateFactory.php b/src/Factories/TemplateFactory.php
@@ -49,7 +49,7 @@ public function __construct(
     public function build(
         string $templateName,
         array $data = [],
-        string $activeHrefPath = null,
+        ?string $activeHrefPath = null,
         ?bool $includeDefaultMenuItems = null,
         ?bool $showMenu = null,
         ?bool $showModuleName = null,
diff --git a/src/ModuleConfig.php b/src/ModuleConfig.php
@@ -118,7 +118,7 @@ class ModuleConfig
     public function __construct(
         string $fileName = self::DEFAULT_FILE_NAME, // Primarily used for easy (unit) testing overrides.
         array $overrides = [], // Primarily used for easy (unit) testing overrides.
-        Configuration $sspConfig = null,
+        ?Configuration $sspConfig = null,
         private readonly SspBridge $sspBridge = new SspBridge(),
     ) {
         $this->moduleConfig = Configuration::loadFromArray(
@@ -225,7 +225,7 @@ public function config(): Configuration
     }
 
     // TODO mivanci Move to dedicated \SimpleSAML\Module\oidc\Utils\Routes::getModuleUrl
-    public function getModuleUrl(string $path = null): string
+    public function getModuleUrl(?string $path = null): string
     {
         $base = $this->sspBridge->module()->getModuleURL(self::MODULE_NAME);
 
diff --git a/src/Repositories/AccessTokenRepository.php b/src/Repositories/AccessTokenRepository.php
@@ -63,10 +63,10 @@ public function getNewToken(
         OAuth2ClientEntityInterface $clientEntity,
         array $scopes,
         $userIdentifier = null,
-        string $authCodeId = null,
-        array $requestedClaims = null,
-        string $id = null,
-        DateTimeImmutable $expiryDateTime = null,
+        ?string $authCodeId = null,
+        ?array $requestedClaims = null,
+        ?string $id = null,
+        ?DateTimeImmutable $expiryDateTime = null,
     ): AccessTokenEntityInterface {
         if (!is_null($userIdentifier)) {
             $userIdentifier = (string)$userIdentifier;
diff --git a/src/Repositories/Interfaces/AccessTokenRepositoryInterface.php b/src/Repositories/Interfaces/AccessTokenRepositoryInterface.php
@@ -29,7 +29,7 @@ public function getNewToken(
         OAuth2ClientEntityInterface $clientEntity,
         array $scopes,
         $userIdentifier = null,
-        string $authCodeId = null,
-        array $requestedClaims = null,
+        ?string $authCodeId = null,
+        ?array $requestedClaims = null,
     ): AccessTokenEntityInterface;
 }
diff --git a/src/Server/AuthorizationServer.php b/src/Server/AuthorizationServer.php
@@ -49,8 +49,8 @@ public function __construct(
         ScopeRepositoryInterface $scopeRepository,
         CryptKey|string $privateKey,
         Key|string $encryptionKey,
-        ResponseTypeInterface $responseType = null,
-        RequestRulesManager $requestRulesManager = null,
+        ?ResponseTypeInterface $responseType = null,
+        ?RequestRulesManager $requestRulesManager = null,
     ) {
         parent::__construct(
             $clientRepository,
diff --git a/src/Server/Exceptions/OidcServerException.php b/src/Server/Exceptions/OidcServerException.php
@@ -6,6 +6,7 @@
 
 use League\OAuth2\Server\Exception\OAuthServerException;
 use Psr\Http\Message\ResponseInterface;
+use SimpleSAML\OpenID\Codebooks\ErrorsEnum;
 use Throwable;
 
 use function http_build_query;
@@ -57,10 +58,10 @@ public function __construct(
         int $code,
         string $errorType,
         int $httpStatusCode = 400,
-        string $hint = null,
-        string $redirectUri = null,
-        Throwable $previous = null,
-        string $state = null,
+        ?string $hint = null,
+        ?string $redirectUri = null,
+        ?Throwable $previous = null,
+        ?string $state = null,
     ) {
         parent::__construct($message, $code, $errorType, $httpStatusCode, $hint, $redirectUri, $previous);
 
@@ -93,8 +94,8 @@ public function __construct(
      * @return self
      */
     public static function unsupportedResponseType(
-        string $redirectUri = null,
-        string $state = null,
+        ?string $redirectUri = null,
+        ?string $state = null,
         bool $useFragment = false,
     ): OidcServerException {
         $errorMessage = 'The response type is not supported by the authorization server.';
@@ -117,7 +118,7 @@ public static function unsupportedResponseType(
     public static function invalidScope(
         $scope,
         $redirectUri = null,
-        string $state = null,
+        ?string $state = null,
         bool $useFragment = false,
     ): OidcServerException {
         // OAuthServerException correctly implements this error, however, it misses state parameter.
@@ -142,9 +143,9 @@ public static function invalidScope(
     public static function invalidRequest(
         $parameter,
         $hint = null,
-        Throwable $previous = null,
-        string $redirectUri = null,
-        string $state = null,
+        ?Throwable $previous = null,
+        ?string $redirectUri = null,
+        ?string $state = null,
         bool $useFragment = false,
     ): OidcServerException {
         $e = parent::invalidRequest($parameter, $hint, $previous);
@@ -167,8 +168,8 @@ public static function invalidRequest(
     public static function accessDenied(
         $hint = null,
         $redirectUri = null,
-        Throwable $previous = null,
-        string $state = null,
+        ?Throwable $previous = null,
+        ?string $state = null,
         bool $useFragment = false,
     ): OidcServerException {
         $e = parent::accessDenied($hint, $redirectUri, $previous);
@@ -190,10 +191,10 @@ public static function accessDenied(
      * @return self
      */
     public static function loginRequired(
-        string $hint = null,
-        string $redirectUri = null,
-        Throwable $previous = null,
-        string $state = null,
+        ?string $hint = null,
+        ?string $redirectUri = null,
+        ?Throwable $previous = null,
+        ?string $state = null,
         bool $useFragment = false,
     ): OidcServerException {
         $errorMessage = "End-User is not already authenticated.";
@@ -216,10 +217,10 @@ public static function loginRequired(
      * @return self
      */
     public static function requestNotSupported(
-        string $hint = null,
-        string $redirectUri = null,
-        Throwable $previous = null,
-        string $state = null,
+        ?string $hint = null,
+        ?string $redirectUri = null,
+        ?Throwable $previous = null,
+        ?string $state = null,
         bool $useFragment = false,
     ): OidcServerException {
         $errorMessage = "Request object not supported.";
@@ -239,21 +240,30 @@ public static function requestNotSupported(
      * @return self
      * @psalm-suppress LessSpecificImplementedReturnType
      */
-    public static function invalidRefreshToken($hint = null, Throwable $previous = null): OidcServerException
+    public static function invalidRefreshToken($hint = null, ?Throwable $previous = null): OidcServerException
     {
         return new self('The refresh token is invalid.', 8, 'invalid_grant', 400, $hint, null, $previous);
     }
 
     public static function invalidTrustChain(
-        string $hint = null,
-        string $redirectUri = null,
-        Throwable $previous = null,
-        string $state = null,
+        ?string $hint = null,
+        ?string $redirectUri = null,
+        ?Throwable $previous = null,
+        ?string $state = null,
         bool $useFragment = false,
     ): OidcServerException {
         $errorMessage = 'Trust chain validation failed.';
 
-        $e = new self($errorMessage, 12, 'trust_chain_validation_failed', 400, $hint, $redirectUri, $previous, $state);
+        $e = new self(
+            $errorMessage,
+            12,
+            ErrorsEnum::InvalidTrustChain->value,
+            400,
+            $hint,
+            $redirectUri,
+            $previous,
+            $state,
+        );
         $e->useFragmentInHttpResponses($useFragment);
 
         return $e;
@@ -268,7 +278,7 @@ public static function invalidTrustChain(
      * @return self
      * @psalm-suppress LessSpecificImplementedReturnType
      */
-    public static function forbidden(string $hint = null, Throwable $previous = null): OidcServerException
+    public static function forbidden(?string $hint = null, ?Throwable $previous = null): OidcServerException
     {
         return new self(
             'Request understood, but refused to process it.',
@@ -304,7 +314,7 @@ public function setPayload(array $payload): void
     /**
      * @param string|null $redirectUri Set to string, or unset it with null
      */
-    public function setRedirectUri(string $redirectUri = null): void
+    public function setRedirectUri(?string $redirectUri = null): void
     {
         $this->redirectUri = $redirectUri;
     }
@@ -337,7 +347,7 @@ public function getRedirectUri(): ?string
     /**
      * @param string|null $state Set to string, or unset it with null
      */
-    public function setState(string $state = null): void
+    public function setState(?string $state = null): void
     {
         if ($state === null) {
             unset($this->payload['state']);
diff --git a/src/Server/Grants/AuthCodeGrant.php b/src/Server/Grants/AuthCodeGrant.php
@@ -314,7 +314,7 @@ protected function issueOidcAuthCode(
         string $userIdentifier,
         string $redirectUri,
         array $scopes = [],
-        string $nonce = null,
+        ?string $nonce = null,
     ): AuthCodeEntityInterface {
         $maxGenerationAttempts = self::MAX_RANDOM_TOKEN_GENERATION_ATTEMPTS;
 
@@ -748,7 +748,7 @@ public function validateAuthorizationRequestWithCheckerResultBag(
      */
     protected function issueRefreshToken(
         OAuth2AccessTokenEntityInterface $accessToken,
-        string $authCodeId = null,
+        ?string $authCodeId = null,
     ): ?RefreshTokenEntityInterface {
         if (! is_a($accessToken, AccessTokenEntityInterface::class)) {
             throw OidcServerException::serverError('Unexpected access token entity type.');
diff --git a/src/Server/Grants/Traits/IssueAccessTokenTrait.php b/src/Server/Grants/Traits/IssueAccessTokenTrait.php
@@ -48,8 +48,8 @@ protected function issueAccessToken(
         ClientEntityInterface $client,
         $userIdentifier = null,
         array $scopes = [],
-        string $authCodeId = null,
-        array $requestedClaims = null,
+        ?string $authCodeId = null,
+        ?array $requestedClaims = null,
     ): AccessTokenEntityInterface {
         $maxGenerationAttempts = AbstractGrant::MAX_RANDOM_TOKEN_GENERATION_ATTEMPTS;
 
diff --git a/src/Server/LogoutHandlers/BackChannelLogoutHandler.php b/src/Server/LogoutHandlers/BackChannelLogoutHandler.php
@@ -29,7 +29,7 @@ public function __construct(
      * @param \GuzzleHttp\HandlerStack|null $handlerStack For easier testing
      * @throws \League\OAuth2\Server\Exception\OAuthServerException
      */
-    public function handle(array $relyingPartyAssociations, HandlerStack $handlerStack = null): void
+    public function handle(array $relyingPartyAssociations, ?HandlerStack $handlerStack = null): void
     {
         $clientConfig = ['timeout' => 3, 'verify' => false, 'handler' => $handlerStack];
 
diff --git a/src/Server/TokenIssuers/RefreshTokenIssuer.php b/src/Server/TokenIssuers/RefreshTokenIssuer.php
diff --git a/src/Server/Validators/BearerTokenValidator.php b/src/Server/Validators/BearerTokenValidator.php
diff --git a/src/Services/DatabaseMigration.php b/src/Services/DatabaseMigration.php
diff --git a/tests/unit/src/Repositories/UserRepositoryTest.php b/tests/unit/src/Repositories/UserRepositoryTest.php
diff --git a/tests/unit/src/Utils/RequestParamsResolverTest.php b/tests/unit/src/Utils/RequestParamsResolverTest.php

Original file line number	Diff line number	Diff line change
`@@ -20,7 +20,7 @@ public function __construct(Item ...$items)`
`20`	`20`	`array_push($this->items, ...$items);`
`21`	`21`	`}`
`22`	`22`
`23`		`- public function addItem(Item $menuItem, int $offset = null): void`
	`23`	`+ public function addItem(Item $menuItem, ?int $offset = null): void`
`24`	`24`	`{`
`25`	`25`	`$offset ??= count($this->items);`
`26`	`26`