Skip to content
This repository has been archived by the owner on Feb 6, 2023. It is now read-only.

Adding cache driver and deprecating metadata #95

Merged
merged 1 commit into from
Mar 1, 2022

Conversation

clink-aaron
Copy link
Contributor

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

@bretterer
Copy link
Contributor

@clink-aaron Thank you for this PR. we are going to get this into our work stream to take a look at.

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
@bretterer
Copy link
Contributor

Thank you so much for this PR!

CLA was submitted

@bretterer bretterer merged commit 432570d into okta:develop Mar 1, 2022
@clink-aaron clink-aaron deleted the caching branch March 1, 2022 16:31
@clink-aaron clink-aaron restored the caching branch March 1, 2022 17:29
@clink-aaron clink-aaron deleted the caching branch March 1, 2022 17:58
@selvaramj
Copy link

Hi @clink-aaron, we use this library to validate JWT with JWKS in drupal9(CMS).
But the cache (If no implementation is passed, an instance will be created only using an in-memory
a store that is only valid for the lifecycle of the request.) is not working. Always hitting the okta key endpoint to get the JWKS instead of saving and getting from the cache.
Please, could you share your thoughts?

@clink-aaron
Copy link
Contributor Author

@selvaramj

Always hitting the okta key endpoint to get the JWKS instead of saving and getting from the cache.

This is expected behavior, you will need to explicitly pass an \Psr\SimpleCache\CacheInterface implementation to start leveraging caching.

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.

@selvaramj
Copy link

Hi @clink-aaron ,
My Code,
// php

$cache = new Psr16Cache($fileInstance); // symfony cache file implemented PSR cache-interface
$verifier = (new \Okta\JwtVerifier\JwtVerifierBuilder())// This is not needed if using oauth. The other option is new \Okta\JwtVerifier\Discovery\OIDC
->setDiscovery(new \Okta\JwtVerifier\Discovery\Oauth)
->setAdaptor(new FirebasePhpJwt(null, 0, $cache))
->setAudience('api://default')
->setClientId('0oa6j8ytisDEv2p0f5d7')
->setIssuer('https://dev-4370145.okta.com/oauth2/default')
->build();
$result = $verifier->verifyIdToken($code);

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

GIT URL: #107
PR Url: #106

  • already this issue was noticed by some developer and he raised Pull request as well. still waiting for this code changes available in official 'okta-jwt-verifier-php' php package.
    Please, could you share your thoughts?

@clink-aaron
Copy link
Contributor Author

already this issue was noticed by some developer and he raised Pull request as well. still waiting for this code changes available in official 'okta-jwt-verifier-php' php package.
Please, could you share your thoughts?

@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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants