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

deauth queue when user logs out (#397), restart threads if they fail (#380) #404

Merged
merged 3 commits into from
Jun 13, 2019

Conversation

redshiftzero
Copy link
Contributor

@redshiftzero redshiftzero commented Jun 5, 2019

Description

Fixes #397
Fixes #380

Test Plan

In a single run of the client:

  1. Log in
  2. Ensure you can download a file (must be a file not a message)
  3. Log out
  4. Ensure that you cannot download a file (there isn't good user messaging in the UI - see my comment here)
  5. Log back in
  6. Ensure you can download the file now

Checklist

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

  • I have tested these changes in Qubes
  • I do not have a Qubes OS workstation (the reviewer will need to test these changes in Qubes)

@sssoleileraaa
Copy link
Contributor

I still need to review the code, but I followed the test plan and everything worked as described.

I think this PR should also include removing the "Downloading file" message. It's pretty misleading since that's exactly what it is not doing. I read your comment about catching ApiInaccessibleErrors and setting the UI message there, however I imagine we will want to check if the user is offline before creating and making calls from DownloadJobs?

For now it might make sense to check whether or not the application is running in offline mode and returning a custom message for when they click on the Download link. Otherwise they will see this:

wrong-msg

@redshiftzero redshiftzero force-pushed the 397-sec-auth-queues branch from 3523131 to 4b37596 Compare June 6, 2019 23:07
@redshiftzero
Copy link
Contributor Author

added a commit to implement the behavior from the user's perspective described in #379 which looks like the following:

Screen Shot 2019-06-06 at 4 10 41 PM

this means that currently we both:

  1. don't allow download job to get added to the queue by checking the auth status in the controller (this may change based on What can make an API call [Discussion] #400 / what we decide to do with a ApiInaccessibleError signal in Timeout / No Auth #379 but at least for now in master it means the behavior isn't confusing) and
  2. even if a download job is added to the queue, after log out it will no longer run and will raise ApiInaccessibleError

sssoleileraaa
sssoleileraaa previously approved these changes Jun 6, 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.

lgtm with one nit

securedrop_client/queue.py Outdated Show resolved Hide resolved
@redshiftzero redshiftzero dismissed sssoleileraaa’s stale review June 7, 2019 00:34

dismissing until i implement that nit / add more tests

@redshiftzero redshiftzero force-pushed the 397-sec-auth-queues branch from 4b37596 to 8edc797 Compare June 11, 2019 01:44
@redshiftzero redshiftzero changed the title deauth queue when user logs out (#397) deauth queue when user logs out (#397), restart threads if they fail (#380) Jun 11, 2019
* security bugfix: deauth queue when user logs out (#397)

* make sure queues are started when we enqueue a new job (#380)

* also ensure that we can, in one run of the client:

1. Log in, be authed to make network calls
2. Log out, not be authed to make network calls
3. Log _back_ in, once again be authed to make network calls

* show "user must login" message when download clicked if offline
@sssoleileraaa
Copy link
Contributor

taking another look

securedrop_client/queue.py Show resolved Hide resolved
securedrop_client/queue.py Show resolved Hide resolved
securedrop_client/queue.py Show resolved Hide resolved
securedrop_client/logic.py Show resolved Hide resolved
securedrop_client/queue.py Show resolved Hide resolved
@sssoleileraaa sssoleileraaa self-requested a review June 12, 2019 22:00
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, my comments were addressed. just needs to be that follow-up issue and kushal was planning on reviewing this as well.

@redshiftzero
Copy link
Contributor Author

cool thanks, followup added in #419, @kushaldas feel free to take this for a spin tomorrow

@sssoleileraaa
Copy link
Contributor

we all decided that this looks good to merge w/o a second review, merging...

rip deauth queue

@sssoleileraaa sssoleileraaa merged commit 4bde665 into master Jun 13, 2019
@sssoleileraaa sssoleileraaa deleted the 397-sec-auth-queues branch June 13, 2019 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Queue stays authenticated after log out Add support for restarting threads
2 participants