Add basic message filtering in the SI #6306

zenmonkeykstop · 2022-02-25T20:06:57Z

Status

Work in progress (tests required)

Description of Changes

Fixes #6297.
Fixes #6298.

adds options in the JI instance config, and in the SI, to:

set a minimum message length for source's initial messages, as per Add message length checks to the Source Interface #6297
reject initial messages that consist of the source's codename, as per Add check for codename-only messages in Source Interface #6298

Testing

check out this branch and run make dev

minimum message length

in the JI, navigate to the Instance Config page via the Admin page
- verify that the Submission Preferences section includes a "Prevent sources from sending initial messages shorter than the minimum required length" option, currently unchecked and with the length field set to 0.
check the "Prevent sources from sending initial messages shorter than..." checkbox but do not set a length. Click Update Submission Preferences
- verify that the checkbox is unchecked and an error message is displayed asking you to set a length.
check the "Prevent sources from sending initial messages shorter than..." checkbox and set a length of between 5 and 20 chars. Click Update Submission Preferences
- verify that the checkbox is checked, the length you chose is set, and a success message is displayed.
Create a new source account on the SI, navigating through to the /lookup page
- verify that a notice is displayed under the message textfield like "If you are only sending a message, it must be at least N characters long" where N is the length you chose above
- Verify that if you try to submit too short a message, the submission fails with a flashed error like "Your first message must be at least N characters long"
- Verify that a message N characters long can be submitted successfully
- after a successful first submission, verify that the notice under the text field is no longer displayed and you can submit message shorter than N chars
- in the JI, uncheck the "Prevent sources from sending initial messages shorter than..." checkbox,, click Update Submission Prefs and verify that the length is set to zero and a success message is displayed
- Create a new source account in the SI and verify that no message length limit is mentioned or set

codename messages

in the JI, navigate to the Instance Config page via the Admin page
- verify that the Submission Preferences section includes a "Prevent sources from submitting their codename as an initial message." option, currently unchecked.
check the "Prevent sources from submitting their codename as an initial message" checkbox. Click Update Submission Preferences
- verify that the checkbox is checked and a success message is displayed.
Create a new source account on the SI, navigating through to the /lookup page
- Enter the codename as a message and click Submit - verify that the message is not submitted and an error is flashed like "Please do not submit your codename.."
- Enter anything else as a message and click Submit - verify that the message was submitted correctly
- Enter your codename again and click Submit - verify that the message was submitted correctly.

Deployment

DB migration script required, but new functionality is disabled by default.

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

legoktm · 2022-02-25T21:24:23Z

Do we also want to use <textarea minlength="...">? https://developer.mozilla.org/en-US/docs/Web/HTML/Element/textarea#attr-minlength

zenmonkeykstop · 2022-02-28T19:22:22Z

Do we also want to use <textarea minlength="...">? https://developer.mozilla.org/en-US/docs/Web/HTML/Element/textarea#attr-minlength

Yep, that'd be a nice addition.

zenmonkeykstop · 2022-03-03T17:08:21Z

Looking at that minlength addition with @eaon, it occurred to me that it would prevent initial submissions with a file attachment but no message (which are currently allowed). So, in the absence of client-side logic to allow for that case I think we can't include it except for the edge case upon an edge case of message-only configs. Length checks will be server-side only.

lgtm-com · 2022-03-04T00:32:44Z

This pull request fixes 2 alerts when merging 2fea4d7 into 2236d3d - view on LGTM.com

fixed alerts:

2 for Testing equality to None

lgtm-com · 2022-03-04T00:39:28Z

This pull request fixes 2 alerts when merging 82a76ca into 2236d3d - view on LGTM.com

fixed alerts:

2 for Testing equality to None

lgtm-com · 2022-03-04T01:53:38Z

This pull request fixes 2 alerts when merging 6b31fd7 into 2236d3d - view on LGTM.com

fixed alerts:

2 for Testing equality to None

eloquence · 2022-03-04T01:59:10Z

Thanks for all the hard work on this! A couple of initial UX notes:

Organization of new preferences is potentially confusing

There are IMO a few UX issues here:

Having the user have to set a "0" value to enable or disable a preference makes it harder for them to follow a single process for all yes/no preferences, to quickly parse the state of preferences, and to know what the previous state was when the feature is disabled.
One way to avoid this would be to have a "Prevent initial messages below a minimum character length" preference, with the actual character length either as an inline field, or as a separate, indented sub-option.
Because other preferences follow the "Prevent .." format, the fact that the "Minimum message length" preference does not, can make it feel like it is associated with the previous preference.
The actual input control only needs to accommodate 4 digits, so it can be far less wide.

Error message scrolled out of view

Can you reproduce this? If I configure a minimum length and submit a message that gets blocked, the error message is not scrolled within the viewport:

I don't see this behavior with error messages in develop (e.g., when attempting to send a blank message).

codecov-commenter · 2022-03-04T02:15:39Z

Codecov Report

Merging #6306 (e0844fd) into develop (842bfb8) will decrease coverage by 0.26%.
The diff coverage is 70.37%.

@@             Coverage Diff             @@
##           develop    #6306      +/-   ##
===========================================
- Coverage    84.03%   83.76%   -0.27%     
===========================================
  Files           60       61       +1     
  Lines         4227     4293      +66     
  Branches       511      521      +10     
===========================================
+ Hits          3552     3596      +44     
- Misses         552      571      +19     
- Partials       123      126       +3

Impacted Files	Coverage Δ
...d22043_remove_partial_index_on_valid_until_set_.py	`40.90% <40.90%> (ø)`
securedrop/journalist_app/admin.py	`88.10% <55.55%> (-1.44%)`	⬇️
securedrop/journalist_app/forms.py	`89.39% <70.58%> (-6.84%)`	⬇️
securedrop/models.py	`89.57% <81.81%> (+0.05%)`	⬆️
securedrop/source_app/main.py	`93.13% <100.00%> (+0.46%)`	⬆️
securedrop/source_app/utils.py	`93.10% <100.00%> (+1.10%)`	⬆️

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 842bfb8...e0844fd. Read the comment docs.

zenmonkeykstop · 2022-03-04T02:24:21Z

All seems doable, had planned to restrict that length field size via CSS, could probably just add an attribute directly as well.

I'm thinking that the extra para in the SI about message lengths might be what's pushing the flash error out of viewport - for neatness sake I was already thinking of moving that into the placeholder text in the textarea instead. Will push up something tomorrow.

zenmonkeykstop · 2022-03-04T18:13:15Z

@eloquence a revision with some UX-related changes addressing your points above is working its way thru CI. Note that it does not update the placeholder text in the submission textarea as I mentioned above - instead the same design language/placement as is currently used for file size limits was applied.

lgtm-com · 2022-03-04T18:16:16Z

This pull request fixes 2 alerts when merging 2939066 into 8e61004 - view on LGTM.com

fixed alerts:

2 for Testing equality to None

lgtm-com · 2022-03-07T16:23:37Z

This pull request fixes 2 alerts when merging e170d3d into 8e61004 - view on LGTM.com

fixed alerts:

2 for Testing equality to None

lgtm-com · 2022-03-07T22:41:08Z

This pull request fixes 2 alerts when merging 940edf5 into 8e61004 - view on LGTM.com

fixed alerts:

2 for Testing equality to None

lgtm-com · 2022-03-07T22:58:26Z

This pull request fixes 2 alerts when merging 75f4aad into 8e61004 - view on LGTM.com

fixed alerts:

2 for Testing equality to None

eloquence · 2022-03-08T01:37:22Z

Thanks for the changes, it's coming together nicely. I'm still noticing the issue with the error message not being scrolled into view, not sure if you've been able to reproduce / had a chance to debug that one.

The new preferences form seems to work well. I think it would be nice if it remembered the previous value of the form field even after you disable the feature, but that's definitely a nice-to-have.

Regarding the UX changes, I would suggest tweaking as follows:

Tweaks:

No background color (the gray color can suggest a "disabled" look, and does not IMO meaningfully add to the grouping; with the new wording, it seems clearer to me that these are distinct preferences)
Narrower input
Changed "minimum required" to just "minimum" ("prevent" in combination with "minimum" are IMO sufficiently clear)
Avoid repetition in the content hint, style it a bit differently from the field label

Regarding the minimum length hint, I would suggest also slightly tweaking it:

If your first submission is message-only, it must be at least 5 characters long.

->

If you are only sending a message, it must be at least 5 characters long.

Avoid the awkward construction "message-only"
Keep it simple, trying to communicate too many qualifiers means the message is more likely to be ignored as "fine print".

zenmonkeykstop · 2022-03-08T15:43:27Z

@eloquence further tweaks applied.

lgtm-com · 2022-03-08T15:52:55Z

This pull request fixes 2 alerts when merging e5a565c into 842bfb8 - view on LGTM.com

fixed alerts:

2 for Testing equality to None

securedrop/sass/source.sass

eaon · 2022-03-10T19:51:42Z

Regarding #flashed viewport jump issue, I've not paid enough attention to this behaviour before, but I just confirmed that this happens to me on the demo instance. I opened #6333 so we can separate that problem from the PR

lgtm-com · 2022-03-10T20:13:52Z

This pull request fixes 2 alerts when merging e7c1d7e into be16809 - view on LGTM.com

fixed alerts:

2 for Testing equality to None

legoktm · 2022-03-10T20:18:00Z

Test plan all checks out (and pointed out a gap in the tests I wrote)!

zenmonkeykstop · 2022-03-10T22:24:13Z

(for whoever ends up merging, this can probably be rebased into a single commit first)

eloquence · 2022-03-10T23:21:54Z

securedrop/source_app/main.py

+                flash(gettext(
+                    "Your first message must be at least {} characters long.".format(min_len)),
+                    "error")
+                return redirect(f"{url_for('main.lookup')}#flashed")


Per #6336 I think we'll want to now redirect without the #flashed fragment.

eloquence · 2022-03-10T23:22:11Z

securedrop/source_app/main.py

+                    escape(gettext("Keep your codename secret, and use it to log in later"
+                                   " to check for replies."))
+                    )), "error")
+                return redirect(f"{url_for('main.lookup')}#flashed")


eloquence · 2022-03-11T01:03:28Z

I've pushed up a branch rebased to develop, with the anchor tweaks above, to add-message-filtering-redux, in case you want to use it. (I did not want to force push to this branch in case you have local changes.)

legoktm · 2022-03-11T01:43:52Z

I'm assuming Kev had nothing left locally, so I'm going to cherry-pick 540a693 over to this branch, push, then squash+rebase it all and push again.

lgtm-com · 2022-03-11T01:51:51Z

This pull request fixes 2 alerts when merging a2ee4ef into 970aab5 - view on LGTM.com

fixed alerts:

2 for Testing equality to None

lgtm-com · 2022-03-11T02:13:17Z

This pull request fixes 2 alerts when merging 500531d into 970aab5 - view on LGTM.com

fixed alerts:

2 for Testing equality to None

legoktm

I think we're set to land this but I'd like someone else to confirm that they reviewed the test cases I wrote (so I'm not merging them without review), and double check my squash and commit message.

cfm

Reviewed test cases and rebase/squash at @legoktm's request. All looks good to me except for the following nit about the commit message on the squashed 500531d:

When trying to do the schema change for the InstanceConfig table, it was
discovered that SQLAlchemy/Alembic don't support the partial index on
the table, so it's dropped and the valid_until column is now
nullable=False. The current/active config is represented by a
valid_until of the unix epoch (0).

On first reading, if one isn't already familiar with this problem, it might be unclear whether the clause beginning "so it's dropped" describes (a) SQLAlchemy's behavior in not supporting the partial index prior to this commit or (b) the fix made by this commit. Up to you, @legoktm, whether it's worth revising, but since the commit (once merged) is the immutable thing here I thought I'd mention it. :-)

- Updated instance_config table, removing partial index on valid_until field that was used to force NULL values to be unique, and using unix epoch start as the valid_until value for the active configuration instead. (Partial indexes are not fully supported by the Alembic version currently in use.) - Added optional message filtering: - A minimum length can now be set for a source's initial message. Messages shorter than this length are rejected with an error in the SI. This feature is managed via the Admin interface and is disabled by default. - Initial messages can now be rejected if their content matches the source's codename. This feature is managed via the Admin interface and is disabled by default. Fixes #6297. Fixes #6298.

zenmonkeykstop · 2022-03-11T15:50:58Z

On first reading, if one isn't already familiar with this problem, it might be unclear whether the clause beginning "so it's dropped" describes (a) SQLAlchemy's behavior in not supporting the partial index prior to this commit or (b) the fix made by this commit. Up to you, @legoktm, whether it's worth revising, but since the commit (once merged) is the immutable thing here I thought I'd mention it. :-)

Agreed, reworded.

lgtm-com · 2022-03-11T15:53:06Z

This pull request fixes 2 alerts when merging fb36b50 into 970aab5 - view on LGTM.com

fixed alerts:

2 for Testing equality to None

conorsch

Re-blessing for merge after requested changes were made. 😎

zenmonkeykstop force-pushed the add-message-filtering branch from 6026689 to 2a37036 Compare February 28, 2022 19:12

zenmonkeykstop force-pushed the add-message-filtering branch 2 times, most recently from 0955939 to 0c5db78 Compare March 2, 2022 16:28

zenmonkeykstop force-pushed the add-message-filtering branch from fc0c7a5 to 2fea4d7 Compare March 4, 2022 00:26

zenmonkeykstop force-pushed the add-message-filtering branch from 2fea4d7 to 82a76ca Compare March 4, 2022 00:33

zenmonkeykstop force-pushed the add-message-filtering branch from 82a76ca to 6b31fd7 Compare March 4, 2022 01:47

zenmonkeykstop force-pushed the add-message-filtering branch from 2939066 to e170d3d Compare March 7, 2022 16:17

zenmonkeykstop changed the title ~~Add message filtering~~ Add basic message filtering in the SI Mar 7, 2022

zenmonkeykstop force-pushed the add-message-filtering branch 2 times, most recently from bd93db1 to 940edf5 Compare March 7, 2022 22:33

zenmonkeykstop force-pushed the add-message-filtering branch from 940edf5 to 75f4aad Compare March 7, 2022 22:50

zenmonkeykstop force-pushed the add-message-filtering branch from 75f4aad to e5a565c Compare March 8, 2022 15:43

eaon reviewed Mar 8, 2022

View reviewed changes

securedrop/sass/source.sass Outdated Show resolved Hide resolved

legoktm force-pushed the add-message-filtering branch from 9e8e6f7 to e7c1d7e Compare March 10, 2022 20:07

eaon mentioned this pull request Mar 10, 2022

SI: focusing on #flashed message after submitting sometimes scrolls the viewport too far, may obscure message #6333

Closed

zenmonkeykstop marked this pull request as ready for review March 10, 2022 22:24

zenmonkeykstop requested a review from a team as a code owner March 10, 2022 22:24

eloquence reviewed Mar 10, 2022

View reviewed changes

legoktm force-pushed the add-message-filtering branch from a2ee4ef to 500531d Compare March 11, 2022 02:06

legoktm previously approved these changes Mar 11, 2022

View reviewed changes

cfm previously approved these changes Mar 11, 2022

View reviewed changes

zenmonkeykstop dismissed stale reviews from cfm and legoktm via fb36b50 March 11, 2022 15:47

zenmonkeykstop force-pushed the add-message-filtering branch from 500531d to fb36b50 Compare March 11, 2022 15:47

conorsch self-requested a review March 11, 2022 19:52

conorsch approved these changes Mar 11, 2022

View reviewed changes

conorsch merged commit 4b2399f into develop Mar 11, 2022

legoktm mentioned this pull request Mar 14, 2022

Alembic autogeneration fails when instance_config table is changed. #6312

Closed

zenmonkeykstop mentioned this pull request Mar 17, 2022

Release SecureDrop 2.3.0 #6334

Closed

34 tasks

eloquence mentioned this pull request Mar 17, 2022

Document message filtering settings freedomofpress/securedrop-docs#319

Closed

legoktm deleted the add-message-filtering branch January 9, 2025 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic message filtering in the SI #6306

Add basic message filtering in the SI #6306

zenmonkeykstop commented Feb 25, 2022 •

edited by legoktm

Loading

legoktm commented Feb 25, 2022

zenmonkeykstop commented Feb 28, 2022

zenmonkeykstop commented Mar 3, 2022

lgtm-com bot commented Mar 4, 2022

lgtm-com bot commented Mar 4, 2022

lgtm-com bot commented Mar 4, 2022

eloquence commented Mar 4, 2022

codecov-commenter commented Mar 4, 2022 •

edited

Loading

zenmonkeykstop commented Mar 4, 2022 •

edited

Loading

zenmonkeykstop commented Mar 4, 2022

lgtm-com bot commented Mar 4, 2022

lgtm-com bot commented Mar 7, 2022

lgtm-com bot commented Mar 7, 2022

lgtm-com bot commented Mar 7, 2022

eloquence commented Mar 8, 2022 •

edited

Loading

zenmonkeykstop commented Mar 8, 2022

lgtm-com bot commented Mar 8, 2022

eaon commented Mar 10, 2022 •

edited

Loading

lgtm-com bot commented Mar 10, 2022

legoktm commented Mar 10, 2022

zenmonkeykstop commented Mar 10, 2022

eloquence Mar 10, 2022

eloquence Mar 10, 2022

eloquence commented Mar 11, 2022

legoktm commented Mar 11, 2022

lgtm-com bot commented Mar 11, 2022

lgtm-com bot commented Mar 11, 2022

legoktm left a comment

cfm left a comment

zenmonkeykstop commented Mar 11, 2022

lgtm-com bot commented Mar 11, 2022

conorsch left a comment

Add basic message filtering in the SI #6306

Add basic message filtering in the SI #6306

Conversation

zenmonkeykstop commented Feb 25, 2022 • edited by legoktm Loading

Status

Description of Changes

Testing

minimum message length

codename messages

Deployment

Checklist

If you made changes to the server application code:

If you made non-trivial code changes:

legoktm commented Feb 25, 2022

zenmonkeykstop commented Feb 28, 2022

zenmonkeykstop commented Mar 3, 2022

lgtm-com bot commented Mar 4, 2022

lgtm-com bot commented Mar 4, 2022

lgtm-com bot commented Mar 4, 2022

eloquence commented Mar 4, 2022

Organization of new preferences is potentially confusing

Error message scrolled out of view

codecov-commenter commented Mar 4, 2022 • edited Loading

Codecov Report

zenmonkeykstop commented Mar 4, 2022 • edited Loading

zenmonkeykstop commented Mar 4, 2022

lgtm-com bot commented Mar 4, 2022

lgtm-com bot commented Mar 7, 2022

lgtm-com bot commented Mar 7, 2022

lgtm-com bot commented Mar 7, 2022

eloquence commented Mar 8, 2022 • edited Loading

zenmonkeykstop commented Mar 8, 2022

lgtm-com bot commented Mar 8, 2022

eaon commented Mar 10, 2022 • edited Loading

lgtm-com bot commented Mar 10, 2022

legoktm commented Mar 10, 2022

zenmonkeykstop commented Mar 10, 2022

eloquence Mar 10, 2022

Choose a reason for hiding this comment

eloquence Mar 10, 2022

Choose a reason for hiding this comment

eloquence commented Mar 11, 2022

legoktm commented Mar 11, 2022

lgtm-com bot commented Mar 11, 2022

lgtm-com bot commented Mar 11, 2022

legoktm left a comment

Choose a reason for hiding this comment

cfm left a comment

Choose a reason for hiding this comment

zenmonkeykstop commented Mar 11, 2022

lgtm-com bot commented Mar 11, 2022

conorsch left a comment

Choose a reason for hiding this comment

zenmonkeykstop commented Feb 25, 2022 •

edited by legoktm

Loading

codecov-commenter commented Mar 4, 2022 •

edited

Loading

zenmonkeykstop commented Mar 4, 2022 •

edited

Loading

eloquence commented Mar 8, 2022 •

edited

Loading

eaon commented Mar 10, 2022 •

edited

Loading