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

Checking if path is writable in pip.utils.filesystem.check_path_owner #2396

Merged
merged 2 commits into from Apr 7, 2015

Conversation

Projects
None yet
2 participants
@sashkab
Contributor

sashkab commented Feb 4, 2015

Fixes #2390.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Mar 16, 2015

Member

I know I suggested this, but I realized why I didn't do this to begin with. That directory is writable to the root user when using sudo without the sudo -H flag. So this can't actually work like this without rendering the warning I originally added useless.

So I guess the underlying question here is what check is reasonable (or if no check is reasonable we should just remove the warning and the check all together).

Member

dstufft commented Mar 16, 2015

I know I suggested this, but I realized why I didn't do this to begin with. That directory is writable to the root user when using sudo without the sudo -H flag. So this can't actually work like this without rendering the warning I originally added useless.

So I guess the underlying question here is what check is reasonable (or if no check is reasonable we should just remove the warning and the check all together).

@sashkab

This comment has been minimized.

Show comment
Hide comment
@sashkab

sashkab Mar 16, 2015

Contributor

If user can write - then write. If can't - raise or disable caching with the warning.

Contributor

sashkab commented Mar 16, 2015

If user can write - then write. If can't - raise or disable caching with the warning.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Mar 16, 2015

Member

That's what we did originally. The problem with that is when people did sudo pip install <foo>, that sudo doesn't reset the value of $HOME. So people had the root account writing files into their person home directories and causing them a lot of problems. The warning was put into place to help guide those people to use sudo -H pip install <foo> instead which causes sudo to reset the value of $HOME.

That doesn't mean that we can't change that of course. Perhaps the other @pypa/pip-developers have an opinion. I mostly am trying to decide if anything has changed between when we put in the warning and now (people updated their scripts to do -H?), or if the original warning was a bad idea, or if we should perhaps modify it (maybe explicitly check for sudo or root since that was the underlying cause).

Member

dstufft commented Mar 16, 2015

That's what we did originally. The problem with that is when people did sudo pip install <foo>, that sudo doesn't reset the value of $HOME. So people had the root account writing files into their person home directories and causing them a lot of problems. The warning was put into place to help guide those people to use sudo -H pip install <foo> instead which causes sudo to reset the value of $HOME.

That doesn't mean that we can't change that of course. Perhaps the other @pypa/pip-developers have an opinion. I mostly am trying to decide if anything has changed between when we put in the warning and now (people updated their scripts to do -H?), or if the original warning was a bad idea, or if we should perhaps modify it (maybe explicitly check for sudo or root since that was the underlying cause).

@sashkab

This comment has been minimized.

Show comment
Hide comment
@sashkab

sashkab Mar 16, 2015

Contributor

We need to assume worst, that they didn't.

What we need to do is to check if userid is root, but owner of cache_dir isn't - disable cache, as a workaround for this case.

Contributor

sashkab commented Mar 16, 2015

We need to assume worst, that they didn't.

What we need to do is to check if userid is root, but owner of cache_dir isn't - disable cache, as a workaround for this case.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Mar 16, 2015

Member

That works for me.

Member

dstufft commented Mar 16, 2015

That works for me.

@sashkab

This comment has been minimized.

Show comment
Hide comment
@sashkab

sashkab Mar 19, 2015

Contributor

@sdtufft I've implemented change as discussed.

Contributor

sashkab commented Mar 19, 2015

@sdtufft I've implemented change as discussed.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Apr 7, 2015

Member

Sorry! I didn't see the comment on this.

Member

dstufft commented Apr 7, 2015

Sorry! I didn't see the comment on this.

dstufft added a commit that referenced this pull request Apr 7, 2015

Merge pull request #2396 from sashkab/b2390-check-path-owner
Checking if path is writable in pip.utils.filesystem.check_path_owner

@dstufft dstufft merged commit 3d78b82 into pypa:develop Apr 7, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment