docker/add-user.sh: Don't crash on updating when there is a single user

[erjoalgo]

When a single dovecot user exists and their password is updated,
the grep -v command intended to remove the user's old password
will not match any lines and fail, causing the entire script to fail.

[albertito]

Hi! Thank you for sending this!

While using sed -i to remove the line seems reasonable in principle, I am not sure if I understand the bug this is fixing.

I am worried that what makes the change work for you could be the removal of the : at the end of the expression, which can be a problem because the wrong email could be removed (e.g. if you have a user abc and an abcde).

So I think I need to understand the problem better.

Can you send the contents of the users file the current script is failing on? How did you generate it?

Thank you!

[erjoalgo]

Here is an example repro script:

#!/bin/bash -x

set -e

# prelude setting up /data/dovecot/users
sudo mkdir -p /data/dovecot/
sudo chown -R $(whoami) /data
cat <<EOF > /data/dovecot/users
[email protected]:{CRYPT}$2y$05$gTTsE0TKRLke8d8H/qIKhfyQf0HCdZ17mh975LdHfTMZbPhmVQ/56::::
EOF
[email protected]

# code from add-users.sh
mkdir -p /data/dovecot
touch /data/dovecot/users
if grep -q "^${EMAIL}:" /data/dovecot/users; then
	cp /data/dovecot/users /data/dovecot/users.old
        # the next command will fail
	grep -v "^${EMAIL}:" /data/dovecot/users.old \
		> /data/dovecot/users
fi

echo "success" # this line never runs

Basically the grep -v command will fail because after removing the email in question, there are no lines left.

The removal of the : was not intended, and I've added it back and updated the PR.

[albertito]

Thank you for the details!

Here is an example repro script:

cat <<EOF > /data/dovecot/users
[email protected]:{CRYPT}$2y$05$gTTsE0TKRLke8d8H/qIKhfyQf0HCdZ17mh975LdHfTMZbPhmVQ/56::::
EOF

[...]

if grep -q "^${EMAIL}:" /data/dovecot/users; then
	cp /data/dovecot/users /data/dovecot/users.old
        # the next command will fail
	grep -v "^${EMAIL}:" /data/dovecot/users.old \
		> /data/dovecot/users
fi

Basically the grep -v command will fail because after removing the email in question, there are no lines left.

Oooohhhh I see, the problem is the exit value of grep -v:

$ echo abcd:efg > xxx
$ grep -v '^abcd:' xxx; echo $?
1
$ grep -v '^pppp:' xxx; echo $?
abcd:efg
0

Thank you, now I get where the problem is. sed -i seems like a more practical way to implement this.

I'll have one comment inline on the patch, but this seems fine to me, thank you!

The removal of the : was not intended, and I've added it back and updated the PR.

Great, thanks! Glad to rule that out too.

[albertito]

Can you use sed --in-place=.old instead of sed -i, so it leaves the previous file around just in case there are issues?

Thank you!

[erjoalgo]

Done

[albertito]

Can you replace this with:

if test -z "${EMAIL}"; then
        read -r -p "Email (full user@domain format): " EMAIL
fi

It should be the same effect, and I am okay with the change, I just find this form more readable in this case.

Is that okay with you? What do you think?

Thank you!

[erjoalgo]

Done

[albertito]

Thanks a lot for the explanation and the updated patches!

I've merged them to next with minor edits to the commit messages, please take a look and tell me if there's something you prefer me to change. They are a0f0930 and 0ce84a3.

Thanks again!

[erjoalgo]

Looks great to me. Thanks for the quick response.