From a420eef1f5205c5c2d54d4eb9b84f2f2f6fc9131 Mon Sep 17 00:00:00 2001 From: Whistleblower Aid <54696764+wbaid@users.noreply.github.com> Date: Sun, 20 Oct 2019 17:21:34 -0400 Subject: [PATCH 01/12] feat: InstanceConfig key-value store updates app.config --- ...779_add_instance_config_key_value_store.py | 32 +++++++++++++++ securedrop/models.py | 14 ++++++- securedrop/source_app/__init__.py | 11 ++++- .../migrations/migration_fef03c1ec779.py | 40 +++++++++++++++++++ 4 files changed, 95 insertions(+), 2 deletions(-) create mode 100644 securedrop/alembic/versions/fef03c1ec779_add_instance_config_key_value_store.py create mode 100644 securedrop/tests/migrations/migration_fef03c1ec779.py diff --git a/securedrop/alembic/versions/fef03c1ec779_add_instance_config_key_value_store.py b/securedrop/alembic/versions/fef03c1ec779_add_instance_config_key_value_store.py new file mode 100644 index 0000000000..2fd42c03d0 --- /dev/null +++ b/securedrop/alembic/versions/fef03c1ec779_add_instance_config_key_value_store.py @@ -0,0 +1,32 @@ +"""add instance_config key-value store + +Revision ID: fef03c1ec779 +Revises: 3da3fcab826a +Create Date: 2019-10-21 00:43:19.477835 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = 'fef03c1ec779' +down_revision = '3da3fcab826a' +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_table('instance_config', + sa.Column('name', sa.String(), nullable=False), + sa.Column('value', sa.JSON(), nullable=True), + sa.PrimaryKeyConstraint('name') + ) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_table('instance_config') + # ### end Alembic commands ### diff --git a/securedrop/models.py b/securedrop/models.py index 0b083824cd..1f09970fbd 100644 --- a/securedrop/models.py +++ b/securedrop/models.py @@ -17,7 +17,7 @@ from passlib.hash import argon2 from sqlalchemy import ForeignKey from sqlalchemy.orm import relationship, backref -from sqlalchemy import Column, Integer, String, Boolean, DateTime, LargeBinary +from sqlalchemy import Column, Integer, String, Boolean, DateTime, LargeBinary, JSON from sqlalchemy.orm.exc import MultipleResultsFound, NoResultFound from db import db @@ -739,3 +739,15 @@ class RevokedToken(db.Model): id = Column(Integer, primary_key=True) journalist_id = Column(Integer, ForeignKey('journalists.id')) token = db.Column(db.Text, nullable=False, unique=True) + + +class InstanceConfig(db.Model): + '''Key-value store of settings configurable from the journalist interface. + ''' + + __tablename__ = 'instance_config' + name = Column(String, primary_key=True) + value = Column(JSON) + + def __repr__(self): + return "" % (self.name, self.value) diff --git a/securedrop/source_app/__init__.py b/securedrop/source_app/__init__.py index 5c671bd5ca..550e407a93 100644 --- a/securedrop/source_app/__init__.py +++ b/securedrop/source_app/__init__.py @@ -14,7 +14,7 @@ from crypto_util import CryptoUtil from db import db -from models import Source +from models import InstanceConfig, Source from request_that_secures_file_uploads import RequestThatSecuresFileUploads from source_app import main, info, api from source_app.decorators import ignore_static @@ -130,6 +130,15 @@ def check_tor2web(): .format(url=url_for('info.tor2web_warning'))), "banner-warning") + @app.before_request + @ignore_static + def load_instance_config(): + """Update app.config from the InstanceConfig table.""" + + instance_config = InstanceConfig.query.all() + settings = dict(map(lambda x: (x.name, x.value), instance_config)) + app.config.from_mapping(settings) + @app.before_request @ignore_static def setup_g(): diff --git a/securedrop/tests/migrations/migration_fef03c1ec779.py b/securedrop/tests/migrations/migration_fef03c1ec779.py new file mode 100644 index 0000000000..e16cc53140 --- /dev/null +++ b/securedrop/tests/migrations/migration_fef03c1ec779.py @@ -0,0 +1,40 @@ +from sqlalchemy import text +from sqlalchemy.exc import OperationalError + +from db import db +from journalist_app import create_app + + +instance_config_sql = "SELECT * FROM instance_config" + + +class UpgradeTester: + def __init__(self, config): + self.config = config + self.app = create_app(config) + + def load_data(self): + pass + + def check_upgrade(self): + with self.app.app_context(): + db.engine.execute(text(instance_config_sql)).fetchall() + + +class DowngradeTester: + def __init__(self, config): + self.config = config + self.app = create_app(config) + + def load_data(self): + pass + + def check_downgrade(self): + with self.app.app_context(): + try: + db.engine.execute(text(instance_config_sql)).fetchall() + + # The SQLite driver appears to return this rather than the + # expected NoSuchTableError. + except OperationalError: + pass From f820b0452d15cf488e25be386c7ede63623d30fc Mon Sep 17 00:00:00 2001 From: Whistleblower Aid <54696764+wbaid@users.noreply.github.com> Date: Sun, 20 Oct 2019 18:25:28 -0400 Subject: [PATCH 02/12] feat(/lookup,/submit): disable file submissions if ALLOW_DOCUMENT_UPLOADS = False --- securedrop/source_app/main.py | 9 ++++++++- securedrop/source_templates/lookup.html | 2 ++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/securedrop/source_app/main.py b/securedrop/source_app/main.py index 87b70502e9..1246b89434 100644 --- a/securedrop/source_app/main.py +++ b/securedrop/source_app/main.py @@ -83,6 +83,9 @@ def create(): @view.route('/lookup', methods=('GET',)) @login_required def lookup(): + allow_document_uploads = current_app.config.get( + 'ALLOW_DOCUMENT_UPLOADS', + True) replies = [] source_inbox = Reply.query.filter(Reply.source_id == g.source.id) \ .filter(Reply.deleted_by_source == False).all() # noqa @@ -121,6 +124,7 @@ def lookup(): return render_template( 'lookup.html', + allow_document_uploads=allow_document_uploads, codename=g.codename, replies=replies, flagged=g.source.flagged, @@ -131,9 +135,12 @@ def lookup(): @view.route('/submit', methods=('POST',)) @login_required def submit(): + allow_document_uploads = current_app.config.get( + 'ALLOW_DOCUMENT_UPLOADS', + True) msg = request.form['msg'] fh = None - if 'fh' in request.files: + if allow_document_uploads and 'fh' in request.files: fh = request.files['fh'] # Don't submit anything if it was an "empty" submission. #878 diff --git a/securedrop/source_templates/lookup.html b/securedrop/source_templates/lookup.html index e0793d3a49..af0fe3988e 100644 --- a/securedrop/source_templates/lookup.html +++ b/securedrop/source_templates/lookup.html @@ -26,11 +26,13 @@

{{ gettext('Submit Files or Messages') }}

+{% if allow_document_uploads %}

{{ gettext('Maximum upload size: 500 MB') }}

+{% endif %}
From 024c88ddf0e776e79065ef0eb11ea7a882b515d7 Mon Sep 17 00:00:00 2001 From: Whistleblower Aid <54696764+wbaid@users.noreply.github.com> Date: Sat, 28 Sep 2019 20:29:17 -0400 Subject: [PATCH 03/12] feat(/lookup): if uploads are disallowed, make div.message fill div.snippet --- securedrop/sass/modules/_snippet.sass | 3 +++ securedrop/source_templates/lookup.html | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/securedrop/sass/modules/_snippet.sass b/securedrop/sass/modules/_snippet.sass index 03abdbe3ef..f51b78aa7e 100644 --- a/securedrop/sass/modules/_snippet.sass +++ b/securedrop/sass/modules/_snippet.sass @@ -43,3 +43,6 @@ &:focus outline: none + + .wide + width: 100% diff --git a/securedrop/source_templates/lookup.html b/securedrop/source_templates/lookup.html index af0fe3988e..952aa6c133 100644 --- a/securedrop/source_templates/lookup.html +++ b/securedrop/source_templates/lookup.html @@ -33,7 +33,7 @@

{{ gettext('Submit Files or Messages') }}

{{ gettext('Maximum upload size: 500 MB') }}

{% endif %} -
+
From 94f8ff075fd0a537c95a2e1632af1effdf472621 Mon Sep 17 00:00:00 2001 From: Whistleblower Aid <54696764+wbaid@users.noreply.github.com> Date: Sat, 28 Sep 2019 20:29:22 -0400 Subject: [PATCH 04/12] feat(/lookup): if uploads are disallowed, remove most UI references to files --- securedrop/source_templates/lookup.html | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/securedrop/source_templates/lookup.html b/securedrop/source_templates/lookup.html index 952aa6c133..7b95b59de3 100644 --- a/securedrop/source_templates/lookup.html +++ b/securedrop/source_templates/lookup.html @@ -15,8 +15,12 @@ {% endif %} +{% if allow_document_uploads %}

{{ gettext('Submit Files or Messages') }}

{{ gettext('You can submit any kind of file, a message, or both.') }}

+{% else %} +

{{ gettext('Submit Messages') }}

+{% endif %}

{{ gettext('If you are already familiar with GPG, you can optionally encrypt your files and messages with our public key before submission. Files are encrypted as they are received by SecureDrop.').format(url=url_for('info.download_journalist_pubkey')) }} {{ gettext('Learn more.').format(url=url_for('info.why_download_journalist_pubkey')) }}

From 9062742d0df9ff938b4ad87a2beec4881a8d3153 Mon Sep 17 00:00:00 2001 From: Whistleblower Aid <54696764+wbaid@users.noreply.github.com> Date: Sun, 20 Oct 2019 20:31:42 -0400 Subject: [PATCH 05/12] feat(/admin/config): configurable ALLOW_DOCUMENT_UPLOADS --- securedrop/journalist_app/__init__.py | 10 +++++- securedrop/journalist_app/admin.py | 36 +++++++++++++++++---- securedrop/journalist_app/forms.py | 4 +++ securedrop/journalist_templates/config.html | 17 +++++++++- securedrop/tests/test_journalist.py | 23 ++++++++++++- 5 files changed, 80 insertions(+), 10 deletions(-) diff --git a/securedrop/journalist_app/__init__.py b/securedrop/journalist_app/__init__.py index bee65789c6..21d7aeecd0 100644 --- a/securedrop/journalist_app/__init__.py +++ b/securedrop/journalist_app/__init__.py @@ -20,7 +20,7 @@ from journalist_app.utils import (get_source, logged_in, JournalistInterfaceSessionInterface, cleanup_expired_revoked_tokens) -from models import Journalist +from models import InstanceConfig, Journalist from store import Storage import typing @@ -124,6 +124,14 @@ def _handle_http_exception(error): def expire_blacklisted_tokens(): return cleanup_expired_revoked_tokens() + @app.before_request + def load_instance_config(): + """Update app.config from the InstanceConfig table.""" + + instance_config = InstanceConfig.query.all() + settings = dict(map(lambda x: (x.name, x.value), instance_config)) + app.config.from_mapping(settings) + @app.before_request def setup_g(): # type: () -> Optional[Response] diff --git a/securedrop/journalist_app/admin.py b/securedrop/journalist_app/admin.py index 88b7d7c0e8..3d1bacb681 100644 --- a/securedrop/journalist_app/admin.py +++ b/securedrop/journalist_app/admin.py @@ -9,11 +9,12 @@ from sqlalchemy.orm.exc import NoResultFound from db import db -from models import Journalist, InvalidUsernameException, FirstOrLastNameError, PasswordError +from models import (InstanceConfig, Journalist, InvalidUsernameException, + FirstOrLastNameError, PasswordError) from journalist_app.decorators import admin_required from journalist_app.utils import (make_password, commit_account_changes, set_diceware_password, validate_hotp_secret, revoke_token) -from journalist_app.forms import LogoForm, NewUserForm +from journalist_app.forms import AllowDocumentUploadsForm, LogoForm, NewUserForm def make_blueprint(config): @@ -28,9 +29,13 @@ def index(): @view.route('/config', methods=('GET', 'POST')) @admin_required def manage_config(): - form = LogoForm() - if form.validate_on_submit(): - f = form.logo.data + allow_document_uploads_form = AllowDocumentUploadsForm( + allow_document_uploads=current_app.config.get( + 'ALLOW_DOCUMENT_UPLOADS', + True)) + logo_form = LogoForm() + if logo_form.validate_on_submit(): + f = logo_form.logo.data custom_logo_filepath = os.path.join(current_app.static_folder, 'i', 'custom_logo.png') try: @@ -42,10 +47,27 @@ def manage_config(): finally: return redirect(url_for("admin.manage_config")) else: - for field, errors in list(form.errors.items()): + for field, errors in list(logo_form.errors.items()): for error in errors: flash(error, "logo-error") - return render_template("config.html", form=form) + return render_template("config.html", + allow_document_uploads_form=allow_document_uploads_form, + logo_form=logo_form) + + @view.route('/set-allow-document-uploads', methods=['POST']) + @admin_required + def set_allow_document_uploads(): + form = AllowDocumentUploadsForm() + if form.validate_on_submit(): + # Upsert ALLOW_DOCUMENT_UPLOADS: + setting = InstanceConfig.query.get('ALLOW_DOCUMENT_UPLOADS') + if not setting: + setting = InstanceConfig(name='ALLOW_DOCUMENT_UPLOADS') + setting.value = bool(request.form.get('allow_document_uploads')) + + db.session.add(setting) + db.session.commit() + return redirect(url_for('admin.manage_config')) @view.route('/add', methods=('GET', 'POST')) @admin_required diff --git a/securedrop/journalist_app/forms.py b/securedrop/journalist_app/forms.py index 367f6cfbdb..1241b31dfa 100644 --- a/securedrop/journalist_app/forms.py +++ b/securedrop/journalist_app/forms.py @@ -64,6 +64,10 @@ class ReplyForm(FlaskForm): ) +class AllowDocumentUploadsForm(FlaskForm): + allow_document_uploads = BooleanField('allow_document_uploads') + + class LogoForm(FlaskForm): logo = FileField(validators=[ FileRequired(message=gettext('File required.')), diff --git a/securedrop/journalist_templates/config.html b/securedrop/journalist_templates/config.html index e9424cdb63..542684ec0c 100644 --- a/securedrop/journalist_templates/config.html +++ b/securedrop/journalist_templates/config.html @@ -29,7 +29,7 @@

{{ gettext('Logo Image') }}

- {{ form.logo(id="logo-upload") }} + {{ logo_form.logo(id="logo-upload") }}

@@ -41,4 +41,19 @@
{% include 'logo_upload_flashed.html' %} +
+ +

{{ gettext('Document Uploads') }}

+ +
+ +

+ {{ allow_document_uploads_form.allow_document_uploads() }} + +

+ +
+ {% endblock %} diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index 3e429ddc3e..5d43066523 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -25,7 +25,7 @@ from sdconfig import SDConfig, config from db import db -from models import (InvalidPasswordLength, Journalist, Reply, Source, +from models import (InvalidPasswordLength, InstanceConfig, Journalist, Reply, Source, Submission) from .utils.instrument import InstrumentedApp @@ -1287,6 +1287,27 @@ def test_admin_add_user_integrity_error(journalist_app, test_admin, mocker): "None\n[SQL: STATEMENT]\n[parameters: 'PARAMETERS']") in log_event +def test_allow_document_uploads(journalist_app, test_admin): + with journalist_app.test_client() as app: + _login_user(app, test_admin['username'], test_admin['password'], + test_admin['otp_secret']) + form = journalist_app_module.forms.AllowDocumentUploadsForm( + allow_document_uploads=True) + app.post(url_for('admin.set_allow_document_uploads'), + data=form.data, + follow_redirects=True) + assert InstanceConfig.query.get('ALLOW_DOCUMENT_UPLOADS').value is True + + +def test_disallow_document_uploads(journalist_app, test_admin): + with journalist_app.test_client() as app: + _login_user(app, test_admin['username'], test_admin['password'], + test_admin['otp_secret']) + app.post(url_for('admin.set_allow_document_uploads'), + follow_redirects=True) + assert InstanceConfig.query.get('ALLOW_DOCUMENT_UPLOADS').value is False + + def test_logo_upload_with_valid_image_succeeds(journalist_app, test_admin): # Save original logo to restore after test run logo_image_location = os.path.join(config.SECUREDROP_ROOT, From 0db9f226860442448176dc00fc9e1885e2a655dc Mon Sep 17 00:00:00 2001 From: Whistleblower Aid <54696764+wbaid@users.noreply.github.com> Date: Sat, 28 Sep 2019 20:29:32 -0400 Subject: [PATCH 06/12] feat(/metadata): include allow_document_uploads --- securedrop/source_app/api.py | 5 ++++- securedrop/tests/test_source.py | 2 ++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/securedrop/source_app/api.py b/securedrop/source_app/api.py index 6ae59de884..4bc0aba202 100644 --- a/securedrop/source_app/api.py +++ b/securedrop/source_app/api.py @@ -1,7 +1,7 @@ import json import platform -from flask import Blueprint, make_response +from flask import Blueprint, current_app, make_response import version @@ -12,6 +12,9 @@ def make_blueprint(config): @view.route('/metadata') def metadata(): meta = { + 'allow_document_uploads': current_app.config.get( + 'ALLOW_DOCUMENT_UPLOADS', + True), 'gpg_fpr': config.JOURNALIST_KEY, 'sd_version': version.__version__, 'server_os': platform.linux_distribution()[1], diff --git a/securedrop/tests/test_source.py b/securedrop/tests/test_source.py index 925265b435..950230b12b 100644 --- a/securedrop/tests/test_source.py +++ b/securedrop/tests/test_source.py @@ -552,6 +552,8 @@ def test_metadata_route(config, source_app): resp = app.get(url_for('api.metadata')) assert resp.status_code == 200 assert resp.headers.get('Content-Type') == 'application/json' + assert resp.json.get('allow_document_uploads') ==\ + source_app.config.get('ALLOW_DOCUMENT_UPLOADS', True) assert resp.json.get('sd_version') == version.__version__ assert resp.json.get('server_os') == '16.04' assert resp.json.get('supported_languages') ==\ From b5d493cea63fcf5f3b74fb6bbc2da3931fa1f174 Mon Sep 17 00:00:00 2001 From: Whistleblower Aid <54696764+wbaid@users.noreply.github.com> Date: Mon, 21 Oct 2019 23:28:05 -0400 Subject: [PATCH 07/12] fix: create tables required by @before_request hook load_instance_config() --- securedrop/tests/test_i18n.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/securedrop/tests/test_i18n.py b/securedrop/tests/test_i18n.py index 86296d2bad..22bf73931e 100644 --- a/securedrop/tests/test_i18n.py +++ b/securedrop/tests/test_i18n.py @@ -236,6 +236,8 @@ def __getattr__(self, name): return getattr(config, name) not_translated = 'code hello i18n' with source_app.create_app(Config()).test_client() as c: + with c.application.app_context(): + db.create_all() c.get('/') assert not_translated == gettext(not_translated) From bb673a080f61031ec6d67460e11f925d368c3cf9 Mon Sep 17 00:00:00 2001 From: Whistleblower Aid <54696764+wbaid@users.noreply.github.com> Date: Sat, 2 Nov 2019 18:15:38 -0400 Subject: [PATCH 08/12] fix: "[e]rror [and template] strings could be confusing to sources" --- securedrop/source_app/main.py | 9 ++++++--- securedrop/source_templates/lookup.html | 7 ++++++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/securedrop/source_app/main.py b/securedrop/source_app/main.py index 1246b89434..a3d3b6fdc4 100644 --- a/securedrop/source_app/main.py +++ b/securedrop/source_app/main.py @@ -145,9 +145,12 @@ def submit(): # Don't submit anything if it was an "empty" submission. #878 if not (msg or fh): - flash(gettext( - "You must enter a message or choose a file to submit."), - "error") + if allow_document_uploads: + flash(gettext( + "You must enter a message or choose a file to submit."), + "error") + else: + flash(gettext("You must enter a message."), "error") return redirect(url_for('main.lookup')) fnames = [] diff --git a/securedrop/source_templates/lookup.html b/securedrop/source_templates/lookup.html index 7b95b59de3..29c90152fe 100644 --- a/securedrop/source_templates/lookup.html +++ b/securedrop/source_templates/lookup.html @@ -22,7 +22,12 @@

{{ gettext('Submit Files or Messages') }}

{{ gettext('Submit Messages') }}

{% endif %} -

{{ gettext('If you are already familiar with GPG, you can optionally encrypt your files and messages with our public key before submission. Files are encrypted as they are received by SecureDrop.').format(url=url_for('info.download_journalist_pubkey')) }} +

+{% if allow_document_uploads %} +{{ gettext('If you are already familiar with GPG, you can optionally encrypt your files and messages with our public key before submission. Files are encrypted as they are received by SecureDrop.').format(url=url_for('info.download_journalist_pubkey')) }} +{% else %} +{{ gettext('If you are already familiar with GPG, you can optionally encrypt your messages with our public key before submission.').format(url=url_for('info.download_journalist_pubkey')) }} +{% endif %} {{ gettext('Learn more.').format(url=url_for('info.why_download_journalist_pubkey')) }}

From f272aef283af9d88d79e8f207db6985b3657fe66 Mon Sep 17 00:00:00 2001 From: Whistleblower Aid <54696764+wbaid@users.noreply.github.com> Date: Sat, 2 Nov 2019 18:19:24 -0400 Subject: [PATCH 09/12] fix: "textarea width on Source Interface" --- securedrop/sass/modules/_snippet.sass | 3 +++ 1 file changed, 3 insertions(+) diff --git a/securedrop/sass/modules/_snippet.sass b/securedrop/sass/modules/_snippet.sass index f51b78aa7e..fef1e030b0 100644 --- a/securedrop/sass/modules/_snippet.sass +++ b/securedrop/sass/modules/_snippet.sass @@ -46,3 +46,6 @@ .wide width: 100% + + textarea + width: 100% From 25ba95e59f5d7d1da20b7d047f4a1c9150d5ec52 Mon Sep 17 00:00:00 2001 From: Whistleblower Aid <54696764+wbaid@users.noreply.github.com> Date: Sat, 2 Nov 2019 18:23:23 -0400 Subject: [PATCH 10/12] refactor: "Proposed alternative language" for "Instance Configuration" page --- securedrop/journalist_app/admin.py | 19 +++++++++++-------- securedrop/journalist_app/forms.py | 4 ++-- securedrop/journalist_templates/config.html | 12 ++++++------ securedrop/tests/test_journalist.py | 16 ++++++++-------- 4 files changed, 27 insertions(+), 24 deletions(-) diff --git a/securedrop/journalist_app/admin.py b/securedrop/journalist_app/admin.py index 3d1bacb681..6737b758c1 100644 --- a/securedrop/journalist_app/admin.py +++ b/securedrop/journalist_app/admin.py @@ -14,7 +14,7 @@ from journalist_app.decorators import admin_required from journalist_app.utils import (make_password, commit_account_changes, set_diceware_password, validate_hotp_secret, revoke_token) -from journalist_app.forms import AllowDocumentUploadsForm, LogoForm, NewUserForm +from journalist_app.forms import LogoForm, NewUserForm, SubmissionPreferencesForm def make_blueprint(config): @@ -29,8 +29,9 @@ def index(): @view.route('/config', methods=('GET', 'POST')) @admin_required def manage_config(): - allow_document_uploads_form = AllowDocumentUploadsForm( - allow_document_uploads=current_app.config.get( + # The UI prompt ("prevent") is the opposite of the setting ("allow"): + submission_preferences_form = SubmissionPreferencesForm( + prevent_document_uploads=not current_app.config.get( 'ALLOW_DOCUMENT_UPLOADS', True)) logo_form = LogoForm() @@ -51,19 +52,21 @@ def manage_config(): for error in errors: flash(error, "logo-error") return render_template("config.html", - allow_document_uploads_form=allow_document_uploads_form, + submission_preferences_form=submission_preferences_form, logo_form=logo_form) - @view.route('/set-allow-document-uploads', methods=['POST']) + @view.route('/update-submission-preferences', methods=['POST']) @admin_required - def set_allow_document_uploads(): - form = AllowDocumentUploadsForm() + def update_submission_preferences(): + form = SubmissionPreferencesForm() if form.validate_on_submit(): # Upsert ALLOW_DOCUMENT_UPLOADS: setting = InstanceConfig.query.get('ALLOW_DOCUMENT_UPLOADS') if not setting: setting = InstanceConfig(name='ALLOW_DOCUMENT_UPLOADS') - setting.value = bool(request.form.get('allow_document_uploads')) + + # The UI prompt ("prevent") is the opposite of the setting ("allow"): + setting.value = not bool(request.form.get('prevent_document_uploads')) db.session.add(setting) db.session.commit() diff --git a/securedrop/journalist_app/forms.py b/securedrop/journalist_app/forms.py index 1241b31dfa..5e196a59e3 100644 --- a/securedrop/journalist_app/forms.py +++ b/securedrop/journalist_app/forms.py @@ -64,8 +64,8 @@ class ReplyForm(FlaskForm): ) -class AllowDocumentUploadsForm(FlaskForm): - allow_document_uploads = BooleanField('allow_document_uploads') +class SubmissionPreferencesForm(FlaskForm): + prevent_document_uploads = BooleanField('prevent_document_uploads') class LogoForm(FlaskForm): diff --git a/securedrop/journalist_templates/config.html b/securedrop/journalist_templates/config.html index 542684ec0c..219aea289a 100644 --- a/securedrop/journalist_templates/config.html +++ b/securedrop/journalist_templates/config.html @@ -43,16 +43,16 @@
-

{{ gettext('Document Uploads') }}

+

{{ gettext('Submission Preferences') }}

-
+

- {{ allow_document_uploads_form.allow_document_uploads() }} - + {{ submission_preferences_form.prevent_document_uploads() }} +

-
diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index 5d43066523..82d8519d8f 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -1287,25 +1287,25 @@ def test_admin_add_user_integrity_error(journalist_app, test_admin, mocker): "None\n[SQL: STATEMENT]\n[parameters: 'PARAMETERS']") in log_event -def test_allow_document_uploads(journalist_app, test_admin): +def test_prevent_document_uploads(journalist_app, test_admin): with journalist_app.test_client() as app: _login_user(app, test_admin['username'], test_admin['password'], test_admin['otp_secret']) - form = journalist_app_module.forms.AllowDocumentUploadsForm( - allow_document_uploads=True) - app.post(url_for('admin.set_allow_document_uploads'), + form = journalist_app_module.forms.SubmissionPreferencesForm( + prevent_document_uploads=True) + app.post(url_for('admin.update_submission_preferences'), data=form.data, follow_redirects=True) - assert InstanceConfig.query.get('ALLOW_DOCUMENT_UPLOADS').value is True + assert InstanceConfig.query.get('ALLOW_DOCUMENT_UPLOADS').value is False -def test_disallow_document_uploads(journalist_app, test_admin): +def test_no_prevent_document_uploads(journalist_app, test_admin): with journalist_app.test_client() as app: _login_user(app, test_admin['username'], test_admin['password'], test_admin['otp_secret']) - app.post(url_for('admin.set_allow_document_uploads'), + app.post(url_for('admin.update_submission_preferences'), follow_redirects=True) - assert InstanceConfig.query.get('ALLOW_DOCUMENT_UPLOADS').value is False + assert InstanceConfig.query.get('ALLOW_DOCUMENT_UPLOADS').value is True def test_logo_upload_with_valid_image_succeeds(journalist_app, test_admin): From d2d953f35921fe1726b4f1d137d7a0109ab4bf7c Mon Sep 17 00:00:00 2001 From: Whistleblower Aid <54696764+wbaid@users.noreply.github.com> Date: Sat, 2 Nov 2019 20:38:04 -0400 Subject: [PATCH 11/12] refactor: versioned instance_config, "one configuration option per column" --- ...fff3f969c_add_versioned_instance_config.py | 41 ++++++++++++ ...779_add_instance_config_key_value_store.py | 32 ---------- securedrop/journalist_app/__init__.py | 6 +- securedrop/journalist_app/admin.py | 15 +---- securedrop/models.py | 63 +++++++++++++++++-- securedrop/source_app/__init__.py | 6 +- securedrop/source_app/api.py | 4 +- securedrop/source_app/main.py | 9 +-- ...03c1ec779.py => migration_523fff3f969c.py} | 0 securedrop/tests/test_journalist.py | 4 +- securedrop/tests/test_source.py | 4 +- 11 files changed, 111 insertions(+), 73 deletions(-) create mode 100644 securedrop/alembic/versions/523fff3f969c_add_versioned_instance_config.py delete mode 100644 securedrop/alembic/versions/fef03c1ec779_add_instance_config_key_value_store.py rename securedrop/tests/migrations/{migration_fef03c1ec779.py => migration_523fff3f969c.py} (100%) diff --git a/securedrop/alembic/versions/523fff3f969c_add_versioned_instance_config.py b/securedrop/alembic/versions/523fff3f969c_add_versioned_instance_config.py new file mode 100644 index 0000000000..0108ee17df --- /dev/null +++ b/securedrop/alembic/versions/523fff3f969c_add_versioned_instance_config.py @@ -0,0 +1,41 @@ +"""add versioned instance config + +Revision ID: 523fff3f969c +Revises: 3da3fcab826a +Create Date: 2019-11-02 23:06:12.161868 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '523fff3f969c' +down_revision = '3da3fcab826a' +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_table('instance_config', + sa.Column('version', sa.Integer(), nullable=False), + sa.Column('valid_until', sa.DateTime(), nullable=True), + sa.Column('allow_document_uploads', sa.Boolean(), nullable=True), + + sa.PrimaryKeyConstraint('version'), + sa.UniqueConstraint('valid_until'), + ) + # ### end Alembic commands ### + + # Data migration: Since allow_document_uploads is the first + # instance_config setting (column), all we have to do is insert a + # row with its default value. + conn = op.get_bind() + conn.execute("""INSERT INTO instance_config (allow_document_uploads) VALUES (1)""") + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_table('instance_config') + # ### end Alembic commands ### diff --git a/securedrop/alembic/versions/fef03c1ec779_add_instance_config_key_value_store.py b/securedrop/alembic/versions/fef03c1ec779_add_instance_config_key_value_store.py deleted file mode 100644 index 2fd42c03d0..0000000000 --- a/securedrop/alembic/versions/fef03c1ec779_add_instance_config_key_value_store.py +++ /dev/null @@ -1,32 +0,0 @@ -"""add instance_config key-value store - -Revision ID: fef03c1ec779 -Revises: 3da3fcab826a -Create Date: 2019-10-21 00:43:19.477835 - -""" -from alembic import op -import sqlalchemy as sa - - -# revision identifiers, used by Alembic. -revision = 'fef03c1ec779' -down_revision = '3da3fcab826a' -branch_labels = None -depends_on = None - - -def upgrade(): - # ### commands auto generated by Alembic - please adjust! ### - op.create_table('instance_config', - sa.Column('name', sa.String(), nullable=False), - sa.Column('value', sa.JSON(), nullable=True), - sa.PrimaryKeyConstraint('name') - ) - # ### end Alembic commands ### - - -def downgrade(): - # ### commands auto generated by Alembic - please adjust! ### - op.drop_table('instance_config') - # ### end Alembic commands ### diff --git a/securedrop/journalist_app/__init__.py b/securedrop/journalist_app/__init__.py index 21d7aeecd0..959dd46bc3 100644 --- a/securedrop/journalist_app/__init__.py +++ b/securedrop/journalist_app/__init__.py @@ -126,11 +126,7 @@ def expire_blacklisted_tokens(): @app.before_request def load_instance_config(): - """Update app.config from the InstanceConfig table.""" - - instance_config = InstanceConfig.query.all() - settings = dict(map(lambda x: (x.name, x.value), instance_config)) - app.config.from_mapping(settings) + app.instance_config = InstanceConfig.get_current() @app.before_request def setup_g(): diff --git a/securedrop/journalist_app/admin.py b/securedrop/journalist_app/admin.py index 6737b758c1..8132ca18d5 100644 --- a/securedrop/journalist_app/admin.py +++ b/securedrop/journalist_app/admin.py @@ -31,9 +31,7 @@ def index(): def manage_config(): # The UI prompt ("prevent") is the opposite of the setting ("allow"): submission_preferences_form = SubmissionPreferencesForm( - prevent_document_uploads=not current_app.config.get( - 'ALLOW_DOCUMENT_UPLOADS', - True)) + prevent_document_uploads=not current_app.instance_config.allow_document_uploads) logo_form = LogoForm() if logo_form.validate_on_submit(): f = logo_form.logo.data @@ -60,16 +58,9 @@ def manage_config(): def update_submission_preferences(): form = SubmissionPreferencesForm() if form.validate_on_submit(): - # Upsert ALLOW_DOCUMENT_UPLOADS: - setting = InstanceConfig.query.get('ALLOW_DOCUMENT_UPLOADS') - if not setting: - setting = InstanceConfig(name='ALLOW_DOCUMENT_UPLOADS') - # The UI prompt ("prevent") is the opposite of the setting ("allow"): - setting.value = not bool(request.form.get('prevent_document_uploads')) - - db.session.add(setting) - db.session.commit() + value = not bool(request.form.get('prevent_document_uploads')) + InstanceConfig.set('allow_document_uploads', value) return redirect(url_for('admin.manage_config')) @view.route('/add', methods=('GET', 'POST')) diff --git a/securedrop/models.py b/securedrop/models.py index 1f09970fbd..61b4d45728 100644 --- a/securedrop/models.py +++ b/securedrop/models.py @@ -17,7 +17,7 @@ from passlib.hash import argon2 from sqlalchemy import ForeignKey from sqlalchemy.orm import relationship, backref -from sqlalchemy import Column, Integer, String, Boolean, DateTime, LargeBinary, JSON +from sqlalchemy import Column, Integer, String, Boolean, DateTime, LargeBinary from sqlalchemy.orm.exc import MultipleResultsFound, NoResultFound from db import db @@ -742,12 +742,65 @@ class RevokedToken(db.Model): class InstanceConfig(db.Model): - '''Key-value store of settings configurable from the journalist interface. + '''Versioned key-value store of settings configurable from the journalist + interface. The current version has valid_until=None. ''' __tablename__ = 'instance_config' - name = Column(String, primary_key=True) - value = Column(JSON) + version = Column(Integer, primary_key=True) + valid_until = Column(DateTime, default=None, unique=True) + + allow_document_uploads = Column(Boolean, default=True) + + # Columns not listed here will be included by InstanceConfig.copy() when + # updating the configuration. + metadata_cols = ['version', 'valid_until'] def __repr__(self): - return "" % (self.name, self.value) + return "" % (self.version, self.valid_until) + + def copy(self): + '''Make a copy of only the configuration columns of the given + InstanceConfig object: i.e., excluding metadata_cols. + ''' + + new = type(self)() + for col in self.__table__.columns: + if col.name in self.metadata_cols: + continue + + setattr(new, col.name, getattr(self, col.name)) + + return new + + @classmethod + def get_current(cls): + '''If the database was created via db.create_all(), data migrations + weren't run, and the "instance_config" table is empty. In this case, + save and return a base configuration derived from each setting's + column-level default. + ''' + + try: + return cls.query.filter(cls.valid_until == None).one() # noqa: E711 + except NoResultFound: + current = cls() + db.session.add(current) + db.session.commit() + return current + + @classmethod + def set(cls, name, value): + '''Invalidate the current configuration and append a new one with the + requested change. + ''' + + old = cls.get_current() + old.valid_until = datetime.datetime.utcnow() + db.session.add(old) + + new = old.copy() + setattr(new, name, value) + db.session.add(new) + + db.session.commit() diff --git a/securedrop/source_app/__init__.py b/securedrop/source_app/__init__.py index 550e407a93..b52d76849a 100644 --- a/securedrop/source_app/__init__.py +++ b/securedrop/source_app/__init__.py @@ -133,11 +133,7 @@ def check_tor2web(): @app.before_request @ignore_static def load_instance_config(): - """Update app.config from the InstanceConfig table.""" - - instance_config = InstanceConfig.query.all() - settings = dict(map(lambda x: (x.name, x.value), instance_config)) - app.config.from_mapping(settings) + app.instance_config = InstanceConfig.get_current() @app.before_request @ignore_static diff --git a/securedrop/source_app/api.py b/securedrop/source_app/api.py index 4bc0aba202..e6171211c7 100644 --- a/securedrop/source_app/api.py +++ b/securedrop/source_app/api.py @@ -12,9 +12,7 @@ def make_blueprint(config): @view.route('/metadata') def metadata(): meta = { - 'allow_document_uploads': current_app.config.get( - 'ALLOW_DOCUMENT_UPLOADS', - True), + 'allow_document_uploads': current_app.instance_config.allow_document_uploads, 'gpg_fpr': config.JOURNALIST_KEY, 'sd_version': version.__version__, 'server_os': platform.linux_distribution()[1], diff --git a/securedrop/source_app/main.py b/securedrop/source_app/main.py index a3d3b6fdc4..d6c90f0fae 100644 --- a/securedrop/source_app/main.py +++ b/securedrop/source_app/main.py @@ -83,9 +83,6 @@ def create(): @view.route('/lookup', methods=('GET',)) @login_required def lookup(): - allow_document_uploads = current_app.config.get( - 'ALLOW_DOCUMENT_UPLOADS', - True) replies = [] source_inbox = Reply.query.filter(Reply.source_id == g.source.id) \ .filter(Reply.deleted_by_source == False).all() # noqa @@ -124,7 +121,7 @@ def lookup(): return render_template( 'lookup.html', - allow_document_uploads=allow_document_uploads, + allow_document_uploads=current_app.instance_config.allow_document_uploads, codename=g.codename, replies=replies, flagged=g.source.flagged, @@ -135,9 +132,7 @@ def lookup(): @view.route('/submit', methods=('POST',)) @login_required def submit(): - allow_document_uploads = current_app.config.get( - 'ALLOW_DOCUMENT_UPLOADS', - True) + allow_document_uploads = current_app.instance_config.allow_document_uploads msg = request.form['msg'] fh = None if allow_document_uploads and 'fh' in request.files: diff --git a/securedrop/tests/migrations/migration_fef03c1ec779.py b/securedrop/tests/migrations/migration_523fff3f969c.py similarity index 100% rename from securedrop/tests/migrations/migration_fef03c1ec779.py rename to securedrop/tests/migrations/migration_523fff3f969c.py diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index 82d8519d8f..886b2a7189 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -1296,7 +1296,7 @@ def test_prevent_document_uploads(journalist_app, test_admin): app.post(url_for('admin.update_submission_preferences'), data=form.data, follow_redirects=True) - assert InstanceConfig.query.get('ALLOW_DOCUMENT_UPLOADS').value is False + assert InstanceConfig.get_current().allow_document_uploads is False def test_no_prevent_document_uploads(journalist_app, test_admin): @@ -1305,7 +1305,7 @@ def test_no_prevent_document_uploads(journalist_app, test_admin): test_admin['otp_secret']) app.post(url_for('admin.update_submission_preferences'), follow_redirects=True) - assert InstanceConfig.query.get('ALLOW_DOCUMENT_UPLOADS').value is True + assert InstanceConfig.get_current().allow_document_uploads is True def test_logo_upload_with_valid_image_succeeds(journalist_app, test_admin): diff --git a/securedrop/tests/test_source.py b/securedrop/tests/test_source.py index 950230b12b..7cf06ae7f4 100644 --- a/securedrop/tests/test_source.py +++ b/securedrop/tests/test_source.py @@ -14,7 +14,7 @@ import version from db import db -from models import Source, Reply +from models import InstanceConfig, Source, Reply from source_app import main as source_app_main from source_app import api as source_app_api from .utils.db_helper import new_codename @@ -553,7 +553,7 @@ def test_metadata_route(config, source_app): assert resp.status_code == 200 assert resp.headers.get('Content-Type') == 'application/json' assert resp.json.get('allow_document_uploads') ==\ - source_app.config.get('ALLOW_DOCUMENT_UPLOADS', True) + InstanceConfig.get_current().allow_document_uploads assert resp.json.get('sd_version') == version.__version__ assert resp.json.get('server_os') == '16.04' assert resp.json.get('supported_languages') ==\ From ab25a8f1097bda4dd53164d0614bd5e6df357861 Mon Sep 17 00:00:00 2001 From: Whistleblower Aid <54696764+wbaid@users.noreply.github.com> Date: Tue, 19 Nov 2019 17:48:34 -0500 Subject: [PATCH 12/12] test(integration): check source interface complies with allow_document_uploads --- securedrop/tests/test_integration.py | 54 ++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/securedrop/tests/test_integration.py b/securedrop/tests/test_integration.py index d2e0ffeefd..cb2fb0e944 100644 --- a/securedrop/tests/test_integration.py +++ b/securedrop/tests/test_integration.py @@ -16,6 +16,7 @@ from flask import current_app, escape, g, session from pyotp import HOTP, TOTP +import journalist_app as journalist_app_module from . import utils from .utils.instrument import InstrumentedApp @@ -692,3 +693,56 @@ def test_login_after_regenerate_hotp(journalist_app, test_journo): password=test_journo['password'], token=HOTP(b32_otp_secret).at(1))) ins.assert_redirects(resp, '/') + + +def test_prevent_document_uploads(source_app, journalist_app, test_admin): + '''Test that the source interface accepts only messages when + allow_document_uploads == False. + + ''' + + # Set allow_document_uploads = False: + with journalist_app.test_client() as app: + _login_user(app, test_admin) + form = journalist_app_module.forms.SubmissionPreferencesForm( + prevent_document_uploads=True) + resp = app.post('/admin/update-submission-preferences', + data=form.data, + follow_redirects=True) + assert resp.status_code == 200 + + # Check that the source interface accepts only messages: + with source_app.test_client() as app: + app.get('/generate') + resp = app.post('/create', follow_redirects=True) + assert resp.status_code == 200 + + text = resp.data.decode('utf-8') + soup = BeautifulSoup(text, 'html.parser') + assert 'Submit Messages' in text + assert len(soup.select('input[type="file"]')) == 0 + + +def test_no_prevent_document_uploads(source_app, journalist_app, test_admin): + '''Test that the source interface accepts both files and messages when + allow_document_uploads == True. + + ''' + + # Set allow_document_uploads = True: + with journalist_app.test_client() as app: + _login_user(app, test_admin) + resp = app.post('/admin/update-submission-preferences', + follow_redirects=True) + assert resp.status_code == 200 + + # Check that the source interface accepts both files and messages: + with source_app.test_client() as app: + app.get('/generate') + resp = app.post('/create', follow_redirects=True) + assert resp.status_code == 200 + + text = resp.data.decode('utf-8') + soup = BeautifulSoup(text, 'html.parser') + assert 'Submit Files or Messages' in text + assert len(soup.select('input[type="file"]')) == 1