Skip to content

added date validation dedicated exception - #155

Merged
lbalmaceda merged 5 commits into
auth0:masterfrom
Spyna:master
Apr 12, 2017
Merged

added date validation dedicated exception#155
lbalmaceda merged 5 commits into
auth0:masterfrom
Spyna:master

Conversation

@Spyna

@Spyna Spyna commented Mar 16, 2017

Copy link
Copy Markdown

for issue #153

@hzalaz

hzalaz commented Mar 16, 2017

Copy link
Copy Markdown
Contributor

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

Spyna commented Mar 16, 2017

Copy link
Copy Markdown
Author

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

@lbalmaceda

Copy link
Copy Markdown
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

Spyna commented Mar 16, 2017

Copy link
Copy Markdown
Author

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

thank you

@lbalmaceda

lbalmaceda commented Mar 16, 2017

Copy link
Copy Markdown
Contributor

@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