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

main window might never launch after successful login #575

Closed
redshiftzero opened this issue Oct 16, 2019 · 1 comment · Fixed by #605
Closed

main window might never launch after successful login #575

redshiftzero opened this issue Oct 16, 2019 · 1 comment · Fixed by #605
Assignees
Labels
bug Something isn't working
Milestone

Comments

@redshiftzero
Copy link
Contributor

Description

Here's an interesting case that can arise if a network action fails right after login but before the main window is shown - I hit this in Qubes while testing #567

STR

Login

Expected Behavior

Main window is shown.

Actual Behavior

If the network actions after the login fail, then the main window never launches, and the API actions are never retried. The application must be restarted.

Comments

This is the logic in the controller causing this issue:

  1. In on_authenticate_success we hide the login window, we start a sync, then we use self.api.get_current_user - this last call can fail.
  2. If self.api.get_current_user fails, then on_get_current_user_failure is triggered - it tries to show an error in the gui - but the gui is not displayed yet, since we display the main GUI in the success callback.

In terms of how to resolve, this is happening because we want to pass a user object to the gui such that we can show the nice journalist icon with their name. Here are two ways to proceed:

  1. Provide all the details we want from the /user endpoint in the response from the authentication/token endpoint (just first name, last name I think). Then we don't need to do another round trip.
    or
  2. Display the main GUI window regardless of success or failure, and update the user info once we get the details. Use the queue to get the current user details such that it will automatically retry if it fails.

1 seems more straightforward to me but either should work. @creviera I vaguely recall we discussed providing the first name and last name in the response from the /token endpoint before, I forget why we decided not to implement it though.

@redshiftzero redshiftzero added the bug Something isn't working label Oct 16, 2019
@eloquence eloquence added this to Nominated for next sprint in SecureDrop Team Board Oct 22, 2019
@eloquence eloquence added this to the 0.2.0alpha milestone Oct 22, 2019
@eloquence eloquence moved this from Nominated for next sprint to Current Sprint - 10/23 -11/6 in SecureDrop Team Board Oct 23, 2019
@redshiftzero redshiftzero self-assigned this Nov 5, 2019
@redshiftzero redshiftzero moved this from Current Sprint - 10/23 -11/6 to In Development in SecureDrop Team Board Nov 5, 2019
redshiftzero added a commit to freedomofpress/securedrop that referenced this issue Nov 5, 2019
@redshiftzero
Copy link
Contributor Author

redshiftzero commented Nov 6, 2019

option 1 fix described above is implemented in three PRs, they should be merged in the following order:

  1. server: api: add journalist first name, last name to token response securedrop#4971
  2. SDK: add journalist first and last name as attributes on API securedrop-sdk#105 and an SDK release
  3. client: app: skip round trip to user endpoint during login (fix #575) #605

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants