Repository: ansible/ansible
Pull request: Add backup feature to user module
##### SUMMARY

Add feature to backup shadow file only on platforms where the module modifies the shadow file directly, which are currently SunOS variants.

Fixes #40696 

##### ISSUE TYPE
<!--- Pick one below and delete the rest: -->
 - Feature Pull Request

##### COMPONENT NAME
`user.py`

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes below -->
```
2.7
```


Problem statement:
user module should backup shadow to shadow-
##### SUMMARY
Tools that modify `/etc/shadow` (useradd, groupadd etc) all make a backup at `/etc/shadow-`. Ansible should also do the same. Thoughts?

##### ISSUE TYPE
 - Feature Idea

##### COMPONENT NAME
user

##### ANSIBLE VERSION
```
ansible 2.5.3
  config file = /home/lae/.ansible.cfg
  configured module search path = ['/home/lae/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python3.6/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 3.6.5 (default, May 11 2018, 04:00:52) [GCC 8.1.0]
```

##### OS / ENVIRONMENT
N/A

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

If these files are inaccurate, 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 --->
cc @sfromm
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
cc @samdoran
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
I like this idea. We could use the existing `backup_local` method which would add a timestamped backup. We would have to detail in the docs that this is only for the shadow file to keep the scope and expectations focused.
The user module does not normally edit /etc/shadow directly, it actually uses useradd/usermod or the tool appropriate for the platform targeted.
I verified that the shadow file is backed up to `shadow-` by the underlying tools, making this feature redundant:

```
----------. 1 root root 1016 Jun 18 12:02 shadow-
----------. 1 root root 1016 Jun 18 12:02 shadow.8033.2018-06-18@12:06:56~
----------. 1 root root 1019 Jun 18 12:06 shadow
```

I did implement this feature in [this branch](https://github.com/samdoran/ansible/commit/ecf4146293534d89f52bc31cae0d3af357c97b75), but I don't feel like it's needed for the reason @bcoca mentioned.

needs_info
I believe the SunOS support in user.py still does not make a backup copy of /etc/shadow and still edits the file directly (not via a CLI).  So this issue is still valid for that OS.
Okay, that's weird - I forget exactly where I had the issue where `shadow` wasn't backed up to `shadow-` (prompting me to open this issue), but I think it was a set of Ubuntu 14 hosts. I'll try checking on the weekend.
@ericwb You are correct. So I can implement this so that it only works for SunOS, or leave it globally. Thoughts?
only where we use it, depending on setup /etc/shadow might not even be touched with changes via the tools as you can store your password elsewhere

Commit message: Update docs on backup feature

Patch to review:
diff --git a/lib/ansible/modules/system/user.py b/lib/ansible/modules/system/user.py
index 94b3d685e823f9..82bf993a9d2a9e 100644
--- a/lib/ansible/modules/system/user.py
+++ b/lib/ansible/modules/system/user.py
@@ -194,6 +194,14 @@
         type: bool
         default: 'no'
         version_added: "2.4"
+    backup:
+        description:
+            - Whether to backup the shadow file if a change is made.
+            - Currently only affects SunOS variants since shadow file backup is handled by underlying tools
+              on other platforms.
+        type: bool
+        default: 'no'
+        version_added: "2.7"
 '''
 
 EXAMPLES = '''
@@ -414,6 +422,7 @@ def __init__(self, module):
         self.password_lock = module.params['password_lock']
         self.groups = None
         self.local = module.params['local']
+        self.backup = module.params['backup']
 
         if module.params['groups'] is not None:
             self.groups = ','.join(module.params['groups'])
@@ -438,6 +447,10 @@ def execute_command(self, cmd, use_unsafe_shell=False, data=None, obey_checkmode
             cmd = [str(x) for x in cmd]
             return self.module.run_command(cmd, use_unsafe_shell=use_unsafe_shell, data=data)
 
+    def backup_shadow(self):
+        if self.backup and not self.module.check_mode and self.SHADOWFILE:
+            return self.module.backup_local(self.SHADOWFILE)
+
     def remove_user_userdel(self):
         if self.local:
             command_name = 'luserdel'
@@ -1558,6 +1571,7 @@ def create_user(self):
         if not self.module.check_mode:
             # we have to set the password by editing the /etc/shadow file
             if self.password is not None:
+                self.backup_shadow()
                 minweeks, maxweeks, warnweeks = self.get_password_defaults()
                 try:
                     lines = []
@@ -1662,6 +1676,7 @@ def modify_user_usermod(self):
 
         # we have to set the password by editing the /etc/shadow file
         if self.update_password == 'always' and self.password is not None and info[1] != self.password:
+            self.backup_shadow()
             (rc, out, err) = (0, '', '')
             if not self.module.check_mode:
                 minweeks, maxweeks, warnweeks = self.get_password_defaults()
@@ -2353,6 +2368,7 @@ def main():
             state=dict(type='str', default='present', choices=['absent', 'present']),
             name=dict(type='str', required=True, aliases=['user']),
             uid=dict(type='str'),
+            backup=dict(type='bool', default=False),
             non_unique=dict(type='bool', default=False),
             group=dict(type='str'),
             groups=dict(type='list'),
@@ -2402,6 +2418,7 @@ def main():
     result = {}
     result['name'] = user.name
     result['state'] = user.state
+    result['backup'] = user.backup
     if user.state == 'absent':
         if user.user_exists():
             if module.check_mode:


Code review comments:
diff_hunk: @@ -194,6 +194,14 @@
         type: bool
         default: 'no'
         version_added: "2.4"
+    backup:
path: lib/ansible/modules/system/user.py
text: @user1:
i would not even make this an option, specially since it behaves differntly than other 'backup' options, we should just 'do this'

@author:
Ok. I'll just add a note then and remove the option.