Merge pull request #280 from jenkoian/check-nonce-isset

Check nonce isset

Author: azmeuk

CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Changed
 
 * signOut() Method parameter $accessToken -> $idToken to prevent confusion about access and id tokens usage. #127
+* Fixed issue where missing nonce within the claims was causing an exception. #280
 
 ## [0.9.4]
 

phpunit.xml.dist

@@ -0,0 +1,28 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<phpunit
+        bootstrap="./vendor/autoload.php"
+        colors="true"
+        convertErrorsToExceptions="true"
+        convertNoticesToExceptions="true"
+        convertWarningsToExceptions="true"
+        verbose="true"
+        stopOnFailure="false"
+        processIsolation="false"
+        backupGlobals="false"
+        syntaxCheck="true"
+>
+    <testsuites>
+        <testsuite name="Tests">
+            <directory>./tests</directory>
+        </testsuite>
+    </testsuites>
+    <filter>
+        <whitelist addUncoveredFilesFromWhitelist="true">
+            <directory suffix=".php">./src</directory>
+            <exclude>
+                <directory>./vendor</directory>
+                <directory>./tests</directory>
+            </exclude>
+        </whitelist>
+    </filter>
+</phpunit>

src/OpenIDConnectClient.php

@@ -1026,7 +1026,7 @@ protected function verifyJWTclaims($claims, $accessToken = null) {
         }
         return (($this->issuerValidator->__invoke($claims->iss))
             && (($claims->aud === $this->clientID) || in_array($this->clientID, $claims->aud, true))
-            && ($claims->nonce === $this->getNonce())
+            && (!isset($claims->nonce) || $claims->nonce === $this->getNonce())
             && ( !isset($claims->exp) || ((gettype($claims->exp) === 'integer') && ($claims->exp >= time() - $this->leeway)))
             && ( !isset($claims->nbf) || ((gettype($claims->nbf) === 'integer') && ($claims->nbf <= time() + $this->leeway)))
             && ( !isset($claims->at_hash) || !isset($accessToken) || $claims->at_hash === $expected_at_hash )

tests/OpenIDConnectClientTest.php

@@ -1,6 +1,7 @@
 <?php
 
 use Jumbojett\OpenIDConnectClient;
+use Jumbojett\OpenIDConnectClientException;
 
 class OpenIDConnectClientTest extends PHPUnit_Framework_TestCase
 {
@@ -17,4 +18,39 @@ public function testGetRedirectURL()
         $_SERVER['REQUEST_URI'] = '/path/index.php?foo=bar&baz#fragment';
         self::assertSame('http://domain.test/path/index.php', $client->getRedirectURL());
     }
+
+    public function testAuthenticateDoesNotThrowExceptionIfClaimsIsMissingNonce()
+    {
+        $fakeClaims = new \StdClass();
+        $fakeClaims->iss = 'fake-issuer';
+        $fakeClaims->aud = 'fake-client-id';
+        $fakeClaims->nonce = null;
+
+        $_REQUEST['id_token'] = 'abc.123.xyz';
+        $_REQUEST['state'] = false;
+        $_SESSION['openid_connect_state'] = false;
+
+        /** @var OpenIDConnectClient | PHPUnit_Framework_MockObject_MockObject $client */
+        $client = $this->getMockBuilder(OpenIDConnectClient::class)->setMethods(['decodeJWT', 'getProviderConfigValue', 'verifyJWTsignature'])->getMock();
+        $client->method('decodeJWT')->willReturn($fakeClaims);
+        $client->method('getProviderConfigValue')->with('jwks_uri')->willReturn(true);
+        $client->method('verifyJWTsignature')->willReturn(true);
+
+        $client->setClientID('fake-client-id');
+        $client->setIssuer('fake-issuer');
+        $client->setIssuerValidator(function() {
+            return true;
+        });
+        $client->setAllowImplicitFlow(true);
+        $client->setProviderURL('https://jwt.io/');
+
+        try {
+            $authenticated = $client->authenticate();
+            $this->assertTrue($authenticated);
+        } catch ( OpenIDConnectClientException $e ) {
+            if ( $e->getMessage() === 'Unable to verify JWT claims' ) {
+                self::fail( 'OpenIDConnectClientException was thrown when it should not have been.' );
+            }
+        }
+    }
 }