-
Notifications
You must be signed in to change notification settings - Fork 686
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
calculated etag as sha256 of file on disk #4314
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.
I have two small comments, otherwise, approved from me.
@@ -160,5 +160,10 @@ def __init__(self): | |||
except AttributeError: | |||
pass | |||
|
|||
if getattr(self, 'env', 'prod') == 'test': |
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 really wish this env
value to be set in the top line in case env
is missing in the old config. That way one less getattr
call.
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.
We never set a default value so we're kinda stuck with this for now :/
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.
Testing in the dev env was successful (etag header return is identical to the sha256sum of the encrypted file). I used the instructions provided in #4023 .
To ensure the headers are properly passed by Apache, also ran the test against staging VMs as follows:
[X] Build deb packages on this branch and provision staging VMs
[ ] Submit a file (:exclamation: see below)
[ ] Create Journalist account, add JI ATHS token to torrc and restart tor
[ ] Retrieve file via API via Journalist interface
[ ] Verify sha256sum of encrypted file is same as etag header
Submitting a file causes a server error, the contents of /var/log/apache2/source-error.log
are as follows:
[Mon Apr 15 15:29:07.585403 2019] [wsgi:error] [pid 1496:tid 3761315505920] ERROR:flask.app:Exception on /submit [POST]
[Mon Apr 15 15:29:07.585429 2019] [wsgi:error] [pid 1496:tid 3761315505920] Traceback (most recent call last):
[Mon Apr 15 15:29:07.585433 2019] [wsgi:error] [pid 1496:tid 3761315505920] File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 2292, in wsgi_app
[Mon Apr 15 15:29:07.585437 2019] [wsgi:error] [pid 1496:tid 3761315505920] response = self.full_dispatch_request()
[Mon Apr 15 15:29:07.585446 2019] [wsgi:error] [pid 1496:tid 3761315505920] File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 1815, in full_dispatch_request
[Mon Apr 15 15:29:07.585450 2019] [wsgi:error] [pid 1496:tid 3761315505920] rv = self.handle_user_exception(e)
[Mon Apr 15 15:29:07.585453 2019] [wsgi:error] [pid 1496:tid 3761315505920] File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 1718, in handle_user_exception
[Mon Apr 15 15:29:07.585456 2019] [wsgi:error] [pid 1496:tid 3761315505920] reraise(exc_type, exc_value, tb)
[Mon Apr 15 15:29:07.585459 2019] [wsgi:error] [pid 1496:tid 3761315505920] File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 1813, in full_dispatch_request
[Mon Apr 15 15:29:07.585461 2019] [wsgi:error] [pid 1496:tid 3761315505920] rv = self.dispatch_request()
[Mon Apr 15 15:29:07.585464 2019] [wsgi:error] [pid 1496:tid 3761315505920] File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 1799, in dispatch_request
[Mon Apr 15 15:29:07.585467 2019] [wsgi:error] [pid 1496:tid 3761315505920] return self.view_functions[rule.endpoint](**req.view_args)
[Mon Apr 15 15:29:07.585470 2019] [wsgi:error] [pid 1496:tid 3761315505920] File "/var/www/securedrop/source_app/decorators.py", line 12, in decorated_function
[Mon Apr 15 15:29:07.585473 2019] [wsgi:error] [pid 1496:tid 3761315505920] return f(*args, **kwargs)
[Mon Apr 15 15:29:07.585475 2019] [wsgi:error] [pid 1496:tid 3761315505920] File "/var/www/securedrop/source_app/main.py", line 208, in submit
[Mon Apr 15 15:29:07.585478 2019] [wsgi:error] [pid 1496:tid 3761315505920] store.async_add_checksum_for_file(sub)
[Mon Apr 15 15:29:07.585481 2019] [wsgi:error] [pid 1496:tid 3761315505920] File "/var/www/securedrop/store.py", line 210, in async_add_checksum_for_file
[Mon Apr 15 15:29:07.585484 2019] [wsgi:error] [pid 1496:tid 3761315505920] current_app.config['SQLALCHEMY_DATABASE_URI'],
[Mon Apr 15 15:29:07.585486 2019] [wsgi:error] [pid 1496:tid 3761315505920] File "/var/www/securedrop/worker.py", line 34, in enqueue
[Mon Apr 15 15:29:07.585489 2019] [wsgi:error] [pid 1496:tid 3761315505920] return (self.__app or current_app).extensions[self.__EXT_NAME].enqueue(*nargs, **kwargs)
[Mon Apr 15 15:29:07.585492 2019] [wsgi:error] [pid 1496:tid 3761315505920] KeyError: 'rq-worker-queue’
fbbb3e4
to
29e6f7b
Compare
29e6f7b
to
7a9ed83
Compare
@emkll I'm pretty sure I fixed the bug your reported, but I am in molecule/vagrant hell and can't get things to boot right. I'll see if I can get that sorted out later. |
Thanks @heartsucker the issue has been fixed. Functional testing successful following this plan: [X] Build deb packages on this branch and provision staging VMs Visual review of the diff looks good to merge from my perspective, but I'd like another developer to perform a visual review of this PR |
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.
Tested again:
- Build deb packages on this branch and provision staging VMs
- Submit a file and a message
- Create Journalist account, added ATHS token to torrc and restart tor
- Retrieve file and message via API via Journalist interface
- Verify sha256sum of downloaded file/message are the same as etag header presented
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.
lgtm @heartsucker, all comments were addressed after the removal of hasattr
, approving based on visual review of the diff. thanks @emkll and @kushaldas for testing and review!
Status
Ready for review
Description of Changes
Fixes #4032
Add the column
checksum
to thesubmissions
andreplies
tables. Use this value as theETag
header in the API responses.Testing
CI covers most of this, but there is one part that needs manual review because of the complexity of injecting
pytest
fixtures across the subprocess boundary../bin/dev-shell
./bin/run-test -vv tests/test_alembic.py::test_upgrade_with_data[b58139cfdc8c]
cat /tmp/test_rqworker.log
OperationalError
. The tables are missing because the app context in the alembic migration uses the prod values.Additionally, the part of the migration is allowed to fail because it is covered by the fact that the columns are nullable and the
checksum
value is populated when the response is generated if it is missing.Deployment
This PR has a migration and it should be checked that it is sufficiently robust.
Checklist
If you made changes to the server application code:
make ci-lint
) and tests (make -C securedrop test
) pass in the development containerIf you made non-trivial code changes: