From 0bda4843a7b8a3e227b5e4bdf4078a2d9561c5de Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Sat, 3 Sep 2022 16:15:57 -0700 Subject: [PATCH] *: 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 Reviewed-by: James Valleroy --- plinth/modules/power/views.py | 9 +- plinth/modules/upgrades/views.py | 7 +- plinth/package.py | 91 +++-------- plinth/privileged/__init__.py | 7 +- .../packages => plinth/privileged/packages.py | 149 ++++++------------ plinth/setup.py | 3 +- 6 files changed, 91 insertions(+), 175 deletions(-) rename actions/packages => plinth/privileged/packages.py (74%) mode change 100755 => 100644 diff --git a/plinth/modules/power/views.py b/plinth/modules/power/views.py index 2a53b8a95..c4198f09e 100644 --- a/plinth/modules/power/views.py +++ b/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 }) diff --git a/plinth/modules/upgrades/views.py b/plinth/modules/upgrades/views.py index 98700f3de..0736ff000 100644 --- a/plinth/modules/upgrades/views.py +++ b/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 diff --git a/plinth/package.py b/plinth/package.py index fe03981e7..c4e2a80dc 100644 --- a/plinth/package.py +++ b/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']) - extra_arguments = [] - if skip_recommends: - extra_arguments.append('--skip-recommends') - - if force_configuration is not None: - extra_arguments.append( - '--force-configuration={}'.format(force_configuration)) - - if reinstall: - extra_arguments.append('--reinstall') - - 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: + privileged.update() + kwargs = { + 'app_id': self.app_id, + 'packages': self.package_names, + 'skip_recommends': skip_recommends, + 'force_configuration': force_configuration, + 'reinstall': reinstall, + 'force_missing_configuration': force_missing_configuration + } + privileged.install(**kwargs) + except Exception 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'] + - self.package_names) - except subprocess.CalledProcessError as exception: + privileged.remove(app_id=self.app_id, packages=self.package_names) + except Exception 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']) - except subprocess.CalledProcessError as exception: + privileged.update() + 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'], - log_error=False) - return True - except actions.ActionError: + return privileged.is_package_manager_busy(_log_error=False) + except Exception: 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( - 'packages', - ['filter-conffile-packages', '--packages'] + list(packages)) - return json.loads(response) + return privileged.filter_conffile_packages(list(packages)) def packages_installed(candidates: Union[list, tuple]) -> list: diff --git a/plinth/privileged/__init__.py b/plinth/privileged/__init__.py index 82d80ca2b..14ade672b 100644 --- a/plinth/privileged/__init__.py +++ b/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', - 'restart', 'start', 'stop', 'try_restart', 'unmask' + 'filter_conffile_packages', 'install', 'is_package_manager_busy', 'remove', + 'update', 'disable', 'enable', 'is_enabled', 'is_running', 'mask', + 'reload', 'restart', 'start', 'stop', 'try_restart', 'unmask' ] diff --git a/actions/packages b/plinth/privileged/packages.py old mode 100755 new mode 100644 similarity index 74% rename from actions/packages rename to plinth/privileged/packages.py index 8ac12a27b..76f63a764 --- a/actions/packages +++ b/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, - run_apt_command) -from plinth.package import Packages +from plinth.action_utils import run_apt_command +from plinth.actions import privileged logger = logging.getLogger(__name__) -def parse_arguments(): - """Return parsed command line arguments as dictionary.""" - 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): +@privileged +def update(): """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) - except Exception as exception: - print('Access check failed:', exception, file=sys.stderr) - sys.exit(99) + _assert_managed_packages(app_id, packages) + except Exception: + raise PermissionError(f'Packages are not managed: {packages}') 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 + - arguments.packages) + returncode = run_apt_command(['install'] + extra_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) - except Exception as exception: - print('Access check failed:', exception, file=sys.stderr) - sys.exit(99) + _assert_managed_packages(app_id, packages) + except Exception: + raise PermissionError(f'Packages are not managed: {packages}') 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(): - sys.exit(-1) + return action_utils.is_package_manager_busy() -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() diff --git a/plinth/setup.py b/plinth/setup.py index 0a9449377..dddbe40bf 100644 --- a/plinth/setup.py +++ b/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()