Check for network connection before running preflight updater

[rocodes]

Status

Ready for review

Description of Changes

Fixes #633
Towards #649

Changes proposed in this pull request:

• Add network connectivity check before running Preflight Updater, Only launch UpgradeThread if network check is successful.
• If network check is unsuccessful, show an error message.
• Add override command-line option, --skip-netcheck, to bypass the network check and allow the Prelight Updater to run as usual (this is for development/testing purposes).
• Add tests for UpdaterApp.py to test the above functionality.

Testing

Manual testing in Qubes/dom0

Check out this branch and make clone it into dom0. Disconnect your computer from the internet.

• run python3 /opt/securedrop/launcher/sdw-launcher.py and click "Apply Updates." Observe an immediate network failure error message.
• Reconnect to the internet and rerun the launcher script. Ensure you can successfully run the preflight updater.
• Ensure the above steps work with both ethernet and wireless connections.

Manual testing outside of dom0

• check out this branch, activate your virtual environment, and run python3 /opt/securedrop/launcher/sdw-launcher.py. When you attempt to apply updates you should see the "network failed" message (regardless of your network status).
• Now run python3 /opt/securedrop/launcher/sdw-launcher.py --skip-netcheck and you should be able to continue to the updater "run" (which will not actually run or apply updates since you're outside of dom0). The network error dialog view should not appear. The updater "run" will finish with a failure message. (Note: you will encounter error messages printed to the console relating to the failure to run some updater functionality outside of dom0:
  sudo: qubes-dom0-update: command not found,
  sudo: qubesctl: command not found, Running 'echo '3' > /home/user/.securedrop_client/sdw-update-status' on sd-app \ sd-app: Failed to access 'default_user' property)
• Ensure the above steps work with both the PyQt4 and the PyQt5 environments. You will need to export SDW_UPDATER_QT=4 for PyQt4 (and 5 for PyQt5) as per these instructions.

[rocodes]

This is failing on CI due to #745, which I've proposed a fix for, but make check passes locally in both PyQt4-based and PyQt5-based venvs

[rocodes]

@conorsch I've marked this as "Ready for review" because I'm ready for feedback, but #746 needs to land first before this will pass on CI.

[rocodes]

This is a little bit brittle because it relies on the name of the virtual environment to decide whether we should import PyQt4 or PyQt5. If a user didn't follow the developer docs (which include Makefile targets for PyQt4-based venv and PyQt5 based venv) and made up an unexpected name for their pyqt4 venv, this check wouldn't work. However, it would default to PyQt5, which is the newer/more common standard.

I'm open to feedback if there's a better way of managing this for testing purposes though!

[conorsch]

All prod installs on Qubes 4.0 are still using PyQt4, via F25 in dom0, so that should be the default. The logic in the if statement on L14 seems to invert the "use 4 unless 5 is requested" pragma introduced back in #610. Taking a closer look, we're using Python3.8 in CI to test the launcher code, but dom0 is still on 3.5. This isn't a regression introduced by your changes, @rocodes, but your changes are surfacing them. =) I'll take a look at what switching CI over to a more prod-like testing environment would take, and report back.

[rocodes]

Thanks for fixing CI, lifesaver. :)

My comment here (around PyQt4 vs PyQt5 being the default) was only relevant to machines that are performing unit testing (for machines that run test_updaterapp.py specifically). But in any case, I've reworked this section to flip the logic to be more consistent with the rest of the codebase, and to check the SDW_UPDATER_QT environment variable to decide which version of PyQt to import (note: this means test environments must follow the dev docs and always set the environment variable before running tests).

[conorsch]

@rocodes I took a look at the CI and unbroke it as best I could. Also sprinkled in some container updates as described in freedomofpress/securedrop-client#1384, because it was convenient.

Work in progress

The tests are passing, so to my eye, this PR is ready for review! Happy to run through a deep review if you feel you're finished with logic changes for now.

[rocodes]

@conorsch yup, ready for you, and thanks for your work on CI :)

[conorsch]

Encountered two (2) problems during local testing:

1. network check always returns False, so updater doesn't run; see comments about --pass-io
2. some of the helpful logging messages never make it to disk; the problem seems only to affect logging calls like L206; I did see log messages like those on L139.

Magnificent test plan, thanks for taking the time!

[conorsch]

Logging is good! More logging! However, I wasn't able to find this message in the logs during my testing. Logging the actual return value would be helpful during debugging sessions. From the docs:

If the return code was non-zero it raises a CalledProcessError. The CalledProcessError object will have the return code in the returncode attribute and any output in the output attribute.

So I suggest something like:

except subprocess.CalledProcessError as e:
    logger.error("{} (connectivity check) failed; state reported as '{}'".format(command.decode("utf-8"), e.output))

Make sure you can find that string in the on-disk log files when a failure event is hit.

[rocodes]

Thank you -addressed in d5f2d1d (Note this log message will not be encountered in a no-network scenario; in that case nmcli will still return 0 but the error log will read Network connectivity check failed).

[conorsch]

Logically the _is_netcheck_successful seems like a utility to me, and therefore best resides in sdw_util/. That's a nit, so don't change it just for cleanliness's sake, but given the logging problems mentioned in this review on L206, maybe moving into utils will make the logging config easier to bootstrap.

[rocodes]

I left this as-is because this is currently the only place we have a need for connectivity checking in the Updater code, and I have a vague future wish to disentangle some of the dependencies in UI classes (so eg Updater.py can import Util.py, but maybe some day in the distant future the UpdaterApp, more UI-focused, won't have dependencies outside of the sdw_updater_gui package). However, I agree that some methods (this netcheck, and maybe launch_securedrop_client) could belong elsewhere.

[conorsch]

Summarizing out of band discussion: we went ahead and merged the CI fixes in a separate PR (#752), so @rocodes is going to rebase here and ping when it's ready for a second look.

[eaon]

This looks and works great! Followed the test plan, success rate was 💯 Logging also works as expected, showing connectivity or lack thereof in dom0 and failure to detect Qubes OS outside of dom0, including netcheck skips.

Nice work!

[conorsch]

Looks great! Tested mostly in dom0, and confirmed that log messages were recorded correctly when network was disconnected. Online via wifi or ethernet was correctly understood as "online". Dabbled with the qt4/pqt5 differences in the dev env, and both appeared to work well (with qt5 unsurprisingly looking subtly spiffier).

Thanks for your patience and fortitude on getting this change to land, @rocodes!