Repository: ansible/ansible
Pull request: Check if skeleton is /dev/null while creating home folder
##### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Add additional check for skeleton is a valid directory in addition to the pure existence of the path.

This prevents `shutil.copytree()` to throw an exception and also allows the use of `/dev/null` as a valid skeleton, which results in a empty home-directory to be created.

Fixes #75063

<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below -->
Fix in User-Module.

##### ADDITIONAL INFORMATION
<!--- Include additional information to help people understand the change here -->
<!--- A step-by-step reproduction of the problem is helpful if there is no related issue -->

<!--- Paste verbatim command output below, e.g. before and after your change -->
```paste below

```


Problem statement:
/dev/null must be allowed for skeleton in user module
### Summary

If you set `skeleton` to `/dev/null` in `user` module you get an error saying that it's not a directory. However it's perfectly fine to do `useradd --skel /dev/null`, so ansible shouldn't be limiting the functionality of the `useradd` command.

`useradd --skel /dev/null` will cause nothing to be added to the new home directoy. It used to work correctly in older versions of ansible and should be fixed in this version.

### Issue Type

Bug Report

### Component Name

user

### Ansible Version

```console
$ ansible --version
2.11.1
```


### Configuration

```console
$ ansible-config dump --only-changed
```


### OS / Environment

linux

### Steps to Reproduce

<!--- Paste example playbooks or commands between quotes below -->
```yaml (paste below)

```


### Expected Results

`/dev/null` should be allowed and result in no files being added to new home directory

### Actual Results

```console
It caused an error saying that /dev/null is not a directory
```


### Code of Conduct

- [X] I agree to follow the Ansible Code of Conduct

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/modules/user.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/user.py)

If these files are incorrect, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->

Commit message: Check if skeleton is directory while creating home folder

Allow /dev/null as skeleton.

Patch to review:
diff --git a/changelogs/fragments/75063-allow-dev-nul-as-skeleton-for-new-homedir.yml b/changelogs/fragments/75063-allow-dev-nul-as-skeleton-for-new-homedir.yml
new file mode 100644
index 00000000000000..e492384dec6cc9
--- /dev/null
+++ b/changelogs/fragments/75063-allow-dev-nul-as-skeleton-for-new-homedir.yml
@@ -0,0 +1,2 @@
+bugfixes:
+  - modules/user.py - Add check for valid directory when creating new user homedir (allows /dev/null as skeleton) (https://github.com/ansible/ansible/issues/75063)
\ No newline at end of file
diff --git a/lib/ansible/modules/user.py b/lib/ansible/modules/user.py
index 6d22a5f41cf7cd..24772b0b4466f4 100644
--- a/lib/ansible/modules/user.py
+++ b/lib/ansible/modules/user.py
@@ -1247,7 +1247,7 @@ def create_homedir(self, path):
             else:
                 skeleton = '/etc/skel'
 
-            if os.path.exists(skeleton):
+            if os.path.exists(skeleton) and os.path.isdir(skeleton):
                 try:
                     shutil.copytree(skeleton, path, symlinks=True)
                 except OSError as e:
diff --git a/test/integration/targets/user/tasks/test_create_user_home.yml b/test/integration/targets/user/tasks/test_create_user_home.yml
index 1b529f76be3853..a2d90486e989ae 100644
--- a/test/integration/targets/user/tasks/test_create_user_home.yml
+++ b/test/integration/targets/user/tasks/test_create_user_home.yml
@@ -134,3 +134,19 @@
     name: randomuser
     state: absent
     remove: yes
+
+# https://github.com/ansible/ansible/issues/75063
+# Create user home directory with /dev/null as skeleton
+- name: "Create user home directory with /dev/null as skeleton"
+  user:
+    name: withskeleton
+    state: present
+    skeleton: "/dev/null"
+    createhome: yes
+  register: create_user_with_skeleton_dev_null
+
+- name: "Remove test user"
+  user:
+    name: withskeleton
+    state: absent
+    remove: yes


Code review comments:
diff_hunk: @@ -1247,7 +1247,7 @@ def create_homedir(self, path):
             else:
                 skeleton = '/etc/skel'
 
-            if os.path.exists(skeleton):
+            if os.path.exists(skeleton) and os.path.isdir(skeleton):
path: lib/ansible/modules/user.py
text: @user1:
This change looks like it would just silently ignore `skeleton` if it wasn't a dir.  We effectively relied on the `OSError` to be the thing that errored if someone provided a file for example.

As such, this probably needs to add additional error checking.  Probably by means of also using `stat.S_ISCHR` to account for `/dev/null`

@author:
Yes I see, skipping over in general is not a good idea if You have to rely on `OSError` if a file is provided.

`stat.S_ISCHR` is maybe a bit to general since it would return `True` also on other device-files, which is surely not welcome. Also, adding a lot of extra checking and error-handling just to make sure we just filter out `/dev/null` seems to be a bit to much for what we want to archive.

Maybe this suggestion takes us a bit closer? This way file-paths still bleed through and get caught while executing `shutil.copytree()`, also the intention is clearer:
```suggestion
            if os.path.exists(skeleton) and skeleton != os.devnull:
```

@user2:
I think the suggested change is ok and should account for sivel's concerns there.