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

app: skip round trip to user endpoint during login (fix #575) #605

Merged
merged 1 commit into from Nov 12, 2019

Conversation

redshiftzero
Copy link
Contributor

@redshiftzero redshiftzero commented Nov 6, 2019

Description

Fixes #575 following option 1 presented in the bottom of that ticket: avoiding the round-trip to the /user endpoint by presenting everything we need at login time in the response to the /token endpoint

⚠️ This is marked as "Draft" until the corresponding SDK changes are merged and released

Test Plan

  1. Apply small server diff: https://github.com/freedomofpress/securedrop/compare/api-journo-names
  2. Merge SDK change - or simply apply it during review of this PR via pip install -e git+https://github.com/freedomofpress/securedrop-sdk.git@token-endpoint-journo-names#egg=securedrop-sdk
  3. Confirm that login still works, and verify in the diff that there is now no second call to the server prior to the main window being displayed

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

  • I have tested these changes in the appropriate Qubes environment
  • I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
  • These changes should not need testing in Qubes

@eloquence
Copy link
Member

To clarify, will this break login against unpatched prod instances until 1.2.0?

@redshiftzero
Copy link
Contributor Author

once this is merged one can either:

  • use securedrop-client 0.0.9 to use securedrop server 1.1.0
  • use nightlies, and ensure that you are testing against the server patch that will be merged in (I will apply to the SF test instance once it's merged to develop) until securedrop server 1.2.0 is released next month

@eloquence eloquence added this to Current Sprint - 11/6-11/20 in SecureDrop Team Board Nov 6, 2019
@sssoleileraaa sssoleileraaa moved this from Current Sprint - 11/6-11/20 to Under Review in SecureDrop Team Board Nov 9, 2019
Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

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

  • Confirm that login still works
  • verify in the diff that there is now no second call to the server prior to the main window being displayed

One thing I'm checking now is if it's fine to call sync_api after resume_queues, I think what we want is to continue to call sync_api before resume_queues so that new jobs can get added and prioritized before the queue starts again (a higher priority job might need to get bumped to the front of the queue like an auth token reset job. What do you think?

@redshiftzero
Copy link
Contributor Author

oh yeah, very good point, I will move sync_api to before resume_queues

Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

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

lgtm!

@sssoleileraaa sssoleileraaa merged commit 3e0ed0e into master Nov 12, 2019
SecureDrop Team Board automation moved this from Under Review to Done Nov 12, 2019
@sssoleileraaa sssoleileraaa deleted the main-view-disappear branch November 12, 2019 19:08
@redshiftzero
Copy link
Contributor Author

Ah, I marked this as draft to block merge until we bring in the corresponding SDK changes, I'll open a followup for updating to the latest version of the SDK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

main window might never launch after successful login
3 participants