From 21b84cd77cdfb4ab8eb69b921dd57d755b5036c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Thu, 5 Dec 2024 15:39:40 +0100 Subject: [PATCH 1/7] Change error code as per OIDF draft 41 --- src/Server/Exceptions/OidcServerException.php | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/Server/Exceptions/OidcServerException.php b/src/Server/Exceptions/OidcServerException.php index 695b5e69..adcce46c 100644 --- 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; @@ -253,7 +254,16 @@ public static function invalidTrustChain( ): 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; From 74d2f56b444a0d0a18ba2cd8b478a3a62d2537c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Thu, 5 Dec 2024 16:15:36 +0100 Subject: [PATCH 2/7] Explicitly mark nullable parameters --- src/Admin/Menu.php | 2 +- src/Entities/AccessTokenEntity.php | 10 ++-- src/Entities/AuthCodeEntity.php | 6 +- .../Entities/AccessTokenEntityFactory.php | 8 +-- .../Entities/AuthCodeEntityFactory.php | 6 +- src/Factories/Entities/ScopeEntityFactory.php | 4 +- src/Factories/TemplateFactory.php | 2 +- src/ModuleConfig.php | 4 +- src/Repositories/AccessTokenRepository.php | 8 +-- .../AccessTokenRepositoryInterface.php | 4 +- src/Server/AuthorizationServer.php | 4 +- src/Server/Exceptions/OidcServerException.php | 56 +++++++++---------- src/Server/Grants/AuthCodeGrant.php | 4 +- .../Grants/Traits/IssueAccessTokenTrait.php | 4 +- .../BackChannelLogoutHandler.php | 2 +- .../TokenIssuers/RefreshTokenIssuer.php | 2 +- .../Validators/BearerTokenValidator.php | 2 +- src/Services/DatabaseMigration.php | 2 +- .../src/Repositories/UserRepositoryTest.php | 10 ++-- .../src/Utils/RequestParamsResolverTest.php | 6 +- 20 files changed, 73 insertions(+), 73 deletions(-) diff --git a/src/Admin/Menu.php b/src/Admin/Menu.php index 0c5e15a6..0ccbb8ae 100644 --- 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 index 5873044e..67630acd 100644 --- 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 index c96488c7..c0bf7c0a 100644 --- 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 index f656fa12..f4672e98 100644 --- 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 index 30d65939..be0cdee2 100644 --- 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 index 36e4da7f..b12ef45a 100644 --- 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 index de0d223c..a3039779 100644 --- 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 index 6196ddb2..973d1f16 100644 --- 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 index 3e1ac577..4298211e 100644 --- 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 index dae29026..18453a20 100644 --- 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 index 70d946e1..4c444e70 100644 --- 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 index adcce46c..5a9be60d 100644 --- a/src/Server/Exceptions/OidcServerException.php +++ b/src/Server/Exceptions/OidcServerException.php @@ -58,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); @@ -94,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.'; @@ -118,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. @@ -143,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); @@ -168,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); @@ -191,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."; @@ -217,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."; @@ -240,16 +240,16 @@ 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.'; @@ -278,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.', @@ -314,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; } @@ -347,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 index aec720b9..5d73bcaf 100644 --- 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 index 742c756b..6660ec92 100644 --- 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 index a0987572..e1fb8478 100644 --- 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 index 8aad35d2..f136dded 100644 --- a/src/Server/TokenIssuers/RefreshTokenIssuer.php +++ b/src/Server/TokenIssuers/RefreshTokenIssuer.php @@ -35,7 +35,7 @@ public function __construct( public function issue( Oauth2TokenEntityInterface $accessToken, DateInterval $refreshTokenTtl, - string $authCodeId = null, + ?string $authCodeId = null, int $maxGenerationAttempts = self::MAX_RANDOM_TOKEN_GENERATION_ATTEMPTS, ): ?RefreshTokenEntityInterface { if (! is_a($accessToken, AccessTokenEntityInterface::class)) { diff --git a/src/Server/Validators/BearerTokenValidator.php b/src/Server/Validators/BearerTokenValidator.php index c6a80572..94c7b183 100644 --- a/src/Server/Validators/BearerTokenValidator.php +++ b/src/Server/Validators/BearerTokenValidator.php @@ -44,7 +44,7 @@ class BearerTokenValidator extends OAuth2BearerTokenValidator public function __construct( AccessTokenRepositoryInterface $accessTokenRepository, CryptKey $publicKey, - DateInterval $jwtValidAtDateLeeway = null, + ?DateInterval $jwtValidAtDateLeeway = null, protected LoggerService $loggerService = new LoggerService(), ) { parent::__construct($accessTokenRepository, $jwtValidAtDateLeeway); diff --git a/src/Services/DatabaseMigration.php b/src/Services/DatabaseMigration.php index 8f4f3a74..a4936e88 100644 --- a/src/Services/DatabaseMigration.php +++ b/src/Services/DatabaseMigration.php @@ -30,7 +30,7 @@ class DatabaseMigration { private readonly Database $database; - public function __construct(Database $database = null) + public function __construct(?Database $database = null) { $this->database = $database ?? Database::getInstance(); } diff --git a/tests/unit/src/Repositories/UserRepositoryTest.php b/tests/unit/src/Repositories/UserRepositoryTest.php index fc2e7270..ec2189b0 100644 --- a/tests/unit/src/Repositories/UserRepositoryTest.php +++ b/tests/unit/src/Repositories/UserRepositoryTest.php @@ -79,11 +79,11 @@ protected function setUp(): void } protected function mock( - ModuleConfig|MockObject $moduleConfig = null, - Database|MockObject $database = null, - ProtocolCache|MockObject $protocolCache = null, - Helpers|MockObject $helpers = null, - UserEntityFactory|MockObject $userEntityFactory = null, + ?ModuleConfig $moduleConfig = null, + ?Database $database = null, + ?ProtocolCache $protocolCache = null, + ?Helpers $helpers = null, + ?UserEntityFactory $userEntityFactory = null, ): UserRepository { $moduleConfig ??= $this->moduleConfigMock; $database ??= $this->database; // Let's use real database instance for tests by default. diff --git a/tests/unit/src/Utils/RequestParamsResolverTest.php b/tests/unit/src/Utils/RequestParamsResolverTest.php index da084d8e..0cbc269a 100644 --- a/tests/unit/src/Utils/RequestParamsResolverTest.php +++ b/tests/unit/src/Utils/RequestParamsResolverTest.php @@ -57,9 +57,9 @@ protected function setUp(): void } protected function mock( - MockObject $helpersMock = null, - MockObject $coreMock = null, - MockObject $federationMock = null, + ?MockObject $helpersMock = null, + ?MockObject $coreMock = null, + ?MockObject $federationMock = null, ): RequestParamsResolver { $helpersMock ??= $this->helpersMock; $coreMock ??= $this->coreMock; From e9dd29e14bd32579d755664cbd95a49fcfcf8cfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Thu, 5 Dec 2024 16:17:15 +0100 Subject: [PATCH 3/7] Add PHP v8.4 to GH PHP version matrix --- .github/workflows/test.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index c8f89f8c..b8ac67af 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -13,7 +13,7 @@ jobs: strategy: fail-fast: false matrix: - php-versions: ["8.2", "8.3"] + php-versions: ["8.2", "8.3", "8.4"] steps: - name: Setup PHP, with composer and extensions From 143c7dc0f53319f9f64b5728039159d54d1c5ded Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Thu, 5 Dec 2024 16:31:13 +0100 Subject: [PATCH 4/7] Skip PHP v8.4 GH action check for now as psalm is not ready --- .github/workflows/test.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index b8ac67af..c8f89f8c 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -13,7 +13,7 @@ jobs: strategy: fail-fast: false matrix: - php-versions: ["8.2", "8.3", "8.4"] + php-versions: ["8.2", "8.3"] steps: - name: Setup PHP, with composer and extensions From 20c6cbf40a719435418f8b9d9ddbb1e0084c8269 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Thu, 5 Dec 2024 16:42:28 +0100 Subject: [PATCH 5/7] Start testing with SSP v2.3 --- README.md | 4 ++-- UPGRADE.md | 2 +- composer.json | 7 ++++--- docker/Dockerfile | 4 ++-- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 797bdfea..6b3c0e94 100644 --- 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 index a6f42115..9a766943 100644 --- 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 index 8c09654f..69de0580 100644 --- 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 index c8a12a77..46543010 100644 --- 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 From 6edc11529198f19acc899c83e4c00264d6dddd6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Sat, 14 Dec 2024 18:07:56 +0100 Subject: [PATCH 6/7] Enable testing trust chain resolution in admin UI --- config-templates/module_oidc.php | 27 +++ routing/routes/routes.php | 7 + routing/services/services.yml | 2 + src/Codebooks/LimitsEnum.php | 11 ++ src/Codebooks/RoutesEnum.php | 4 + src/Controllers/Admin/TestController.php | 111 ++++++++++++ src/Controllers/Federation/Test.php | 16 +- src/Factories/RequestRulesManagerFactory.php | 3 + src/Factories/TemplateFactory.php | 11 +- src/Forms/ClientForm.php | 1 + src/Helpers/Arr.php | 21 +++ src/Helpers/Str.php | 12 ++ src/ModuleConfig.php | 33 +++- .../RequestRules/Rules/ClientIdRule.php | 15 +- src/Services/Container.php | 7 + src/Utils/Debug/ArrayLogger.php | 160 ++++++++++++++++++ .../FederationParticipationValidator.php | 38 +++++ src/Utils/Routes.php | 7 + templates/tests/trust-chain-resolution.twig | 91 ++++++++++ tests/unit/src/Helpers/ArrTest.php | 49 ++++++ .../RequestRules/Rules/ClientIdRuleTest.php | 14 +- .../unit/src/Utils/Debug/ArrayLoggerTest.php | 89 ++++++++++ 22 files changed, 711 insertions(+), 18 deletions(-) create mode 100644 src/Codebooks/LimitsEnum.php create mode 100644 src/Controllers/Admin/TestController.php create mode 100644 src/Utils/Debug/ArrayLogger.php create mode 100644 src/Utils/FederationParticipationValidator.php create mode 100644 templates/tests/trust-chain-resolution.twig create mode 100644 tests/unit/src/Helpers/ArrTest.php create mode 100644 tests/unit/src/Utils/Debug/ArrayLoggerTest.php diff --git a/config-templates/module_oidc.php b/config-templates/module_oidc.php index ad985a17..30064f74 100644 --- a/config-templates/module_oidc.php +++ b/config-templates/module_oidc.php @@ -368,6 +368,33 @@ // 'eyJ...GHg', ], + // (optional) Federation participation limit by Trust Marks. This is an array with the following format: + // [ + // 'trust-anchor-id' => [ + // 'limit-id' => [ + // 'trust-mark-id', + // 'trust-mark-id-2', + // ], + // ], + // ], + // Check example below on how this can be used. If federation participation limit is configured for particular + // Trust Anchor ID, at least one combination of "limit ID" => "trust mark list" should be defined. + ModuleConfig::OPTION_FEDERATION_PARTICIPATION_LIMIT_BY_TRUST_MARKS => [ + // We are limiting federation participation using Trust Marks for 'https://ta.example.org/'. + 'https://ta.example.org/' => [ + // Entities must have (at least) one Trust Mark from the list below. + \SimpleSAML\Module\oidc\Codebooks\LimitsEnum::OneOf->value => [ + 'trust-mark-id', + 'trust-mark-id-2', + ], + // Entities must have all Trust Marks from the list below. + \SimpleSAML\Module\oidc\Codebooks\LimitsEnum::AllOf->value => [ + 'trust-mark-id-3', + 'trust-mark-id-4', + ], + ], + ], + // (optional) Dedicated federation cache adapter, used to cache federation artifacts like trust chains, entity // statements, etc. It will also be used for token reuse check in federation context. Setting this option is // recommended in production environments. If set to null, no caching will be used. Can be set to any diff --git a/routing/routes/routes.php b/routing/routes/routes.php index 12f96743..6d52f78a 100644 --- a/routing/routes/routes.php +++ b/routing/routes/routes.php @@ -10,6 +10,7 @@ use SimpleSAML\Module\oidc\Controllers\AccessTokenController; use SimpleSAML\Module\oidc\Controllers\Admin\ClientController; use SimpleSAML\Module\oidc\Controllers\Admin\ConfigController; +use SimpleSAML\Module\oidc\Controllers\Admin\TestController; use SimpleSAML\Module\oidc\Controllers\AuthorizationController; use SimpleSAML\Module\oidc\Controllers\ConfigurationDiscoveryController; use SimpleSAML\Module\oidc\Controllers\EndSessionController; @@ -57,6 +58,12 @@ ->controller([ClientController::class, 'delete']) ->methods([HttpMethodsEnum::POST->value]); + // Testing + + $routes->add(RoutesEnum::AdminTestTrustChainResolution->name, RoutesEnum::AdminTestTrustChainResolution->value) + ->controller([TestController::class, 'trustChainResolution']) + ->methods([HttpMethodsEnum::GET->value, HttpMethodsEnum::POST->value]); + /***************************************************************************************************************** * OpenID Connect ****************************************************************************************************************/ diff --git a/routing/services/services.yml b/routing/services/services.yml index 75e6030e..f120f146 100644 --- a/routing/services/services.yml +++ b/routing/services/services.yml @@ -99,6 +99,8 @@ services: factory: ['@SimpleSAML\Module\oidc\Factories\ResourceServerFactory', 'build'] # Utils + SimpleSAML\Module\oidc\Utils\Debug\ArrayLogger: ~ + SimpleSAML\Module\oidc\Utils\FederationParticipationValidator: ~ SimpleSAML\Module\oidc\Utils\Routes: ~ SimpleSAML\Module\oidc\Utils\RequestParamsResolver: ~ SimpleSAML\Module\oidc\Utils\ClassInstanceBuilder: ~ diff --git a/src/Codebooks/LimitsEnum.php b/src/Codebooks/LimitsEnum.php new file mode 100644 index 00000000..90dd4993 --- /dev/null +++ b/src/Codebooks/LimitsEnum.php @@ -0,0 +1,11 @@ +authorization->requireAdmin(true); + } + + /** + * @throws \SimpleSAML\Error\ConfigurationError + * @throws \SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException + * @throws \SimpleSAML\Module\oidc\Exceptions\OidcException + */ + public function trustChainResolution(Request $request): Response + { + $this->arrayLogger->setWeight(ArrayLogger::WEIGHT_WARNING); + // Let's create new Federation instance so we can inject our debug logger and go without cache. + $federation = new Federation( + supportedAlgorithms: $this->federation->supportedAlgorithms(), + cache: null, + logger: $this->arrayLogger, + ); + + $leafEntityId = $this->moduleConfig->getIssuer(); + $trustChainBag = null; + $resolvedMetadata = []; + $isFormSubmitted = false; + + try { + $trustAnchorIds = $this->moduleConfig->getFederationTrustAnchorIds(); + } catch (\Throwable $exception) { + $this->arrayLogger->error('Module config error: ' . $exception->getMessage()); + $trustAnchorIds = []; + } + + if ($request->isMethod(Request::METHOD_POST)) { + $isFormSubmitted = true; + + !empty($leafEntityId = $request->request->getString('leafEntityId')) || + throw new OidcException('Empty leaf entity ID.'); + !empty($rawTrustAnchorIds = $request->request->getString('trustAnchorIds')) || + throw new OidcException('Empty Trust Anchor IDs.'); + + /** @var non-empty-array $trustAnchorIds */ + $trustAnchorIds = $this->helpers->str()->convertTextToArray($rawTrustAnchorIds); + + try { + $trustChainBag = $federation->trustChainResolver()->for($leafEntityId, $trustAnchorIds); + + foreach ($trustChainBag->getAll() as $index => $trustChain) { + $metadataEntries = []; + foreach (EntityTypesEnum::cases() as $entityTypeEnum) { + try { + $metadataEntries[$entityTypeEnum->value] = + $trustChain->getResolvedMetadata($entityTypeEnum); + } catch (\Throwable $exception) { + $this->arrayLogger->error( + 'Metadata resolving error: ' . $exception->getMessage(), + compact('index', 'entityTypeEnum'), + ); + continue; + } + } + $resolvedMetadata[$index] = array_filter($metadataEntries); + } + } catch (TrustChainException $exception) { + $this->arrayLogger->error('Trust chain error: ' . $exception->getMessage()); + } + } + + $trustAnchorIds = implode("\n", $trustAnchorIds); + $logMessages = $this->arrayLogger->getEntries(); +//dd($this->arrayLogger->getEntries()); + return $this->templateFactory->build( + 'oidc:tests/trust-chain-resolution.twig', + compact( + 'leafEntityId', + 'trustAnchorIds', + 'trustChainBag', + 'resolvedMetadata', + 'logMessages', + 'isFormSubmitted', + ), + RoutesEnum::AdminTestTrustChainResolution->value, + ); + } +} diff --git a/src/Controllers/Federation/Test.php b/src/Controllers/Federation/Test.php index f0e06be4..97eaaee4 100644 --- a/src/Controllers/Federation/Test.php +++ b/src/Controllers/Federation/Test.php @@ -65,25 +65,25 @@ public function __invoke(): Response // $requestObject = $requestObjectFactory->fromToken($unprotectedJws); // dd($requestObject, $requestObject->getPayload(), $requestObject->getHeader()); -// $cache->clear(); + $this->federationCache?->cache->clear(); $trustChain = $this->federation ->trustChainResolver() ->for( - 'https://08-dap.localhost.markoivancic.from.hr/openid/entities/ALeaf/', +// 'https://08-dap.localhost.markoivancic.from.hr/openid/entities/ALeaf/', // 'https://trust-anchor.testbed.oidcfed.incubator.geant.org/oidc/rp/', // 'https://relying-party-php.testbed.oidcfed.incubator.geant.org/', -// 'https://gorp.testbed.oidcfed.incubator.geant.org', + 'https://gorp.testbed.oidcfed.incubator.geant.org', // 'https://maiv1.incubator.geant.org', [ -// 'https://trust-anchor.testbed.oidcfed.incubator.geant.org/', + 'https://trust-anchor.testbed.oidcfed.incubator.geant.org/', 'https://08-dap.localhost.markoivancic.from.hr/openid/entities/ABTrustAnchor/', -// 'https://08-dap.localhost.markoivancic.from.hr/openid/entities/CTrustAnchor/', + 'https://08-dap.localhost.markoivancic.from.hr/openid/entities/CTrustAnchor/', ], - ); - + )->getAll(); +dd($trustChain); $leaf = $trustChain->getResolvedLeaf(); -// dd($leaf); + dd($leaf->getPayload()); $leafFederationJwks = $leaf->getJwks(); // dd($leafFederationJwks); // /** @psalm-suppress PossiblyNullArgument */ diff --git a/src/Factories/RequestRulesManagerFactory.php b/src/Factories/RequestRulesManagerFactory.php index ffc2ec4f..8aa57b32 100644 --- a/src/Factories/RequestRulesManagerFactory.php +++ b/src/Factories/RequestRulesManagerFactory.php @@ -36,6 +36,7 @@ use SimpleSAML\Module\oidc\Services\LoggerService; use SimpleSAML\Module\oidc\Utils\ClaimTranslatorExtractor; use SimpleSAML\Module\oidc\Utils\FederationCache; +use SimpleSAML\Module\oidc\Utils\FederationParticipationValidator; use SimpleSAML\Module\oidc\Utils\JwksResolver; use SimpleSAML\Module\oidc\Utils\ProtocolCache; use SimpleSAML\Module\oidc\Utils\RequestParamsResolver; @@ -58,6 +59,7 @@ public function __construct( private readonly Federation $federation, private readonly Helpers $helpers, private readonly JwksResolver $jwksResolver, + private readonly FederationParticipationValidator $federationParticipationValidator, private readonly ?FederationCache $federationCache = null, private readonly ?ProtocolCache $protocolCache = null, ) { @@ -88,6 +90,7 @@ private function getDefaultRules(): array $this->federation, $this->helpers, $this->jwksResolver, + $this->federationParticipationValidator, $this->federationCache, ), new RedirectUriRule($this->requestParamsResolver), diff --git a/src/Factories/TemplateFactory.php b/src/Factories/TemplateFactory.php index a3039779..0350af95 100644 --- a/src/Factories/TemplateFactory.php +++ b/src/Factories/TemplateFactory.php @@ -107,6 +107,13 @@ protected function includeDefaultMenuItems(): void ), ); + $this->oidcMenu->addItem( + $this->oidcMenu->buildItem( + $this->moduleConfig->getModuleUrl(RoutesEnum::AdminClients->value), + Translate::noop('Client Registry'), + ), + ); + $this->oidcMenu->addItem( $this->oidcMenu->buildItem( $this->moduleConfig->getModuleUrl(RoutesEnum::AdminConfigProtocol->value), @@ -123,8 +130,8 @@ protected function includeDefaultMenuItems(): void $this->oidcMenu->addItem( $this->oidcMenu->buildItem( - $this->moduleConfig->getModuleUrl(RoutesEnum::AdminClients->value), - Translate::noop('Client Registry'), + $this->moduleConfig->getModuleUrl(RoutesEnum::AdminTestTrustChainResolution->value), + Translate::noop('Test Trust Chain Resolution'), ), ); } diff --git a/src/Forms/ClientForm.php b/src/Forms/ClientForm.php index 3285f028..c0a272d3 100644 --- a/src/Forms/ClientForm.php +++ b/src/Forms/ClientForm.php @@ -414,6 +414,7 @@ protected function getScopes(): array } /** + * TODO mivanci Move to Str helper. * @return string[] */ protected function convertTextToArrayWithLinesAsValues(string $text): array diff --git a/src/Helpers/Arr.php b/src/Helpers/Arr.php index e15185e5..c7df69ac 100644 --- a/src/Helpers/Arr.php +++ b/src/Helpers/Arr.php @@ -14,4 +14,25 @@ public function ensureStringValues(array $values): array { return array_map(fn(mixed $value): string => (string)$value, $values); } + + public function isValueOneOf(mixed $value, array $set): bool + { + $value = is_array($value) ? $value : [$value]; + return !empty(array_intersect($value, $set)); + } + + public function isValueSubsetOf(mixed $value, array $superset): bool + { + $value = is_array($value) ? $value : [$value]; + + return empty(array_diff($value, $superset)); + } + + public function isValueSupersetOf(mixed $value, array $subset): bool + { + $value = is_array($value) ? $value : [$value]; + + // Opposite of subset... + return $this->isValueSubsetOf($subset, $value); + } } diff --git a/src/Helpers/Str.php b/src/Helpers/Str.php index 4674c786..9218119d 100644 --- a/src/Helpers/Str.php +++ b/src/Helpers/Str.php @@ -16,4 +16,16 @@ public function convertScopesStringToArray(string $scopes, string $delimiter = ' { return array_filter(explode($delimiter, trim($scopes)), fn($scope) => !empty($scope)); } + + /** + * @param non-empty-string $pattern + * @return string[] + */ + public function convertTextToArray(string $text, string $pattern = "/[\t\r\n]+/"): array + { + return array_filter( + preg_split($pattern, $text), + fn(string $line): bool => !empty(trim($line)), + ); + } } diff --git a/src/ModuleConfig.php b/src/ModuleConfig.php index 973d1f16..0ed106de 100644 --- a/src/ModuleConfig.php +++ b/src/ModuleConfig.php @@ -81,6 +81,8 @@ class ModuleConfig final public const OPTION_PROTOCOL_CACHE_ADAPTER = 'protocol_cache_adapter'; final public const OPTION_PROTOCOL_CACHE_ADAPTER_ARGUMENTS = 'protocol_cache_adapter_arguments'; final public const OPTION_PROTOCOL_USER_ENTITY_CACHE_DURATION = 'protocol_user_entity_cache_duration'; + final public const OPTION_FEDERATION_PARTICIPATION_LIMIT_BY_TRUST_MARKS = + 'federation_participation_limit_by_trust_marks'; protected static array $standardScopes = [ ScopesEnum::OpenId->value => [ @@ -465,7 +467,7 @@ public function getProtocolUserEntityCacheDuration(): DateInterval /***************************************************************************************************************** - * OpenID Connect related config. + * OpenID Federation related config. ****************************************************************************************************************/ public function getFederationEnabled(): bool @@ -669,4 +671,33 @@ public function getTrustAnchorJwks(string $trustAnchorId): ?array sprintf('Unexpected JWKS format for Trust Anchor %s: %s', $trustAnchorId, var_export($jwks, true)), ); } + + public function getFederationParticipationLimitByTrustMarks(): array + { + return $this->config()->getOptionalArray( + self::OPTION_FEDERATION_PARTICIPATION_LIMIT_BY_TRUST_MARKS, + [], + ); + } + + /** + * @throws \SimpleSAML\Error\ConfigurationError + */ + public function getTrustMarksNeededForFederationParticipationFor(string $trustAnchorId): array + { + $participationLimit = $this->getFederationParticipationLimitByTrustMarks()[$trustAnchorId] ?? []; + if (!is_array($participationLimit)) { + throw new ConfigurationError('Invalid configuration for federation participation limit.'); + } + + return $participationLimit; + } + + /** + * @throws \SimpleSAML\Error\ConfigurationError + */ + public function isFederationParticipationLimitedByTrustMarksFor(string $trustAnchorId): bool + { + return !empty($this->getTrustMarksNeededForFederationParticipationFor($trustAnchorId)); + } } diff --git a/src/Server/RequestRules/Rules/ClientIdRule.php b/src/Server/RequestRules/Rules/ClientIdRule.php index 7f64bb61..bd66acec 100644 --- a/src/Server/RequestRules/Rules/ClientIdRule.php +++ b/src/Server/RequestRules/Rules/ClientIdRule.php @@ -19,6 +19,7 @@ use SimpleSAML\Module\oidc\Server\RequestRules\Result; use SimpleSAML\Module\oidc\Services\LoggerService; use SimpleSAML\Module\oidc\Utils\FederationCache; +use SimpleSAML\Module\oidc\Utils\FederationParticipationValidator; use SimpleSAML\Module\oidc\Utils\JwksResolver; use SimpleSAML\Module\oidc\Utils\RequestParamsResolver; use SimpleSAML\OpenID\Codebooks\EntityTypesEnum; @@ -39,6 +40,7 @@ public function __construct( protected Federation $federation, protected Helpers $helpers, protected JwksResolver $jwksResolver, + protected FederationParticipationValidator $federationParticipationValidator, protected ?FederationCache $federationCache = null, ) { parent::__construct($requestParamsResolver); @@ -125,7 +127,7 @@ public function checkRule( $trustChain = $this->federation->trustChainResolver()->for( $clientEntityId, $this->moduleConfig->getFederationTrustAnchorIds(), - ); + )->getShortest(); } catch (ConfigurationError $exception) { throw OidcServerException::serverError( 'invalid OIDC configuration: ' . $exception->getMessage(), @@ -191,7 +193,16 @@ public function checkRule( // Verify signature on Request Object using client JWKS. $requestObject->verifyWithKeySet($clientJwks); - // Signature verified, we can persist (new) client registration. + // Check if federation participation is limited by Trust Marks. + if ( + $this->moduleConfig->isFederationParticipationLimitedByTrustMarksFor( + $trustChain->getResolvedTrustAnchor()->getIssuer(), + ) + ) { + $this->federationParticipationValidator->byTrustMarksFor($trustChain); + } + + // All is verified, We can persist (new) client registration. if ($existingClient) { $this->clientRepository->update($registrationClient); } else { diff --git a/src/Services/Container.php b/src/Services/Container.php index e9a3faa1..5a4a46cb 100644 --- a/src/Services/Container.php +++ b/src/Services/Container.php @@ -103,6 +103,7 @@ use SimpleSAML\Module\oidc\Utils\ClaimTranslatorExtractor; use SimpleSAML\Module\oidc\Utils\ClassInstanceBuilder; use SimpleSAML\Module\oidc\Utils\FederationCache; +use SimpleSAML\Module\oidc\Utils\FederationParticipationValidator; use SimpleSAML\Module\oidc\Utils\JwksResolver; use SimpleSAML\Module\oidc\Utils\ProtocolCache; use SimpleSAML\Module\oidc\Utils\RequestParamsResolver; @@ -346,6 +347,11 @@ public function __construct() $jwksResolver = new JwksResolver($jwks); $this->services[JwksResolver::class] = $jwksResolver; + $federationParticipationValidator = new FederationParticipationValidator( + $moduleConfig, + $loggerService, + ); + $this->services[FederationParticipationValidator::class] = $federationParticipationValidator; $requestRules = [ new StateRule($requestParamsResolver), @@ -357,6 +363,7 @@ public function __construct() $federation, $helpers, $jwksResolver, + $federationParticipationValidator, $federationCache, ), new RedirectUriRule($requestParamsResolver), diff --git a/src/Utils/Debug/ArrayLogger.php b/src/Utils/Debug/ArrayLogger.php new file mode 100644 index 00000000..d228693f --- /dev/null +++ b/src/Utils/Debug/ArrayLogger.php @@ -0,0 +1,160 @@ +setWeight($weight); + } + + public function setWeight(int $weight): void + { + $this->weight = max(self::WEIGHT_DEBUG, min($weight, self::WEIGHT_EMERGENCY)); + } + + /** + * @inheritDoc + */ + public function emergency(\Stringable|string $message, array $context = []): void + { + // Always log emergency. + $this->entries[] = $this->prepareEntry(LogLevel::EMERGENCY, $message, $context); + } + + /** + * @inheritDoc + */ + public function alert(\Stringable|string $message, array $context = []): void + { + if ($this->weight > self::WEIGH_ALERT) { + return; + } + $this->entries[] = $this->prepareEntry(LogLevel::ALERT, $message, $context); + } + + /** + * @inheritDoc + */ + public function critical(\Stringable|string $message, array $context = []): void + { + if ($this->weight > self::WEIGHT_CRITICAL) { + return; + } + $this->entries[] = $this->prepareEntry(LogLevel::CRITICAL, $message, $context); + } + + /** + * @inheritDoc + */ + public function error(\Stringable|string $message, array $context = []): void + { + if ($this->weight > self::WEIGHT_ERROR) { + return; + } + $this->entries[] = $this->prepareEntry(LogLevel::ERROR, $message, $context); + } + + /** + * @inheritDoc + */ + public function warning(\Stringable|string $message, array $context = []): void + { + if ($this->weight > self::WEIGHT_WARNING) { + return; + } + $this->entries[] = $this->prepareEntry(LogLevel::WARNING, $message, $context); + } + + /** + * @inheritDoc + */ + public function notice(\Stringable|string $message, array $context = []): void + { + if ($this->weight > self::WEIGHT_NOTICE) { + return; + } + $this->entries[] = $this->prepareEntry(LogLevel::NOTICE, $message, $context); + } + + /** + * @inheritDoc + */ + public function info(\Stringable|string $message, array $context = []): void + { + if ($this->weight > self::WEIGHT_INFO) { + return; + } + $this->entries[] = $this->prepareEntry(LogLevel::INFO, $message, $context); + } + + /** + * @inheritDoc + */ + public function debug(\Stringable|string $message, array $context = []): void + { + if ($this->weight > self::WEIGHT_DEBUG) { + return; + } + $this->entries[] = $this->prepareEntry(LogLevel::DEBUG, $message, $context); + } + + /** + * @inheritDoc + */ + public function log($level, \Stringable|string $message, array $context = []): void + { + match ($level) { + LogLevel::EMERGENCY => $this->emergency($message, $context), + LogLevel::ALERT => $this->alert($message, $context), + LogLevel::CRITICAL => $this->critical($message, $context), + LogLevel::ERROR => $this->error($message, $context), + LogLevel::WARNING => $this->warning($message, $context), + LogLevel::NOTICE => $this->notice($message, $context), + LogLevel::INFO => $this->info($message, $context), + LogLevel::DEBUG => $this->debug($message, $context), + default => throw new InvalidArgumentException("Unrecognized log level '$level''"), + }; + } + + public function getEntries(): array + { + return $this->entries; + } + + protected function prepareEntry(string $logLevel, \Stringable|string $message, array $context = []): string + { + return sprintf( + '%s %s %s %s', + $this->helpers->dateTime()->getUtc()->format(DateTimeInterface::RFC3339_EXTENDED), + strtoupper($logLevel), + $message, + empty($context) ? '' : 'Context: ' . var_export($context, true), + ); + } +} diff --git a/src/Utils/FederationParticipationValidator.php b/src/Utils/FederationParticipationValidator.php new file mode 100644 index 00000000..06bd37a5 --- /dev/null +++ b/src/Utils/FederationParticipationValidator.php @@ -0,0 +1,38 @@ +getResolvedTrustAnchor(); + + $trustMarkLimitsRules = $this->moduleConfig + ->getTrustMarksNeededForFederationParticipationFor($trustAnchor->getIssuer()); + + if (empty($trustMarkLimitsRules)) { + $this->loggerService->debug('No Trust Mark limits emposed for ' . $trustAnchor->getIssuer()); + return; + } + + $this->loggerService->debug('Trust Mark limits for ' . $trustAnchor->getIssuer(), $trustMarkLimitsRules); + + //$leaf = $trustChain->getResolvedLeaf(); + //$leafTrustMarks = $leaf->getTrustMarks(); + + // TODO mivanci continue + } +} diff --git a/src/Utils/Routes.php b/src/Utils/Routes.php index d256adf9..7b87f514 100644 --- a/src/Utils/Routes.php +++ b/src/Utils/Routes.php @@ -134,6 +134,13 @@ public function urlAdminClientsDelete(string $clientId, array $parameters = []): return $this->getModuleUrl(RoutesEnum::AdminClientsDelete->value, $parameters); } + // Testing + + public function urlAdminTestTrustChainResolution(array $parameters = []): string + { + return $this->getModuleUrl(RoutesEnum::AdminTestTrustChainResolution->value, $parameters); + } + /***************************************************************************************************************** * OpenID Connect URLs. ****************************************************************************************************************/ diff --git a/templates/tests/trust-chain-resolution.twig b/templates/tests/trust-chain-resolution.twig new file mode 100644 index 00000000..972aa720 --- /dev/null +++ b/templates/tests/trust-chain-resolution.twig @@ -0,0 +1,91 @@ +{% set subPageTitle = 'Test Trust Chain Resolution'|trans %} + +{% extends "@oidc/base.twig" %} + +{% block oidcContent %} + +

+ {{ 'You can use the form below to test Trust Chain resolution from a leaf entity ID to Trust Anchors.'|trans }} + {{ 'By default, form is populated with current OP issuer and configured Trust Anchors, but you are free to adjust entries as needed.'|trans }} + {{ 'Log messages will show if any warnings or errors were raised during chain resolution.'|trans }} +

+ +
+ +
+ + + + + + + {{ 'Enter one Trust Anchor ID per line.'|trans }} + +
+ +
+
+ + {% if isFormSubmitted|default %} + +

{{ 'Log messages'|trans }}

+

+ {% if logMessages|default %} + + {{- logMessages|json_encode(constant('JSON_PRETTY_PRINT') b-or constant('JSON_UNESCAPED_SLASHES')) -}} + + {% else %} + {{ 'No entries.'|trans }} + {% endif %} +

+ +

{{ 'Resolved chains'|trans }}

+ {% if trustChainBag|default %} +

+ {{ 'Total chains:'|trans }} {{ trustChainBag.getCount }} +

+ {% for index, trustChain in trustChainBag.getAll %} +

+ {{ loop.index }}. {{ 'Trust Anchor ID:'|trans }} {{ trustChain.getResolvedTrustAnchor.getIssuer }} +

+ {{ 'Path:'|trans }} +
+ {% for entity in trustChain.getEntities %} + {% if loop.index > 1 %} + ⇘ {{ loop.index0 }}. {{ entity.getSubject }}
+ {% endif %} + {% endfor %} + +
+ {{ 'Resolved metadata:' }}
+ {% if resolvedMetadata[index]|default is not empty %} + + {{- resolvedMetadata[index]|json_encode(constant('JSON_PRETTY_PRINT') b-or constant('JSON_UNESCAPED_SLASHES')) -}} + + {% else %} + {{ 'N/A'|trans }} + {% endif %} +

+ {% if not loop.last %} +

+ {% endif %} + {% endfor %} + {% else %} +

{{ 'No entries.'|trans }}

+ {% endif %} + + {% endif %} + +{% endblock oidcContent -%} diff --git a/tests/unit/src/Helpers/ArrTest.php b/tests/unit/src/Helpers/ArrTest.php new file mode 100644 index 00000000..a6fdd7e1 --- /dev/null +++ b/tests/unit/src/Helpers/ArrTest.php @@ -0,0 +1,49 @@ +assertTrue($this->sut()->isValueOneOf('a', ['a'])); + $this->assertTrue($this->sut()->isValueOneOf(['a'], ['a'])); + $this->assertTrue($this->sut()->isValueOneOf(['a', 'b'], ['a'])); + + $this->assertFalse($this->sut()->isValueOneOf('a', ['b'])); + $this->assertFalse($this->sut()->isValueOneOf(['a'], ['b'])); + } + + public function testIsValueSubsetOf(): void + { + $this->assertTrue($this->sut()->isValueSubsetOf('a', ['a', 'b', 'c'])); + $this->assertTrue($this->sut()->isValueSubsetOf(['a'], ['a', 'b', 'c'])); + $this->assertTrue($this->sut()->isValueSubsetOf(['a', 'b'], ['a', 'b', 'c'])); + + $this->assertFalse($this->sut()->isValueSubsetOf('a', [])); + $this->assertFalse($this->sut()->isValueSubsetOf('a', ['b'])); + $this->assertFalse($this->sut()->isValueSubsetOf(['a', 'c'], ['b'])); + } + + public function testIsValueSupersetOf(): void + { + $this->assertTrue($this->sut()->isValueSupersetOf('a', ['a'])); + $this->assertTrue($this->sut()->isValueSupersetOf(['a'], ['a'])); + $this->assertTrue($this->sut()->isValueSupersetOf(['a', 'b'], ['a'])); + + $this->assertFalse($this->sut()->isValueSupersetOf('a', ['b'])); + $this->assertFalse($this->sut()->isValueSupersetOf(['a'], ['b'])); + } +} diff --git a/tests/unit/src/Server/RequestRules/Rules/ClientIdRuleTest.php b/tests/unit/src/Server/RequestRules/Rules/ClientIdRuleTest.php index 5bf55763..fda3d8ae 100644 --- a/tests/unit/src/Server/RequestRules/Rules/ClientIdRuleTest.php +++ b/tests/unit/src/Server/RequestRules/Rules/ClientIdRuleTest.php @@ -18,6 +18,7 @@ use SimpleSAML\Module\oidc\Server\RequestRules\Rules\ClientIdRule; use SimpleSAML\Module\oidc\Services\LoggerService; use SimpleSAML\Module\oidc\Utils\FederationCache; +use SimpleSAML\Module\oidc\Utils\FederationParticipationValidator; use SimpleSAML\Module\oidc\Utils\JwksResolver; use SimpleSAML\Module\oidc\Utils\RequestParamsResolver; use SimpleSAML\OpenID\Federation; @@ -39,6 +40,7 @@ class ClientIdRuleTest extends TestCase protected Stub $clientEntityFactoryStub; protected Stub $helpersStub; protected Stub $jwksResolverStub; + protected Stub $federationParticipationValidatorStub; /** * @throws \Exception @@ -57,9 +59,10 @@ protected function setUp(): void $this->clientEntityFactoryStub = $this->createStub(ClientEntityFactory::class); $this->helpersStub = $this->createStub(Helpers::class); $this->jwksResolverStub = $this->createStub(JwksResolver::class); + $this->federationParticipationValidatorStub = $this->createStub(FederationParticipationValidator::class); } - protected function mock(): ClientIdRule + protected function sut(): ClientIdRule { return new ClientIdRule( $this->requestParamsResolverStub, @@ -69,20 +72,21 @@ protected function mock(): ClientIdRule $this->federationStub, $this->helpersStub, $this->jwksResolverStub, + $this->federationParticipationValidatorStub, $this->federationCacheStub, ); } public function testConstruct(): void { - $this->assertInstanceOf(ClientIdRule::class, $this->mock()); + $this->assertInstanceOf(ClientIdRule::class, $this->sut()); } public function testCheckRuleEmptyClientIdThrows(): void { $this->requestParamsResolverStub->method('getBasedOnAllowedMethods')->willReturn(null); $this->expectException(OidcServerException::class); - $this->mock()->checkRule( + $this->sut()->checkRule( $this->requestStub, $this->resultBagStub, $this->loggerServiceStub, @@ -94,7 +98,7 @@ public function testCheckRuleInvalidClientThrows(): void $this->requestParamsResolverStub->method('getBasedOnAllowedMethods')->willReturn('123'); $this->clientRepositoryStub->method('getClientEntity')->willReturn('invalid'); $this->expectException(OidcServerException::class); - $this->mock()->checkRule( + $this->sut()->checkRule( $this->requestStub, $this->resultBagStub, $this->loggerServiceStub, @@ -110,7 +114,7 @@ public function testCheckRuleForValidClientId(): void $this->requestParamsResolverStub->method('getBasedOnAllowedMethods')->willReturn('123'); $this->clientRepositoryStub->method('getClientEntity')->willReturn($this->clientEntityStub); - $result = $this->mock()->checkRule( + $result = $this->sut()->checkRule( $this->requestStub, $this->resultBagStub, $this->loggerServiceStub, diff --git a/tests/unit/src/Utils/Debug/ArrayLoggerTest.php b/tests/unit/src/Utils/Debug/ArrayLoggerTest.php new file mode 100644 index 00000000..bd519163 --- /dev/null +++ b/tests/unit/src/Utils/Debug/ArrayLoggerTest.php @@ -0,0 +1,89 @@ +helpersMock = $this->createMock(Helpers::class); + $this->dateTimeMock = $this->createMock(Helpers\DateTime::class); + $this->helpersMock->method('dateTime')->willReturn($this->dateTimeMock); + $this->dateTimeMock->method('getUtc')->willReturn(new \DateTimeImmutable()); + $this->weight = ArrayLogger::WEIGHT_DEBUG; + } + + protected function sut( + ?Helpers $helpers = null, + ?int $weight = null, + ): ArrayLogger { + $helpers ??= $this->helpersMock; + $weight ??= $this->weight; + + return new ArrayLogger($helpers, $weight); + } + + public function testCanCreateInstance(): void + { + $this->assertInstanceOf(ArrayLogger::class, $this->sut()); + } + + public function testCanLogEntriesBasedOnWeight(): void + { + $sut = $this->sut(); + $this->assertEmpty($sut->getEntries()); + + $sut->debug('debug message'); + $sut->info('info message'); + $sut->notice('notice message'); + $sut->warning('warning message'); + $sut->error('error message'); + $sut->critical('critical message'); + $sut->alert('alert message'); + $sut->emergency('emergency message'); + $sut->log(LogLevel::DEBUG, 'debug message'); + + $this->assertCount(9, $sut->getEntries()); + + } + + public function testWontLogLessThanEmergency(): void + { + $sut = $this->sut(weight: ArrayLogger::WEIGHT_EMERGENCY); + + $sut->debug('debug message'); + $sut->info('info message'); + $sut->notice('notice message'); + $sut->warning('warning message'); + $sut->error('error message'); + $sut->critical('critical message'); + $sut->alert('alert message'); + + $this->assertEmpty($sut->getEntries()); + + $sut->emergency('emergency message'); + $this->assertNotEmpty($sut->getEntries()); + } + + public function testThrowsOnInvalidLogLevel(): void + { + $this->expectException(InvalidArgumentException::class); + + $this->sut()->log('invalid', 'message'); + } +} From e5e652995a598526b9fd8f6d754b02b6a3655b79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Sat, 14 Dec 2024 18:17:07 +0100 Subject: [PATCH 7/7] Fix phpcs --- src/Controllers/Admin/TestController.php | 6 +++--- tests/unit/src/Utils/Debug/ArrayLoggerTest.php | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/Controllers/Admin/TestController.php b/src/Controllers/Admin/TestController.php index bbb30af8..7da4c8d5 100644 --- a/src/Controllers/Admin/TestController.php +++ b/src/Controllers/Admin/TestController.php @@ -61,9 +61,9 @@ public function trustChainResolution(Request $request): Response $isFormSubmitted = true; !empty($leafEntityId = $request->request->getString('leafEntityId')) || - throw new OidcException('Empty leaf entity ID.'); + throw new OidcException('Empty leaf entity ID.'); !empty($rawTrustAnchorIds = $request->request->getString('trustAnchorIds')) || - throw new OidcException('Empty Trust Anchor IDs.'); + throw new OidcException('Empty Trust Anchor IDs.'); /** @var non-empty-array $trustAnchorIds */ $trustAnchorIds = $this->helpers->str()->convertTextToArray($rawTrustAnchorIds); @@ -76,7 +76,7 @@ public function trustChainResolution(Request $request): Response foreach (EntityTypesEnum::cases() as $entityTypeEnum) { try { $metadataEntries[$entityTypeEnum->value] = - $trustChain->getResolvedMetadata($entityTypeEnum); + $trustChain->getResolvedMetadata($entityTypeEnum); } catch (\Throwable $exception) { $this->arrayLogger->error( 'Metadata resolving error: ' . $exception->getMessage(), diff --git a/tests/unit/src/Utils/Debug/ArrayLoggerTest.php b/tests/unit/src/Utils/Debug/ArrayLoggerTest.php index bd519163..0a090174 100644 --- a/tests/unit/src/Utils/Debug/ArrayLoggerTest.php +++ b/tests/unit/src/Utils/Debug/ArrayLoggerTest.php @@ -59,7 +59,6 @@ public function testCanLogEntriesBasedOnWeight(): void $sut->log(LogLevel::DEBUG, 'debug message'); $this->assertCount(9, $sut->getEntries()); - } public function testWontLogLessThanEmergency(): void