Ensure support for common date string formats

[gonzalo-bulnes]

Fixes #171

Context freedomofpress/securedrop#6257

[sssoleileraaa]

lgtm

• SDK works when installed on the latest client running against the latest SecureDrop server
• SDK works when installed on the latest client running against a SecureDrop server that supports timezones. I lazily checked this by checking out the commit before e4f385fb9 and made the following patch so that it was ISO8601 compliant:

diff --git a/securedrop/journalist_app/api.py b/securedrop/journalist_app/api.py
index e36acdf4f..0b9196564 100644
--- a/securedrop/journalist_app/api.py
+++ b/securedrop/journalist_app/api.py
@@ -121,7 +121,7 @@ def make_blueprint(config: SDConfig) -> Blueprint:
 
             response = jsonify({
                 'token': journalist.generate_api_token(expiration=TOKEN_EXPIRATION_MINS * 60),
-                'expiration': token_expiry.isoformat() + 'Z',
+                'expiration': token_expiry.isoformat(),
                 'journalist_uuid': journalist.uuid,
                 'journalist_first_name': journalist.first_name,
                 'journalist_last_name': journalist.last_name,
(END)

[sssoleileraaa]

I missed that this was in draft mode, but that's okay because it gives you a chance to address the nits if you'd like and expand on my test plan above.

[gonzalo-bulnes]

To the test plan above, I'd add the following step:

• SDK works when installed on the latest client running against a SecureDrop server that responds with the specific bad format: 2022-02-09T05:22:12.165773+00:00Z (see Add support for an ISO 8601 timezone compliant token expiration date-timestamp #171) - the server version freedomofpress/securedrop@5de9dc0 should allow to test that.

[gonzalo-bulnes]

Note: I'd love to add a couple of recorded responses for different expiration date formats, but I'm not having much success setting them up.

[gonzalo-bulnes]

Edit: I removed the commit that this comment refers to, see comment below 🙂

Original comment below the line for future reference.

---

@creviera I think I found a way to add the integration tests by creating three new VCR cassettes, each with a specific token expiration timestamp. One of them is a generic cassette, which should behave like any of the existing ones. Specifically, I believe there shouldn't be issues removing and regenerating it.

However, the other two cassettes describe the API responses for obsolete, or soon-to-be obsolete versions of the SecureDrop servers, with different token expiration date formats. If those two were removed and regenerated, they would stop performing their role, which is to ensure we support those specific formats even though up-to-date servers don't emit them anymore.

I left a note on top of those two files, the idea being that they're left alone until we decide all servers were updated and we can remove the corresponding code. I'm not sure how that plays into existing workflows, though, so I let you judge whether those are regression tests worth having, or more complexity than benefits. 👉 Feel free to strip this commit from the PR! I pushed it to the freedomofpress repo, so you should be able to do it without affecting the other commit 🙂

[gonzalo-bulnes]

💡 Ah-ha, this failing CI build answered my question. If we've got a specific build step to "remove VCR cassettes and run tests against latest API", it means using them to kept records of historical responses is not the intention.

I'll remove the commit from this branch, and keep a copy around so that you can take a look at it anyway. 😉

Edit: here is the commit (on my fork, so it doesn't go away): gonzalo-bulnes@03d9901

[gonzalo-bulnes]

I think this is good for review then! 🕵️

[sssoleileraaa]

we've got a specific build step to "remove VCR cassettes and run tests against latest API", it means using them to kept records of historical responses is not the intention.

yup, exactly! I think it's fine to leave forward-compatibility testing in the unit tests and then when the API changes, we can regenerate all the cassettes that use the updated API.

[sssoleileraaa]

test plan still checks out

[gonzalo-bulnes]

Done @creviera ! 🙂

[sssoleileraaa]

Test Plan

• SDK works when installed on the latest client running against the latest SecureDrop server
• SDK works when installed on the latest client running against a SecureDrop server that supports timezones (I had to patch the server to test this)
• SDK works when installed on the latest client running against a SecureDrop server that responds with the specific bad format: 2022-02-09T05:22:12.165773+00:00Z (see Add support for an ISO 8601 timezone compliant token expiration date-timestamp #171)

[sssoleileraaa]

Whoops, the default merge button was not the merge commit option. I'll see about disabling the other merge options across all workstation repos.