Adds "safe deletion" functionality to journalist interface #5770

zenmonkeykstop · 2021-02-03T16:50:36Z

Status

Ready for review

Description of Changes

Fixes #5766
Fixes #5767

adds option to delete source data while preserving codename
adds UX improvements to reduce the risk of user error during deletion operations.
updates flash notification styling towards Update JI/AI Flash Messaging Styles #5767
misc JI UI updates based on safe deletion prototype feedback

Testing

files+messages (JS enabled)

log into the SI and submit multiple messages/files
log into the JI and click Delete on the All Sources page without selecting any sources' checkboxes
- a server call is not made, and a modal is displayed under the Delete button asking the user to select one or more checkboxes.
select the checkbox for the source created above in the "All Sources" page and Click Delete..:
- a modal is displayed under the delete button giving the option to delete files and messages, delete source accounts, or cancel - the number of sources selected is also displayed.
click Cancel
- The source entry is present and its file/message counts are unchanged
ensure that the source is selected and click Delete.. again, then click Files and Messages
- A success flash message is displayed
- The source is still present and its file/message counts are both 0
in the SI, submit a message
- The message is submitted successfully
- in the JI, when the All Sources page is refreshed the message count is now 1.
- clicking on the source codename opens the source page, the message is listed and can be downloaded.
- on the source page, a reply can be successfully sent to the source
Return to the All Sources page, select the source, and choose Delete > Files and Messages
- The source is present and counts are 0
- clicking through to the source page works and no files/messages/replies are listed.
In the SI, submit some more messages/files, then log out, create a new source account, and submit more messages. Repeat to create a total of 3 sources with submissions.
In the JI, return to the All Sources page.
select two sources, choose Delete > Files and Messages
- both sources are present with zeroed file/message counts
- the third source is present and its counts are unchanged (and non-zero)

source accounts (JS enabled)

log into the SI, recording the source codename, and submit multiple messages/files
log into the JI and select the checkbox for the source created above in the "All Sources" page
Click Delete..:
- a modal is displayed under the delete button giving the option to delete files and messages, delete source accounts, or cancel, the count of selected sources is also displayed
Click Source Accounts
- A second explanatory modal is displayed giving the option to cancel or delete source accounts
Click Yes, Delete Source Accounts
- a success flash message is displayed and the source account is removed from the listing
- the source's files are all queued for deletion on the server
- the source's database entry is deleted
- the sources' reply key is deleted.
return to the SI and attempt to log in as the source
- the source codename is not found.
In the SI, log in with a new account submit some more messages/files, then log out, create a new source account, and submit more messages. Repeat to create a total of 3 sources with submissions.
return to the JI and open the All Sources page
select two sources, choose Delete > Source Accounts > Yes, Delete Source Accounts
- a success flash message is displayed
- the two sources selected are deleted from the All sources page and the server (store/db/reply key)
- the remaining source is unaffected.

JS disabled

test cases above pass with the the following exceptions
- the selected source count is not displayed on the initial deletion modal
- the modals are centered in the page, not displayed under the delete button
- a flash error message is displayed if the user clicks Delete on All Sources when nothing is selected

Flash message styling

Flash message styling matches the examples in Update JI/AI Flash Messaging Styles #5767

UX changes

UX changes conform to the descriptions in "Safe Delete" project delivery #5766

Deployment

Checklist

If you made changes to the server application code:

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

If you added or removed a file deployed with the application:

I have updated AppArmor rules to include the change

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

lgtm-com · 2021-02-03T23:57:15Z

This pull request introduces 1 alert when merging 59d8ebf into bf2cc5b - view on LGTM.com

new alerts:

1 for Missing variable declaration

lgtm-com · 2021-02-04T01:39:02Z

This pull request introduces 1 alert when merging 9783f5c into bf2cc5b - view on LGTM.com

new alerts:

1 for Missing variable declaration

lgtm-com · 2021-02-04T22:29:50Z

This pull request introduces 1 alert when merging cf95945 into 31d1b7a - view on LGTM.com

new alerts:

1 for Missing variable declaration

lgtm-com · 2021-02-05T01:14:46Z

This pull request introduces 1 alert when merging ab6686c into 31d1b7a - view on LGTM.com

new alerts:

1 for Missing variable declaration

lgtm-com · 2021-02-05T01:32:14Z

This pull request introduces 1 alert when merging 40007dc into 5395bbc - view on LGTM.com

new alerts:

1 for Missing variable declaration

emkll

Thanks @zenmonkeykstop (and @ninavizz and @rmol) for these changes. I went through the test plan in staging VMs, both JavaScript enabled and disabled. Functionally (other than the AppArmor rules, see inline) the changes here work well, and the test plan completes successfully in staging VMs. See inline comments for discussion

As @zenmonkeykstop has stated previously, we will need to update some screenshots of the journalist guide after merging these changes, we should track as an issue in the docs repo.

Finally, prior to merge, we should squash some commits (specifically lint/typo related ones), if possible.

files+messages (JS enabled)

log into the SI and submit multiple messages/files
log into the JI and click Delete on the All Sources page without selecting any sources' checkboxes
- a server call is not made, and a modal is displayed under the Delete button asking the user to select one or more checkboxes.
select the checkbox for the source created above in the "All Sources" page and Click Delete..:
- a modal is displayed under the delete button giving the option to delete files and messages, delete source accounts, or cancel - the number of sources selected is also displayed.
click Cancel
- The source entry is present and its file/message counts are unchanged
ensure that the source is selected and click Delete.. again, then click Files and Messages
- A success flash message is displayed
- The source is still present and its file/message counts are both 0
in the SI, submit a message
- The message is submitted successfully
- in the JI, when the All Sources page is refreshed the message count is now 1.
- clicking on the source codename opens the source page, the message is listed and can be downloaded.
- on the source page, a reply can be successfully sent to the source
Return to the All Sources page, select the source, and choose Delete > Files and Messages
- The source is present and counts are 0
- clicking through to the source page works and no files/messages/replies are listed.
In the SI, submit some more messages/files, then log out, create a new source account, and submit more messages. Repeat to create a total of 3 sources with submissions.
In the JI, return to the All Sources page.
select two sources, choose Delete > Files and Messages
- both sources are present with zeroed file/message counts
- the third source is present and its counts are unchanged (and non-zero)

source accounts (JS enabled)

log into the SI, recording the source codename, and submit multiple messages/files
log into the JI and select the checkbox for the source created above in the "All Sources" page
Click Delete..:
- a modal is displayed under the delete button giving the option to delete files and messages, delete source accounts, or cancel, the count of selected sources is also displayed
Click Source Accounts
- A second explanatory modal is displayed giving the option to cancel or delete source accounts
Click Yes, Delete Source Accounts
- a success flash message is displayed and the source account is removed from the listing
- the source's files are all queued for deletion on the server
- the source's database entry is deleted
- the sources' reply key is deleted.
return to the SI and attempt to log in as the source
- the source codename is not found.
In the SI, log in with a new account submit some more messages/files, then log out, create a new source account, and submit more messages. Repeat to create a total of 3 sources with submissions.
return to the JI and open the All Sources page
select two sources, choose Delete > Source Accounts > Yes, Delete Source Accounts
- a success flash message is displayed
- the two sources selected are deleted from the All sources page and the server (store/db/reply key)
- the remaining source is unaffected.

JS disabled

test cases above pass with the the following exceptions
- the selected source count is not displayed on the initial deletion modal
- the modals are centered in the page, not displayed under the delete button
- a flash error message is displayed if the user clicks Delete on All Sources when nothing is selected

Flash message styling

Flash message styling matches the examples in Update JI/AI Flash Messaging Styles #5767 (see comments inline)

UX changes

UX changes conform to the descriptions in "Safe Delete" project delivery #5766 (see comments inline)

emkll · 2021-02-15T15:14:02Z

securedrop/journalist_templates/flashed.html

        {% elif category == "error" %}
-        <img src="{{ url_for('static', filename='i/font-awesome/exclamation-triangle-black.png') }}" height="17" width="20">
+        <img src="{{ url_for('static', filename='i/flash-error.png') }}" height="30" width="9">


To me, based on the examples provided in the parent issue, the exclamation icon here might look slightly too large/tall high, at least in the login error page, when compared to #5767 (comment)

In #5767 the text (presumably "login failed" is also bolded and the same color as the icon, is this something we want to apply here, or defer to later? Similar improvements are already visible in the alert below:

For reference, is what the flash text looked like before the changes introduced here:

Sizes are as specified in #5767, though I do agree that they seem a little on the big side. Updating the Login failed text.

Here's the exclamation/error flash icon set to height="25" and width="7" . I think it looks better and is more aligned with the mockups in #5767, but feel free to disregard this suggestion.

We can revisit this later

install_files/ansible-base/roles/build-securedrop-app-code-deb-pkg/files/usr.sbin.apache2

securedrop/journalist_templates/flashed.html

securedrop/journalist_templates/index.html

securedrop/journalist_templates/flashed.html

securedrop/journalist_app/main.py

securedrop/journalist_app/utils.py

securedrop/journalist_templates/_confirmation_modal.html

securedrop/journalist_templates/col.html

securedrop/sass/modules/_confirm-modal.sass

review is stale

emkll

I went through the latest changes and all looks great with and without JavaScript enabled on the Journalist Interface, on staging instance.

Other than a reply to an inline comment (flash icon), one last comment; it looks like the box size is different here, likely due to the font weird difference between the two buttons.

In the prototype, their height was the same and their the font weight also appears to be the same across the two buttons:

Other than that, It's good to merge from my perspective. Given the scope and the nature of these changes, I would appreciate if @rmol or @eloquence could take one final quick pass.

Thanks again @zenmonkeykstop

zenmonkeykstop · 2021-02-17T22:40:57Z

Fixed the font-weight which also seems to address the button size disparity - those buttons have similar dimensions/margins/padding otherwise.

eloquence · 2021-02-17T22:57:07Z

Given the scope and the nature of these changes, I would appreciate if @rmol or @eloquence could take one final quick pass.

Taking a look now.

eloquence · 2021-02-18T00:03:19Z

securedrop/journalist_app/utils.py

+           Markup(
+             ngettext(
+               '<b>Success!</b> The account and all data for {num} source have been deleted.',
+               '<b>Success!</b> The accounts and all data for {num} sources have been deleted',


Final . is missing in the plural variant.

eloquence · 2021-02-18T00:12:17Z

To begin with, stepping through the test plan, before more styling review and exploratory testing. Tested in Docker dev env only. Functionality looks great at a high level.

files+messages (JS enabled)

log into the SI and submit multiple messages/files
log into the JI and click Delete on the All Sources page without selecting any sources' checkboxes
- a server call is not made, and a modal is displayed under the Delete button asking the user to select one or more checkboxes.
select the checkbox for the source created above in the "All Sources" page and Click Delete..:
- a modal is displayed under the delete button giving the option to delete files and messages, delete source accounts, or cancel - the number of sources selected is also displayed.
click Cancel
- The source entry is present and its file/message counts are unchanged
ensure that the source is selected and click Delete.. again, then click Files and Messages
- A success flash message is displayed
- The source is still present and its file/message counts are both 0
in the SI, submit a message
- The message is submitted successfully
- in the JI, when the All Sources page is refreshed the message count is now 1.
- clicking on the source codename opens the source page, the message is listed and can be downloaded.
- on the source page, a reply can be successfully sent to the source
Return to the All Sources page, select the source, and choose Delete > Files and Messages
- The source is present and counts are 0
- clicking through to the source page works and no files/messages/replies are listed.
In the SI, submit some more messages/files, then log out, create a new source account, and submit more messages. Repeat to create a total of 3 sources with submissions.
In the JI, return to the All Sources page.
select two sources, choose Delete > Files and Messages
- both sources are present with zeroed file/message counts
- the third source is present and its counts are unchanged (and non-zero)

source accounts (JS enabled)

log into the SI, recording the source codename, and submit multiple messages/files
log into the JI and select the checkbox for the source created above in the "All Sources" page
Click Delete..:
- a modal is displayed under the delete button giving the option to delete files and messages, delete source accounts, or cancel, the count of selected sources is also displayed
Click Source Accounts
- A second explanatory modal is displayed giving the option to cancel or delete source accounts
Click Yes, Delete Source Accounts
- a success flash message is displayed and the source account is removed from the listing
- the source's files are all queued for deletion on the server
  - See output below, also verified that shredder and store are empty.
- the source's database entry is deleted
- the sources' reply key is deleted.
  - Verified via gpg2 --homedir=/var/lib/securedrop/keys/ --list-keys
return to the SI and attempt to log in as the source
- the source codename is not found.
In the SI, log in with a new account submit some more messages/files, then log out, create a new source account, and submit more messages. Repeat to create a total of 3 sources with submissions.
return to the JI and open the All Sources page
select two sources, choose Delete > Source Accounts > Yes, Delete Source Accounts
- a success flash message is displayed
- the two sources selected are deleted from the All sources page and the server (store/db/reply key)
- the remaining source is unaffected.

JS disabled

test cases above pass with the the following exceptions
- the selected source count is not displayed on the initial deletion modal
- the modals are centered in the page, not displayed under the delete button
- ❓ a flash error message is displayed if the user clicks Delete on All Sources when nothing is selected
  - The error is shown after the user steps through the entire flow, including the confirmation step for deleting source accounts. I'm guessing that's the expected behavior here, given that our choice here is basically either an immediate POST submission or the CSS flow.

Regarding the on-disk behavior, attaching sample log output as well for an account deletion.

Dev env log of source account deletion

2021-02-18 00:09:10,808 INFO Purging deleted sources (1)
2021-02-18 00:09:10,808 INFO Moving /var/lib/securedrop/store/Q3MWSPMEP7B3EIJCVYFRSH6G32F67HB4E6UJTAUKX3HP6OT2EAMCBLSUS4KYHYZJJ2I466EWC6REEZKHW7RWEDVVOLR5BWHHOVQEB3I= to shredder: /var/lib/securedrop/shredder/tmp185s3kjw/Q3MWSPMEP7B3EIJCVYFRSH6G32F67HB4E6UJTAUKX3HP6OT2EAMCBLSUS4KYHYZJJ2I466EWC6REEZKHW7RWEDVVOLR5BWHHOVQEB3I=
2021-02-18 00:09:10,817 INFO Setting homedir to '/var/lib/securedrop/keys'
2021-02-18 00:09:10,827 INFO 
Initialised settings:
binary: /usr/bin/gpg2
binary version: 2.1.11
homedir: /var/lib/securedrop/keys
ignore_homedir_permissions: False
keyring: /var/lib/securedrop/keys/pubring.gpg
secring: /var/lib/securedrop/keys/secring.gpg
default_preference_list: SHA512 SHA384 SHA256 AES256 CAMELLIA256 TWOFISH AES192 ZLIB ZIP Uncompressed
keyserver: hkp://wwwkeys.pgp.net
options: ['--yes']
verbose: False
use_agent: False

2021-02-18 00:09:20,241 INFO Clearing shredder
2021-02-18 00:09:20,242 INFO Files to delete: 2
2021-02-18 00:09:20,242 INFO Securely deleting file 1/2: /var/lib/securedrop/shredder/tmp185s3kjw/Q3MWSPMEP7B3EIJCVYFRSH6G32F67HB4E6UJTAUKX3HP6OT2EAMCBLSUS4KYHYZJJ2I466EWC6REEZKHW7RWEDVVOLR5BWHHOVQEB3I=/1-sophomore_flophouse-msg.gpg
2021-02-18 00:09:20,368 INFO Securely deleted file 1/2: /var/lib/securedrop/shredder/tmp185s3kjw/Q3MWSPMEP7B3EIJCVYFRSH6G32F67HB4E6UJTAUKX3HP6OT2EAMCBLSUS4KYHYZJJ2I466EWC6REEZKHW7RWEDVVOLR5BWHHOVQEB3I=/1-sophomore_flophouse-msg.gpg
2021-02-18 00:09:20,368 INFO Securely deleting file 2/2: /var/lib/securedrop/shredder/tmp185s3kjw/Q3MWSPMEP7B3EIJCVYFRSH6G32F67HB4E6UJTAUKX3HP6OT2EAMCBLSUS4KYHYZJJ2I466EWC6REEZKHW7RWEDVVOLR5BWHHOVQEB3I=/2-sophomore_flophouse-doc.gz.gpg
2021-02-18 00:09:20,575 INFO Securely deleted file 2/2: /var/lib/securedrop/shredder/tmp185s3kjw/Q3MWSPMEP7B3EIJCVYFRSH6G32F67HB4E6UJTAUKX3HP6OT2EAMCBLSUS4KYHYZJJ2I466EWC6REEZKHW7RWEDVVOLR5BWHHOVQEB3I=/2-sophomore_flophouse-doc.gz.gpg

securedrop/journalist_templates/_sources_confirmation_modal.html

eloquence · 2021-02-18T00:40:03Z

securedrop/journalist_templates/js-strings.html

@@ -6,4 +6,5 @@
  <div id="select-none-string" hidden>{{ gettext('Select None') }}</div>
  <div id="delete-user-confirm-string" hidden>{{ gettext('Are you sure you want to delete the user {username}?') }}</div>
  <div id="reset-user-mfa-confirm-string" hidden>{{ gettext('Are you sure you want to reset two-factor authentication for {username}?') }}</div>
+  <div id="sources-selected" hidden>{{ gettext('Sources selected: ') }}</div>


If I'm not mistaken, this string is a compromise to accommodate lack of pluralization support in our current solution for managing client-side strings. It may be worth at least minimally augmenting the get_string JS function to support fetching basic singular/plural variants so we can use more human-friendly strings. For any changes to the JI, including admin features, this problem is likely to arise again in future. I understand this may be pushing it for 1.8.0.

Yup @rmol and I briefly discussed this. Having some way to properly handle translateable JS strings would be useful, but it's not going to happen for this release.

The consensus among Arabic translators was that this usage ("Things: x") is not optimal, but people would understand.

securedrop/journalist_templates/_sources_confirmation_final_modal.html

eloquence · 2021-02-18T00:53:49Z

securedrop/sass/modules/_flash.sass


    &.notification
-      border: 1px solid #8ed9f6
-      background-color: #e3f7fc
+      background-color: $color_notification_background


Because this is a shared module, these changes bleed into the Source Interface, while the icon changes do not. As a consequence, some Source Interface messages are now a hybrid between the old style and the new style. Case in point (rendered without border but with old icon):

Since we are updating the shared style, I think it would be reasonable to update the impacted icon(s) in the Source Interface flash template as well, and make sure that all notifications look how we want them to.

I'm actually gonna split them for now - all old style for source, new style for Journalist - as a) the SI has different notification states, so we may not want to use the same icons, b) there would probably be text updates needed in there too, and c) if there are text updates, we're adding even more to translator workflow for this release. Should also reduce release QA reqs.

If there isn't already a ticket for improving the SI flash messaging, we should put one in. Otherwise this is just scope creep, and a bit late in the process for it.

securedrop/journalist_templates/_sources_confirmation_final_modal.html

securedrop/journalist_templates/col.html

securedrop/static/js/journalist.js

eloquence · 2021-02-18T01:15:39Z

Done with my detail review for today (can take a second pass or help with some of the potential changes if desired), mostly nits from my end. I'll open up a smol docs PR now without screenshots since small aspects may still change around a little bit. Q: Will this already auto-generate screenshots via the functional tests?

- added option to delete store data while retaining a source's account - added double modal confirm for source account deletion on All Sources page - added extra functionality whe Javascript enabled to provide more information on deletion operations - updated related strings and flash messages to accurately describe impact and results of deletion operations - updated source deletion UI on single sources page - updated flash message styling and icons - replaced references to "documents" with "submissions" or "files and messages" in strings - other misc UX fixes

lgtm-com · 2021-02-18T15:52:33Z

This pull request introduces 1 alert when merging d033d0e into 88ac049 - view on LGTM.com

new alerts:

1 for Missing variable declaration

emkll

Went through the PR again, all looks good to me, one last comment for discussion before merge

emkll · 2021-02-19T19:10:58Z

securedrop/journalist_templates/flashed.html

        {% elif category == "error" %}
-        <img src="{{ url_for('static', filename='i/font-awesome/exclamation-triangle-black.png') }}" height="17" width="20">
+        <img src="{{ url_for('static', filename='i/flash-error.png') }}" height="30" width="9">


We can revisit this later

securedrop/journalist_app/col.py

emkll

Thanks @zenmonkeykstop for your patience and hard work on this one, went through the latest round of changes visually and functionally, and look good to me.

zenmonkeykstop added this to the 1.8.0 milestone Feb 5, 2021

zenmonkeykstop marked this pull request as ready for review February 5, 2021 20:52

zenmonkeykstop requested review from kushaldas and redshiftzero as code owners February 5, 2021 20:52

zenmonkeykstop removed request for kushaldas and redshiftzero February 5, 2021 21:14

zenmonkeykstop force-pushed the nellie-bly branch 2 times, most recently from d8789e8 to 123bb3a Compare February 12, 2021 21:18

zenmonkeykstop requested review from conorsch and emkll as code owners February 12, 2021 21:18

emkll self-assigned this Feb 15, 2021

emkll previously requested changes Feb 15, 2021

View reviewed changes

emkll removed their assignment Feb 15, 2021

emkll reviewed Feb 15, 2021

View reviewed changes

securedrop/sass/modules/_confirm-modal.sass Outdated Show resolved Hide resolved

zenmonkeykstop force-pushed the nellie-bly branch from 79052d7 to f74d7ad Compare February 17, 2021 15:35

emkll self-assigned this Feb 17, 2021

eloquence mentioned this pull request Feb 17, 2021

Document changes to source deletion freedomofpress/securedrop-docs#155

Closed

emkll reviewed Feb 17, 2021

View reviewed changes