1
\$\begingroup\$

One year and a half ago I posted the second iteration of this script for a review here:

Editing system files in Linux (as root) with GUI and CLI text editors #2

Since then, it has been "hibernated" as I had way too much work, and I would like to ask you for a review of the possible final edit. I made a big effort for it to be final, but we all know there is always some space to improve. Thank you in advance!

As stated in there:

My intention is to POSIX-ly write one generalized function for running various text editors I use for different purposes through sudoedit, i.e. editing files as root safely. Safely = for instance, if a power loss occurs during the file edit; another example could be lost SSH connection, etc.

The script follows

#!/bin/sh

# Please, customize these lists to your preference before using this script!
cli_editors='vi nano'
gui_editors='subl xed'

# NON-COMPLIANT: code -w --user-data-dir; I did not find a way to run it through `sudoedit`;
# atom -w --no-sandbox; gedit -w does not work via rooted ssh; pluma does not have -w switch


# USAGE INSTRUCTIONS

# 1. Customize the editor lists at the beginning of this script.
#
# 2. Bash: Source this script in your ~/.bashrc or ~/.bash_aliases with:
# . /full/path/to/sudoedit-enhanced
# Other shells: Generally put it inside your shell's startup config file...
#
# 3. Call an alias, for instance, one CLI, and one GUI editor:
# sunano /path/to/file
# susubl /path/to/file
#
# Explanation: This script works with standard `sudoedit`, but
# it does a bit more checks and allows some GUI editors to be used.
# It needs to be sourced into your shell's environment first.
# Then simply use the pre-defined aliases or define some yourself.

sudoedit_err ()
{
    printf >&2 '%s\n' 'Error in sudoedit_run():'
    printf >&2 '%b\n' "$@"
    exit 1
}

sudoedit_run ()
{
    # print usage
    if [ "$#" -eq 2 ]; then
        printf >&2 '%s\n' 'Usage example: sunano /file1/path /file2/path'
        exit 1
    fi

    # sudo must be installed
    if ! command -v sudo > /dev/null 2>&1; then
        sudoedit_err "'sudo' is required by this function. This is because" \
        "'sudoedit' is part of 'sudo'\`s edit feature (sudo -e)"
    fi

    # the first argument must be an editor type
    case "$1" in
        ('cli'|'gui') ;;
        (*) sudoedit_err "'$1' is unrecognized editor type, expected 'cli' or 'gui'." ;;
    esac

    # store editor's type and name and move these two out of argument array
    editor_type=$1; editor_name=$2; shift 2

    # find and store path to this editor
    editor_path=$( command -v "$editor_name" 2> /dev/null )

    # check if such editor = executable path exists
    if ! [ -x "$editor_path" ]; then
        sudoedit_err "The '$editor_name' editor is not properly installed on this system."
    fi

    # `sudoedit` does not work with symlinks!
    # translating symlinks to normal file paths using `readlink` if available
    if command -v readlink > /dev/null 2>&1; then
        for file in "$@"; do
            if [ -L "$file" ]; then
                if ! file=$( readlink -f "$file" ); then
                    sudoedit_err "readlink -f $file failed."
                fi
            fi
            set -- "$@" "$file"
            shift
        done
    fi

    if [ "$editor_type" = gui ]; then
        # 1. GUI editors will "sit" on the terminal until closed thanks to the wait option
        # 2. Various editors errors might flood the terminal, so we redirect all output,
        #    into prepared temporary file in order to filter possible "sudoedit: file unchanged" lines.
        if tmpfile=$( mktemp /tmp/sudoedit_run.XXXXXXXXXX ); then
            SUDO_EDITOR="$editor_path -w" sudoedit -- "$@" > "$tmpfile" 2>&1
            grep 'sudoedit:' "$tmpfile"
            rm "$tmpfile"
        else
            sudoedit_err "mktemp /tmp/sudoedit_run.XXXXXXXXXX failed."
        fi
    else
        # 1. CLI editors do not cause problems mentioned above.
        # 2. This is a generic and proper way using `sudoedit`;
        #    Running the editor with one-time SUDO_EDITOR setup.
        SUDO_EDITOR="$editor_path" sudoedit -- "$@"
    fi
}

sudoedit_sub ()
{
    ( sudoedit_run "$@" )
}

# Editor aliases generators:
# - avoid generating editors aliases for which editor is not installed
# - avoid generating editors aliases for which already have an alias

for cli_editor in $cli_editors; do
    if command -v "$cli_editor" > /dev/null 2>&1; then
        # shellcheck disable=SC2139,SC2086
        if [ -z "$( alias su$cli_editor 2> /dev/null)" ]; then
            alias su$cli_editor="sudoedit_sub cli $cli_editor"
        fi
    fi
done

for gui_editor in $gui_editors; do
    if command -v "$gui_editor" > /dev/null 2>&1; then
        # shellcheck disable=SC2139,SC2086
        if [ -z "$( alias su$gui_editor 2> /dev/null)" ]; then
            alias su$gui_editor="sudoedit_sub gui $gui_editor"
        fi
    fi
done

unset cli_editors gui_editors
Improve this question
\$\endgroup\$

2 Answers 2

Reset to default
2
\$\begingroup\$

Aside from your self-review, I noticed a few (very minor) things I thought may be worth pointing out

First, a design opinion rather than a comment on the code. I'm not sure putting the lists of editors in variables in the file is the best approach - it has its advantages, but allowing it to be managed separately in configuration files may have advantages. Were I a user of this script I would prefer to be told to go edit something like a $XDG_CONFIG_DIRS/sudoedit-enhanced file over being encouraged to edit the script itself

It feels a bit weird to have sudoedit_run print sunano as a usage example regardless of whether nano is desired, or available. Would it be at all realistic to offer up one of the aliases that were successfully defined?

While you can usually assume /tmp exists, the user may prefer to store their temp files elsewhere. Consider having the mktemp call instead use the --tmpdir flag to create a file relative to the $TMPDIR set by the user - not many users will care, but those who do will probably have very strong feelings about it

The alias generators act a bit weird on editors whose names contain spaces - which binary files can even if they rarely do. Right now, as you store each editor as a word in a space-separated string you really can't worry about what to do with them, but if you were to some day end up refactoring something in such a way that it becomes possible to do something funky like cli_editor="per se", consider how a line like su$cli_editor="sudoedit_run cli $cli_editor" would react to that (it's sometimes but not always an error, and there might be side effects). Quoting the alias name, to force a failure due to an invalid alias name (at least in bash, not sure if other shells permit spaces in alias names) might be preferable

sudoedit_run also has a subtle bug in how it checks for an editor - command -v will report the name of shell builtins in favor of commands. If someone were to do something silly like listing echo as an editor, command -v echo would return echo. This would lead to it working just fine as long as there's a file called echo in your current directory, but get reported as not properly installed otherwise. It's enough of a corner case that it's almost certainly not worth caring about, but still

Improve this answer
\$\endgroup\$
0
1
\$\begingroup\$

  1. The print error function is only useful to a point, I have rather removed it and used direct in-place constructs instead, so this function has been removed:

    sudoedit_err () { ... ; exit 1; }

    and the code had changed to this:

    printf >&2 '%s\n' '...'
return 1

    So, it is now without the useless headline, and also we eliminated the exit keyword completely. In all of the code return is used now. Also there is now no need for '%b\n' in printf as there are no escape sequences like Tab or New line.

  2. Thanks to the above change it was possible to remove the subshell function:

    sudoedit_sub ()
{
    ( sudoedit_run "$@" )
}

  3. The print usage routine now returns 0 exit code, as it seems more logical to me if someone just hits the alias without any argument like with nano:

    sunano

  4. New opening routine added to address the issue when someone would try to hit sudoedit_run with 0 or 1 argument:

    if [ "$#" -le 1 ]; then
    printf >&2 '%s\n' 'Implementation example: sudoedit_run cli nano /path/to/file'
    return 1
fi

  5. Finally, there was an unseen error in the GUI editor routine, where it would always return 0 exit code, no matter how sudoedit would end up, this is now corrected:

    if [ "$editor_type" = 'gui' ]; then
    # 1. GUI editors will "sit" on the terminal until closed thanks to the wait option
    # 2. Various editors errors might flood the terminal, so we redirect all output,
    #    into prepared temporary file in order to filter possible "sudoedit: file unchanged" lines.
    if tmpfile=$( mktemp /tmp/sudoedit_run.XXXXXXXXXX ); then
        SUDO_EDITOR="$editor_path -w" sudoedit -- "$@" > "$tmpfile" 2>&1
        exit_code=$?
        grep 'sudoedit:' "$tmpfile"
        rm "$tmpfile"
        return "$exit_code"
    else
        printf >&2 '%s\n' "mktemp /tmp/sudoedit_run.XXXXXXXXXX failed."
        return 1
    fi
else
    ... CLI routine ...
fi

The refactored code follows:

#!/bin/sh

#+------------------------------------------------------------------------------+
#|                        Safe text files editing as `root`                     |
#|                          Language: POSIX shell script                        |
#|                     Copyright: 2020-2021 Vlastimil Burian                    |
#|                      M@il: info[..]vlastimilburian[..]cz                     |
#|                               License: GPL 3.0                               |
#|                                 Version: 2.1                                 |
#|                         GitHub: https://git.io/Jvnzq                         |
#+------------------------------------------------------------------------------+

# Customize these lists to your preference before using this script!
cli_editors='vi nano'
gui_editors='subl xed'

# NON-COMPLIANT-GUI:
#    - code -w --user-data-dir; I did not find a way to run it through `sudoedit`
#    - atom -w --no-sandbox
#    - gedit -w does not work via rooted ssh
#    - pluma does not have -w switch

# USAGE INSTRUCTIONS
#
# 1. Customize the editor lists at the beginning of this script.
#
# 2. Bash: Source this script in your ~/.bashrc or ~/.bash_aliases with:
#    source /full/path/to/sudoedit-enhanced
#
# Other shells: Put it inside your shell's startup config file universally using dot:
#    . /full/path/to/sudoedit-enhanced
#
# 3. Call an alias, for instance, one CLI, and one GUI editor:
#
# sunano /path/to/file
# susubl /path/to/file
#
# Explanation: This script works with standard `sudoedit`, but
# it does a bit more checks and allows some GUI editors to be used.
# It also allows symbolic links be translated into ordinary files.
# It needs to be sourced into your shell's environment first.
# Then simply use the pre-defined aliases or define some yourself.

sudoedit_run ()
{
    # print error if called with 0 or 1 argument
    if [ "$#" -le 1 ]; then
        printf >&2 '%s\n' 'Implementation example: sudoedit_run cli nano /path/to/file'
        return 1
    fi

    # print usage if called with 2 arguments
    if [ "$#" -eq 2 ]; then
        printf '%s\n' 'Usage example: sunano /path/to/file1 /path/to/file2'
        return 0
    fi

    # sudo must be installed
    if ! command -v sudo > /dev/null 2>&1; then
        printf >&2 '%s\n' "'sudo' is required by this function. This is because" \
                          "'sudoedit' is part of 'sudo'\`s edit feature (sudo -e)."
        return 1
    fi

    # the first argument must be an editor type
    case "$1" in
        ('cli'|'gui')
        ;;
        (*)
            printf >&2 '%s\n' "'$1' is unrecognized editor type, expected 'cli' or 'gui'."
            return 1
        ;;
    esac

    # store editor's type and name and move these two out of argument array
    editor_type=$1; editor_name=$2; shift 2

    # find and store path to this editor
    editor_path=$( command -v "$editor_name" 2> /dev/null )

    # check if such editor = executable path exists
    if ! [ -x "$editor_path" ]; then
        printf >&2 '%s\n' "The '$editor_name' editor is not properly installed on this system."
        return 1
    fi

    # `sudoedit` does not work with symlinks!
    # translating symlinks to normal file paths using `readlink` if available
    if command -v readlink > /dev/null 2>&1; then
        for file in "$@"; do
            if [ -L "$file" ]; then
                if ! file=$( readlink -f "$file" ); then
                    printf >&2 '%s\n' "readlink -f $file failed."
                    return 1
                fi
            fi
            set -- "$@" "$file"
            shift
        done
    fi

    if [ "$editor_type" = 'gui' ]; then
        # 1. GUI editors will "sit" on the terminal until closed thanks to the wait option
        # 2. Various editors errors might flood the terminal, so we redirect all output,
        #    into prepared temporary file in order to filter possible "sudoedit: file unchanged" lines.
        if tmpfile=$( mktemp /tmp/sudoedit_run.XXXXXXXXXX ); then
            SUDO_EDITOR="$editor_path -w" sudoedit -- "$@" > "$tmpfile" 2>&1
            exit_code=$?
            grep 'sudoedit:' "$tmpfile"
            rm "$tmpfile"
            return "$exit_code"
        else
            printf >&2 '%s\n' "mktemp /tmp/sudoedit_run.XXXXXXXXXX failed."
            return 1
        fi
    else
        # 1. CLI editors do not cause problems mentioned above.
        # 2. This is a generic and proper way using `sudoedit`;
        #    Running the editor with one-time SUDO_EDITOR setup.
        SUDO_EDITOR="$editor_path" sudoedit -- "$@"
    fi
}

# Editor aliases generators:
# - avoid generating editors aliases for which editor is not installed
# - avoid generating editors aliases for which already have an alias

for cli_editor in $cli_editors; do
    if command -v "$cli_editor" > /dev/null 2>&1; then
        # shellcheck disable=SC2139,SC2086
        if [ -z "$( alias su$cli_editor 2> /dev/null)" ]; then
            alias su$cli_editor="sudoedit_run cli $cli_editor"
        fi
    fi
done

for gui_editor in $gui_editors; do
    if command -v "$gui_editor" > /dev/null 2>&1; then
        # shellcheck disable=SC2139,SC2086
        if [ -z "$( alias su$gui_editor 2> /dev/null)" ]; then
            alias su$gui_editor="sudoedit_run gui $gui_editor"
        fi
    fi
done

unset cli_editors gui_editors cli_editor gui_editor
Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.