Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TokenVerifier for non-OIDC-compliant JWTs #428

Merged
merged 2 commits into from
Feb 19, 2020

Conversation

joshcanhelp
Copy link
Contributor

@joshcanhelp joshcanhelp commented Feb 14, 2020

Description

Adds a generic JWT verifier class, TokenVerifier, which IdTokenVerifier now extends.

References

Testing

  • This change adds test coverage for new/changed/fixed functionality

Checklist

Note: documentation will need to come after v7.1.0 of this library is released. PR:

auth0/docs#8765

  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not master

@joshcanhelp joshcanhelp added this to the 7.1.0 milestone Feb 14, 2020
@joshcanhelp joshcanhelp requested a review from a team February 14, 2020 23:58
{

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All checks, properties, and methods were moved to TokenVerifier

@@ -0,0 +1,163 @@
<?php
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything here was previously in IdTokenVerifier

$verifier = new IdTokenVerifier('__test_iss__', uniqid(), new SymmetricVerifier('__test_secret__'));
$builder = (new Builder())->issuedBy('__test_iss__');
$verifier = new IdTokenVerifier('__test_iss__', '__test_aud__', new SymmetricVerifier('__test_secret__'));
$builder = (new Builder())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests changed because the standard JWT claims are now verified before the ID-token specific ones.

Copy link

@stevehobbsdev stevehobbsdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just left a couple of things. Changes requested around the docs on verify as I think either the time doc needs to come out, or the description changed because it's not accurate - one of the two.

}

/**
* Verifies and decodes an JWT.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: grammar

Suggested change
* Verifies and decodes an JWT.
* Verifies and decodes a JWT.

* @param array $options Options to adjust the verification. Can be:
* - "nonce" to check the nonce contained in the token (recommended).
* - "max_age" to check the auth_time of the token.
* - "time" Unix timestamp to use as the current time for exp, iat, and auth_time checks. Used for testing.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: is this typically something we want to highlight as an option in the docs and right there in the editor? Wouldn't it make it more visible and thus more likely someone could use this to manipulate the validator?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way, if the documentation is to remain, it doesn't look like iat and auth_time are being checked here. Looks like those checks have been left in the subclass.

@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2022
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.

Authorized Party (azp) claim mismatch in the ID token
2 participants