5
\$\begingroup\$

I have this bash script that adds users to NextCloud from a csv file. It expects to be run from the same directory as the docker-compose.yml file.

I'm pretty new to bash scripting and I'm wondering if I am doing things correctly or if there are pieces that could be improved in terms of efficiency, correctness, style? Any comments are appreciated really.

Thanks in advance!

#!/bin/bash

# Handle printing errors
die () {
  printf '%s\n' "$1" >&2
  exit 1
}

usage () {
  echo ""
  echo "batch_users.sh"          
  echo "SYNOPSIS"
  echo "batch_users [-p] [file]"
  echo "DESCRIPTION"
  echo "The batch_users script adds a batch of users to an instance of NextCloud running inside of a docker container by reading a list from a csv file."
  echo ""
  echo "-p, --password    Set users password. If no option is passed, the default password is nomoremonkeysjumpingonthebed ."
  echo ""
  echo "csv file should be formatted in one of the following configurations."
  echo "username,Display Name,group,email@address.domain,"
  echo "username,Display Name,group,"
  echo "username,Display Name,"
  echo "username,"
  echo ""
  echo "EXAMPLES"
  echo "The command:"
  echo "batch_users.sh -p 123password321 foobar.csv"
  echo "will add the users from foobar.csv and assign them the password 123password321"
  echo "The command:"
  echo "batch_users.sh foobar.csv"
  echo "will add the users from foobar.csv and assign them the default password."
  echo ""
  echo "batch_users will return 0 on success and a positive number on failure."
  echo ""
}
# flags
password=nomoremonkeysjumpingonthebed
while :; do
  case $1 in
    -h|-\?|--help)
      usage # Display a usage synopsis.
      exit
      ;;
    -p|--password)
      if [ "$2" ]; then
        password=$2
        shift
      else
        die 'Error: "--password" requires a non-empty option argument.'
      fi
      ;;
    --password=?*)
      password=${1#*=} # Delete everything up to = and assign the remainder.
      ;;
    --password=) # Handle the case of empty --password=
      die 'Error: "--password" requires a non-empty option argument.'
      ;;
    --)
      shift
      break
      ;;
    -?*)
      printf 'WARN: Unknown option (ignored): %s\n' "$1" >&2
      ;;
    *) # Default case. No more options, so break out of the loop
      break
  esac
  shift
done


# Check to see if there was at least one argument passed.
# If not, print error and exit.
if [[ $# -eq  0 ]]
then
  die 'Error: Expected at least one argument, but no arguments were supplied.'
fi

# Check to see if the file passed in exists.
# If not, print error and exit.
if [[ ! -e $1 ]]
then
  die "Couldn't find file ${1}."
  exit 1
fi


input_file="$1"

while IFS=, read -r f1 f2 f3 f4
do
  # check --password flag
  # f1, f2, f3 exist?
  if [[ -n $f1  && -n $f2 && -n $f3 ]]
  then

    sh -c "docker-compose exec -T --env OC_PASS=${password} --user www-data app php occ \
      user:add --password-from-env --display-name=\"${f2}\" --group=\"${f3}\" \"$f1\" " < /dev/null

  elif [[ -n $f1 && -n $f2 ]]
  then
    # f1 and f2

    sh -c "docker-compose exec -T --env OC_PASS=${password} --user www-data app php occ \
      user:add --password-from-env --display-name=\"${f2}\" \"$f1\" " < /dev/null

  elif [[ -n $f1 ]]
  then
    #only f1

    sh -c "docker-compose exec -T --env OC_PASS=${password} --user www-data app php occ \
      user:add --password-from-env \"$f1\" " < /dev/null

  else
    #error
    die "Expected at least one field, but none were supplied."
  fi

  # If there is a fourth value in the csv, use it to set the user email.
  if [[ -z ${f4+x} ]]
  then
    break
  else
    sh -c "docker-compose exec -T --user www-data app php occ\
      user:setting \"$f1\" settings email \"${f4}\" " < /dev/null
  fi
done <"$input_file"
exit 0

\$\endgroup\$
4
\$\begingroup\$

Review

This code looks pretty good. I know very little about the docker-compose command being executed, but I can look at the script style.

Shellcheck spots no issues, which is always a good start.

I see nothing in here that really demands Bash rather than standard (POSIX) shell - all those [[ can be easily converted to [.

I recommend setting -u and probably -e. In any case, consider what the effects of any individual command failing should be. Within the loop, I recommend continuing to the next iteration, but remember whether any iteration failed. One way I do that is

status=true   # until a failure
for i in ....
do
    some_command  || status=false
done

# terminate with appropriate exit code    
exec $status

Instead of using many echo commands in usage(), it's easier to copy a single here-document:

usage() {
    cat <<'EOF'
batch_users.sh
SYNOPSIS
...
EOF
}
if [[ ! -e $1 ]]
then
  die "Couldn't find file ${1}."
  exit 1
fi

Braces not required here for $1, and exit 1 is unreachable. Mere existence of the file is insufficient: we need it to be readable. Here's my version:

[ -r "$1" ] || die "Couldn't find file $1."

We need to be very careful when composing strings to be interpreted by other commands, especially shells:

    sh -c "docker-compose exec -T --env OC_PASS=${password} --user www-data app php occ \
      user:add --password-from-env --display-name=\"${f2}\" --group=\"${f3}\" \"$f1\" " < /dev/null

$password in particular could contain almost anything (including quote characters), so that's really not safe in the inner sh instance. Do we really need that shell, or can we simply invoke docker-compose directly? The latter is much easier to get right. If we really need the sh, then we'll need to be a lot more careful about constructing that command (I think we need to use a printf that supports %q conversion).

We can avoid the repetition by using ${var+} to conditionally include the optional arguments:

if [ "$f1" ]
then
    docker-compose exec -T --env OC_PASS="$password" \
                   --user www-data app php occ \
                   user:add --password-from-env \
                   ${f2:+"--display-name=$f2"} \
                   ${f3:+"--group=$f3"} \
                   "$f1" \
                   </dev/null \
         || status=false
else
    #error
    echo "Expected at least one field, but none were supplied." >&2
    status=false
    continue
fi

I think we have an unnecessary password exposure,. --password-from-env allows passing the password in environment, so that it doesn't appear in command-line arguments. But we've immediately lost that advantage when we wrote --env OC_PASS="${password}". I think the correct thing to do is to put the password into environment before the loop:

OC_PASS=$password
export OC_PASS

And then specify to copy it without giving a new value: --env OC_PASS. We could also ditch $password and assign directly to OC_PASS. It would be worth not setting a default, so that users can use the same environment variable to avoid exposing the password using when invoking the script.


Modified code

#!/bin/sh

set -eu

# Handle printing errors
die () {
    printf '%s\n' "$1" >&2
    exit 1
}

usage () {
    cat <<'EOF'
batch_users.sh       
SYNOPSIS
batch_users [-p] [file]
DESCRIPTION
The batch_users script adds a batch of users to an instance of NextCloud
running inside of a docker container by reading a list from a csv file.

-p, --password    Set user password. If no option is passed, the password
                  should be passed in the OC_PASS environment variable.

csv file should be formatted in one of the following configurations:
username,Display Name,group,email@address.domain,
username,Display Name,group,
username,Display Name,
username,

EXAMPLES
The command:
    batch_users.sh -p 123password321 foobar.csv
will add the users from foobar.csv and assign them the password 123password321
The command:
    batch_users.sh foobar.csv
will add the users from foobar.csv and assign them the default password.

batch_users will return 0 on success and a positive number on failure.
EOF
}


# flags
while true
do
    case $1 in
        -h|-\?|--help)
            usage # Display a usage synopsis.
            exit
            ;;
        -p|--password)
            if [ "$2" ]; then
                OC_PASS=$2
                export OC_PASS
                shift
            else
                die 'Error: "--password" requires a non-empty option argument.'
            fi
            ;;
        --password=?*)
            OC_PASS=${1#*=} # Delete everything up to = and assign the remainder.
            export OC_PASS
            ;;
        --password=) # Handle the case of empty --password=
            die 'Error: "--password" requires a non-empty option argument.'
            ;;
        --)
            shift
            break
            ;;
        -?*)
            printf 'WARN: Unknown option (ignored): %s\n' "$1" >&2
            ;;
        *) # Default case. No more options, so break out of the loop
            break
    esac
    shift
done


# Was exactly one filename argument passed?
[ $# -eq 1 ] || die "$0: Expected a filename argument"

# Is the file readable?
[ -r "$1" ] || die "$1: missing or not readable"

[ "$OC_PASS" ] || die "$0: No password specified. Run with --help for more info."




status=true                     # until a command fails

while IFS=, read -r f1 f2 f3 f4
do
    if [ "$f1" ]
    then
        docker-compose exec -T --user www-data --env OC_PASS \
                       app php occ \
                       user:add --password-from-env \
                       ${f2:+"--display-name=$f2"} \
                       ${f3:+"--group=$f3"} \
                       "$f1" \
                       </dev/null \
            || status=false
    else
        echo "Expected at least one field, but none were supplied." >&2
        status=false
        continue
    fi

    # If there is a fourth value in the csv, use it to set the user email.
    if [ "$f4" ]
    then
        docker-compose exec -T \
                       --user www-data app php occ \
                       user:setting "$f1" settings email "$f4" \
                       </dev/null \
            || status=false
    fi
done <"$1"

exec $status

Further suggestions

  • Perhaps it would be useful to add a --verbose option that shows what is to be done for each input line.
  • Does the input really need to come from a file? It would be useful to be able to provide it on standard input (so we could filter a list using grep, for example).
\$\endgroup\$
  • \$\begingroup\$ I had to make a few changes to your script to make it run. In order to get the environment variables into docker, I had to set --env OC_PASS=$OC_PASS. Is this still "bad"? Not sure how else I could do it. Also, in the case where $f2 or $f3 are missing, I was getting an error Call to a member function getGID() on null which I solved by changing ${f3+"--group=$f3"} to ${f3:+"--group=$f3"}. Thanks for the excellent review. I learned a lot. \$\endgroup\$ – quarterpi Mar 27 at 19:25
  • \$\begingroup\$ I always get mixed up between + and :+ in that context - sorry about that. I would expect that there's a way to avoid putting the password on the command-line like that (on many systems, including Linux, command-lines are viewable by anyone; environment variables are private). And I assume you mean -env OC_PASS="$OC_PASS" - Shellcheck will catch the missed double-quotes, I expect. \$\endgroup\$ – Toby Speight Mar 27 at 19:28
  • \$\begingroup\$ I read that docker run can be passed -e without = to use the value from the current environment. Let me know whether export OC_PASS; docker-compose exec -e OC_PASS app php occ user:add --password-from-env works; if so, that's the right way to do it without exposing the password in visible process arguments, and I'll be able to update the answer accordingly. \$\endgroup\$ – Toby Speight Mar 28 at 8:31
  • \$\begingroup\$ Wow, thank you. I don’t know how I missed that in the docs. I’m away from my computer today but I am sure that’s the way to do it. I’ll let you know for sure when I get a chance. Do you want to submit a PR on GitHub since you changed the original script so much? github.com/quarterpi/nextcloud-batch-users if so. \$\endgroup\$ – quarterpi Mar 28 at 12:21
  • 1
    \$\begingroup\$ That's great; I've already made the edit, so thanks for confirming it. I don't have a GitHub account, so won't submit a PR, but happy for you to credit/blame me for the changes! ;-) \$\endgroup\$ – Toby Speight Mar 29 at 14:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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