fix: ImpersonatedCredentials does not use Calendar for expiration#1908
fix: ImpersonatedCredentials does not use Calendar for expiration#1908
Conversation
| @Test | ||
| void refreshAccessToken_GMT_dateParsedCorrectly() throws IOException, IllegalStateException { | ||
| Calendar c = Calendar.getInstance(); | ||
| c.add(Calendar.SECOND, VALID_LIFETIME); | ||
|
|
||
| mockTransportFactory.getTransport().setTargetPrincipal(IMPERSONATED_CLIENT_EMAIL); | ||
| mockTransportFactory.getTransport().setAccessToken(ACCESS_TOKEN); | ||
| mockTransportFactory.getTransport().setExpireTime(getFormattedTime(c.getTime())); | ||
| mockTransportFactory.getTransport().addStatusCodeAndMessage(HttpStatusCodes.STATUS_CODE_OK, ""); |
There was a problem hiding this comment.
Removed old tests that reference the obsolete api (setting custom calendar)
There was a problem hiding this comment.
qq: Are there still use cases where calendar may be used without serializing and deserializing the credentials?
There was a problem hiding this comment.
not as far as I know. A code search through the repo shows that it's really only used for computing the expire time with the correct timezone offset.
|
diegomarquezp
left a comment
diegomarquezp
left a comment
There was a problem hiding this comment.
LGTM overall, added a couple of questions.
| expirationInstant = | ||
| Instant.from( | ||
| DateTimeFormatter.ISO_INSTANT | ||
| .withZone(calendar.getTimeZone().toZoneId()) |
There was a problem hiding this comment.
IIUC the only effect of the calendar parameter is to change the formatter's timezone. Does that affect the effective expiration instant in the end, or is it just format?
There was a problem hiding this comment.
It should not affect the effective expire time. The expireTime represents a moment in time, however, it may be a moment of time represented in a different time zone. This change is to ensure that we do not assume that it is always in Zulu time (e.g. account for offsets like +02:00)
| @Test | ||
| void refreshAccessToken_GMT_dateParsedCorrectly() throws IOException, IllegalStateException { | ||
| Calendar c = Calendar.getInstance(); | ||
| c.add(Calendar.SECOND, VALID_LIFETIME); | ||
|
|
||
| mockTransportFactory.getTransport().setTargetPrincipal(IMPERSONATED_CLIENT_EMAIL); | ||
| mockTransportFactory.getTransport().setAccessToken(ACCESS_TOKEN); | ||
| mockTransportFactory.getTransport().setExpireTime(getFormattedTime(c.getTime())); | ||
| mockTransportFactory.getTransport().addStatusCodeAndMessage(HttpStatusCodes.STATUS_CODE_OK, ""); |
There was a problem hiding this comment.
qq: Are there still use cases where calendar may be used without serializing and deserializing the credentials?
) * chore: Add ObsoleteApi annotation * chore: Add tests for serializing ImpersonatedCredentials * chore: Restore old code * chore: Update error message * chore: Fix failing test issue * chore: Remove old tests referecing obsolete api functionality Original-PR: googleapis/google-auth-library-java#1908
) * chore: Add ObsoleteApi annotation * chore: Add tests for serializing ImpersonatedCredentials * chore: Restore old code * chore: Update error message * chore: Fix failing test issue * chore: Remove old tests referecing obsolete api functionality Original-PR: googleapis/google-auth-library-java#1908
2 participants
See b/423905355 for more information.
Previously seen issues where calendar (marked transient) results in NPE when serializing a credential