Various fixes for updating existing gitlab users (#1724) (#1757)

* fixes various issues related to updating an ...

... existing gitlab user, in detail:

  - fixes updating admin status not working
  - fixes user passwords not updated
  - fixes confirmation skipping param ignored for user updates
  - added tests for code changes

* fixing sanity issues

* fixing sanity issues 02

* fixing sanity issues 03

* fixing sanity issues 04

* fixing unit test failures

* fixing unit test failures 02

* add changelog fragment

* fixing unit test failures 03

* forgot to add changelog fragment

* fix changelog sanity issues

* fix changelog sanity issues 02

* incorporate review suggestions

Co-authored-by: Mirko Wilhelmi <Mirko.Wilhelmi@sma.de>
(cherry picked from commit c03ae754d2)

Co-authored-by: morco <thegreatwiper@web.de>

changelogs/fragments/1724-various-fixes-for-updating-existing-gitlab-user.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - gitlab_user - make updates to the ``isadmin``, ``password`` and ``confirm`` options of an already existing GitLab user work (https://github.com/ansible-collections/community.general/pull/1724).

plugins/modules/source_control/gitlab/gitlab_user.py

@@ -205,6 +205,7 @@ class GitLabUser(object):
     '''
     def createOrUpdateUser(self, username, options):
         changed = False
+        potentionally_changed = False
 
         # Because we have already call userExists in main()
         if self.userObject is None:
@@ -218,11 +219,36 @@ class GitLabUser(object):
                 'external': options['external']})
             changed = True
         else:
-            changed, user = self.updateUser(self.userObject, {
+            changed, user = self.updateUser(
-                'name': options['name'],
+                self.userObject, {
-                'email': options['email'],
+                    # add "normal" parameters here, put uncheckable
-                'is_admin': options['isadmin'],
+                    # params in the dict below
-                'external': options['external']})
+                    'name': {'value': options['name']},
+                    'email': {'value': options['email']},
+
+                    # note: for some attributes like this one the key
+                    #   from reading back from server is unfortunately
+                    #   different to the one needed for pushing/writing,
+                    #   in that case use the optional setter key
+                    'is_admin': {
+                        'value': options['isadmin'], 'setter': 'admin'
+                    },
+                    'external': {'value': options['external']},
+                },
+                {
+                    # put "uncheckable" params here, this means params
+                    # which the gitlab does accept for setting but does
+                    # not return any information about it
+                    'skip_reconfirmation': {'value': not options['confirm']},
+                    'password': {'value': options['password']},
+                }
+            )
+
+            # note: as we unfortunately have some uncheckable parameters
+            #   where it is not possible to determine if the update
+            #   changed something or not, we must assume here that a
+            #   changed happend and that an user object update is needed
+            potentionally_changed = True
 
         # Assign ssh keys
         if options['sshkey_name'] and options['sshkey_file']:
@@ -237,14 +263,15 @@ class GitLabUser(object):
             changed = changed or group_changed
 
         self.userObject = user
-        if changed:
+        if (changed or potentionally_changed) and not self._module.check_mode:
-            if self._module.check_mode:
-                self._module.exit_json(changed=True, msg="Successfully created or updated the user %s" % username)
-
             try:
                 user.save()
             except Exception as e:
                 self._module.fail_json(msg="Failed to update user: %s " % to_native(e))
+
+        if changed:
+            if self._module.check_mode:
+                self._module.exit_json(changed=True, msg="Successfully created or updated the user %s" % username)
             return True
         else:
             return False
@@ -348,15 +375,23 @@ class GitLabUser(object):
     @param user User object
     @param arguments User attributes
     '''
-    def updateUser(self, user, arguments):
+    def updateUser(self, user, arguments, uncheckable_args):
         changed = False
 
         for arg_key, arg_value in arguments.items():
-            if arguments[arg_key] is not None:
+            av = arg_value['value']
-                if getattr(user, arg_key) != arguments[arg_key]:
+
-                    setattr(user, arg_key, arguments[arg_key])
+            if av is not None:
+                if getattr(user, arg_key) != av:
+                    setattr(user, arg_value.get('setter', arg_key), av)
                     changed = True
 
+        for arg_key, arg_value in uncheckable_args.items():
+            av = arg_value['value']
+
+            if av is not None:
+                setattr(user, arg_value.get('setter', arg_key), av)
+
         return (changed, user)
 
     '''

tests/integration/targets/gitlab_user/tasks/main.yml

@@ -10,25 +10,25 @@
 
 - name: Clean up gitlab user
   gitlab_user:
-    server_url: "{{ gitlab_host }}"
+    api_url: "{{ gitlab_host }}"
     name: ansible_test_user
     username: ansible_test_user
     password: Secr3tPassw00rd
     email: root@localhost
     validate_certs: false
-    login_token: "{{ gitlab_login_token }}"
+    api_token: "{{ gitlab_login_token }}"
     state: absent
 
 
 - name: Create gitlab user
   gitlab_user:
-    server_url: "{{ gitlab_host }}"
+    api_url: "{{ gitlab_host }}"
     email: "{{ gitlab_user_email }}"
     name: "{{ gitlab_user }}"
     username: "{{ gitlab_user }}"
     password: "{{ gitlab_user_pass }}"
     validate_certs: False
-    login_token: "{{ gitlab_login_token }}"
+    api_token: "{{ gitlab_login_token }}"
     state: present
   register: gitlab_user_state
 
@@ -39,13 +39,13 @@
 
 - name: Create gitlab user again
   gitlab_user:
-    server_url: "{{ gitlab_host }}"
+    api_url: "{{ gitlab_host }}"
     email: root@localhost
     name: ansible_test_user
     username: ansible_test_user
     password: Secr3tPassw00rd
     validate_certs: False
-    login_token: "{{ gitlab_login_token }}"
+    api_token: "{{ gitlab_login_token }}"
     state: present
   register: gitlab_user_state_again
 
@@ -53,3 +53,198 @@
   assert:
     that:
       - gitlab_user_state_again is not changed
+      - gitlab_user_state_again.user.is_admin == False
+
+
+- name: Update User Test => Make User Admin 
+  gitlab_user:
+    api_url: "{{ gitlab_host }}"
+    email: "{{ gitlab_user_email }}"
+    name: "{{ gitlab_user }}"
+    username: "{{ gitlab_user }}"
+    isadmin: true
+    validate_certs: False
+    api_token: "{{ gitlab_login_token }}"
+    state: present
+  register: gitlab_user_state
+
+- name: Check if user is admin now
+  assert:
+    that:
+      - gitlab_user_state is changed
+      - gitlab_user_state.user.is_admin == True
+
+- name: Update User Test => Make User Admin (Again)
+  gitlab_user:
+    api_url: "{{ gitlab_host }}"
+    email: "{{ gitlab_user_email }}"
+    name: "{{ gitlab_user }}"
+    username: "{{ gitlab_user }}"
+    isadmin: true
+    validate_certs: False
+    api_token: "{{ gitlab_login_token }}"
+    state: present
+  register: gitlab_user_state
+
+- name: Check state is not changed
+  assert:
+    that:
+      - gitlab_user_state is not changed
+      - gitlab_user_state.user.is_admin == True
+
+- name: Update User Test => Remove Admin Rights
+  gitlab_user:
+    api_url: "{{ gitlab_host }}"
+    email: "{{ gitlab_user_email }}"
+    name: "{{ gitlab_user }}"
+    username: "{{ gitlab_user }}"
+    isadmin: false
+    validate_certs: False
+    api_token: "{{ gitlab_login_token }}"
+    state: present
+  register: gitlab_user_state
+
+- name: Check if user is not admin anymore
+  assert:
+    that:
+      - gitlab_user_state is changed
+      - gitlab_user_state.user.is_admin == False
+
+
+- name: Update User Test => Try Changing Mail without Confirmation Skipping
+  gitlab_user:
+    api_url: "{{ gitlab_host }}"
+    email: foo@bar.baz
+    name: "{{ gitlab_user }}"
+    username: "{{ gitlab_user }}"
+    confirm: True
+    validate_certs: False
+    api_token: "{{ gitlab_login_token }}"
+    state: present
+  register: gitlab_user_state
+
+- name: Check that eMail is unchanged (Only works with confirmation skipping)
+  assert:
+    that:
+      - gitlab_user_state is changed
+      - gitlab_user_state.user.email == gitlab_user_email
+
+- name: Update User Test => Change Mail with Confirmation Skip
+  gitlab_user:
+    api_url: "{{ gitlab_host }}"
+    email: foo@bar.baz
+    name: "{{ gitlab_user }}"
+    username: "{{ gitlab_user }}"
+    confirm: false
+    validate_certs: False
+    api_token: "{{ gitlab_login_token }}"
+    state: present
+  register: gitlab_user_state
+
+- name: Check that mail has changed now
+  assert:
+    that:
+      - gitlab_user_state is changed
+      - gitlab_user_state.user.email == 'foo@bar.baz'
+
+- name: Update User Test => Change Mail with Confirmation Skip (Again)
+  gitlab_user:
+    api_url: "{{ gitlab_host }}"
+    email: foo@bar.baz
+    name: "{{ gitlab_user }}"
+    username: "{{ gitlab_user }}"
+    confirm: false
+    validate_certs: False
+    api_token: "{{ gitlab_login_token }}"
+    state: present
+  register: gitlab_user_state
+
+- name: Check state is not changed
+  assert:
+    that:
+      - gitlab_user_state is not changed
+      - gitlab_user_state.user.email == 'foo@bar.baz'
+
+- name: Update User Test => Revert to original Mail Address
+  gitlab_user:
+    api_url: "{{ gitlab_host }}"
+    email: "{{ gitlab_user_email }}"
+    name: "{{ gitlab_user }}"
+    username: "{{ gitlab_user }}"
+    confirm: false
+    validate_certs: False
+    api_token: "{{ gitlab_login_token }}"
+    state: present
+  register: gitlab_user_state
+
+- name: Check that reverting mail back to original has worked
+  assert:
+    that:
+      - gitlab_user_state is changed
+      - gitlab_user_state.user.email == gitlab_user_email
+
+
+- name: Update User Test => Change User Password
+  gitlab_user:
+    api_url: "{{ gitlab_host }}"
+    validate_certs: False
+
+    # note: the only way to check if a password really is what it is expected 
+    #   to be is to use it for login, so we use it here instead of the 
+    #   default token assuming that a user can always change its own password
+    api_username: "{{ gitlab_user }}"
+    api_password: "{{ gitlab_user_pass }}"
+
+    email: "{{ gitlab_user_email }}"
+    name: "{{ gitlab_user }}"
+    username: "{{ gitlab_user }}"
+    password: new-super-password
+    state: present
+  register: gitlab_user_state
+
+- name: Check PW setting return state
+  assert:
+    that:
+        # note: there is no way to determine if a password has changed or 
+        #   not, so it can only be always yellow or always green, we 
+        #   decided for always green for now
+      - gitlab_user_state is not changed
+
+- name: Update User Test => Reset User Password
+  gitlab_user:
+    api_url: "{{ gitlab_host }}"
+    validate_certs: False
+
+    api_username: "{{ gitlab_user }}"
+    api_password: new-super-password
+
+    email: "{{ gitlab_user_email }}"
+    name: "{{ gitlab_user }}"
+    username: "{{ gitlab_user }}"
+    password: "{{ gitlab_user_pass }}"
+    state: present
+  register: gitlab_user_state
+
+- name: Check PW setting return state (Again)
+  assert:
+    that:
+      - gitlab_user_state is not changed
+
+- name: Update User Test => Check that password was reset
+  gitlab_user:
+    api_url: "{{ gitlab_host }}"
+    validate_certs: False
+
+    api_username: "{{ gitlab_user }}"
+    api_password: "{{ gitlab_user_pass }}"
+
+    email: "{{ gitlab_user_email }}"
+    name: "{{ gitlab_user }}"
+    username: "{{ gitlab_user }}"
+    state: present
+  register: gitlab_user_state
+
+- name: Check PW setting return state (Reset)
+  assert:
+    that:
+      - gitlab_user_state is not changed

tests/unit/plugins/modules/source_control/gitlab/test_gitlab_user.py

@@ -88,16 +88,33 @@ class TestGitlabUser(GitlabModuleTestCase):
     @with_httmock(resp_get_user)
     def test_update_user(self):
         user = self.gitlab_instance.users.get(1)
-        changed, newUser = self.moduleUtil.updateUser(user, {'name': "Jack Smith", "is_admin": "true"})
+
+        changed, newUser = self.moduleUtil.updateUser(
+            user,
+            {'name': {'value': "Jack Smith"}, "is_admin": {'value': "true", 'setter': 'admin'}}, {}
+        )
 
         self.assertEqual(changed, True)
         self.assertEqual(newUser.name, "Jack Smith")
-        self.assertEqual(newUser.is_admin, "true")
+        self.assertEqual(newUser.admin, "true")
 
-        changed, newUser = self.moduleUtil.updateUser(user, {'name': "Jack Smith"})
+        changed, newUser = self.moduleUtil.updateUser(user, {'name': {'value': "Jack Smith"}}, {})
 
         self.assertEqual(changed, False)
 
+        changed, newUser = self.moduleUtil.updateUser(
+            user,
+            {}, {
+                'skip_reconfirmation': {'value': True},
+                'password': {'value': 'super_secret-super_secret'},
+            }
+        )
+
+        # note: uncheckable parameters dont set changed state
+        self.assertEqual(changed, False)
+        self.assertEqual(newUser.skip_reconfirmation, True)
+        self.assertEqual(newUser.password, 'super_secret-super_secret')
+
     @with_httmock(resp_find_user)
     @with_httmock(resp_delete_user)
     def test_delete_user(self):