Nonce and max_age handling with new CookieStore class #395
Conversation
joshcanhelp
force-pushed
the
add-cookie-store
branch
from
a9cb633
to
d76c20f
Compare
joshcanhelp
changed the title
| @@ -27,15 +29,7 @@ class Auth0Test extends TestCase | |||
| * | |||
| * @var array | |||
| */ | |||
| public static $baseConfig = [ | |||
There was a problem hiding this comment.
Need to move this to setUp to use a class for auth_store
| * | ||
| * @var integer | ||
| */ | ||
| protected $maxAge; |
There was a problem hiding this comment.
Allows a max_age time to be set at the initialization level for all requests.
src/Auth0.php
Outdated
| @@ -613,8 +651,15 @@ public function setIdToken($idToken) | |||
| $sigVerifier = new SymmetricVerifier($this->clientSecret); | |||
| } | |||
|
|
|||
| $verifierOptions = [ | |||
| 'nonce' => $this->authStore->get('nonce'), | |||
There was a problem hiding this comment.
If the stored values are empty, null is returned and the check is skipped
There was a problem hiding this comment.
I guess this is related to @stevehobbsdev 's question. Devs should not be able to skip this check. As far as I know, the only optional value here is max_age. For leeway you always have a default value of 60 secs, and for nonce you will either get the value set by the dev or one generated by you prior to initialize the auth.
Please make it required. Happy to discuss internally ⚡️
src/Auth0.php
Outdated
| @@ -613,8 +651,15 @@ public function setIdToken($idToken) | |||
| $sigVerifier = new SymmetricVerifier($this->clientSecret); | |||
| } | |||
|
|
|||
| $verifierOptions = [ | |||
| 'nonce' => $this->authStore->get('nonce'), | |||
There was a problem hiding this comment.
I guess this is related to @stevehobbsdev 's question. Devs should not be able to skip this check. As far as I know, the only optional value here is max_age. For leeway you always have a default value of 60 secs, and for nonce you will either get the value set by the dev or one generated by you prior to initialize the auth.
Please make it required. Happy to discuss internally ⚡️
|
@joshcanhelp this is the main blocker on my review #395 (comment), the rest are observations or questions. Once you fix that, it should be ready to merge provided @stevehobbsdev agrees. I basically added that blocker because your comment stated that the dev could be able to skip checks when desired. They shouldn't be able to skip any checks 👍 |
|
@stevehobbsdev - Made a few fixes and corrections and added tests for CookieStore. I addressed all of @lbalmaceda's issues here as well. |
|
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. |
Changes
Potentially breaking changes:
auth0_nonce) and required to be checked in ID tokens. All checking is handled in the SDK but some managed hosts require whitelisted cookies and/or specific cookie names so this could be breaking.Other changes:
auth_storetransient_storeconfig option for ID tokennoncehandling; defaults to newCookieStoreclass (Edit:auth_storein this PR was changed totransient_storein Improve transient authorization data handling #397 pre-release)max_ageconfig option forAuth0class to set amax_ageURL parameter on all authorization requests.leewayconfig option forAuth0class to set an ID token time check leewayTesting
Also went through a number of manual tests to ensure the default cookie setting and deleting was working.
Checklist