Skip to content
This repository has been archived by the owner on Jan 7, 2024. It is now read-only.

Adds APIProxy class to use securedrop-proxy #21

Merged
merged 14 commits into from Oct 16, 2018
Merged

Adds APIProxy class to use securedrop-proxy #21

merged 14 commits into from Oct 16, 2018

Conversation

kushaldas
Copy link
Contributor

@kushaldas kushaldas commented Oct 11, 2018

The APIProxy class can now use securedrop-proxy service in the QubesOS. By default sd-journalist is the proxy name, but, we can override it using a configuration file at /etc/sd-sdk.conf file.

[proxy]
name = proxy-debian

How to test?

pipenv run python -m unittest discover -v tests

We also found freedomofpress/securedrop-proxy#131 while working on this.

Fixes freedomofpress/securedrop-proxy#16.

We can now use the securedrop-proxy to fetch data from the server.
This has the initial authentication and get_source* methods.
The methods have same calling signature as in ``API`` class,
thus will help the developers to use the either as required.
We now have a decorator dastollervey_datasaver which can help
to mock out qrexec calls to securedrop-proxy service.
This saves the mock input/output into data/ directory.
Uses dastollervey_datasaver decorator to save the mock data.
The default proxy vmname is sd-journalist. If you want to override
it, add a file at ``/etc/sd-sdk.conf`` with the following details:

[proxy]
name = proxy-debian

The above example updates the proxy vmname to be ``proxy-debian``.
We have two methods to download replies and submissions
@kushaldas
Copy link
Contributor Author

Blah, now I should test it on my normal box.

@kushaldas
Copy link
Contributor Author

Tests are failing because of the randomness of the dictionary.

@redshiftzero
Copy link
Contributor

hey I started DRYing up the API and APIProxy classes in 218eaac, check it out and let me know what you think (still a few test failures to resolve, but an approach like this should trim down the added non-test code by 650 lines or so)

@kushaldas
Copy link
Contributor Author

hey I started DRYing up the API and APIProxy classes in 218eaac, check it out and let me know what you think (still a few test failures to resolve, but an approach like this should trim down the added non-test code by 650 lines or so)

This is a good start, I will look at the failing tests today.

@kushaldas
Copy link
Contributor Author

Finally managed to fix rest of the API calls and also the test cases. I have to push new test JSON files for the same. c711bd5 has changes. Please let me know what do you think.

@redshiftzero
Copy link
Contributor

c711bd5 looks great, exactly what i was looking for!

ETags are being stripped from staging/production servers
due to an Apache misconfiguration.

For now we should not use the ETag until this is addressed
server-side.
Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

approving - diff looks great, thanks for doing the DRYing up :-)

Manual testing done here: I verified that using the SDK in the sd-svs AppVM in Qubes with the securedrop-proxy service worked without issue - I was able to download submissions from sources 🎉 (note that I did not manually test every API endpoint and am relying on unit tests for that).

@redshiftzero
Copy link
Contributor

Hey @ntoll I just want to check prior to my merging this that you're not using etags for the ticket you're currently working on for the client - they are provided by API.download_submission and API.download_reply (reference). If so, we'll need to snip out that logic and drop in a branch until defect freedomofpress/securedrop#3877 is addressed on the server side (the ETag header is present in the development environment but not in staging/production servers, so you probably have not hit any issues accessing that header).

@ntoll
Copy link
Contributor

ntoll commented Oct 16, 2018

@redshiftzero nope... I'm not (knowingly) using etags, I'm consciously trying to do the simplest possible thing via this API layer. :-) I think you're safe to merge, and I'll certainly yelp if something breaks.

@redshiftzero
Copy link
Contributor

ah great, thanks for confirming @ntoll ! mergin'

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants