check permissions in doctor command #6129
check permissions in doctor command #6129
Conversation
de96d90
to
40c73e2
Compare
0f7c518
to
12cce8e
Compare
It turns out |
lib/bundler/cli/doctor.rb
Outdated
@@ -61,6 +62,7 @@ def check! | |||
end | |||
|
|||
def run | |||
check_home_permissions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is the best place to put the new home permissions check, it just jumped out to me as the most convenient. I'm happy to move it around if there is a more appropriate place.
lib/bundler/cli/doctor.rb
Outdated
|
||
def check_for_files_not_readable_or_writable | ||
return unless any_files_not_readable_or_writable? | ||
raise ProductionError, "Files exist in Bundler home that are not " \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose to raise ProductionError
because I saw it being raised in the run
method; if I should raise a different error please let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree with using exceptions as part of regular program behavior flow. I suggest printing a message to the user using Bundler.ui.info
instead.
lib/bundler/cli/doctor.rb
Outdated
|
||
def check_for_files_not_owned_by_current_user_but_still_rw | ||
return unless any_files_not_owned_by_current_user_but_still_rw? | ||
Bundler.ui.warn "Files exist in Bundler home that are owned by another " \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to say which files are not owned by the user or cannot be read/written to. The current error message does not have much value to the user as we're leaving it up to the user to go looking in the bundler dir to see which file(s) have the problem.
lib/bundler/cli/doctor.rb
Outdated
|
||
def check_for_files_not_readable_or_writable | ||
return unless any_files_not_readable_or_writable? | ||
raise ProductionError, "Files exist in Bundler home that are not " \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree with using exceptions as part of regular program behavior flow. I suggest printing a message to the user using Bundler.ui.info
instead.
lib/bundler/cli/doctor.rb
Outdated
Bundler.ui.warn "Files exist in Bundler home that are owned by another " \ | ||
"user, but are stil readable/writable" | ||
"user, but are stil readable/writable. These files are: #{files_not_owned_by_current_user_but_still_rw.join("\n")}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe These files are:\n - #{files.join("\n - ")}
so theres some more visual differentiation?
Right now, this is the output I get:
Files exist in Bundler home that are not readable/writable to the current user. These files are: /Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/CLAide-7f26d3d016ef/.git/objects/pack/pack-37fe8b0ecb7fb4bb549140a193f9b3e5f57bd982.idx
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/CLAide-7f26d3d016ef/.git/objects/pack/pack-37fe8b0ecb7fb4bb549140a193f9b3e5f57bd982.pack
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/Core-df21f20a2825/.git/objects/pack/pack-6a6bf20937cabbe439abad31192fb692d6585ae9.idx
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/Core-df21f20a2825/.git/objects/pack/pack-6a6bf20937cabbe439abad31192fb692d6585ae9.pack
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/Core-df6877912172/.git/objects/pack/pack-2c1d1f2113e0f0d1aa8cc9bbf30f63fc862dc08f.idx
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/Core-df6877912172/.git/objects/pack/pack-2c1d1f2113e0f0d1aa8cc9bbf30f63fc862dc08f.pack
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/Molinillo-b17c754d2b93/.git/objects/pack/pack-3d267bb97f882693d85a6e1b2851f665109fae4e.idx
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/Molinillo-b17c754d2b93/.git/objects/pack/pack-3d267bb97f882693d85a6e1b2851f665109fae4e.pack
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/Molinillo-b17c754d2b93/.git/objects/pack/pack-933914d968c3dd6509e6a5566f17d21f147f12f8.idx
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/Molinillo-b17c754d2b93/.git/objects/pack/pack-933914d968c3dd6509e6a5566f17d21f147f12f8.pack
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/Molinillo-c2f655cd6978/.git/objects/pack/pack-d63e0ec18a4aac8667d7c0bfc3deac537ad1f9f0.idx
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/Molinillo-c2f655cd6978/.git/objects/pack/pack-d63e0ec18a4aac8667d7c0bfc3deac537ad1f9f0.pack
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/Nanaimo-c9b1cf573452/.git/objects/pack/pack-20dc04ae125dbd210206bbb95986cf199c46d0e9.idx
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/Nanaimo-c9b1cf573452/.git/objects/pack/pack-20dc04ae125dbd210206bbb95986cf199c46d0e9.pack
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/Xcodeproj-4279382c6003/.git/objects/pack/pack-5584e2008093ac49ed59feb9082ee93cd70f0cc1.idx
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/Xcodeproj-4279382c6003/.git/objects/pack/pack-5584e2008093ac49ed59feb9082ee93cd70f0cc1.pack
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/Xcodeproj-d9be15fd7ac9/.git/objects/pack/pack-e4ebf9f8c265c099ac1869bd0b4c5ea222053a54.idx
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/Xcodeproj-d9be15fd7ac9/.git/objects/pack/pack-e4ebf9f8c265c099ac1869bd0b4c5ea222053a54.pack
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/cocoapods-deintegrate-3735087ac0d5/.git/objects/pack/pack-a08336c992731d0640ff4d3f6adb02131d2a8d5a.idx
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/cocoapods-deintegrate-3735087ac0d5/.git/objects/pack/pack-a08336c992731d0640ff4d3f6adb02131d2a8d5a.pack
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/cocoapods-downloader-74c8d27df0bf/.git/objects/pack/pack-7da53b6d4c74635672bcb2c8455171f14c79bdeb.idx
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/cocoapods-downloader-74c8d27df0bf/.git/objects/pack/pack-7da53b6d4c74635672bcb2c8455171f14c79bdeb.pack
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/cocoapods-plugins-c222b56ed644/.git/objects/pack/pack-23621f86d62c2e1c0722acb0f5621fac307f383b.idx
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/cocoapods-plugins-c222b56ed644/.git/objects/pack/pack-23621f86d62c2e1c0722acb0f5621fac307f383b.pack
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/cocoapods-search-eb60c856fc55/.git/objects/pack/pack-a86b94fa0bd854a521d7c6ab65bd67a27f9645d1.idx
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/cocoapods-search-eb60c856fc55/.git/objects/pack/pack-a86b94fa0bd854a521d7c6ab65bd67a27f9645d1.pack
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/cocoapods-stats-5eaccbc74899/.git/objects/pack/pack-4745e20dba5624f24613c734914364f70a54d5b3.idx
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/cocoapods-stats-5eaccbc74899/.git/objects/pack/pack-4745e20dba5624f24613c734914364f70a54d5b3.pack
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/cocoapods-trunk-649ccab360b3/.git/objects/pack/pack-a247d31191008395fabda8c36ac8e3ddbbd106d7.idx
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/cocoapods-trunk-649ccab360b3/.git/objects/pack/pack-a247d31191008395fabda8c36ac8e3ddbbd106d7.pack
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/cocoapods-trunk-a3d9e816e04b/.git/objects/pack/pack-cd3508e822e9ee6bf974849b8194a8db6222088e.idx
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/cocoapods-trunk-a3d9e816e04b/.git/objects/pack/pack-cd3508e822e9ee6bf974849b8194a8db6222088e.pack
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/cocoapods-try-2c0840a6ba64/.git/objects/pack/pack-7189861dc396805fd212e6aaee59813717ac8d4b.idx
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/cocoapods-try-2c0840a6ba64/.git/objects/pack/pack-7189861dc396805fd212e6aaee59813717ac8d4b.pack
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/json-a9588bc4334c/.git/objects/pack/pack-4da8020924009c5291978ebe90f221cb3d33943f.idx
/Users/segiddins/Development/Rainforest/CocoaPods/.bundle/ruby/2.4.0/bundler/gems/json-a9588bc4334c/.git/objects/pack/pack-4da8020924009c5291978ebe90f221cb3d33943f.pack
Hey @ajwann, I made a few small changes, if you want to look them over. @colby-swandale r? |
@segiddins you're changes look good to me! Thanks for your help. |
lib/bundler/cli/doctor.rb
Outdated
files_not_readable_or_writable = [] | ||
files_not_owned_by_current_user_but_still_rw = [] | ||
Find.find(Bundler.home.to_s).each do |f| | ||
if !File.writable?(f) || !File.readable?(f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern here is that a file can be not readable/writable and also be owned by a different user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@segiddins, @colby-swandale I can add that functionality if it's needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool PR!
I made inline comments on details where I had questions as a reader.
lib/bundler/cli/doctor.rb
Outdated
ok = true | ||
if files_not_owned_by_current_user_but_still_rw.any? | ||
Bundler.ui.warn "Files exist in the Bundler home that are owned by another " \ | ||
"user, but are stil readable/writable. These files are:\n - #{files_not_owned_by_current_user_but_still_rw.join("\n - ")}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: stil
=> still
(String also appears in the tests.)
lib/bundler/cli/doctor.rb
Outdated
|
||
if files_not_readable_or_writable.any? | ||
Bundler.ui.warn "Files exist in the Bundler home that are not " \ | ||
"readable/writable to the current user. These files are:\n - #{files_not_readable_or_writable.join("\n - ")}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammar note: to the current user
=> by the current user
(String also appears in the tests.)
@@ -86,9 +88,41 @@ def run | |||
end | |||
end.flatten.sort.each {|m| message += m } | |||
raise ProductionError, message | |||
else | |||
elsif !permissions_valid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Is there a !
too much in this conditional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally like how @segiddins implemented the permissions_valid
method to be truthy, and then negated it. I prefer having a truthy method like permissions_valid
with a negation, as opposed to a falsy method (like permissions_invalid
), with no negation.
That's just my preference though, and if anyone has a counterargument I'm open to hearing it.
spec/commands/doctor_spec.rb
Outdated
@@ -58,4 +57,35 @@ | |||
* rack: /usr/local/opt/icu4c/lib/libicui18n.57.1.dylib | |||
E | |||
end | |||
|
|||
it "exits with an error if home contains files that are not read/write" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps: read/write
=> readable/writable
?
@olleolleolle I appreciate the feedback and I'm fine with making those changes. @segiddins I should have time this weekend to wrap up this PR. |
Thanks @ajwann ! |
2c942b8
to
8ce18f7
Compare
@olleolleolle, @segiddins and @colby-swandale; this PR should be good to go. Please let me know if you guys need any additional changes! |
spec/commands/doctor_spec.rb
Outdated
@@ -58,4 +57,51 @@ | |||
* rack: /usr/local/opt/icu4c/lib/libicui18n.57.1.dylib | |||
E | |||
end | |||
|
|||
it "exits with an error if home contains files that are not readable/writable" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests can be wrapped with context blocks.
it 'exits with an error if home contains files that are not readable/writable'`
can be better written as:
context 'when home contains files that are not readable/writable'
it 'exits with an error'
It also means that we can move some of setup for these tests into before
blocks
(Same applies to the other specs)
spec/commands/doctor_spec.rb
Outdated
gem "rack" | ||
G | ||
|
||
stat = double("stat") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need these to be applied for every test in the file? i would wrap these and the check file permission specs in a describe
so we keep everything close and only use these we're needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need that double on line 15 to be applied for every test, because we need the existing tests to be able to stat their files and see that they were created by the current user (Process.uid
)
@colby-swandale I think I've dried up the tests as much as can be, but if you see something specific let me know. |
bab8a62
to
65f5479
Compare
65f5479
to
23f434b
Compare
23f434b
to
d82a9ef
Compare
I'm not sure why the build failed here, since all I did was squash some commits. I re-ran |
It looks like the build keeps failing on |
#6275 should fix this issue |
@bundlerbot r+ |
📌 Commit d82a9ef has been approved by |
…olby-swandale check permissions in doctor command Thanks so much for the contribution! To make reviewing this PR a bit easier, please fill out answers to the following questions. ### What was the end-user problem that led to this PR? The problem was... #5786 > We should have a check in bundle doctor for the file/folder permissions in the Bundler home directory. >We should print a warning if there are any files/folders that is owned by another user but is readable/writable but prints an error when a file cannot be read or written to. ### What is your fix for the problem, implemented in this PR? Created private method ```check_home_permissions``` that will print a warning if there are any files/folders that are owned by another user but are readable/writable, and print an error when the bundler home dir contains a file cannot be read or written to ### Why did you choose this fix out of the possible options? I chose this fix because it's what was requested in the open issue.
☀️ Test successful - status-travis |
…olby-swandale check permissions in doctor command Thanks so much for the contribution! To make reviewing this PR a bit easier, please fill out answers to the following questions. ### What was the end-user problem that led to this PR? The problem was... #5786 > We should have a check in bundle doctor for the file/folder permissions in the Bundler home directory. >We should print a warning if there are any files/folders that is owned by another user but is readable/writable but prints an error when a file cannot be read or written to. ### What is your fix for the problem, implemented in this PR? Created private method ```check_home_permissions``` that will print a warning if there are any files/folders that are owned by another user but are readable/writable, and print an error when the bundler home dir contains a file cannot be read or written to ### Why did you choose this fix out of the possible options? I chose this fix because it's what was requested in the open issue. (cherry picked from commit fe9d698)
…olby-swandale check permissions in doctor command Thanks so much for the contribution! To make reviewing this PR a bit easier, please fill out answers to the following questions. ### What was the end-user problem that led to this PR? The problem was... #5786 > We should have a check in bundle doctor for the file/folder permissions in the Bundler home directory. >We should print a warning if there are any files/folders that is owned by another user but is readable/writable but prints an error when a file cannot be read or written to. ### What is your fix for the problem, implemented in this PR? Created private method ```check_home_permissions``` that will print a warning if there are any files/folders that are owned by another user but are readable/writable, and print an error when the bundler home dir contains a file cannot be read or written to ### Why did you choose this fix out of the possible options? I chose this fix because it's what was requested in the open issue. (cherry picked from commit fe9d698)
Thanks so much for the contribution!
To make reviewing this PR a bit easier, please fill out answers to the following questions.
What was the end-user problem that led to this PR?
The problem was...
#5786
What is your fix for the problem, implemented in this PR?
Created private method
check_home_permissions
that will print a warning if there are any files/folders that are owned by another user but are readable/writable, and print an error when thebundler home dir contains a file cannot be read or written to
Why did you choose this fix out of the possible options?
I chose this fix because it's what was requested in the open issue.