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

Show more specific error message if user needs to re-authenticate with server #736

Closed
rocodes opened this issue Jan 27, 2020 · 10 comments · Fixed by #750
Closed

Show more specific error message if user needs to re-authenticate with server #736

rocodes opened this issue Jan 27, 2020 · 10 comments · Fixed by #750

Comments

@rocodes
Copy link
Contributor

rocodes commented Jan 27, 2020

Currently, if a user has been logged in too long to the client, they receive a 403 when trying to perform actions such as sync/refresh in the client (as seen in sd-proxy). They need to re-authenticate themselves with the server, but the error message displayed is "The SecureDrop server cannot be reached" with the "Retry" action suggested.

If possible, it would be good to show a message that differs from the "flaky Tor" message, since users may become accustomed to clicking "Retry" and may not realize they need to sign out and sign in again.

(In an ideal world, they will not be logged in for this long, but just a thought about more specific network-related error messages if/when possible).

@eloquence
Copy link
Member

Per https://docs.securedrop.org/en/release-1.2.0/development/journalist_api.html#authentication tokens expire after 8 hours. Is there some other expiry you're hitting, or is this behavior itself actually a bug?

@rocodes
Copy link
Contributor Author

rocodes commented Jan 27, 2020

No, this is normal behaviour - my suggestion was only that the message displayed to the user be different in this case ("session expired", for example) rather than "the server cannot be reached," which does not prompt the user to re-authenticate.

@sssoleileraaa
Copy link
Contributor

How about we temporarily make it "Session expired. Please log in again to start a new session." instead of "The server cannot be reached." and then we can spend however long it takes to agree on final language for this?

@eloquence
Copy link
Member

How much more tricky would it be to switch the user into offline mode while we're at it?

@sssoleileraaa
Copy link
Contributor

it looks like all our api methods return an AuthError if there is "error" in the data or the request response to the server: https://github.com/freedomofpress/securedrop-sdk/blob/5af0b200f00bc931cfa78c0923e7354aeda09d0e/sdclientapi/__init__.py#L282-L283

Right now we pause the queue on an AuthError so instead we could log the user out

@sssoleileraaa
Copy link
Contributor

but if we log the user out we wouldn't be able to tell them why

@sssoleileraaa
Copy link
Contributor

or maybe we could, we would need a flag to say to notify to the user that they've been logged out because of an invalid token (since the top bar of the GUI gets redrawn in offline mode, we could have a followup issue to not recreate the top bar's error message bar when switching between modes later)

@eloquence
Copy link
Member

eloquence commented Jan 29, 2020

If it's easy to show a message, I would go with something like "You have been logged out because your session has expired." If not, since this is a bit of an edge case, I think it would be acceptable to not show a message in the first iteration.

@sssoleileraaa
Copy link
Contributor

okayt, so how about for the first iteration we log the user out and open the login screen?

@eloquence
Copy link
Member

I like that proposal -- while there's no strong security difference (you can always select offline mode), there's a small likelihood that a sensitive submission will unexpectedly be displayed on the screen, so keeping that out of sight is probably a good default behavior.

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 a pull request may close this issue.

3 participants