Add ability to break cache if token kid is not found

auth0 · Jan 31, 2020 · d68140a · d68140a
1 parent c0ebd9d
commit d68140a
Show file tree

Hide file tree

Showing 10 changed files with 233 additions and 38 deletions.
diff --git a/lib/WP_Auth0_LoginManager.php b/lib/WP_Auth0_LoginManager.php
@@ -523,8 +523,7 @@ private function decode_id_token( $id_token ) {
 		$expectedIss = 'https://' . $this->a0_options->get( 'domain' ) . '/';
 		$expectedAlg = $this->a0_options->get( 'client_signing_algorithm' );
 		if ( 'RS256' === $expectedAlg ) {
-			$jwks        = ( new WP_Auth0_JwksFetcher() )->getKeys();
-			$sigVerifier = new WP_Auth0_AsymmetricVerifier( $jwks );
+			$sigVerifier = new WP_Auth0_AsymmetricVerifier( new WP_Auth0_JwksFetcher() );
 		} elseif ( 'HS256' === $expectedAlg ) {
 			$sigVerifier = new WP_Auth0_SymmetricVerifier( $this->a0_options->get( 'client_secret' ) );
 		} else {

diff --git a/lib/token-verifier/WP_Auth0_AsymmetricVerifier.php b/lib/token-verifier/WP_Auth0_AsymmetricVerifier.php
@@ -18,7 +18,6 @@
  */
 final class WP_Auth0_AsymmetricVerifier extends WP_Auth0_SignatureVerifier {
 
-
 	/**
 	 * JWKS array with kid as keys, PEM cert as values.
 	 *
@@ -29,9 +28,9 @@ final class WP_Auth0_AsymmetricVerifier extends WP_Auth0_SignatureVerifier {
 	/**
 	 * JwksVerifier constructor.
 	 *
-	 * @param array $jwks JWKS to use.
+	 * @param WP_Auth0_JwksFetcher $jwks WP_Auth0_JwksFetcher to use.
 	 */
-	public function __construct( array $jwks ) {
+	public function __construct( WP_Auth0_JwksFetcher $jwks ) {
 		$this->jwks = $jwks;
 		parent::__construct( 'RS256' );
 	}
@@ -46,11 +45,14 @@ public function __construct( array $jwks ) {
 	 * @throws WP_Auth0_InvalidIdTokenException If ID token kid was not found in the JWKS.
 	 */
 	protected function checkSignature( Token $token ) : bool {
-		$tokenKid = $token->getHeader( 'kid', false );
-		if ( ! array_key_exists( $tokenKid, $this->jwks ) ) {
-			throw new WP_Auth0_InvalidIdTokenException( 'Could not find a public key for Key ID (kid) "' . $tokenKid . '"' );
+		$token_kid   = $token->getHeader( 'kid', false );
+		$signing_key = $this->jwks->getKey( $token_kid );
+		if ( ! $signing_key ) {
+			throw new WP_Auth0_InvalidIdTokenException(
+				'Could not find a public key for Key ID (kid) "' . $token_kid . '"'
+			);
 		}
 
-		return $token->verify( new RsSigner(), new Key( $this->jwks[ $tokenKid ] ) );
+		return $token->verify( new RsSigner(), new Key( $signing_key ) );
 	}
 }
diff --git a/lib/token-verifier/WP_Auth0_JwksFetcher.php b/lib/token-verifier/WP_Auth0_JwksFetcher.php
@@ -33,13 +33,32 @@ protected function convertCertToPem( string $cert ) : string {
 		return $output;
 	}
 
+	/**
+	 * Get a key using a kid.
+	 *
+	 * @param string $kid Key ID to get.
+	 *
+	 * @return string
+	 */
+	public function getKey( string $kid ) {
+		$keys = $this->getKeys();
+
+		if ( ! empty( $keys ) && empty( $keys[ $kid ] ) ) {
+			$keys = $this->getKeys( false );
+		}
+
+		return $keys[ $kid ] ?? null;
+	}
+
 	/**
 	 * Gets an array of keys from the JWKS as kid => x5c.
 	 *
+	 * @param bool $use_cache Defaults to true to use a cached value.
+	 *
 	 * @return array
 	 */
-	public function getKeys() : array {
-		$keys = get_transient( WPA0_JWKS_CACHE_TRANSIENT_NAME );
+	public function getKeys( $use_cache = true ) : array {
+		$keys = $use_cache ? get_transient( WPA0_JWKS_CACHE_TRANSIENT_NAME ) : [];
 		if ( is_array( $keys ) && ! empty( $keys ) ) {
 			return $keys;
 		}

diff --git a/tests/testApiGetJwks.php b/tests/testApiGetJwks.php
@@ -57,7 +57,7 @@ public function testThatNetworkErrorIsHandled() {
 	}
 
 	/**
-	 * Test that a network error (caught by WP and returned as a WP_Error) is handled properly.
+	 * Test that an Auth0 API error (caught by WP and returned as a WP_Error) is handled properly.
 	 */
 	public function testThatApiErrorIsHandled() {
 		$this->startHttpMocking();
@@ -74,7 +74,7 @@ public function testThatApiErrorIsHandled() {
 	}
 
 	/**
-	 * Test that a network error (caught by WP and returned as a WP_Error) is handled properly.
+	 * Test that a successful call is handled correctly.
 	 */
 	public function testThatSuccessfulCallIsReturned() {
 		$this->startHttpMocking();

diff --git a/tests/testJwksFetcher.php b/tests/testJwksFetcher.php
@@ -0,0 +1,110 @@
+<?php
+/**
+ * Contains Class TestJwksFetcher.
+ *
+ * @package WP-Auth0
+ *
+ * @since 4.0.0
+ */
+
+/**
+ * Class TestJwksFetcher.
+ * Test the WP_Auth0_LoginManager::init_auth0() method.
+ */
+class TestJwksFetcher extends WP_Auth0_Test_Case {
+
+	use HttpHelpers;
+
+	use TokenHelper;
+
+	public function setUp() {
+		parent::setUp();
+		$this->assertFalse( get_transient( 'WP_Auth0_JWKS_cache' ) );
+	}
+
+	public function testThatGetKeysUsesCache() {
+		set_transient( 'WP_Auth0_JWKS_cache', [ '__test_key__' ], 3600 );
+		$jwks = new WP_Auth0_JwksFetcher();
+
+		$this->assertEquals( [ '__test_key__' ], $jwks->getKeys() );
+	}
+
+	public function testThatNotUsingCacheCallsEndpoint() {
+		$this->startHttpMocking();
+		$this->http_request_type = 'success_jwks';
+
+		set_transient( 'WP_Auth0_JWKS_cache', [ '__test_key__' ], 3600 );
+
+		$this->assertEquals( [ '__test_key__' ], get_transient( 'WP_Auth0_JWKS_cache' ) );
+
+		$jwks = new WP_Auth0_JwksFetcher();
+		$keys = $jwks->getKeys( false );
+
+		$this->assertEquals(
+			[ '__test_kid_1__' => "-----BEGIN CERTIFICATE-----\n__test_x5c_1__\n-----END CERTIFICATE-----\n" ],
+			$keys
+		);
+
+		$this->assertEquals( $keys, get_transient( 'WP_Auth0_JWKS_cache' ) );
+	}
+
+	public function testThatNonArrayCacheCallsEndpoint() {
+		$this->startHttpMocking();
+		$this->http_request_type = 'success_jwks';
+
+		set_transient( 'WP_Auth0_JWKS_cache', '__invalid_cache__', 3600 );
+		$jwks = new WP_Auth0_JwksFetcher();
+
+		$this->assertEquals(
+			[ '__test_kid_1__' => "-----BEGIN CERTIFICATE-----\n__test_x5c_1__\n-----END CERTIFICATE-----\n" ],
+			$jwks->getKeys()
+		);
+	}
+
+	public function testThatInvalidJwksReturnsEmptyArray() {
+		$this->startHttpMocking();
+		$this->http_request_type = 'auth0_api_error';
+
+		$jwks = new WP_Auth0_JwksFetcher();
+		$this->assertEquals( [], $jwks->getKeys() );
+	}
+
+	public function testThatGetKeyPullsFromCache() {
+		$this->startHttpMocking();
+		$this->http_request_type = 'halt';
+
+		set_transient(
+			'WP_Auth0_JWKS_cache',
+			[ '__test_kid_1__' => "-----BEGIN CERTIFICATE-----\n__test_x5c_1__\n-----END CERTIFICATE-----\n" ],
+			3600
+		);
+
+		$jwks = new WP_Auth0_JwksFetcher();
+
+		$this->assertEquals(
+			"-----BEGIN CERTIFICATE-----\n__test_x5c_1__\n-----END CERTIFICATE-----\n",
+			$jwks->getKey( '__test_kid_1__' )
+		);
+	}
+
+	public function testThatGetKeyNotFoundForcesEndpointCall() {
+		$this->startHttpMocking();
+		$this->http_request_type = 'success_jwks';
+
+		set_transient( 'WP_Auth0_JWKS_cache', [ '__invalid_kid__' => '__invalid_x5c__' ], 3600 );
+
+		$jwks      = new WP_Auth0_JwksFetcher();
+		$found_x5c = $jwks->getKey( '__test_kid_1__' );
+
+		$this->assertEquals( $found_x5c, $jwks->getKey( '__test_kid_1__' ) );
+		$this->assertEquals( [ '__test_kid_1__' => $found_x5c ], get_transient( 'WP_Auth0_JWKS_cache' ) );
+	}
+
+	public function testThatNotFoundKidReturnsNull() {
+		$this->startHttpMocking();
+		$this->http_request_type = 'success_jwks';
+
+		$jwks = new WP_Auth0_JwksFetcher();
+		$this->assertNull( $jwks->getKey( '__not_found_kid__' ) );
+	}
+}
diff --git a/tests/testLoginManagerRedirectLogin.php b/tests/testLoginManagerRedirectLogin.php
@@ -89,18 +89,28 @@ public function auth0_get_wp_user_handler( $user, $userinfo ) {
 	 * @throws Exception - If set to halt on response.
 	 */
 	public function httpMock( $response_type = null, array $args = null, $url = null ) {
-		$response_type = $response_type ?: $this->getResponseType();
+		$response_type    = $response_type ?: $this->getResponseType();
+		$id_token_payload = [
+			'sub'   => '__test_id_token_sub__',
+			'iss'   => 'https://test.auth0.com/',
+			'aud'   => '__test_client_id__',
+			'nonce' => '__test_nonce__',
+			'exp'   => time() + 1000,
+			'iat'   => time() - 1000,
+		];
+
 		switch ( $response_type ) {
-			case 'success_exchange_code_valid_id_token':
-				$id_token_payload = [
-					'sub'   => '__test_id_token_sub__',
-					'iss'   => 'https://test.auth0.com/',
-					'aud'   => '__test_client_id__',
-					'nonce' => '__test_nonce__',
-					'exp'   => time() + 1000,
-					'iat'   => time() - 1000,
+			case 'success_exchange_code_valid_HS_id_token':
+				$id_token = self::makeHsToken( $id_token_payload, '__test_client_secret__' );
+				return [
+					'body'     => sprintf(
+						'{"access_token":"__test_access_token__","id_token":"%s"}',
+						$id_token
+					),
+					'response' => [ 'code' => 200 ],
 				];
-				$id_token         = self::makeToken( $id_token_payload, '__test_client_secret__' );
+			case 'success_exchange_code_valid_RS_id_token':
+				$id_token = self::makeRsToken( $id_token_payload );
 				return [
 					'body'     => sprintf(
 						'{"access_token":"__test_access_token__","id_token":"%s"}',
@@ -293,7 +303,7 @@ public function testThatGetUserCallIsCorrect() {
 		$this->startHttpMocking();
 		$this->http_request_type = [
 			// Mocked successful code exchange with a valid ID token.
-			'success_exchange_code_valid_id_token',
+			'success_exchange_code_valid_HS_id_token',
 			// Stop the get user call.
 			'halt',
 		];
@@ -326,7 +336,7 @@ public function testThatLoginUserIsCalledWithManagementApiUserinfo() {
 		$this->startHttpMocking();
 		$this->http_request_type = [
 			// Mocked successful code exchange with a valid ID token.
-			'success_exchange_code_valid_id_token',
+			'success_exchange_code_valid_HS_id_token',
 			// Mocked successful get user call.
 			'success_get_user',
 		];
@@ -362,7 +372,7 @@ public function testThatLoginUserIsCalledWithIdTokenIfNoApiAccess() {
 		$this->startHttpMocking();
 		$this->http_request_type = [
 			// Mocked successful code exchange with a valid ID token.
-			'success_exchange_code_valid_id_token',
+			'success_exchange_code_valid_HS_id_token',
 			// Mocked failed get user call.
 			'auth0_api_error',
 		];
@@ -395,7 +405,7 @@ public function testThatLoginUserIsCalledWithIdTokenIfFilterIsSetToFalse() {
 		$this->startHttpMocking();
 		$this->http_request_type = [
 			// Mocked successful code exchange with a valid ID token.
-			'success_exchange_code_valid_id_token',
+			'success_exchange_code_valid_HS_id_token',
 			// Mocked successful get user call.
 			'success_get_user',
 		];
@@ -419,4 +429,37 @@ public function testThatLoginUserIsCalledWithIdTokenIfFilterIsSetToFalse() {
 
 		$this->assertEquals( '__test_id_token_sub__', $user_data['userinfo']->user_id );
 	}
+
+	/**
+	 * @throws WP_Auth0_BeforeLoginException Should not be thrown in this test.
+	 * @throws WP_Auth0_InvalidIdTokenException Should not be thrown in this test.
+	 * @throws WP_Auth0_LoginFlowValidationException Should not be thrown in this test.
+	 */
+	public function testThatGetJwksIsCalledForRs256IdToken() {
+		$this->startHttpMocking();
+		$this->http_request_type = [
+			// Mocked successful code exchange with a valid ID token.
+			'success_exchange_code_valid_RS_id_token',
+			// Stop the get user call.
+			'success_jwks',
+		];
+
+		self::$opts->set( 'domain', 'test.auth0.com' );
+		self::$opts->set( 'client_id', '__test_client_id__' );
+		self::$opts->set( 'client_signing_algorithm', 'RS256' );
+		self::$opts->set( 'cache_expiration', 999999 );
+		$_REQUEST['code']       = uniqid();
+		$_COOKIE['auth0_nonce'] = '__test_nonce__';
+		try {
+			// Need to hide error messages here because a cookie is set.
+			// phpcs:ignore
+			@$this->login->redirect_login();
+		} catch ( InvalidArgumentException $e ) {
+			// Stop process at next exception.
+		}
+
+		$cached_jwks = get_transient( WPA0_JWKS_CACHE_TRANSIENT_NAME );
+
+		$this->assertArrayHasKey( '__test_kid_1__', $cached_jwks );
+	}
 }
diff --git a/tests/testOptionMigrationWs.php b/tests/testOptionMigrationWs.php
@@ -158,7 +158,7 @@ public function testThatChangingMigrationToOnKeepsToken() {
 	 */
 	public function testThatChangingMigrationToOnKeepsWithJwtSetsId() {
 		$client_secret   = '__test_client_secret__';
-		$migration_token = self::makeToken( [ 'jti' => '__test_token_id__' ], $client_secret );
+		$migration_token = self::makeHsToken( [ 'jti' => '__test_token_id__' ], $client_secret );
 		self::$opts->set( 'migration_token', $migration_token );
 		$input = [
 			'migration_ws'  => '1',

diff --git a/tests/testRoutesGetUser.php b/tests/testRoutesGetUser.php
@@ -98,7 +98,7 @@ public function testThatGetUserRouteIsUnauthorizedIfWrongJti() {
 		self::$opts->set( 'client_secret', $client_secret );
 		self::$opts->set( 'migration_token_id', '__test_token_id__' );
 
-		$_POST['access_token'] = self::makeToken( [ 'jti' => uniqid() ], $client_secret );
+		$_POST['access_token'] = self::makeHsToken( [ 'jti' => uniqid() ], $client_secret );
 
 		$output = json_decode( wp_auth0_custom_requests( self::$wp, true ) );