From a3730477561a62b9f3a63fb94c88b625a27c07b1 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 15 Apr 2021 22:27:05 +0200 Subject: [PATCH] More uniform tmp dir for apps, remove some weird 'admin' ownership --- src/yunohost/app.py | 156 +++++++++++++++-------------------------- src/yunohost/backup.py | 36 ++++------ 2 files changed, 72 insertions(+), 120 deletions(-) diff --git a/src/yunohost/app.py b/src/yunohost/app.py index a3c10df0b..3ce73aeaa 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -33,6 +33,7 @@ import re import subprocess import glob import urllib.parse +import tempfile from collections import OrderedDict from moulinette import msignals, m18n, msettings @@ -57,10 +58,8 @@ from yunohost.log import is_unit_operation, OperationLogger logger = getActionLogger("yunohost.app") -APPS_PATH = "/usr/share/yunohost/apps" APPS_SETTING_PATH = "/etc/yunohost/apps/" -INSTALL_TMP = "/var/cache/yunohost" -APP_TMP_FOLDER = INSTALL_TMP + "/from_file" +APP_TMP_WORKDIRS = "/var/cache/yunohost/app_tmp_work_dirs" APPS_CATALOG_CACHE = "/var/cache/yunohost/repo" APPS_CATALOG_CONF = "/etc/yunohost/apps_catalog.yml" @@ -459,34 +458,12 @@ def app_change_url(operation_logger, app, domain, path): operation_logger.extra.update({"env": env_dict}) operation_logger.start() - if os.path.exists(os.path.join(APP_TMP_FOLDER, "scripts")): - shutil.rmtree(os.path.join(APP_TMP_FOLDER, "scripts")) - - shutil.copytree( - os.path.join(APPS_SETTING_PATH, app, "scripts"), - os.path.join(APP_TMP_FOLDER, "scripts"), - ) - - if os.path.exists(os.path.join(APP_TMP_FOLDER, "conf")): - shutil.rmtree(os.path.join(APP_TMP_FOLDER, "conf")) - - shutil.copytree( - os.path.join(APPS_SETTING_PATH, app, "conf"), - os.path.join(APP_TMP_FOLDER, "conf"), - ) + tmp_workdir_for_app = _make_tmp_workdir_for_app(app=app) + change_url_script = os.path.join(tmp_workdir_for_app, "scripts/change_url") # Execute App change_url script - os.system("chown -R admin: %s" % INSTALL_TMP) - os.system("chmod +x %s" % os.path.join(os.path.join(APP_TMP_FOLDER, "scripts"))) - os.system( - "chmod +x %s" - % os.path.join(os.path.join(APP_TMP_FOLDER, "scripts", "change_url")) - ) - - if ( - hook_exec(os.path.join(APP_TMP_FOLDER, "scripts/change_url"), env=env_dict)[0] - != 0 - ): + ret = hook_exec(change_url_script, env=env_dict)[0] + if ret != 0: msg = "Failed to change '%s' url." % app logger.error(msg) operation_logger.error(msg) @@ -496,6 +473,7 @@ def app_change_url(operation_logger, app, domain, path): app_setting(app, "domain", value=old_domain) app_setting(app, "path", value=old_path) return + shutil.rmtree(tmp_workdir_for_app) # this should idealy be done in the change_url script but let's avoid common mistakes app_setting(app, "domain", value=domain) @@ -620,7 +598,7 @@ def app_upgrade(app=[], url=None, file=None, force=False): _check_manifest_requirements(manifest, app_instance_name=app_instance_name) _assert_system_is_sane_for_app(manifest, "pre") - app_setting_path = APPS_SETTING_PATH + "/" + app_instance_name + app_setting_path = os.path.join(APPS_SETTING_PATH, app_instance_name) # Retrieve arguments list for upgrade script # TODO: Allow to specify arguments @@ -646,9 +624,6 @@ def app_upgrade(app=[], url=None, file=None, force=False): operation_logger = OperationLogger("app_upgrade", related_to, env=env_dict) operation_logger.start() - # Execute App upgrade script - os.system("chown -hR admin: %s" % INSTALL_TMP) - # Execute the app upgrade script upgrade_failed = True try: @@ -775,6 +750,12 @@ def app_upgrade(app=[], url=None, file=None, force=False): % (extracted_app_folder, file_to_copy, app_setting_path) ) + # Clean and set permissions + shutil.rmtree(extracted_app_folder) + os.system("chmod 600 %s" % app_setting_path) + os.system("chmod 400 %s/settings.yml" % app_setting_path) + os.system("chown -R root: %s" % app_setting_path) + # So much win logger.success(m18n.n("app_upgraded", app=app_instance_name)) @@ -816,10 +797,6 @@ def app_install( ) from yunohost.regenconf import manually_modified_files - # Fetch or extract sources - if not os.path.exists(INSTALL_TMP): - os.makedirs(INSTALL_TMP) - def confirm_install(confirm): # Ignore if there's nothing for confirm (good quality app), if --force is used # or if request on the API (confirm already implemented on the API side) @@ -953,10 +930,6 @@ def app_install( } _set_app_settings(app_instance_name, app_settings) - os.system("chown -R admin: " + extracted_app_folder) - - # Execute App install script - os.system("chown -hR admin: %s" % INSTALL_TMP) # Move scripts and manifest to the right place if os.path.exists(os.path.join(extracted_app_folder, "manifest.json")): os.system("cp %s/manifest.json %s" % (extracted_app_folder, app_setting_path)) @@ -1131,9 +1104,9 @@ def app_install( # Clean and set permissions shutil.rmtree(extracted_app_folder) - os.system("chmod -R 400 %s" % app_setting_path) + os.system("chmod 600 %s" % app_setting_path) + os.system("chmod 400 %s/settings.yml" % app_setting_path) os.system("chown -R root: %s" % app_setting_path) - os.system("chown -R admin: %s/scripts" % app_setting_path) logger.success(m18n.n("installation_complete")) @@ -1212,13 +1185,7 @@ def app_remove(operation_logger, app): logger.info(m18n.n("app_start_remove", app=app)) - app_setting_path = APPS_SETTING_PATH + app - - # TODO: display fail messages from script - try: - shutil.rmtree("/tmp/yunohost_remove") - except Exception: - pass + app_setting_path = os.path.join(APPS_SETTING_PATH, app) # Attempt to patch legacy helpers ... _patch_legacy_helpers(app_setting_path) @@ -1228,13 +1195,8 @@ def app_remove(operation_logger, app): _patch_legacy_php_versions(app_setting_path) manifest = _get_manifest_of_app(app_setting_path) - - os.system( - "cp -a %s /tmp/yunohost_remove && chown -hR admin: /tmp/yunohost_remove" - % app_setting_path - ) - os.system("chown -R admin: /tmp/yunohost_remove") - os.system("chmod -R u+rX /tmp/yunohost_remove") + tmp_workdir_for_app = _make_tmp_workdir_for_app(app=app) + remove_script = f"{tmp_workdir_for_app}/scripts/remove" env_dict = {} app_id, app_instance_nb = _parse_app_instance_name(app) @@ -1246,7 +1208,7 @@ def app_remove(operation_logger, app): operation_logger.flush() try: - ret = hook_exec("/tmp/yunohost_remove/scripts/remove", env=env_dict)[0] + ret = hook_exec(remove_script, env=env_dict)[0] # Here again, calling hook_exec could fail miserably, or get # manually interrupted (by mistake or because script was stuck) # In that case we still want to proceed with the rest of the @@ -1256,6 +1218,8 @@ def app_remove(operation_logger, app): import traceback logger.error(m18n.n("unexpected_error", error="\n" + traceback.format_exc())) + finally: + shutil.rmtree(tmp_workdir_for_app) if ret == 0: logger.success(m18n.n("app_removed", app=app)) @@ -1269,7 +1233,7 @@ def app_remove(operation_logger, app): if os.path.exists(app_setting_path): shutil.rmtree(app_setting_path) - shutil.rmtree("/tmp/yunohost_remove") + hook_remove(app) permission_sync_to_user() @@ -1717,7 +1681,6 @@ def app_action_run(operation_logger, app, action, args=None): logger.warning(m18n.n("experimental_feature")) from yunohost.hook import hook_exec - import tempfile # will raise if action doesn't exist actions = app_action_list(app)["actions"] @@ -1755,8 +1718,9 @@ def app_action_run(operation_logger, app, action, args=None): if action_declaration.get("cwd"): cwd = action_declaration["cwd"].replace("$app", app) else: - cwd = "/etc/yunohost/apps/" + app + cwd = os.path.join(APPS_SETTING_PATH, app) + # FIXME: this should probably be ran in a tmp workdir... retcode = hook_exec( path, env=env_dict, @@ -1811,6 +1775,7 @@ def app_config_show_panel(operation_logger, app): "YNH_APP_INSTANCE_NUMBER": str(app_instance_nb), } + # FIXME: this should probably be ran in a tmp workdir... return_code, parsed_values = hook_exec( config_script, args=["show"], env=env, return_format="plain_dict" ) @@ -1923,6 +1888,7 @@ def app_config_apply(operation_logger, app, args): "Ignore key '%s' from arguments because it is not in the config", key ) + # FIXME: this should probably be ran in a tmp workdir... return_code = hook_exec( config_script, args=["apply"], @@ -2237,43 +2203,28 @@ def _set_app_settings(app_id, settings): yaml.safe_dump(settings, f, default_flow_style=False) -def _extract_app_from_file(path, remove=False): +def _extract_app_from_file(path): """ - Unzip or untar application tarball in APP_TMP_FOLDER, or copy it from a directory + Unzip / untar / copy application tarball or directory to a tmp work directory Keyword arguments: path -- Path of the tarball or directory - remove -- Remove the tarball after extraction - - Returns: - Dict manifest - """ logger.debug(m18n.n("extracting")) - if os.path.exists(APP_TMP_FOLDER): - shutil.rmtree(APP_TMP_FOLDER) - os.makedirs(APP_TMP_FOLDER) - path = os.path.abspath(path) + extracted_app_folder = _make_tmp_workdir_for_app() + if ".zip" in path: - extract_result = os.system( - "unzip %s -d %s > /dev/null 2>&1" % (path, APP_TMP_FOLDER) - ) - if remove: - os.remove(path) + extract_result = os.system(f"unzip '{path}' -d {extracted_app_folder} > /dev/null 2>&1") elif ".tar" in path: - extract_result = os.system( - "tar -xf %s -C %s > /dev/null 2>&1" % (path, APP_TMP_FOLDER) - ) - if remove: - os.remove(path) + extract_result = os.system(f"tar -xf '{path}' -C {extracted_app_folder} > /dev/null 2>&1") elif os.path.isdir(path): - shutil.rmtree(APP_TMP_FOLDER) + shutil.rmtree(extracted_app_folder) if path[-1] != "/": path = path + "/" - extract_result = os.system('cp -a "%s" %s' % (path, APP_TMP_FOLDER)) + extract_result = os.system(f"cp -a '{path}' {extracted_app_folder}") else: extract_result = 1 @@ -2281,7 +2232,6 @@ def _extract_app_from_file(path, remove=False): raise YunohostError("app_extraction_failed") try: - extracted_app_folder = APP_TMP_FOLDER if len(os.listdir(extracted_app_folder)) == 1: for folder in os.listdir(extracted_app_folder): extracted_app_folder = extracted_app_folder + "/" + folder @@ -2508,24 +2458,11 @@ def _get_git_last_commit_hash(repository, reference="HEAD"): def _fetch_app_from_git(app): """ - Unzip or untar application tarball in APP_TMP_FOLDER + Unzip or untar application tarball to a tmp directory Keyword arguments: app -- App_id or git repo URL - - Returns: - Dict manifest - """ - extracted_app_folder = APP_TMP_FOLDER - - app_tmp_archive = "{0}.zip".format(extracted_app_folder) - if os.path.exists(extracted_app_folder): - shutil.rmtree(extracted_app_folder) - if os.path.exists(app_tmp_archive): - os.remove(app_tmp_archive) - - logger.debug(m18n.n("downloading")) # Extract URL, branch and revision to download if ("@" in app) or ("http://" in app) or ("https://" in app): @@ -2549,6 +2486,10 @@ def _fetch_app_from_git(app): branch = app_info["git"]["branch"] revision = str(app_info["git"]["revision"]) + extracted_app_folder = _make_tmp_workdir_for_app() + + logger.debug(m18n.n("downloading")) + # Download only this commit try: # We don't use git clone because, git clone can't download @@ -3384,6 +3325,23 @@ def _load_apps_catalog(): # ############################### # # +def _make_tmp_workdir_for_app(app=None): + + # Create parent dir if it doesn't exists yet + if not os.path.exists(APP_TMP_WORKDIRS): + os.makedirs(APP_TMP_WORKDIRS) + + # Cleanup old dirs + for dir_ in os.listdir(APP_TMP_WORKDIRS): + shutil.rmtree(os.path.join(APP_TMP_WORKDIRS, dir_)) + tmpdir = tempfile.mkdtemp(prefix="app_", dir=APP_TMP_WORKDIRS) + + # Copy existing app scripts, conf, ... if an app arg was provided + if app: + os.system(f"cp -a {APPS_SETTING_PATH}/{app}/* {tmpdir}") + + return tmpdir + def is_true(arg): """ diff --git a/src/yunohost/backup.py b/src/yunohost/backup.py index a17b752f6..92b668b6e 100644 --- a/src/yunohost/backup.py +++ b/src/yunohost/backup.py @@ -53,6 +53,7 @@ from yunohost.app import ( _patch_legacy_php_versions, _patch_legacy_php_versions_in_settings, LEGACY_PHP_VERSION_REPLACEMENTS, + _make_tmp_workdir_for_app ) from yunohost.hook import ( hook_list, @@ -644,7 +645,7 @@ class BackupManager: restore_hooks_dir = os.path.join(self.work_dir, "hooks", "restore") if not os.path.exists(restore_hooks_dir): - filesystem.mkdir(restore_hooks_dir, mode=0o750, parents=True, uid="admin") + filesystem.mkdir(restore_hooks_dir, mode=0o700, parents=True, uid="root") restore_hooks = hook_list("restore")["hooks"] @@ -705,21 +706,17 @@ class BackupManager: settings_dir = os.path.join(self.work_dir, "apps", app, "settings") logger.info(m18n.n("app_start_backup", app=app)) - tmp_folder = tempfile.mkdtemp() + tmp_workdir_for_app = _make_tmp_workdir_for_app(app=app) try: # Prepare backup directory for the app - filesystem.mkdir(tmp_app_bkp_dir, 0o750, True, uid="admin") + filesystem.mkdir(tmp_app_bkp_dir, 0o700, True, uid="root") # Copy the app settings to be able to call _common.sh shutil.copytree(app_setting_path, settings_dir) - # Copy app backup script in a temporary folder and execute it - app_script = os.path.join(app_setting_path, "scripts/backup") - tmp_script = os.path.join(tmp_folder, "backup") - subprocess.call(["install", "-Dm555", app_script, tmp_script]) - hook_exec( - tmp_script, raise_on_error=True, chdir=tmp_app_bkp_dir, env=env_dict + f"{tmp_workdir_for_app}/scripts/backup", + raise_on_error=True, chdir=tmp_app_bkp_dir, env=env_dict )[0] self._import_to_list_to_backup(env_dict["YNH_BACKUP_CSV"]) @@ -750,8 +747,7 @@ class BackupManager: # Remove tmp files in all situations finally: - if tmp_folder and os.path.exists(tmp_folder): - shutil.rmtree(tmp_folder) + shutil.rmtree(tmp_workdir_for_app) filesystem.rm(env_dict["YNH_BACKUP_CSV"], force=True) # @@ -1402,13 +1398,11 @@ class RestoreManager: filesystem.chown(app_scripts_new_path, "root", None, True) # Copy the app scripts to a writable temporary folder - # FIXME : use 'install -Dm555' or something similar to what's done - # in the backup method ? - tmp_folder_for_app_restore = tempfile.mkdtemp(prefix="restore") - copytree(app_scripts_in_archive, tmp_folder_for_app_restore) - filesystem.chmod(tmp_folder_for_app_restore, 0o550, 0o550, True) - filesystem.chown(tmp_folder_for_app_restore, "root", None, True) - restore_script = os.path.join(tmp_folder_for_app_restore, "restore") + tmp_workdir_for_app = _make_tmp_workdir_for_app() + copytree(app_scripts_in_archive, tmp_workdir_for_app) + filesystem.chmod(tmp_workdir_for_app, 0o700, 0o700, True) + filesystem.chown(tmp_workdir_for_app, "root", None, True) + restore_script = os.path.join(tmp_workdir_for_app, "restore") # Restore permissions if not os.path.isfile("%s/permissions.yml" % app_settings_new_path): @@ -1463,7 +1457,7 @@ class RestoreManager: # Cleanup shutil.rmtree(app_settings_new_path, ignore_errors=True) - shutil.rmtree(tmp_folder_for_app_restore, ignore_errors=True) + shutil.rmtree(tmp_workdir_for_app, ignore_errors=True) return @@ -1513,7 +1507,7 @@ class RestoreManager: failure_message_with_debug_instructions = operation_logger.error(error) finally: # Cleaning temporary scripts directory - shutil.rmtree(tmp_folder_for_app_restore, ignore_errors=True) + shutil.rmtree(tmp_workdir_for_app, ignore_errors=True) if not restore_failed: self.targets.set_result("apps", app_instance_name, "Success") @@ -1869,7 +1863,7 @@ class CopyBackupMethod(BackupMethod): dest_parent = os.path.dirname(dest) if not os.path.exists(dest_parent): - filesystem.mkdir(dest_parent, 0o750, True, uid="admin") + filesystem.mkdir(dest_parent, 0o700, True, uid="admin") if os.path.isdir(source): shutil.copytree(source, dest)