-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix relative path expansion #4
Conversation
Thanks for your work on this. I think the bug is related the working directory for the elevated process being different (see here for pkexec). I'd guess that Windows and I need to think about this as it |
ok, this is WIP but pretty cool and it works in my Python 3/2.7.15+, with sudo and pkexec. the challenge is, print the first line of the first argument. $ ./example.py README.rst
before: /home/cat/Sync/projects/git/elevate
before: False
execlp ['/usr/bin/python3', '/home/cat/Sync/projects/git/elevate/example.py', '--_with-elevate-cwd=/home/cat/Sync/projects/git/elevate', 'README.rst']
before: /home/cat/Sync/projects/git/elevate
before: True
after: /home/cat/Sync/projects/git/elevate
after: True
Elevate: Request root privileges
$ ./example.py README.rst
before: /home/cat/Sync/projects/git/elevate
before: False
execlp ['/usr/bin/python3', '/home/cat/Sync/projects/git/elevate/example.py', '--_with-elevate-cwd=/home/cat/Sync/projects/git/elevate', 'README.rst']
before: /root
before: True
after: /home/cat/Sync/projects/git/elevate
after: True
Elevate: Request root privileges
$ cd ..
$ elevate/example.py elevate/README.rst
before: /home/cat/Sync/projects/git
before: False
execlp ['/usr/bin/python3', '/home/cat/Sync/projects/git/elevate/example.py', '--_with-elevate-cwd=/home/cat/Sync/projects/git', 'elevate/README.rst']
before: /root
before: True
after: /home/cat/Sync/projects/git
after: True
Elevate: Request root privileges
$ elevate/example.py elevate/README.rst
before: /home/cat/Sync/projects/git
before: False
execlp ['/usr/bin/python3', '/home/cat/Sync/projects/git/elevate/example.py', '--_with-elevate-cwd=/home/cat/Sync/projects/git', 'elevate/README.rst']
before: /root
before: True
after: /home/cat/Sync/projects/git
after: True
['/home/cat/Sync/projects/git/elevate/example.py', 'elevate/README.rst']
Elevate: Request root privileges this -- preserving the previous directory with a secret implementation detail -- obviated the need to translate other arguments than now, let me go and test it / fix it up on Windows and maybe BSD or some other Linuxes. I don't have a Mac, though. |
01b9e4d
to
e4e1e15
Compare
I can't reproduce #3 on Windows at all, especially not with the new abspath, so I don't think any further changes to that effect in Windows are needed. |
It seems this (mostly) fixes #5 because the way the elevated process is invoked makes it very easy to prevent infinite recursion (although I could leave that for a separate PR). (when ready for merge, I'll squash all these and get rid of all the debugging print()s and |
Sorry for the slow response. This is a clever fix, and the Windows issue hadn't occurred to me. I worry that this prevents a user calling As for #5, I wonder if we can't detect the failure of the UAC thing through some other means, either in the parent or child process? |
I'm not sure what you mean, could you elaborate? Since
Ok, I can understand that! I'll add it.
From my testing, it's completely undetectable / opaque and the only way to prevent an infinite recursion, other than sharing memory directly between the old and new processes, is by passing data to the new process invoked by elevate, through the command line (or maybe a named pipe...) |
Sorry for the poor explanation. I'm considering this sort of use case:
This use case should work fine because it hasn't actually modified anything or performed any real work. I left "early" a little undefined in the README, but it basically means "as late as your program can be considered re-entrant".
How frustrating! Your approach sounds good in this case 👍 |
Ah, that is quite a significant problem I forgot about. There are only a couple options I can think of,
IMO this last point isn't a big problem considering that elevate has the responsibility of re-starting the current process and it makes sense to think that maybe it has the authority to hook its import. |
Anyway, on that note, now there's an import hook (see the commit message) which filters and remembers the options when The question is, should that import hook become a public function that the user can call if they want |
de7fa09
to
6d07020
Compare
Actually, this is cool, there is one more solution, it's to make it into an OO interface (but the same in the backend), then the argument parsing modification is only done when the user invokes the class instead of at import time automatically. Also, no more module global variable, that's usually a plus of an OO re-design. Unfortunately it slightly changes elevate from being a "simple" one-function functional interface, but that's what it takes to get around these edge cases. catb0t@f48dc48 |
- close barneygale#3: relative path expansion - **temporarily** fix barneygale#5 for windows - documentation is added to the workarounds in elevate_util This implementation uses an import hook, which may not be the cleanest solution possible. The import hook fixes argparse and similar modules seeing and complaining about --_with-elevate-* options, if the argument parser is called before the re-entrant call to elevate().
I squashed the 11 commits on this branch and cleaned up the commit message. If you prefer the class-based OO implementation I mentioned in my last comment, over this import-hook, then I can rebase this branch on |
Sorry for the long delay. I'm not a fan of the import-time stuff as it feels too magic. I think the best course of action is to not add command-line arguments by default - but if users want to opt into it, we shouldn't try to hide it from them. What do you think of 6b788dc? Only in a branch for now. |
Sorry, I finally got around to testing your code. It's objectively superior to my implementation, including my class-based one, but I personally prefer mine. I think it's a bit silly how basic functionality becomes opt-in, but you're the maintainer, and I think you should go with your solution. |
For me it's a finely-balanced thing.
My main reason is this: I think the problem I appreciate the thought and effort you've put into this - I couldn't have arrived at my patch without yours, so I'm sorry to you to be closing a 6 month old PR without merging. I'm not ruling out changing the default in future if I hear more feedback. Thank you again! I promise I'm not usually this much of a diva. |
closes #3, for Windows as well
closes #5
before this patch
after this patch