From cfd52c80d258f6530efa2389775ed7e5bba0c19b Mon Sep 17 00:00:00 2001 From: jaimergp Date: Wed, 6 Sep 2023 19:06:13 +0200 Subject: [PATCH 1/7] Rework automatic elevation logic --- menuinst/utils.py | 153 ++++++++++++++++++++++++++++++---------------- 1 file changed, 100 insertions(+), 53 deletions(-) diff --git a/menuinst/utils.py b/menuinst/utils.py index e1260c83..cac2236d 100644 --- a/menuinst/utils.py +++ b/menuinst/utils.py @@ -3,10 +3,9 @@ import shlex import subprocess import sys -import traceback import xml.etree.ElementTree as XMLTree from contextlib import suppress -from functools import wraps +from functools import wraps, lru_cache from logging import getLogger from pathlib import Path from typing import Callable, Iterable, Literal, Mapping, Optional, Sequence, Union @@ -246,6 +245,52 @@ def deep_update(mapping: Mapping, *updating_mappings: Iterable[Mapping]) -> Mapp return updated_mapping +def needs_admin(prefix: os.PathLike, base_prefix: os.PathLike) -> bool: + """ + Checks if the current installation needs admin permissions. + """ + if user_is_admin(): + return False + + if Path(prefix, ".nonadmin").exists(): + # This file is planted by the constructor installer + # and signals we don't need admin permissions + return False + + try: + Path(prefix, ".nonadmin").touch() + return False + except Exception as exc: + logger.debug("Attempt to write %s/.nonadmin failed.", prefix, exc_info=exc) + + if base_prefix == prefix: + # We are already in the base env, no need to check further + return True + + # I can't think of cases where users can't write to the target prefix but can to base + # so maybe we can skip everything underneath? + + if Path(base_prefix, ".nonadmin").exists(): + return False + + if os.name == "nt": + # Absence of $base_prefix/.nonadmin in Windows means we need admin permissions + return True + + if os.name == "posix": + # Absence of $base_prefix/.nonadmin in Linux, macOS and other posix systems + # has no meaning for historic reasons, so let's try to see if we can + # write to the installation root + try: + Path(base_prefix, ".nonadmin").touch() + except Exception as exc: + logger.debug("Attempt to write %s/.nonadmin failed.", prefix, exc_info=exc) + return True + else: + return False + + +@lru_cache(maxsize=1) def user_is_admin() -> bool: if os.name == "nt": from .platforms.win_utils.win_elevate import isUserAdmin @@ -316,65 +361,67 @@ def elevate_as_needed(func: Callable) -> Callable: @wraps(func) def wrapper_elevate( *args, + prefix: os.PathLike = None, base_prefix: os.PathLike = None, **kwargs, ): kwargs.pop("_mode", None) - base_prefix = base_prefix or DEFAULT_BASE_PREFIX - if not (Path(base_prefix) / ".nonadmin").exists(): - if user_is_admin(): - return func( - base_prefix=base_prefix, - _mode="system", - *args, - **kwargs, - ) - if os.environ.get("_MENUINST_RECURSING") != "1": - # call the wrapped func with elevated prompt... - # from the command line; not pretty! - try: - if func.__module__ == "__main__": - import_func = ( - f"import runpy;" - f"{func.__name__} = runpy.run_path('{__file__}')" - f"['{func.__name__}'];" - ) - else: - import_func = f"from {func.__module__} import {func.__name__};" - env_vars = ";".join( - [ - f"os.environ.setdefault('{k}', '{v}')" - for (k, v) in os.environ.items() - if k.startswith(("CONDA_", "CONSTRUCTOR_", "MENUINST_")) - ] - ) - cmd = [ - *python_executable(), - "-c", - f"import os;" - f"os.environ.setdefault('_MENUINST_RECURSING', '1');" - f"{env_vars};" - f"{import_func}" - f"{func.__name__}(" - f"*{args!r}," - f"base_prefix={base_prefix!r}," - f"_mode='system'," - f"**{kwargs!r}" - ")", - ] - logger.debug("Elevating command: %s", cmd) - return_code = run_as_admin(cmd) - except Exception: - logger.warn( - "Error occurred! Falling back to user mode. Exception:\n%s", - traceback.format_exc(), + prefix = prefix or DEFAULT_BASE_PREFIX + base_prefix = base_prefix or DEFAULT_BASE_PREFIX + if needs_admin(prefix, base_prefix) and os.environ.get("_MENUINST_RECURSING") != "1": + # call the wrapped func with elevated prompt... + # from the command line; not pretty! + try: + if func.__module__ == "__main__": + import_func = ( + f"import runpy;" + f"{func.__name__} = runpy.run_path('{__file__}')" + f"['{func.__name__}'];" ) else: - os.environ.pop("_MENUINST_RECURSING", None) - if return_code == 0: # success, no need to fallback - return + import_func = f"from {func.__module__} import {func.__name__};" + env_vars = ";".join( + [ + f"os.environ.setdefault('{k}', '{v}')" + for (k, v) in os.environ.items() + if k.startswith(("CONDA_", "CONSTRUCTOR_", "MENUINST_")) + ] + ) + cmd = [ + *python_executable(), + "-c", + f"import os;" + f"os.environ.setdefault('_MENUINST_RECURSING', '1');" + f"{env_vars};" + f"{import_func}" + f"{func.__name__}(" + f"*{args!r}," + f"prefix={prefix!r}," + f"base_prefix={base_prefix!r}," + f"_mode='system'," + f"**{kwargs!r}" + ")", + ] + logger.debug("Elevating command: %s", cmd) + return_code = run_as_admin(cmd) + except Exception as exc: + logger.warn("Elevation failed! Falling back to user mode.", exc_info=exc) + else: + os.environ.pop("_MENUINST_RECURSING", None) + if return_code == 0: # success, we are done + return + elif user_is_admin(): + # We are already running as admin, no need to elevate + return func( + prefix=prefix, + base_prefix=base_prefix, + _mode="system", + *args, + **kwargs, + ) # We have not returned yet? Well, let's try as a normal user return func( + prefix=prefix, base_prefix=base_prefix, _mode="user", *args, From 205580bc93f3c5c8f2fe085f9101e680fe1fb388 Mon Sep 17 00:00:00 2001 From: jaimergp Date: Wed, 6 Sep 2023 19:41:13 +0200 Subject: [PATCH 2/7] prefix -> target_prefix --- menuinst/utils.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/menuinst/utils.py b/menuinst/utils.py index cac2236d..4792c951 100644 --- a/menuinst/utils.py +++ b/menuinst/utils.py @@ -245,29 +245,29 @@ def deep_update(mapping: Mapping, *updating_mappings: Iterable[Mapping]) -> Mapp return updated_mapping -def needs_admin(prefix: os.PathLike, base_prefix: os.PathLike) -> bool: +def needs_admin(target_prefix: os.PathLike, base_prefix: os.PathLike) -> bool: """ Checks if the current installation needs admin permissions. """ if user_is_admin(): return False - if Path(prefix, ".nonadmin").exists(): + if Path(target_prefix, ".nonadmin").exists(): # This file is planted by the constructor installer # and signals we don't need admin permissions return False try: - Path(prefix, ".nonadmin").touch() + Path(target_prefix, ".nonadmin").touch() return False except Exception as exc: - logger.debug("Attempt to write %s/.nonadmin failed.", prefix, exc_info=exc) + logger.debug("Attempt to write %s/.nonadmin failed.", target_prefix, exc_info=exc) - if base_prefix == prefix: + if base_prefix == target_prefix: # We are already in the base env, no need to check further return True - # I can't think of cases where users can't write to the target prefix but can to base + # I can't think of cases where users can't write to target_prefix but can to base # so maybe we can skip everything underneath? if Path(base_prefix, ".nonadmin").exists(): @@ -284,7 +284,7 @@ def needs_admin(prefix: os.PathLike, base_prefix: os.PathLike) -> bool: try: Path(base_prefix, ".nonadmin").touch() except Exception as exc: - logger.debug("Attempt to write %s/.nonadmin failed.", prefix, exc_info=exc) + logger.debug("Attempt to write %s/.nonadmin failed.", target_prefix, exc_info=exc) return True else: return False @@ -361,14 +361,14 @@ def elevate_as_needed(func: Callable) -> Callable: @wraps(func) def wrapper_elevate( *args, - prefix: os.PathLike = None, + target_prefix: os.PathLike = None, base_prefix: os.PathLike = None, **kwargs, ): kwargs.pop("_mode", None) - prefix = prefix or DEFAULT_BASE_PREFIX + target_prefix = target_prefix or DEFAULT_BASE_PREFIX base_prefix = base_prefix or DEFAULT_BASE_PREFIX - if needs_admin(prefix, base_prefix) and os.environ.get("_MENUINST_RECURSING") != "1": + if needs_admin(target_prefix, base_prefix) and os.environ.get("_MENUINST_RECURSING") != "1": # call the wrapped func with elevated prompt... # from the command line; not pretty! try: @@ -396,7 +396,7 @@ def wrapper_elevate( f"{import_func}" f"{func.__name__}(" f"*{args!r}," - f"prefix={prefix!r}," + f"target_prefix={target_prefix!r}," f"base_prefix={base_prefix!r}," f"_mode='system'," f"**{kwargs!r}" @@ -413,7 +413,7 @@ def wrapper_elevate( elif user_is_admin(): # We are already running as admin, no need to elevate return func( - prefix=prefix, + target_prefix=target_prefix, base_prefix=base_prefix, _mode="system", *args, @@ -421,7 +421,7 @@ def wrapper_elevate( ) # We have not returned yet? Well, let's try as a normal user return func( - prefix=prefix, + target_prefix=target_prefix, base_prefix=base_prefix, _mode="user", *args, From f936ecad49a9725f1b214ee9cb49850c209951ad Mon Sep 17 00:00:00 2001 From: jaimergp Date: Thu, 7 Sep 2023 10:32:52 +0200 Subject: [PATCH 3/7] fix func signature --- menuinst/utils.py | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/menuinst/utils.py b/menuinst/utils.py index 4792c951..d942b143 100644 --- a/menuinst/utils.py +++ b/menuinst/utils.py @@ -251,12 +251,12 @@ def needs_admin(target_prefix: os.PathLike, base_prefix: os.PathLike) -> bool: """ if user_is_admin(): return False - + if Path(target_prefix, ".nonadmin").exists(): # This file is planted by the constructor installer # and signals we don't need admin permissions return False - + try: Path(target_prefix, ".nonadmin").touch() return False @@ -276,7 +276,7 @@ def needs_admin(target_prefix: os.PathLike, base_prefix: os.PathLike) -> bool: if os.name == "nt": # Absence of $base_prefix/.nonadmin in Windows means we need admin permissions return True - + if os.name == "posix": # Absence of $base_prefix/.nonadmin in Linux, macOS and other posix systems # has no meaning for historic reasons, so let's try to see if we can @@ -366,9 +366,12 @@ def wrapper_elevate( **kwargs, ): kwargs.pop("_mode", None) - target_prefix = target_prefix or DEFAULT_BASE_PREFIX - base_prefix = base_prefix or DEFAULT_BASE_PREFIX - if needs_admin(target_prefix, base_prefix) and os.environ.get("_MENUINST_RECURSING") != "1": + target_prefix = target_prefix or DEFAULT_BASE_PREFIX + base_prefix = base_prefix or DEFAULT_BASE_PREFIX + if ( + needs_admin(target_prefix, base_prefix) + and os.environ.get("_MENUINST_RECURSING") != "1" + ): # call the wrapped func with elevated prompt... # from the command line; not pretty! try: @@ -431,7 +434,11 @@ def wrapper_elevate( return wrapper_elevate -def _test_elevation(base_prefix: Optional[os.PathLike] = None, _mode: _UserOrSystem = "user"): +def _test_elevation( + target_prefix: Optional[os.PathLike] = None, + base_prefix: Optional[os.PathLike] = None, + _mode: _UserOrSystem = "user", +): if os.name == "nt": if base_prefix: output = os.path.join(base_prefix, "_test_output.txt") From 926c7983c624012b84fdec1077a590d0c1394ddd Mon Sep 17 00:00:00 2001 From: jaimergp Date: Thu, 7 Sep 2023 11:09:10 +0200 Subject: [PATCH 4/7] change permissions to force elevation --- tests/test_elevation.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tests/test_elevation.py b/tests/test_elevation.py index f0e087f6..e12fae49 100644 --- a/tests/test_elevation.py +++ b/tests/test_elevation.py @@ -31,11 +31,16 @@ def test_elevation(tmp_path, capfd): _test_elevation(str(tmp_path)) assert capfd.readouterr().out.strip() == "user_is_admin(): False env_var: TEST _mode: user" - elevate_as_needed(_test_elevation)(base_prefix=str(tmp_path)) + # make tmp_path not writable by the current user to force elevation + tmp_path.chmod(0o500) + elevate_as_needed(_test_elevation)(target_prefix=str(tmp_path), base_prefix=str(tmp_path)) assert ( capfd.readouterr().out.strip() == "user_is_admin(): True env_var: TEST _mode: system" ) + assert not (tmp_path / ".nonadmin").exists() - (tmp_path / ".nonadmin").touch() - elevate_as_needed(_test_elevation)(base_prefix=str(tmp_path)) + # restore permissions + tmp_path.chmod(0o700) + elevate_as_needed(_test_elevation)(target_prefix=str(tmp_path), base_prefix=str(tmp_path)) assert capfd.readouterr().out.strip() == "user_is_admin(): False env_var: TEST _mode: user" + assert (tmp_path / ".nonadmin").exists() From 1951c317392b9c271913155564e207c8a0bfc2f5 Mon Sep 17 00:00:00 2001 From: jaimergp Date: Thu, 7 Sep 2023 11:12:01 +0200 Subject: [PATCH 5/7] pre-commit --- menuinst/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/menuinst/utils.py b/menuinst/utils.py index d942b143..7e5cf8be 100644 --- a/menuinst/utils.py +++ b/menuinst/utils.py @@ -5,7 +5,7 @@ import sys import xml.etree.ElementTree as XMLTree from contextlib import suppress -from functools import wraps, lru_cache +from functools import lru_cache, wraps from logging import getLogger from pathlib import Path from typing import Callable, Iterable, Literal, Mapping, Optional, Sequence, Union From 1c86f9a22969964fb97cddbe44394e16bf40dc95 Mon Sep 17 00:00:00 2001 From: jaimergp Date: Thu, 7 Sep 2023 11:17:37 +0200 Subject: [PATCH 6/7] add news --- news/156-elevation | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 news/156-elevation diff --git a/news/156-elevation b/news/156-elevation new file mode 100644 index 00000000..477e5430 --- /dev/null +++ b/news/156-elevation @@ -0,0 +1,19 @@ +### Enhancements + +* Implement auto-elevation logic in Unix systems so it doesn't depend on pre-existing `.nonadmin` files. (#150 via #156) + +### Bug fixes + +* + +### Deprecations + +* + +### Docs + +* + +### Other + +* From 4ee2de0766f6c34a351c7db0fc7d5d3eb30da680 Mon Sep 17 00:00:00 2001 From: jaimergp Date: Thu, 7 Sep 2023 11:26:21 +0200 Subject: [PATCH 7/7] fiix func signature on windows --- tests/test_elevation.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_elevation.py b/tests/test_elevation.py index e12fae49..dadd442a 100644 --- a/tests/test_elevation.py +++ b/tests/test_elevation.py @@ -12,14 +12,14 @@ def test_elevation(tmp_path, capfd): # Windows runners on GHA always run as admin assert not is_admin - _test_elevation(str(tmp_path)) + _test_elevation(target_prefix=str(tmp_path), base_prefix=str(tmp_path)) output = (tmp_path / "_test_output.txt").read_text().strip() if on_ci: assert output.endswith("env_var: TEST _mode: user") else: assert output.endswith("user_is_admin(): False env_var: TEST _mode: user") - elevate_as_needed(_test_elevation)(base_prefix=str(tmp_path)) + elevate_as_needed(_test_elevation)(target_prefix=str(tmp_path), base_prefix=str(tmp_path)) output = (tmp_path / "_test_output.txt").read_text().strip() if on_ci: assert output.endswith("env_var: TEST _mode: system")