Skip to content
This repository has been archived by the owner on Jan 7, 2024. It is now read-only.

Add support for an ISO 8601 timezone compliant token expiration date-timestamp #171

Closed
rocodes opened this issue Feb 8, 2022 · 6 comments · Fixed by #172
Closed

Add support for an ISO 8601 timezone compliant token expiration date-timestamp #171

rocodes opened this issue Feb 8, 2022 · 6 comments · Fixed by #172

Comments

@rocodes
Copy link
Contributor

rocodes commented Feb 8, 2022

Description

In order to support the server adding timezone information to the token expiration timestamp, the SDK needs to update

self.token_expiration = datetime.strptime(token_data["expiration"], "%Y-%m-%dT%H:%M:%S.%fZ")
to allow timezone data.

For background info, see freedomofpress/securedrop#6256. If the Journalist API begins sending timezone info without any update to the SDK, the following issue will occur when you start the securedrop-client:

check_everything_ohno

And the error log:

2022-02-08 16:21:31,201 - root:206(start_app) INFO: Starting SecureDrop Client 0.6.0-rc2
2022-02-08 16:22:11,743 - securedrop_client.logic:126(call_api) ERROR: time data '2022-02-09T05:22:12.165773+00:00Z' does not match format '%Y-%m-%dT%H:%M:%S.%fZ'
@rocodes rocodes changed the title Feb 8, 2022
@conorsch
Copy link
Contributor

conorsch commented Feb 8, 2022

Confirming I see the same:

❯ tail -f /tmp/tmp.ib6gtRq0aa/logs/client.log
2022-02-08 13:26:11,531 - root:206(start_app) INFO: Starting SecureDrop Client 0.6.0-rc2
2022-02-08 13:26:30,187 - securedrop_client.logic:126(call_api) ERROR: time data '2022-02-09T05:26:30.173419+00:00Z' does not match format '%Y-%m-%dT%H:%M:%S.%fZ'

@zenmonkeykstop
Copy link
Contributor

the Flask 2.0 update requires the use of timezone-aware datetimes, so if thje client can't grok them then that would be the root cause here. It looks like the format is %Y-%m-%dT%H:%M:%S.%fZ but should be %Y-%m-%dT%H:%M:%S.%f%zZ.

it might make sense for this to be a server-side fix - I'm assuming that right now the client just uses the datetimes from the server as gospel without any timezone considerations (which is 😬 if clients are in different timezones to the server), so converting them to server local time and making them naive datetimes would preserve the current behaviour

@conorsch
Copy link
Contributor

conorsch commented Feb 8, 2022

Of the open QA issues, only freedomofpress/securedrop-client#1414 mentions an explicit server version, and that was 2.1.0 prod. Given that @rocodes and I explicitly used 2.2.0-rc1 during review of freedomofpress/securedrop-client#1415, makes sense it's the first time we're encountering it. Let's make sure to include server versions in the test reports we file.

@zenmonkeykstop
Copy link
Contributor

Cool, filed issue mentioned above server-side.

@sssoleileraaa
Copy link
Contributor

When we opened the QA issue, it was assumed that we would test against the latest version of SecureDrop. Now that there is an rc out for SecureDrop, the QA test plan has been updated to test against the latest rc (right now it's 2.2.0-rc1). However, I think it is fine to release the 0.6.0 client before testing against every single rc version of the server. This issue helps with making sure there are no breaking changes on the server, but it is not specific to the 0.6.0 release of the client.

@sssoleileraaa sssoleileraaa changed the title Feb 8, 2022
@sssoleileraaa sssoleileraaa transferred this issue from freedomofpress/securedrop-client Feb 9, 2022
@sssoleileraaa sssoleileraaa changed the title Client fails to launch against 2.2.0-rc1 servers - datetime format issue Add support for an ISO 8601 timezone compliant token expiration date-timestamp Feb 9, 2022
@gonzalo-bulnes
Copy link
Contributor

I have reasons to think that it is preferable that we favor writing the UTC offset as +00:00 instead of Z. I've written the details in freedomofpress/securedrop#6257

For clarity: I don't think this is exclusif of supporting the Z notation. Part of the plan laid out in that issue includes making sure the SDK is lenient in its inputs, so I would still support the Z notation for the time being.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants