Using pure python code to download and verify gpg encrypted files/messages #3692

kushaldas · 2018-08-03T13:04:43Z

Status

Ready for review

Description of Changes

Changes proposed in this pull request:

Testing

./bin/dev-shell ./bin/run-test --capture=no -v tests/functional/

To test against the local container, and then create an instance_information.json file for an external staging server, and then against the same.

Deployment

Any special considerations for deployment? Consider both:

Upgrading existing production instances.
New installs.

Checklist

If you made changes to the server application code:

Linting (make ci-lint) and tests (make -C securedrop test) pass in the development container

If you made changes to `securedrop-admin`:

Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

Configuration tests pass

If you made non-trivial code changes:

I have written a test plan and validated it for this PR

If you made changes to documentation:

Doc linting (make docs-lint) passed locally

Removing old method calls from user creation logic, this is only used inside of the container for the functional testing.

The test requirements now have requests[socks] as dependency. Using the same we are now directly downloading the files/messages from the .onion address for functional tests. The old external command file also got removed this committ. We are creating the gpg object for both container based local testing and external testing (in functional tests). Fixes: freedomofpress#3691 freedomofpress#3687

zenmonkeykstop · 2018-08-04T17:44:30Z

Tested as follows:

make build-debs
cd securedrop
./bin/dev-shell ./bin/run-test tests/functional

All functional tests pass locally again, LGTM.

This will fail against staging until the app-test role is fixed, so I didn't do the instance_information.json version of the test. But if this is merged into tbb-0.9.0 then PR #3697 will get staging working too.

redshiftzero

diff looks good (one nit inline), just needs testing on external server to verify proxies logic works in return_downloaded_content

redshiftzero · 2018-08-06T18:08:22Z

securedrop/tests/functional/journalist_navigation_steps.py

@@ -640,14 +641,13 @@ def _journalist_downloads_message(self):

        # Downloading files with Selenium is tricky because it cannot automate
        # the browser's file download dialog. We can directly request the file
-        # using urllib2, but we need to pass the cookies for the logged in user
+        # using requests, but we need to pass the cookies for the logged in user
        # for Flask to allow this.
        def cookie_string_from_selenium_cookies(cookies):


Nit: this function name is now misleading so we should update it since now it's returning a dict for requests

redshiftzero · 2018-08-06T18:10:12Z

securedrop/tests/functional/functional_test.py

@@ -279,7 +268,7 @@ def setup(self, session_expiration=30):
            # This user is required for our tests cases to login
            self.admin_user = {
                                "name": "journalist",
-                                "password": "WEjwn8ZyczDhQSK24YKM8C9a",
+                                "password": "correct horse battery staple profanity oil chewy",


thanks for updating this 👍

conorsch · 2018-08-07T01:06:30Z

Having some trouble testing this against remote servers (#3697 is intended to fix). So far, looks like much of the struggle I've had relates to formatting problems on the instance_information.json file.

Here's a brief report on state of functional tests passing

tests/functional/test_admin_interface.py::TestAdminInterface::test_admin_interface PASSED
tests/functional/test_admin_interface.py::TestAdminInterface::test_admin_edits_hotp_secret FAILED
tests/functional/test_admin_interface.py::TestAdminInterface::test_admin_deletes_user FAILED
tests/functional/test_admin_interface.py::TestAdminInterface::test_admin_updates_image PASSED
tests/functional/test_admin_interface.py::TestAdminInterface::test_ossec_alert_button PASSED
tests/functional/test_journalist.py::TestJournalist::test_journalist_verifies_deletion_of_one_submission_modal FAILED
tests/functional/test_journalist.py::TestJournalist::test_journalist_uses_col_delete_collection_button_modal FAILED
tests/functional/test_journalist.py::TestJournalist::test_journalist_uses_index_delete_collections_button_modal FAILED
tests/functional/test_journalist.py::TestJournalist::test_journalist_interface_ui_with_modal FAILED
tests/functional/test_make_account_changes.py::TestMakeAccountChanges::test_admin_edit_account_html_template_rendering FAILED
tests/functional/test_source.py::TestSourceInterface::test_lookup_codename_hint PASSED
tests/functional/test_source_notfound.py::TestSourceInterfaceNotFound::test_not_found PASSED
tests/functional/test_source_session_timeout.py::TestSourceSessions::test_source_session_timeout FAILED
tests/functional/test_source_warnings.py::TestSourceInterfaceBannerWarnings::test_warning_appears_if_tor_browser_not_in_use PASSED
tests/functional/test_source_warnings.py::TestSourceInterfaceBannerWarnings::test_warning_appears_if_orbot_is_used PASSED
tests/functional/test_source_warnings.py::TestSourceInterfaceBannerWarnings::test_warning_high_security PASSED
tests/functional/test_submission_not_in_memory.py::TestSubmissionNotInMemory::test_message_is_not_retained_in_memory xfail
tests/functional/test_submission_not_in_memory.py::TestSubmissionNotInMemory::test_file_upload_is_not_retained_in_memory xfail
tests/functional/test_submit_and_retrieve_file.py::TestSubmitAndRetrieveFile::test_submit_and_retrieve_happy_path FAILED
tests/functional/test_submit_and_retrieve_file.py::TestSubmitAndRetrieveFile::test_source_cancels_at_login_page PASSED
tests/functional/test_submit_and_retrieve_file.py::TestSubmitAndRetrieveFile::test_source_cancels_at_submit_page PASSED
tests/functional/test_submit_and_retrieve_message.py::TestSubmitAndRetrieveMessage::test_submit_and_retrieve_happy_path

Regarding the formatting tweaks, here are a few rules I've sketched out based on my understanding of instance_information.json:

journalist_location must be prefixed with http://; even though there's conditional logic to related to making the prefix optional, an exception is raised if omitted
user.secret must not contain whitespace; even though the ./manage.py output contains whitespace, the test logic doesn't seem to strip it.

It's also necessary to create a test user manually, via ./manage.py; running ./create-dev-data.py will cause all tests to fail. See #3672 for a potential fix on that front.

kushaldas · 2018-08-07T02:12:38Z

@conorsch can you please add "sleep_time": 30 in the instance_information.json and retry the tests? I wonder where they fail.

redshiftzero · 2018-08-07T19:47:29Z

Based on conversations with @kushaldas, the steps to test against an external server interactively (i.e. not using xvfb or Tor Browser in the container) are:

Provision staging VMs on develop (due to bug [functional testing] Remove TBB install logic from app-test role #3678)
Ensure that the staging VM database is clean (i.e. use ./manage.py reset)
Manually create a test journalist user using the credentials from create-dev-data.py
Comment out two lines in functional/functional_test.py:

self.xvfb_display = start_xvfb()
stop_xvfb(self.xvfb_display)

Install latest tor browser (7.5.6) inside ~/.local/tbb/
Install the securedrop requirements and test requirements in a virtualenv
Install geckodriver 0.17.0 somewhere in your path
Create instance information JSON file in the functional tests directory:

{
    "hidserv_token": "asfjsdfag",
    "journalist_location": "http://thejournalistfqb.onion",
    "source_location": "http://thesourceadsfa.onion",
    "sleep_time": 10,
    "user": {
        "name": "journalist",
        "password": "WEjwn8ZyczDhQSK24YKM8C9a",
        "secret": "JHCOGO7VCER3EJ4L"
    }
}

Run tests with pytest -v --capture=no functional/test_journalist.py

I am using these steps to test now and will report back on this ticket.

kushaldas · 2018-08-07T19:49:57Z

I also found that you can create the staging vm from the same branch itself, when it will fail in the tbb ansible things, just ssh into vagrant app-staging and restart the apache2 process.

redshiftzero · 2018-08-07T19:53:49Z

Well, to reduce the number of variables while testing, if someone is testing this interactively, let's have them follow these steps for clarity.

kushaldas · 2018-08-07T19:56:45Z

The same tests which I had failing before, are now passing while the only thing I changed is that I am on 100Mbps connection.

kushaldas · 2018-08-07T20:32:06Z

Here is an example instance_information.json for me

{
    "hidserv_token": "mysirvicetosen",
    "journalist_location": "http://cskslw43sv.onion",
    "source_location": "http://nwjl3srmkx.onion",
    "user": {
        "name": "journalist",
        "password": "correct horse battery staple profanity oil chewy",
        "secret": "JHCOGO7VCER3EJ4L"
    }
}

conorsch · 2018-08-08T16:29:03Z

By raising the test timeouts, I've gotten most of the tests to pass when running over tor against a local server. There are some flakey performance issues still, but optimistic that #3486 will add some stability.

After this goes in, #3697 should be tested next (after rebase), then #3672.

msheiny

Despite the tests timeout and a few failures .... i vote we keep this moving into the feature branch.. we have way too many lingering PRs that need to go in here that its becoming a bottleneck to review. Obv. before merge into develop everything has to pass but lets work out of one concise branch for that resolution

conorsch · 2018-08-08T22:14:55Z

@msheiny I'm with you on that, especially since driving forward here will unblock the upcoming CI work, which will give us a heck of a lot more clarity on test behavior, as well as minimize review time by running the tests in CI, rather than on developer workstations. Let's keep moving!

conorsch

Approving for merge into the tbb-0.9.0 feature branch. There's more work to do, and plenty of outstanding PRs. Looking forward to hammering CI into respectable shape on the feature branch to increase confidence in the new test logic.

kushaldas added 2 commits August 3, 2018 13:45

Updates test user creation logic inside of container

164daea

Removing old method calls from user creation logic, this is only used inside of the container for the functional testing.

kushaldas requested review from heartsucker and redshiftzero as code owners August 3, 2018 13:04

kushaldas mentioned this pull request Aug 4, 2018

[functional testing] functional tests fail at setup when run locally #3696

Closed

zenmonkeykstop mentioned this pull request Aug 4, 2018

3678 remove apptest browser installs #3697

Merged

redshiftzero reviewed Aug 6, 2018

View reviewed changes

msheiny approved these changes Aug 8, 2018

View reviewed changes

conorsch self-requested a review August 8, 2018 22:15

conorsch approved these changes Aug 8, 2018

View reviewed changes

conorsch merged commit 46aebc0 into freedomofpress:tbb-0.9.0 Aug 8, 2018

zenmonkeykstop mentioned this pull request Sep 28, 2018

[functional testing] Functional tests using GPG fail when run against staging VMs #3687

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using pure python code to download and verify gpg encrypted files/messages #3692

Using pure python code to download and verify gpg encrypted files/messages #3692

kushaldas commented Aug 3, 2018

zenmonkeykstop commented Aug 4, 2018 •

edited

Loading

redshiftzero left a comment

redshiftzero Aug 6, 2018

redshiftzero Aug 6, 2018

conorsch commented Aug 7, 2018 •

edited

Loading

kushaldas commented Aug 7, 2018

redshiftzero commented Aug 7, 2018 •

edited

Loading

kushaldas commented Aug 7, 2018

redshiftzero commented Aug 7, 2018

kushaldas commented Aug 7, 2018

kushaldas commented Aug 7, 2018

conorsch commented Aug 8, 2018

msheiny left a comment

conorsch commented Aug 8, 2018

conorsch left a comment

Using pure python code to download and verify gpg encrypted files/messages #3692

Using pure python code to download and verify gpg encrypted files/messages #3692

Conversation

kushaldas commented Aug 3, 2018

Status

Description of Changes

Testing

Deployment

Checklist

If you made changes to the server application code:

If you made changes to securedrop-admin:

If you made changes to the system configuration:

If you made non-trivial code changes:

If you made changes to documentation:

zenmonkeykstop commented Aug 4, 2018 • edited Loading

redshiftzero left a comment

Choose a reason for hiding this comment

redshiftzero Aug 6, 2018

Choose a reason for hiding this comment

redshiftzero Aug 6, 2018

Choose a reason for hiding this comment

conorsch commented Aug 7, 2018 • edited Loading

kushaldas commented Aug 7, 2018

redshiftzero commented Aug 7, 2018 • edited Loading

kushaldas commented Aug 7, 2018

redshiftzero commented Aug 7, 2018

kushaldas commented Aug 7, 2018

kushaldas commented Aug 7, 2018

conorsch commented Aug 8, 2018

msheiny left a comment

Choose a reason for hiding this comment

conorsch commented Aug 8, 2018

conorsch left a comment

Choose a reason for hiding this comment

If you made changes to `securedrop-admin`:

zenmonkeykstop commented Aug 4, 2018 •

edited

Loading

conorsch commented Aug 7, 2018 •

edited

Loading

redshiftzero commented Aug 7, 2018 •

edited

Loading