Skip to content

appdirs: return paths relative to effective user's homedir #5360

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

Closed
wants to merge 2 commits into from

Conversation

terminalmage
Copy link

When executing a pip install from within a Python process in which the euid has been changed, the appdirs helpers which use expanduser() return paths relative to the home directory of the actual user, rather than the effective user. This causes the install to fail when the effective user cannot write to the cachedir:

>>> import subprocess
>>> import os
>>> os.geteuid()
0
>>> os.seteuid(1000)
>>> os.geteuid()
1000
>>> p = subprocess.Popen(['/home/erik/virtualenv/pipdev/bin/pip', 'install', 'pudb'], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
>>> stdout, stderr = p.communicate()
>>> p.returncode
1
>>> stderr
"Could not install packages due to an EnvironmentError: [Errno 13] Permission denied: '/root/.cache/pip/wheels/ec/bb/79/09cd8a9982b637b251208ac3400f9702c72e63accc8e4b69e3'\nConsider using the `--user` option or check the permissions.\n\n"
>>>

With this bugfix, the installation proceeds as expected, and the sources/wheels are downloaded to the effective user's cachedir:

>>> import subprocess
>>> import os
>>> os.geteuid()
0
>>> os.seteuid(1000)
>>> os.geteuid()
1000
>>> p = subprocess.Popen(['/home/erik/virtualenv/pipdev/bin/pip', 'install', 'pudb'], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
>>> stdout, stderr = p.communicate()
>>> p.returncode
0
>>> print(stdout)
Collecting pudb
Collecting urwid>=1.1.1 (from pudb)
Collecting pygments>=1.0 (from pudb)
  Using cached https://files.pythonhosted.org/packages/02/ee/b6e02dc6529e82b75bb06823ff7d005b141037cb1416b10c6f00fc419dca/Pygments-2.2.0-py2.py3-none-any.whl
Installing collected packages: urwid, pygments, pudb
Successfully installed pudb-2017.1.4 pygments-2.2.0 urwid-2.0.1

>>>

With regard to testing, I'm not certain how this would work with the existing setup, as the code appears to just be patching function calls, and in the absence of something like a MagicMock where you can do more complicated patching, I'm unsure how we'd confirm that we're pulling from the effective user's environment. I would welcome any guidance offered in this respect.

Resolves #5359.

@terminalmage terminalmage force-pushed the appdirs-changed-euid branch from 6acf7cd to 81020d6 Compare May 3, 2018 05:56
Since `expanduser()` is used to expand the `~` to the home directory,
`~/.cache` will always return the home directory of the user under which
the python process is running. However, if pip is executed from within
a Python process in which the euid has changed, this means that the
cache dir (and other paths) will not be writable, causing pip to fail.

This commit fixes that issue by attempting to get the username matching
the euid, and using that username (e.g. `~user/.cache`) when invoking
`expanduser()`.
@terminalmage terminalmage force-pushed the appdirs-changed-euid branch from 81020d6 to 1915d8b Compare May 3, 2018 06:01
@terminalmage
Copy link
Author

Unit test failures are clearly related, I'll take a look on Thursday or Friday.

@benoit-pierre
Copy link
Member

Note: there are 2 copies of appdirs: ./src/pip/_internal/utils/appdirs.py and ./src/pip/_vendor/appdirs.py. IMHO this should be reported to the appdirs project, and pip can get the fix by re-vendoring the new version and updating its copy. See previous discussion on whether pip's internal copy should be dropped or not.

@terminalmage
Copy link
Author

Ahh, thanks for catching (this is what I get when I don't read the docstring!). I can resubmit upstream.

@terminalmage
Copy link
Author

Submitted upstream as ActiveState/appdirs#105

@benoit-pierre
Copy link
Member

I'm also not sure this is a bug and not normal behavior if HOME or one of the XDG.. variable is set. Is that the case? Shouldn't you pass a patched-up environment to subprocess?

@pradyunsg pradyunsg added S: needs triage Issues/PRs that need to be triaged resolution: no action When the resolution is to not do anything and removed S: needs triage Issues/PRs that need to be triaged labels May 11, 2018
@pradyunsg
Copy link
Member

pradyunsg commented May 11, 2018

Closing this PR since the fix has to happen upstream and this hasn't been shown to be significant and/or urgent enough that pip carries a patch for it.

@pradyunsg pradyunsg closed this May 11, 2018
@terminalmage
Copy link
Author

I agree, it's not terribly urgent. It's true that one can pass a HOME to subprocess via the env, though in my opinion one shouldn't have to do so. But given that there is a simple workaround, getting this fix from upstream doesn't seem like a high priority.

@lock
Copy link

lock bot commented Jun 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation resolution: no action When the resolution is to not do anything
Projects
None yet
Development

Successfully merging this pull request may close these issues.

appdirs helpers return wrong path when euid changed
3 participants