Merge pull request #230 from glensc/codereview

codereview fixes
SAML-Toolkits · Sep 12, 2017 · 09a38ce · 09a38ce
2 parents 30d7251 + 0d6df77
commit 09a38ce
Show file tree

Hide file tree

Showing 14 changed files with 55 additions and 66 deletions.
diff --git a/README.md b/README.md
@@ -726,7 +726,7 @@ unset($_SESSION['AuthNRequestID']);
 $errors = $auth->getErrors();
 
 if (!empty($errors)) {
-    print_r('<p>'.implode(', ', $errors).'</p>');
+    echo '<p>', implode(', ', $errors), '</p>';
     exit();
 }
 
@@ -865,9 +865,9 @@ $auth->processSLO(false, $requestID);
 $errors = $auth->getErrors();
 
 if (empty($errors)) {
-    print_r('Sucessfully logged out');
+    echo 'Sucessfully logged out';
 } else {
-    print_r(implode(', ', $errors));
+    echo implode(', ', $errors);
 }
 ```
 
@@ -1009,7 +1009,7 @@ if (isset($_SESSION['samlNameIdFormat'])) {
     $nameIdFormat = $_SESSION['samlNameIdFormat'];
 }
 $auth->logout($returnTo, $paramters, $nameId, $sessionIndex, false, $nameIdFormat);
-``` 
+```
 
 If a match on the future LogoutResponse ID and the LogoutRequest ID to be sent is required, that LogoutRequest ID must to be extracted and stored.
 
@@ -1037,8 +1037,7 @@ session_start();    // Initialize the session, we do that because
                     // Note that processResponse and processSLO
                     // methods could manipulate/close that session
 
-require_once dirname(dirname(__FILE__)).'/_toolkit_loader.php'; // Load Saml2 and
-                                                                // external libs
+require_once dirname(__DIR__).'/_toolkit_loader.php'; // Load Saml2 and external libs
 require_once 'settings.php';    // Load the setting info as an Array
 
 $auth = new OneLogin_Saml2_Auth($settingsInfo);  // Initialize the SP SAML instance
@@ -1059,7 +1058,7 @@ if (isset($_GET['sso'])) {    // SSO action.  Will send an AuthNRequest to the I
                                    // that could took place during the process
 
     if (!empty($errors)) {
-        print_r('<p>'.implode(', ', $errors).'</p>');
+        echo '<p>', implode(', ', $errors), '</p>';
     }
                                           // This check if the response was
     if (!$auth->isAuthenticated()) {      // sucessfully validated and the user
@@ -1075,9 +1074,9 @@ if (isset($_GET['sso'])) {    // SSO action.  Will send an AuthNRequest to the I
     $auth->processSLO();            // Process the Logout Request & Logout Response
     $errors = $auth->getErrors(); // Retrieves possible validation errors
     if (empty($errors)) {
-        print_r('<p>Sucessfully logged out</p>');
+        echo '<p>Sucessfully logged out</p>';
     } else {
-        print_r('<p>'.implode(', ', $errors).'</p>');
+        echo '<p>', implode(', ', $errors), '</p>';
     }
 }
 

diff --git a/_toolkit_loader.php b/_toolkit_loader.php
@@ -1,15 +1,15 @@
 <?php
 
-// Create an __autoload function 
+// Create an __autoload function
 // (can conflicts other autoloaders)
 // http://php.net/manual/en/language.oop5.autoload.php
 
-$libDir = dirname(__FILE__) . '/lib/Saml2/';
-$extlibDir = dirname(__FILE__) . '/extlib/';
+$libDir = __DIR__ . '/lib/Saml2/';
+$extlibDir = __DIR__ . '/extlib/';
 
 // Load composer
-if (file_exists('vendor/autoload.php')) {
-    require 'vendor/autoload.php';
+if (file_exists(__DIR__ .'/vendor/autoload.php')) {
+    require __DIR__ . '/vendor/autoload.php';
 }
 
 // Load now external libs
@@ -20,6 +20,6 @@
 foreach ($folderInfo as $element) {
     if (is_file($libDir.$element) && (substr($element, -4) === '.php')) {
         include_once $libDir.$element;
+        break;
     }
 }
-
diff --git a/composer.json b/composer.json
@@ -2,8 +2,7 @@
     "name": "onelogin/php-saml",
     "description": "OneLogin PHP SAML Toolkit",
     "license": "MIT",
-    "version": "2.11.0",
-    "homepage": "https://onelogin.zendesk.com/hc/en-us/sections/200245634-SAML-Toolkits",
+    "homepage": "https://developers.onelogin.com/saml/php",
     "keywords": ["saml", "saml2", "onelogin"],
     "autoload": {
         "classmap": [
@@ -19,6 +18,7 @@
     },
     "require": {
         "php": ">=5.3.2",
+        "ext-curl": "*",
         "ext-openssl": "*",
         "ext-dom": "*",
         "ext-mcrypt": "*"
@@ -32,8 +32,6 @@
         "squizlabs/php_codesniffer": "2.9.0"
     },
     "suggest": {
-        "lib-openssl": "Install openssl lib in order to handle with x509 certs (require to support sign and encryption)",
-        "ext-mcrypt": "Install mcrypt and php5-mcrypt libs in order to support encryption",
         "ext-gettext": "Install gettext and php5-gettext libs to handle translations"
     }
 }
diff --git a/demo1/index.php b/demo1/index.php
@@ -1,12 +1,11 @@
 <?php
-
 /**
  *  SAML Handler
  */
 
 session_start();
 
-require_once dirname(dirname(__FILE__)).'/_toolkit_loader.php';
+require_once dirname(__DIR__).'/_toolkit_loader.php';
 
 require_once 'settings.php';
 
@@ -65,7 +64,7 @@
     $errors = $auth->getErrors();
 
     if (!empty($errors)) {
-        print_r('<p>'.implode(', ', $errors).'</p>');
+        echo '<p>',implode(', ', $errors),'</p>';
     }
 
     if (!$auth->isAuthenticated()) {
@@ -91,9 +90,9 @@
     $auth->processSLO(false, $requestID);
     $errors = $auth->getErrors();
     if (empty($errors)) {
-        print_r('<p>Sucessfully logged out</p>');
+        echo '<p>Sucessfully logged out</p>';
     } else {
-        print_r('<p>'.implode(', ', $errors).'</p>');
+        echo '<p>', implode(', ', $errors), '</p>');
     }
 }
 

diff --git a/demo1/metadata.php b/demo1/metadata.php
@@ -4,7 +4,7 @@
  *  SAML Metadata view
  */
 
-require_once dirname(dirname(__FILE__)).'/_toolkit_loader.php';
+require_once dirname(__DIR__).'/_toolkit_loader.php';
 
 require_once 'settings.php' ;
 

diff --git a/endpoints/acs.php b/endpoints/acs.php
@@ -1,12 +1,12 @@
 <?php
- 
+
 /**
  *  SP Assertion Consumer Service Endpoint
  */
 
 session_start();
 
-require_once dirname(dirname(__FILE__)).'/_toolkit_loader.php';
+require_once dirname(__DIR__).'/_toolkit_loader.php';
 
 $auth = new OneLogin_Saml2_Auth();
 
@@ -15,7 +15,7 @@
 $errors = $auth->getErrors();
 
 if (!empty($errors)) {
-    print_r('<p>'.implode(', ', $errors).'</p>');
+    echo '<p>', implode(', ', $errors), '</p>';
     exit();
 }
 

diff --git a/endpoints/metadata.php b/endpoints/metadata.php
@@ -4,7 +4,7 @@
  *  SP Metadata Endpoint
  */
 
-require_once dirname(dirname(__FILE__)).'/_toolkit_loader.php';
+require_once dirname(__DIR__).'/_toolkit_loader.php';
 
 try {
     $auth = new OneLogin_Saml2_Auth();

diff --git a/endpoints/sls.php b/endpoints/sls.php
@@ -1,12 +1,11 @@
 <?php
-
 /**
  *  SP Single Logout Service Endpoint
  */
 
 session_start();
 
-require_once dirname(dirname(__FILE__)).'/_toolkit_loader.php';
+require_once dirname(__DIR__).'/_toolkit_loader.php';
 
 $auth = new OneLogin_Saml2_Auth();
 
@@ -15,7 +14,7 @@
 $errors = $auth->getErrors();
 
 if (empty($errors)) {
-    print_r('Sucessfully logged out');
+    echo 'Sucessfully logged out';
 } else {
-    print_r(implode(', ', $errors));
+    echo implode(', ', $errors);
 }
diff --git a/lib/Saml/AuthRequest.php b/lib/Saml/AuthRequest.php
@@ -56,10 +56,8 @@ protected function _generateUniqueID()
      */
     protected function _getTimestamp()
     {
-        $defaultTimezone = date_default_timezone_get();
-        date_default_timezone_set('UTC');
-        $timestamp = strftime("%Y-%m-%dT%H:%M:%SZ");
-        date_default_timezone_set($defaultTimezone);
+        $date = new DateTime('now', new DateTimeZone('UTC'));
+        $timestamp = $date->format("Y-m-d\TH:i:s\Z");
         return $timestamp;
     }
 }
diff --git a/lib/Saml/Metadata.php b/lib/Saml/Metadata.php
@@ -30,10 +30,9 @@ public function getXml()
      */
     protected function _getMetadataValidTimestamp()
     {
-        $timeZone = date_default_timezone_get();
-        date_default_timezone_set('UTC');
-        $time = strftime("%Y-%m-%dT%H:%M:%SZ", time() + self::VALIDITY_SECONDS);
-        date_default_timezone_set($timeZone);
+        $timestamp = time() + self::VALIDITY_SECONDS;
+        $date = new DateTime("@$timestamp", new DateTimeZone('UTC'));
+        $time = $date->format("Y-m-d\TH:i:s\Z");
         return $time;
     }
 }
diff --git a/lib/Saml2/Settings.php b/lib/Saml2/Settings.php
@@ -152,7 +152,7 @@ public function __construct($settings = null, $spValidationOnly = false)
      */
     private function _loadPaths()
     {
-        $basePath = dirname(dirname(dirname(__FILE__))).'/';
+        $basePath = dirname(dirname(__DIR__)).'/';
         $this->_paths = array (
             'base' => $basePath,
             'config' => $basePath,

diff --git a/lib/Saml2/Utils.php b/lib/Saml2/Utils.php
@@ -1,5 +1,5 @@
 <?php
- 
+
 /**
  * Utils of OneLogin PHP Toolkit
  *
@@ -50,7 +50,7 @@ public static function t($msg, $args = array())
     {
         assert('is_string($msg)');
         if (extension_loaded('gettext')) {
-            bindtextdomain("phptoolkit", dirname(dirname(dirname(__FILE__))).'/locale');
+            bindtextdomain("phptoolkit", dirname(dirname(__DIR__)).'/locale');
             textdomain('phptoolkit');
 
             $translatedMsg = gettext($msg);
@@ -124,7 +124,7 @@ public static function validateXML($xml, $schema, $debug = false)
             }
         }
 
-        $schemaFile = dirname(__FILE__).'/schemas/' . $schema;
+        $schemaFile = __DIR__.'/schemas/' . $schema;
         $oldEntityLoader = libxml_disable_entity_loader(false);
         $res = $dom->schemaValidate($schemaFile);
         libxml_disable_entity_loader($oldEntityLoader);
@@ -253,14 +253,13 @@ public static function redirect($url, $parameters = array(), $stay = false)
         }
 
         /* Verify that the URL is to a http or https site. */
-        if (!preg_match('@^https?:\/\/@i', $url)) {
+        if (!preg_match('@^https?://@i', $url)) {
             throw new OneLogin_Saml2_Error(
                 'Redirect to invalid URL: ' . $url,
                 OneLogin_Saml2_Error::REDIRECT_INVALID_URL
             );
         }
 
-
         /* Add encoded parameters */
         if (strpos($url, '?') === false) {
             $paramPrefix = '?';
@@ -306,7 +305,7 @@ public static function setBaseURL($baseurl)
     {
         if (!empty($baseurl)) {
             $baseurlpath = '/';
-            if (preg_match('#^https?:\/\/([^\/]*)\/?(.*)#i', $baseurl, $matches)) {
+            if (preg_match('#^https?://([^/]*)/?(.*)#i', $baseurl, $matches)) {
                 if (strpos($baseurl, 'https://') === false) {
                     self::setSelfProtocol('http');
                     $port = '80';
@@ -587,7 +586,7 @@ public static function getSelfURL()
         if (!empty($_SERVER['REQUEST_URI'])) {
             $requestURI = $_SERVER['REQUEST_URI'];
             if ($requestURI[0] !== '/') {
-                if (preg_match('#^https?:\/\/[^\/]*(\/.*)#i', $requestURI, $matches)) {
+                if (preg_match('#^https?://[^/]*(/.*)#i', $requestURI, $matches)) {
                     $requestURI = $matches[1];
                 }
             }
@@ -658,10 +657,8 @@ public static function generateUniqueID()
      */
     public static function parseTime2SAML($time)
     {
-        $defaultTimezone = date_default_timezone_get();
-        date_default_timezone_set('UTC');
-        $timestamp = strftime("%Y-%m-%dT%H:%M:%SZ", $time);
-        date_default_timezone_set($defaultTimezone);
+        $date = new DateTime("@$time", new DateTimeZone('UTC'));
+        $timestamp = $date->format("Y-m-d\TH:i:s\Z");
         return $timestamp;
     }
 
@@ -690,15 +687,15 @@ public static function parseSAML2Time($time)
         }
 
         /* Extract the different components of the time from the
-         * matches in the regex. intval will ignore leading zeroes
+         * matches in the regex. int cast will ignore leading zeroes
          * in the string.
          */
-        $year = intval($matches[1]);
-        $month = intval($matches[2]);
-        $day = intval($matches[3]);
-        $hour = intval($matches[4]);
-        $minute = intval($matches[5]);
-        $second = intval($matches[6]);
+        $year = (int)$matches[1];
+        $month = (int)$matches[2];
+        $day = (int)$matches[3];
+        $hour = (int)$matches[4];
+        $minute = (int)$matches[5];
+        $second = (int)$matches[6];
 
         /* We use gmmktime because the timestamp will always be given
          * in UTC.
@@ -862,7 +859,7 @@ public static function query($dom, $query, $context = null)
      */
     public static function isSessionStarted()
     {
-        if (version_compare(phpversion(), '5.4.0', '>=')) {
+        if (PHP_VERSION_ID >= 50400) {
             return session_status() === PHP_SESSION_ACTIVE ? true : false;
         } else {
             return session_id() === '' ? false : true;
@@ -1151,7 +1148,7 @@ public static function decryptElement(DOMElement $encryptedData, XMLSecurityKey
                 OneLogin_Saml2_ValidationError::INVALID_XML_FORMAT
             );
         }
- 
+
         $decryptedElement = $newDoc->firstChild->firstChild;
         if ($decryptedElement === null) {
             throw new OneLogin_Saml2_ValidationError(

diff --git a/tests/bootstrap.php b/tests/bootstrap.php
@@ -2,11 +2,11 @@
 
 ob_start();
 
-$basePath = dirname(dirname(__FILE__));
+$basePath = dirname(__DIR__);
 
 require_once $basePath.'/_toolkit_loader.php';
 
-if (!defined('TEST_ROOT')) define('TEST_ROOT', dirname(__FILE__));
+if (!defined('TEST_ROOT')) define('TEST_ROOT', __DIR__);
 
 if (!defined('XMLSECLIBS_DIR')) define('XMLSECLIBS_DIR', $basePath.'/extlib/xmlseclibs/');
 require_once XMLSECLIBS_DIR . 'xmlseclibs.php';
@@ -19,7 +19,7 @@
 require_once ONELOGIN_SAML_DIR . 'XmlSec.php';
 
 if (!defined('ONELOGIN_CUSTOMPATH')) {
-    define('ONELOGIN_CUSTOMPATH', dirname(__FILE__).'/data/customPath/');
+    define('ONELOGIN_CUSTOMPATH', __DIR__.'/data/customPath/');
 }
 
 date_default_timezone_set('America/Los_Angeles');

diff --git a/tests/src/OneLogin/Saml2/UtilsTest.php b/tests/src/OneLogin/Saml2/UtilsTest.php
@@ -697,7 +697,7 @@ public function testParseTime2SAML()
             OneLogin_Saml2_Utils::parseTime2SAML('invalidtime');
             $this->fail('Exception was not raised');
         } catch (Exception $e) {
-            $this->assertContains('strftime() expects parameter 2 to be', $e->getMessage());
+            $this->assertContains('Failed to parse time string', $e->getMessage());
         }
     }