Fix issues and limitations in account mgmt commands (#58441)

* fix issues and limitations in account mgmt commands

* fix pep8 and example yaml errors

* remove new option and update option aliases

* find next empty slot when adding user via PATCH

lib/ansible/module_utils/redfish_utils.py

@@ -43,7 +43,8 @@ class RedfishUtils(object):
             msg = self._get_extended_message(e)
             return {'ret': False,
                     'msg': "HTTP Error %s on GET request to '%s', extended message: '%s'"
-                           % (e.code, uri, msg)}
+                           % (e.code, uri, msg),
+                    'status': e.code}
         except URLError as e:
             return {'ret': False, 'msg': "URL Error on GET request to '%s': '%s'"
                                          % (uri, e.reason)}
@@ -66,7 +67,8 @@ class RedfishUtils(object):
             msg = self._get_extended_message(e)
             return {'ret': False,
                     'msg': "HTTP Error %s on POST request to '%s', extended message: '%s'"
-                           % (e.code, uri, msg)}
+                           % (e.code, uri, msg),
+                    'status': e.code}
         except URLError as e:
             return {'ret': False, 'msg': "URL Error on POST request to '%s': '%s'"
                                          % (uri, e.reason)}
@@ -100,7 +102,8 @@ class RedfishUtils(object):
             msg = self._get_extended_message(e)
             return {'ret': False,
                     'msg': "HTTP Error %s on PATCH request to '%s', extended message: '%s'"
-                           % (e.code, uri, msg)}
+                           % (e.code, uri, msg),
+                    'status': e.code}
         except URLError as e:
             return {'ret': False, 'msg': "URL Error on PATCH request to '%s': '%s'"
                                          % (uri, e.reason)}
@@ -110,9 +113,10 @@ class RedfishUtils(object):
                     'msg': "Failed PATCH request to '%s': '%s'" % (uri, to_text(e))}
         return {'ret': True, 'resp': resp}
 
-    def delete_request(self, uri, pyld):
+    def delete_request(self, uri, pyld=None):
         try:
-            resp = open_url(uri, data=json.dumps(pyld),
+            data = json.dumps(pyld) if pyld else None
+            resp = open_url(uri, data=data,
                             headers=DELETE_HEADERS, method="DELETE",
                             url_username=self.creds['user'],
                             url_password=self.creds['pswd'],
@@ -123,7 +127,8 @@ class RedfishUtils(object):
             msg = self._get_extended_message(e)
             return {'ret': False,
                     'msg': "HTTP Error %s on DELETE request to '%s', extended message: '%s'"
-                           % (e.code, uri, msg)}
+                           % (e.code, uri, msg),
+                    'status': e.code}
         except URLError as e:
             return {'ret': False, 'msg': "URL Error on DELETE request to '%s': '%s'"
                                          % (uri, e.reason)}
@@ -670,6 +675,60 @@ class RedfishUtils(object):
             return response
         return {'ret': True, 'changed': True}
 
+    def _find_account_uri(self, username=None, acct_id=None):
+        if not any((username, acct_id)):
+            return {'ret': False, 'msg':
+                    'Must provide either account_id or account_username'}
+
+        response = self.get_request(self.root_uri + self.accounts_uri)
+        if response['ret'] is False:
+            return response
+        data = response['data']
+
+        uris = [a.get('@odata.id') for a in data.get('Members', []) if
+                a.get('@odata.id')]
+        for uri in uris:
+            response = self.get_request(self.root_uri + uri)
+            if response['ret'] is False:
+                continue
+            data = response['data']
+            headers = response['headers']
+            if username:
+                if username == data.get('UserName'):
+                    return {'ret': True, 'data': data,
+                            'headers': headers, 'uri': uri}
+            if acct_id:
+                if acct_id == data.get('Id'):
+                    return {'ret': True, 'data': data,
+                            'headers': headers, 'uri': uri}
+
+        return {'ret': False, 'no_match': True, 'msg':
+                'No account with the given account_id or account_username found'}
+
+    def _find_empty_account_slot(self):
+        response = self.get_request(self.root_uri + self.accounts_uri)
+        if response['ret'] is False:
+            return response
+        data = response['data']
+
+        uris = [a.get('@odata.id') for a in data.get('Members', []) if
+                a.get('@odata.id')]
+        if uris:
+            # first slot may be reserved, so move to end of list
+            uris += [uris.pop(0)]
+        for uri in uris:
+            response = self.get_request(self.root_uri + uri)
+            if response['ret'] is False:
+                continue
+            data = response['data']
+            headers = response['headers']
+            if data.get('UserName') == "" and not data.get('Enabled', True):
+                return {'ret': True, 'data': data,
+                        'headers': headers, 'uri': uri}
+
+        return {'ret': False, 'no_match': True, 'msg':
+                'No empty account slot found'}
+
     def list_users(self):
         result = {}
         # listing all users has always been slower than other operations, why?
@@ -684,7 +743,7 @@ class RedfishUtils(object):
         result['ret'] = True
         data = response['data']
 
-        for users in data[u'Members']:
+        for users in data.get('Members', []):
             user_list.append(users[u'@odata.id'])   # user_list[] are URIs
 
         # for each user, get details
@@ -703,54 +762,184 @@ class RedfishUtils(object):
         result["entries"] = users_results
         return result
 
+    def add_user_via_patch(self, user):
+        if user.get('account_id'):
+            # If Id slot specified, use it
+            response = self._find_account_uri(acct_id=user.get('account_id'))
+        else:
+            # Otherwise find first empty slot
+            response = self._find_empty_account_slot()
+
+        if not response['ret']:
+            return response
+        uri = response['uri']
+        payload = {}
+        if user.get('account_username'):
+            payload['UserName'] = user.get('account_username')
+        if user.get('account_password'):
+            payload['Password'] = user.get('account_password')
+        if user.get('account_roleid'):
+            payload['RoleId'] = user.get('account_roleid')
+        response = self.patch_request(self.root_uri + uri, payload)
+        if response['ret'] is False:
+            return response
+        return {'ret': True}
+
     def add_user(self, user):
-        uri = self.root_uri + self.accounts_uri + "/" + user['userid']
+        if not user.get('account_username'):
-        username = {'UserName': user['username']}
+            return {'ret': False, 'msg':
-        pswd = {'Password': user['userpswd']}
+                    'Must provide account_username for AddUser command'}
-        roleid = {'RoleId': user['userrole']}
+
-        enabled = {'Enabled': True}
+        response = self._find_account_uri(username=user.get('account_username'))
-        for payload in username, pswd, roleid, enabled:
+        if response['ret']:
-            response = self.patch_request(uri, payload)
+            # account_username already exists, nothing to do
-            if response['ret'] is False:
+            return {'ret': True, 'changed': False}
+
+        response = self.get_request(self.root_uri + self.accounts_uri)
+        if not response['ret']:
+            return response
+        headers = response['headers']
+
+        if 'allow' in headers:
+            methods = [m.strip() for m in headers.get('allow').split(',')]
+            if 'POST' not in methods:
+                # if Allow header present and POST not listed, add via PATCH
+                return self.add_user_via_patch(user)
+
+        payload = {}
+        if user.get('account_username'):
+            payload['UserName'] = user.get('account_username')
+        if user.get('account_password'):
+            payload['Password'] = user.get('account_password')
+        if user.get('account_roleid'):
+            payload['RoleId'] = user.get('account_roleid')
+
+        response = self.post_request(self.root_uri + self.accounts_uri, payload)
+        if not response['ret']:
+            if response.get('status') == 405:
+                # if POST returned a 405, try to add via PATCH
+                return self.add_user_via_patch(user)
+            else:
                 return response
         return {'ret': True}
 
     def enable_user(self, user):
-        uri = self.root_uri + self.accounts_uri + "/" + user['userid']
+        response = self._find_account_uri(username=user.get('account_username'),
+                                          acct_id=user.get('account_id'))
+        if not response['ret']:
+            return response
+        uri = response['uri']
+        data = response['data']
+
+        if data.get('Enabled'):
+            # account already enabled, nothing to do
+            return {'ret': True, 'changed': False}
+
         payload = {'Enabled': True}
-        response = self.patch_request(uri, payload)
+        response = self.patch_request(self.root_uri + uri, payload)
+        if response['ret'] is False:
+            return response
+        return {'ret': True}
+
+    def delete_user_via_patch(self, user, uri=None, data=None):
+        if not uri:
+            response = self._find_account_uri(username=user.get('account_username'),
+                                              acct_id=user.get('account_id'))
+            if not response['ret']:
+                return response
+            uri = response['uri']
+            data = response['data']
+
+        if data and data.get('UserName') == '' and not data.get('Enabled', False):
+            # account UserName already cleared, nothing to do
+            return {'ret': True, 'changed': False}
+
+        payload = {'UserName': ''}
+        if 'Enabled' in data:
+            payload['Enabled'] = False
+        response = self.patch_request(self.root_uri + uri, payload)
         if response['ret'] is False:
             return response
         return {'ret': True}
 
     def delete_user(self, user):
-        uri = self.root_uri + self.accounts_uri + "/" + user['userid']
+        response = self._find_account_uri(username=user.get('account_username'),
-        payload = {'UserName': ""}
+                                          acct_id=user.get('account_id'))
-        response = self.patch_request(uri, payload)
+        if not response['ret']:
-        if response['ret'] is False:
+            if response.get('no_match'):
-            return response
+                # account does not exist, nothing to do
+                return {'ret': True, 'changed': False}
+            else:
+                # some error encountered
+                return response
+
+        uri = response['uri']
+        headers = response['headers']
+        data = response['data']
+
+        if 'allow' in headers:
+            methods = [m.strip() for m in headers.get('allow').split(',')]
+            if 'DELETE' not in methods:
+                # if Allow header present and DELETE not listed, del via PATCH
+                return self.delete_user_via_patch(user, uri=uri, data=data)
+
+        response = self.delete_request(self.root_uri + uri)
+        if not response['ret']:
+            if response.get('status') == 405:
+                # if DELETE returned a 405, try to delete via PATCH
+                return self.delete_user_via_patch(user, uri=uri, data=data)
+            else:
+                return response
         return {'ret': True}
 
     def disable_user(self, user):
-        uri = self.root_uri + self.accounts_uri + "/" + user['userid']
+        response = self._find_account_uri(username=user.get('account_username'),
+                                          acct_id=user.get('account_id'))
+        if not response['ret']:
+            return response
+        uri = response['uri']
+        data = response['data']
+
+        if not data.get('Enabled'):
+            # account already disabled, nothing to do
+            return {'ret': True, 'changed': False}
+
         payload = {'Enabled': False}
-        response = self.patch_request(uri, payload)
+        response = self.patch_request(self.root_uri + uri, payload)
         if response['ret'] is False:
             return response
         return {'ret': True}
 
     def update_user_role(self, user):
-        uri = self.root_uri + self.accounts_uri + "/" + user['userid']
+        if not user.get('account_roleid'):
-        payload = {'RoleId': user['userrole']}
+            return {'ret': False, 'msg':
-        response = self.patch_request(uri, payload)
+                    'Must provide account_roleid for UpdateUserRole command'}
+
+        response = self._find_account_uri(username=user.get('account_username'),
+                                          acct_id=user.get('account_id'))
+        if not response['ret']:
+            return response
+        uri = response['uri']
+        data = response['data']
+
+        if data.get('RoleId') == user.get('account_roleid'):
+            # account already has RoleId , nothing to do
+            return {'ret': True, 'changed': False}
+
+        payload = {'RoleId': user.get('account_roleid')}
+        response = self.patch_request(self.root_uri + uri, payload)
         if response['ret'] is False:
             return response
         return {'ret': True}
 
     def update_user_password(self, user):
-        uri = self.root_uri + self.accounts_uri + "/" + user['userid']
+        response = self._find_account_uri(username=user.get('account_username'),
-        payload = {'Password': user['userpswd']}
+                                          acct_id=user.get('account_id'))
-        response = self.patch_request(uri, payload)
+        if not response['ret']:
+            return response
+        uri = response['uri']
+        payload = {'Password': user['account_password']}
+        response = self.patch_request(self.root_uri + uri, payload)
         if response['ret'] is False:
             return response
         return {'ret': True}

lib/ansible/modules/remote_management/redfish/redfish_command.py

@@ -41,7 +41,7 @@ options:
   username:
     required: true
     description:
-      - User for authentication with OOB controller
+      - Username for authentication with OOB controller
     type: str
     version_added: "2.8"
   password:
@@ -51,26 +51,30 @@ options:
     type: str
   id:
     required: false
+    aliases: [ account_id ]
     description:
-      - ID of user to add/delete/modify
+      - ID of account to delete/modify
     type: str
     version_added: "2.8"
   new_username:
     required: false
+    aliases: [ account_username ]
     description:
-      - name of user to add/delete/modify
+      - Username of account to add/delete/modify
     type: str
     version_added: "2.8"
   new_password:
     required: false
+    aliases: [ account_password ]
     description:
-      - password of user to add/delete/modify
+      - New password of account to add/modify
     type: str
     version_added: "2.8"
   roleid:
     required: false
+    aliases: [ account_roleid ]
     description:
-      - role of user to add/delete/modify
+      - Role of account to add/modify
     type: str
     version_added: "2.8"
   bootdevice:
@@ -146,6 +150,55 @@ EXAMPLES = '''
       username: "{{ username }}"
       password: "{{ password }}"
 
+  - name: Add user
+    redfish_command:
+      category: Accounts
+      command: AddUser
+      baseuri: "{{ baseuri }}"
+      username: "{{ username }}"
+      password: "{{ password }}"
+      new_username: "{{ new_username }}"
+      new_password: "{{ new_password }}"
+      roleid: "{{ roleid }}"
+
+  - name: Add user using new option aliases
+    redfish_command:
+      category: Accounts
+      command: AddUser
+      baseuri: "{{ baseuri }}"
+      username: "{{ username }}"
+      password: "{{ password }}"
+      account_username: "{{ account_username }}"
+      account_password: "{{ account_password }}"
+      account_roleid: "{{ account_roleid }}"
+
+  - name: Delete user
+    redfish_command:
+      category: Accounts
+      command: DeleteUser
+      baseuri: "{{ baseuri }}"
+      username: "{{ username }}"
+      password: "{{ password }}"
+      account_username: "{{ account_username }}"
+
+  - name: Disable user
+    redfish_command:
+      category: Accounts
+      command: DisableUser
+      baseuri: "{{ baseuri }}"
+      username: "{{ username }}"
+      password: "{{ password }}"
+      account_username: "{{ account_username }}"
+
+  - name: Enable user
+    redfish_command:
+      category: Accounts
+      command: EnableUser
+      baseuri: "{{ baseuri }}"
+      username: "{{ username }}"
+      password: "{{ password }}"
+      account_username: "{{ account_username }}"
+
   - name: Add and enable user
     redfish_command:
       category: Accounts
@@ -153,20 +206,10 @@ EXAMPLES = '''
       baseuri: "{{ baseuri }}"
       username: "{{ username }}"
       password: "{{ password }}"
-      id: "{{ id }}"
       new_username: "{{ new_username }}"
       new_password: "{{ new_password }}"
       roleid: "{{ roleid }}"
 
-  - name: Disable and delete user
-    redfish_command:
-      category: Accounts
-      command: ["DisableUser", "DeleteUser"]
-      baseuri: "{{ baseuri }}"
-      username: "{{ username }}"
-      password: "{{ password }}"
-      id: "{{ id }}"
-
   - name: Update user password
     redfish_command:
       category: Accounts
@@ -174,8 +217,18 @@ EXAMPLES = '''
       baseuri: "{{ baseuri }}"
       username: "{{ username }}"
       password: "{{ password }}"
-      id: "{{ id }}"
+      account_username: "{{ account_username }}"
-      new_password: "{{ new_password }}"
+      account_password: "{{ account_password }}"
+
+  - name: Update user role
+    redfish_command:
+      category: Accounts
+      command: UpdateUserRole
+      baseuri: "{{ baseuri }}"
+      username: "{{ username }}"
+      password: "{{ password }}"
+      account_username: "{{ account_username }}"
+      roleid: "{{ roleid }}"
 
   - name: Clear Manager Logs with a timeout of 20 seconds
     redfish_command:
@@ -220,10 +273,10 @@ def main():
             baseuri=dict(required=True),
             username=dict(required=True),
             password=dict(required=True, no_log=True),
-            id=dict(),
+            id=dict(aliases=["account_id"]),
-            new_username=dict(),
+            new_username=dict(aliases=["account_username"]),
-            new_password=dict(no_log=True),
+            new_password=dict(aliases=["account_password"], no_log=True),
-            roleid=dict(),
+            roleid=dict(aliases=["account_roleid"]),
             bootdevice=dict(),
             timeout=dict(type='int', default=10),
             uefi_target=dict(),
@@ -240,10 +293,10 @@ def main():
              'pswd': module.params['password']}
 
     # user to add/modify/delete
-    user = {'userid': module.params['id'],
+    user = {'account_id': module.params['id'],
-            'username': module.params['new_username'],
+            'account_username': module.params['new_username'],
-            'userpswd': module.params['new_password'],
+            'account_password': module.params['new_password'],
-            'userrole': module.params['roleid']}
+            'account_roleid': module.params['roleid']}
 
     # timeout
     timeout = module.params['timeout']