Skip to content

[SDK-3186] Support date/time custom claim validation #538

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

Merged
merged 4 commits into from
Mar 16, 2022

Conversation

jimmyjames
Copy link
Contributor

Changes

Adds a new verification method to verify a custom date/time claim: Verification#withClaim(String name, Instant value)

Notes:

  • Default implementation delegates to existing withClaim(String name, Date value) method to avoid breaking changes
  • The current Date-based claim validation delegates to the new Instant-based validation, allowing us to remove Date-based validation code
  • The current custom Date-based claim verification was arguably broken, or at least not intuitive. Because date claims are serialized as seconds since the epoch, any expected Date claim would need to have its milliseconds set to zero to be considered equal. This change fixes that by truncating the expected claim's value to seconds.

References

Builds on #537

@jimmyjames jimmyjames added this to the v4-Beta milestone Mar 9, 2022
@jimmyjames jimmyjames requested a review from a team as a code owner March 9, 2022 23:20
@@ -517,7 +517,7 @@ public void shouldValidateCustomClaimOfTypeBoolean() {
@Test
public void shouldValidateCustomClaimOfTypeDate() {
String token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoxNDc4ODkxNTIxfQ.mhioumeok8fghQEhTKF3QtQAksSvZ_9wIhJmgZLhJ6c";
Date date = new Date(1478891521000L);
Date date = new Date(1478891521123L);
Copy link
Contributor Author

@jimmyjames jimmyjames Mar 9, 2022

Choose a reason for hiding this comment

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

Changing the date time to include milliseconds causes this test to fail without the change to truncate milliseconds, demonstrating the issue with current custom Date-based claim validation

@jimmyjames
Copy link
Contributor Author

Codecov failing because default implementation isn't covered. I'll look into adding a test for that (and other default implementations in that interface).

@jimmyjames
Copy link
Contributor Author

jimmyjames commented Mar 10, 2022

Codecov failing because default implementation isn't covered. I'll look into adding a test for that (and other default implementations in that interface).

Additional tests added for default method implementations in Verification instance, as well as for the null handling of Verification implementation methods.

Copy link
Contributor

@poovamraj poovamraj left a comment

Choose a reason for hiding this comment

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

Hey Jim, left a couple of subjective points on the PR. let me know what you think of it

@jimmyjames jimmyjames force-pushed the instant-claim-verification branch from 52ccddd to 451bf78 Compare March 14, 2022 19:09
@jimmyjames jimmyjames requested a review from poovamraj March 15, 2022 15:25
@jimmyjames jimmyjames merged commit 98a6c96 into v4-dev Mar 16, 2022
@jimmyjames jimmyjames deleted the instant-claim-verification branch March 17, 2022 13:11
@poovamraj poovamraj modified the milestones: v4-Beta, 4.0.0-beta.0 May 6, 2022
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.

2 participants