-
Notifications
You must be signed in to change notification settings - Fork 688
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned earlier, I think this could be split a bit to make easier to review:
- datetime changes: utcnow() -> now(utc)
- Introduction of and switch to Storage.get_default()
- Introduction of and switch to InstanceConfig.get_default()
@@ -110,14 +110,10 @@ def _handle_http_exception( | |||
def expire_blacklisted_tokens() -> None: | |||
cleanup_expired_revoked_tokens() | |||
|
|||
@app.before_request | |||
def load_instance_config() -> None: | |||
app.instance_config = InstanceConfig.get_current() |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
securedrop/store.py
Outdated
filename: typing.Optional[str], | ||
stream: 'IO[bytes]') -> str: | ||
|
||
sanitized_filename = secure_filename("unknown.file") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be easier to read if it was in an else block below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, updated.
securedrop/journalist_app/col.py
Outdated
@@ -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) | |||
the_file = Submission.query.filter(Submission.filename == fn).one() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're renaming this maybe a better name would be submitted_file?
This whole block could be more DRY by not duplicating the mark_seen
call for each if stanza...but that can be done separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 updated (the name, not the mark_seen() calls)
@@ -82,7 +84,7 @@ def create() -> werkzeug.Response: | |||
create_source_user( | |||
db_session=db.session, | |||
source_passphrase=codename, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also documented as taking a DicewarePassphrase, so I don't understand why mypy is happy with it but below wants you to wrap it. In any case, shouldn't the codename variable be turned into a DicewarePassphrase as soon as its pulled out of the session object?
12c7ef0
to
6ec6f69
Compare
This makes sense - I've reordered and merged commits to simplify things and group related changes. Let me know if it's helpful! |
Codecov Report
@@ Coverage Diff @@
## develop #6217 +/- ##
===========================================
+ Coverage 85.13% 85.15% +0.01%
===========================================
Files 59 59
Lines 4090 4102 +12
Branches 487 490 +3
===========================================
+ Hits 3482 3493 +11
Misses 491 491
- Partials 117 118 +1
Continue to review full report at Codecov.
|
d9cd3b2
to
17e8791
Compare
flask-babel==0.11.2 \ | ||
--hash=sha256:462a3c599b0ccf426ca1757cc612f1db383844efd346d14170da04c8c76dd521 \ | ||
--hash=sha256:c0d75710bd4b0fe866f9f2347de6e19208712f9cec006436b4c1c15d4cb0c939 | ||
flask-babel==2.0.0 \ | ||
--hash=sha256:e6820a052a8d344e178cdd36dd4bb8aea09b4bda3d5f9fa9f008df2c7f2f5468 \ | ||
--hash=sha256:f9faf45cdb2e1a32ea2ec14403587d4295108f35017a7821a2b1acb8cfd9257d | ||
# via -r requirements/python3/securedrop-app-code-requirements.in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
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.
redis==3.3.6 \ | ||
--hash=sha256:45682ecf226c7611efe731974c4fa3390170ba045b9cdb26f0051114a5c2a68b \ | ||
--hash=sha256:f2609a85e5f37f489ba3b5652e1175dc3711c4d7a7818c4f657615810afd23df | ||
redis==3.5.3 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diff reviews complete for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like securedrop/requirements/python3/test-requirements.txt
still needs to be updated; it contains itsdangerous==0.24
(the old version) and also Flash v1 still.
Yup, that's an outstanding task in the TODOs - current workaround also described there. |
jinja notes:
I reviewed all the doc and src/ changes, ended up skipping the tests/ changes due to a lack of time. |
# via | ||
# flask | ||
# flask-wtf | ||
jinja2==3.0.2 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
securedrop/models.py
Outdated
@@ -877,6 +879,13 @@ def copy(self) -> "InstanceConfig": | |||
|
|||
return new | |||
|
|||
@classmethod | |||
def get_default(cls, refresh: Optional[bool] = False) -> "InstanceConfig": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put refresh: bool = False
; not sure there's a need for refresh to ever be None?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️ - updated.
@@ -32,30 +32,29 @@ def create_file_in_source_dir(config, filesystem_id, filename): | |||
return source_directory, file_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To further simplify the tests in this file, I would recommend:
- Removing all mentions of the journalist_app, as it is out of scope for what is being tested here (only the Storage class is being tested; not the full app).
- Not using the storage fixture as I would argue that creating a Storage class should be done in the test code as that's what's being tested: it's not "setup" code. The benefit will be that the tests won't be coupled with the config fixture (which is used in a lot of tests and seems unnecessary here) and the tests will be easier to read/follow.
You can see an example of tis approach above in TestPassphrasesGenerator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The journalist_app
fixture is unused in the majority of the tests alright - there are a few that do require an app context - eg. the shredder tests, but they could probably be moved to a separate file.
I'm less convinced about removing the app_storage
fixture in all cases. Some of the tests that use it and the config
fixture are essentially verifying that files are being created in the store in the expected location based on config parameters, and for those it would seem either tautological to just remove config
, or duplicative to effectively recreate both fixtures in the test. But it can definitely be removed from tests not using config
and replaced with a test-specific Storage object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually on further reflection there are a lot more cases where the latter makes sense...
# Given a source user | ||
with source_app.app_context(): | ||
source_user = create_source_user( | ||
db_session=db.session, | ||
source_passphrase=PassphraseGenerator.get_default().generate_passphrase(), | ||
source_app_storage=source_app.storage, | ||
source_app_storage=app_storage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not super important, but you might now be able to remove the source_app fixture from this test and the one below 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like yes to the one below, but no to this one without independently setting up the db and creating an app and passing its context within the test.
6f89f27
to
aba1749
Compare
if filename is not None: | ||
sanitized_filename = secure_filename(filename) | ||
else: | ||
sanitized_filename = secure_filename("unknown.file") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Not very important) You could simplify this by switching the argument's declaration to:
filename: str = "unknown.file",
Then no if/else would be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kindof like it as is, I think @legoktm's previous point about readability is a good one.
rq==1.10.0 \ | ||
--hash=sha256:92950a3e60863de48dd1800882939bbaf089a37497ebf9f2ecf7c9fd0a4c4a95 \ | ||
--hash=sha256:be09ec43fae9a75a4d26ea3cd520e5fa3ea2ea8cf481be33e6ec9416f0369cac |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got through the first half of the test plan today, should finish the rest on Monday. |
Originally, test requirements were installed in the Docker environment after application requirements. Some dependencies are duplicated in both, with older versions present in the test requirements. The order of installation was switched to ensure application requirements were always present and not overridden.
- click from 6.7 to 8.0.3 - flask-babel from 011.2 to 2.0.0 - flask-wtf from 0.14.2 to 1.0.0 - Flask from 1.0.2 to 2.0.2 - itsdangerous from 0.24 to 2.0.1 - jinja2 from 2.11.3 to 3.0.2 - markupsafe from 1.1.1 to 2.0.1 - redis from 3.3.6 to 3.5.3 - rq from 1.1.0 to 1.10.0 - werkzeug from 0.16.0 to 2.0.2
Updated tests to use timezone-aware datetimes.
See pallets/flask#4020 for more info on this error.
- added global Storage object - updated source app to use global storage - updated journalist app to use global storage - updated loaddata.py to use global storage - updated shredder to use global storage - updated alembic migration scripts to use global storage object
- disabled broken typechecking for csrf error handlers - disabled typechecking for blueprint 404 error handling workaround - updated save_session() with expected type - updated _secure_file_stream args to match those expected by Werkzeug 2.0 - fixed to_json() return type - updated file submissions to use expected types and handle case with missing filename - ignore babel_instance attribute dynamically added by flask-babel - explicitly cast source passphrase for login - use expected SessionMixin type instead of Dict[Any, Any] for sessions - use BytesIO instead of BufferedBaseIO - changed invalid variable name 'file' to 'the_file'
…nctions. In models.py, the Submission and Reply constructors reference the Storage object to calculate file size information. Originally they used Storage.get_default() directly, but passing the Storage object as a parameter instead more cleanly separates the model from the application and simplifies the tests. Similar considerations apply to the async_add_checksum_for_file() function in store.py.
Some test changes were needed to account for the use of a global Storage object. An app_storage fixture was added, returning a function-scoped Storage object. For tests which access storage directly, that fixture was added to their args and references ito current_app.storage were changed to app_storage. For tests that access storage indirectly (for example, via a test journalist or source application), the Storage.get_default() method must be mocked to return the app_storage fixture. This patch has been added in the source_app and journalist_app fixtures, so it will be applied automatically in tests that use them.
If the instance config is updated in the Journalist Interface, the change should be reflected in both web applications immediately. To allow for this, an optional refresh argument forcing a database lookup was added, and the source and journalist apps were updated to use the refresh option before each request. Subsequent uses of the InstanceConfig within a request should not need to hit the database.
… as per PR review
aba1749
to
39eb634
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look great. Try as I might, I'm unable to break it, which is grand. =) Tested via the prod VM upgrade scenario, and ran through the full test plan. Diff is quite readable, and post-reorg, the code is a bit more intuitive IMO. Great work on this one, @zenmonkeykstop—and thanks also to @legoktm for detailed review.
v3.0.2 was diff-reviewed in freedomofpress/securedrop#6217.
v3.0.2 was diff-reviewed in freedomofpress/securedrop#6217.
v3.0.2 was diff-reviewed in freedomofpress/securedrop#6217.
v3.0.2 was diff-reviewed in freedomofpress/securedrop#6217.
Status
Ready for review (but test plan TK)
Description of Changes
Fixes #6154.
This PR updates the application Flask version to 2.0.2 and updates associated dependencies. It also refactors out the app.storage and app.instance_config dynamic attributes and corrects several typing errors, in order to allow for successul static typechecking via mypy.
Application refactor:
.get_default()
class method that creates the global object if it doesn't already exist, and then returns it.Test updates:
Tests have been updated to account for the refactor above. An app_storage fixture was added, returning a function-scoped Storage object. For tests which access storage directly, that fixture was added to their args and references like
current_app.storage.whatever()
were changed toapp_storage.whatever()
. For tests that access storage indirectly (for example, via a test journalist or source application), theStorage.get_default()
method must be mocked to return theapp_storage fixture
. This patch has been added in thesource_app
andjournalist_app
fixtures, so it will be applied automatically in tests that use them.General notes:
TheFixed in Stop using app.babel_instance #6220babel_instance
Flask app dynamic attribute has not been refactored out - it's added byflask-babel
and an upstream change is probably requiredapp.error_handler_spec
structureTesting
Storage-specific tests
Secure temporary files
make dev-tor
(some latency will help when observing the tempfile behaviour)docker exec -t securedrop-dev-0 bash
watch -n 1 ls -l /tmp
.aes
extension are written and then deleted as the upload completes (the first is the gzipped initial submission, second is decompressed version)delete=False
argument to the super().__init() in the SecureTemporaryFile class and submitting another file larger than 512kSubmissions and replies
make dev
docker exec -t securedrop-dev-0 bash
/var/lib/securedrop/store/<UUID>/
paths for the respective sourcesInstance config tests
make dev
, then log in to the JIEnd-to-end tests
2.1.0
test plan as you have time and patience for.Upgrade scenario
Deployment
These application changes will be deployed with the next release containing them. There are no schema changes or data migrations, but 2 of the alembic migration scripts were modified to reference the new global Storage object - as such, attention should be paid during testing to QA scenarios involving upgrades to verify that they work as expected.
Checklist
If you made changes to the server application code:
make lint
) and tests (make test
) pass in the development containerIf you made changes to the system configuration:
If you made non-trivial code changes:
Choose one of the following:
If you added or updated a production code dependency:
Production code dependencies are defined in:
admin/requirements.in
admin/requirements-ansible.in
securedrop/requirements/python3/securedrop-app-code-requirements.in
If you changed another
requirements.in
file that applies only to developmentor testing environments, then no diff review is required, and you can skip
(remove) this section.
Choose one of the following: