[BUG] State user.present use shell for unless condition

[frenkye]

Description If you want to create user.present, with - shell: /bin/false it gets used for unless command run and by that it gets ignored.

By default mysql create user in debian like: mysql:x:119:128:MySQL Server,,,:/nonexistent:/bin/false

We create that before instalation, because we want to do some preseed operations, with owner setup. This worked without problem on version 3000.2. I was doing upgrades because of CVE to 3002.

Setup

mysql_user:
  user.present:
    - name: mysql
    - system: True
    - home: /nonexistent
    - createhome: False
    - shell: /bin/false
    - fullname: MySQL Server
    - unless: /bin/grep -i mysql /etc/passwd

Steps to Reproduce the behavior Use sls above and for diffrent runs try to change - home, by the unless user should not be changes becase he exists.

[WARNING ] Attempt to run a shell command with what may be an invalid shell! Check to ensure that the shell </bin/false> is valid for this user.
[INFO    ] Executing command '/bin/grep -i mysql /etc/passwd' in directory '/home/XXXXX'
[INFO    ] Executing command ['usermod', '-d', '/nonexistent', 'mysql'] in directory '/home/XXXXX'

if you comment shell setting or use shell like bash, unless works.

Expected behavior Shell should not be change by setting user shell at least this should be 2 diffrent options.

Versions Report

salt --versions-report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.) master and minions are same versions ``` Salt Version: Salt: 3002.2 Dependency Versions: cffi: Not Installed cherrypy: Not Installed dateutil: 2.7.3 docker-py: Not Installed gitdb: Not Installed gitpython: Not Installed Jinja2: 2.10 libgit2: Not Installed M2Crypto: Not Installed Mako: Not Installed msgpack: 0.5.6 msgpack-pure: Not Installed mysql-python: Not Installed pycparser: Not Installed pycrypto: 2.6.1 pycryptodome: 3.6.1 pygit2: Not Installed Python: 3.7.3 (default, Dec 20 2019, 18:57:59) python-gnupg: Not Installed PyYAML: 3.13 PyZMQ: 17.1.2 smmap: Not Installed timelib: Not Installed Tornado: 4.5.3 ZMQ: 4.3.1 System Versions: dist: debian 10 buster locale: UTF-8 machine: x86_64 release: 4.19.0-10-amd64 system: Linux version: Debian GNU/Linux 10 buster ```

Additional context Tried to find this in other issues, but cannot find one. If this is duplicity i apologize.

[twangboy]

Looks like it's being caused by the code here:

https://github.com/saltstack/salt/blob/master/salt/state.py#L890

        if "shell" in low_data:
            cmd_opts["shell"] = low_data["shell"]
        elif "shell" in self.opts["grains"]:
            cmd_opts["shell"] = self.opts["grains"].get("shell")

where it's pulling shell as defined in the state file and passing that to the cmd.run for unless https://github.com/saltstack/salt/blob/master/salt/state.py#L902

        if "unless" in low_data:
            _ret = self._run_check_unless(low_data, cmd_opts)
            # If either result is True, the returned result should be True
            ret["result"] = _ret["result"] or ret["result"]
            ret["comment"].append(_ret["comment"])
            if "skip_watch" in _ret:
                # If either result is True, the returned result should be True
                ret["skip_watch"] = _ret["skip_watch"] or ret["skip_watch"]

So, the solution here may be to rename the option from shell to user_shell or something...

[s0undt3ch]

Proposed fix in https://github.com/saltstack/salt/issues/60555#issue-945417831 which was closed in favor of this issue

[agraul]

@s0undt3ch @twangboy do you have a preferred way to fix this? Are you in favor of a local change, e.g. renaming shell -> login_shell or a more general one like a flag indicating what args (not) to pass to cmd.retcode as described in https://github.com/saltstack/salt/pull/57825 under "Risks"?