Skip to content
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

fixed sudo checks #47

Merged
merged 1 commit into from Mar 7, 2019

Conversation

Projects
None yet
2 participants
@rcmdnk
Copy link

rcmdnk commented Feb 7, 2019

To check sudo command,
it is better to check /bin/mkdir -p #{HOMEBREW_PREFIX_DEFAULT}" command instead of using -v,
because users can have limited sudo right and they can sudo -v test.

In addition Select the Linuxbrew installation directory prompts were not working before.
This PR changes to do:

  • Check /usr/bin/sudo -n /bin/mkdir -p #{HOMEBREW_PREFIX_DEFAULT}
  • If not, ask user to select (Enter password for /home/linuxbrew, C-D for $HOME/, C-C to interrupt)
  • If Password is entered, try /usr/bin/sudo /bin/mkdir -p #{HOMEBREW_PREFIX_DEFAULT}
  • If not, or C-D, use $HOME/

@sjackman sjackman self-assigned this Feb 7, 2019

install Outdated
# Tests will fail if the prefix exists, but we don't have execution
# permissions. Abort in this case.
abort <<-EOABORT if File.directory?(HOMEBREW_PREFIX) && (!File.executable? HOMEBREW_PREFIX)
The Homebrew prefix, #{HOMEBREW_PREFIX}, exists but is not searchable. If this is
not intentional, please restore the default permissions and try running the
installer again:
sudo chmod 775 #{HOMEBREW_PREFIX}
sudo chmod 775 $USER #{HOMEBREW_PREFIX}

This comment has been minimized.

@sjackman

sjackman Feb 7, 2019

Member

What's the purpose of this change?

This comment has been minimized.

@rcmdnk

rcmdnk Feb 8, 2019

Author

Sorry, this is just a misunderstanding that I saw somehow chmod as chown.
$USER was removed.

@sjackman

This comment has been minimized.

Copy link
Member

sjackman commented Feb 7, 2019

I'm happy with the purpose of this PR. Thanks for this work, @rcmdnk. The patch seems bigger than necessary though. Can we instead replace /usr/bin/sudo -v with /usr/bin/sudo -l /bin/mkdir >/dev/null ?

install Outdated
# Invalidate sudo timestamp before exiting (if it wasn't active before).
Kernel.system "/usr/bin/sudo -n -v 2>/dev/null"
at_exit { Kernel.system "/usr/bin/sudo", "-k" } unless $CHILD_STATUS.success?

This comment has been minimized.

@sjackman

sjackman Feb 7, 2019

Member

Instead of moving this chunk let's patch line 222.

This comment has been minimized.

@rcmdnk

rcmdnk Feb 8, 2019

Author

As sudo is used only when it is needed, it should be added below.

This comment has been minimized.

@sjackman

sjackman Feb 8, 2019

Member

Let's solve one issue per PR. This issue need not be address in this PR.

This comment has been minimized.

@rcmdnk

rcmdnk Feb 8, 2019

Author

This is directly related to sudo check below.
If a password is required, this simple check fails and does not implement sudo -k at exit.

install Outdated
@@ -218,10 +211,6 @@ def shell_profile
end
end

# Invalidate sudo timestamp before exiting (if it wasn't active before).
Kernel.system "/usr/bin/sudo -n -v 2>/dev/null"

This comment has been minimized.

@sjackman

sjackman Feb 7, 2019

Member

You can prevent the prompt by disabling this line on Linux.

Kernel.system "/usr/bin/sudo -n -v 2>/dev/null" if mac?

This comment has been minimized.

@rcmdnk

rcmdnk Feb 8, 2019

Author

It is unnecessary because sudo will be checked only when it is needed in below.

This comment has been minimized.

@sjackman

sjackman Feb 8, 2019

Member

Yep, so disable this line on Linux as above.

This comment has been minimized.

@rcmdnk

rcmdnk Feb 8, 2019

Author

On Mac, in where sudo requires a password,
sudo -n -v fails.
Therefore these lines are meaningless if mac is added.

This comment has been minimized.

@sjackman

This comment has been minimized.

@rcmdnk

rcmdnk Feb 8, 2019

Author

Sorry, the previous comment is wrong and I had misunderstandings.
This at_exit code works as expected and do not needed to be moved,
as sudo -k do nothing as you mentioned.

I will change them back as before about at_exit.

install Outdated
out = io.read
io.close
@have_sudo = $CHILD_STATUS.success?
if (not @have_sudo) && out == "sudo: a password is required\n"

This comment has been minimized.

@sjackman

sjackman Feb 7, 2019

Member

How about…

    Kernel.system "/usr/bin/sudo -n -l /bin/mkdir >/dev/null 2>/dev/null"
    unless $CHILD_STATUS.success?
…

This comment has been minimized.

@rcmdnk

rcmdnk Feb 8, 2019

Author

Some users have right to do sudo with password, and some have right w/o password.
This is needed to distinguish users which do not have the right anyway.
Otherwise, the next prompt will be strange for these users.

This comment has been minimized.

@rcmdnk

rcmdnk Feb 8, 2019

Author

sudo -l /bin/mkdir instead of sudo /bin/mkdir -p #{HOMEBREW_PREFIX_DEFAULT} seems fine.

This comment has been minimized.

@sjackman

sjackman Feb 8, 2019

Member

You can use sudo -nv to check whether a user has sudo access without a password.

This comment has been minimized.

@rcmdnk

rcmdnk Feb 8, 2019

Author

Yes, but it fails if the user can do sudo only with a password.
Therefore, here,

  1. first check if the user can use sudo without password by sudo -n -l mkdir.
  2. If failed, check if a password is required or not by the output.
  3. Only if a password is required, ask user to put password to create /home/linuxbrew otherwise make $HOME/.linuxbrew

This comment has been minimized.

@sjackman

sjackman Feb 8, 2019

Member

I suggest…

if `sudo -nv 2>&1` == "sudo: a password is required\n"

This comment has been minimized.

@rcmdnk

rcmdnk Feb 8, 2019

Author

Yes, that is enough in this case. updated.

@sjackman
Copy link
Member

sjackman left a comment

Thanks for this work!

install Outdated
# Invalidate sudo timestamp before exiting (if it wasn't active before).
at_exit { Kernel.system "/usr/bin/sudo", "-k" } if @have_sudo


This comment has been minimized.

@sjackman

sjackman Feb 7, 2019

Member

Please remove this chunk.

This comment has been minimized.

@rcmdnk

rcmdnk Feb 8, 2019

Author

This needs to be here because have_sudo is just set above.

This comment has been minimized.

@sjackman

sjackman Feb 8, 2019

Member

I believe sudo -k is successful whether or not the user has sudo access, so it doesn't need to be conditional on @have_sudo.

This comment has been minimized.

@rcmdnk

rcmdnk Feb 8, 2019

Author

Yes, both cases work in the same as a result.

I just realized that if we want to implement correctly as a comment (if it wasn't active before),
we put this line after line 242.

@@ -9,9 +9,8 @@ case $(uname -m) in
*) echo "Error: Your CPU $(uname -m) is not supported." >&2; exit 1 ;;
esac

if test -w /home/linuxbrew || sudo -v; then
if test -w /home/linuxbrew || sudo mkdir -p /home/linuxbrew; then

This comment has been minimized.

@sjackman

sjackman Feb 7, 2019

Member

I like this change. Thanks! Could you please add sudo -p to add a custom prompt?

Suggested change
if test -w /home/linuxbrew || sudo mkdir -p /home/linuxbrew; then
if test -w /home/linuxbrew || /usr/bin/sudo -p "[sudo] Enter password for $USER to install Ruby: " mkdir -p /home/linuxbrew; then

This comment has been minimized.

@rcmdnk

rcmdnk Feb 8, 2019

Author

This seems good, included.

@rcmdnk rcmdnk force-pushed the rcmdnk:fix_sudo branch 2 times, most recently from f8bba42 to 1491f2e Feb 8, 2019

@sjackman

This comment has been minimized.

Copy link
Member

sjackman commented Feb 10, 2019

Please fix these style errors:

$ gem install rubocop
$ rubocop
Inspecting 3 files
C..
Offenses:
install:236:9: C: Style/Not: Use ! instead of not.
    if (not @have_sudo) && out == "sudo: a password is required\n"
        ^^^
install:244:7: C: Metrics/BlockNesting: Avoid more than 3 levels of block nesting.
      rescue Interrupt ...
      ^^^^^^^^^^^^^^^^
3 files inspected, 2 offenses detected
The command "rubocop" exited with 1.

See the TravisCI output https://travis-ci.org/Linuxbrew/install/builds/490450955

@rcmdnk rcmdnk force-pushed the rcmdnk:fix_sudo branch from 1491f2e to d992501 Feb 10, 2019

@rcmdnk

This comment has been minimized.

Copy link
Author

rcmdnk commented Feb 10, 2019

To reduce block nests, a new function prefix was implemented.

Show resolved Hide resolved install

@rcmdnk rcmdnk force-pushed the rcmdnk:fix_sudo branch from a928afa to 1644031 Feb 11, 2019

@rcmdnk

This comment has been minimized.

Copy link
Author

rcmdnk commented Feb 11, 2019

I refactored the code and commits.
I think this is a minimum change to have similar functions as before.

install Outdated
@have_sudo = $CHILD_STATUS && $CHILD_STATUS.success?
begin
Kernel.system "/usr/bin/sudo", "-l", "mkdir"
@have_sudo = $CHILD_STATUS.success?

This comment has been minimized.

@sjackman

sjackman Feb 12, 2019

Member
Suggested change
@have_sudo = $CHILD_STATUS.success?
@have_sudo = $CHILD_STATUS&.success?

$CHILD_STATUS can be nil if /usr/bin/sudo does not exist.

This comment has been minimized.

@rcmdnk

rcmdnk Mar 6, 2019

Author

Changed to

@have_sudo = $CHILD_STATUS && $CHILD_STATUS.success? || false

to keep compatibility for Ruby 2.2

This comment has been minimized.

@sjackman

sjackman Mar 6, 2019

Member

No need for the || false. The falsey value of nil for false is fine.

install Outdated
@have_sudo = $CHILD_STATUS.success?
rescue Interrupt
exit!
end

This comment has been minimized.

@sjackman

sjackman Feb 12, 2019

Member

Is the begin/rescue/end block needed? Please remove it if not.

This comment has been minimized.

@rcmdnk

rcmdnk Mar 6, 2019

Author

If this does not exist,
Ctrl-C shows traceback message.

This comment has been minimized.

@sjackman

sjackman Mar 6, 2019

Member

Thanks for the explanation. I believe exit should work and also prevent the traceback. Let's use that rather than exit!.

This comment has been minimized.

@rcmdnk

rcmdnk Mar 7, 2019

Author

I think I misunderstood your comment.
begin~end is not necessary here, so I removed it.

Show resolved Hide resolved install Outdated
@sjackman

This comment has been minimized.

Copy link
Member

sjackman commented Feb 12, 2019

This PR is getting much closer. Thank you, @rcmdnk!

@sjackman

This comment has been minimized.

Copy link
Member

sjackman commented Feb 12, 2019

If you know how to squash a PR down to a single commit using git rebase -i origin/master, please do that. If you have no idea what I'm talking about, don't worry about it. I'll squash it when I merge it.

@rcmdnk rcmdnk force-pushed the rcmdnk:fix_sudo branch from 1644031 to e49bd2c Feb 12, 2019

@rcmdnk

This comment has been minimized.

Copy link
Author

rcmdnk commented Feb 12, 2019

Yes, these commits were squished.

@sjackman

This comment has been minimized.

Copy link
Member

sjackman commented Mar 6, 2019

@rcmdnk Are you interested in continuing with this PR?

@rcmdnk

This comment has been minimized.

Copy link
Author

rcmdnk commented Mar 6, 2019

Yes, it should be merged otherwise the script does not work in some of my nodes.

I squished commits in a single commit.
Do you need any other changes to merge?

@sjackman

This comment has been minimized.

Copy link
Member

sjackman commented Mar 6, 2019

Yes, there's a number of unresolved comments.
See https://github.com/Linuxbrew/install/pull/47/files

@rcmdnk rcmdnk force-pushed the rcmdnk:fix_sudo branch from e49bd2c to b11e9e4 Mar 6, 2019

@rcmdnk

This comment has been minimized.

Copy link
Author

rcmdnk commented Mar 6, 2019

Sorry, I missed these comments.
I updated a commit for them.

install Outdated
Kernel.system "/usr/bin/sudo", "-l", "mkdir"
@have_sudo = $CHILD_STATUS && $CHILD_STATUS.success? || false
rescue Interrupt
exit!

This comment has been minimized.

@sjackman

sjackman Mar 6, 2019

Member
Suggested change
exit!
exit

exit also prevents the traceback.

This comment has been minimized.

@rcmdnk

rcmdnk Mar 7, 2019

Author

As SystemExit trap does not exist, they work almost the same.
But maybe exit is better for the future update.

The difference here is just a default exit code (1 or 0).
I thought it should be an error exit, but you can decide later if necessary.

I changed it to exit.

This comment has been minimized.

@sjackman

sjackman Mar 7, 2019

Member

Good point. exit 1 is probably better.

Suggested change
exit!
exit 1
install Outdated
@have_sudo = $CHILD_STATUS && $CHILD_STATUS.success?
begin
Kernel.system "/usr/bin/sudo", "-l", "mkdir"
@have_sudo = $CHILD_STATUS && $CHILD_STATUS.success? || false

This comment has been minimized.

@sjackman

sjackman Mar 6, 2019

Member
Suggested change
@have_sudo = $CHILD_STATUS && $CHILD_STATUS.success? || false
@have_sudo = $CHILD_STATUS && $CHILD_STATUS.success?

nil for false is fine.

This comment has been minimized.

@sjackman

sjackman Mar 6, 2019

Member
Suggested change
@have_sudo = $CHILD_STATUS && $CHILD_STATUS.success? || false
@have_sudo = $CHILD_STATUS.success?

I was wrong here. $CHILD_STATUS is not nil when /usr/bin/sudo does not exist.

This comment has been minimized.

@rcmdnk

rcmdnk Mar 7, 2019

Author

Maybe I also did a similar misunderstanding that I observed nil before adding `require "English" in the test script...
The last one is enough, I agree.

I put || false because if @have_sudo is nil, sudo? cannot pass the first line always.

Anyways, I updated to just $CHILD_STATUS.success?.

This comment has been minimized.

@sjackman

sjackman Mar 7, 2019

Member

Maybe I also did a similar misunderstanding that I observed nil before adding `require "English" in the test script...

I think I made the identical error.

I put || false because if @have_sudo is nil, sudo? cannot pass the first line always.

Yes, good point. I missed that.

Fixed sudo checks
Use mkdir to check sudo in install-ruby.
Use mkdir to check sudo in install, for other than initial validation check.
Ask password only when sudo failed by password requirement.
Trap Ctrl-C at sudo.

@rcmdnk rcmdnk force-pushed the rcmdnk:fix_sudo branch from b11e9e4 to c3fce3e Mar 7, 2019

Kernel.system "/usr/bin/sudo", "-l", "mkdir"
@have_sudo = $CHILD_STATUS.success?
rescue Interrupt
exit

This comment has been minimized.

@sjackman

sjackman Mar 7, 2019

Member
Suggested change
exit
exit 1

@sjackman sjackman merged commit 988d295 into Linuxbrew:master Mar 7, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sjackman

This comment has been minimized.

Copy link
Member

sjackman commented Mar 7, 2019

Thank you very much for these improvements, @rcmdnk ! And thank you for your first contribution to Linuxbrew/install! 🏅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.