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

added date validation dedicated exception #155

Merged
merged 5 commits into from
Apr 12, 2017
Merged

added date validation dedicated exception #155

merged 5 commits into from
Apr 12, 2017

Conversation

Spyna
Copy link

@Spyna Spyna commented Mar 16, 2017

for issue #153

@hzalaz
Copy link
Member

hzalaz commented Mar 16, 2017

Thanks for the PR @Spyna can you try to reduce the diff the PR has, e.g. avoid reordering the class elements since its hard to tell what you changed?

@Spyna
Copy link
Author

Spyna commented Mar 16, 2017

@hzalaz I' ve restored the original order.
The modified method is assertValidDateClaim

@lbalmaceda
Copy link
Contributor

Hi @Spyna
Thanks for the changes. We think it should throw a specific Exception only in case the exp claim is invalid: lets call that TokenExpiredException and make it extend JWTVerificationException, as it's not that the claim it's invalid but rather that the token was ok but it has expired. Also let's keep the InvalidClaimException in case of invalid nbf and iat claims. Sounds good to you?

The changes should be done in the assertValidDateClaim method: When the boolean is true means we are here for the exp claim, and if it's invalid we should throw this new exception type. When the boolean is false, means it's one of the other two claims and if it's invalid we should throw the old exception. You can also split the method in 2 and handle these scenarios in a separate way if you want. Please make sure to add/update the tests to check the new exception.

Cheers

@Spyna
Copy link
Author

Spyna commented Mar 16, 2017

Sounds great to me,
I'll modify the code and push,

thank you

@lbalmaceda
Copy link
Contributor

lbalmaceda commented Mar 16, 2017

@Spyna Please rollback the code formatting changes in order to leave a clearer diff of the JWTVerifier class. Besides that it's looking fine. Thanks

@lbalmaceda lbalmaceda self-requested a review March 17, 2017 14:10
@lbalmaceda lbalmaceda added this to the v3-Next milestone Apr 7, 2017
@lbalmaceda lbalmaceda merged commit f5cf048 into auth0:master Apr 12, 2017
@lbalmaceda lbalmaceda modified the milestones: v3-Next, 3.2.0 May 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants