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

Upgrade Flask dependency to 2.0.2 and refactor applications to improve static type checks #6217

Merged
merged 15 commits into from
Feb 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion molecule/testinfra/vars/app-qubes-staging.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ app_ip: 10.137.0.50

pip_deps:
- name: 'Flask'
version: '1.0.2'
version: '2.0.2'

apparmor_complain: []

Expand Down
2 changes: 1 addition & 1 deletion molecule/testinfra/vars/app-staging.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ app_ip: 10.0.1.2

pip_deps:
- name: 'Flask'
version: '1.0.2'
version: '2.0.2'

apparmor_complain: []

Expand Down
2 changes: 1 addition & 1 deletion molecule/testinfra/vars/prod.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ app_ip: 10.20.2.2

pip_deps:
- name: 'Flask'
version: '1.0.2'
version: '2.0.2'

apparmor_complain: []

Expand Down
2 changes: 1 addition & 1 deletion molecule/testinfra/vars/prodVM.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ app_ip: 10.0.1.4

pip_deps:
- name: 'Flask'
version: '1.0.2'
version: '2.0.2'

apparmor_complain: []

Expand Down
2 changes: 1 addition & 1 deletion molecule/testinfra/vars/qubes-staging.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ app_ip: 10.137.0.50

pip_deps:
- name: 'Flask'
version: '1.0.2'
version: '2.0.2'

apparmor_complain: []

Expand Down
2 changes: 1 addition & 1 deletion molecule/testinfra/vars/staging.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ app_ip: 10.0.1.2

pip_deps:
- name: 'Flask'
version: '1.0.2'
version: '2.0.2'

apparmor_complain: []

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
try:
from journalist_app import create_app
from sdconfig import config
from store import NoFileFoundException, TooManyFilesException
from store import NoFileFoundException, TooManyFilesException, Storage
except ImportError:
# This is a fresh install, and config.py has not been created yet.
if raise_errors:
Expand Down Expand Up @@ -61,8 +61,8 @@ def upgrade():
""").bindparams(id=submission.id)
)

path = app.storage.path_without_filesystem_id(submission.filename)
app.storage.move_to_shredder(path)
path = Storage.get_default().path_without_filesystem_id(submission.filename)
Storage.get_default().move_to_shredder(path)
except NoFileFoundException:
# The file must have been deleted by the admin, remove the row
conn.execute(
Expand All @@ -83,8 +83,8 @@ def upgrade():
""").bindparams(id=reply.id)
)

path = app.storage.path_without_filesystem_id(reply.filename)
app.storage.move_to_shredder(path)
path = Storage.get_default().path_without_filesystem_id(reply.filename)
Storage.get_default().move_to_shredder(path)
except NoFileFoundException:
# The file must have been deleted by the admin, remove the row
conn.execute(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from journalist_app import create_app
from models import Submission, Reply
from sdconfig import config
from store import queued_add_checksum_for_file
from store import queued_add_checksum_for_file, Storage
from worker import create_queue
except: # noqa
if raise_errors:
Expand Down Expand Up @@ -58,7 +58,7 @@ def upgrade():
"""
)
for (sub_id, filesystem_id, filename) in conn.execute(query):
full_path = app.storage.path(filesystem_id, filename)
full_path = Storage.get_default().path(filesystem_id, filename)
create_queue().enqueue(
queued_add_checksum_for_file,
Submission,
Expand All @@ -75,7 +75,7 @@ def upgrade():
"""
)
for (rep_id, filesystem_id, filename) in conn.execute(query):
full_path = app.storage.path(filesystem_id, filename)
full_path = Storage.get_default().path(filesystem_id, filename)
create_queue().enqueue(
queued_add_checksum_for_file,
Reply,
Expand Down
4 changes: 2 additions & 2 deletions securedrop/dockerfiles/focal/python3/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ RUN wget https://github.com/mozilla/geckodriver/releases/download/${GECKODRIVER_
COPY requirements requirements
RUN python3 -m venv /opt/venvs/securedrop-app-code && \
/opt/venvs/securedrop-app-code/bin/pip3 install --no-deps --require-hashes -r requirements/python3/docker-requirements.txt && \
/opt/venvs/securedrop-app-code/bin/pip3 install --no-deps --require-hashes -r requirements/python3/securedrop-app-code-requirements.txt && \
/opt/venvs/securedrop-app-code/bin/pip3 install --no-deps --require-hashes -r requirements/python3/test-requirements.txt
/opt/venvs/securedrop-app-code/bin/pip3 install --no-deps --require-hashes -r requirements/python3/test-requirements.txt && \
/opt/venvs/securedrop-app-code/bin/pip3 install --no-deps --require-hashes -r requirements/python3/securedrop-app-code-requirements.txt

RUN if test $USER_NAME != root ; then useradd --no-create-home --home-dir /tmp --uid $USER_ID $USER_NAME && echo "$USER_NAME ALL=(ALL) NOPASSWD:ALL" >> /etc/sudoers ; fi && \
cp -r /root/.local /tmp/ && chmod +x /tmp/.local/tbb/tor-browser_en-US/Browser/firefox && chmod -R 777 /tmp/.local && \
Expand Down
4 changes: 2 additions & 2 deletions securedrop/i18n.py
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,6 @@ def set_locale(config: SDConfig) -> None:
Update locale info in request and session.
"""
locale = get_locale(config)
g.localeinfo = RequestLocaleInfo(locale)
g.localeinfo = RequestLocaleInfo(locale) # pylint: disable=assigning-non-slot
session["locale"] = locale
g.locales = LOCALES
g.locales = LOCALES # pylint: disable=assigning-non-slot
39 changes: 20 additions & 19 deletions securedrop/journalist_app/__init__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# -*- coding: utf-8 -*-

from datetime import datetime, timedelta
from datetime import datetime, timedelta, timezone
from pathlib import Path

from flask import (Flask, session, redirect, url_for, flash, g, request,
Expand All @@ -21,7 +21,6 @@
JournalistInterfaceSessionInterface,
cleanup_expired_revoked_tokens)
from models import InstanceConfig, Journalist
from store import Storage

import typing
# https://www.python.org/dev/peps/pep-0484/#runtime-or-type-checking
Expand Down Expand Up @@ -69,11 +68,9 @@ def create_app(config: 'SDConfig') -> Flask:
app.config['SQLALCHEMY_DATABASE_URI'] = config.DATABASE_URI
db.init_app(app)

# TODO: Attaching a Storage dynamically like this disables all type checking (and
# breaks code analysis tools) for code that uses current_app.storage; it should be refactored
app.storage = Storage(config.STORE_DIR, config.TEMP_DIR)

@app.errorhandler(CSRFError)
# TODO: enable type checking once upstream Flask fix is available. See:
# https://github.com/pallets/flask/issues/4295
@app.errorhandler(CSRFError) # type: ignore
def handle_csrf_error(e: CSRFError) -> 'Response':
app.logger.error("The CSRF token is invalid.")
session.clear()
Expand All @@ -86,9 +83,12 @@ def _handle_http_exception(
) -> 'Tuple[Union[Response, str], Optional[int]]':
# Workaround for no blueprint-level 404/5 error handlers, see:
# https://github.com/pallets/flask/issues/503#issuecomment-71383286
# TODO: clean up API error handling such that all except 404/5s are
# registered in the blueprint and 404/5s are handled at the application
# level.
handler = list(app.error_handler_spec['api'][error.code].values())[0]
if request.path.startswith('/api/') and handler:
return handler(error)
return handler(error) # type: ignore

return render_template('error.html', error=error), error.code

Expand All @@ -111,13 +111,13 @@ def expire_blacklisted_tokens() -> None:
cleanup_expired_revoked_tokens()

@app.before_request
def load_instance_config() -> None:
app.instance_config = InstanceConfig.get_current()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm understanding this correctly, this is a behavior shift now in that InstanceConfig is just initialized once at startup rather than on each request. Is that intentional? I haven't figured out how that works with the "valid_until" field yet...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the instance config is now retrieved every time InstanceConfig.get_default() is called, so it's actually hitting the DB as much as before (or potentially more often if a single request has multiple .get_default()s) This reduces the benefit of a global object, so there's probably some opportunity for optimization there. The trick would be to detect configuration changes when they're made in the journalist interface, as we want those to be reflected immediately in the source app.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the InstanceConfig get_default() method to be lazier - it will now only hit the db if explicitly requested or if the global is None. With some tweaks to @app.before_request-decorate functions, changes should show up immediately in both apps without extra DB overhead.

Also, re valid_until - it might be a bit counter-intuitive. It records the historical info about previous configs, and when they were invalidated. (TBH I'm not sure we need to store said previous configs, but it does provide an audit trail for instance_config changes.)

def update_instance_config() -> None:
InstanceConfig.get_default(refresh=True)

@app.before_request
def setup_g() -> 'Optional[Response]':
"""Store commonly used values in Flask's special g object"""
if 'expires' in session and datetime.utcnow() >= session['expires']:
if 'expires' in session and datetime.now(timezone.utc) >= session['expires']:
session.clear()
flash(gettext('You have been logged out due to inactivity.'),
'error')
Expand All @@ -131,24 +131,25 @@ def setup_g() -> 'Optional[Response]':
flash(gettext('You have been logged out due to password change'),
'error')

session['expires'] = datetime.utcnow() + \
session['expires'] = datetime.now(timezone.utc) + \
timedelta(minutes=getattr(config,
'SESSION_EXPIRATION_MINUTES',
120))

uid = session.get('uid', None)
if uid:
g.user = Journalist.query.get(uid)
g.user = Journalist.query.get(uid) # pylint: disable=assigning-non-slot

i18n.set_locale(config)

if app.instance_config.organization_name:
g.organization_name = app.instance_config.organization_name
if InstanceConfig.get_default().organization_name:
g.organization_name = \
InstanceConfig.get_default().organization_name # pylint: disable=assigning-non-slot
else:
g.organization_name = gettext('SecureDrop')
g.organization_name = gettext('SecureDrop') # pylint: disable=assigning-non-slot
Comment on lines -145 to +149
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit in passing: I wondered if this could be simplified, since InstanceConfig.organization_name has default="SecureDrop", but it looks like that default's not applied in the upgrade migration.


try:
g.logo = get_logo_url(app)
g.logo = get_logo_url(app) # pylint: disable=assigning-non-slot
except FileNotFoundError:
app.logger.error("Site logo not found.")

Expand All @@ -161,8 +162,8 @@ def setup_g() -> 'Optional[Response]':
if request.method == 'POST':
filesystem_id = request.form.get('filesystem_id')
if filesystem_id:
g.filesystem_id = filesystem_id
g.source = get_source(filesystem_id)
g.filesystem_id = filesystem_id # pylint: disable=assigning-non-slot
g.source = get_source(filesystem_id) # pylint: disable=assigning-non-slot

return None

Expand Down
4 changes: 2 additions & 2 deletions securedrop/journalist_app/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ def index() -> str:
def manage_config() -> Union[str, werkzeug.Response]:
# The UI prompt ("prevent") is the opposite of the setting ("allow"):
submission_preferences_form = SubmissionPreferencesForm(
prevent_document_uploads=not current_app.instance_config.allow_document_uploads)
prevent_document_uploads=not InstanceConfig.get_default().allow_document_uploads)
organization_name_form = OrgNameForm(
organization_name=current_app.instance_config.organization_name)
organization_name=InstanceConfig.get_default().organization_name)
logo_form = LogoForm()
if logo_form.validate_on_submit():
f = logo_form.logo.data
Expand Down
14 changes: 7 additions & 7 deletions securedrop/journalist_app/api.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import collections.abc
import json

from datetime import datetime, timedelta
from datetime import datetime, timedelta, timezone
from typing import Tuple, Callable, Any, Set, Union

import flask
import werkzeug
from flask import abort, Blueprint, current_app, jsonify, request
from flask import abort, Blueprint, jsonify, request
from functools import wraps

from sqlalchemy import Column
Expand All @@ -22,7 +22,7 @@
BadTokenException, InvalidOTPSecretException,
WrongPasswordException)
from sdconfig import SDConfig
from store import NotEncrypted
from store import NotEncrypted, Storage


TOKEN_EXPIRATION_MINS = 60 * 8
Expand Down Expand Up @@ -116,7 +116,7 @@ def get_token() -> Tuple[flask.Response, int]:

try:
journalist = Journalist.login(username, passphrase, one_time_code)
token_expiry = datetime.utcnow() + timedelta(
token_expiry = datetime.now(timezone.utc) + timedelta(
seconds=TOKEN_EXPIRATION_MINS * 60)

response = jsonify({
Expand All @@ -128,7 +128,7 @@ def get_token() -> Tuple[flask.Response, int]:
})

# Update access metadata
journalist.last_access = datetime.utcnow()
journalist.last_access = datetime.now(timezone.utc)
db.session.add(journalist)
db.session.commit()

Expand Down Expand Up @@ -253,7 +253,7 @@ def all_source_replies(source_uuid: str) -> Tuple[flask.Response, int]:

source.interaction_count += 1
try:
filename = current_app.storage.save_pre_encrypted_reply(
filename = Storage.get_default().save_pre_encrypted_reply(
source.filesystem_id,
source.interaction_count,
source.journalist_filename,
Expand All @@ -265,7 +265,7 @@ def all_source_replies(source_uuid: str) -> Tuple[flask.Response, int]:
# issue #3918
filename = path.basename(filename)

reply = Reply(user, source, filename)
reply = Reply(user, source, filename, Storage.get_default())

reply_uuid = data.get('uuid', None)
if reply_uuid is not None:
Expand Down
9 changes: 5 additions & 4 deletions securedrop/journalist_app/col.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
col_download_all, col_star, col_un_star,
col_delete, col_delete_data, mark_seen)
from sdconfig import SDConfig
from store import Storage


def make_blueprint(config: SDConfig) -> Blueprint:
Expand Down Expand Up @@ -118,7 +119,7 @@ def download_single_file(filesystem_id: str, fn: str) -> werkzeug.Response:
if '..' in fn or fn.startswith('/'):
abort(404)

file = current_app.storage.path(filesystem_id, fn)
file = Storage.get_default().path(filesystem_id, fn)
if not Path(file).is_file():
flash(
gettext(
Expand All @@ -137,15 +138,15 @@ def download_single_file(filesystem_id: str, fn: str) -> werkzeug.Response:
reply = Reply.query.filter(Reply.filename == fn).one()
mark_seen([reply], journalist)
elif fn.endswith("-doc.gz.gpg") or fn.endswith("doc.zip.gpg"):
file = Submission.query.filter(Submission.filename == fn).one()
mark_seen([file], journalist)
submitted_file = Submission.query.filter(Submission.filename == fn).one()
mark_seen([submitted_file], journalist)
else:
message = Submission.query.filter(Submission.filename == fn).one()
mark_seen([message], journalist)
except NoResultFound as e:
current_app.logger.error("Could not mark {} as seen: {}".format(fn, e))

return send_file(current_app.storage.path(filesystem_id, fn),
return send_file(Storage.get_default().path(filesystem_id, fn),
mimetype="application/pgp-encrypted")

return view
11 changes: 6 additions & 5 deletions securedrop/journalist_app/main.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# -*- coding: utf-8 -*-
from datetime import datetime
from datetime import datetime, timezone
from pathlib import Path
from typing import Union

Expand All @@ -19,6 +19,7 @@
from journalist_app.utils import (validate_user, bulk_delete, download,
confirm_bulk_delete, get_source)
from sdconfig import SDConfig
from store import Storage


def make_blueprint(config: SDConfig) -> Blueprint:
Expand All @@ -36,7 +37,7 @@ def login() -> Union[str, werkzeug.Response]:
request.form['token']))

# Update access metadata
user.last_access = datetime.utcnow()
user.last_access = datetime.now(timezone.utc)
db.session.add(user)
db.session.commit()

Expand Down Expand Up @@ -135,16 +136,16 @@ def reply() -> werkzeug.Response:
EncryptionManager.get_default().encrypt_journalist_reply(
for_source_with_filesystem_id=g.filesystem_id,
reply_in=form.message.data,
encrypted_reply_path_out=Path(current_app.storage.path(g.filesystem_id, filename)),
encrypted_reply_path_out=Path(Storage.get_default().path(g.filesystem_id, filename)),
)

try:
reply = Reply(g.user, g.source, filename)
reply = Reply(g.user, g.source, filename, Storage.get_default())
db.session.add(reply)
seen_reply = SeenReply(reply=reply, journalist=g.user)
db.session.add(seen_reply)
db.session.commit()
store.async_add_checksum_for_file(reply)
store.async_add_checksum_for_file(reply, Storage.get_default())
except Exception as exc:
flash(gettext(
"An unexpected error occurred! Please "
Expand Down
Loading