Prevent duplicate instances of source /generate page

[DrGFreeman]

Status

Ready for review

Description of Changes

Fixes #4458.

If a source clicks on 'Get Started' in two separate browser windows or tabs, instead of returning a server error (status code 500), the second instance will redirect the source to the source index page with a flash message stating that another window is already on the 'Get Started' page.

Testing

How should the reviewer test this PR?

1. In the dev VM, launch the Source Interface in a browser and click on "Get Sarted".
2. Note the codename displayed.
3. Open a second browser tab (or window), navigate to the Source Interface index page and click "Get Started".
4. Verify that the second tab (or window) redirects to the source index page with an appropriate flash message.
5. Return to the first tab (or window), click on "Submmit Documents".
6. Display the hidden codename on the submission page and verify it matches the one noted in item 2.

Deployment

Any special considerations for deployment?

No special considerations.

Checklist

If you made changes to the server application code:

• Linting (make lint) and tests (make test) pass in the development container

If you made changes to securedrop-admin:

• Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

• Configuration tests pass

If you made non-trivial code changes:

• I have written a test plan and validated it for this PR

Test added in tests/functional/test_source_warnings.py.

If you made changes to documentation:

• Doc linting (make docs-lint) passed locally

If you added or updated a code dependency:

Choose one of the following:

• I have performed a diff review and pasted the contents to the packaging wiki
• I would like someone else to do the diff review

[DrGFreeman]

I would appreciate comments on the flash message text (from a UX perspective). Thx.

[ninavizz]

@DrGFreeman Hello, thank you for contributing! Reviewing related issues to understand the problem a bit better; will post more, later, once having conferred w/ the team wrt options on this. :)

I'd like to understand the context and goals of this message. Why should the user focus their attention to one version of the same page, and not another? I'm not seeing that it matters much, if the Source has not yet committed to a codename with a submission; and explaining that in the message, feels hairy.

TL;DR, I want to be mindful that we not impose our expectations of how the user should be behaving, onto a user, if there's either no clear benefit to the user as to why, or if it actually doesn't matter at this point in their process.

Typically I wouldn't do this step-back on a PR, but this not being evaluated as a UX Issue in advance of the PR landing, it's helpful for me to understand the needs/motivations/benefits some more. Thanks!!

[zenmonkeykstop]

Functionality works as per test plan, but one case it doesn't address is the scenario where:

1. a user opens Tab A and goes to /generate
2. they then open Tab B on /
3. they then close tab A
4. they then click on Get Started on Tab B and get the flashed message, but have no valid tab to return to and are stuck without clearing the session or manually going to /lookup.

The messaging could address this - something like "A codename has already been generated for your session. Please return to the window or tab in which your codename was first generated to continue, or press New Identity to start over"

(This is pretty similar to the existing text, but doesn't assume Tab A still exists and asks people to get a new Tor identity explicitly rather than closing browser windows.)

[ninavizz]

Yeah, I just don't feel a message is the best way to be handling this—from what I've read on the issues filed that this was built to address.

Automagic redirects to wherever the user already is in the flow with the 1st codename, feels like a much better way to be handling it. The text in the message I don't feel can be kept to the 1-2 sentence maximum, on an aside, for handing both use cases. To me, that's a caveat for all messaging.

Tangentially: this handling of concurrent sessions (with a message) is an older interaction pattern, not seen much anymore on modern consumer webapps. These are the design principles previously established, for the Source UI. Unfamiliar patterns can compromise a user's ability to trust an experience, and disrupt their calm when navigating the experience. Those are my core two reasons for desiring an automagic redirect.

[zenmonkeykstop]

I get your point - the Source Interface can't really be expected to have all the same behaviours available as on modern webapps, due to the contraints imposed by the lack of JS on the recommended TBB security slider setting, but we should be able to figure out a better strategy here.

At the same time, this PR does fix an existing bug and provide guidance to the user on the safest path for them to follow, so with the promise of looking at the broader issue of session setup and management, and a tweaked flash message, I'd say it's a good addition.

[eloquence]

Thanks from me as well, @DrGFreeman, for taking a stab at fixing this bug!

As Nina mentioned, there are significant UX implications to any fix of the original issue, beyond the text of a message. More thoughts on that later today.

For now, just a quick note that as currently written, this PR introduces a regression, at least in my local testing: you can no longer reload the /generate page.

STR

• Load http://localhost:8080/generate in a private browser session
• Reload the page

Expected behavior

• A new codename is generated

Actual behavior

• The user is trapped in the error flow

[eloquence]

Even if the issue above is fixed and the message is updated, we still have the UX issue of potentially "trapping" the user in a "computer broken please press button" workflow (one which still presents all other aspects of the UI as if they were functional), which is a user experience we should IMO avoid if at all possible.

Here's the behavior that Nina and I would propose, after consideration:

1. If the user attempts to load the /generate page or to advance to the "Submit Files or Messages" page while they're already logged in, they should be redirected to the "Submit Files or Messages" page with the standard redirection notice ("You were redirected because you are already logged in. If you want to create a new account, you should log out first.")

2. If the user reloads the /generate page without having previously advanced to the "Submit Files or Messages" page, they should be presented with a new codename (current behavior).

This is not without remaining UX issues. The user in 1) may have written down the wrong codename, and may not notice the alert. However, we would recommend addressing this in follow-up issues, to get to a resolution of the original issue as quickly as possible.

@DrGFreeman @zenmonkeykstop @redshiftzero What do you think? Does that approach make sense to you?

[zenmonkeykstop]

Yup, that addresses the error that I saw as well.

[redshiftzero]

Recapping and considering the situations presented above - this bug is a situation where our duplicate codename logic can result from normal operation:

1. User opens /generate in tab A, codename=X in session.
2. User opens /generate in tab B, codename=Y in session.
3. Click through in either tab A or B, the user is logged in and a source is created with codename=Y.
4. If they click through in the second tab, they'll hit a 500.

Previously we didn't consider this case, and as such we had this exception handling which sets the user as logged out and intentionally causes the 500 server error.

Since this is something that we realize can happen in routine operation, a fix for this bug that also handles the cases discussed above is just removing the logic here. An attacker doesn't learn anything new: they learn already via the 500 that this is a valid codename, which they can then use to login (note: making it infeasible for attackers to guess valid codenames is why we have such a long codename generated from a large wordlist).

While this fixes the 500, it instead logs the user in with a potentially unexpected codename that they haven't written down. Since new_user=True will be in the session we could flash "You were logged in via a different tab. Your codename is codename" when they're redirected to /lookup or similar. Note that again this is providing no new information: the codename is already displayed for users for their first logged in session in /lookup, it's just at the bottom of the page and thus very easy to miss. I realize @eloquence / @ninavizz asked to do that in a followup but it kinda seems like fixing the 500 will still be pretty confusing from the user's perspective unless we flash something to them when the redirect happens letting them know what their account details are.

[ninavizz]

Mmmhhh... Loads of appreciation for all of the thought everyone has invested in this issue. I only grumble with discontent, because—holy cow—we just don't seem to have the bandwidth right now, to do the design, research, design iteration, and then frontend execution on a proper fix.

@redshiftzero The positioning of what I've been calling the "Codename Widget" on the submit page, is most likely problematic. That we call it "a codename," may also be problematic. The E2E "Codename Experience" (as I've sorta called it in my own head) is likely problematic. Which I don't say to whack anyone over the head... I just feel like anything else we do on this ticket, will be akin to the Dutch boy sticking his finger in another hole on the dam, for our users.

I really dislike the 500-Error, but for now I appreciate that it at least flags to the user that something is super wrong—and that they should start over again. Which is a "shitty" experience, but it's not a "confusing" experience (in the sense that confusion might lead to error on their part). I'd rather be shitty than confusing, if I had to pick one over the other. Especially since our users are here with a clear purpose, and we're not worried about losing a passively engaged "meh" kinda user.

My preference would be to give Source users the "shitty" 500-error experience, for now, while designing/testing a better one to implement immediately after we've launched the Workstation Pilot. It's mid-November, now. If the Workstation Pilot keeps on schedule, user testing and design iterations should have a solid solution established for implementation to address all Codename-experience things, come Feb; one that will resolve all usability issues associated with Source use/retention of codenames, which will also (hopefully!) improve things for Journalists by reducing the number of "Same Source, different codename" instances (identified as a common problem in both the recent survey, and in interviews).

Whatcha think @redshiftzero? @eloquence Thoughts, @eloquence?

Again: I so appreciate the desire to not give our users a shitty experience. I don't want that, either. Tough choices + acting with intent, however, feel like a necessary growing-pain for us to lean-into as a team, now, however. If @DrGFreeman or another volunteer would be keen to help implement a solution once validated by user testing, we could also get something better deployed, much sooner; but I doubt I can get a validated solution out, before mid-Jan (budgeting for holidaze & such).

[eloquence]

My preference would be to give Source users the "shitty" 500-error experience, for now, while designing/testing a better one to implement immediately after we've launched the Workstation Pilot.

I consider the current behavior buggy in the narrow sense (it exposes an internal error to the user, which should never happen during normal operation). It is also confusing, because it logs the user out of both sessions. If they attempt an operation in the original tab, they will just be redirected to the "Enter codename" page, and anything they submitted as part of that operation will be silently discarded (!).

My read of @redshiftzero's proposal is pretty straightforward: do the redirection, but make it a bit more obvious what's going on by crafting a custom message, like:

PLEASE NOTE: Your were already logged in via a different tab. Your codename is seven dwarves eat lots of cheddar cheese. Please take care to write it down, and discard the codename you saw on the previous page.

I think that's reasonable to resolve the immediate issue, understanding that it does not eliminate all residual risk of confusion. I agree the "codename" flow needs UX attention, but I don't think we should defer fixing this edge case bug until then.

[ninavizz]

WRT Jen's proposal... I'm concerned about exposing the Codename in such a visible fashion, as a violation of opsec for shoulder surfers. Giving Sources agency to show and hide the codename at their leisure, gives them the opportunity to use SD in plain sight and with folks behind them (such as in a cafe), but without that most critical piece of their experience exposed—until they have a chance to hide that part of their screen from view. If they know to be that careful.

If we were to implement Jen's proposal, I'd at least want to re-word it so that it says something to the effect of:

You are currently using SecureDrop with a codename that includes the words dwarves and cheese

Is there a way to either extract random words from the codename to throw into such a message, or to always extract the 2nd and the 7th word (or something similarly predictable, which would be less desirable imho because it'd compromise security to a shoulder surfer)?

[eloquence]

WRT Jen's proposal... I'm concerned about exposing the Codename in such a visible fashion, as a violation of opsec for shoulder surfers

The codename is shown in full on the previous page, which the user will have to dwell on for a significant amount of time. That said, I share some mild discomfort revealing it in contexts where it is not currently revealed by default. An alternative to picking selected words would be to incorporate the same show/hide logic we already use on the bottom of the page:

PLEASE NOTE: Your were already logged in via a different tab, so we have not generated a new codename for you. Your codename is:
(Show)
Please make sure you have written it down, and discard the codename you saw on the previous page.

[ninavizz]

I like the idea of embedding the hide/show widget in the message, as as simpler approach that achieves the same goal.

WRT

The codename is shown in full on the previous page, which the user will have to dwell on for a significant amount of time.

I don't feel we know enough about how Sources use SD, to assume it's safe to plainly expose the codename to them, ever—eg on the generate page as we currently do. I'd honestly prefer a show/hide widget there, also, but that's a different discussion.

I consulted with colleagues some time ago, who've worked on both GMail and Yahoo! Mail, as well as banking apps, and none of them had ever seen a user attempt to memorize a password. That we'd expect users to memorize a 7-word long passphrase w/o ample research on actual Sources to prove that they would, feels dangerously optimistic. TL;DR, I don't feel it's safe to assume users dwell on the generate page for a significant amount of time.

Anyhow—without distracting on the above paragrpah's tangent—I'll work on a mockup of how to show/execute something around what Jen proposed, and with the hide/show widget embedded. Will get that out sometime tomorrow, if that's ok.

Meanwhile, I'll cross-reference this PR in a new Issue in the UX repo, to be sure we consider this use-case in a future examination of the Codename experience... which I do feel needs re-examination as a near-term priority among Source UI things.

[eloquence]

I'll work on a mockup of how to show/execute something around what Jen proposed, and with the hide/show widget embedded. Will get that out sometime tomorrow, if that's ok.

Perfect, thanks. :)

to be sure we consider this use-case in a future examination of the Codename experience... which I do feel needs re-examination as a near-term priority among Source UI things.

Agreed, it's also high on my mental list of UX issues to consider in the Source Experience once we have bandwidth for it.

@DrGFreeman - thanks for your patience and please don't hesitate to jump in if you have thoughts re: the user experience considerations discussed so far.

[DrGFreeman]

Thanks to all for the feedback and sorry I didn't jump in earlier as I didn't have time to allocate to this project since submitting the PR.

The solution to this apparently simple issue is indeed more complicated than it seems and I appreciate the discussion on the pro & cons of different options.

Referring to the steps described by @redshiftzero,

1. User opens /generate in tab A, codename=X in session.
2. User opens /generate in tab B, codename=Y in session.
3. Click through in either tab A or B, the user is logged in and a source is created with codename=Y.
4. If they click through in the second tab, they'll hit a 500.

my attempt was aimed at solving the risky codename confusion in 3) by preventing the generation of codenames in duplicate tabs altogether, thus eliminating the path to the 500 error at the same time.

This has its drawbacks from a user experience perspective, i.e. the inability to refresh the /generate page and the need to close all tabs or reset identity to start over, but I thought that with a proper message this would be better than the potential consequences of a confusion in codename that could prevent subsequent user login.

To paraphrase @ninavizz, this would be a "shitty" experience, but not a "confusing" experience.

The solution proposed by @redshiftzero has the merit of being much smoother for the user while ensuring the source is aware of the "final" codename used. However, I believe it can still lead to confusion as to why the codename is Y if the user clicked "Submit Documents" in tab A where codename X was displayed.

I will rally to whatever solution is agreed upon and will remain available to help with its implementation ;)

[DrGFreeman]

The solution proposed by @redshiftzero has the merit of being much smoother for the user while ensuring the source is aware of the "final" codename used. However, I believe it can still lead to confusion as to why the codename is Y if the user clicked "Submit Documents" in tab A where codename X was displayed.

I was thinking about this issue and had an idea. My experience with Flask and HTML forms is limited and I don't know if this is feasible or exactly how that would be done but here it is:

If, in each browser tab where /generate is accessed, we would include a unique identifier hidden in the form, we could have a session variable that is a dictionary with the unique identifiers as the keys and the generated codenames as the values. When the user would click "Submit Documents" in one of the tabs, the unique identifier would be included in the form, allowing to retrieve the codename associated with that tab. This codename would be passed to the session['codename'] variable and used to register the source in the database. If the source clicked "Submit Documents" in another tab, they would be redirected to /lookup with a flash message as proposed by @redshiftzero.

Does this make sense?

[eloquence]

Hey @DrGFreeman, sorry we've not followed up further on this yet. As discussed, we're leaning towards the approach outlined in this comment, with an inline show/collapse of the codename in the message. Nina will be proposing styling for that soon, but if you want to take a stab at testing an implementation approach with placeholder messaging/styling for now, we can iterate from there. If you prefer to wait for more UX and technical input, we should be able to respond in a few days.

[DrGFreeman]

Hi @eloquence, as per your suggestion, I've started an implementation of the changes proposed in #5048 and am looking for comments from the team.

[zenmonkeykstop]

@DrGFreeman - OK with me closing this PR in favour of #5048 ?

[DrGFreeman]

@zenmonkeykstop - Absolutely. I wasn't sure if I should do it myself so I let it open until #5048 was reviewed. Closing it now.