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

Updates v3 warnings with varying messages based on instance configs #5679

Merged
merged 5 commits into from
Jan 8, 2021

Conversation

zenmonkeykstop
Copy link
Contributor

@zenmonkeykstop zenmonkeykstop commented Dec 21, 2020

Status

Work in progress

Description of Changes

Fixes #5672 .

Updates the upgrade banner text/color in the Journalist Interface for instances with v2+v3 onion services enabled, and separate more urgent message if an instance is v2-only.

Testing

Staging:

  • run make build-debs and make staging against this branch
  • navigate to the JI login page and verify that no banner is displayed
  • log in to the JI and verify that the v2+v3 banner is displayed
  • log out of the JI
  • log in to the app-staging server, remove the file /var/lib/securedrop/source_v3_url, and restart apache
  • navigate to the JI login page and verify that no banner is displayed
  • log in to the JI and verify that the v2-only banner is displayed

Deployment

Deployed as part of regular install/update.

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 non-trivial code changes:

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

Choose one of the following:

  • I have opened a PR in the docs repo for these changes, or will do so later
  • I would appreciate help with the documentation
  • These changes do not require documentation

@zenmonkeykstop
Copy link
Contributor Author

zenmonkeykstop commented Dec 21, 2020

I'm leaving this as draft for now pending UX feedback. For @eloquence and/or @ninavizz attention, screenshots attached with suggested text/color from #5672 for both cases (and one with a smaller window just coz):
v2Plusv3narrow
v2Plusv3
v2_only

@zenmonkeykstop zenmonkeykstop added this to Ready for Review in SecureDrop Team Board Dec 21, 2020
@zenmonkeykstop zenmonkeykstop added this to the 1.7.0 milestone Dec 21, 2020
@codecov-io
Copy link

codecov-io commented Dec 21, 2020

Codecov Report

Merging #5679 (ed2a20e) into develop (0454478) will decrease coverage by 4.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5679      +/-   ##
===========================================
- Coverage    85.54%   81.43%   -4.12%     
===========================================
  Files           52       53       +1     
  Lines         3771     3969     +198     
  Branches       474      497      +23     
===========================================
+ Hits          3226     3232       +6     
- Misses         440      632     +192     
  Partials       105      105              
Impacted Files Coverage Δ
securedrop/journalist_app/__init__.py 93.69% <100.00%> (+0.23%) ⬆️
securedrop/loaddata.py 0.00% <0.00%> (ø)
securedrop/source_app/utils.py 92.20% <0.00%> (+2.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0454478...ed2a20e. Read the comment docs.

@emkll emkll moved this from Ready for Review to Under Review in SecureDrop Team Board Jan 5, 2021
@emkll emkll self-requested a review January 5, 2021 14:42
@emkll emkll self-assigned this Jan 5, 2021
@zenmonkeykstop zenmonkeykstop marked this pull request as ready for review January 5, 2021 14:49
<div id="v2-onion-eol" class="warning-banner">
{{ gettext('<strong>Update Required:</strong> Your SecureDrop servers are still running v2 onion services, which are being phased out for security reasons. In February 2021, v2 onion services will be disabled, and your SecureDrop servers may become unreachable. <a href="//securedrop.org/v2-onion-eol" rel="noreferrer">Learn More</a>') }}
<div id="v2-onion-eol" class="alert-banner">
{{ gettext('<strong>Update Required:</strong> You must switch your SecureDrop to v3 onion services to ensure that it is reachable after April 30, 2021. <a href="//securedrop.org/v2-onion-eol" rel="noreferrer">Learn More</a>') }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about "update required" -> "action required"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might have been a bit facetious - I think it would be good to hash out final wording with the larger group and with @ninavizz' input.

Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes @zenmonkeykstop , once tests are added I believe this should be good to merge. My comments are mostly related to the language/style of the alerts, best suited to discuss in a synchronous discussion to reduce back and forth


{% if g.show_v2_onion_migration_warning %}
<div id="v2-onion-eol" class="alert-banner">
{{ gettext('<strong>Update Required:</strong> Your SecureDrop servers are still reachable via v2 and v3 onion services. Make sure admins, journalists, and sources are using v3 onion services - then disable v2 onion services. <a href="//securedrop.org/v2-onion-eol" rel="noreferrer">Learn More</a>') }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose we word it as follows, as I was under the impression we were will be disabling v2 services as part of 1.8.0 (or later):
Make sure admins, journalists and sources are using v3 onion services, as they will be disabled disabled on $DATE

</div>
{% endif %}

{% if g.show_v2_onion_migration_warning %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that v3 has already been configured, it might make sense to make this alert a lower level than alert

securedrop/sass/modules/_alert-banner.sass Show resolved Hide resolved
@emkll emkll moved this from Under Review to Discussion Points in SecureDrop Team Board Jan 5, 2021
@ninavizz
Copy link
Member

ninavizz commented Jan 5, 2021

Just created tickets in the docs repo, for requested changes for the Docs page this banner lands upon. Important for the banner to be meaningful or have impact, as it's the behavior of users from an experience we care about; not page prettiness.
freedomofpress/securedrop-docs#122

Will post mox here for banner, later today.

@ninavizz
Copy link
Member

ninavizz commented Jan 6, 2021

Found the Issue for this, so put UX stuff in it per 2019 discussion to keep UX discussion out of PRs.
#5672

@eloquence eloquence moved this from Discussion Points to In Development in SecureDrop Team Board Jan 6, 2021
@eloquence eloquence moved this from In Development to Ready for Review in SecureDrop Team Board Jan 6, 2021

{% if g.show_v2_onion_migration_warning %}
<div id="v2-complete-migration" class="alert-banner">
<img src="{{ url_for('static', filename='i/bang-circle.png') }}" width="20" height="20"> {{ gettext('<strong>Update Required:</strong> Steps remain to complete your onion service update. Please notifyyour admin. <a href="//securedrop.org/v2-onion-eol" rel="noreferrer">Learn More</a>') }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: notifyyour->notify your

I'm fine with the wording here but I would suggest a slightly tweaked version

Update Completion Required: Steps remain to complete your SecureDrop server's update to v3 onion services.

Rationale:

  • The wording "Update Completion" is from @ninavizz's proposal in Add clearer action reminder in Journalist Interface to enable v3 #5672 and I think makes sense, especially since completing the update may involve landing page changes & multi-stakeholder comms. If it sounds too nonstandard, perhaps "Action Required" would work as well.

  • I think the phrase "v3 onion services" should be included because it very clearly identifies the technical component we're talking about (if I'm an admin, there's at least a chance I've heard of it). The user shouldn't have to click through to "Learn more" to find out that important detail.

  • IMO we can ditch the "Please notify" for brevity; it's essentially implied, not included in the other notice, and the person seeing the notice may well be an admin.

Copy link
Contributor Author

@zenmonkeykstop zenmonkeykstop Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about best of all worlds:
"Action Required Set up v3 onion services before April 30 to keep your SecureDrop servers online. Learn more"
"Action Required Complete the v3 onion services setup before April 30. Learn more"

(This suggestion is based on both your comments above and feedback in #5672 - it's about as terse as I think it can get while still being accurate. I'm dead set against "Update Completion Required," it has a high cognitive load factor compared to just "Update required" IMO and reads like an awkward phishing attempt.)

@ninavizz
Copy link
Member

ninavizz commented Jan 7, 2021

Clearly identifying technical components to non-technical users, I am not ok with. This is not the time nor the place for that. Docs text is the time and place for that. Likewise, "implying" what action we want the user to take in a notification breaks the number one rule of notifications, which is to respect your user enough to directly tell them what is being asked of them.

This text is for journalist users. Not admins. Sure, "some" admins are both, but the primary user here is journalists—whom are non-technical users. Period. We need them to alert their admins. Behavioral compliance is the desired outcome. My recommendations follow that. It is a known problem, getting orgs to follow update instruction. Please try something new, here, for a change.

Most of the edit suggestions from @eloquence and @zenmonkeykstop break best-practice rules for messaging text, which frustrates me a lot. I did not compose the text willy-nilly, and am not looking for excuses to justify poor grammar. Those are simply industry standard rules, period. Security engineers are not the audience; non-technical users, are.

The best summation of best practice "rules" for UI text that I've seen to date, are in Material.io's guide. I cannot give my blessing to messaging that neither tells the user explicitly what to do, nor that includes overly technical details. If this were the Admin UI, it would be different, but it is not. It is the Journalist UI.

@zenmonkeykstop
Copy link
Contributor Author

Fair enough, will update banners as per text in #5672 and call it done.

@eloquence
Copy link
Member

Clearly identifying technical components to non-technical users, I am not ok with.

If this is in reference to whether or not to include "v3 onion services" in the banner, then I disagree, but I'll defer to @emkll to make the final call here in review.

Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few comments inline, but tl;dr I think that the language proposed in #5679 (comment) seems like the best approach to me.

I however disagree with the following:

IMO we can ditch the "Please notify" for brevity; it's essentially implied, not included in the other notice, and the person seeing the notice may well be an admin.

It seems that through the back-and-forth and different perspectives, we all agree with the ultimate goal of this change: Upon seeing the notification,a Journalist should contact their administrator (so that the administrator can migrate their instance to v3 onion services). This will ensure journalists reach out to admins, reducing the risk of potential confusion. We should therefore be concise and accurate when describing the issue.

In other words, the goal of this PR as I see it, is to

  1. Have journalists contact admins
  2. When contacting their admin, the journalist can describe the issue in less than a sentence

Better phrased by @zenmonkeykstop in #5672 (comment) :

We're not providing directly relevant instructions/warnings, we're trying to convince users that there's a completely new problem that they have to go and solve somewhere else.

The guidance/instructions for the migration (and its rationale) is provided in the documentation, this PR merely introduced a notification. Being concise and (technically) accurate to me is key here, provided of course, it results in a journalist reaching out to an administrator after looking at the notification.

We should ensure both journalists and admins are not alarmed by the messaging (adhering to the existing tone/language in the Journalist Interface is one way of doing that). Given the timeline, I would be inclined to reserve changes to style for a later date, especially given the relatively straightforward nature of the task, from the perspective of a journalist (please reach out to your admin)

What do you all think with the following?
"Action Required Set up v3 Onion services before April 30 to keep your SecureDrop servers online. Please contact your administrator or learn more"
"Action Required Complete the v3 Onion Services setup before April 30. Please contact your administrator or learn more"


{% if g.show_v2_onion_migration_warning %}
<div id="v2-complete-migration" class="alert-banner">
<img src="{{ url_for('static', filename='i/bang-circle.png') }}" width="20" height="20"> {{ gettext('<strong>Update Required</strong>&nbsp;&nbsp;Steps remain to complete your onion service update. Please notify admin. <a href="//securedrop.org/v2-onion-eol" rel="noreferrer">Learn More</a>') }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your comment in #5679 (comment)

steps remain to complete -> complete , in the spirit of not only less is more, but it may be interpreted by a journalist that they (the journalist) need to update something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose we also update the wording
please notify admin -> please notify your administrator.

From the associated issue:

UI text is often not grammatically correct per-se, for usability reasons.

In my experience, this makes translation more difficult.

<div id="v2-onion-eol" class="warning-banner">
{{ gettext('<strong>Update Required:</strong> Your SecureDrop servers are still running v2 onion services, which are being phased out for security reasons. In February 2021, v2 onion services will be disabled, and your SecureDrop servers may become unreachable. <a href="//securedrop.org/v2-onion-eol" rel="noreferrer">Learn More</a>') }}
<div id="v2-onion-eol" class="alert-banner">
<img src="{{ url_for('static', filename='i/bang-circle.png') }}" width="20" height="20"> {{ gettext('<strong>Update Required</strong>&nbsp;&nbsp;Your SecureDrop servers must be updated to v3 onion services by April 30 to remain live. <a href="//securedrop.org/v2-onion-eol" rel="noreferrer">Learn More</a>') }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend something along the lines of "please notify your administrator" per @ninavizz comment in #5679 (comment)

We need them to alert their admins.

<div id="v2-onion-eol" class="warning-banner">
{{ gettext('<strong>Update Required:</strong> Your SecureDrop servers are still running v2 onion services, which are being phased out for security reasons. In February 2021, v2 onion services will be disabled, and your SecureDrop servers may become unreachable. <a href="//securedrop.org/v2-onion-eol" rel="noreferrer">Learn More</a>') }}
<div id="v2-onion-eol" class="alert-banner">
<img src="{{ url_for('static', filename='i/bang-circle.png') }}" width="20" height="20"> {{ gettext('<strong>Update Required</strong>&nbsp;&nbsp;Your SecureDrop servers must be updated to v3 onion services by April 30 to remain live. <a href="//securedrop.org/v2-onion-eol" rel="noreferrer">Learn More</a>') }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think onion services need to be capitalized

<div id="v2-onion-eol" class="warning-banner">
{{ gettext('<strong>Update Required:</strong> Your SecureDrop servers are still running v2 onion services, which are being phased out for security reasons. In February 2021, v2 onion services will be disabled, and your SecureDrop servers may become unreachable. <a href="//securedrop.org/v2-onion-eol" rel="noreferrer">Learn More</a>') }}
<div id="v2-onion-eol" class="alert-banner">
<img src="{{ url_for('static', filename='i/bang-circle.png') }}" width="20" height="20"> {{ gettext('<strong>Update Required</strong>&nbsp;&nbsp;Your SecureDrop servers must be updated to v3 onion services by April 30 to remain live. <a href="//securedrop.org/v2-onion-eol" rel="noreferrer">Learn More</a>') }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we include the year?

@eloquence
Copy link
Member

eloquence commented Jan 7, 2021

What do you all think with the following?

Sounds good from my end (I'm assuming formatting as in the current implementation). I have a soft preference for "Please contact your administrator. [Learn more]" over "Please contact your administrator or learn more", but either is fine with me. Thanks much y'all for hammering this out.

@zenmonkeykstop
Copy link
Contributor Author

Updating banner text based on recent discussion.

Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @zenmonkeykstop and everyone for input here, this is now good to merge for inclusion in the upcoming 1.7.0 release.

@emkll emkll merged commit 7f16b5f into develop Jan 8, 2021
SecureDrop Team Board automation moved this from Ready for Review to Done Jan 8, 2021
@emkll emkll deleted the 5672-update-v3-warnings branch January 8, 2021 18:14
@conorsch conorsch mentioned this pull request Jan 15, 2021
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Add clearer action reminder in Journalist Interface to enable v3
5 participants