Improved OIDC compliance #386
Conversation
src/Helpers/JwksVerifier.php
Outdated
| * | ||
| * @param array $jwks JWKS to use. | ||
| */ | ||
| public function __construct(array $jwks) |
There was a problem hiding this comment.
Sample used a URL here but I don't think that HTTP requests and caching should be a part of this class, should be more focused on the task. See the code above to see how this would look in situ:
if ('RS256' === $this->idTokenAlg) {
$jwksFetcher = new JWKFetcher($this->cacheHandler, $this->guzzleOptions);
$jwks = $jwksFetcher->getKeys($this->idTokenIss.'.well-known/jwks.json');
$sigVerifier = new JwksVerifier( $jwks );
} else if ('HS256' === $this->idTokenAlg) {
$sigVerifier = new SymmetricVerifier($this->clientSecret);
}
src/Helpers/IdTokenVerifier.php
Outdated
|
|
||
| if ($now > $authValidUntil) { | ||
| throw new InvalidTokenException( sprintf( | ||
| 'Authentication Time (auth_time) claim in the ID token indicates that too much time has passed since the last end-user authentication. Current time (%d) is after last auth at %d', |
There was a problem hiding this comment.
I think this is worded incorrectly in the sample, now that I'm implementing. Shouldn't it say something like:
Current time (CURRENT_TIME_VALUE) is more than MAX_AGE_VALUE seconds after last auth at AUTH_TIME_VALUE
joshcanhelp
changed the title
src/Auth0.php
Outdated
| if (isset($config['cache_handler']) && $config['cache_handler']) { | ||
| $this->cacheHandler = $config['cache_handler']; | ||
| } else { | ||
| $this->cacheHandler = new NoCacheHandler(); | ||
| } |
There was a problem hiding this comment.
$this->cacheHandler = $config['cache_handler'] ?? new NoCacheHandler();?
There was a problem hiding this comment.
Better, I agree (still getting used to the fact that I can use null coalescing now). I also need to check that it's the right instance, though.
Will push a change for that now.
| protected function checkSignature(Token $token) : bool | ||
| { | ||
| $tokenKid = $token->getHeader('kid', false); | ||
| if (! array_key_exists($tokenKid, $this->jwks)) { |
There was a problem hiding this comment.
Would ! isset() or empty() make more sense here? The current code is not type safe, given that the Key constructor requires a string $content:
There was a problem hiding this comment.
Good catch but we're using the stable 3.3 branch and that's not a concern:
https://github.com/lcobucci/jwt/blob/3.3/src/Signer/Key.php#L34
An empty value here will fail validation rather than throw a type error.
| */ | ||
| abstract class SignatureVerifier | ||
| { | ||
|
|
| namespace Auth0\SDK\Helpers\Tokens; | ||
|
|
||
| use Auth0\SDK\Exception\InvalidTokenException; | ||
| use stdClass; |
There was a problem hiding this comment.
Doesn't appear to be used.
joshcanhelp
force-pushed
the
oidc-improvements
branch
from
ea4ef5c to
c5b21c4
Compare
joshcanhelp
force-pushed
the
oidc-improvements
branch
from
c5b21c4 to
d4d2173
Compare
|
Oops, looks like I commented on something in the middle of rebase. 😅 |
|
@shadowhand - Yeah, my fault ... bad |
|
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
This update improves the SDK support for OpenID Connect. In particular, it modifies the sign in verification phase by substituting backchannel based checks with id_token validation.
References
Testing
Checklist