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

createdAt should be deserialized as ISO8601 UTC (not local time) #564

Merged
merged 4 commits into from
Jun 9, 2022

Conversation

adamjmcgrath
Copy link
Contributor

Changes

Fix issue where createdAt from the UserProfile was being interpreted as a local date rather than UTC.

/api/v2/users (and /userinfo for old clients that have OIDC conformant disabled) will issue User Profiles with created_at and updated_at dates in ISO8601 format.

image

see get_users_by_id

We used SimpleDateFormat through Gson by supplying a dateFormat, but SimpleDateFormat doesn't support ISO8601 until API v24. And since the API put a Z at the end of the date to indicate UTC following ISO8601, we ignored the Z (timezone indicator) by putting it in quotes yyyy-MM-dd'T'HH:mm:ss.SSS'Z' (here https://github.com/google/gson/blob/gson-parent-2.8.9/gson/src/main/java/com/google/gson/internal/bind/DateTypeAdapter.java#L85).

With no timezone information Gson deserialized it as a date time local to the device. If the device is in GMT + 3, it assumes the date is GMT + 3, when it's actually UTC (Z = "UTC")

Gson supports ISO8601 and it's the fallback when no suitable date formats are supplied (see https://github.com/google/gson/blob/gson-parent-2.8.9/gson/src/main/java/com/google/gson/internal/bind/DateTypeAdapter.java#L85). So I've changed the default Gson builder to not use SimpleDateFormat and instead fallback to its ISO8601Utils.

I had to leave the serialization/de-serialization of dates in SecureCredentialsManager the same, because user's devices will have the local time stored with the ISO8601 UTC timezone indicator at the end (the Z) and these still need to be interpreted as local times since this is how they were stored.

References

fixes #553

Testing

Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. Since this library has unit testing, tests should be added for new functionality and existing tests should complete without errors.

  • This change adds unit test coverage

  • This change adds integration test coverage

  • This change has been tested on the latest version of the platform/language or why not

Checklist

@adamjmcgrath adamjmcgrath requested a review from a team as a code owner June 6, 2022 17:07
@Throws(Exception::class)
public fun shouldCreateProfileFromISO8601CreatedAt() {
val profileUtc = pojoFrom(
StringReader("""{ "created_at": "2022-04-29T07:09:23.000Z" }"""),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used the example in #553

@adamjmcgrath
Copy link
Contributor Author

Will take a look at those failing tests

poovamraj
poovamraj previously approved these changes Jun 7, 2022
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.

This looks good to me @adamjmcgrath 👍

@poovamraj poovamraj added this to the v2-Next milestone Jun 8, 2022
@poovamraj
Copy link
Contributor

@adamjmcgrath you can update to the latest changes in the vNext branch which has the fix for the flaky CI.

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.

+1

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.

None yet

2 participants