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

make file submissions dis/allowable #4879

Merged

Conversation

@wbaid
Copy link
Contributor

wbaid commented Sep 29, 2019

Status

Ready for review

  • See next section for tests outstanding

Status of requested changes (since b5d493c)

  1. fix: "Error strings [...], and also in the inline comment
    securedrop/source_templates/lookup.html, which could be confusing
    to sources." (1, 2)

  2. fix: "Bug: textarea width on Source Interface"

  3. refactor: "Proposed alternative language" for "Instance Configuration" page

    1. fix: "UX issue: 'Update' button on Instance Configuration page"
    2. fix: "UX issue: 'Document Uploads', 'Allow' language on Instance
      Configuration page"
  4. refactor: versioned instance_config

    1. "one configuration option per column"
    2. migration to add column sets default value
    3. "use the configuration option that has null valid_until. When we
      update a config, we store the historical configuration entry by
      setting valid_until=datetime.datetime.utcnow(), then store the new
      configuration with null valid_until."
    • test: migrations
  5. test: integration test

Description of Changes

  1. InstanceConfig versioned key-value store as outlined

    • load_instance_config() sets each app's app.instance_config
      (via @before_request)
  2. Source interface checks
    app.instance_config.allow_document_uploads; if False:

    1. /lookup hides the file input and
    2. changes its heading to "Submit Messages" (rather than "Submit
      Files or Messages") and other strings accordingly; and
    3. /submit skips processing of request.files.
    4. /metadata returns this setting as allow_document_uploads.
  3. Journalist interface:

    1. /admin/config adds a section "Submission Preferences"; and
    2. /admin/update-submission-preferences updates
      InstanceConfig.allow_document_uploads.

Testing

Test by toggling the Prevent sources from uploading documents
checkbox in the "Submission Preferences" section of the "Instance
Config" view.

  1. If unchecked (default): Observe no changes from current behavior.

  2. If checked: Observe the changes described in (2) and (3) above.

Deployment

No action required. After upgrading, administrators may use the
"Submission Preferences" section of the "Instance Config" view.

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make test) pass in the
    development container
    • Fix tests/test_i18n.py::test_verify_default_locale_en_us_if_not_defined_in_config (expected by end of day Monday, October 21)
@lgtm-com

This comment has been minimized.

Copy link

lgtm-com bot commented Sep 29, 2019

This pull request introduces 1 alert when merging 3b028ac into 94e4a9d - view on LGTM.com

new alerts:

  • 1 for Implicit string concatenation in a list
@wbaid wbaid force-pushed the wbaid:config-allow-document-uploads branch from 3b028ac to 3b4777f Sep 29, 2019
@lgtm-com

This comment has been minimized.

Copy link

lgtm-com bot commented Sep 29, 2019

This pull request introduces 1 alert when merging 3b4777f into 94e4a9d - view on LGTM.com

new alerts:

  • 1 for Implicit string concatenation in a list
@wbaid wbaid force-pushed the wbaid:config-allow-document-uploads branch from 3b4777f to 55bda5b Sep 29, 2019
@lgtm-com

This comment has been minimized.

Copy link

lgtm-com bot commented Sep 29, 2019

This pull request introduces 1 alert when merging 55bda5b into 94e4a9d - view on LGTM.com

new alerts:

  • 1 for Implicit string concatenation in a list
@zenmonkeykstop

This comment has been minimized.

Copy link
Contributor

zenmonkeykstop commented Oct 1, 2019

Thanks for this PR! It looks like a valuable feature. There might be issues safely applying the config change (or lack thereof) to older instances - one way around this would be to make this feature configurable via the Journalist Interface (similar to the custom logo option). The SD core team's unavailable right now, but we'll discuss and review early next week and add more feedback at that point.

@eloquence eloquence added this to Ready for review in SecureDrop Team Board Oct 7, 2019
@eloquence eloquence moved this from Ready for review to Near Term - SecureDrop Core in SecureDrop Team Board Oct 7, 2019
@zenmonkeykstop

This comment has been minimized.

Copy link
Contributor

zenmonkeykstop commented Oct 7, 2019

Hi @wbaid - it looks like having this configured via the web interface is the preferred option for the core team. We'll be entering feature freeze for v1.1.0 tomorrow so will be unlikely to be able to suggest or incorporate any changes for that version, but this should be on the list of changes for the following v1.2.0 release.

If you have availability and would like to talk about the change, the Securedrop gitter forum is active at https://gitter.im/freedomofpress/securedrop , and we host weekday public standup meetings at 10AM Pacific/1PM Eastern at https://meet.google.com/ekb-kkhf-mrk

@wbaid

This comment has been minimized.

Copy link
Contributor Author

wbaid commented Oct 8, 2019

@redshiftzero

This comment has been minimized.

Copy link
Member

redshiftzero commented Oct 8, 2019

I agree with @zenmonkeykstop that this seems like a useful feature. Doing 1 (adding a new table called e.g. instance_config that we can use to store key/value config values) I think is the best way to go here. In that scenario the update process from the admin's perspective is better since:

  1. immediate, i.e. no Ansible playbook run required,
  2. has nicer UX since using the CLI is not needed,
  3. and it's easier to coordinate changes in the case of multiple admins.
@wbaid

This comment has been minimized.

Copy link
Contributor Author

wbaid commented Oct 9, 2019

@eloquence eloquence moved this from Near Term - SecureDrop Core to Current Sprint - 10/9-10/23 in SecureDrop Team Board Oct 9, 2019
@eloquence

This comment has been minimized.

Copy link
Contributor

eloquence commented Oct 9, 2019

Thanks @wbaid -- we've set aside time in our current sprint (10/9 to 10/23) to help get this over the finish line if you have time to work on it from your end.

@eloquence eloquence moved this from Current Sprint - 10/9-10/23 to In Development in SecureDrop Team Board Oct 10, 2019
@wbaid

This comment has been minimized.

Copy link
Contributor Author

wbaid commented Oct 10, 2019

@wbaid

This comment has been minimized.

Copy link
Contributor Author

wbaid commented Oct 18, 2019

wbaid added 6 commits Oct 20, 2019
…nippet
…o files
…OADS = False
@wbaid wbaid force-pushed the wbaid:config-allow-document-uploads branch from 55bda5b to 0db9f22 Oct 21, 2019
@wbaid wbaid force-pushed the wbaid:config-allow-document-uploads branch from 4a95f8d to 745009c Nov 4, 2019
@wbaid

This comment has been minimized.

Copy link
Contributor Author

wbaid commented Nov 4, 2019

@lgtm-com

This comment has been minimized.

Copy link

lgtm-com bot commented Nov 4, 2019

This pull request introduces 2 alerts when merging 745009c into 486853c - view on LGTM.com

new alerts:

  • 1 for Testing equality to None
  • 1 for Unused import
@wbaid

This comment has been minimized.

Copy link
Contributor Author

wbaid commented Nov 9, 2019

@eloquence eloquence moved this from Under Review to In Development in SecureDrop Team Board Nov 13, 2019
@eloquence eloquence moved this from In Development to Current Sprint - 11/6-11/20 in SecureDrop Team Board Nov 14, 2019
@wbaid

This comment has been minimized.

Copy link
Contributor Author

wbaid commented Nov 19, 2019

@redshiftzero

This comment has been minimized.

Copy link
Member

redshiftzero commented Nov 19, 2019

heads up @wbaid it looks like there are some alembic tests failures occurring in CI related to this diff

Copy link
Member

redshiftzero left a comment

i just took a spin through this diff and tested in a dev container, the previous UX feedback is incorporated and the versioned instance_config table we discussed is working as advertised:

redshiftzero@8dd9552eef95:/var/lib/securedrop$ sqlite3 db.sqlite
SQLite version 3.11.0 2016-02-15 17:29:24
Enter ".help" for usage hints.
sqlite> select * from instance_config;
1|2019-11-19 19:05:02.421767|1
2|2019-11-19 19:17:31.742951|0
3|2019-11-19 19:17:45.151717|1
4||0

i think once we get the tests passing and add the functional test coverage this should be good to go (btw if you want to make the alembic migrations a bit easier to address you can just squash the two migrations into one)

to be honest, this is unlikely to make it into the merge window for 1.2.0 but is very likely to make it into 1.3.0

@wbaid wbaid force-pushed the wbaid:config-allow-document-uploads branch from 745009c to 1435d51 Nov 19, 2019
@wbaid

This comment has been minimized.

Copy link
Contributor Author

wbaid commented Nov 19, 2019

@lgtm-com

This comment has been minimized.

Copy link

lgtm-com bot commented Nov 19, 2019

This pull request introduces 1 alert when merging 1435d51 into ed014be - view on LGTM.com

new alerts:

  • 1 for Testing equality to None
@redshiftzero

This comment has been minimized.

Copy link
Member

redshiftzero commented Nov 19, 2019

oh nice thanks for the fast update! Just the integration and unit coverage is required for merge given all the work you've put in here. Let's consider the functional/selenium based tests as nice to have and we can add as a followup issue for another contributor

wbaid added 2 commits Nov 3, 2019
…lumn"
…t_uploads
@wbaid wbaid force-pushed the wbaid:config-allow-document-uploads branch from 1435d51 to ab25a8f Nov 19, 2019
@wbaid

This comment has been minimized.

Copy link
Contributor Author

wbaid commented Nov 19, 2019

@lgtm-com

This comment has been minimized.

Copy link

lgtm-com bot commented Nov 19, 2019

This pull request introduces 1 alert when merging ab25a8f into ed014be - view on LGTM.com

new alerts:

  • 1 for Testing equality to None
@eloquence

This comment has been minimized.

Copy link
Contributor

eloquence commented Nov 20, 2019

Thanks for making all the UX changes previously discussed, @wbaid -- as @redshiftzero noted, it looks great from a UX perspective! The only thing we may want to track as a follow-up enhancement is adding a flashed message after you click "Update Submission Preferences", to let the user know that the settings have been successfully modified. But this is clearly not a blocker for landing this feature.

Copy link
Member

redshiftzero left a comment

new integration tests are good, thank you for those and the contribution @wbaid!

This PR looks ready to me so I'm going to approve and merge this now - so it will be included in 1.2.0 (final release planned for December 3rd).

@redshiftzero redshiftzero merged commit 7b76c60 into freedomofpress:develop Nov 20, 2019
9 checks passed
9 checks passed
LGTM analysis: JavaScript No new or fixed alerts
Details
LGTM analysis: Python 1 new alert
Details
ci/circleci: admin-tests Your tests passed on CircleCI!
Details
ci/circleci: app-tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: staging-test-with-rebase Your tests passed on CircleCI!
Details
ci/circleci: static-analysis-and-no-known-cves Your tests passed on CircleCI!
Details
ci/circleci: translation-tests Your tests passed on CircleCI!
Details
ci/circleci: updater-gui-tests Your tests passed on CircleCI!
Details
SecureDrop Team Board automation moved this from Under Review to Done Nov 20, 2019
@wbaid

This comment has been minimized.

Copy link
Contributor Author

wbaid commented Nov 20, 2019

@zenmonkeykstop zenmonkeykstop mentioned this pull request Nov 22, 2019
23 of 24 tasks complete
@rocodes rocodes mentioned this pull request Nov 27, 2019
33 of 35 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.