From 563387a7b42ce16d6280be469a08fe7f0b87bfe1 Mon Sep 17 00:00:00 2001 From: Ilya Kulakov Date: Tue, 14 Feb 2023 13:24:36 -0800 Subject: [PATCH 1/4] sudo: explicitly specify the root user where necessary With sudoers one may override default sudo user. This mostly works provided the admin configured the replacement appropriately. However there are exceptions that absolutely must be run by root such as /usr/sbin/installer and, under certain circumstances, /bin/launchctl. --- .../cask/artifact/abstract_uninstall.rb | 20 ++++++++++--------- Library/Homebrew/cask/artifact/pkg.rb | 2 +- Library/Homebrew/cask/pkg.rb | 8 ++++---- Library/Homebrew/system_command.rb | 12 ++++++++--- 4 files changed, 25 insertions(+), 17 deletions(-) diff --git a/Library/Homebrew/cask/artifact/abstract_uninstall.rb b/Library/Homebrew/cask/artifact/abstract_uninstall.rb index d1c56a303d..6528b5ddc8 100644 --- a/Library/Homebrew/cask/artifact/abstract_uninstall.rb +++ b/Library/Homebrew/cask/artifact/abstract_uninstall.rb @@ -106,13 +106,14 @@ module Cask all_services.each do |service| ohai "Removing launchctl service #{service}" booleans.each do |with_sudo| + sudo_user = with_sudo ? "root" : "" plist_status = command.run( "/bin/launchctl", args: ["list", service], - sudo: with_sudo, print_stderr: false + sudo: sudo_user, print_stderr: false ).stdout if plist_status.start_with?("{") - command.run!("/bin/launchctl", args: ["remove", service], sudo: with_sudo) + command.run!("/bin/launchctl", args: ["remove", service], sudo: sudo_user) sleep 1 end paths = [ @@ -122,13 +123,13 @@ module Cask paths.each { |elt| elt.prepend(Dir.home).freeze } unless with_sudo paths = paths.map { |elt| Pathname(elt) }.select(&:exist?) paths.each do |path| - command.run!("/bin/rm", args: ["-f", "--", path], sudo: with_sudo) + command.run!("/bin/rm", args: ["-f", "--", path], sudo: sudo_user) end # undocumented and untested: pass a path to uninstall :launchctl next unless Pathname(service).exist? - command.run!("/bin/launchctl", args: ["unload", "-w", "--", service], sudo: with_sudo) - command.run!("/bin/rm", args: ["-f", "--", service], sudo: with_sudo) + command.run!("/bin/launchctl", args: ["unload", "-w", "--", service], sudo: sudo_user) + command.run!("/bin/rm", args: ["-f", "--", service], sudo: sudo_user) sleep 1 end end @@ -301,14 +302,15 @@ module Cask def uninstall_kext(*kexts, command: nil, **_) kexts.each do |kext| ohai "Unloading kernel extension #{kext}" - is_loaded = system_command!("/usr/sbin/kextstat", args: ["-l", "-b", kext], sudo: true).stdout + is_loaded = system_command!("/usr/sbin/kextstat", args: ["-l", "-b", kext], sudo: "root").stdout if is_loaded.length > 1 - system_command!("/sbin/kextunload", args: ["-b", kext], sudo: true) + system_command!("/sbin/kextunload", args: ["-b", kext], sudo: "root") sleep 1 end - system_command!("/usr/sbin/kextfind", args: ["-b", kext], sudo: true).stdout.chomp.lines.each do |kext_path| + kexts = system_command!("/usr/sbin/kextfind", args: ["-b", kext], sudo: "root").stdout + kexts.chomp.lines.each do |kext_path| ohai "Removing kernel extension #{kext_path}" - system_command!("/bin/rm", args: ["-rf", kext_path], sudo: true) + system_command!("/bin/rm", args: ["-rf", kext_path], sudo: "root") end end end diff --git a/Library/Homebrew/cask/artifact/pkg.rb b/Library/Homebrew/cask/artifact/pkg.rb index 7a22057909..7002257be8 100644 --- a/Library/Homebrew/cask/artifact/pkg.rb +++ b/Library/Homebrew/cask/artifact/pkg.rb @@ -62,7 +62,7 @@ module Cask "USER" => User.current, "USERNAME" => User.current, } - command.run!("/usr/sbin/installer", sudo: true, args: args, print_stdout: true, env: env) + command.run!("/usr/sbin/installer", sudo: "root", args: args, print_stdout: true, env: env) end end diff --git a/Library/Homebrew/cask/pkg.rb b/Library/Homebrew/cask/pkg.rb index cc3d4d06cb..15258a420f 100644 --- a/Library/Homebrew/cask/pkg.rb +++ b/Library/Homebrew/cask/pkg.rb @@ -32,7 +32,7 @@ module Cask "/usr/bin/xargs", args: ["-0", "--", "/bin/rm", "--"], input: pkgutil_bom_files.join("\0"), - sudo: true, + sudo: "root", ) end @@ -42,7 +42,7 @@ module Cask "/usr/bin/xargs", args: ["-0", "--", "/bin/rm", "--"], input: pkgutil_bom_specials.join("\0"), - sudo: true, + sudo: "root", ) end @@ -59,7 +59,7 @@ module Cask sig { void } def forget odebug "Unregistering pkg receipt (aka forgetting)" - @command.run!("/usr/sbin/pkgutil", args: ["--forget", package_id], sudo: true) + @command.run!("/usr/sbin/pkgutil", args: ["--forget", package_id], sudo: "root") end sig { returns(T::Array[Pathname]) } @@ -114,7 +114,7 @@ module Cask "/usr/bin/xargs", args: ["-0", "--", RMDIR_SH.to_s], input: Array(path).join("\0"), - sudo: true, + sudo: "root", ) end diff --git a/Library/Homebrew/system_command.rb b/Library/Homebrew/system_command.rb index 9e7e98fabe..f9d843561a 100644 --- a/Library/Homebrew/system_command.rb +++ b/Library/Homebrew/system_command.rb @@ -64,7 +64,7 @@ class SystemCommand params( executable: T.any(String, Pathname), args: T::Array[T.any(String, Integer, Float, URI::Generic)], - sudo: T::Boolean, + sudo: T.any(T::Boolean, String), env: T::Hash[String, String], input: T.any(String, T::Array[String]), must_succeed: T::Boolean, @@ -122,7 +122,12 @@ class SystemCommand attr_reader :executable, :args, :input, :chdir, :env - attr_predicate :sudo?, :print_stdout?, :print_stderr?, :must_succeed? + attr_predicate :print_stdout?, :print_stderr?, :must_succeed? + + sig { returns(T::Boolean) } + def sudo? + @sudo != false && @sudo != "" + end sig { returns(T::Boolean) } def debug? @@ -153,8 +158,9 @@ class SystemCommand sig { returns(T::Array[String]) } def sudo_prefix + user_flags = @sudo.is_a?(String) ? ["-u", @sudo] : [] askpass_flags = ENV.key?("SUDO_ASKPASS") ? ["-A"] : [] - ["/usr/bin/sudo", *askpass_flags, "-E", *env_args, "--"] + ["/usr/bin/sudo", *user_flags, *askpass_flags, "-E", *env_args, "--"] end sig { returns(T::Array[String]) } From d470661b37aa790d46a0a3250061b158ae11e956 Mon Sep 17 00:00:00 2001 From: Ilya Kulakov Date: Fri, 17 Feb 2023 14:48:13 -0800 Subject: [PATCH 2/4] sudo: add the sudo_user arg to SystemCommand. --- .../cask/artifact/abstract_uninstall.rb | 52 +++++++++++++++---- Library/Homebrew/cask/artifact/pkg.rb | 2 +- Library/Homebrew/cask/pkg.rb | 23 ++++---- Library/Homebrew/system_command.rb | 17 +++--- 4 files changed, 65 insertions(+), 29 deletions(-) diff --git a/Library/Homebrew/cask/artifact/abstract_uninstall.rb b/Library/Homebrew/cask/artifact/abstract_uninstall.rb index 6528b5ddc8..97b6e1cd99 100644 --- a/Library/Homebrew/cask/artifact/abstract_uninstall.rb +++ b/Library/Homebrew/cask/artifact/abstract_uninstall.rb @@ -106,11 +106,13 @@ module Cask all_services.each do |service| ohai "Removing launchctl service #{service}" booleans.each do |with_sudo| - sudo_user = with_sudo ? "root" : "" + sudo_user = with_sudo ? "root" : nil plist_status = command.run( "/bin/launchctl", - args: ["list", service], - sudo: sudo_user, print_stderr: false + args: ["list", service], + sudo: with_sudo, + sudo_user: sudo_user, + print_stderr: false, ).stdout if plist_status.start_with?("{") command.run!("/bin/launchctl", args: ["remove", service], sudo: sudo_user) @@ -123,13 +125,23 @@ module Cask paths.each { |elt| elt.prepend(Dir.home).freeze } unless with_sudo paths = paths.map { |elt| Pathname(elt) }.select(&:exist?) paths.each do |path| - command.run!("/bin/rm", args: ["-f", "--", path], sudo: sudo_user) + command.run!("/bin/rm", args: ["-f", "--", path], sudo: with_sudo, sudo_user: sudo_user) end # undocumented and untested: pass a path to uninstall :launchctl next unless Pathname(service).exist? - command.run!("/bin/launchctl", args: ["unload", "-w", "--", service], sudo: sudo_user) - command.run!("/bin/rm", args: ["-f", "--", service], sudo: sudo_user) + command.run!( + "/bin/launchctl", + args: ["unload", "-w", "--", service], + sudo: with_sudo, + sudo_user: sudo_user, + ) + command.run!( + "/bin/rm", + args: ["-f", "--", service], + sudo: with_sudo, + sudo_user: sudo_user, + ) sleep 1 end end @@ -302,15 +314,35 @@ module Cask def uninstall_kext(*kexts, command: nil, **_) kexts.each do |kext| ohai "Unloading kernel extension #{kext}" - is_loaded = system_command!("/usr/sbin/kextstat", args: ["-l", "-b", kext], sudo: "root").stdout + is_loaded = system_command!( + "/usr/sbin/kextstat", + args: ["-l", "-b", kext], + sudo: true, + sudo_user: "root", + ).stdout if is_loaded.length > 1 - system_command!("/sbin/kextunload", args: ["-b", kext], sudo: "root") + system_command!( + "/sbin/kextunload", + args: ["-b", kext], + sudo: true, + sudo_user: "root", + ) sleep 1 end - kexts = system_command!("/usr/sbin/kextfind", args: ["-b", kext], sudo: "root").stdout + kexts = system_command!( + "/usr/sbin/kextfind", + args: ["-b", kext], + sudo: true, + sudo_user: "root", + ).stdout kexts.chomp.lines.each do |kext_path| ohai "Removing kernel extension #{kext_path}" - system_command!("/bin/rm", args: ["-rf", kext_path], sudo: "root") + system_command!( + "/bin/rm", + args: ["-rf", kext_path], + sudo: true, + sudo_user: "root", + ) end end end diff --git a/Library/Homebrew/cask/artifact/pkg.rb b/Library/Homebrew/cask/artifact/pkg.rb index 7002257be8..b8a000a5b8 100644 --- a/Library/Homebrew/cask/artifact/pkg.rb +++ b/Library/Homebrew/cask/artifact/pkg.rb @@ -62,7 +62,7 @@ module Cask "USER" => User.current, "USERNAME" => User.current, } - command.run!("/usr/sbin/installer", sudo: "root", args: args, print_stdout: true, env: env) + command.run!("/usr/sbin/installer", sudo: true, sudo_user: "root", args: args, print_stdout: true, env: env) end end diff --git a/Library/Homebrew/cask/pkg.rb b/Library/Homebrew/cask/pkg.rb index 15258a420f..6cbda0c9d3 100644 --- a/Library/Homebrew/cask/pkg.rb +++ b/Library/Homebrew/cask/pkg.rb @@ -30,9 +30,10 @@ module Cask odebug "Deleting pkg files" @command.run!( "/usr/bin/xargs", - args: ["-0", "--", "/bin/rm", "--"], - input: pkgutil_bom_files.join("\0"), - sudo: "root", + args: ["-0", "--", "/bin/rm", "--"], + input: pkgutil_bom_files.join("\0"), + sudo: true, + sudo_user: "root", ) end @@ -40,9 +41,10 @@ module Cask odebug "Deleting pkg symlinks and special files" @command.run!( "/usr/bin/xargs", - args: ["-0", "--", "/bin/rm", "--"], - input: pkgutil_bom_specials.join("\0"), - sudo: "root", + args: ["-0", "--", "/bin/rm", "--"], + input: pkgutil_bom_specials.join("\0"), + sudo: true, + sudo_user: "root", ) end @@ -59,7 +61,7 @@ module Cask sig { void } def forget odebug "Unregistering pkg receipt (aka forgetting)" - @command.run!("/usr/sbin/pkgutil", args: ["--forget", package_id], sudo: "root") + @command.run!("/usr/sbin/pkgutil", args: ["--forget", package_id], sudo: true, sudo_user: "root") end sig { returns(T::Array[Pathname]) } @@ -112,9 +114,10 @@ module Cask def rmdir(path) @command.run!( "/usr/bin/xargs", - args: ["-0", "--", RMDIR_SH.to_s], - input: Array(path).join("\0"), - sudo: "root", + args: ["-0", "--", RMDIR_SH.to_s], + input: Array(path).join("\0"), + sudo: true, + sudo_user: "root", ) end diff --git a/Library/Homebrew/system_command.rb b/Library/Homebrew/system_command.rb index f9d843561a..a285e6663e 100644 --- a/Library/Homebrew/system_command.rb +++ b/Library/Homebrew/system_command.rb @@ -64,7 +64,8 @@ class SystemCommand params( executable: T.any(String, Pathname), args: T::Array[T.any(String, Integer, Float, URI::Generic)], - sudo: T.any(T::Boolean, String), + sudo: T::Boolean, + sudo_user: T.nilable(String), env: T::Hash[String, String], input: T.any(String, T::Array[String]), must_succeed: T::Boolean, @@ -81,6 +82,7 @@ class SystemCommand executable, args: [], sudo: false, + sudo_user: nil, env: {}, input: [], must_succeed: false, @@ -95,7 +97,11 @@ class SystemCommand require "extend/ENV" @executable = executable @args = args + + raise ArgumentError, "sudo_user cannot be set if sudo is false" if !sudo && !sudo_user.nil? + @sudo = sudo + @sudo_user = sudo_user env.each_key do |name| next if /^[\w&&\D]\w*$/.match?(name) @@ -122,12 +128,7 @@ class SystemCommand attr_reader :executable, :args, :input, :chdir, :env - attr_predicate :print_stdout?, :print_stderr?, :must_succeed? - - sig { returns(T::Boolean) } - def sudo? - @sudo != false && @sudo != "" - end + attr_predicate :sudo?, :print_stdout?, :print_stderr?, :must_succeed? sig { returns(T::Boolean) } def debug? @@ -158,7 +159,7 @@ class SystemCommand sig { returns(T::Array[String]) } def sudo_prefix - user_flags = @sudo.is_a?(String) ? ["-u", @sudo] : [] + user_flags = @sudo_user.nil? ? [] : ["-u", @sudo_user] askpass_flags = ENV.key?("SUDO_ASKPASS") ? ["-A"] : [] ["/usr/bin/sudo", *user_flags, *askpass_flags, "-E", *env_args, "--"] end From 476d97934f4f162f191a8efdddfd2744ab7e75da Mon Sep 17 00:00:00 2001 From: Ilya Kulakov Date: Mon, 27 Mar 2023 14:06:07 -0700 Subject: [PATCH 3/4] sudo: change sudo_user to sudo_as_root. --- .../cask/artifact/abstract_uninstall.rb | 60 ++++++++++--------- .../Homebrew/cask/artifact/keyboard_layout.rb | 7 ++- Library/Homebrew/cask/artifact/pkg.rb | 9 ++- Library/Homebrew/cask/pkg.rb | 31 ++++++---- Library/Homebrew/sorbet/rbi/parlour.rbi | 3 + Library/Homebrew/system_command.rb | 13 ++-- 6 files changed, 74 insertions(+), 49 deletions(-) diff --git a/Library/Homebrew/cask/artifact/abstract_uninstall.rb b/Library/Homebrew/cask/artifact/abstract_uninstall.rb index 97b6e1cd99..c23be839d5 100644 --- a/Library/Homebrew/cask/artifact/abstract_uninstall.rb +++ b/Library/Homebrew/cask/artifact/abstract_uninstall.rb @@ -105,42 +105,46 @@ module Cask all_services.each do |service| ohai "Removing launchctl service #{service}" - booleans.each do |with_sudo| - sudo_user = with_sudo ? "root" : nil + booleans.each do |sudo| plist_status = command.run( "/bin/launchctl", args: ["list", service], - sudo: with_sudo, - sudo_user: sudo_user, + sudo: sudo, + sudo_as_root: sudo, print_stderr: false, ).stdout if plist_status.start_with?("{") - command.run!("/bin/launchctl", args: ["remove", service], sudo: sudo_user) + command.run!( + "/bin/launchctl", + args: ["remove", service], + sudo: sudo, + sudo_as_root: sudo, + ) sleep 1 end paths = [ +"/Library/LaunchAgents/#{service}.plist", +"/Library/LaunchDaemons/#{service}.plist", ] - paths.each { |elt| elt.prepend(Dir.home).freeze } unless with_sudo + paths.each { |elt| elt.prepend(Dir.home).freeze } unless sudo paths = paths.map { |elt| Pathname(elt) }.select(&:exist?) paths.each do |path| - command.run!("/bin/rm", args: ["-f", "--", path], sudo: with_sudo, sudo_user: sudo_user) + command.run!("/bin/rm", args: ["-f", "--", path], sudo: sudo, sudo_as_root: sudo) end # undocumented and untested: pass a path to uninstall :launchctl next unless Pathname(service).exist? command.run!( "/bin/launchctl", - args: ["unload", "-w", "--", service], - sudo: with_sudo, - sudo_user: sudo_user, + args: ["unload", "-w", "--", service], + sudo: sudo, + sudo_as_root: sudo, ) command.run!( "/bin/rm", - args: ["-f", "--", service], - sudo: with_sudo, - sudo_user: sudo_user, + args: ["-f", "--", service], + sudo: sudo, + sudo_as_root: sudo, ) sleep 1 end @@ -316,32 +320,32 @@ module Cask ohai "Unloading kernel extension #{kext}" is_loaded = system_command!( "/usr/sbin/kextstat", - args: ["-l", "-b", kext], - sudo: true, - sudo_user: "root", + args: ["-l", "-b", kext], + sudo: true, + sudo_as_root: true, ).stdout if is_loaded.length > 1 system_command!( "/sbin/kextunload", - args: ["-b", kext], - sudo: true, - sudo_user: "root", + args: ["-b", kext], + sudo: true, + sudo_as_root: true, ) sleep 1 end - kexts = system_command!( + found_kexts = system_command!( "/usr/sbin/kextfind", - args: ["-b", kext], - sudo: true, - sudo_user: "root", - ).stdout - kexts.chomp.lines.each do |kext_path| + args: ["-b", kext], + sudo: true, + sudo_as_root: true, + ).stdout.chomp.lines + found_kexts.each do |kext_path| ohai "Removing kernel extension #{kext_path}" system_command!( "/bin/rm", - args: ["-rf", kext_path], - sudo: true, - sudo_user: "root", + args: ["-rf", kext_path], + sudo: true, + sudo_as_root: true, ) end end diff --git a/Library/Homebrew/cask/artifact/keyboard_layout.rb b/Library/Homebrew/cask/artifact/keyboard_layout.rb index c7d549be00..608279d2e2 100644 --- a/Library/Homebrew/cask/artifact/keyboard_layout.rb +++ b/Library/Homebrew/cask/artifact/keyboard_layout.rb @@ -22,7 +22,12 @@ module Cask private def delete_keyboard_layout_cache(command: nil, **_) - command.run!("/bin/rm", args: ["-f", "--", "/System/Library/Caches/com.apple.IntlDataCache.le*"], sudo: true) + command.run!( + "/bin/rm", + args: ["-f", "--", "/System/Library/Caches/com.apple.IntlDataCache.le*"], + sudo: true, + sudo_as_root: true, + ) end end end diff --git a/Library/Homebrew/cask/artifact/pkg.rb b/Library/Homebrew/cask/artifact/pkg.rb index b8a000a5b8..61cd142ec0 100644 --- a/Library/Homebrew/cask/artifact/pkg.rb +++ b/Library/Homebrew/cask/artifact/pkg.rb @@ -62,7 +62,14 @@ module Cask "USER" => User.current, "USERNAME" => User.current, } - command.run!("/usr/sbin/installer", sudo: true, sudo_user: "root", args: args, print_stdout: true, env: env) + command.run!( + "/usr/sbin/installer", + sudo: true, + sudo_as_root: true, + args: args, + print_stdout: true, + env: env, + ) end end diff --git a/Library/Homebrew/cask/pkg.rb b/Library/Homebrew/cask/pkg.rb index 6cbda0c9d3..a71e0eb17b 100644 --- a/Library/Homebrew/cask/pkg.rb +++ b/Library/Homebrew/cask/pkg.rb @@ -30,10 +30,10 @@ module Cask odebug "Deleting pkg files" @command.run!( "/usr/bin/xargs", - args: ["-0", "--", "/bin/rm", "--"], - input: pkgutil_bom_files.join("\0"), - sudo: true, - sudo_user: "root", + args: ["-0", "--", "/bin/rm", "--"], + input: pkgutil_bom_files.join("\0"), + sudo: true, + sudo_as_root: true, ) end @@ -41,10 +41,10 @@ module Cask odebug "Deleting pkg symlinks and special files" @command.run!( "/usr/bin/xargs", - args: ["-0", "--", "/bin/rm", "--"], - input: pkgutil_bom_specials.join("\0"), - sudo: true, - sudo_user: "root", + args: ["-0", "--", "/bin/rm", "--"], + input: pkgutil_bom_specials.join("\0"), + sudo: true, + sudo_as_root: true, ) end @@ -61,7 +61,12 @@ module Cask sig { void } def forget odebug "Unregistering pkg receipt (aka forgetting)" - @command.run!("/usr/sbin/pkgutil", args: ["--forget", package_id], sudo: true, sudo_user: "root") + @command.run!( + "/usr/sbin/pkgutil", + args: ["--forget", package_id], + sudo: true, + sudo_as_root: true, + ) end sig { returns(T::Array[Pathname]) } @@ -114,10 +119,10 @@ module Cask def rmdir(path) @command.run!( "/usr/bin/xargs", - args: ["-0", "--", RMDIR_SH.to_s], - input: Array(path).join("\0"), - sudo: true, - sudo_user: "root", + args: ["-0", "--", RMDIR_SH.to_s], + input: Array(path).join("\0"), + sudo: true, + sudo_as_root: true, ) end diff --git a/Library/Homebrew/sorbet/rbi/parlour.rbi b/Library/Homebrew/sorbet/rbi/parlour.rbi index 803e17a5cb..d6bf8bb460 100644 --- a/Library/Homebrew/sorbet/rbi/parlour.rbi +++ b/Library/Homebrew/sorbet/rbi/parlour.rbi @@ -107,6 +107,9 @@ class SystemCommand sig { returns(T::Boolean) } def sudo?; end + sig { returns(T::Boolean) } + def sudo_as_root?; end + sig { returns(T::Boolean) } def print_stdout?; end diff --git a/Library/Homebrew/system_command.rb b/Library/Homebrew/system_command.rb index a285e6663e..2a2d7bd6b6 100644 --- a/Library/Homebrew/system_command.rb +++ b/Library/Homebrew/system_command.rb @@ -65,7 +65,7 @@ class SystemCommand executable: T.any(String, Pathname), args: T::Array[T.any(String, Integer, Float, URI::Generic)], sudo: T::Boolean, - sudo_user: T.nilable(String), + sudo_as_root: T::Boolean, env: T::Hash[String, String], input: T.any(String, T::Array[String]), must_succeed: T::Boolean, @@ -82,7 +82,7 @@ class SystemCommand executable, args: [], sudo: false, - sudo_user: nil, + sudo_as_root: false, env: {}, input: [], must_succeed: false, @@ -98,10 +98,10 @@ class SystemCommand @executable = executable @args = args - raise ArgumentError, "sudo_user cannot be set if sudo is false" if !sudo && !sudo_user.nil? + raise ArgumentError, "sudo_as_root cannot be set if sudo is false" if !sudo && sudo_as_root @sudo = sudo - @sudo_user = sudo_user + @sudo_as_root = sudo_as_root env.each_key do |name| next if /^[\w&&\D]\w*$/.match?(name) @@ -128,7 +128,7 @@ class SystemCommand attr_reader :executable, :args, :input, :chdir, :env - attr_predicate :sudo?, :print_stdout?, :print_stderr?, :must_succeed? + attr_predicate :sudo?, :sudo_as_root?, :print_stdout?, :print_stderr?, :must_succeed? sig { returns(T::Boolean) } def debug? @@ -159,7 +159,8 @@ class SystemCommand sig { returns(T::Array[String]) } def sudo_prefix - user_flags = @sudo_user.nil? ? [] : ["-u", @sudo_user] + user_flags = [] + user_flags += ["-u", "root"] if sudo_as_root? askpass_flags = ENV.key?("SUDO_ASKPASS") ? ["-A"] : [] ["/usr/bin/sudo", *user_flags, *askpass_flags, "-E", *env_args, "--"] end From 6a4322833fd2a3fe31cbe829b4a3f04ec6d70f19 Mon Sep 17 00:00:00 2001 From: Ilya Kulakov Date: Wed, 26 Apr 2023 19:51:26 -0700 Subject: [PATCH 4/4] sudo: Fix tests. --- .../Homebrew/test/cask/artifact/pkg_spec.rb | 2 + .../artifact/shared_examples/uninstall_zap.rb | 77 ++++++++++++++----- .../test/cask/dsl/shared_examples/staged.rb | 12 ++- Library/Homebrew/test/cask/pkg_spec.rb | 12 +-- .../cask/Casks/with-uninstall-script-app.rb | 5 +- .../with-uninstall-script-user-relative.rb | 5 +- .../helper/cask/never_sudo_system_command.rb | 2 +- Library/Homebrew/test/system_command_spec.rb | 26 ++++++- 8 files changed, 108 insertions(+), 33 deletions(-) diff --git a/Library/Homebrew/test/cask/artifact/pkg_spec.rb b/Library/Homebrew/test/cask/artifact/pkg_spec.rb index 5a617fe2dd..6c015613b9 100644 --- a/Library/Homebrew/test/cask/artifact/pkg_spec.rb +++ b/Library/Homebrew/test/cask/artifact/pkg_spec.rb @@ -16,6 +16,7 @@ describe Cask::Artifact::Pkg, :cask do "/usr/sbin/installer", args: ["-pkg", cask.staged_path.join("MyFancyPkg", "Fancy.pkg"), "-target", "/"], sudo: true, + sudo_as_root: true, print_stdout: true, env: { "LOGNAME" => ENV.fetch("USER"), @@ -65,6 +66,7 @@ describe Cask::Artifact::Pkg, :cask do cask.staged_path.join("/tmp/choices.xml") ], sudo: true, + sudo_as_root: true, print_stdout: true, env: { "LOGNAME" => ENV.fetch("USER"), diff --git a/Library/Homebrew/test/cask/artifact/shared_examples/uninstall_zap.rb b/Library/Homebrew/test/cask/artifact/shared_examples/uninstall_zap.rb index 77a4ec099c..ec195718f0 100644 --- a/Library/Homebrew/test/cask/artifact/shared_examples/uninstall_zap.rb +++ b/Library/Homebrew/test/cask/artifact/shared_examples/uninstall_zap.rb @@ -31,14 +31,26 @@ shared_examples "#uninstall_phase or #zap_phase" do it "works when job is owned by user" do allow(fake_system_command).to receive(:run) - .with("/bin/launchctl", args: ["list", "my.fancy.package.service"], print_stderr: false, sudo: false) + .with( + "/bin/launchctl", + args: ["list", "my.fancy.package.service"], + print_stderr: false, + sudo: false, + sudo_as_root: false, + ) .and_return(instance_double(SystemCommand::Result, stdout: service_info)) allow(fake_system_command).to receive(:run) - .with("/bin/launchctl", args: ["list", "my.fancy.package.service"], print_stderr: false, sudo: true) + .with( + "/bin/launchctl", + args: ["list", "my.fancy.package.service"], + print_stderr: false, + sudo: true, + sudo_as_root: true, + ) .and_return(instance_double(SystemCommand::Result, stdout: unknown_response)) expect(fake_system_command).to receive(:run!) - .with("/bin/launchctl", args: ["remove", "my.fancy.package.service"], sudo: false) + .with("/bin/launchctl", args: ["remove", "my.fancy.package.service"], sudo: false, sudo_as_root: false) .and_return(instance_double(SystemCommand::Result)) subject.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command) @@ -46,14 +58,26 @@ shared_examples "#uninstall_phase or #zap_phase" do it "works when job is owned by system" do allow(fake_system_command).to receive(:run) - .with("/bin/launchctl", args: ["list", "my.fancy.package.service"], print_stderr: false, sudo: false) + .with( + "/bin/launchctl", + args: ["list", "my.fancy.package.service"], + print_stderr: false, + sudo: false, + sudo_as_root: false, + ) .and_return(instance_double(SystemCommand::Result, stdout: unknown_response)) allow(fake_system_command).to receive(:run) - .with("/bin/launchctl", args: ["list", "my.fancy.package.service"], print_stderr: false, sudo: true) + .with( + "/bin/launchctl", + args: ["list", "my.fancy.package.service"], + print_stderr: false, + sudo: true, + sudo_as_root: true, + ) .and_return(instance_double(SystemCommand::Result, stdout: service_info)) expect(fake_system_command).to receive(:run!) - .with("/bin/launchctl", args: ["remove", "my.fancy.package.service"], sudo: true) + .with("/bin/launchctl", args: ["remove", "my.fancy.package.service"], sudo: true, sudo_as_root: true) .and_return(instance_double(SystemCommand::Result)) subject.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command) @@ -94,14 +118,26 @@ shared_examples "#uninstall_phase or #zap_phase" do .and_return(["my.fancy.package.service.12345"]) allow(fake_system_command).to receive(:run) - .with("/bin/launchctl", args: ["list", "my.fancy.package.service.12345"], print_stderr: false, sudo: false) + .with( + "/bin/launchctl", + args: ["list", "my.fancy.package.service.12345"], + print_stderr: false, + sudo: false, + sudo_as_root: false, + ) .and_return(instance_double(SystemCommand::Result, stdout: unknown_response)) allow(fake_system_command).to receive(:run) - .with("/bin/launchctl", args: ["list", "my.fancy.package.service.12345"], print_stderr: false, sudo: true) + .with( + "/bin/launchctl", + args: ["list", "my.fancy.package.service.12345"], + print_stderr: false, + sudo: true, + sudo_as_root: true, + ) .and_return(instance_double(SystemCommand::Result, stdout: service_info)) expect(fake_system_command).to receive(:run!) - .with("/bin/launchctl", args: ["remove", "my.fancy.package.service.12345"], sudo: true) + .with("/bin/launchctl", args: ["remove", "my.fancy.package.service.12345"], sudo: true, sudo_as_root: true) .and_return(instance_double(SystemCommand::Result)) subject.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command) @@ -148,19 +184,19 @@ shared_examples "#uninstall_phase or #zap_phase" do it "is supported" do allow(subject).to receive(:system_command!) - .with("/usr/sbin/kextstat", args: ["-l", "-b", kext_id], sudo: true) + .with("/usr/sbin/kextstat", args: ["-l", "-b", kext_id], sudo: true, sudo_as_root: true) .and_return(instance_double("SystemCommand::Result", stdout: "loaded")) expect(subject).to receive(:system_command!) - .with("/sbin/kextunload", args: ["-b", kext_id], sudo: true) + .with("/sbin/kextunload", args: ["-b", kext_id], sudo: true, sudo_as_root: true) .and_return(instance_double("SystemCommand::Result")) expect(subject).to receive(:system_command!) - .with("/usr/sbin/kextfind", args: ["-b", kext_id], sudo: true) + .with("/usr/sbin/kextfind", args: ["-b", kext_id], sudo: true, sudo_as_root: true) .and_return(instance_double("SystemCommand::Result", stdout: "/Library/Extensions/FancyPackage.kext\n")) expect(subject).to receive(:system_command!) - .with("/bin/rm", args: ["-rf", "/Library/Extensions/FancyPackage.kext"], sudo: true) + .with("/bin/rm", args: ["-rf", "/Library/Extensions/FancyPackage.kext"], sudo: true, sudo_as_root: true) subject.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command) end @@ -281,13 +317,14 @@ shared_examples "#uninstall_phase or #zap_phase" do it "is supported" do allow(fake_system_command).to receive(:run).with(any_args).and_call_original - expect(fake_system_command).to receive(:run).with( - cask.staged_path.join("MyFancyPkg", "FancyUninstaller.tool"), - args: ["--please"], - must_succeed: true, - print_stdout: true, - sudo: false, - ) + expect(fake_system_command).to receive(:run) + .with( + cask.staged_path.join("MyFancyPkg", "FancyUninstaller.tool"), + args: ["--please"], + must_succeed: true, + print_stdout: true, + sudo: false, + ) InstallHelper.install_without_artifacts(cask) subject.public_send(:"#{artifact_dsl_key}_phase", command: fake_system_command) diff --git a/Library/Homebrew/test/cask/dsl/shared_examples/staged.rb b/Library/Homebrew/test/cask/dsl/shared_examples/staged.rb index db8a145522..07fe99b05d 100644 --- a/Library/Homebrew/test/cask/dsl/shared_examples/staged.rb +++ b/Library/Homebrew/test/cask/dsl/shared_examples/staged.rb @@ -67,7 +67,11 @@ shared_examples Cask::Staged do allow(staged).to receive(:Pathname).and_return(fake_pathname) expect(fake_system_command).to receive(:run!) - .with("/usr/sbin/chown", args: ["-R", "--", "fake_user:staff", fake_pathname, fake_pathname], sudo: true) + .with( + "/usr/sbin/chown", + args: ["-R", "--", "fake_user:staff", fake_pathname, fake_pathname], + sudo: true, + ) staged.set_ownership([fake_pathname.to_s, fake_pathname.to_s]) end @@ -78,7 +82,11 @@ shared_examples Cask::Staged do allow(staged).to receive(:Pathname).and_return(fake_pathname) expect(fake_system_command).to receive(:run!) - .with("/usr/sbin/chown", args: ["-R", "--", "other_user:other_group", fake_pathname], sudo: true) + .with( + "/usr/sbin/chown", + args: ["-R", "--", "other_user:other_group", fake_pathname], + sudo: true, + ) staged.set_ownership(fake_pathname.to_s, user: "other_user", group: "other_group") end diff --git a/Library/Homebrew/test/cask/pkg_spec.rb b/Library/Homebrew/test/cask/pkg_spec.rb index ba60408a43..4476b80ed7 100644 --- a/Library/Homebrew/test/cask/pkg_spec.rb +++ b/Library/Homebrew/test/cask/pkg_spec.rb @@ -63,8 +63,9 @@ describe Cask::Pkg, :cask do expect(fake_system_command).to receive(:run!).with( "/usr/sbin/pkgutil", - args: ["--forget", "my.fake.pkg"], - sudo: true, + args: ["--forget", "my.fake.pkg"], + sudo: true, + sudo_as_root: true, ) pkg.uninstall @@ -114,9 +115,10 @@ describe Cask::Pkg, :cask do allow(fake_system_command).to receive(:run!).and_call_original expect(fake_system_command).to receive(:run!).with( "/usr/bin/xargs", - args: ["-0", "--", a_string_including("rmdir")], - input: [fake_dir].join("\0"), - sudo: true, + args: ["-0", "--", a_string_including("rmdir")], + input: [fake_dir].join("\0"), + sudo: true, + sudo_as_root: true, ).and_return(instance_double(SystemCommand::Result, stdout: "")) pkg.uninstall diff --git a/Library/Homebrew/test/support/fixtures/cask/Casks/with-uninstall-script-app.rb b/Library/Homebrew/test/support/fixtures/cask/Casks/with-uninstall-script-app.rb index 22735487d2..70ec4b2252 100644 --- a/Library/Homebrew/test/support/fixtures/cask/Casks/with-uninstall-script-app.rb +++ b/Library/Homebrew/test/support/fixtures/cask/Casks/with-uninstall-script-app.rb @@ -15,7 +15,8 @@ cask "with-uninstall-script-app" do end uninstall script: { - executable: "#{appdir}/MyFancyApp.app/uninstall.sh", - sudo: false, + executable: "#{appdir}/MyFancyApp.app/uninstall.sh", + sudo: false, + sudo_as_root: false, } end diff --git a/Library/Homebrew/test/support/fixtures/cask/Casks/with-uninstall-script-user-relative.rb b/Library/Homebrew/test/support/fixtures/cask/Casks/with-uninstall-script-user-relative.rb index a1f97c3108..4ad95b53f8 100644 --- a/Library/Homebrew/test/support/fixtures/cask/Casks/with-uninstall-script-user-relative.rb +++ b/Library/Homebrew/test/support/fixtures/cask/Casks/with-uninstall-script-user-relative.rb @@ -15,7 +15,8 @@ cask "with-uninstall-script-user-relative" do end uninstall script: { - executable: "~/MyFancyApp.app/uninstall.sh", - sudo: false, + executable: "~/MyFancyApp.app/uninstall.sh", + sudo: false, + sudo_as_root: false, } end diff --git a/Library/Homebrew/test/support/helper/cask/never_sudo_system_command.rb b/Library/Homebrew/test/support/helper/cask/never_sudo_system_command.rb index 8cb5e22562..84f3b2035a 100644 --- a/Library/Homebrew/test/support/helper/cask/never_sudo_system_command.rb +++ b/Library/Homebrew/test/support/helper/cask/never_sudo_system_command.rb @@ -5,6 +5,6 @@ require "system_command" class NeverSudoSystemCommand < SystemCommand def self.run(command, **options) - super(command, **options.merge(sudo: false)) + super(command, **options.merge(sudo: false, sudo_as_root: false)) end end diff --git a/Library/Homebrew/test/system_command_spec.rb b/Library/Homebrew/test/system_command_spec.rb index ab2e4d139a..d06c5430be 100644 --- a/Library/Homebrew/test/system_command_spec.rb +++ b/Library/Homebrew/test/system_command_spec.rb @@ -9,12 +9,14 @@ describe SystemCommand do env: env, must_succeed: true, sudo: sudo, + sudo_as_root: sudo_as_root, ) end let(:env_args) { ["bash", "-c", 'printf "%s" "${A?}" "${B?}" "${C?}"'] } let(:env) { { "A" => "1", "B" => "2", "C" => "3" } } let(:sudo) { false } + let(:sudo_as_root) { false } context "when given some environment variables" do its("run!.stdout") { is_expected.to eq("123") } @@ -45,8 +47,9 @@ describe SystemCommand do end end - context "when given some environment variables and sudo: true" do + context "when given some environment variables and sudo: true, sudo_as_root: false" do let(:sudo) { true } + let(:sudo_as_root) { false } describe "the resulting command line" do it "includes the given variables explicitly" do @@ -64,6 +67,27 @@ describe SystemCommand do end end end + + context "when given some environment variables and sudo: true, sudo_as_root: true" do + let(:sudo) { true } + let(:sudo_as_root) { true } + + describe "the resulting command line" do + it "includes the given variables explicitly" do + expect(Open3) + .to receive(:popen3) + .with( + an_instance_of(Hash), ["/usr/bin/sudo", "/usr/bin/sudo"], "-u", "root", + "-E", "A=1", "B=2", "C=3", "--", "env", *env_args, pgroup: nil + ) + .and_wrap_original do |original_popen3, *_, &block| + original_popen3.call("true", &block) + end + + command.run! + end + end + end end context "when the exit code is 0" do