Silent failure on error to create user

[GregoryEAllen]

The installer should report an error if:

• the username exists, but has a different uid, or
• the uid exists, but has a different username

[GregoryEAllen]

Actually, since createuser finds by username, the first case isn't necessarily a problem. It will update the user without changing the uid. The uid that was provided is simply ignored.

[gregneagle]

"The installer should report an error" is ambiguous.

Packages can be installed by double-clicking and using Installer.app, or via /usr/sbin/installer. I agree that either method should return a failure to install if creating the user fails. That is typically done by returning non-zero in the postinstall script.

How to make the error detail discoverable is a different (though related) problem -- but the answer to that is different if the package is being installed via Installer.app (the less interesting to me strategy) or via /usr/sbin/installer.

[gregneagle]

"Actually, since createuser finds by username, the first case isn't necessarily a problem. It will update the user without changing the uid. The uid that was provided is simply ignored."

That is an intended use case -- admins can use a package to, say, create a local admin account, and then revise (and reinstall) the package to update the password for this same local admin account.

[GregoryEAllen]

Agreed, and it might be nice to know that you didn't get the uid that you specified.

Here are the two conditions:

    # uid already exists, but username does not
    if id -u $USERID >/dev/null 2>&1 && ! id -un $USERNAME >/dev/null 2>&1; then
        >&2 echo "error: uid $USERID already exists as user \"$(id -un $USERID)\""
        exit -1
    fi
    # username exists, but uid does not match
    if id -un $USERNAME >/dev/null 2>&1 && [[ $(id -u $USERNAME) != $USERID ]]; then
        >&2 echo "warning: \"$USERNAME\" has uid $(id -u $USERNAME), ignoring uid $USERID"
    fi

Perhaps you'd prefer to do nothing in the second case.

[GregoryEAllen]

I think echoing the error and exit -1 is appropriate for a CLI install, but not very friendly for a GUI install. I could osascript a dialog if I knew how to tell which way it's being installed.

[GregoryEAllen]

From man installer:

ENVIRONMENT
     COMMAND_LINE_INSTALL  Set when performing an installation using the
                           installer command.

[GregoryEAllen]

Even better: createuser is already reporting and returning an error. It's just not getting checked.

[GregoryEAllen]

Unfortuately, createuser leaves a broken user when it fails. The above tests are better, unless we make changes to createuser.

[gregneagle]

Well, yes, a broken user account is a failure. If the user account was not broken, there would be no failure. I think maybe you are trying to use this tool in way I didn't design it for (which is fine -- but it might mean you'll want to fork it to better meet your specific needs). Maybe you could more fully describe your scenarios here.

I designed this tool to create a package that could be used to create a user account when a machine is first setup/configured, and to (optionally) keep the password for that same user account updated over time. Typically there are no local user account present when this package is initially installed, so UID collision is not an expected issue (unless you choose UIDs < 501).

It's not really designed to create a user account on a machine that might have been deployed some time ago any might have any number of local user accounts with an unpredictable set of used UIDs. That problem is definitely solvable, but not one I needed to solve.

I might accept PRs that solve whatever issues you need solved -- but I have to make sure those changes don't negatively affect my required scenarios, and that I'm comfortable supporting the changes going forward. I don't really love supporting code that I don't myself use, as I won't notice if/when it breaks, and I don't always know how to fix it.

[GregoryEAllen]

The error handing in createuser has a problem.

In createUserRecord when createRecordWithRecordType fails a message is reported, but the erroneous record is still returned.

Then in main, setAttributesForUser gets called on the same erroneous record.

Perhaps createUserRecord should set the erroneous record to nil?

Edit: I tried it. The record still gets created, but is now empty.