Add ClaimAccessor Support for ISO Date formatted timestamps #6189

greyfairer · 2018-11-29T16:43:20Z

…y Auth0 and OneLogin)

#6187

…y Auth0 and OneLogin)

pivotal-issuemaster · 2018-11-29T16:43:23Z

@greyfairer Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster · 2018-11-29T16:44:09Z

@greyfairer Thank you for signing the Contributor License Agreement!

greyfairer · 2018-11-29T17:00:21Z

oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/ClaimAccessor.java

+				return Instant.parse((String) claimValue);
+			}
+			catch(DateTimeParseException e){
+				throw new IllegalArgumentException("Unable to convert claim '" + claim +


We could return null, here. Or do a try-catch in the calling code.

Let's keep the existing behaviour of throwing IllegalArgumentException as mentioned in previous comment.

oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/ClaimAccessor.java

jgrandja

Thanks for the PR @greyfairer. I've provided some comments.

jgrandja · 2018-11-30T19:28:49Z

oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/ClaimAccessor.java

@@ -98,8 +99,18 @@ default Instant getClaimAsInstant(String claim) {
 		if (Instant.class.isAssignableFrom(claimValue.getClass())) {
 			return (Instant) claimValue;
 		}
+		if (String.class.isAssignableFrom(claimValue.getClass())) {
+			try {
+				return Instant.parse((String) claimValue);


Instead of Instant.parse() let's attempt both of these:

ZonedDateTime.parse(claimValue, DateTimeFormatter.ISO_INSTANT) ZonedDateTime.parse(claimValue, DateTimeFormatter.ISO_ZONED_DATE_TIME)

If both of these do not parse along with the previous (existing) parse attempts than we'll keep with throwing IllegalArgumentException as is the current behaviour. I know I said I agreed to not throw for getters, but this is a special case IMO. If the claim exists, which is the case if the parse attempt proceed than we cannot just return null as we are basically silently ignoring the error and the existing claim. This will just cause bigger issues for the user as tracking the root issue will make things more difficult.

ZonedDateTime.parse(claimValue, DateTimeFormatter.ISO_INSTANT) doesn't work, but Instant.from(DateTimeFormatter.ISO_INSTANT.parse((String)claimValue)) does.

The second one: DateTimeFormatter.ISO_ZONED_DATE_TIME works for UTC timestamps too (i.e. ending with Z), so I'm not sure if doing the ISO_INSTANT first is necessary.

jgrandja · 2018-11-30T19:29:58Z

oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/ClaimAccessor.java

+				return Instant.parse((String) claimValue);
+			}
+			catch(DateTimeParseException e){
+				throw new IllegalArgumentException("Unable to convert claim '" + claim +


Let's keep the existing behaviour of throwing IllegalArgumentException as mentioned in previous comment.

oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/ClaimAccessor.java

jgrandja · 2018-11-30T19:35:37Z

...2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/ClaimAccessorTests.java

+	// gh-6187
+	@Test
+	public void getClaimAsInstantWhenISODateStringThenReturnInstant() {
+		Instant expectedClaimValue = Instant.now();


Let's explicitly provide the String value for the test, for example, 2011-12-03T10:15:30+01:00

greyfairer · 2018-12-05T14:37:28Z

oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/ClaimAccessor.java

@@ -98,8 +101,24 @@ default Instant getClaimAsInstant(String claim) {
 		if (Instant.class.isAssignableFrom(claimValue.getClass())) {
 			return (Instant) claimValue;
 		}
+		if (String.class.isAssignableFrom(claimValue.getClass())) {
+			try {
+				return Instant.from(DateTimeFormatter.ISO_INSTANT.parse((String)claimValue));


The second one: DateTimeFormatter.ISO_ZONED_DATE_TIME works for UTC timestamps too (i.e. ending with Z), so I'm not sure if doing the ISO_INSTANT first is necessary, but I'll keep it as per your request.

ok let's remove DateTimeFormatter.ISO_INSTANT and let DateTimeFormatter.ISO_ZONED_DATE_TIME handle both cases. Can you ensure we have 2 tests for:

2011-12-03T10:15:30Z

2011-12-03T10:15:30+01:00

jgrandja · 2018-12-05T18:46:44Z

oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/ClaimAccessor.java

@@ -98,8 +101,24 @@ default Instant getClaimAsInstant(String claim) {
 		if (Instant.class.isAssignableFrom(claimValue.getClass())) {
 			return (Instant) claimValue;
 		}
+		if (String.class.isAssignableFrom(claimValue.getClass())) {
+			try {
+				return Instant.from(DateTimeFormatter.ISO_INSTANT.parse((String)claimValue));


ok let's remove DateTimeFormatter.ISO_INSTANT and let DateTimeFormatter.ISO_ZONED_DATE_TIME handle both cases. Can you ensure we have 2 tests for:

2011-12-03T10:15:30Z

2011-12-03T10:15:30+01:00

jgrandja · 2018-12-05T18:49:00Z

oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/ClaimAccessor.java

+					return ZonedDateTime.from(DateTimeFormatter.ISO_ZONED_DATE_TIME.parse((String)claimValue))
+							.toInstant();
+				}
+				catch(DateTimeParseException e2){


Can you ensure the code formatting is consistent with the class for try/catch - see getClaimAsURL

jgrandja · 2018-12-05T18:52:23Z

...2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/ClaimAccessorTests.java

+	}
+
+	// gh-6187
+	@Test(expected = IllegalArgumentException.class)


Please remove expected = IllegalArgumentException.class and use assertThat(this.claimAccessor... to be consistent with other tests in this class. Also, assertThat(..) is the preferred pattern for testing.

jgrandja · 2018-12-06T20:10:36Z

Closing this PR. Please see comment

Issue 6187: support ISO Date formatted timestamps (for 'updated_at' b…

42db8c0

…y Auth0 and OneLogin)

greyfairer commented Nov 29, 2018

View reviewed changes

oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/ClaimAccessor.java Show resolved Hide resolved

rwinch requested a review from jgrandja November 30, 2018 03:09

rwinch assigned jgrandja Nov 30, 2018

rwinch added the status: waiting-for-triage An issue we've not yet triaged label Nov 30, 2018

jgrandja added type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 30, 2018

jgrandja added this to the 5.2.0.M1 milestone Nov 30, 2018

jgrandja requested changes Nov 30, 2018

View reviewed changes

Issue 6187: resolve remarks

16aede9

greyfairer commented Dec 5, 2018

View reviewed changes

jgrandja requested changes Dec 5, 2018

View reviewed changes

rwinch changed the title ~~Issue 6187: support ISO Date formatted timestamps (for 'updated_at' b…~~ Support ISO Date formatted timestamps Dec 6, 2018

rwinch changed the title ~~Support ISO Date formatted timestamps~~ Add ClaimAccessor Support for ISO Date formatted timestamps Dec 6, 2018

jgrandja closed this Dec 6, 2018

jgrandja removed their assignment Dec 6, 2018

jgrandja removed type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Dec 6, 2018

jgrandja removed this from the 5.2.0.M1 milestone Dec 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ClaimAccessor Support for ISO Date formatted timestamps #6189

Add ClaimAccessor Support for ISO Date formatted timestamps #6189

greyfairer commented Nov 29, 2018

pivotal-issuemaster commented Nov 29, 2018

pivotal-issuemaster commented Nov 29, 2018

greyfairer Nov 29, 2018

jgrandja Nov 30, 2018

jgrandja left a comment

jgrandja Nov 30, 2018

greyfairer Dec 5, 2018

jgrandja Nov 30, 2018

jgrandja Nov 30, 2018

greyfairer Dec 5, 2018

jgrandja Dec 5, 2018

jgrandja Dec 5, 2018

jgrandja Dec 5, 2018

jgrandja Dec 5, 2018

jgrandja commented Dec 6, 2018

Add ClaimAccessor Support for ISO Date formatted timestamps #6189

Add ClaimAccessor Support for ISO Date formatted timestamps #6189

Conversation

greyfairer commented Nov 29, 2018

pivotal-issuemaster commented Nov 29, 2018

pivotal-issuemaster commented Nov 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja commented Dec 6, 2018