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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ad1e140
updated v3 warnings with varying CTA based on instance configs
zenmonkeykstop Dec 21, 2020
2f4d8ce
removed unnecessary test
zenmonkeykstop Dec 21, 2020
8f1d356
Updated banner rendering, text based on UX review.
zenmonkeykstop Jan 6, 2021
740994f
further banner text updates
zenmonkeykstop Jan 7, 2021
ed2a20e
Further banner text revisions
zenmonkeykstop Jan 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions install_files/ansible-base/roles/build-securedrop-app-code-deb-pkg/files/usr.sbin.apache2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@
/var/www/securedrop/static/i/delete_gray.png r,
/var/www/securedrop/static/i/delete_red.png r,
/var/www/securedrop/static/i/bang-stop.png r,
/var/www/securedrop/static/i/bang-circle.png r,
/var/www/securedrop/static/i/favicon.png r,
/var/www/securedrop/static/i/font-awesome/black/guard.svg r,
/var/www/securedrop/static/i/font-awesome/black/times.svg r,
Expand Down
14 changes: 11 additions & 3 deletions securedrop/journalist_app/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,13 @@ def create_app(config: 'SDConfig') -> Flask:
app.config['SQLALCHEMY_DATABASE_URI'] = config.DATABASE_URI
db.init_app(app)

v2_enabled = path.exists(path.join(config.SECUREDROP_DATA_ROOT, 'source_v2_url'))
v3_enabled = path.exists(path.join(config.SECUREDROP_DATA_ROOT, 'source_v3_url'))
def _url_exists(u: str) -> bool:
return path.exists(path.join(config.SECUREDROP_DATA_ROOT, u))

v2_enabled = _url_exists('source_v2_url') or ((not _url_exists('source_v2_url'))
and (not _url_exists('source_v3_url')))
v3_enabled = _url_exists('source_v3_url')

app.config.update(V2_ONION_ENABLED=v2_enabled, V3_ONION_ENABLED=v3_enabled)

# TODO: Attaching a Storage dynamically like this disables all type checking (and
Expand Down Expand Up @@ -161,9 +166,12 @@ def setup_g() -> 'Optional[Response]':
else:
g.organization_name = gettext('SecureDrop')

if not app.config['V3_ONION_ENABLED'] or app.config['V2_ONION_ENABLED']:
if app.config['V2_ONION_ENABLED'] and not app.config['V3_ONION_ENABLED']:
g.show_v2_onion_eol_warning = True

if app.config['V2_ONION_ENABLED'] and app.config['V3_ONION_ENABLED']:
g.show_v2_onion_migration_warning = True

if request.path.split('/')[1] == 'api':
pass # We use the @token_required decorator for the API endpoints
else: # We are not using the API
Expand Down
10 changes: 8 additions & 2 deletions securedrop/journalist_templates/base.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,14 @@

{% if g.user %}
{% if g.show_v2_onion_eol_warning %}
<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.

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

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?

</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

<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>
{% endif %}

Expand Down
3 changes: 3 additions & 0 deletions securedrop/sass/_base.sass
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@
@import modules/warning
// Warning - Warning messages that show up in a banner to the user.
@import modules/warning-banner
// Alert - Urgent messages requiring action that show up in a banner to the user.
@import modules/alert-banner
// Confirm prompt - When deleting something this prompt is shown
@import modules/confirm-prompt
// 'Serious' text - Seems to be unused. Delete?
Expand Down Expand Up @@ -122,6 +124,7 @@
+panel
+warning
+warning-banner
+alert-banner
+confirm-prompt
+serious-text
+code
Expand Down
21 changes: 21 additions & 0 deletions securedrop/sass/modules/_alert-banner.sass
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
=alert-banner
.alert-banner
display: block
background-color: $color_urgent_coral
color: white
padding: 10px 0
emkll marked this conversation as resolved.
Show resolved Hide resolved
width: 100%
text-align: center
font-size: small
box-sizing: border-box
-moz-box-sizing: border-box

img
vertical-align: text-bottom

a
color: white
border-bottom: 1px solid white

.close
cursor: pointer
Binary file added securedrop/static/i/bang-circle.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
24 changes: 6 additions & 18 deletions securedrop/tests/test_journalist.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ def test_user_sees_v2_eol_warning_if_only_v2_is_enabled(config, journalist_app,
resp = app.get(url_for('main.index'))

text = resp.data.decode('utf-8')
assert "v2-onion-eol" in text, text
assert 'id="v2-onion-eol"' in text, text
assert 'id="v2-complete-migration"' not in text, text


def test_user_sees_v2_eol_warning_if_both_v2_and_v3_enabled(config, journalist_app, test_journo):
Expand All @@ -91,7 +92,8 @@ def test_user_sees_v2_eol_warning_if_both_v2_and_v3_enabled(config, journalist_a
resp = app.get(url_for('main.index'))

text = resp.data.decode('utf-8')
assert "v2-onion-eol" in text, text
assert 'id="v2-onion-eol"' not in text, text
assert 'id="v2-complete-migration"' in text, text


def test_user_does_not_see_v2_eol_warning_if_only_v3_enabled(config, journalist_app, test_journo):
Expand All @@ -106,22 +108,8 @@ def test_user_does_not_see_v2_eol_warning_if_only_v3_enabled(config, journalist_
resp = app.get(url_for('main.index'))

text = resp.data.decode('utf-8')
assert "v2-onion-eol" not in text, text


def test_user_sees_v2_eol_warning_if_both_urls_do_not_exist(config, journalist_app, test_journo):
journalist_app.config.update(V2_ONION_ENABLED=False, V3_ONION_ENABLED=False)
with journalist_app.test_client() as app:
_login_user(
app,
test_journo['username'],
test_journo['password'],
test_journo['otp_secret'])

resp = app.get(url_for('main.index'))

text = resp.data.decode('utf-8')
assert "v2-onion-eol" in text, text
assert 'id="v2-onion-eol"' not in text, text
assert 'id="v2-complete-migration"' not in text, text


def test_user_with_whitespace_in_username_can_login(journalist_app):
Expand Down