-
Notifications
You must be signed in to change notification settings - Fork 70
Adding cache driver and deprecating metadata #95
Conversation
@clink-aaron Thank you for this PR. we are going to get this into our work stream to take a look at. |
c4c3518
to
ec3c827
Compare
I understand that these seem like drastic changes, but this maintains existing functionality, while significantly improving performance. *Removing metadata retrieval in JwtVerifier constructor* The Okta documentation clearly indicates that the keys endpoint is not dynamic (see here: https://developer.okta.com/docs/reference/api/oidc/#keys). Retrieving the metadata every time the model is instantiated is unnessesary network overhead, process latency, and app complexity. There really just isn't a good reason for doing so. In order to preserve backwards compatibility, that request process has been moved into the `getMeaData()` function, and a `@deprecated` tag added. *Adding caching functionality* Okta's own documentation strongly suggests caching the retrieved keys (see https://developer.okta.com/docs/reference/api/oidc/#best-practices). In an effort to improve upon this, you can now pass an implementation of `\Psr\SimpleCache\CacheInterface`. The ttl is hardcoded at 24 hours, though okta suggests caching for up to 90 days. NOTE: If no implemenation is passed, an instance will be created only using an in-memory store that is only valid for the lifecycle of the request. *various code cleanup* - Return types added to modified methods - unused imports removed - removed irrelivent checks in validation methods - bringing in mockery library to facilitate tests - added missing `ext-json` dependency
Thank you so much for this PR! CLA was submitted |
Hi @clink-aaron, we use this library to validate JWT with JWKS in drupal9(CMS). |
This is expected behavior, you will need to explicitly pass an I'm not sure what Drupal 9 uses, but there are adapters out there that allow you to seamlessly use either: https://symfony.com/doc/current/components/cache/psr6_psr16_adapters.html Check the readme for details on passing that cache instance in. If you are already passing the cache instance in and it's not working, I'd be glad to take a look if you can post an example reproducing the error. |
Hi @clink-aaron , $cache = new Psr16Cache($fileInstance); // symfony cache file implemented PSR cache-interface Getting Error: Symfony\Component\Cache\Exception\InvalidArgumentException: Expiration date must be an integer, a DateInterval or null, "Carbon\Carbon" given. in Symfony\Component\Cache\CacheItem->expiresAfter() (line 106 of C:\xampp\htdocs\drupal-9.2.3\vendor\symfony\cache\CacheItem.php). Reason: The cache used in the FirebaseJwtAdapter is not compatible with symfony/cache because it uses a Carbon date for the cache item TTL but Symfony only accepts an integer or a DateInterval. https://github.com/symfony/cache/blob/364fc90734230d936ac2db8e897cc03ec8497bbb/CacheItem.php#L90
|
@selvaramj this is a valid concern. From my understanding Okta is not accepting any further pull requests in this repo. That puts a damper on resolving issues that can be handled just by updating the package dependencies. The best long term course of action at this point is to fork the repo and get it set up on packagist. The short-term solution is to use a fork and build process configured to pull the dependency directly from your fork via GitHub I am going to be unable to work on this for the next two weeks as I'm off on vacation, however after that I will give it some good investigation into setting up a fork that we can more easily set up for long-term support. |
I understand that these seem like drastic changes, but this maintains
existing functionality, while significantly improving performance.
Removing metadata retrieval in JwtVerifier constructor
The Okta documentation clearly indicates that the keys endpoint is not
dynamic (see here: https://developer.okta.com/docs/reference/api/oidc/#keys).
Retrieving the metadata every time the model is instantiated is
unnessesary network overhead, process latency, and app complexity.
There really just isn't a good reason for doing so.
In order to preserve backwards compatibility, that request process has
been moved into the
getMeaData()
function, and a@deprecated
tagadded.
Adding caching functionality
Okta's own documentation strongly suggests caching the retrieved keys
(see
https://developer.okta.com/docs/reference/api/oidc/#best-practices).
In an effort to improve upon this, you can now pass an implementation of
\Psr\SimpleCache\CacheInterface
.The ttl is hardcoded at 24 hours, though okta suggests caching for up to 90 days.
NOTE: If no implemenation is passed, an instance will be created only using an in-memory
store that is only valid for the lifecycle of the request.
various code cleanup
ext-json
dependency