[wip] Add Tor Browser-based functional testing #4029
Conversation
This prevents the accidental commit of private information.
The 'when' conditional detecting a grsec kernel, used for running the paxctl commands on the TBB binary, was needlessly duplicated on the relevant task. Fortunately that didn't cause breakage, because the 'when' lines were identical, but only one was active.
Now installs Firefox 52 ESR, rather than Firefox 46, for use inside the
test container.
Includes changes to run-test shell script:
TOR_FORCE_NET_CONFIG=0 is required to directly connect to Tor
network, otherwise it will wait for userinput to either connect
or to configure
The `run_xvfb` invocation is no longer necessary, since the test suite
code bootstraps the headless server now.
Bootstrapping the application services within the functional test suite. Includes some cleanup, culling unused debugging code, and also cleans up the various print statements. Ignore functional test firefox logs (thanks, @msheiny!) Adds retries for tor network connection failure, using the pre-existing logic.
The version of torsocks in the Trusty repos isn't recent enough to support custom ports. Rather than install from other sources, which requires manual package verification (or configuring non-trusty repos, which could break other packages), let's fall back to good ol' nc.
We need to create a new firefox profile to test the orbot specific warning. This works for both locally and over Tor.
Now we can safely execute the account changes in the tests running on the Tor browser. The logic update makes sure to create different user for this test than any other test.
For whatever reason, this Firefox signing key was not available on the keyserver in the prior diff, but was available on Mozilla's keyserver.
Removed time.sleep()s, updated TBB version in Dockerfile
redshiftzero
requested review from
conorsch,
emkll,
heartsucker,
kushaldas and
msheiny
as code owners
Codecov Report
@@ Coverage Diff @@
## develop #4029 +/- ##
===========================================
- Coverage 84.7% 84.56% -0.14%
===========================================
Files 43 43
Lines 2765 2767 +2
Branches 300 300
===========================================
- Hits 2342 2340 -2
- Misses 355 359 +4
Partials 68 68
Continue to review full report at Codecov.
|
| wget https://www.torproject.org/dist/torbrowser/${TBB_VERSION}/tor-browser-linux64-${TBB_VERSION}_en-US.tar.xz.asc && \ | ||
| gpg --verify tor-browser-linux64-${TBB_VERSION}_en-US.tar.xz.asc tor-browser-linux64-${TBB_VERSION}_en-US.tar.xz | ||
|
|
||
| RUN tar -xvJf tor-browser-linux64-${TBB_VERSION}_en-US.tar.xz && \ |
There was a problem hiding this comment.
Is there a reason to make this a separate step? At the very least it seems that the extract should be a part of the previous RUN in order to cut down on the size of the intermediate layers.
| @@ -34,6 +60,8 @@ RUN pip install -r requirements/securedrop-app-code-requirements.txt && \ | |||
| pip install -r requirements/test-requirements.txt | |||
|
|
|||
| RUN if test $USER_NAME != root ; then useradd --no-create-home --home-dir /tmp --uid $USER_ID $USER_NAME && echo "$USER_NAME ALL=(ALL) NOPASSWD:ALL" >> /etc/sudoers ; fi | |||
| RUN cp -r /root/.local /tmp/ && chmod +x /tmp/.local/tbb/tor-browser_en-US/Browser/firefox && chmod -R 777 /tmp/.local | |||
There was a problem hiding this comment.
It might be worth squashing these three RUNs together to save space
| --rm \ | ||
| --user "${USER:-root}" \ | ||
| --volume "${TOPLEVEL}:${TOPLEVEL}" \ | ||
| --workdir "${TOPLEVEL}/securedrop" \ | ||
| -ti ${DOCKER_RUN_ARGUMENTS:-} securedrop-test "$@" | ||
| -ti ${DOCKER_RUN_ARGUMENTS:---rm --name securedrop-dev} securedrop-test "$@" |
There was a problem hiding this comment.
This --rm seems to be a duplicate since it is included above.
| @@ -7,4 +7,7 @@ py | |||
| pytest | |||
| pytest-cov | |||
| pytest-mock | |||
| selenium < 3 | |||
| requests[socks]>2.19.1 | |||
There was a problem hiding this comment.
We should specify in a comment why we're requiring at least this version.
| @@ -37,6 +36,9 @@ def _source_clicks_submit_documents_on_homepage(self): | |||
| assert submit_button_icon.is_displayed() is False | |||
| submit_button_hover_icon = self.driver.find_element_by_css_selector( | |||
| 'a#submit-documents-button > img.on-hover') | |||
| ActionChains(self.driver).move_to_element( | |||
| submit_button_hover_icon).perform() | |||
| self.wait_for(lambda: submit_button_hover_icon.is_displayed()) | |||
There was a problem hiding this comment.
Doing a wait_for on condition X then asserting X below seems redundant. There are several instances of this, but maybe we want to leave it for clarity / explicitness.
| @@ -14,11 +14,13 @@ def test_submit_and_retrieve_happy_path(self): | |||
| self._source_continues_to_submit_page() | |||
| self._source_submits_a_file() | |||
| self._source_logs_out() | |||
| self.swap_drivers() # To use Firefox | |||
There was a problem hiding this comment.
What is the reasoning for using both FF and Tor in a single test run? I think this should be clarified somewhere in a comment.
There was a problem hiding this comment.
The Tor Browser does not allow get any cookies, that is why we have to use both firefox and tor browser.
There was a problem hiding this comment.
Word. That's fair. I think we should note that somewhere so it's understood why it's happening since these tests are already complex.
| actions.send_keys(Keys.RETURN) | ||
| actions.perform() | ||
|
|
||
| def swap_drivers(self): |
There was a problem hiding this comment.
It might be worth adding a named argument here like
def swap_drivers(self, new_driver):
if new_driver == 'tor':
...That way at the call sites here it's clear why driver we're swapping to and not just via a comment. This would also robustify tests so that we're not unexpectedly swapping drivers when we really don't want. This could even raise an error to fail the test if we try to swap to the driver that's already in use.
Like are these swaps done because we are only swapping between FF and TB? Or at they also used to swap between two Tor instances, one pointing at the source interface and one pointing at the journalist interface?
|
|
||
| self.source_app = source_app.create_app(config) | ||
| self.journalist_app = journalist_app.create_app(config) | ||
| self.patcher2 = mock.patch('source_app.main.get_entropy_estimate') |
There was a problem hiding this comment.
Nit: I know we don't access this patcher2 anywhere except on teardown, but it could have a more specific name.
| tbb_sleep_time: 160 | ||
| tbb_selenium_user: journalist | ||
| tbb_selenium_password: correct horse battery staple profanity oil chewy | ||
| tbb_selenium_secret: JHCOGO7VCER3EJ4L |
There was a problem hiding this comment.
I think this variable could be renamed tbb_selenium_totp_secret because at first I thought this meant "secret that is for selenium".
| def _edit_user(self, username): | ||
| user = Journalist.query.filter_by(username=username).one() | ||
| def _edit_user(self, username, is_admin=False): | ||
| # XXXX |
There was a problem hiding this comment.
Is this a TODO? If so, can we add why it's here, and if not can we remove it?
|
Recapping what I said in Gitter earlier: there is so much divergence from develop, and the git history is so messy here in parts (i.e. we add logic that we then remove later in the history), I propose the fastest way to merge this is by merging in parts of the diff in separate PRs, cherry picking where it makes sense and making new commits where it doesn't. This will enable us to get the changes in here merged without making it too nightmarish for those working on this branch, i.e. no more rebasing 80+ commits and sometimes introducing errors during conflict resolution. These individual PRs would include:
Other PRs may make sense along the way, but these above changes that are all self-contained and can be merged separately to scope this task down to something that is hopefully easier for us to manage. What do y'all think @rmol @zenmonkeykstop @kushaldas ? |
|
Checked with Jen, OK to close. |
Status
This is getting merged in stages, sub PRs:
Description of Changes
Fixes #3659. Fixes #1502. We'll need to file a followup when this PR is merged for testing over Tor (we have #3661 but there could be an intermediate step which is getting the external server testing working prior to the CI integration).
Changes proposed in this pull request:
Testing
Does CI pass?
If flakes are discovered, we should resolve them in this PR.
Deployment
Testing only.
Checklist
If you made non-trivial code changes:
If you made changes to documentation:
make docs-lint) passed locally