Don't fallback to tempdir when required directories exist. #6550
Conversation
|
Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality. We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below. If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #bundler channel on Slack. For more information about contributing to the Bundler project feel free to review our CONTRIBUTING guide |
brodock
force-pushed
the
6546-fix-home-permissions
branch
3 times, most recently
from
0a9acf0
to
03f482f
Compare
brodock
force-pushed
the
6546-fix-home-permissions
branch
2 times, most recently
from
5162c94
to
4159879
Compare
lib/bundler.rb
Outdated
| @@ -159,12 +159,13 @@ def ruby_scope | |||
| def user_home | |||
| @user_home ||= begin | |||
| home = Bundler.rubygems.user_home | |||
| user_dirs = home ? %w[.bundle .gem].map {|path| File.join(home, path) } : [] | |||
There was a problem hiding this comment.
why is .gem called out here? I could set my GEM_HOME to something other than ~/.gem
There was a problem hiding this comment.
Good question, I didn't think too much on the required folders. I think now we only need the .bundle.
brodock
force-pushed
the
6546-fix-home-permissions
branch
from
4159879
to
467d90a
Compare
|
@segiddins code now checks only |
brodock
force-pushed
the
6546-fix-home-permissions
branch
from
467d90a
to
facf625
Compare
|
Is the ‘.bundle’ folder not defined anywhere in SharedHelpers or in Bundler? I’m concerned about having a hard coded string. |
|
@colby-swandale There are other 3 relevant hard-coded |
|
Thanks for finding that out, we will have to refactor that out in the future. |
lib/bundler.rb
Outdated
| @@ -159,12 +159,13 @@ def ruby_scope | |||
| def user_home | |||
| @user_home ||= begin | |||
| home = Bundler.rubygems.user_home | |||
| dotbundle = home ? File.join(home, ".bundle") : nil | |||
There was a problem hiding this comment.
This is super nitpicky but can we rename this to bundle_home. dotbundle doesn't really mean anything. 🙏
When home directory is not writable, but the required .bundle is, we should use it instead of falling back to use tempdirs. This creates a workaround for more restrictive setups using Omnibus Docker or any hardened setup, to overcome the annoyances introduced by rubygems#4951.
brodock
force-pushed
the
6546-fix-home-permissions
branch
from
facf625
to
fb40210
Compare
|
@colby-swandale done :) |
|
Thanks! @bundlerbot r+ |
|
📌 Commit fb40210 has been approved by |
…dale Don't fallback to tempdir when required directories exist. ### What was the end-user problem that led to this PR? When running Omnibus packaged software with updated bundler, a warning is displayed because the home folder is not owned by the user: ``` `/var/opt/gitlab` is not writable. Bundler will use `/tmp/bundler/home/root' as your home directory temporarily. ``` There are valid reasons why this is desired, and I don't have control over it. What I can do is create the required folders used by bundler and provide them with the right permissions. See #6546 ### What was your diagnosis of the problem? In practice instead of asking for permission on a higher level, if required folders are present and they have the right permissions, we shouldn't fallback to warning + temp directory, we should just use what is provided. ### What is your fix for the problem, implemented in this PR? When home directory is not writable, but the required .gem and .bundle are, we should use them instead of falling back to use tempdirs. This creates a workaround for more restrictive setups using Omnibus Docker or any hardened setup, to overcome the annoyances introduced by #4951. ### Why did you choose this fix out of the possible options? This allows for distributions, package maintainers, etc to provide an alternative while keeping their hardenings requirements. When provided the required folders with the required ownership/permission, we should not bother by not having any write permissions on the `$HOME` directory.
|
☀️ Test successful - status-travis |
…dale Don't fallback to tempdir when required directories exist. ### What was the end-user problem that led to this PR? When running Omnibus packaged software with updated bundler, a warning is displayed because the home folder is not owned by the user: ``` `/var/opt/gitlab` is not writable. Bundler will use `/tmp/bundler/home/root' as your home directory temporarily. ``` There are valid reasons why this is desired, and I don't have control over it. What I can do is create the required folders used by bundler and provide them with the right permissions. See #6546 ### What was your diagnosis of the problem? In practice instead of asking for permission on a higher level, if required folders are present and they have the right permissions, we shouldn't fallback to warning + temp directory, we should just use what is provided. ### What is your fix for the problem, implemented in this PR? When home directory is not writable, but the required .gem and .bundle are, we should use them instead of falling back to use tempdirs. This creates a workaround for more restrictive setups using Omnibus Docker or any hardened setup, to overcome the annoyances introduced by #4951. ### Why did you choose this fix out of the possible options? This allows for distributions, package maintainers, etc to provide an alternative while keeping their hardenings requirements. When provided the required folders with the required ownership/permission, we should not bother by not having any write permissions on the `$HOME` directory. (cherry picked from commit 31b53cf)
A few reasons for this change: GitLab CE and Gitaly are now using bundler v1.16.6 to avoid rubygems/bundler#6537. 1.16.3 also ships with rubygems/bundler#6550, so we no longer need the patch for this.
What was the end-user problem that led to this PR?
When running Omnibus packaged software with updated bundler, a warning is displayed because the home folder is not owned by the user:
There are valid reasons why this is desired, and I don't have control over it. What I can do is create the required folders used by bundler and provide them with the right permissions.
See #6546
What was your diagnosis of the problem?
In practice instead of asking for permission on a higher level, if required folders are present and they have the right permissions, we shouldn't fallback to warning + temp directory, we should just use what is provided.
What is your fix for the problem, implemented in this PR?
When home directory is not writable, but the required .gem and .bundle
are, we should use them instead of falling back to use tempdirs.
This creates a workaround for more restrictive setups using Omnibus
Docker or any hardened setup, to overcome the annoyances introduced by #4951.
Why did you choose this fix out of the possible options?
This allows for distributions, package maintainers, etc to provide an alternative while keeping their hardenings requirements.
When provided the required folders with the required ownership/permission, we should not bother by not having any write permissions on the
$HOMEdirectory.