Repository: ansible/ansible
Pull request: Fix AWS iam_user remove
##### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->

<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
Fixes #59078 

As Boto3 documentation tells (https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/iam.html#IAM.Client.delete_user) before delete an user it is needed to delete some user's associated resources, like login profile(console password), access keys, etc..

> 
    Password ( DeleteLoginProfile )
    Access keys ( DeleteAccessKey )
    Signing certificate ( DeleteSigningCertificate )
    SSH public key ( DeleteSSHPublicKey )
    Git credentials ( DeleteServiceSpecificCredential )
    Multi-factor authentication (MFA) device ( DeactivateMFADevice , DeleteVirtualMFADevice )
    Inline policies ( DeleteUserPolicy )
    Attached managed policies ( DetachUserPolicy )
    Group memberships ( RemoveUserFromGroup )

The current implementation of `iam_user` don't take most of this into account so user's removal fails.
This PR removes all the user's associated resources before deletes the user.

**Rationale of PR**:
- I refactor the code a bit to be more clear.
- I don't include every step in it's own try/except as I don't believe it adds value over the already present: 'user can't be removed' with included stack-trace.


##### 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 -->
iam_user

##### 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 -->

To test this I've created manually the following IAM users and remove them with the `iam_user` module of this PR:

- test1 -> simple user with access keys
- test2 -> simple user with login profile (console password)
- test3 -> user with: access key, group membership, virtual MFA, attached policy, ssh public key, signing certificate, git https credentials.
- test4 -> user with: login profile, group membership, U2F MFA, inline policy, ssh public key, git https credentials.


Problem statement:
iam_user module fails to remove an AWS user
<!--- Verify first that your issue is not already reported on GitHub -->
<!--- Also test if the latest release and devel branch are affected too -->
<!--- Complete *all* sections as described, this form is processed automatically -->

##### SUMMARY
<!--- Explain the problem briefly below -->
`iam_user` module fails to remove am AWS user if it has: a group membership, console password (login profile), access keys, MFA device, etc..

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below, use your best guess if unsure -->
iam_user

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes -->
```paste below
ansible 2.8.2
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/home/agustin/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = .../venv/lib/python3.5/site-packages/ansible
  executable location =.../venv/bin/ansible
  python version = 3.5.7 (default, Mar 30 2019, 01:05:18) [GCC 7.3.0]
```

##### CONFIGURATION
<!--- Paste verbatim output from "ansible-config dump --only-changed" between quotes -->
```paste below

```

##### OS / ENVIRONMENT
<!--- Provide all relevant information below, e.g. target OS versions, network device firmware, etc. -->


##### STEPS TO REPRODUCE
<!--- Describe exactly how to reproduce the problem, using a minimal test-case -->
Create an AWS user `test` with password or access keys

<!--- Paste example playbooks or commands between quotes below -->
```yaml
- hosts: localhost
  tasks:
    - iam_user:
        name: test
        state: absent
```

<!--- HINT: You can paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- Describe what you expected to happen when running the steps above -->
User's state = absent

##### ACTUAL RESULTS
<!--- Describe what actually happened. If possible run with extra verbosity (-vvvv) -->

<!--- Paste verbatim command output between quotes -->
```paste below
TASK [iam_user] **************************************************************************************************************************************
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: botocore.errorfactory.DeleteConflictException: An error occurred (DeleteConflict) when calling the DeleteUser operation: Cannot delete entity, must delete login profile first.
fatal: [localhost]: FAILED! => {"changed": false, "error": {"code": "DeleteConflict", "message": "Cannot delete entity, must delete login profile first.", "type": "Sender"}, "msg": "Unable to delete user test: An error occurred (DeleteConflict) when calling the DeleteUser operation: Cannot delete entity, must delete login profile first.", "response_metadata": {"http_headers": {"content-length": "300", "content-type": "text/xml", "date": "Sun, 14 Jul 2019 18:34:25 GMT", "x-amzn-requestid": "01da0109-a666-11e9-adeb-83c56b62b76d"}, "http_status_code": 409, "request_id": "01da0109-a666-11e9-adeb-83c56b62b76d", "retry_attempts": 0}}

```

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/modules/cloud/amazon/iam_user.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/amazon/iam_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 @joshsouza
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
@tinproject, just so you are aware we have a dedicated Working Group for aws.
You can find other people interested in this in `#ansible-aws` on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

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

Commit message: Make iam_user not fail on user deletion.

- Use guard clause on already absent user
- Refactor, use variable instead nested dict
- Ensure needed prerequisites for boto3 delete_user successfully
- Use AnsibleAWSModule on iam_user.
- Fix fail_json_aws calls

Patch to review:
diff --git a/lib/ansible/modules/cloud/amazon/iam_user.py b/lib/ansible/modules/cloud/amazon/iam_user.py
index f5d319405e3eca..12d917298ddc32 100644
--- a/lib/ansible/modules/cloud/amazon/iam_user.py
+++ b/lib/ansible/modules/cloud/amazon/iam_user.py
@@ -99,7 +99,7 @@
 '''
 
 from ansible.module_utils._text import to_native
-from ansible.module_utils.basic import AnsibleModule
+from ansible.module_utils.aws.core import AnsibleAWSModule
 from ansible.module_utils.ec2 import camel_dict_to_snake_dict, ec2_argument_spec, get_aws_connection_info, boto3_conn
 from ansible.module_utils.ec2 import HAS_BOTO3
 
@@ -227,38 +227,81 @@ def create_or_update_user(connection, module):
 
 def destroy_user(connection, module):
 
-    params = dict()
-    params['UserName'] = module.params.get('name')
+    user_name = module.params.get('name')
 
-    if get_user(connection, module, params['UserName']):
-        # Check mode means we would remove this user
-        if module.check_mode:
-            module.exit_json(changed=True)
+    user = get_user(connection, module, user_name)
+    # User is not present
+    if not user:
+        module.exit_json(changed=False)
 
-        # Remove any attached policies otherwise deletion fails
-        try:
-            for policy in get_attached_policy_list(connection, module, params['UserName']):
-                connection.detach_user_policy(UserName=params['UserName'], PolicyArn=policy['PolicyArn'])
-        except ClientError as e:
-            module.fail_json(msg="Unable to detach policy {0} from user {1}: {2}".format(
-                             policy['PolicyArn'], params['UserName'], to_native(e)),
-                             exception=traceback.format_exc(), **camel_dict_to_snake_dict(e.response))
-        except ParamValidationError as e:
-            module.fail_json(msg="Unable to detach policy {0} from user {1}: {2}".format(
-                             policy['PolicyArn'], params['UserName'], to_native(e)),
-                             exception=traceback.format_exc())
+    # Check mode means we would remove this user
+    if module.check_mode:
+        module.exit_json(changed=True)
 
-        try:
-            connection.delete_user(**params)
-        except ClientError as e:
-            module.fail_json(msg="Unable to delete user {0}: {1}".format(params['UserName'], to_native(e)),
-                             exception=traceback.format_exc(), **camel_dict_to_snake_dict(e.response))
-        except ParamValidationError as e:
-            module.fail_json(msg="Unable to delete user {0}: {1}".format(params['UserName'], to_native(e)),
-                             exception=traceback.format_exc())
+    # Remove any attached policies otherwise deletion fails
+    try:
+        for policy in get_attached_policy_list(connection, module, user_name):
+            connection.detach_user_policy(UserName=user_name, PolicyArn=policy['PolicyArn'])
+    except ClientError as e:
+        module.fail_json(msg="Unable to detach policy {0} from user {1}: {2}".format(
+                         policy['PolicyArn'], user_name, to_native(e)),
+                         exception=traceback.format_exc(), **camel_dict_to_snake_dict(e.response))
+    except ParamValidationError as e:
+        module.fail_json(msg="Unable to detach policy {0} from user {1}: {2}".format(
+                         policy['PolicyArn'], user_name, to_native(e)),
+                         exception=traceback.format_exc())
 
-    else:
-        module.exit_json(changed=False)
+    try:
+        # Remove user's access keys
+        access_keys = connection.list_access_keys(UserName=user_name)["AccessKeyMetadata"]
+        for access_key in access_keys:
+            connection.delete_access_key(UserName=user_name, AccessKeyId=access_key["AccessKeyId"])
+
+        # Remove user's login profile (console password)
+        delete_user_login_profile(connection, module, user_name)
+
+        # Remove user's ssh public keys
+        ssh_public_keys = connection.list_ssh_public_keys(UserName=user_name)["SSHPublicKeys"]
+        for ssh_public_key in ssh_public_keys:
+            connection.delete_ssh_public_key(UserName=user_name, SSHPublicKeyId=ssh_public_key["SSHPublicKeyId"])
+
+        # Remove user's service specific credentials
+        service_credentials = connection.list_service_specific_credentials(UserName=user_name)["ServiceSpecificCredentials"]
+        for service_specific_credential in service_credentials:
+            connection.delete_service_specific_credential(
+                UserName=user_name,
+                ServiceSpecificCredentialId=service_specific_credential["ServiceSpecificCredentialId"]
+            )
+
+        # Remove user's signing certificates
+        signing_certificates = connection.list_signing_certificates(UserName=user_name)["Certificates"]
+        for signing_certificate in signing_certificates:
+            connection.delete_signing_certificate(
+                UserName=user_name,
+                CertificateId=signing_certificate["CertificateId"]
+            )
+
+        # Remove user's MFA devices
+        mfa_devices = connection.list_mfa_devices(UserName=user_name)["MFADevices"]
+        for mfa_device in mfa_devices:
+            connection.deactivate_mfa_device(UserName=user_name, SerialNumber=mfa_device["SerialNumber"])
+
+        # Remove user's inline policies
+        inline_policies = connection.list_user_policies(UserName=user_name)["PolicyNames"]
+        for policy_name in inline_policies:
+            connection.delete_user_policy(UserName=user_name, PolicyName=policy_name)
+
+        # Remove user's group membership
+        user_groups = connection.list_groups_for_user(UserName=user_name)["Groups"]
+        for group in user_groups:
+            connection.remove_user_from_group(UserName=user_name, GroupName=group["GroupName"])
+
+        connection.delete_user(UserName=user_name)
+    except ClientError as e:
+        module.fail_json(msg="Unable to delete user {0}: {1}".format(user_name, to_native(e)),
+                         exception=traceback.format_exc(), **camel_dict_to_snake_dict(e.response))
+    except ParamValidationError as e:
+        module.fail_json_aws(e, msg="Unable to delete user {0}".format(user_name))
 
     module.exit_json(changed=True)
 
@@ -286,8 +329,18 @@ def get_attached_policy_list(connection, module, name):
         if e.response['Error']['Code'] == 'NoSuchEntity':
             return None
         else:
-            module.fail_json(msg="Unable to get policies for user {0}: {1}".format(name, to_native(e)),
-                             **camel_dict_to_snake_dict(e.response))
+            module.fail_json_aws(e, msg="Unable to get policies for user {0}".format(name))
+
+
+def delete_user_login_profile(connection, module, user_name):
+
+    try:
+        return connection.delete_login_profile(UserName=user_name)
+    except ClientError as e:
+        if e.response["Error"]["Code"] == "NoSuchEntity":
+            return None
+        else:
+            module.fail_json_aws(e, msg="Unable to delete login profile for user {0}".format(user_name))
 
 
 def main():
@@ -302,7 +355,7 @@ def main():
         )
     )
 
-    module = AnsibleModule(
+    module = AnsibleAWSModule(
         argument_spec=argument_spec,
         supports_check_mode=True
     )


Code review comments:
diff_hunk: @@ -227,38 +227,81 @@ def create_or_update_user(connection, module):
 
 def destroy_user(connection, module):
 
-    params = dict()
-    params['UserName'] = module.params.get('name')
+    user_name = module.params.get('name')
 
-    if get_user(connection, module, params['UserName']):
-        # Check mode means we would remove this user
-        if module.check_mode:
-            module.exit_json(changed=True)
+    user = get_user(connection, module, user_name)
+    # User is not present
+    if not user:
+        module.exit_json(changed=False)
 
-        # Remove any attached policies otherwise deletion fails
-        try:
-            for policy in get_attached_policy_list(connection, module, params['UserName']):
-                connection.detach_user_policy(UserName=params['UserName'], PolicyArn=policy['PolicyArn'])
-        except ClientError as e:
-            module.fail_json(msg="Unable to detach policy {0} from user {1}: {2}".format(
-                             policy['PolicyArn'], params['UserName'], to_native(e)),
-                             exception=traceback.format_exc(), **camel_dict_to_snake_dict(e.response))
-        except ParamValidationError as e:
-            module.fail_json(msg="Unable to detach policy {0} from user {1}: {2}".format(
-                             policy['PolicyArn'], params['UserName'], to_native(e)),
-                             exception=traceback.format_exc())
+    # Check mode means we would remove this user
+    if module.check_mode:
+        module.exit_json(changed=True)
 
-        try:
-            connection.delete_user(**params)
-        except ClientError as e:
-            module.fail_json(msg="Unable to delete user {0}: {1}".format(params['UserName'], to_native(e)),
-                             exception=traceback.format_exc(), **camel_dict_to_snake_dict(e.response))
-        except ParamValidationError as e:
-            module.fail_json(msg="Unable to delete user {0}: {1}".format(params['UserName'], to_native(e)),
-                             exception=traceback.format_exc())
+    # Remove any attached policies otherwise deletion fails
+    try:
+        for policy in get_attached_policy_list(connection, module, user_name):
+            connection.detach_user_policy(UserName=user_name, PolicyArn=policy['PolicyArn'])
+    except ClientError as e:
+        module.fail_json(msg="Unable to detach policy {0} from user {1}: {2}".format(
+                         policy['PolicyArn'], user_name, to_native(e)),
+                         exception=traceback.format_exc(), **camel_dict_to_snake_dict(e.response))
+    except ParamValidationError as e:
+        module.fail_json(msg="Unable to detach policy {0} from user {1}: {2}".format(
+                         policy['PolicyArn'], user_name, to_native(e)),
+                         exception=traceback.format_exc())
 
-    else:
-        module.exit_json(changed=False)
+    try:
+        # Remove user's access keys
+        access_keys = connection.list_access_keys(UserName=user_name)["AccessKeyMetadata"]
+        for access_key in access_keys:
+            connection.delete_access_key(UserName=user_name, AccessKeyId=access_key["AccessKeyId"])
+
+        # Remove user's login profile (console password)
+        delete_user_login_profile(connection, module, user_name)
+
+        # Remove user's ssh public keys
+        ssh_public_keys = connection.list_ssh_public_keys(UserName=user_name)["SSHPublicKeys"]
+        for ssh_public_key in ssh_public_keys:
+            connection.delete_ssh_public_key(UserName=user_name, SSHPublicKeyId=ssh_public_key["SSHPublicKeyId"])
+
+        # Remove user's service specific credentials
+        service_credentials = connection.list_service_specific_credentials(UserName=user_name)["ServiceSpecificCredentials"]
+        for service_specific_credential in service_credentials:
+            connection.delete_service_specific_credential(
+                UserName=user_name,
+                ServiceSpecificCredentialId=service_specific_credential["ServiceSpecificCredentialId"]
+            )
+
+        # Remove user's signing certificates
+        signing_certificates = connection.list_signing_certificates(UserName=user_name)["Certificates"]
+        for signing_certificate in signing_certificates:
+            connection.delete_signing_certificate(
+                UserName=user_name,
+                CertificateId=signing_certificate["CertificateId"]
+            )
+
+        # Remove user's MFA devices
+        mfa_devices = connection.list_mfa_devices(UserName=user_name)["MFADevices"]
+        for mfa_device in mfa_devices:
+            connection.deactivate_mfa_device(UserName=user_name, SerialNumber=mfa_device["SerialNumber"])
+
+        # Remove user's inline policies
+        inline_policies = connection.list_user_policies(UserName=user_name)["PolicyNames"]
+        for policy_name in inline_policies:
+            connection.delete_user_policy(UserName=user_name, PolicyName=policy_name)
+
+        # Remove user's group membership
+        user_groups = connection.list_groups_for_user(UserName=user_name)["Groups"]
+        for group in user_groups:
+            connection.remove_user_from_group(UserName=user_name, GroupName=group["GroupName"])
+
+        connection.delete_user(UserName=user_name)
+    except ClientError as e:
path: lib/ansible/modules/cloud/amazon/iam_user.py
text: @user1:
This too can just be
```suggestion
    except (ClientError, BotoCoreError) as e:
        module.fail_json_aws(e, msg="Unable to delete user {0}".format(user_name))
```

diff_hunk: @@ -227,38 +227,81 @@ def create_or_update_user(connection, module):
 
 def destroy_user(connection, module):
 
-    params = dict()
-    params['UserName'] = module.params.get('name')
+    user_name = module.params.get('name')
 
-    if get_user(connection, module, params['UserName']):
-        # Check mode means we would remove this user
-        if module.check_mode:
-            module.exit_json(changed=True)
+    user = get_user(connection, module, user_name)
+    # User is not present
+    if not user:
+        module.exit_json(changed=False)
 
-        # Remove any attached policies otherwise deletion fails
-        try:
-            for policy in get_attached_policy_list(connection, module, params['UserName']):
-                connection.detach_user_policy(UserName=params['UserName'], PolicyArn=policy['PolicyArn'])
-        except ClientError as e:
-            module.fail_json(msg="Unable to detach policy {0} from user {1}: {2}".format(
-                             policy['PolicyArn'], params['UserName'], to_native(e)),
-                             exception=traceback.format_exc(), **camel_dict_to_snake_dict(e.response))
-        except ParamValidationError as e:
-            module.fail_json(msg="Unable to detach policy {0} from user {1}: {2}".format(
-                             policy['PolicyArn'], params['UserName'], to_native(e)),
-                             exception=traceback.format_exc())
+    # Check mode means we would remove this user
+    if module.check_mode:
+        module.exit_json(changed=True)
 
-        try:
-            connection.delete_user(**params)
-        except ClientError as e:
-            module.fail_json(msg="Unable to delete user {0}: {1}".format(params['UserName'], to_native(e)),
-                             exception=traceback.format_exc(), **camel_dict_to_snake_dict(e.response))
-        except ParamValidationError as e:
-            module.fail_json(msg="Unable to delete user {0}: {1}".format(params['UserName'], to_native(e)),
-                             exception=traceback.format_exc())
+    # Remove any attached policies otherwise deletion fails
+    try:
+        for policy in get_attached_policy_list(connection, module, user_name):
+            connection.detach_user_policy(UserName=user_name, PolicyArn=policy['PolicyArn'])
+    except ClientError as e:
+        module.fail_json(msg="Unable to detach policy {0} from user {1}: {2}".format(
+                         policy['PolicyArn'], user_name, to_native(e)),
+                         exception=traceback.format_exc(), **camel_dict_to_snake_dict(e.response))
+    except ParamValidationError as e:
path: lib/ansible/modules/cloud/amazon/iam_user.py
text: @user1:
I see you didn't change this from before, but please update this to handle BotoCoreError instead here, as ParamValidationError is only a subset of the things that could go really wrong. You'll need to import it too.

Since you ported this to AnsibleAWSModule it's preferable to use fail_json_aws and handle these two exceptions together; ClientError is only handled separately if you need to parse the .response['Error']['Code'] because it doesn't exist for BotoCoreError. `fail_json_aws` cuts down on boilerplate (it will add the traceback, to_native(e), and e.response for you if these things exist and handle it gracefully if not).
```suggestion
    except (ClientError, BotoCoreError) as e:
        module.fail_json_aws(e, msg="Unable to detach policy {0} from user {1}".format(policy['PolicyArn'], user_name))
```