Batch upload users to nextcloud on docker with bash csv

Question

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 protected],"
 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

Answer 1

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 protected],
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).

• quarterpi: 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.
• Toby Speight: 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.
• Toby Speight: 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.
• quarterpi: 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.
• Toby Speight: 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! ;-)