-
Notifications
You must be signed in to change notification settings - Fork 687
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
Fixes #4521 download journalist key via source interface #4523
Conversation
We need different code for Python2 and Python3 to download journalist key via the source interface. This also adds a function test for the same.
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.
Thanks @kushaldas the fix looks good to me. Functional testing performed as follows:
Testing in dev container
While it does, hitting the journalist-key endpoint does return an error and an empty journalist-key.asc file:
172.17.0.1 - - [14/Jun/2019 15:27:37] "GET /journalist-key HTTP/1.1" 200 -
Error on request:
Traceback (most recent call last):
File "/usr/local/lib/python2.7/dist-packages/werkzeug/serving.py", line 270, in run_wsgi
execute(self.server.app)
File "/usr/local/lib/python2.7/dist-packages/werkzeug/serving.py", line 261, in execute
write(data)
File "/usr/local/lib/python2.7/dist-packages/werkzeug/serving.py", line 241, in write
assert isinstance(data, bytes), 'applications must write bytes'
AssertionError: applications must write bytes
- I no longer observe the error on this branch
Upgrade testing
On this branch:
- make build-debs
- make upgrade-start
- 0.13.0 box is pulled down, journalist-key endpoint 404's
- make upgrade-test local
- Observe string change and journalist-key endpoint is now working, key is served with no error
|
||
if six.PY3: | ||
data = data.decode('utf-8') | ||
assert "BEGIN PGP PUBLIC KEY BLOCK" in data |
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 should properly assess success/failure: journalist key served was an empty 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.
@emkll - do you mean you got an empty file testing this PR, or 0.13.0 returns an empty file for you? I was seeing a 404 on 0.13.0.
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.
Sorry for the confusion:
- In dev env, was getting an empty key (and an error in the logs)
- In staging VMs, was getting 404
Tested as follows:
|
|
||
if six.PY3: | ||
data = data.decode('utf-8') | ||
assert "BEGIN PGP PUBLIC KEY BLOCK" in data |
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 corresponding unit test in tests/test_source.py::test_lookup
did not fail because the assert causing the failure (assert isinstance(data, bytes), 'applications must write bytes'
) from werkzeug/serving.py
does not occur in test since the dev server is not running: this underscores the importance of having full functional test coverage of all endpoints
Thanks @redshiftzero for the investigation into why the unit test has failed us (#4523 (comment)) and for opening #4526 to improve integration testing to avoid regressions like these in the future. Merging based on the 2 approving reviews above. |
Status
Ready for review.
Description of Changes
Fixes #4521
We need different code for Python2 and Python3 to download
journalist key via the source interface. This also adds a functional
test for the same.
Testing
make test
Deployment
Any special considerations for deployment? Consider both:
Checklist
If you made changes to the server application code:
make lint
) and tests (make -C securedrop test
) pass in the development containerIf you made changes to
securedrop-admin
:make -C admin test
) pass in the admin development containerIf you made changes to the system configuration:
If you made non-trivial code changes:
If you made changes to documentation:
make docs-lint
) passed locally