Shell syntax error near unexpected token `done'

shell, token

Solution

The `if` starting at:

if [ $? == 0 ]; then
  echo "User Already Exists : Error adding user with username ...
  exit 1
else

is ended with the `done` instead of the `fi` that is required.

The `while` loop starting a couple of lines earlier:

cat userlist.txt | while read uname password gname fullname

is missing its `do` (another bug); if that was present, then it would also need the `done` at the end. Someone lost track of the indentation. (Using 2 characters per level is better than 0 or 1, but it is easier to track levels if you use 4 spaces per level.) Note that the shell hasn't gotten around to complaining about the lack of `do` because the syntax for a while loop is:

while cmd1
      cmd2
      cmd3 ...
do

and as far as the shell is concerned, it is still processing commands in the list `cmd1, cmd2, cmd3, ...`.

Here's a semi-decently indented version of the script. There was a missing `fi` at the top of the script, too.

#!/bin/bash
#Purpose: Automatically add new users in a linux system based upon the data found within a text file
#         Assign encryped passwords to each user
#         Add users to groups or create new groups for these users
#         Report errors and successful operations where necessary in log files
#         post help options (echo)

#Root validation
if [[ $(id -u) -eq 0 ]]
then
    #Argument validation
    if [[ -z "$1" ]]
    then
        echo "No arguments found!"
        echo "Please include a user detail text file as first argument"
        echo "Please include a report text file as second argument"
        echo "Please include an error report text file as the third argument"
        echo "Use the -h argument (i.e. ./script -h) for help"
        exit 1
    fi
fi

#Help validation and Help file
if [[ "$1" = "-h" ]]
then
    echo "This is the help information file"
    echo "This script is designed to add users to a linux system by reading information from a user detail file (such as userlist.txt)"
    echo "The format of this user detail text file is "username password groupname fullname" seperated using TAB spacing"
    echo "This script will read the first argument as the user detail file, the second and third arguments will be read as a success report file and error report file respectively"
    exit
fi

#Reads first argument as user detail file for data
cat userlist.txt | while read uname password gname fullname
do
    #Reads /etc/passwd for Username
    egrep -w "^$uname" /etc/passwd
    #If Username is found then error reports
    if [ $? == 0 ]
    then
        echo "User Already Exists : Error adding user with username $uname;$gname;$fullname" >> Successes1.log
        exit 1
    else
        #Reads /etc/group for Groupname
        egrep -w "^$gname" /etc/group
        #If Groupname is found then nothing
        if [ $? == 0 ]
        then
            echo ""
        else
            #If Groupname not found then creates new group and reports
            groupadd "$gname"
            echo "Group Not Found: New Group $gname was created" >> Successes1.log
        fi
        #Retrieves Date
        createddate=$(date)
        #Perl password script takes input from Userlist
        pass=$(perl -e 'print crypt($ARGV[0], "Password")' $pass)
        #Adds Users with variables from userlist
        useradd "$uname" -g "$gname"  -c "$fullname" -p "$pass"
        #Reports information to successlist and errorlist report files
        if [ $? == 0 ]
        then
            groupid=$(id -g $uname)
            userid=$(id -u $uname)
            echo "User Successfully Added: $uname;$userid;$gname;$groupid;$createddate;$fullname" >> Successes1.log
        else
            groupid=$(id -g $uname)
            userid=$(id -u $uname)
            echo "Useradd Error Occurred: $uname;$userid;$gname;$groupid;$createddate;$fullname" >> Errors1.log
            echo "Error: Must be root user to execute script"
            exit
        fi
    fi
done

There is still much room for improvement. The root validation block should exit if the user is not root; that happens instead a mile further down inside the loop. You can check the number of arguments better: `$#` gives you the number of arguments. If I tried `yourscript.sh '' arg2 arg3`, you'd claim there were no arguments when in fact the problem is that `$1` is present but is an empty string. The standard convention for reporting how to use a command is something like:

echo "Usage: $0 usernames report errors" 1>&2

This reports the command's name, and the arguments expected. The `1>&2` sends the message to standard error instead of standard output. The logic here is a little bizarre even so. You check that the user is root and only then check that there are arguments. If the user is not root, you don't check the arguments. Not entirely sensible, I submit.

We can debate the UUOC (Useless Use of Cat). There's actually an award for it; I don't think this qualifies. However, it would be possible to write:

while read uname password gname fullname
do
    ...
done < $1

Hmmm...the script is supposed to take a file name argument that specifies the users, but the `cat` takes a fixed file name, not the file name argument!

Similarly, arguments 2 and 3 are studiously ignored; the log files are hard-coded.

egrep -w "^$uname" /etc/passwd
#If Username is found then error reports
if [ $? == 0 ]

This fragment can be improved several ways:

if egrep -w "^$uname:" /etc/passwd >/dev/null 2>&1
then
    #If Username is found then error report

This tests the exit status of the `egrep` command directly; it also prevents a new user `roo` from being treated as pre-existing because of user `root`. It sends the output and error output to `/dev/null` so that you won't see anything when the user does exist.

It might be better not to exit when the user name is found; you could at least try to process the next entry. It is also odd that the report that the user exists (which terminates the processing) is recorded in `Successes1.log` rather than in `Errors1.log`; it is treated like an error.

The group check constructs are similar and should be similarly upgraded.

You read the password into `$password` with the `read` line; when it comes to creating the password, though, you have:

pass=$(perl -e 'print crypt($ARGV[0], "Password")' $pass)

On the first cycle, `$pass` is empty (most probably); you should have used `$password` in double quotes at the end:

pass=$(perl -e 'print crypt($ARGV[0], "Password")' "$password")

As with the `egrep` commands, you can check the status of the `useradd` command directly too. It is a bit sweeping to say `if [ $? == 0 ]` is the mark of a tyro, but it isn't too far off the truth.

The final `exit` in the script should be `exit 1` to indicate an error exit. As already noted, this is preceded by the comment about 'you must be root', even though there was a check at the top for root privileges.

Caveat: I've not attempted to run the script; I could easily have missed some issues. It does, however, pass `sh -v -n`, so there are no gross syntactic errors left.

Once a shell script is syntactically correct, then you normally debug it using `sh -x script` (or, if it takes arguments, then `sh -x script arg1 arg2 arg3 ...`). This is the execution trace mode. The shell tells you, more or less inscrutably, what it is doing. The information is written to standard error. You can even trap the output for later scrutiny if you like with:

sh -x script arg1 arg2 arg3 2>script-x.log

The `2>script-x.log` notation sends the standard error to the file `script-x.log` (choose your own meaningful name; I often use `x` or `xxx` for files I won't want to keep, but I also remove such files without necessarily even looking at them because I know they are throwaway files).

Problem

Original source