simplesamlphp · tvdijen · Mar 9, 2019 · Mar 9, 2019 · Mar 9, 2019 · Mar 9, 2019
diff --git a/.php_cd.dist b/.php_cd.dist
@@ -0,0 +1,19 @@
+<?php
+$finder = PhpCsFixer\Finder::create()
+    ->in([
+        __DIR__ . '/config-templates',
+        __DIR__ . '/lib',
+        __DIR__ . '/hooks',
+        __DIR__ . '/tests',
+        __DIR__ . '/www',
+    ])
+;
+return PhpCsFixer\Config::create()
+    ->setRules([
+        '@PSR2' => true,
+        '@PSR4' => true,
+        '@PSR5' => true,
+    ])
+    ->setFinder($finder)
+;
+
diff --git a/.travis.yml b/.travis.yml
@@ -11,7 +11,7 @@ php:
   - 7.3
 
 env:
-  - SIMPLESAMLPHP_VERSION=1.16.*
+  - SIMPLESAMLPHP_VERSION=1.17.*
 
 matrix:
   include:
@@ -28,15 +28,16 @@ matrix:
   allow_failures:
     - env: SIMPLESAMLPHP_VERSION=dev-master
     - php: 7.3
-    - php: hhvm
 
 before_script:
   - composer require "simplesamlphp/simplesamlphp:${SIMPLESAMLPHP_VERSION}" --no-update
   - composer update --no-interaction
+  - if [[ "$TRAVIS_PHP_VERSION" == "7.3" ]]; then composer require --dev vimeo/psalm; fi
 
 script:
   - bin/check-syntax.sh
   - if [[ "$TRAVIS_PHP_VERSION" == "5.6" ]]; then php vendor/phpunit/phpunit/phpunit; else php vendor/phpunit/phpunit/phpunit --no-coverage; fi
+  - if [[ "$TRAVIS_PHP_VERSION" == "7.3" ]]; then vendor/bin/psalm; fi
 
 after_success:
   # Codecov, need to edit bash uploader for incorrect TRAVIS_PYTHON_VERSION environment variable matching, at least until codecov/codecov-bash#133 is resolved

diff --git a/bin/check-syntax.sh b/bin/check-syntax.sh
@@ -4,7 +4,7 @@ PHP='/usr/bin/env php'
 RETURN=0
 
 # check PHP files
-for FILE in `find config-templates hooks lib templates www -name "*.php"`; do
+for FILE in `find config-templates hooks lib www -name "*.php"`; do
     $PHP -l $FILE > /dev/null 2>&1
     if [ $? -ne 0 ]; then
         echo "Syntax check failed for ${FILE}"

diff --git a/composer.json b/composer.json
@@ -16,11 +16,11 @@
         }
     ],
     "require": {
-        "simplesamlphp/composer-module-installer": "~1.0"
+        "simplesamlphp/composer-module-installer": "~1.1"
     },
     "require-dev": {
-        "simplesamlphp/simplesamlphp": "^1.16",
-        "phpunit/phpunit": "~4.8.35"
+        "simplesamlphp/simplesamlphp": "^1.17",
+        "phpunit/phpunit": "~4.8.36"
     }
 }
 
diff --git a/hooks/hook_cron.php b/hooks/hook_cron.php
@@ -4,6 +4,7 @@
  * cron hook to update aggregator2 metadata.
  *
  * @param array &$croninfo  Output
+ * @return void
  */
 function aggregator2_hook_cron(&$croninfo)
 {

diff --git a/hooks/hook_frontpage.php b/hooks/hook_frontpage.php
@@ -4,6 +4,7 @@
  * Hook to add the aggregator2 lik to the frontpage.
  *
  * @param array &$links The links on the frontpage, split into sections.
+ * @return void
  */
 function aggregator2_hook_frontpage(&$links)
 {

diff --git a/lib/Aggregator.php b/lib/Aggregator.php
@@ -84,7 +84,7 @@ class Aggregator
     /**
      * Duration we should cache generated metadata.
      *
-     * @var int
+     * @var int|null
      */
     protected $cacheGenerated;
 
@@ -160,7 +160,7 @@ class Aggregator
      *
      * @var string
      */
-    protected $cacheId;
+    protected $cacheId = 'dummy';
 
     /**
      * The cache tag for our generated metadata.
@@ -170,7 +170,7 @@ class Aggregator
      *
      * @var string
      */
-    protected $cacheTag;
+    protected $cacheTag = 'dummy';
 
     /**
      * The registration information for our generated metadata.
@@ -220,21 +220,23 @@ protected function __construct($id, Configuration $config)
         $signKey = $config->getString('sign.privatekey', null);
         if ($signKey !== null) {
             $signKey = System::resolvePath($signKey, $certDir);
-            $this->signKey = @file_get_contents($signKey);
-            if ($this->signKey === null) {
+            $sk = @file_get_contents($signKey);
+            if ($sk === false) {
                 throw new Exception('Unable to load private key from '.var_export($signKey, true));
             }
+            $this->signKey = $sk;
         }
 
         $this->signKeyPass = $config->getString('sign.privatekey_pass', null);
 
         $signCert = $config->getString('sign.certificate', null);
         if ($signCert !== null) {
             $signCert = System::resolvePath($signCert, $certDir);
-            $this->signCert = @file_get_contents($signCert);
-            if ($this->signCert === null) {
+            $sc = @file_get_contents($signCert);
+            if ($sc === false) {
                 throw new Exception('Unable to load certificate file from '.var_export($signCert, true));
             }
+            $this->signCert = $sc;
         }
 
         $this->signAlg = $config->getString('sign.algorithm', XMLSecurityKey::RSA_SHA1);
@@ -244,7 +246,7 @@ protected function __construct($id, Configuration $config)
 
         $this->sslCAFile = $config->getString('ssl.cafile', null);
 
-        $this->regInfo = $config->getArray('RegistrationInfo', null);
+        $this->regInfo = $config->getArray('RegistrationInfo', []);
 
         $this->initSources($config->getConfigList('sources'));
     }
@@ -256,6 +258,7 @@ protected function __construct($id, Configuration $config)
      * This is called from the constructor, and can be overridden in subclasses.
      *
      * @param array $sources  The sources as an array of SimpleSAML_Configuration objects.
+     * @return void
      */
     protected function initSources(array $sources)
     {
@@ -269,6 +272,7 @@ protected function initSources(array $sources)
      * Return an instance of the aggregator with the given id.
      *
      * @param string $id  The id of the aggregator.
+     * @return Aggregator
      */
     public static function getAggregator($id)
     {
@@ -297,6 +301,7 @@ public function getId()
      * @param string $data  The data.
      * @param int $expires  The timestamp the data expires.
      * @param string|null $tag  An extra tag that can be used to verify the validity of the cached data.
+     * @return void
      */
     public function addCacheItem($id, $data, $expires, $tag = null)
     {
@@ -305,7 +310,7 @@ public function addCacheItem($id, $data, $expires, $tag = null)
         assert('is_int($expires)');
         assert('is_null($tag) || is_string($tag)');
 
-        $cacheFile = $this->cacheDirectory.'/'.$id;
+        $cacheFile = strval($this->cacheDirectory).'/'.$id;
         try {
             System::writeFile($cacheFile, $data);
         } catch (\Exception $e) {
@@ -339,7 +344,7 @@ public function isCacheValid($id, $tag = null)
         assert('is_string($id)');
         assert('is_null($tag) || is_string($tag)');
 
-        $cacheFile = $this->cacheDirectory.'/'.$id;
+        $cacheFile = strval($this->cacheDirectory).'/'.$id;
         if (!file_exists($cacheFile)) {
             return false;
         }
@@ -390,7 +395,7 @@ public function getCacheItem($id, $tag = null)
             return null;
         }
 
-        $cacheFile = $this->cacheDirectory.'/'.$id;
+        $cacheFile = strval($this->cacheDirectory).'/'.$id;
         return @file_get_contents($cacheFile);
     }
 
@@ -405,7 +410,7 @@ public function getCacheFile($id)
     {
         assert('is_string($id)');
 
-        $cacheFile = $this->cacheDirectory.'/'.$id;
+        $cacheFile = strval($this->cacheDirectory).'/'.$id;
         if (!file_exists($cacheFile)) {
             return null;
         }
@@ -427,13 +432,15 @@ public function getCAFile()
 
     /**
      * Sign the generated EntitiesDescriptor.
+     * @return void
      */
     protected function addSignature(SignedElement $element)
     {
         if ($this->signKey === null) {
             return;
         }
 
+        /** @var string $this->signAlg */
         $privateKey = new XMLSecurityKey($this->signAlg, ['type' => 'private']);
         if ($this->signKeyPass !== null) {
             $privateKey->passphrase = $this->signKeyPass;
@@ -456,14 +463,8 @@ protected function addSignature(SignedElement $element)
      *
      * @return array An array containing all the EntityDescriptors found.
      */
-    private static function extractEntityDescriptors($entity)
+    private static function extractEntityDescriptors(EntitiesDescriptor $entity)
     {
-        assert('$entity instanceof EntitiesDescriptor');
-
-        if (!($entity instanceof EntitiesDescriptor)) {
-            return [];
-        }
-
         $results = [];
         foreach ($entity->children as $child) {
             if ($child instanceof EntityDescriptor) {
@@ -488,7 +489,7 @@ protected function getEntitiesDescriptor()
         $now = time();
 
         // add RegistrationInfo extension if enabled
-        if ($this->regInfo !== null) {
+        if (!empty($this->regInfo)) {
             $ri = new RegistrationInfo();
             $ri->registrationInstant = $now;
             foreach ($this->regInfo as $riName => $riValues) {
@@ -607,6 +608,7 @@ protected function filter(EntitiesDescriptor $descriptor)
      * Set this aggregator to exclude a set of entities from the resulting aggregate.
      *
      * @param array|null $entities The entity IDs of the entities to exclude.
+     * @return void
      */
     public function excludeEntities($entities)
     {
@@ -634,6 +636,7 @@ public function excludeEntities($entities)
      * - 'shib13-aa': all SHIB1.3-capable attribute authorities.
      *
      * @param array|null $set An array of the different roles and protocols to filter by.
+     * @return void
      */
     public function setFilters($set)
     {
@@ -728,6 +731,7 @@ public function getMetadata()
 
     /**
      * Update the cached copy of our metadata.
+     * @return void
      */
     public function updateCache()
     {

diff --git a/lib/EntitySource.php b/lib/EntitySource.php
@@ -61,7 +61,7 @@ class EntitySource
      *
      * @var \SAML2\XML\md\EntitiesDescriptor|\SAML2\XML\md\EntityDescriptor|null
      */
-    protected $metadata;
+    protected $metadata = null;
 
     /**
      * The cache ID.
@@ -82,7 +82,7 @@ class EntitySource
      *
      * @var bool
      */
-    protected $updateAttempted;
+    protected $updateAttempted = false;
 
 
     /**
@@ -127,13 +127,15 @@ private function downloadMetadata()
             $context['ssl']['CN_match'] = parse_url($this->url, PHP_URL_HOST);
         }
 
-        $data = HTTP::fetch($this->url, $context);
-        if ($data === false || $data === null) {
+        try {
+            $data = HTTP::fetch($this->url, $context, false);
+        } catch (\SimpleSAML\Error\Exception $e) {
             Logger::error($this->logLoc.'Unable to load metadata from '.var_export($this->url, true));
             return null;
         }
 
         $doc = new \DOMDocument();
+        /** @var string $data */
         $res = $doc->loadXML($data);
         if (!$res) {
             Logger::error($this->logLoc.'Error parsing XML from '.var_export($this->url, true));
@@ -190,6 +192,7 @@ private function downloadMetadata()
 
     /**
      * Attempt to update our cache file.
+     * @return void
      */
     public function updateCache()
     {
@@ -231,6 +234,7 @@ public function getMetadata()
 
         if (!$this->aggregator->isCacheValid($this->cacheId, $this->cacheTag)) {
             $this->updateCache();
+            /** @psalm-suppress TypeDoesNotContainType */
             if ($this->metadata !== null) {
                 return $this->metadata;
             }
@@ -239,15 +243,15 @@ public function getMetadata()
 
         $cacheFile = $this->aggregator->getCacheFile($this->cacheId);
 
-        if (!file_exists($cacheFile)) {
+        if (is_null($cacheFile) || !file_exists($cacheFile)) {
             Logger::error($this->logLoc . 'No cached metadata available.');
             return null;
         }
 
         Logger::debug($this->logLoc.'Using cached metadata from '.var_export($cacheFile, true));
 
         $metadata = file_get_contents($cacheFile);
-        if ($metadata !== null) {
+        if ($metadata !== false) {
             $this->metadata = unserialize($metadata);
             return $this->metadata;
         }

diff --git a/psalm.xml b/psalm.xml
@@ -0,0 +1,33 @@
+<?xml version="1.0"?>
+<psalm
+    name="SimpleSAMLphp Module Aggregator2"
+    useDocblockTypes="true"
+    totallyTyped="false"
+>
+    <projectFiles>
+        <directory name="lib" />
+        <directory name="hooks" />
+        <directory name="www" />
+    </projectFiles>
+
+    <issueHandlers>
+        <LessSpecificReturnType errorLevel="info" />
+
+        <!-- level 3 issues - slightly lazy code writing, but probably low false-negatives -->
+        <DeprecatedMethod errorLevel="info" />
+
+        <MissingClosureReturnType errorLevel="info" />
+        <MissingReturnType errorLevel="info" />
+        <MissingPropertyType errorLevel="info" />
+        <InvalidDocblock errorLevel="info" />
+        <MisplacedRequiredParam errorLevel="info" />
+
+        <PropertyNotSetInConstructor errorLevel="info" />
+        <MissingConstructor errorLevel="info" />
+        <MissingClosureParamType errorLevel="info" />
+        <MissingParamType errorLevel="info" />
+        <UnusedClass errorLevel="info" />
+        <PossiblyUnusedMethod errorLevel="info" />
+    </issueHandlers>
+</psalm>
+