When token invalid user must log back in #750

sssoleileraaa · 2020-01-30T00:10:47Z

Description

Fixes #662
Fixes #736

Related: #420 remains open

Test Plan for #662

With a valid token, log out of the client when there are pending jobs and no longer see error message reported in Network error after signing out #662: https://user-images.githubusercontent.com/213636/70762572-d5f70880-1d05-11ea-97c0-f7e2c9b086a5.png

Test Plan for #736

Update /securedrop/journalist_app/api.py in the securedrop repo to return AuthError when making a get_sources or get_all_replies or get_submissions request and see the client log you out and open a login window, you can use this diff if you'd like, see how this results in an AuthError:

     def get_all_sources():
+        return jsonify({'error':'test', 'message':'test'})

Checklist

These changes should not need testing in Qubes but as always please test in qubes if you'd like and want bonus points

sssoleileraaa · 2020-01-30T04:56:27Z

This is what the user will see now when the token is invalid:

kushaldas

While clicking on logout (just after logging in, and I can see the source names in the source list) causes a crash.

Traceback (most recent call last):
  File "/home/kdas/code/securedrop-client/securedrop_client/logic.py", line 288, in on_queue_paused
    self.logout()
  File "/home/kdas/code/securedrop-client/securedrop_client/logic.py", line 495, in logout
    self.call_api(self.api.logout,
AttributeError: 'NoneType' object has no attribute 'logout'

sssoleileraaa · 2020-01-30T16:31:57Z

While clicking on logout (just after logging in, and I can see the source names in the source list) causes a crash.

@kushaldas what is the STR? There shouldn't be any log out buttons showing when the token is invalid and we open the login window

sssoleileraaa · 2020-01-30T16:32:33Z

also, this doesn't sound related to this PR

kushaldas · 2020-01-30T16:41:56Z

@kushaldas what is the STR? There shouldn't be any log out buttons showing when the token is invalid and we open the login window

NUM_SOURCES=200 make dev
login to the client
click on signout

ninavizz · 2020-01-30T16:42:30Z

This isn't quite right. The bang-message on the Login window is to communicate an error from that screen, in response to something the user did—not an error that lands the user on the Login page. Also, this window was never designed to scale with all the needs of an in-app login (such as being logged-out do to session inactivity, etc). At some point, the in-app login just needs to get implemented.

To get the messaging right so this can go out for Beta: what did the user do to get here, and what are the conditions around this screen?

sssoleileraaa · 2020-01-30T16:42:56Z

Thanks @kushaldas I'll try to repro on master and this branch... will report back

sssoleileraaa · 2020-01-30T16:48:38Z

To get the messaging right so this can go out for Beta: what did the user do to get here, and what are the conditions around this screen?

This is for an edge case where the user is logged in 8+ hours and the token expires

This isn't quite right. The bang-message on the Login window is to communicate an error from that screen, in response to something the user did—not an error that lands the user on the Login page.

So, for this first iteration we have two choices: (1) show an error message with the current way we show error messages in the login dialog, or (2) show no error message

The problem I see with number 2 is that the user might not understand why we logged them out when their token expires.

Also, this window was never designed to scale with all the needs of an in-app login (such as being logged-out do to session inactivity, etc). At some point, the in-app login just needs to get implemented.

I think we have an issue for creating a different log in screen for the sign in button: https://app.zeplin.io/project/5c807ea562f734bd2756b243/screen/5cf1b9bda9ebc81e5009b540:

But we haven't yet collaborated on a design for token expiration yet. Let's discuss further in standup.

eloquence · 2020-01-30T17:37:23Z

For this edge case of session expiry, I like @creviera's solution a lot @ninavizz. I'd rather have a message than no message. And I don't think we're likely to prioritize in-client login soon, even post-beta -- there'll just be too many other requests and issues hitting us.

I also don't know that it'd be a good idea to use here, even if we had it, given that the user may have stepped away, and may not expect content to be showing on the screen when they unlock the screensaver.

I understand your point that this error style is intended for login errors. If you feel strongly that it should not be used here, one change we might still be able to make now is to style this message slightly differently (e.g., different color, icon).

sssoleileraaa · 2020-01-30T17:42:58Z

@kushaldas, what you reported is indeed a bug on master as well as in this pr branch. It happens when the token becomes invalid before a logout. I just pushed a fix, and you can confirm by following this PR's Test Plan and making sure you test with an invalid token. You should see... #750 (comment)

redshiftzero · 2020-01-30T22:36:57Z

in case I missed it: for the fix in 7263eb3 - on master how does the api attribute on the controller become None prior to logout? (my expectation is that AuthError would get raised, but then logout would need to be called to set the api attribute to None)

sssoleileraaa · 2020-01-30T22:44:43Z

in widgets.py, when a user clicks on the logout button the controller logout function gets triggered which make an api.logout request after the api is set to None because an AuthError was seen.

sssoleileraaa · 2020-01-30T22:49:21Z

That was an explanation for the current code. For the code on master:

in widgets.py, when a user clicks on the logout button the controller logout function gets triggered which make an api.logout request which wouldn't check whether or not the api is None, but it looks like we only set a copy of the api to None for the queues, so we carefully avoid ever setting the token to None in the Controller, BUT the token would still be invalid so requests would never succeed.

addressed requested changes

redshiftzero

looking good, some thoughts inline

securedrop_client/queue.py

redshiftzero · 2020-02-03T16:51:16Z

securedrop_client/logic.py

-                      self.on_logout_failure)
-        self.api = None
+        if self.api is not None:
+            self.call_api(self.api.logout, self.on_logout_success, self.on_logout_failure)


gotcha, so if self.api is already None, we don't need to invalidate the token, because the token is already invalid/expired? (from inspecting the logic in on_sync_failure and on_queue_paused).

i think it would be good to write that this is what's going on in a comment somewhere (as it's only obvious from tracing the logic and could be easily missed in future modifications). alternatively we could move the setting of self.api = None into this method and make it more explicit that this is what's happening via an invalidate_token kwarg or something. Let me know if you have a better idea here.

i think only setting api to none in one place is a good idea. i was thinking about how I'd like to refactor how we check whether or not we're in offline_mode vs just offline (no token) but i think make a small change to only set api to None in one place would be a good addition to this pr.

more on the whole offline-mode vs no-token: this is relevant if we want to provide a way for users to regenerate a new token without losing jobs in the api queue (so just pause and wait for a new token), but this is definitely a post-pilot issue

redshiftzero · 2020-02-03T22:06:19Z

securedrop_client/api_jobs/uploads.py

@@ -33,6 +33,9 @@ def call_api(self, api_client: API, session: Session) -> str:
            source = session.query(Source).filter_by(uuid=self.source_uuid).one()
            session.commit()

+            import time
+            time.sleep(5)


redshiftzero · 2020-02-03T22:08:19Z

securedrop_client/logic.py

-            retry=True)
+
+        if isinstance(result, ApiInaccessibleError):
+            self.invalidate_token()


very minor nit: maybe invalidate_token_locally or something? (just because otherwise the dev might think this method calls Api.logout)

sssoleileraaa · 2020-02-03T22:13:18Z

Addressed PR comments and rebased on current master branch.

redshiftzero · 2020-02-03T22:25:16Z

one line uncovered so test job is failing, but otherwise lgtm from me. opened #761 to unpack the auth state tracking a bit more

… state Since #750, application-level state transitions (logging in, logging out, and switching into offline mode) call securedrop_client.storage.mark_all_pending_draft_replies(). However, one or more SendReplyJobs (and their underlying POSTs requests to "/sources/<source_uuid/replies") may be in flight on the network at that time, whether or not the application is connected (or even running) to receive their responses. It's up to a given SendReplyJob to raise an exception if it's failed; the state of a given DraftReply can't be determined, or meaningfully overriden, by an application-level event.

… state Since #750, application-level state transitions (logging in, logging out, and switching into offline mode) call securedrop_client.storage.mark_all_pending_draft_replies(). However, one or more SendReplyJobs (and their underlying POST requests to "/sources/<source_uuid/replies") may be in flight on the network at that time, whether or not the application is connected (or even running) to receive their responses. Until we have better (ideally generalized) logic around upload jobs that have not yet been confirmed by the server, these application-level events should not make assumptions about the results of jobs that have already been dispatched.

… state Since #750, application-level state transitions (logging in, logging out, and switching into offline mode) call securedrop_client.storage.mark_all_pending_draft_replies(). However, one or more SendReplyJobs (and their underlying POST requests to "/sources/<source_uuid/replies>") may be in flight on the network at that time, whether or not the application is connected (or even running) to receive their responses. Until we have better (ideally generalized) logic around upload jobs that have not yet been confirmed by the server, these application-level events should not make assumptions about the results of jobs that have already been dispatched.

… state Since #750, application-level state transitions (logging in, logging out, and switching into offline mode) call securedrop_client.storage.mark_all_pending_draft_replies_as_failed(). However, one or more SendReplyJobs (and their underlying POST requests to "/sources/<source_uuid/replies>") may be in flight on the network at that time, whether or not the application is connected (or even running) to receive their responses. Until we have better (ideally generalized) logic around upload jobs that have not yet been confirmed by the server, these application-level events should not make assumptions about the results of jobs that have already been dispatched.

… state Since #750, application-level state transitions (logging in, logging out, and switching into offline mode) call securedrop_client.storage.mark_all_pending_draft_replies_as_failed(). However, one or more SendReplyJobs (and their underlying POST requests to "/sources/<source_uuid/replies>") may be in flight on the network at that time, whether or not the application is connected (or even running) to receive their responses. Until we have better (ideally generalized) logic around upload jobs that have not yet been confirmed by the server, these application-level events should not make assumptions about the results of jobs that have already been dispatched. Individual SendReplyJobs still call their own _set_status_to_failed() method on non-timeout exceptions.

…-level state Since #750, application-level state transitions (logging in, logging out, and switching into offline mode) call securedrop_client.storage.mark_all_pending_draft_replies_as_failed(). However, one or more SendReplyJobs (and their underlying POST requests to "/sources/<source_uuid/replies>") may be in flight on the network at that time, whether or not the application is connected (or even running) to receive their responses. Until we have better (ideally generalized) logic around upload jobs that have not yet been confirmed by the server, these application-level events should not make assumptions about the results of jobs that have already been dispatched. Individual SendReplyJobs still call their own _set_status_to_failed() method on non-timeout exceptions. [WIP] refactor: consolidate mark_all_pending_drafts_as_failed() calls 1. when queue is cleared 2. when local storage updates after sync

… state Since #750, application-level state transitions (logging in, logging out, and switching into offline mode) call securedrop_client.storage.mark_all_pending_draft_replies_as_failed(). However, one or more SendReplyJobs (and their underlying POST requests to "/sources/<source_uuid/replies>") may be in flight on the network at that time, whether or not the application is connected (or even running) to receive their responses. Until we have better (ideally generalized) logic around upload jobs that have not yet been confirmed by the server, these application-level events should not make assumptions about the results of jobs that have already been dispatched. Individual SendReplyJobs still call their own _set_status_to_failed() method on non-timeout exceptions.

sssoleileraaa force-pushed the when-token-invalid-user-must-log-back-in branch from 4a0f7d2 to 47c04fc Compare January 30, 2020 04:54

sssoleileraaa marked this pull request as ready for review January 30, 2020 04:56

sssoleileraaa requested review from kushaldas and redshiftzero as code owners January 30, 2020 04:56

kushaldas previously requested changes Jan 30, 2020

View reviewed changes

sssoleileraaa force-pushed the when-token-invalid-user-must-log-back-in branch from 58b7ba2 to 7263eb3 Compare January 30, 2020 17:40

sssoleileraaa requested a review from kushaldas January 30, 2020 18:15

sssoleileraaa force-pushed the when-token-invalid-user-must-log-back-in branch from 7263eb3 to a2550c2 Compare January 31, 2020 08:21

redshiftzero reviewed Feb 3, 2020

View reviewed changes

sssoleileraaa mentioned this pull request Feb 3, 2020

Network error after signing out #662

Closed

Allie Crevier added 3 commits February 3, 2020 14:04

force logout when token invalid and stop queues on logout

8f7c719

fix bug where api logout call is made when token expired

8676df9

make invalidating token more explicit

f05794b

emit failed reply to update GUI for drafts that fail

ebc8df7

sssoleileraaa force-pushed the when-token-invalid-user-must-log-back-in branch from e0a39b1 to ebc8df7 Compare February 3, 2020 22:10

redshiftzero previously approved these changes Feb 3, 2020

View reviewed changes

add test for invalidate_token

220995c

sssoleileraaa dismissed redshiftzero’s stale review via 220995c February 3, 2020 22:34

redshiftzero approved these changes Feb 3, 2020

View reviewed changes

redshiftzero merged commit 6b30050 into master Feb 3, 2020

redshiftzero deleted the when-token-invalid-user-must-log-back-in branch February 3, 2020 22:47

redshiftzero mentioned this pull request Mar 4, 2020

Logout and display login dialog if token is invalid/expired #517

Closed

cfm mentioned this pull request Mar 3, 2022

Client retries failed jobs after logging out/in without restart #1420

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When token invalid user must log back in #750

When token invalid user must log back in #750

sssoleileraaa commented Jan 30, 2020 •

edited

Loading

sssoleileraaa commented Jan 30, 2020

kushaldas left a comment

sssoleileraaa commented Jan 30, 2020

sssoleileraaa commented Jan 30, 2020

kushaldas commented Jan 30, 2020

ninavizz commented Jan 30, 2020

sssoleileraaa commented Jan 30, 2020

sssoleileraaa commented Jan 30, 2020

eloquence commented Jan 30, 2020 •

edited

Loading

sssoleileraaa commented Jan 30, 2020

redshiftzero commented Jan 30, 2020

sssoleileraaa commented Jan 30, 2020

sssoleileraaa commented Jan 30, 2020 •

edited

Loading

redshiftzero left a comment

redshiftzero Feb 3, 2020

sssoleileraaa Feb 3, 2020

sssoleileraaa Feb 3, 2020

redshiftzero Feb 3, 2020

redshiftzero Feb 3, 2020

sssoleileraaa commented Feb 3, 2020

redshiftzero commented Feb 3, 2020

When token invalid user must log back in #750

When token invalid user must log back in #750

Conversation

sssoleileraaa commented Jan 30, 2020 • edited Loading

Description

Test Plan for #662

Test Plan for #736

Checklist

sssoleileraaa commented Jan 30, 2020

kushaldas left a comment

Choose a reason for hiding this comment

sssoleileraaa commented Jan 30, 2020

sssoleileraaa commented Jan 30, 2020

kushaldas commented Jan 30, 2020

ninavizz commented Jan 30, 2020

sssoleileraaa commented Jan 30, 2020

sssoleileraaa commented Jan 30, 2020

eloquence commented Jan 30, 2020 • edited Loading

sssoleileraaa commented Jan 30, 2020

redshiftzero commented Jan 30, 2020

sssoleileraaa commented Jan 30, 2020

sssoleileraaa commented Jan 30, 2020 • edited Loading

redshiftzero left a comment

Choose a reason for hiding this comment

redshiftzero Feb 3, 2020

Choose a reason for hiding this comment

sssoleileraaa Feb 3, 2020

Choose a reason for hiding this comment

sssoleileraaa Feb 3, 2020

Choose a reason for hiding this comment

redshiftzero Feb 3, 2020

Choose a reason for hiding this comment

redshiftzero Feb 3, 2020

Choose a reason for hiding this comment

sssoleileraaa commented Feb 3, 2020

redshiftzero commented Feb 3, 2020

sssoleileraaa commented Jan 30, 2020 •

edited

Loading

eloquence commented Jan 30, 2020 •

edited

Loading

sssoleileraaa commented Jan 30, 2020 •

edited

Loading