Merge ~sylvain-pineau/checkbox-ng:fix-1849872 into checkbox-ng:master

[Sylvain Pineau]

Fixes linked bug by setting the DISPLAY env var when the remote slave service running as root runs a job as a normal user.

[Sylvain Pineau (sylvain-pineau)]

Tested on 2 remote systems (16.04 unity and 18.04 gnome )

[Sylvain Pineau (sylvain-pineau)]

Improved version where the DISPLAY env is evaluated only when a session starts or resumes itself.
The extra_env is carried up to the unified runner to be part of the differential env when the service running as root switch to the normal user.

review: Needs Resubmitting

[Maciej Kisielewski (kissiel)]

One potential improvement suggested below

[Jonathan Cave (jocave)]

Thanks for reducing the number of times it is likely these loops will need to be run. I think it would have been an unfair overhead on server and IoT runs to have done this for every job. So +1 for the compromise.

review: Approve

[Sylvain Pineau (sylvain-pineau)]

outlining the psutil stuff as requested

review: Needs Resubmitting

[Sylvain Pineau (sylvain-pineau)]

approving based on initial reviews. the current commit includes a dedicated method to prepare the DISPLAY env var

review: Approve

diff --git a/plainbox/impl/execution.py b/plainbox/impl/execution.py
index 410df68..38c1221 100644
--- a/plainbox/impl/execution.py
+++ b/plainbox/impl/execution.py
@@ -62,7 +62,8 @@ class UnifiedRunner(IJobRunner):
                  command_io_delegate=None, dry_run=False,
                  execution_ctrl_list=None, stdin=False,
                  normal_user_provider=lambda: None,
-                 password_provider=sudo_password_provider.get_sudo_password):
+                 password_provider=sudo_password_provider.get_sudo_password,
+                 extra_env=None):
         self._session_dir = session_dir
         self._session_dir = session_dir
         self._provider_list = provider_list
@@ -78,6 +79,7 @@ class UnifiedRunner(IJobRunner):
         self._password_provider = password_provider
         self._stdin = stdin
         self._running_jobs_pid = None
+        self._extra_env = extra_env
 
     def run_job(self, job, job_state, config=None, ui=None):
         logger.info(_("Running %r"), job)
@@ -269,7 +271,8 @@ class UnifiedRunner(IJobRunner):
             # Get the command and the environment.
             # of this execution controller
             cmd = get_execution_command(
-                job, config, self._session_dir, nest_dir, target_user)
+                job, config, self._session_dir, nest_dir, target_user,
+                self._extra_env)
             env = get_execution_environment(
                 job, config, self._session_dir, nest_dir)
             # run the command
@@ -500,7 +503,8 @@ def get_execution_environment(job, config, session_dir, nest_dir):
     return env
 
 
-def get_differential_execution_environment(job, config, session_dir, nest_dir):
+def get_differential_execution_environment(job, config, session_dir, nest_dir,
+                                           extra_env=None):
     """
     Get the environment required to execute the specified job:
 
@@ -542,6 +546,8 @@ def get_differential_execution_environment(job, config, session_dir, nest_dir):
         delta_env['LANG'] = 'C.UTF-8'
         delta_env['LANGUAGE'] = ''
         delta_env['LC_ALL'] = 'C.UTF-8'
+    if extra_env:
+        delta_env.update(extra_env)
     # Preserve the copy_vars variables + those prefixed with SNAP on Snappy
     if (os.getenv("SNAP") or os.getenv("SNAP_APP_PATH")):
         copy_vars = ['PYTHONHOME', 'PYTHONUSERBASE', 'LD_LIBRARY_PATH',
@@ -553,7 +559,7 @@ def get_differential_execution_environment(job, config, session_dir, nest_dir):
 
 
 def get_execution_command(job, config, session_dir,
-                          nest_dir, target_user=None):
+                          nest_dir, target_user=None, extra_env=None):
     """Generate a command argv to run in the shell."""
     cmd = []
     if target_user:
@@ -568,7 +574,7 @@ def get_execution_command(job, config, session_dir,
     cmd += ['env']
     if target_user:
         env = get_differential_execution_environment(
-            job, config, session_dir, nest_dir)
+            job, config, session_dir, nest_dir, extra_env)
     else:
         env = get_execution_environment(job, config, session_dir, nest_dir)
     cmd += ["{key}={value}".format(key=key, value=value)
diff --git a/plainbox/impl/session/remote_assistant.py b/plainbox/impl/session/remote_assistant.py
index c016697..35a5b11 100644
--- a/plainbox/impl/session/remote_assistant.py
+++ b/plainbox/impl/session/remote_assistant.py
@@ -41,6 +41,8 @@ from plainbox.abc import IJobResult
 from checkbox_ng.config import load_configs
 from checkbox_ng.launcher.run import SilentUI
 
+import psutil
+
 _ = gettext.gettext
 
 _logger = logging.getLogger("plainbox.session.remote_assistant")
@@ -184,6 +186,16 @@ class RemoteSessionAssistant():
         self._last_response = response
         self._state = Running
 
+    def prepare_extra_env(self):
+        # If possible also set the DISPLAY env var
+        # i.e when a user desktop session is running
+        for p in psutil.pids():
+            p_environ = psutil.Process(p).environ()
+            p_user = psutil.Process(p).username()
+            if ("DISPLAY" in p_environ and p_user != 'gdm'):  # gdm uses :1024
+                return {'DISPLAY': p_environ['DISPLAY']}
+                break
+
     @allowed_when(Idle)
     def start_session(self, configuration):
         self._reset_sa()
@@ -207,6 +219,7 @@ class RemoteSessionAssistant():
             'normal_user_provider': lambda: self._normal_user,
             'password_provider': pass_provider,
             'stdin': self._pipe_to_subproc,
+            'extra_env': self.prepare_extra_env(),
         }
         self._sa.start_new_session(session_title, UnifiedRunner, runner_kwargs)
         self._sa.update_app_blob(json.dumps(
@@ -514,6 +527,7 @@ class RemoteSessionAssistant():
             'normal_user_provider': lambda: self._normal_user,
             'password_provider': pass_provider,
             'stdin': self._pipe_to_subproc,
+            'extra_env': self.prepare_extra_env(),
         }
         meta = self._sa.resume_session(session_id, runner_kwargs=runner_kwargs)
         app_blob = json.loads(meta.app_blob.decode("UTF-8"))