refactor: versioned instance_config, "one configuration option per co…

…lumn"

securedrop/alembic/versions/fef03c1ec779_add_instance_config_key_value_store.py → securedrop/alembic/versions/523fff3f969c_add_versioned_instance_config.py

@@ -1,16 +1,16 @@
-"""add instance_config key-value store
+"""add versioned instance config
 
-Revision ID: fef03c1ec779
+Revision ID: 523fff3f969c
 Revises: 3da3fcab826a
-Create Date: 2019-10-21 00:43:19.477835
+Create Date: 2019-11-02 23:06:12.161868
 
 """
 from alembic import op
 import sqlalchemy as sa
 
 
 # revision identifiers, used by Alembic.
-revision = 'fef03c1ec779'
+revision = '523fff3f969c'
 down_revision = '3da3fcab826a'
 branch_labels = None
 depends_on = None
@@ -19,9 +19,10 @@
 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')
+    sa.Column('version', sa.Integer(), nullable=False),
+    sa.Column('valid_until', sa.DateTime(), nullable=True),
+    sa.PrimaryKeyConstraint('version'),
+    sa.UniqueConstraint('valid_until')
     )
     # ### end Alembic commands ###
 

securedrop/alembic/versions/e0719fbc39a0_add_instance_config_allow_document_.py

@@ -0,0 +1,40 @@
+"""add instance_config.allow_document_uploads
+
+Revision ID: e0719fbc39a0
+Revises: 523fff3f969c
+Create Date: 2019-11-02 23:06:52.999672
+
+"""
+from alembic import op
+import sqlalchemy as sa
+
+import datetime
+
+
+# revision identifiers, used by Alembic.
+revision = 'e0719fbc39a0'
+down_revision = '523fff3f969c'
+branch_labels = None
+depends_on = None
+
+
+def upgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    with op.batch_alter_table('instance_config', schema=None) as batch_op:
+        batch_op.add_column(sa.Column('allow_document_uploads', sa.Boolean(), nullable=True))
+
+    # ### 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! ###
+    with op.batch_alter_table('instance_config', schema=None) as batch_op:
+        batch_op.drop_column('allow_document_uploads')
+
+    # ### end Alembic commands ###

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():

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'))

securedrop/models.py

@@ -742,12 +742,64 @@ 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 "<InstanceConfig(name='%s', value='%s')>" % (self.name, self.value)
+        return "<InstanceConfig(version=%s, valid_until=%s)>" % (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()
+        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()

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

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],

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:

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):