{Graph} Refine code and tests for ms graph group

src/azure-cli/azure/cli/command_modules/role/_graph_objects.py

@@ -24,6 +24,15 @@
     'required_resource_accesses': 'requiredResourceAccess',
 }
 
+# https://docs.microsoft.com/en-us/graph/api/resources/group?view=graph-rest-1.0#properties
+group_property_map = {
+    'display_name': 'displayName',
+    'mail_nickname': 'mailNickname',
+    'mail_enabled': 'mailEnabled',
+    'security_enabled': 'securityEnabled',
+    'description': 'description'
+}
+
 
 def set_object_properties(property_map, graph_object, **kwargs):
     """Set properties of the graph object according to property_map.

src/azure-cli/azure/cli/command_modules/role/custom.py

@@ -35,7 +35,7 @@
 from ._client_factory import _auth_client_factory, _graph_client_factory
 from ._multi_api_adaptor import MultiAPIAdaptor
 from ._graph_client import GraphClient
-from ._graph_objects import application_property_map, set_object_properties
+from ._graph_objects import application_property_map, group_property_map, set_object_properties
 
 # ARM RBAC's principalType
 USER = 'User'
@@ -766,17 +766,13 @@ def create_group(client, display_name, mail_nickname, force=None, description=No
                 raise CLIError(err.format(', '.join([x.object_id for x in matches])))
             logger.warning('A group with the same display name and mail nickname already exists, returning.')
             return matches[0]
-    body = {
-        "displayName": display_name,
-        "mailNickname": mail_nickname,
-        "mailEnabled": False,
-        "securityEnabled": True
-    }
-    if description is not None:
-        body["description"] = description
-    group = client.group_create(body=body)
 
-    return group
+    body = {}
+    set_object_properties(group_property_map, body, display_name=display_name,
+                          mail_nickname=mail_nickname, description=description,
+                          mail_enabled=False, security_enabled=True)
+
+    return client.group_create(body=body)
 
 
 def list_groups(client, display_name=None, query_filter=None):
@@ -814,7 +810,7 @@ def list_group_owners(client, group_id):
 def add_group_owner(client, owner_object_id, group_id):
     group_object_id = _resolve_group(client, group_id)
     owners = client.group_owner_list(group_object_id)
-    if not next((x for x in owners if x.object_id == owner_object_id), None):
+    if not next((x for x in owners if x['id'] == owner_object_id), None):
         owner_url = client.base_url + '/users/{id}'.format(id=owner_object_id)
         body = {
             "@odata.id": owner_url

src/azure-cli/azure/cli/command_modules/role/tests/latest/test_graph.py

@@ -588,139 +588,114 @@ def test_graph_user_scenario(self):
 
 class GraphGroupScenarioTest(ScenarioTest):
 
+    def clean_resource(self, resource, is_user=False):
+        try:
+            if is_user:
+                self.cmd('ad user delete --id {}'.format(resource))
+            else:
+                self.cmd('ad group delete -g {}'.format(resource))
+        except Exception:
+            pass
+
     def test_graph_group_scenario(self):
         username = get_signed_in_user(self)
         if not username:
             return  # this test delete users which are beyond a SP's capacity, so quit...
 
         domain = 'AzureSDKTeam.onmicrosoft.com'
         self.kwargs = {
-            'user1': self.create_random_name(prefix='deleteme1', length=15),
-            'user2': self.create_random_name(prefix='deleteme2', length=15),
-            'domain': domain,
-            'new_mail_nick_name': 'deleteme11',
-            'group': 'deleteme_g',
-            'pass': 'Test1234!!'
+            'group': self.create_random_name(prefix='testgroup', length=24),
+            'mail_nick_name': 'deleteme11',
+            'child_group': self.create_random_name(prefix='testchildgroup', length=24),
+            'leaf_group': self.create_random_name(prefix='testleafgroup', length=24),
+            'user1': self.create_random_name(prefix='testgroupuser1', length=24),
+            'user2': self.create_random_name(prefix='testgroupuser2', length=24),
+            'pass': 'Test1234!!',
+            'domain': domain
         }
         self.recording_processors.append(AADGraphUserReplacer('@' + domain, '@example.com'))
         try:
-            # create user1
-            user1_result = self.cmd('ad user create --display-name {user1} --password {pass} --user-principal-name {user1}@{domain}').get_output_in_json()
-            self.kwargs['user1_id'] = user1_result['id']
-
-            # update user1
-            self.cmd('ad user update --display-name {user1}_new --account-enabled false --id {user1}@{domain} --mail-nickname {new_mail_nick_name}')
-            user1_update_result = self.cmd('ad user show --upn-or-object-id {user1}@{domain}', checks=[self.check("displayName", '{user1}_new')]).get_output_in_json()
-            self.cmd('ad user update --id {user1}@{domain} --password {pass}')
-            self.cmd('ad user update --id {user1}@{domain} --password {pass} --force-change-password-next-login true')
-            with self.assertRaises(CLIError):
-                self.cmd('ad user update --id {user1}@{domain} --force-change-password-next-login false')
-            self.kwargs['user1_id'] = user1_update_result['id']
-
-            # create user2
-            user2_result = self.cmd('ad user create --display-name {user2} --password {pass} --user-principal-name {user2}@{domain}').get_output_in_json()
-            self.kwargs['user2_id'] = user2_result['id']
             # create group
-            group_result = self.cmd('ad group create --display-name {group} --mail-nickname {group} --description {group}').get_output_in_json()
+            group_result = self.cmd(
+                'ad group create --display-name {group} --mail-nickname {mail_nick_name} --description {group}',
+                checks=[self.check('displayName', '{group}'),
+                        self.check('mailNickname', '{mail_nick_name}'),
+                        self.check('description', '{group}')]
+            ).get_output_in_json()
             self.kwargs['group_id'] = group_result['id']
-            # add user1 into group
-            self.cmd('ad group member add -g {group} --member-id {user1_id}',
-                     checks=self.is_empty())
-            # add user2 into group
-            self.cmd('ad group member add -g {group} --member-id {user2_id}',
-                     checks=self.is_empty())
-
-            # show user's group memberships
-            self.cmd('ad user get-member-groups --upn-or-object-id {user1_id}',
-                     checks=self.check('[0].displayName', self.kwargs['group']))
+
+            # create again to test idempotency
+            self.cmd('ad group create --display-name {group} --mail-nickname {mail_nick_name}')
+            # list groups
+            self.cmd('ad group list --display-name {group}', checks=self.check('length([])', 1))
             # show group
             self.cmd('ad group show -g {group}', checks=[
                 self.check('id', '{group_id}'),
                 self.check('displayName', '{group}'),
+                self.check('mailNickname', '{mail_nick_name}'),
                 self.check('description', '{group}')
             ])
-            self.cmd('ad group show -g {group}',
-                     checks=self.check('displayName', '{group}'))
-            # list group
-            self.cmd('ad group list --display-name {group}',
-                     checks=self.check('[0].displayName', '{group}'))
-            # show member groups
-            self.cmd('ad group get-member-groups -g {group}',
-                     checks=self.check('length([])', 0))
-            # check user1 memebership
-            self.cmd('ad group member check -g {group} --member-id {user1_id}',
-                     checks=self.check('value', True))
 
-            # check user2 memebership
-            self.cmd('ad group member check -g {group} --member-id {user2_id}',
+            # create other groups to test membership transitivity
+            group_result = self.cmd('ad group create --display-name {child_group} --mail-nickname {mail_nick_name}').get_output_in_json()
+            self.kwargs['child_group_id'] = group_result['id']
+            group_result = self.cmd('ad group create --display-name {leaf_group} --mail-nickname {mail_nick_name}').get_output_in_json()
+            self.kwargs['leaf_group_id'] = group_result['id']
+
+            # add child_group as member of group
+            self.cmd('ad group member add -g {group_id} --member-id {child_group_id}')
+            # add leaf_group as member of child_group
+            self.cmd('ad group member add -g {child_group_id} --member-id {leaf_group_id}')
+
+            # check member transitivity
+            self.cmd('ad group member check -g {group_id} --member-id {child_group_id}',
+                     checks=self.check('value', True))
+            self.cmd('ad group member check -g {child_group_id} --member-id {leaf_group_id}',
+                     checks=self.check('value', True))
+            self.cmd('ad group member check -g {group_id} --member-id {leaf_group_id}',
                      checks=self.check('value', True))
 
-            self.cmd('ad group member list -g {group}', checks=[
-                self.check("length([?displayName=='{user1}_new'])", 1),
-                self.check("length([?displayName=='{user2}'])", 1),
-                self.check("length([?displayName=='{user1}'])", 0),
-                self.check("length([])", 2),
-            ])
-            # remove user1
-            self.cmd('ad group member remove -g {group} --member-id {user1_id}')
-            # check user1 memebership
-            self.cmd('ad group member check -g {group} --member-id {user1_id}',
-                     checks=self.check('value', False))
-            # delete the group
-            self.cmd('ad group delete -g {group}')
-            self.cmd('ad group list',
-                     checks=self.check("length([?displayName=='{group}'])", 0))
-        finally:
-            try:
-                self.cmd('ad user delete --upn-or-object-id {user1_id}')
-                self.cmd('ad user delete --upn-or-object-id {user2_id}')
-                self.cmd('ad group delete -g {group}')
-            except Exception:
-                pass
+            # list members (intransitive)
+            self.cmd('ad group member list -g {group_id}', checks=self.check('length([])', 1))
+            self.cmd('ad group member list -g {child_group_id}', checks=self.check('length([])', 1))
+            self.cmd('ad group member list -g {leaf_group_id}', checks=self.check('length([])', 0))
 
-    def test_graph_group_idempotent(self):
-        account_info = self.cmd('account show').get_output_in_json()
-        if account_info['user']['type'] == 'servicePrincipal':
-            return  # this test delete users which are beyond a SP's capacity, so quit...
+            # get-member-groups transitivity
+            self.cmd('ad group get-member-groups -g {group_id}', checks=self.check('length([])', 0))
+            self.cmd('ad group get-member-groups -g {child_group_id}', checks=self.check('length([])', 1))
+            self.cmd('ad group get-member-groups -g {leaf_group_id}', checks=self.check('length([])', 2))
 
-        self.kwargs = {
-            'group': 'deleteme_g2',
-        }
-        try:
-            self.cmd('ad group create --display-name {group} --mail-nickname {group}')
-            self.cmd('ad group create --display-name {group} --mail-nickname {group}')
-            self.cmd('ad group list',
-                     checks=self.check("length([?displayName=='{group}'])", 1))
-        finally:
-            try:
-                self.cmd('ad group delete -g {group}')
-            except:
-                pass
+            # remove member
+            self.cmd('ad group member remove -g {child_group_id} --member-id {leaf_group_id}')
+            self.cmd('ad group member check -g {child_group_id} --member-id {leaf_group_id}',
+                     checks=self.check('value', False))
 
-    def test_graph_group_show(self):
-        account_info = self.cmd('account show').get_output_in_json()
-        if account_info['user']['type'] == 'servicePrincipal':
-            return  # this test delete users which are beyond a SP's capacity, so quit...
+            # list owners
+            self.cmd('ad group owner list -g {group_id}', checks=self.check('length([])', 0))
 
-        self.kwargs = {
-            'group1': 'show_group_1',
-            'group11': 'show_group_11',
-            'prefix': 'show_prefix',
-            'prefix_group': 'show_prefix_group'
-        }
+            # create users to add group owners
+            user_result = self.cmd('ad user create --display-name {user1} --password {pass} --user-principal-name {user1}@{domain}').get_output_in_json()
+            self.kwargs['user1_id'] = user_result['id']
+            user_result = self.cmd('ad user create --display-name {user2} --password {pass} --user-principal-name {user2}@{domain}').get_output_in_json()
+            self.kwargs['user2_id'] = user_result['id']
+            # add owner
+            self.cmd('ad group owner add -g {group_id} --owner-object-id {user1_id}')
+            self.cmd('ad group owner add -g {group_id} --owner-object-id {user2_id}')
+            self.cmd('ad group owner list -g {group_id}', checks=self.check('length([])', 2))
 
-        self.cmd('ad group create --display-name {group1} --mail-nickname {group1}')
-        self.cmd('ad group create --display-name {group11} --mail-nickname {group11}')
-        self.cmd('ad group create --display-name {prefix_group} --mail-nickname {prefix_group}')
-        self.cmd('ad group show --group {group1}',
-                 checks=self.check('displayName', '{group1}'))
-        self.cmd('ad group show --group {group11}',
-                 checks=self.check('displayName', '{group11}'))
-        self.cmd('ad group show --group {prefix}',
-                 checks=self.check('displayName', '{prefix_group}'))
-        self.cmd('ad group delete -g {group1}')
-        self.cmd('ad group delete -g {group11}')
-        self.cmd('ad group delete -g {prefix}')
+            # remove owner
+            self.cmd('ad group owner remove -g {group_id} --owner-object-id {user1_id}')
+            self.cmd('ad group owner list -g {group_id}', checks=self.check('length([])', 1))
+
+            # delete group
+            self.cmd('ad group delete -g {group}')
+            self.cmd('ad group show -g {group_id}', expect_failure=True)
+        finally:
+            self.clean_resource(self.kwargs['group'])
+            self.clean_resource(self.kwargs['child_group'])
+            self.clean_resource(self.kwargs['leaf_group'])
+            self.clean_resource('{}@{}'.format(self.kwargs['user1'], self.kwargs['domain']), is_user=True)
+            self.clean_resource('{}@{}'.format(self.kwargs['user2'], self.kwargs['domain']), is_user=True)
 
 
 def get_signed_in_user(test_case):
@@ -760,26 +735,6 @@ def test_graph_application_ownership(self):
             except:
                 pass
 
-    def test_graph_group_ownership(self):
-        owner = get_signed_in_user(self)
-        if not owner:
-            return  # this test delete users which are beyond a SP's capacity, so quit...
-
-        self.kwargs = {
-            'owner': owner,
-            'group': self.create_random_name('cli-grp', 15),
-        }
-        self.recording_processors.append(AADGraphUserReplacer(owner, '[email protected]'))
-        try:
-            self.kwargs['owner_object_id'] = self.cmd('ad user show --upn-or-object-id {owner}').get_output_in_json()['objectId']
-            self.kwargs['group_object_id'] = self.cmd('ad group create --display-name {group} --mail-nickname {group}').get_output_in_json()['objectId']
-            self.cmd('ad group owner add -g {group_object_id} --owner-object-id {owner_object_id}')
-            self.cmd('ad group owner add -g {group_object_id} --owner-object-id {owner_object_id}')
-            self.cmd('ad group owner list -g {group_object_id}', checks=self.check('length([*])', 1))
-        finally:
-            if self.kwargs['group_object_id']:
-                self.cmd('ad group delete -g ' + self.kwargs['group_object_id'])
-
 
 class GraphAppCredsScenarioTest(ScenarioTest):
     def test_graph_app_cred_e2e(self):