Subject: bug#68074: eshell sudo/doas does not work for aliases

From: Jim Porter <[email-masked]>
Date: Wed, 24 Jan 2024 18:43:43 -0800

On 1/11/2024 1:15 PM, Jim Porter wrote:
> I've been meaning to look at this in detail, but haven't had time. My 
> gut feeling though is that we shouldn't have a special case in this code 
> for when the alias starts with "cd". I'll have to do some testing with 
> aliases and sudo in order to have a more-helpful answer though.
> 
> At a guess, I think a better place to add code for this is likely in 
> em-tramp.el.

After letting this percolate in my brain for a few weeks, the answer 
revealed itself to me: the functions in "em-tramp.el" shouldn't throw 
'eshell-external', they should throw 'eshell-replace-command'. That 
allows for a two-step command replacement in this case. First, 
'eshell/sudo' (or 'eshell/doas') will do its replacement. Then, Eshell 
will evaluate that and call the inner command, which will do another 
replacement to expand the alias.

Alfonso, does the attached patch work for you? If so, I'll merge it to 
master.

From 450e057a3b2305d1a709fd48ca4defca0bb85bbc Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Wed, 24 Jan 2024 18:32:00 -0800
Subject: [PATCH] Fix command replacement with the Eshell builtin versions of
 "sudo" and "doas"

This is particularly important when the inner command to execute is an
alias.  Aliases throw 'eshell-replace-command' too, so we want to do
this in two phases: first, replace the "sudo"/"doas" with a
let-binding of 'default-directory', and then later, let the alias code
do its own replacement (bug#68074).

* lisp/eshell/em-tramp.el (eshell/sudo, eshell/doas): Use
'eshell-replace-command' to wrap the inner command.
* test/lisp/eshell/em-tramp-tests.el (mock-eshell-named-command):
Remove.
(em-tramp-test/sudo-basic, em-tramp-test/sudo-user)
(em-tramp-test/doas-basic, em-tramp-test/doas-user): Catch
'eshell-replace-command'.
---
 lisp/eshell/em-tramp.el            | 22 ++++----
 test/lisp/eshell/em-tramp-tests.el | 89 ++++++++++++++----------------
 2 files changed, 50 insertions(+), 61 deletions(-)

diff --git a/lisp/eshell/em-tramp.el b/lisp/eshell/em-tramp.el
index 90f9c6cf78d..a58fa6f07d9 100644
--- a/lisp/eshell/em-tramp.el
+++ b/lisp/eshell/em-tramp.el
@@ -121,12 +121,11 @@ eshell/sudo
      :usage "[(-u | --user) USER] (-s | --shell) | COMMAND
 Execute a COMMAND as the superuser or another USER.")
    (let ((dir (eshell--method-wrap-directory default-directory "sudo" user)))
-     (if shell
-         (throw 'eshell-replace-command
-                (eshell-parse-command "cd" (list dir)))
-       (throw 'eshell-external
-              (let ((default-directory dir))
-                (eshell-named-command (car args) (cdr args))))))))
+     (throw 'eshell-replace-command
+            (if shell
+                (eshell-parse-command "cd" (list dir))
+              `(let ((default-directory ,dir))
+                 (eshell-named-command ,(car args) ,(cdr args))))))))
 
 (put 'eshell/sudo 'eshell-no-numeric-conversions t)
 
@@ -144,12 +143,11 @@ eshell/doas
      :usage "[(-u | --user) USER] (-s | --shell) | COMMAND
 Execute a COMMAND as the superuser or another USER.")
    (let ((dir (eshell--method-wrap-directory default-directory "doas" user)))
-     (if shell
-         (throw 'eshell-replace-command
-                (eshell-parse-command "cd" (list dir)))
-       (throw 'eshell-external
-              (let ((default-directory dir))
-                (eshell-named-command (car args) (cdr args))))))))
+     (throw 'eshell-replace-command
+            (if shell
+                (eshell-parse-command "cd" (list dir))
+              `(let ((default-directory ,dir))
+                 (eshell-named-command ,(car args) ,(cdr args))))))))
 
 (put 'eshell/doas 'eshell-no-numeric-conversions t)
 
diff --git a/test/lisp/eshell/em-tramp-tests.el b/test/lisp/eshell/em-tramp-tests.el
index d33f6a2b46a..deae6579401 100644
--- a/test/lisp/eshell/em-tramp-tests.el
+++ b/test/lisp/eshell/em-tramp-tests.el
@@ -59,35 +59,31 @@ em-tramp-test/su-login
         "cd"
         (list ,(format "/su:root@%s:~/" tramp-default-host))))))
 
-(defun mock-eshell-named-command (&rest args)
-  "Dummy function to test Eshell `sudo' command rewriting."
-  (list default-directory args))
-
 (ert-deftest em-tramp-test/sudo-basic ()
   "Test Eshell `sudo' command with default user."
-  (cl-letf (((symbol-function 'eshell-named-command)
-             #'mock-eshell-named-command))
-    (should (equal
-             (catch 'eshell-external (eshell/sudo "echo" "hi"))
-             `(,(format "/sudo:root@%s:%s" tramp-default-host default-directory)
-               ("echo" ("hi")))))
-    (should (equal
-             (catch 'eshell-external (eshell/sudo "echo" "-u" "hi"))
-             `(,(format "/sudo:root@%s:%s" tramp-default-host default-directory)
-               ("echo" ("-u" "hi")))))))
+  (let ((sudo-directory (format "/sudo:root@%s:%s"
+                                tramp-default-host default-directory)))
+    (should (equal (catch 'eshell-replace-command
+                     (eshell/sudo "echo" "hi"))
+                   `(let ((default-directory ,sudo-directory))
+                      (eshell-named-command "echo" ("hi")))))
+    (should (equal (catch 'eshell-replace-command
+                     (eshell/sudo "echo" "-u" "hi"))
+                   `(let ((default-directory ,sudo-directory))
+                      (eshell-named-command "echo" ("-u" "hi")))))))
 
 (ert-deftest em-tramp-test/sudo-user ()
   "Test Eshell `sudo' command with specified user."
-  (cl-letf (((symbol-function 'eshell-named-command)
-             #'mock-eshell-named-command))
-    (should (equal
-             (catch 'eshell-external (eshell/sudo "-u" "USER" "echo" "hi"))
-             `(,(format "/sudo:USER@%s:%s" tramp-default-host default-directory)
-               ("echo" ("hi")))))
-    (should (equal
-             (catch 'eshell-external (eshell/sudo "-u" "USER" "echo" "-u" "hi"))
-             `(,(format "/sudo:USER@%s:%s" tramp-default-host default-directory)
-               ("echo" ("-u" "hi")))))))
+  (let ((sudo-directory (format "/sudo:USER@%s:%s"
+                                tramp-default-host default-directory)))
+    (should (equal (catch 'eshell-replace-command
+                     (eshell/sudo "-u" "USER" "echo" "hi"))
+                   `(let ((default-directory ,sudo-directory))
+                      (eshell-named-command "echo" ("hi")))))
+    (should (equal (catch 'eshell-replace-command
+                     (eshell/sudo "-u" "USER" "echo" "-u" "hi"))
+                   `(let ((default-directory ,sudo-directory))
+                      (eshell-named-command "echo" ("-u" "hi")))))))
 
 (ert-deftest em-tramp-test/sudo-shell ()
   "Test Eshell `sudo' command with -s/--shell option."
@@ -109,34 +105,29 @@ em-tramp-test/sudo-user-shell
 
 (ert-deftest em-tramp-test/doas-basic ()
   "Test Eshell `doas' command with default user."
-  (cl-letf (((symbol-function 'eshell-named-command)
-             #'mock-eshell-named-command))
-    (should (equal
-             (catch 'eshell-external (eshell/doas "echo" "hi"))
-             `(,(format "/doas:root@%s:%s"
-                        tramp-default-host default-directory)
-               ("echo" ("hi")))))
-    (should (equal
-             (catch 'eshell-external (eshell/doas "echo" "-u" "hi"))
-             `(,(format "/doas:root@%s:%s"
-                        tramp-default-host default-directory)
-               ("echo" ("-u" "hi")))))))
+  (let ((doas-directory (format "/doas:root@%s:%s"
+                                tramp-default-host default-directory)))
+    (should (equal (catch 'eshell-replace-command
+                     (eshell/doas "echo" "hi"))
+                   `(let ((default-directory ,doas-directory))
+                      (eshell-named-command "echo" ("hi")))))
+    (should (equal (catch 'eshell-replace-command
+                     (eshell/doas "echo" "-u" "hi"))
+                   `(let ((default-directory ,doas-directory))
+                      (eshell-named-command "echo" ("-u" "hi")))))))
 
 (ert-deftest em-tramp-test/doas-user ()
   "Test Eshell `doas' command with specified user."
-  (cl-letf (((symbol-function 'eshell-named-command)
-             #'mock-eshell-named-command))
-    (should (equal
-             (catch 'eshell-external (eshell/doas "-u" "USER" "echo" "hi"))
-             `(,(format "/doas:USER@%s:%s"
-                        tramp-default-host default-directory)
-               ("echo" ("hi")))))
-    (should (equal
-             (catch 'eshell-external
-               (eshell/doas "-u" "USER" "echo" "-u" "hi"))
-             `(,(format "/doas:USER@%s:%s"
-                        tramp-default-host default-directory)
-               ("echo" ("-u" "hi")))))))
+  (let ((doas-directory (format "/doas:USER@%s:%s"
+                                tramp-default-host default-directory)))
+    (should (equal (catch 'eshell-replace-command
+                     (eshell/doas "-u" "USER" "echo" "hi"))
+                   `(let ((default-directory ,doas-directory))
+                      (eshell-named-command "echo" ("hi")))))
+    (should (equal (catch 'eshell-replace-command
+                     (eshell/doas "-u" "USER" "echo" "-u" "hi"))
+                   `(let ((default-directory ,doas-directory))
+                      (eshell-named-command "echo" ("-u" "hi")))))))
 
 (ert-deftest em-tramp-test/doas-shell ()
   "Test Eshell `doas' command with -s/--shell option."
-- 
2.25.1