calculated etag as sha256 of file on disk#4314
Conversation
kushaldas
left a comment
There was a problem hiding this comment.
I have two small comments, otherwise, approved from me.
| except AttributeError: | ||
| pass | ||
|
|
||
| if getattr(self, 'env', 'prod') == 'test': |
There was a problem hiding this comment.
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.
We never set a default value so we're kinda stuck with this for now :/
emkll
left a comment
There was a problem hiding this comment.
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 |
kushaldas
left a comment
There was a problem hiding this comment.
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
redshiftzero
left a comment
There was a problem hiding this comment.
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
checksumto thesubmissionsandrepliestables. Use this value as theETagheader 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
pytestfixtures across the subprocess boundary../bin/dev-shell./bin/run-test -vv tests/test_alembic.py::test_upgrade_with_data[b58139cfdc8c]cat /tmp/test_rqworker.logOperationalError. 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
checksumvalue 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: