*: Use privileged decorator for package actions

Tests:

- DONE: Check if package manager is busy works
  - DONE: Power app shows status in app/restart/shutdown pages
  - DONE: Upgrades app shows in app page and first boot wizard page
  - DONE: When attempting force upgrade, busy state results in a back-off
- DONE: An app's packages can be installed/uninstalled successfully
  - DONE: apt update is run before install
  - DONE: If network is not available during package install, error message is shown
- DONE: Filtering packages with configuration file prompts works. Tested with
  firewall 1.0.3 to 1.2.1.

Signed-off-by: Sunil Mohan Adapa <sunil@medhas.org>
Reviewed-by: James Valleroy <jvalleroy@mailbox.org>

plinth/modules/power/views.py

@@ -22,7 +22,8 @@ class PowerAppView(AppView):
     def get_context_data(self, *args, **kwargs):
         """Add additional context data for template."""
         context = super().get_context_data(*args, **kwargs)
-        context['pkg_manager_is_busy'] = package.is_package_manager_busy()
+        is_busy = package.is_package_manager_busy()
+        context['pkg_manager_is_busy'] = is_busy
         return context
 
 
@@ -36,12 +37,13 @@ def restart(request):
 
     app = app_module.App.get('power')
     form = Form(prefix='power')
+    is_busy = package.is_package_manager_busy()
     return TemplateResponse(
         request, 'power_restart.html', {
             'title': app.info.name,
             'form': form,
             'manual_page': app.info.manual_page,
-            'pkg_manager_is_busy': package.is_package_manager_busy()
+            'pkg_manager_is_busy': is_busy
         })
 
 
@@ -55,10 +57,11 @@ def shutdown(request):
 
     app = app_module.App.get('power')
     form = Form(prefix='power')
+    is_busy = package.is_package_manager_busy()
     return TemplateResponse(
         request, 'power_shutdown.html', {
             'title': app.info.name,
             'form': form,
             'manual_page': app.info.manual_page,
-            'pkg_manager_is_busy': package.is_package_manager_busy()
+            'pkg_manager_is_busy': is_busy
         })

plinth/modules/upgrades/views.py

@@ -12,8 +12,9 @@ from django.utils.translation import gettext as _
 from django.views.generic import TemplateView
 from django.views.generic.edit import FormView
 
-from plinth import __version__, package
+from plinth import __version__
 from plinth.modules import first_boot, upgrades
+from plinth.privileged import packages as packages_privileged
 from plinth.views import AppView
 
 from . import privileged
@@ -41,7 +42,7 @@ class UpgradesConfigurationView(AppView):
         context['can_activate_backports'] = upgrades.can_activate_backports()
         context['is_backports_requested'] = upgrades.is_backports_requested()
         context['is_busy'] = (_is_updating()
-                              or package.is_package_manager_busy())
+                              or packages_privileged.is_package_manager_busy())
         context['log'] = privileged.get_log()
         context['refresh_page_sec'] = 3 if context['is_busy'] else None
         context['version'] = __version__
@@ -210,7 +211,7 @@ class UpdateFirstbootProgressView(TemplateView):
     def get_context_data(self, *args, **kwargs):
         context = super().get_context_data(*args, **kwargs)
         context['is_busy'] = (_is_updating()
-                              or package.is_package_manager_busy())
+                              or packages_privileged.is_package_manager_busy())
         context['next_step'] = first_boot.next_step()
         context['refresh_page_sec'] = 3 if context['is_busy'] else None
         return context

plinth/package.py

@@ -1,14 +1,9 @@
 # SPDX-License-Identifier: AGPL-3.0-or-later
-"""
+"""Framework for installing and updating distribution packages."""
-Framework for installing and updating distribution packages
-"""
 
 import enum
-import json
 import logging
 import pathlib
-import subprocess
-import threading
 import time
 from typing import Optional, Union
 
@@ -16,7 +11,8 @@ import apt.cache
 from django.utils.translation import gettext as _
 from django.utils.translation import gettext_lazy
 
-from plinth import actions, app
+import plinth.privileged.packages as privileged
+from plinth import app
 from plinth.errors import MissingPackageError
 from plinth.utils import format_lazy
 
@@ -107,6 +103,7 @@ class Packages(app.FollowerComponent):
 
     class ConflictsAction(enum.Enum):
         """Action to take when a conflicting package is installed."""
+
         IGNORE = 'ignore'  # Proceed as if there are no conflicts
         REMOVE = 'remove'  # Remove the packages before installing the app
 
@@ -307,74 +304,36 @@ class Transaction:
 
         """
         try:
-            self._run_apt_command(['update'])
+            privileged.update()
-            extra_arguments = []
+            kwargs = {
-            if skip_recommends:
+                'app_id': self.app_id,
-                extra_arguments.append('--skip-recommends')
+                'packages': self.package_names,
-
+                'skip_recommends': skip_recommends,
-            if force_configuration is not None:
+                'force_configuration': force_configuration,
-                extra_arguments.append(
+                'reinstall': reinstall,
-                    '--force-configuration={}'.format(force_configuration))
+                'force_missing_configuration': force_missing_configuration
-
+            }
-            if reinstall:
+            privileged.install(**kwargs)
-                extra_arguments.append('--reinstall')
+        except Exception as exception:
-
-            if force_missing_configuration:
-                extra_arguments.append('--force-missing-configuration')
-
-            self._run_apt_command(['install'] + extra_arguments +
-                                  [self.app_id] + self.package_names)
-        except subprocess.CalledProcessError as exception:
             logger.exception('Error installing package: %s', exception)
             raise
 
     def uninstall(self):
         """Run an apt-get transaction to uninstall given packages."""
         try:
-            self._run_apt_command(['remove', self.app_id, '--packages'] +
+            privileged.remove(app_id=self.app_id, packages=self.package_names)
-                                  self.package_names)
+        except Exception as exception:
-        except subprocess.CalledProcessError as exception:
             logger.exception('Error uninstalling package: %s', exception)
             raise
 
     def refresh_package_lists(self):
         """Refresh apt package lists."""
         try:
-            self._run_apt_command(['update'])
+            privileged.update()
-        except subprocess.CalledProcessError as exception:
+        except Exception as exception:
             logger.exception('Error updating package lists: %s', exception)
             raise
 
-    def _run_apt_command(self, arguments):
-        """Run apt-get and update progress."""
-        self._reset_status()
-
-        process = actions.superuser_run('packages', arguments,
-                                        run_in_background=True)
-        process.stdin.close()
-
-        stdout_thread = threading.Thread(target=self._read_stdout,
-                                         args=(process, ))
-        stderr_thread = threading.Thread(target=self._read_stderr,
-                                         args=(process, ))
-        stdout_thread.start()
-        stderr_thread.start()
-        stdout_thread.join()
-        stderr_thread.join()
-
-        return_code = process.wait()
-        if return_code != 0:
-            raise PackageException(_('Error running apt-get'), self.stderr)
-
-    def _read_stdout(self, process):
-        """Read the stdout of the process and update progress."""
-        for line in process.stdout:
-            self._parse_progress(line.decode())
-
-    def _read_stderr(self, process):
-        """Read the stderr of the process and store in buffer."""
-        self.stderr = process.stderr.read().decode()
-
     def _parse_progress(self, line):
         """Parse the apt-get process output line.
 
@@ -461,10 +420,8 @@ def uninstall(package_names):
 def is_package_manager_busy():
     """Return whether a package manager is running."""
     try:
-        actions.superuser_run('packages', ['is-package-manager-busy'],
+        return privileged.is_package_manager_busy(_log_error=False)
-                              log_error=False)
+    except Exception:
-        return True
-    except actions.ActionError:
         return False
 
 
@@ -479,12 +436,8 @@ def filter_conffile_prompt_packages(packages):
 
     Information for each package includes: current_version, new_version and
     list of modified_conffiles.
-
     """
-    response = actions.superuser_run(
+    return privileged.filter_conffile_packages(list(packages))
-        'packages',
-        ['filter-conffile-packages', '--packages'] + list(packages))
-    return json.loads(response)
 
 
 def packages_installed(candidates: Union[list, tuple]) -> list:

plinth/privileged/__init__.py

@@ -1,10 +1,13 @@
 # SPDX-License-Identifier: AGPL-3.0-or-later
 """Package holding all the privileged actions outside of apps."""
 
+from .packages import (filter_conffile_packages, install,
+                       is_package_manager_busy, remove, update)
 from .service import (disable, enable, is_enabled, is_running, mask, reload,
                       restart, start, stop, try_restart, unmask)
 
 __all__ = [
-    'disable', 'enable', 'is_enabled', 'is_running', 'mask', 'reload',
+    'filter_conffile_packages', 'install', 'is_package_manager_busy', 'remove',
-    'restart', 'start', 'stop', 'try_restart', 'unmask'
+    'update', 'disable', 'enable', 'is_enabled', 'is_running', 'mask',
+    'reload', 'restart', 'start', 'stop', 'try_restart', 'unmask'
 ]

plinth/privileged/packages.py

@@ -1,131 +1,97 @@
-#!/usr/bin/python3
 # SPDX-License-Identifier: AGPL-3.0-or-later
-"""
+"""Wrapper to handle package installation with apt-get."""
-Wrapper to handle package installation with apt-get.
-"""
 
-import argparse
-import json
 import logging
 import os
 import subprocess
-import sys
 from collections import defaultdict
+from typing import Any, Optional
 
 import apt.cache
 import apt_inst
 import apt_pkg
 
+from plinth import action_utils
 from plinth import app as app_module
 from plinth import module_loader
-from plinth.action_utils import (apt_hold_freedombox, is_package_manager_busy,
+from plinth.action_utils import run_apt_command
-                                 run_apt_command)
+from plinth.actions import privileged
-from plinth.package import Packages
 
 logger = logging.getLogger(__name__)
 
 
-def parse_arguments():
+@privileged
-    """Return parsed command line arguments as dictionary."""
+def update():
-    parser = argparse.ArgumentParser()
-    subparsers = parser.add_subparsers(dest='subcommand', help='Sub command')
-
-    subparsers.add_parser('update', help='update the package lists')
-
-    subparser = subparsers.add_parser('install', help='install packages')
-    subparser.add_argument(
-        '--skip-recommends', action='store_true',
-        help='whether to skip installing recommended packages')
-    subparser.add_argument(
-        '--force-configuration', choices=['new', 'old'],
-        help='force old/new configuration files during install')
-    subparser.add_argument(
-        '--reinstall', action='store_true',
-        help='force re-installation of package even if it is current')
-    subparser.add_argument(
-        '--force-missing-configuration', action='store_true',
-        help='force installation of missing configuration files')
-    subparser.add_argument(
-        'app_id', help='ID of app for which package is being installed')
-    subparser.add_argument('packages', nargs='+',
-                           help='list of packages to install')
-
-    subparser = subparsers.add_parser('remove', help='remove the package(s)')
-    subparser.add_argument(
-        'app_id', help='ID of app for which package is being uninstalled')
-    subparser.add_argument('--packages', required=True,
-                           help='List of packages to remove', nargs='+')
-
-    subparsers.add_parser('is-package-manager-busy',
-                          help='Return whether package manager is busy')
-    subparser = subparsers.add_parser(
-        'filter-conffile-packages',
-        help='Filter out packages that do not have pending conffile prompts')
-    subparser.add_argument('--packages', required=True,
-                           help='List of packages to filter', nargs='+')
-
-    subparsers.required = True
-    return parser.parse_args()
-
-
-def subcommand_update(arguments):
     """Update apt package lists."""
-    sys.exit(run_apt_command(['update']))
+    returncode = run_apt_command(['update'])
+    if returncode:
+        raise RuntimeError(
+            f'Apt command failed with return code: {returncode}')
 
 
-def subcommand_install(arguments):
+@privileged
+def install(app_id: str, packages: list[str], skip_recommends: bool = False,
+            force_configuration: Optional[str] = None, reinstall: bool = False,
+            force_missing_configuration: bool = False):
     """Install packages using apt-get."""
+    if force_configuration not in ('old', 'new', None):
+        raise ValueError('Invalid value for force_configuration')
+
     try:
-        _assert_managed_packages(arguments.app_id, arguments.packages)
+        _assert_managed_packages(app_id, packages)
-    except Exception as exception:
+    except Exception:
-        print('Access check failed:', exception, file=sys.stderr)
+        raise PermissionError(f'Packages are not managed: {packages}')
-        sys.exit(99)
 
     extra_arguments = []
-    if arguments.skip_recommends:
+    if skip_recommends:
         extra_arguments.append('--no-install-recommends')
 
-    if arguments.force_configuration == 'old':
+    if force_configuration == 'old':
         extra_arguments += [
             '-o', 'Dpkg::Options::=--force-confdef', '-o',
             'Dpkg::Options::=--force-confold'
         ]
-    elif arguments.force_configuration == 'new':
+    elif force_configuration == 'new':
         extra_arguments += ['-o', 'Dpkg::Options::=--force-confnew']
 
-    if arguments.reinstall:
+    if reinstall:
         extra_arguments.append('--reinstall')
 
-    if arguments.force_missing_configuration:
+    if force_missing_configuration:
         extra_arguments += ['-o', 'Dpkg::Options::=--force-confmiss']
 
     subprocess.run(['dpkg', '--configure', '-a'], check=False)
-    with apt_hold_freedombox():
+    with action_utils.apt_hold_freedombox():
         run_apt_command(['--fix-broken', 'install'])
-        returncode = run_apt_command(['install'] + extra_arguments +
+        returncode = run_apt_command(['install'] + extra_arguments + packages)
-                                     arguments.packages)
 
-    sys.exit(returncode)
+    if returncode:
+        raise RuntimeError(
+            f'Apt command failed with return code: {returncode}')
 
 
-def subcommand_remove(arguments):
+@privileged
+def remove(app_id: str, packages: list[str]):
     """Remove packages using apt-get."""
     try:
-        _assert_managed_packages(arguments.app_id, arguments.packages)
+        _assert_managed_packages(app_id, packages)
-    except Exception as exception:
+    except Exception:
-        print('Access check failed:', exception, file=sys.stderr)
+        raise PermissionError(f'Packages are not managed: {packages}')
-        sys.exit(99)
 
     subprocess.run(['dpkg', '--configure', '-a'], check=False)
-    with apt_hold_freedombox():
+    with action_utils.apt_hold_freedombox():
         run_apt_command(['--fix-broken', 'install'])
-        returncode = run_apt_command(['remove'] + arguments.packages)
+        returncode = run_apt_command(['remove'] + packages)
 
-    sys.exit(returncode)
+    if returncode:
+        raise RuntimeError(
+            f'Apt command failed with return code: {returncode}')
 
 
 def _assert_managed_packages(app_id, packages):
     """Check that list of packages are in fact managed by module."""
+    from plinth.package import Packages
+
     module_loader.load_modules()
     app_module.apps_init()
     app = app_module.App.get(app_id)
@@ -137,16 +103,18 @@ def _assert_managed_packages(app_id, packages):
         assert package in managed_packages
 
 
-def subcommand_is_package_manager_busy(_):
+@privileged
+def is_package_manager_busy() -> bool:
     """Check whether package manager is busy.
 
     An exit code of zero indicates that package manager is busy.
     """
-    if not is_package_manager_busy():
+    return action_utils.is_package_manager_busy()
-        sys.exit(-1)
 
 
-def subcommand_filter_conffile_packages(arguments):
+@privileged
+def filter_conffile_packages(
+        packages_list: list[str]) -> dict[str, dict[str, Any]]:
     """Return filtered list of packages which have pending conffile prompts.
 
     When considering which file needs a configuration file prompt, mimic the
@@ -177,7 +145,7 @@ def subcommand_filter_conffile_packages(arguments):
 
     """
     apt_pkg.init()  # Read configuration that will be used later.
-    packages = set(arguments.packages)
+    packages = set(packages_list)
 
     status_hashes, current_versions = _get_conffile_hashes_from_status_file(
         packages)
@@ -205,7 +173,7 @@ def subcommand_filter_conffile_packages(arguments):
         }
         packages_info[package] = package_info
 
-    print(json.dumps(packages_info))
+    return packages_info
 
 
 def _get_modified_conffiles(status_hashes, mismatched_hashes,
@@ -333,7 +301,7 @@ def _download_packages(packages):
     run_result = fetcher.run()
     if run_result != apt_pkg.Acquire.RESULT_CONTINUE:
         logger.error('Downloading packages failed.')
-        sys.exit(1)
+        raise RuntimeError('Downloading packages failed.')
 
     downloaded_files = []
     for item in fetcher.items:
@@ -409,16 +377,3 @@ def _get_conffile_hashes_from_downloaded_file(packages, downloaded_file,
         hashes[conffile] = md5sum
 
     return package_name, hashes, new_version
-
-
-def main():
-    """Parse arguments and perform all duties."""
-    arguments = parse_arguments()
-
-    subcommand = arguments.subcommand.replace('-', '_')
-    subcommand_method = globals()['subcommand_' + subcommand]
-    subcommand_method(arguments)
-
-
-if __name__ == '__main__':
-    main()

plinth/setup.py

@@ -18,6 +18,7 @@ from plinth.signals import post_setup
 
 from . import operation as operation_module
 from . import package
+from .privileged import packages as packages_privileged
 
 logger = logging.getLogger(__name__)
 
@@ -418,7 +419,7 @@ class ForceUpgrader():
         if _is_shutting_down:
             raise self.PermanentFailure('Service is shutting down')
 
-        if package.is_package_manager_busy():
+        if packages_privileged.is_package_manager_busy():
             raise self.TemporaryFailure('Package manager is busy')
 
         apps = self._get_list_of_apps_to_force_upgrade()