From 881c3acf445dd731ca8de165950a90faf9ffceb3 Mon Sep 17 00:00:00 2001 From: Marko Ivancic Date: Tue, 25 Jul 2023 14:26:36 +0200 Subject: [PATCH] Resolve PHP v8 depreciations in v2 (#199) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Update nette/forms to v3.0 * Use SSP session instead of nette session for CSRF * Use own quality tools * Fix psalm errors * Fix tests * Move phpcs to dev * Bump conformance-suite version * Update test.yml --------- Co-authored-by: Marko Ivančić --- .github/workflows/test.yaml | 66 ++++++------------- CONFORMANCE_TEST.md | 2 +- composer.json | 10 +-- lib/Form/ClientForm.php | 56 ++++++++++------ lib/Form/Controls/CsrfProtection.php | 11 +++- lib/Repositories/AuthCodeRepository.php | 6 +- lib/Server/Grants/AuthCodeGrant.php | 13 +--- lib/Server/Grants/ImplicitGrant.php | 4 +- .../Grants/Traits/IssueAccessTokenTrait.php | 3 - .../Validators/BearerTokenValidator.php | 1 + lib/Services/IdTokenBuilder.php | 2 +- lib/Utils/Checker/Rules/IdTokenHintRule.php | 2 + lib/Utils/Checker/Rules/ScopeRule.php | 5 ++ lib/Utils/UniqueIdentifierGenerator.php | 4 ++ psalm.xml | 4 +- tests/Services/JsonWebKeySetServiceTest.php | 4 +- .../Checker/Rules/IdTokenHintRuleTest.php | 1 + 17 files changed, 96 insertions(+), 98 deletions(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 1121d5e0..37afbfa0 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -13,7 +13,7 @@ jobs: strategy: fail-fast: false matrix: - php-versions: ["7.4"] + php-versions: ["7.4", "8.0", "8.1"] steps: - name: Setup PHP, with composer and extensions @@ -35,14 +35,14 @@ jobs: git config --global core.autocrlf false git config --global core.eol lf - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - name: Get composer cache directory id: composer-cache run: echo "::set-output name=dir::$(composer config cache-files-dir)" - name: Cache composer dependencies - uses: actions/cache@v1 + uses: actions/cache@v3 with: path: ${{ steps.composer-cache.outputs.dir }} key: ${{ runner.os }}-composer-${{ hashFiles('**/composer.lock') }} @@ -54,9 +54,6 @@ jobs: - name: Install Composer dependencies run: composer install --no-progress --prefer-dist --optimize-autoloader - - name: Syntax check PHP - run: bash vendor/bin/check-syntax-php.sh - - name: Decide whether to run code coverage or not if: ${{ matrix.php-versions != '7.4' }} run: | @@ -90,7 +87,7 @@ jobs: - name: Setup problem matchers for PHP run: echo "::add-matcher::${{ runner.tool_cache }}/php.json" - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - name: Get composer cache directory id: composer-cache @@ -131,14 +128,14 @@ jobs: - name: Setup problem matchers for PHP run: echo "::add-matcher::${{ runner.tool_cache }}/php.json" - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - name: Get composer cache directory id: composer-cache run: echo "::set-output name=dir::$(composer config cache-files-dir)" - name: Cache composer dependencies - uses: actions/cache@v1 + uses: actions/cache@v3 with: path: ${{ steps.composer-cache.outputs.dir }} key: ${{ runner.os }}-composer-${{ hashFiles('**/composer.lock') }} @@ -147,12 +144,6 @@ jobs: - name: Install Composer dependencies run: composer install --no-progress --prefer-dist --optimize-autoloader - - name: Syntax check YAML / XML / JSON - run: | - bash vendor/bin/check-syntax-yaml.sh - bash vendor/bin/check-syntax-xml.sh - bash vendor/bin/check-syntax-json.sh - quality: name: Quality control runs-on: [ubuntu-latest] @@ -169,14 +160,14 @@ jobs: - name: Setup problem matchers for PHP run: echo "::add-matcher::${{ runner.tool_cache }}/php.json" - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - name: Get composer cache directory id: composer-cache run: echo "::set-output name=dir::$(composer config cache-files-dir)" - name: Cache composer dependencies - uses: actions/cache@v1 + uses: actions/cache@v3 with: path: ${{ steps.composer-cache.outputs.dir }} key: ${{ runner.os }}-composer-${{ hashFiles('**/composer.lock') }} @@ -185,13 +176,13 @@ jobs: - name: Install Composer dependencies run: composer install --no-progress --prefer-dist --optimize-autoloader - - uses: actions/download-artifact@v1 + - uses: actions/download-artifact@v3 with: name: build-data path: ${{ github.workspace }}/build - name: Codecov - uses: codecov/codecov-action@v1 + uses: codecov/codecov-action@v3 - name: PHP Code Sniffer if: always() @@ -201,21 +192,19 @@ jobs: if: always() run: php vendor/bin/psalm --show-info=true - - name: Psalter - if: always() - run: php vendor/bin/psalter --issues=UnnecessaryVarAnnotation --dry-run - - build-conformance-suite: + conformance-suite: runs-on: ubuntu-latest env: - VERSION: release-v4.1.11 + SUITE_BASE_URL: https://localhost.emobix.co.uk:8443 + VERSION: release-v4.1.45 steps: - - name: Load Cached Conformance Suite Build - uses: actions/cache@v2 - id: cache + - uses: actions/checkout@v3 with: - path: ./conformance-suite - key: suite-${{ hashFiles('**/test.yml') }} + path: main + - name: Setup Python Dependencies + run: | + pip install --upgrade pip + pip install httpx - name: Conformance Suite Checkout if: ${{ steps.cache.outputs.cache-hit != 'true' }} run: git clone --depth 1 --single-branch --branch $VERSION https://gitlab.com/openid/conformance-suite.git @@ -228,23 +217,6 @@ jobs: sed -i -e 's/localhost/localhost.emobix.co.uk/g' src/main/resources/application.properties sed -i -e 's/-B clean/-B -DskipTests=true/g' builder-compose.yml docker-compose -f builder-compose.yml run builder - - conformance-suite: - runs-on: ubuntu-latest - needs: - - build-conformance-suite - env: - SUITE_BASE_URL: https://localhost.emobix.co.uk:8443 - steps: - - uses: actions/checkout@v2 - with: - path: main - - name: Load Cached Conformance Suite Build - uses: actions/cache@v2 - id: cache - with: - path: ./conformance-suite - key: suite-${{ hashFiles('**/test.yml') }} - name: Run Conformance Suite working-directory: ./conformance-suite run: | diff --git a/CONFORMANCE_TEST.md b/CONFORMANCE_TEST.md index 796aca71..3e91c8dc 100644 --- a/CONFORMANCE_TEST.md +++ b/CONFORMANCE_TEST.md @@ -15,7 +15,7 @@ Clone the conformance test git repo, build the software and run it. git clone https://gitlab.com/openid/conformance-suite.git cd conformance-suite # Version 4.1.10 has a bug when building -git checkout release-v4.1.9 +git checkout release-v4.1.45 MAVEN_CACHE=./m2 docker-compose -f builder-compose.yml run builder docker-compose up ``` diff --git a/composer.json b/composer.json index 94726d39..d306fe8e 100644 --- a/composer.json +++ b/composer.json @@ -27,7 +27,7 @@ "laminas/laminas-httphandlerrunner": "^1.1.0", "lcobucci/jwt": "^4.1", "league/oauth2-server": "^8.1.0", - "nette/forms": "^2.4", + "nette/forms": "^3.1", "psr/container": "^1.0", "psr/log": "^1.1", "simplesamlphp/composer-module-installer": "^1.0", @@ -43,7 +43,8 @@ "phpunit/phpcov": "^8.2.0", "phpunit/phpunit": "^9.0.0", "simplesamlphp/simplesamlphp": "^1.19,<2", - "simplesamlphp/simplesamlphp-test-framework": "^0.1.9" + "squizlabs/php_codesniffer": "^3.7", + "vimeo/psalm": "^5.8" }, "config": { "preferred-install": { @@ -71,12 +72,7 @@ }, "scripts": { "pre-commit": [ - "vendor/bin/check-syntax-php.sh", - "vendor/bin/check-syntax-json.sh", - "vendor/bin/check-syntax-xml.sh", - "vendor/bin/check-syntax-yaml.sh", "vendor/bin/psalm", - "vendor/bin/psalter --issues=UnnecessaryVarAnnotation --dry-run", "vendor/bin/phpcs -p" ], "tests": [ diff --git a/lib/Form/ClientForm.php b/lib/Form/ClientForm.php index d2420bc2..824ba610 100644 --- a/lib/Form/ClientForm.php +++ b/lib/Form/ClientForm.php @@ -21,6 +21,8 @@ class ClientForm extends Form { + protected const TYPE_ARRAY = 'array'; + /** * RFC3986. AppendixB. Parsing a URI Reference with a Regular Expression. */ @@ -55,8 +57,11 @@ public function __construct(ConfigurationService $configurationService) public function validateRedirectUri(Form $form): void { + /** @var array $values */ + $values = $form->getValues(self::TYPE_ARRAY); + $this->validateByMatchingRegex( - $form->getValues()['redirect_uri'], + $values['redirect_uri'] ?? [], self::REGEX_URI, 'Invalid URI: ' ); @@ -64,8 +69,11 @@ public function validateRedirectUri(Form $form): void public function validateAllowedOrigin(Form $form): void { + /** @var array $values */ + $values = $form->getValues(self::TYPE_ARRAY); + $this->validateByMatchingRegex( - $form->getValues()['allowed_origin'] ?? [], + $values['allowed_origin'] ?? [], self::REGEX_ALLOWED_ORIGIN_URL, 'Invalid allowed origin: ' ); @@ -73,8 +81,11 @@ public function validateAllowedOrigin(Form $form): void public function validatePostLogoutRedirectUri(Form $form): void { + /** @var array $values */ + $values = $form->getValues(self::TYPE_ARRAY); + $this->validateByMatchingRegex( - $form->getValues()['post_logout_redirect_uri'] ?? [], + $values['post_logout_redirect_uri'] ?? [], self::REGEX_URI, 'Invalid post-logout redirect URI: ' ); @@ -108,9 +119,10 @@ protected function validateByMatchingRegex( * * @return array */ - public function getValues($asArray = false): array + public function getValues($returnType = null, ?array $controls = null): array { - $values = parent::getValues(true); + /** @var array $values */ + $values = parent::getValues(self::TYPE_ARRAY); // Sanitize redirect_uri and allowed_origin $values['redirect_uri'] = $this->convertTextToArrayWithLinesAsValues($values['redirect_uri']); @@ -136,37 +148,43 @@ public function getValues($asArray = false): array return $values; } - /** - * @param array $values - * @param bool $erase - * - * @return Form - */ - public function setDefaults($values, $erase = false): Form + public function setDefaults($data, bool $erase = false) { - $values['redirect_uri'] = implode("\n", $values['redirect_uri']); + if (! is_array($data)) { + if ($data instanceof \Traversable) { + $data = iterator_to_array($data); + } else { + $data = (array) $data; + } + } + + $data['redirect_uri'] = implode("\n", $data['redirect_uri']); // Allowed origins are only available for public clients (not for confidential clients). - if (! $values['is_confidential'] && isset($values['allowed_origin'])) { - $values['allowed_origin'] = implode("\n", $values['allowed_origin']); + if (! $data['is_confidential'] && isset($data['allowed_origin'])) { + $data['allowed_origin'] = implode("\n", $data['allowed_origin']); } else { - $values['allowed_origin'] = ''; + $data['allowed_origin'] = ''; } - $values['post_logout_redirect_uri'] = implode("\n", $values['post_logout_redirect_uri']); + $data['post_logout_redirect_uri'] = implode("\n", $data['post_logout_redirect_uri']); - $values['scopes'] = array_intersect($values['scopes'], array_keys($this->getScopes())); + $data['scopes'] = array_intersect($data['scopes'], array_keys($this->getScopes())); - return parent::setDefaults($values, $erase); + return parent::setDefaults($data, $erase); } protected function buildForm(): void { $this->getElementPrototype()->addAttributes(['class' => 'ui form']); + /** @psalm-suppress InvalidPropertyAssignmentValue According to docs this is fine. */ $this->onValidate[] = [$this, 'validateRedirectUri']; + /** @psalm-suppress InvalidPropertyAssignmentValue According to docs this is fine. */ $this->onValidate[] = [$this, 'validateAllowedOrigin']; + /** @psalm-suppress InvalidPropertyAssignmentValue According to docs this is fine. */ $this->onValidate[] = [$this, 'validatePostLogoutRedirectUri']; + /** @psalm-suppress InvalidPropertyAssignmentValue According to docs this is fine. */ $this->onValidate[] = [$this, 'validateBackChannelLogoutUri']; $this->setMethod('POST'); diff --git a/lib/Form/Controls/CsrfProtection.php b/lib/Form/Controls/CsrfProtection.php index 7b5ef3d4..d25bb8d7 100644 --- a/lib/Form/Controls/CsrfProtection.php +++ b/lib/Form/Controls/CsrfProtection.php @@ -15,6 +15,7 @@ namespace SimpleSAML\Module\oidc\Form\Controls; use Nette\Forms\Controls\CsrfProtection as BaseCsrfProtection; +use Nette\InvalidStateException; use Nette\Utils\Random; use SimpleSAML\Session; use SimpleSAML\SessionHandler; @@ -25,7 +26,15 @@ class CsrfProtection extends BaseCsrfProtection public function __construct($errorMessage) { - parent::__construct($errorMessage); + // Instead of calling CsrfProtection parent class constructor, go to it's parent (HiddenField), and call + // its constructor. This is to avoid setting a Nette session in CsrfProtection parent, and use the SSP one. + $hiddentFieldParent = get_parent_class(get_parent_class($this)); + + if (! is_string($hiddentFieldParent)) { + throw new InvalidStateException('CsrfProtection initialization error'); + } + + $hiddentFieldParent::__construct(); $this->getRules()->reset(); $this->addRule(self::PROTECTION, $errorMessage); diff --git a/lib/Repositories/AuthCodeRepository.php b/lib/Repositories/AuthCodeRepository.php index ef4e881e..0aa960e0 100644 --- a/lib/Repositories/AuthCodeRepository.php +++ b/lib/Repositories/AuthCodeRepository.php @@ -102,12 +102,12 @@ public function revokeAuthCode($codeId) /** * {@inheritdoc} */ - public function isAuthCodeRevoked($tokenId): bool + public function isAuthCodeRevoked($codeId): bool { - $authCode = $this->findById($tokenId); + $authCode = $this->findById($codeId); if (!$authCode instanceof AuthCodeEntity) { - throw new \RuntimeException("AuthCode not found: {$tokenId}"); + throw new \RuntimeException("AuthCode not found: {$codeId}"); } return $authCode->isRevoked(); diff --git a/lib/Server/Grants/AuthCodeGrant.php b/lib/Server/Grants/AuthCodeGrant.php index 7f398796..3947535d 100644 --- a/lib/Server/Grants/AuthCodeGrant.php +++ b/lib/Server/Grants/AuthCodeGrant.php @@ -71,19 +71,10 @@ class AuthCodeGrant extends OAuth2AuthCodeGrant implements */ protected array $codeChallengeVerifiers = []; - /** - * @var AuthCodeRepositoryInterface - */ protected $authCodeRepository; - /** - * @var AccessTokenRepositoryInterface - */ protected $accessTokenRepository; - /** - * @var RefreshTokenRepositoryInterface - */ protected $refreshTokenRepository; private RequestRulesManager $requestRulesManager; @@ -120,10 +111,10 @@ public function __construct( } /** - * @param ClientEntityInterface $client + * @param OAuth2ClientEntityInterface $client * @return bool */ - protected function shouldCheckPkce(ClientEntityInterface $client): bool + protected function shouldCheckPkce(OAuth2ClientEntityInterface $client): bool { return $this->requireCodeChallengeForPublicClients && ! $client->isConfidential(); diff --git a/lib/Server/Grants/ImplicitGrant.php b/lib/Server/Grants/ImplicitGrant.php index fca781df..3b39e17d 100644 --- a/lib/Server/Grants/ImplicitGrant.php +++ b/lib/Server/Grants/ImplicitGrant.php @@ -236,10 +236,10 @@ private function getRedirectUrl(AuthorizationRequest $authorizationRequest): str $redirectUris = $authorizationRequest->getClient()->getRedirectUri(); if (is_array($redirectUris)) { - return (string) $redirectUris[0]; + return $redirectUris[0]; } - return (string) $redirectUris; + return $redirectUris; } /** diff --git a/lib/Server/Grants/Traits/IssueAccessTokenTrait.php b/lib/Server/Grants/Traits/IssueAccessTokenTrait.php index e7eaf9b1..3f057be1 100644 --- a/lib/Server/Grants/Traits/IssueAccessTokenTrait.php +++ b/lib/Server/Grants/Traits/IssueAccessTokenTrait.php @@ -22,9 +22,6 @@ */ trait IssueAccessTokenTrait { - /** - * @var AccessTokenRepositoryInterface - */ protected $accessTokenRepository; /** diff --git a/lib/Server/Validators/BearerTokenValidator.php b/lib/Server/Validators/BearerTokenValidator.php index 889c6dce..e1af4c49 100644 --- a/lib/Server/Validators/BearerTokenValidator.php +++ b/lib/Server/Validators/BearerTokenValidator.php @@ -66,6 +66,7 @@ protected function initJwtConfiguration() InMemory::empty() ); + /** @psalm-suppress ArgumentTypeCoercion */ $this->jwtConfiguration->setValidationConstraints( new StrictValidAt(new SystemClock(new DateTimeZone(\date_default_timezone_get()))), new SignedWith( diff --git a/lib/Services/IdTokenBuilder.php b/lib/Services/IdTokenBuilder.php index 0ea00c39..fdfe8888 100644 --- a/lib/Services/IdTokenBuilder.php +++ b/lib/Services/IdTokenBuilder.php @@ -158,7 +158,7 @@ protected function generateAccessTokenHash(AccessTokenEntityInterface $accessTok $hashAlgorithm = 'sha' . $jwsAlgorithmBitLength; - $hashByteLength = (int) ($jwsAlgorithmBitLength / 2 / 8); + $hashByteLength = ($jwsAlgorithmBitLength / 2 / 8); return Base64Url::encode( substr( diff --git a/lib/Utils/Checker/Rules/IdTokenHintRule.php b/lib/Utils/Checker/Rules/IdTokenHintRule.php index cb488c9c..8e9bb631 100644 --- a/lib/Utils/Checker/Rules/IdTokenHintRule.php +++ b/lib/Utils/Checker/Rules/IdTokenHintRule.php @@ -58,6 +58,7 @@ public function checkRule( $privateKey = $this->cryptKeyFactory->buildPrivateKey(); $publicKey = $this->cryptKeyFactory->buildPublicKey(); + /** @psalm-suppress ArgumentTypeCoercion */ $jwtConfig = Configuration::forAsymmetricSigner( $this->configurationService->getSigner(), InMemory::plainText($privateKey->getKeyContents(), $privateKey->getPassPhrase() ?? ''), @@ -68,6 +69,7 @@ public function checkRule( /** @var UnencryptedToken $idTokenHint */ $idTokenHint = $jwtConfig->parser()->parse($idTokenHintParam); + /** @psalm-suppress ArgumentTypeCoercion */ $jwtConfig->validator()->assert( $idTokenHint, new IssuedBy($this->configurationService->getSimpleSAMLSelfURLHost()), diff --git a/lib/Utils/Checker/Rules/ScopeRule.php b/lib/Utils/Checker/Rules/ScopeRule.php index 89ccb6db..18c77230 100644 --- a/lib/Utils/Checker/Rules/ScopeRule.php +++ b/lib/Utils/Checker/Rules/ScopeRule.php @@ -68,9 +68,14 @@ public function checkRule( * @param string $scopes * @param string $scopeDelimiterString * @return array + * @throws OidcServerException */ protected function convertScopesQueryStringToArray(string $scopes, string $scopeDelimiterString): array { + if (empty($scopeDelimiterString)) { + throw OidcServerException::serverError('Scope delimiter string can not be empty.'); + } + return array_filter(explode($scopeDelimiterString, trim($scopes)), function ($scope) { return !empty($scope); }); diff --git a/lib/Utils/UniqueIdentifierGenerator.php b/lib/Utils/UniqueIdentifierGenerator.php index 86217112..108c6cf7 100644 --- a/lib/Utils/UniqueIdentifierGenerator.php +++ b/lib/Utils/UniqueIdentifierGenerator.php @@ -17,6 +17,10 @@ class UniqueIdentifierGenerator */ public static function hitMe(int $length = 40): string { + if ($length < 1) { + throw OidcServerException::serverError('Random string lenght can not be less than 1'); + } + try { return bin2hex(random_bytes($length)); } catch (Throwable $e) { diff --git a/psalm.xml b/psalm.xml index 69ed7afa..a287f81e 100644 --- a/psalm.xml +++ b/psalm.xml @@ -4,13 +4,14 @@ useDocblockTypes="true" totallyTyped="false" hideExternalErrors="true" + findUnusedBaselineEntry="true" + findUnusedCode="false" > - @@ -31,7 +32,6 @@ - diff --git a/tests/Services/JsonWebKeySetServiceTest.php b/tests/Services/JsonWebKeySetServiceTest.php index d3ca5721..b7eb74bf 100644 --- a/tests/Services/JsonWebKeySetServiceTest.php +++ b/tests/Services/JsonWebKeySetServiceTest.php @@ -48,7 +48,9 @@ public static function setUpBeforeClass(): void self::$pkGeneratePublic = $pkGenerateDetails['key']; // free resources - openssl_pkey_free($pkGenerate); + if (version_compare(PHP_VERSION, '8.0.0') < 0) { + openssl_pkey_free($pkGenerate); + } file_put_contents(sys_get_temp_dir() . '/oidc_module.crt', self::$pkGeneratePublic); diff --git a/tests/Utils/Checker/Rules/IdTokenHintRuleTest.php b/tests/Utils/Checker/Rules/IdTokenHintRuleTest.php index ba50156d..ea0329eb 100644 --- a/tests/Utils/Checker/Rules/IdTokenHintRuleTest.php +++ b/tests/Utils/Checker/Rules/IdTokenHintRuleTest.php @@ -90,6 +90,7 @@ public function testConstruct(): void public function testCheckRuleIsNullWhenParamNotSet(): void { $rule = new IdTokenHintRule($this->configurationServiceStub, $this->cryptKeyFactoryStub); + $this->requestStub->method('getMethod')->willReturn(''); $result = $rule->checkRule( $this->requestStub, $this->resultBagStub,