Refactor @needs_root to re-run only the decorated function in a subprocess#317
Refactor @needs_root to re-run only the decorated function in a subprocess#317ezio-melotti wants to merge 2 commits into
@needs_root to re-run only the decorated function in a subprocess#317Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the @needs_root decorator in simoc-sam.py so that, when root privileges are required, it re-invokes only the decorated command via sudo in a subprocess rather than execvp-replacing the whole process (avoiding re-running the caller command and accidentally running subsequent code as root).
Changes:
- Added
inspectusage to reconstruct the decorated function’s arguments for re-invocation. - Replaced
os.execvp(...)withsubprocess.run(['sudo', ..., sys.executable, __file__, cmd_name, *cmd_args]). - Returned success based on the subprocess exit code.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bound.apply_defaults() | ||
| # re-run with sudo in a subprocess | ||
| cmd_name = func.__name__ | ||
| cmd_args = [str(arg) for arg in bound.arguments.values()] |
There was a problem hiding this comment.
bound.apply_defaults() causes default values to be forwarded into the sudo subprocess. For parameters where None means “not provided” (e.g. setup_wifi(ssid=None, password=None, ...)), this turns into the literal string "None" on the command line, changing behavior (the is None checks won’t trigger) and can lead to incorrect configuration. Consider not applying defaults and only forwarding explicitly provided arguments, or omit any defaulted arguments (especially those defaulting to None) when building cmd_args.
| bound.apply_defaults() | |
| # re-run with sudo in a subprocess | |
| cmd_name = func.__name__ | |
| cmd_args = [str(arg) for arg in bound.arguments.values()] | |
| # re-run with sudo in a subprocess; only forward explicitly provided, | |
| # non-None arguments to avoid turning default None into "None" | |
| cmd_name = func.__name__ | |
| cmd_args = [str(arg) for arg in bound.arguments.values() if arg is not None] |
| sig = inspect.signature(func) | ||
| bound = sig.bind(*args, **kwargs) | ||
| bound.apply_defaults() | ||
| # re-run with sudo in a subprocess | ||
| cmd_name = func.__name__ | ||
| cmd_args = [str(arg) for arg in bound.arguments.values()] | ||
| cmd = ['sudo', '--preserve-env=HOME', |
There was a problem hiding this comment.
The sudo re-run builds CLI args using only bound.arguments.values(), which loses keyword information and can mis-map arguments if the decorated function is called with keyword arguments (e.g. setup_hotspot(password='x') would pass 'x' as the first positional arg). Either reject kwargs in the non-root path with a clear error, or extend the CLI to accept --param value style flags and preserve names when re-invoking.
| cmd_name = func.__name__ | ||
| cmd_args = [str(arg) for arg in bound.arguments.values()] | ||
| cmd = ['sudo', '--preserve-env=HOME', | ||
| sys.executable, __file__, cmd_name, *cmd_args] | ||
| result = subprocess.run(cmd, cwd=SIMOC_SAM_DIR) |
There was a problem hiding this comment.
This implementation assumes the decorated function is invokable via the script’s command dispatcher (COMMANDS[func.__name__]). If @needs_root is applied to a helper (or a command name differs from __name__), the sudo subprocess will fail at runtime. Consider asserting func.__name__ in COMMANDS (and possibly that COMMANDS[name] is func) and raising a helpful error message if not.
The
@needs_rootdecorator was initially designed with individual commands in mind: if the user ranpython simoc-sam.py foo arg1 arg2andfoorequired root privileges, the decorator would useos.execvpto replace the current process withsudo python simoc-sam.py foo arg1 arg2.This works well for individual commands, but not if the
foo()function was called by other functions as in the following example:Running
python simoc-sam.py test-cmdwould:test_cmd1000foo(arg1, arg2)sudo python simoc-sam.py test-cmdbecause of@needs_roottest_cmdagain0(root user)foo(arg1, arg2)(as root)0againIOW, even though only
foorequired root privileges, the decorator would cause everything beforefoo()to be executed twice (first normally, then as root), and everything afterfoo()to be executed as root (even if it's not needed).This PR fixes this by replacing the
os.execvpcall with a subprocess that only runs the decorated function as root, with the caveat that the function must be a command too.Running
python simoc-sam.py test-cmdnow would:test_cmd1000foo(arg1, arg2)sudo python simoc-sam.py foo arg1 arg2because of@needs_root1000again