diff --git a/changelogs/fragments/1724-various-fixes-for-updating-existing-gitlab-user.yml b/changelogs/fragments/1724-various-fixes-for-updating-existing-gitlab-user.yml new file mode 100644 index 0000000000..eab67e0f47 --- /dev/null +++ b/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). diff --git a/plugins/modules/source_control/gitlab/gitlab_user.py b/plugins/modules/source_control/gitlab/gitlab_user.py index 458a4734b2..1e8ee65a67 100644 --- a/plugins/modules/source_control/gitlab/gitlab_user.py +++ b/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, { - 'name': options['name'], - 'email': options['email'], - 'is_admin': options['isadmin'], - 'external': options['external']}) + changed, user = self.updateUser( + self.userObject, { + # add "normal" parameters here, put uncheckable + # params in the dict below + '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 self._module.check_mode: - self._module.exit_json(changed=True, msg="Successfully created or updated the user %s" % username) - + if (changed or potentionally_changed) and not self._module.check_mode: 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: - if getattr(user, arg_key) != arguments[arg_key]: - setattr(user, arg_key, arguments[arg_key]) + av = arg_value['value'] + + 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) ''' diff --git a/tests/integration/targets/gitlab_user/tasks/main.yml b/tests/integration/targets/gitlab_user/tasks/main.yml index 6408a27f37..6cbcd14c34 100644 --- a/tests/integration/targets/gitlab_user/tasks/main.yml +++ b/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 diff --git a/tests/unit/plugins/modules/source_control/gitlab/test_gitlab_user.py b/tests/unit/plugins/modules/source_control/gitlab/test_gitlab_user.py index f78f0efb71..4a47654a8c 100644 --- a/tests/unit/plugins/modules/source_control/gitlab/test_gitlab_user.py +++ b/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):