Fix several issues reported by Psalm

tvdijen · tvdijen · commit 72c94defbd19 · 2023-01-29T19:03:20.000+01:00
diff --git a/.github/workflows/php.yml b/.github/workflows/php.yml
@@ -28,7 +28,7 @@ jobs:
           extensions: intl, mbstring, mysql, pdo, pdo_sqlite, xml
           tools: composer:v2
           ini-values: error_reporting=E_ALL
-          coverage: pcov
+          coverage: xdebug
 
       - name: Setup problem matchers for PHP
         run: echo "::add-matcher::${{ runner.tool_cache }}/php.json"
diff --git a/composer.lock b/composer.lock
diff --git a/modules/core/src/Auth/Source/Selector/RequestedAuthnContextSelector.php b/modules/core/src/Auth/Source/Selector/RequestedAuthnContextSelector.php
@@ -4,6 +4,7 @@
 
 namespace SimpleSAML\Module\core\Auth\Source\Selector;
 
+use Exception;
 use SAML2\Constants as C;
 use SAML2\Exception\Protocol\NoAuthnContextException;
 use SimpleSAML\Assert\Assert;
diff --git a/modules/core/src/Auth/Source/Selector/SourceIPSelector.php b/modules/core/src/Auth/Source/Selector/SourceIPSelector.php
@@ -4,6 +4,7 @@
 
 namespace SimpleSAML\Module\core\Auth\Source\Selector;
 
+use Exception;
 use SimpleSAML\Assert\Assert;
 use SimpleSAML\Logger;
 use SimpleSAML\Module\core\Auth\Source\AbstractSourceSelector;
diff --git a/modules/saml/src/Controller/Disco.php b/modules/saml/src/Controller/Disco.php
@@ -39,7 +39,7 @@ public function __construct(
     /**
      * Built-in IdP discovery service
      *
-     * @return \SimpleSAML\Http\RunnableResponse
+     * @return \SimpleSAML\HTTP\RunnableResponse
      */
     public function disco(): RunnableResponse
     {
diff --git a/modules/saml/src/Controller/ServiceProvider.php b/modules/saml/src/Controller/ServiceProvider.php
@@ -109,7 +109,7 @@ public function setAuthUtils(Utils\Auth $authUtils): void
      * Handler for response from IdP discovery service.
      *
      * @param \Symfony\Component\HttpFoundation\Request $request
-     * @return \SimpleSAML\Http\RunnableResponse
+     * @return \SimpleSAML\HTTP\RunnableResponse
      */
     public function discoResponse(Request $request): RunnableResponse
     {
@@ -155,7 +155,7 @@ public function wrongAuthnContextClassRef(): Template
      * Handler for the Assertion Consumer Service.
      *
      * @param string $sourceId
-     * @return \SimpleSAML\Http\RunnableResponse
+     * @return \SimpleSAML\HTTP\RunnableResponse
      */
     public function assertionConsumerService(string $sourceId): RunnableResponse
     {
@@ -423,7 +423,7 @@ public function assertionConsumerService(string $sourceId): RunnableResponse
      * This endpoint handles both logout requests and logout responses.
      *
      * @param string $sourceId
-     * @return \SimpleSAML\Http\RunnableResponse
+     * @return \SimpleSAML\HTTP\RunnableResponse
      */
     public function singleLogoutService(string $sourceId): RunnableResponse
     {
diff --git a/psalm.xml b/psalm.xml
@@ -72,6 +72,7 @@
                 <directory name="config" />
                 <directory name="metadata" />
                 <directory name="modules/adfs/metadata-templates" />
+                <directory name="tests/src/SimpleSAML/Metadata/test-metadata" />
             </errorLevel>
         </PossiblyUndefinedGlobalVariable>
 
diff --git a/public/saml2/idp/initSLO.php b/public/saml2/idp/initSLO.php
@@ -17,4 +17,4 @@
 $request = Request::createFromGlobals();
 $config = Configuration::getInstance();
 $controller = new Controller\SingleLogout($config);
-$controller->initSingleLogout()->send($request);
+$controller->initSingleLogout($request)->send();
diff --git a/src/SimpleSAML/Session.php b/src/SimpleSAML/Session.php
@@ -4,6 +4,7 @@
 
 namespace SimpleSAML;
 
+use DOMNodeList;
 use SAML2\XML\saml\AttributeValue;
 use SimpleSAML\Assert\Assert;
 use SimpleSAML\Error;
@@ -621,7 +622,7 @@ public function doLogin(string $authority, array $data = []): void
                 }
 
                 // at this point, this should be a DOMNodeList object...
-                if (!is_a($value, 'DOMNodeList')) {
+                if (!is_a($value, DOMNodeList::class)) {
                     continue;
                 }
 
@@ -894,7 +895,7 @@ public function setData(string $type, string $id, $data, $timeout = null): void
         if ($timeout === self::DATA_TIMEOUT_SESSION_END) {
             $expires = self::DATA_TIMEOUT_SESSION_END;
         } else {
-            $expires = time() + $timeout;
+            $expires = time() + intval($timeout);
         }
 
         $dataInfo = [
diff --git a/src/SimpleSAML/XHTML/Template.php b/src/SimpleSAML/XHTML/Template.php
@@ -198,9 +198,9 @@ class_exists($controller)
     public function asset(string $asset, string $module = null): string
     {
         $baseDir = $this->configuration->getBaseDir();
+        $basePath = $this->configuration->getBasePath();
         if (is_null($module)) {
             $file = $baseDir . 'public/assets/base/' . $asset;
-            $basePath = $this->configuration->getBasePath();
             $path = $basePath . 'assets/base/' . $asset;
         } elseif (file_exists($baseDir . 'public/assets/' . $module)) {
             $file = $baseDir . '/public/assets/' . $module . '/' . $asset;
diff --git a/tests/modules/core/src/Auth/Source/Selector/RequestedAuthnContextSelectorTest.php b/tests/modules/core/src/Auth/Source/Selector/RequestedAuthnContextSelectorTest.php
@@ -93,11 +93,19 @@ public static function doAuthentication(Auth\Source $as, array $state): void
             {
                 // Dummy
             }
+            /**
+             * @param array &$state
+             * @return void
+             */
+            public function authenticate(array &$state): void
+            {
+                $state['finished'] = true;
+            }
         };
 
         $state = ['saml:RequestedAuthnContext' => ['AuthnContextClassRef' => null]];
-        $result = $selector->authenticate($state);
-        $this->assertNull($result);
+        $selector->authenticate($state);
+        $this->assertTrue($state['finished']);
     }
 
 
diff --git a/tests/modules/core/src/Auth/Source/Selector/SourceIPSelectorTest.php b/tests/modules/core/src/Auth/Source/Selector/SourceIPSelectorTest.php
@@ -117,11 +117,20 @@ public static function doAuthentication(Auth\Source $as, array $state): void
             {
                 // Dummy
             }
+
+            /**
+             * @param array &$state
+             * @return void
+             */
+            public function authenticate(array &$state): void
+            {
+                $state['finished'] = true;
+            }
         };
 
         $state = [];
-        $result = $selector->authenticate($state);
-        $this->assertNull($result);
+        $selector->authenticate($state);
+        $this->assertTrue($state['finished']);
     }
 
 
@@ -151,7 +160,7 @@ public function selectAuthSource(array &$state): string
 
 
     /**
-     * @return string
+     * @return array
      */
     public function provideClientIP(): array
     {
diff --git a/tests/modules/saml/src/Controller/MetadataTest.php b/tests/modules/saml/src/Controller/MetadataTest.php
@@ -26,9 +26,6 @@ class MetadataTest extends TestCase
     /** @var \SimpleSAML\Configuration */
     protected Configuration $config;
 
-    /** @var \SimpleSAML\Session */
-    protected Session $session;
-
     /** @var \SimpleSAML\Utils\Auth */
     protected Utils\Auth $authUtils;
 
@@ -86,7 +83,7 @@ public function getMetaData(?string $entityId, string $set): array
             public function getList(string $set = 'saml20-idp-remote', bool $showExpired = false): array
             {
                 if ($set === 'saml20-idp-hosted') {
-                    return $idps;
+                    return $this->idps;
                 }
                 return [];
             }
@@ -96,7 +93,6 @@ public function getMetaDataCurrentEntityID(string $set, string $type = 'entityid
                 return 'urn:example:simplesaml:another:idp';
             }
         };
-        $this->session = Session::getSessionFromRequest();
 
         $this->config = Configuration::loadFromArray(
             [
@@ -157,7 +153,7 @@ public function testMetadataAccess(bool $authenticated, bool $protected): void
             'GET',
         );
 
-        $c = new Controller\Metadata($config, $this->session);
+        $c = new Controller\Metadata($config);
         $c->setMetadataStorageHandler($this->mdh);
 
         if ($authenticated === true) {
@@ -206,7 +202,7 @@ public function testDisabledSAML20IDPReturnsNoAccess(): void
             'GET',
         );
 
-        $c = new Controller\Metadata($config, $this->session);
+        $c = new Controller\Metadata($config);
 
         $this->expectException(\SimpleSAML\Error\Error::class);
         $this->expectExceptionMessage('NOACCESS');
@@ -223,7 +219,7 @@ public function testMetadataUnknownEntityThrowsError(): void
             'GET',
         );
 
-        $c = new Controller\Metadata($this->config, $this->session);
+        $c = new Controller\Metadata($this->config);
         $c->setMetadataStorageHandler($this->mdh);
 
         $this->expectException(\SimpleSAML\Error\Error::class);
@@ -241,7 +237,7 @@ public function testMetadataYieldsContent(): void
             'GET',
         );
 
-        $c = new Controller\Metadata($this->config, $this->session);
+        $c = new Controller\Metadata($this->config);
         $c->setMetadataStorageHandler($this->mdh);
 
         $result = $c->metadata($request);
@@ -266,7 +262,7 @@ public function testMetadataDefaultIdPYieldsContent(): void
             'GET',
         );
 
-        $c = new Controller\Metadata($this->config, $this->session);
+        $c = new Controller\Metadata($this->config);
         $c->setMetadataStorageHandler($this->mdh);
 
         $result = $c->metadata($request);
@@ -291,7 +287,7 @@ public function testMetadataCachingHeaders(): void
             'GET',
         );
 
-        $c = new Controller\Metadata($this->config, $this->session);
+        $c = new Controller\Metadata($this->config);
         $c->setMetadataStorageHandler($this->mdh);
 
         $result = $c->metadata($request);
diff --git a/tests/modules/saml/src/IdP/SAML2Test.php b/tests/modules/saml/src/IdP/SAML2Test.php
@@ -270,7 +270,6 @@ public function testIdPGetHostedMetadataMinimal(): void
         $md = [];
         $hostedMd = $this->idpMetadataHandlerHelper($md);
 
-        $this->assertIsArray($hostedMd);
         $this->assertArrayHasKey('metadata-set', $hostedMd);
         $this->assertEquals('saml20-idp-hosted', $hostedMd['metadata-set']);
         $this->assertArrayHasKey('entityid', $hostedMd);
diff --git a/tests/src/SimpleSAML/Metadata/MetaDataStorageHandlerTest.php b/tests/src/SimpleSAML/Metadata/MetaDataStorageHandlerTest.php
@@ -139,7 +139,8 @@ public function testGetMetadataConfigForSha1NotFoundReturnsNull(): void
      */
     public function testGetMetadataCurrentEmptySet(): void
     {
-        $this->expectException(Exception::class, 'Could not find any default metadata');
+        $this->expectException(Exception::class);
+        $this->expectExceptionMessage('Could not find any default metadata');
         $this->handler->getMetaDataCurrent('saml20-idp-remote');
     }
 
@@ -148,7 +149,8 @@ public function testGetMetadataCurrentEmptySet(): void
      */
     public function testGetMetaDataNonExistentEntity(): void
     {
-        $this->expectException(MetadataNotFound::class, "METADATANOTFOUND('%ENTITYID%' => 'doesnotexist')");
+        $this->expectException(MetadataNotFound::class);
+        $this->expectExceptionMessage("METADATANOTFOUND('%ENTITYID%' => 'doesnotexist')");
         $this->handler->getMetaData('doesnotexist', 'saml20-sp-remote');
     }
 

Original file line number	Diff line number	Diff line change
`@@ -39,7 +39,7 @@ public function __construct(`
`39`	`39`	`/**`
`40`	`40`	`* Built-in IdP discovery service`
`41`	`41`	`*`
`42`		`- * @return \SimpleSAML\Http\RunnableResponse`
	`42`	`+ * @return \SimpleSAML\HTTP\RunnableResponse`
`43`	`43`	`*/`
`44`	`44`	`public function disco(): RunnableResponse`
`45`	`45`	`{`
Original file line number	Diff line number	Diff line change
`@@ -109,7 +109,7 @@ public function setAuthUtils(Utils\Auth $authUtils): void`
`109`	`109`	`* Handler for response from IdP discovery service.`
`110`	`110`	`*`
`111`	`111`	`* @param \Symfony\Component\HttpFoundation\Request $request`
`112`		`- * @return \SimpleSAML\Http\RunnableResponse`
	`112`	`+ * @return \SimpleSAML\HTTP\RunnableResponse`
`113`	`113`	`*/`
`114`	`114`	`public function discoResponse(Request $request): RunnableResponse`
`115`	`115`	`{`
`@@ -155,7 +155,7 @@ public function wrongAuthnContextClassRef(): Template`
`155`	`155`	`* Handler for the Assertion Consumer Service.`
`156`	`156`	`*`
`157`	`157`	`* @param string $sourceId`
`158`		`- * @return \SimpleSAML\Http\RunnableResponse`
	`158`	`+ * @return \SimpleSAML\HTTP\RunnableResponse`
`159`	`159`	`*/`
`160`	`160`	`public function assertionConsumerService(string $sourceId): RunnableResponse`
`161`	`161`	`{`
`@@ -423,7 +423,7 @@ public function assertionConsumerService(string $sourceId): RunnableResponse`
`423`	`423`	`* This endpoint handles both logout requests and logout responses.`
`424`	`424`	`*`
`425`	`425`	`* @param string $sourceId`
`426`		`- * @return \SimpleSAML\Http\RunnableResponse`
	`426`	`+ * @return \SimpleSAML\HTTP\RunnableResponse`
`427`	`427`	`*/`
`428`	`428`	`public function singleLogoutService(string $sourceId): RunnableResponse`
`429`	`429`	`{`
Original file line number	Diff line number	Diff line change
`@@ -4,6 +4,7 @@`
`4`	`4`
`5`	`5`	`namespace SimpleSAML;`
`6`	`6`
	`7`	`+use DOMNodeList;`
`7`	`8`	`use SAML2\XML\saml\AttributeValue;`
`8`	`9`	`use SimpleSAML\Assert\Assert;`
`9`	`10`	`use SimpleSAML\Error;`
`@@ -621,7 +622,7 @@ public function doLogin(string $authority, array $data = []): void`
`621`	`622`	`}`
`622`	`623`
`623`	`624`	`// at this point, this should be a DOMNodeList object...`
`624`		`- if (!is_a($value, 'DOMNodeList')) {`
	`625`	`+ if (!is_a($value, DOMNodeList::class)) {`
`625`	`626`	`continue;`
`626`	`627`	`}`
`627`	`628`
`@@ -894,7 +895,7 @@ public function setData(string $type, string $id, $data, $timeout = null): void`
`894`	`895`	`if ($timeout === self::DATA_TIMEOUT_SESSION_END) {`
`895`	`896`	`$expires = self::DATA_TIMEOUT_SESSION_END;`
`896`	`897`	`} else {`
`897`		`- $expires = time() + $timeout;`
	`898`	`+ $expires = time() + intval($timeout);`
`898`	`899`	`}`
`899`	`900`
`900`	`901`	`$dataInfo = [`