Skip to content

Detect process conflicts with sdw-notify#454

Merged
conorsch merged 3 commits intomasterfrom
detect-notify-process-conflict
Feb 20, 2020
Merged

Detect process conflicts with sdw-notify#454
conorsch merged 3 commits intomasterfrom
detect-notify-process-conflict

Conversation

@eloquence
Copy link
Contributor

@eloquence eloquence commented Feb 14, 2020

Status

Ready for review

Description of Changes

Resolves #453.

We check for a defined list of processes using pgrep to ensure that sdw-notify does not run when provisioning or similar system updates are already underway.

Test plan

Setup:

  1. Check out this branch and run make prep-dom0
  2. Ensure that ~/.securedrop_launcher/sdw-last-updated does not exist, to force the notification to always appear.

Tests:

  • Run /opt/securedrop/launcher/sdw-notify.py. The notification should show up.
  • Run make test and run /opt/securedrop/launcher/sdw-notify.py while it is doing its thing. The notification should not appear, and you should see a log entry in ~/.securedrop_launcher/logs/sdw-notify.log explaining why.
  • (optional, for good measure) Run the Qubes GUI updater and kick off a graphical update process (qubesctl must be running in the process list). Run sdw-notify while that's underway. Result should be the same as above.

Checklist

  • Tested in Qubes
  • make flake8 passes
  • make test passes
  • no packaging changes
  • made with love

@eloquence eloquence marked this pull request as ready for review February 14, 2020 06:13
@eloquence
Copy link
Contributor Author

eloquence commented Feb 14, 2020

We will likely want to use this utility function in sdw-launcher.py as well, but I think that's best handled separately, as the preflight updater is an interactive application, and we need to think carefully about the interaction flow in this case (e.g., we probably don't want to just skip updates and launch the client, but if we error out, we have to clearly explain why).

This is necessary to suppress the notification during provisioning,
or other processes during which we may not want to encourage the
user to run updates. For now, qubesctl and make are detected.
@eloquence eloquence force-pushed the detect-notify-process-conflict branch from c24b311 to 1ad6fcd Compare February 18, 2020 19:57
Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

Functional testing looks good to me, thanks @eloquence

  • Run /opt/securedrop/launcher/sdw-notify.py. The notification should show up.
  • Run make test and run /opt/securedrop/launcher/sdw-notify.py while it is doing its thing. The notification should not appear, and you should see a log entry in ~/.securedrop_launcher/logs/sdw-notify.log explaining why.
  • (optional, for good measure) Run the Qubes GUI updater and kick off a graphical update process (qubesctl must be running in the process list). Run sdw-notify while that's underway. Result should be the same as above.
  • confirmed was made with love and 100% test coverage

assert running_process is True
mocked_error.assert_called_once()
error_string = mocked_error.call_args[0][0]
assert re.search(CONFLICTING_PROCESS_REGEX, error_string) is not None
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be easier and more reliable to use format strings here given the predictability of the output. Since the regex is used only once, there may be little value in declaring outside of the test function (for now) , for example

diff --git a/launcher/tests/test_util.py b/launcher/tests/test_util.py
index 8a25165ea..44e14c034 100644
--- a/launcher/tests/test_util.py
+++ b/launcher/tests/test_util.py
@@ -13,8 +13,6 @@ BUSY_LOCK_REGEX = r"Error obtaining lock on '.*'."
 # Regex for failure to obtain lock due to permission error
 LOCK_PERMISSION_REGEX = r"Error writing to lock file '.*'"
 
-CONFLICTING_PROCESS_REGEX = r"Conflicting process .* is currently running."
-
 relpath_util = "../sdw_util/Util.py"
 path_to_util = os.path.join(os.path.dirname(os.path.abspath(__file__)), relpath_util)
 util = SourceFileLoader("Util", path_to_util).load_module()
@@ -191,9 +189,7 @@ def test_for_conflicting_process(
         mocked_run.assert_called_once()
         if expected_result is True:
             assert running_process is True
-            mocked_error.assert_called_once()
-            error_string = mocked_error.call_args[0][0]
-            assert re.search(CONFLICTING_PROCESS_REGEX, error_string) is not None
+            mocked_error.assert_called_once_with("Conflicting process '{}' is currently running.".format("cowsay"))
         else:
             assert running_process is False
             assert not mocked_error.called

@conorsch conorsch self-requested a review February 20, 2020 16:19
Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Test plan checks out for me, so +1 to @emkl's positive review. Merging!

@conorsch conorsch merged commit aea536d into master Feb 20, 2020
cfm pushed a commit that referenced this pull request Apr 1, 2024
…flict

Detect process conflicts with sdw-notify
@legoktm legoktm deleted the detect-notify-process-conflict branch May 28, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Don't fire sdw-notify during provisioning

3 participants