Add custom JWKS path and kid check to JWKFetcher + tests #287

joshcanhelp · 2018-09-07T22:33:08Z

Add a custom path to a JWKS and kid checking (this will be added to the ID token validation in another PR).

Add $kid and $path parameters to \Auth0\SDK\Helpers\JWKFetcher::fetchKeys
Add \Auth0\SDK\Helpers\JWKFetcher::getJwks to call the JWKS URL
Add \Auth0\SDK\Helpers\JWKFetcher::getProp to get a key from the JWKS

Closes #276

lbalmaceda

Is there any chance that you could keep the existing [method that returns an array instance with "kid" as keys] as deprecated and add a new one that just returns the key for a given kid? Making the code from now on use the new one and keeping the deprecated one just in case someone was using it directly.

src/Helpers/JWKFetcher.php

lbalmaceda · 2018-09-20T17:49:28Z

src/Helpers/JWKFetcher.php


-        if (($secret = $this->cache->get($url)) === null) {
-            $secret = [];
+        // No kid passed so get the kid of the first JWK.


This assumption is wrong. kid is not required by the spec, so if the array contains a single element (one key) and kid==null then yes you might return that key directly. But no need to be picking the kid value here just "to use it later"

You'll see further down in this file that the kid is used in the returned value:

$secret[$kid] = $this->convertCertToPem($x5c);

That's how it was stored before so we can't change that.

lbalmaceda · 2018-09-20T17:54:05Z

src/Helpers/JWKFetcher.php

+        // Need the kid and x5c for a well-formed return value. 
+        if (! is_null($kid) && ! is_null($x5c)) {
+            $secret[$kid] = $this->convertCertToPem($x5c);
+            $this->cache->set($jwks_url, $secret);


this cache is a little bit of a lie 😛 you're saving under the same key every time an array with only 1 key (the one that matches the "kid" passed). So if your app requires a second kid, the previous one would be overwritten. Not longer useful as a "cache", is it? ⚡️

Totally agree but legacy 🤷‍♂️

lbalmaceda · 2018-09-20T17:55:11Z

src/Helpers/JWKFetcher.php

+    public function getProp(array $jwks, $prop, $kid = null)
+    {
+        $r_key = null;
+        if (! $kid) {


shouldn't this be "is null or empty" or that's what this is already doing?

That's what this is already doing. There would be a problem if $kid was not defined but it always is.

lbalmaceda · 2018-09-20T17:56:40Z

src/Helpers/JWKFetcher.php

+    }
+
+    /**
+     * Get a specific property from a JWKS using a key, if provided.


using a "key id" if provided. Or something that makes clearer that "key" refers to the JWK and not the prop name.

lbalmaceda · 2018-09-20T17:58:21Z

src/Helpers/JWKFetcher.php

+     *
+     * @return null|string
+     */
+    public function getProp(array $jwks, $prop, $kid = null)


No need to keep this public. Also, I think it would be more useful having a method that given a JWKS returns the array for the given "kid" value. Then you could do result["prop"] to obtain it. Looks clearer than having a "get prop" method that is also checking for "kid" IMO.

lbalmaceda · 2018-09-20T18:00:03Z

src/Helpers/JWKFetcher.php

        }

-        return $secret;
+        // If the value is an array, get the first element.
+        return is_array( $r_key[$prop] ) ? $r_key[$prop][0] : $r_key[$prop];


If I'm iterating over props of a file I know, I'd like to receive it as is, not being handled as non-array stuff. So I expect this method behaves the same on "any type of prop". e.g. make this return the array or null, and you later could do result[0] on the base of knowing how the JWKS is constructed.

lbalmaceda · 2018-09-20T18:04:03Z

src/Helpers/JWKFetcher.php

+     * Get a JWKS given a domain and path to call.
+     *
+     * @param string $domain Base domain for the JWKS, including scheme.
+     * @param string $path   Path to the JWKS from the $domain.


how can this fail and how can the dev know the reason? Is this "catchable"?

Can fail with an Exception in the HTTP client, I'll add docs.

joshcanhelp · 2018-09-20T18:16:35Z

@lbalmaceda - RE: re-writing and deprecating ... that's not a bad idea, TBH. I'll make the 👍 changes above and move that to it's own method.

lbalmaceda · 2018-09-20T20:05:46Z

src/Helpers/JWKFetcher.php

     *
-     * @throws \Exception If the Guzzle HTTP client cannot complete the request.
+     * @codeCoverageIgnore


👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀

🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈

/** * @luchoIngore */

lbalmaceda · 2018-09-20T20:08:56Z

src/Helpers/JWKFetcher.php

-     * @param string      $prop Property to retrieve.
-     * @param null|string $kid  Kid to check.
+     * @param string      $jwks_url URL to the JWKS.
+     * @param string|null $kid      Key ID.


"if not provided and a single JSON Web Key exists on the set, that one will be returned"

lbalmaceda · 2018-09-20T20:12:06Z

src/Helpers/JWKFetcher.php

-        // If a key was not found or the property does not exist, return.
-        if (is_null($r_key) || ! isset($r_key[$prop])) {
+        $jwks = $this->getJwks($jwks_url);
+        $jwk  = $this->getJwk($jwks, $kid);


I'd call this findKeyById or findJwk

lbalmaceda · 2018-09-20T20:14:52Z

src/Helpers/JWKFetcher.php

-                    break;
-                }
-            }
+        $cache_key = $jwks_url.'|'.(string) $kid;


think of the case that kid is null (not passed as arg). This will also break the cache impl below ;) I personally would skip cache in this particular case

null is technically a value we can handle (first key) so why not cache for that value?

if null get's converted to "null" (note the quotes) then I guess it's a valid string and you will end up having a valid key, so yes, you will be able to handle just fine null kids (pun intended)

lbalmaceda · 2018-09-20T20:17:36Z

src/Helpers/JWKFetcher.php

     */
-    public function getProp(array $jwks, $prop, $kid = null)
+    public function getJwksX5c($jwks_url, $kid = null)


this is a key's certificate. So it would be JWK not JWKS

lbalmaceda · 2018-09-20T20:21:43Z

src/Helpers/JWKFetcher.php

+    }
+
+    /**
+     * Check if an array within an array has at least one element.


method is called subArrayHasOneElement, not HasAtLeastOne ⚡️ . Choose either but be concise

lbalmaceda · 2018-09-20T20:24:16Z

src/Helpers/JWKFetcher.php

+     *
+     * @return boolean
+     *
+     * @codeCoverageIgnore


why? you could easily unit-test this :D

It's private, why would I do that? Also:

Please note that final, private, protected, and static methods cannot be stubbed or mocked.

lbalmaceda · 2018-09-20T20:25:25Z

src/JWTVerifier.php

@@ -125,7 +131,7 @@ public function verifyAndDecode($jwt)
                throw new CoreException("We can't trust on a token issued by: `{$body->iss}`.");
            }

-            $secret = $this->JWKFetcher->fetchKeys($body->iss);
+            $secret = $this->JWKFetcher->fetchKeys($body->iss.$this->jwks_path);


let's move the $body->iss.$this->jwks_path one line above to something like jwks_url for clarity

lbalmaceda · 2018-09-20T20:26:56Z

src/Helpers/JWKFetcher.php

     */
-    protected function getJwks($domain, $path)
+    protected function getJwks($jwks_url)


suggestion: I'd call this "fetch" or "request" jwks. Since it's performing an external query

So does getJwkX5c ... change that too?

If you feel like it, go ahead. Remember that's the method you want to expose to the user (the public method). These other in turn are internal (protected methods)

src/Helpers/JWKFetcher.php

lbalmaceda · 2018-09-25T14:20:33Z

src/Helpers/JWKFetcher.php

@@ -24,10 +34,10 @@ class JWKFetcher
    /**
     * JWKFetcher constructor.
     *
-     * @param CacheHandler|null $cache
-     * @param array             $guzzleOptions
+     * @param CacheHandler|null $cache         Cache handler or null for no caching.


But above you say "NoCacheHandler" as well. Should people pass NoCacheHandler or null for this to have no cache?

lbalmaceda · 2018-09-25T14:22:17Z

src/Helpers/JWKFetcher.php

    /**
+     * TODO: Deprecate, use $this->getJwksX5c() instead.
     *
     * @param string $iss


I know this is going to get deprecated. Anyway, I'd document this parameter like: "The auth0 issuer which will get appended the /.well-known/jwks.json path to fetch the jwks."

lbalmaceda · 2018-09-25T14:26:14Z

src/Helpers/JWKFetcher.php

+     */
+    private function getJwk(array $jwks, $kid = null)
+    {
+        if (! $this->subArrayHasNonEmptyFirstItem($jwks, 'keys')) {


Method is negated ("..has NON empty..") but you're also negating it on every use. What about making it return the final result you want?

lbalmaceda

Usually we don't add a doc block to the test cases. That confused me because then I found a few "test helper functions". We just make sure the name of the test is clear enough to communicate the intent of the test. (e.g. using "shouldDoThis" or "shouldReturnThis" or "shouldThrowWhenThisHappened".

This is not a blocker, but for future tests you might want to implement this.

tests/Helpers/JWKFetcherTest.php

lbalmaceda · 2018-09-25T17:27:32Z

tests/Helpers/JWKFetcherTest.php

+        $jwks_url = 'https://localhost/.well-known/jwks.json';
+        $kid      = '__test_kid_2__';
+
+        $pem = $jwksFetcher->getJwksX5c( $jwks_url, $kid );


before this line I'd make sure that cache for that key is not defined so test is fully independent:

`$cache_handler->set( $jwks_url.'|'.$kid, null );`

lbalmaceda · 2018-09-25T17:30:32Z

tests/Helpers/JWKFetcherTest.php

+        $this->assertEquals( '__new_value__', $pem );
+    }
+
+    /**


I'd add a comment line above this saying that from now on, below you can find "test helper functions".

lbalmaceda · 2018-09-25T17:39:11Z

tests/Helpers/JWKFetcherTest.php

+
+    public function testCacheReturn()
+    {
+        $cache_handler = new TestCacheHandler();


Testing the CacheHandler this way is sort of wrong, since this TestCacheHandler is not the implementation that the user will use, right? wink. Instead, you should mock a CacheHandler and assert that get/set/remove methods get called N times with the given XYZ arguments. You can then remove the TestCacheHandler class introduced in this PR.

Well, we don't know the implementation being used, right? So, in this case, it's testing that "some" CacheHandler implementation will work, as long as it's following the right interface. I could add this as a MemoryCacheHandler "official" implementation?

Well, we don't know the implementation being used, right?

No, but you know the interface and you know you're going to call it to
a) store
b) retrieve
c) delete

Adding a subclass for testing purposes is not wrong but it's not the right way either. IMO, you should create a new mock from the interface definition and assert the calls are being made correctly.

I could add this as a MemoryCacheHandler "official" implementation

If you want, go ahead. But remember whatever you ship can't be undone 💃 and unless you have a "best practice in storage/cache" to recommend users to begin using this class, I'd stay away

lbalmaceda

LGTM. Left a few comments via DM 👍

github-actions · 2022-08-24T00:20:00Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

joshcanhelp added the CH: Added label Sep 7, 2018

joshcanhelp added this to the v5-Next milestone Sep 7, 2018

joshcanhelp force-pushed the add-custom-jwks-path-and-kid branch from d1decae to 5f2660d Compare September 10, 2018 17:49

joshcanhelp added the Ready to Review label Sep 10, 2018

joshcanhelp force-pushed the add-custom-jwks-path-and-kid branch 2 times, most recently from 94dad65 to 5fba1de Compare September 11, 2018 17:30

lbalmaceda reviewed Sep 20, 2018

View reviewed changes

Add custom JWKS path and kid check to JWKFetcher + tests

4e12f64

joshcanhelp force-pushed the add-custom-jwks-path-and-kid branch from 31c8465 to 5be5132 Compare September 20, 2018 19:56

lbalmaceda reviewed Sep 20, 2018

View reviewed changes

joshcanhelp force-pushed the add-custom-jwks-path-and-kid branch 2 times, most recently from 8a6b3d8 to a339af8 Compare September 20, 2018 20:54

lbalmaceda reviewed Sep 25, 2018

View reviewed changes

joshcanhelp force-pushed the add-custom-jwks-path-and-kid branch 2 times, most recently from 7da85e7 to 3fa2637 Compare September 25, 2018 17:35

lbalmaceda reviewed Sep 25, 2018

View reviewed changes

joshcanhelp force-pushed the add-custom-jwks-path-and-kid branch from 3fa2637 to 213a958 Compare September 25, 2018 18:06

PR feedback

e990275

joshcanhelp force-pushed the add-custom-jwks-path-and-kid branch from 213a958 to e990275 Compare September 25, 2018 20:46

lbalmaceda approved these changes Sep 26, 2018

View reviewed changes

joshcanhelp removed the Ready to Review label Sep 26, 2018

joshcanhelp merged commit 20038ed into master Sep 26, 2018

joshcanhelp deleted the add-custom-jwks-path-and-kid branch September 26, 2018 14:43

github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2022

Add custom JWKS path and kid check to JWKFetcher + tests #287

Add custom JWKS path and kid check to JWKFetcher + tests #287

Conversation

joshcanhelp commented Sep 7, 2018 • edited Loading

lbalmaceda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshcanhelp commented Sep 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

lbalmaceda left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 24, 2022

joshcanhelp commented Sep 7, 2018 •

edited

Loading

lbalmaceda Sep 25, 2018 •

edited

Loading