Resolve PHP v8 depreciations in v2 (#199)

* 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ć <[email protected]>
simplesamlphp · Jul 25, 2023 · 881c3ac · 881c3ac
1 parent a3f7643
commit 881c3ac
Show file tree

Hide file tree

Showing 17 changed files with 96 additions and 98 deletions.
diff --git 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
@@ -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
@@ -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
@@ -21,6 +21,8 @@
 
 class ClientForm extends Form
 {
+    protected const TYPE_ARRAY = 'array';
+
     /**
      * RFC3986. AppendixB. Parsing a URI Reference with a Regular Expression.
      */
@@ -55,26 +57,35 @@ 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: '
         );
     }
 
     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: '
         );
     }
 
     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
@@ -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
@@ -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
@@ -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();