From 60057c3b0524388cafeebea6aa9de73ab79dc799 Mon Sep 17 00:00:00 2001 From: Liza Shakury <42377481+LizaShak@users.noreply.github.com> Date: Sun, 11 Sep 2022 16:59:28 +0300 Subject: [PATCH] Support groups in role assignments (#2582) * Support groups in role assignments * Fix lint * Upgrade version * Fix lint * CR changes * Lint fix Co-authored-by: Liza Shakury --- api_app/_version.py | 2 +- api_app/services/aad_authentication.py | 29 ++- .../test_services/test_aad_access_service.py | 244 ++++++++++++++++-- 3 files changed, 240 insertions(+), 35 deletions(-) diff --git a/api_app/_version.py b/api_app/_version.py index 4fef01e0d4..7fe0489074 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.4.28" +__version__ = "0.4.29" diff --git a/api_app/services/aad_authentication.py b/api_app/services/aad_authentication.py index 1592708c24..a4f0c22d22 100644 --- a/api_app/services/aad_authentication.py +++ b/api_app/services/aad_authentication.py @@ -210,29 +210,40 @@ def _get_app_sp_graph_data(self, client_id: str) -> dict: graph_data = requests.get(sp_endpoint, headers=self._get_auth_header(msgraph_token)).json() return graph_data - def _get_user_emails_with_role_asssignment(self, client_id): - msgraph_token = self._get_msgraph_token() + def _get_user_role_assignments(self, client_id, msgraph_token): sp_roles_endpoint = self._get_service_principal_assigned_roles_endpoint(client_id) - roles_graph_data = requests.get(sp_roles_endpoint, headers=self._get_auth_header(msgraph_token)).json() + return requests.get(sp_roles_endpoint, headers=self._get_auth_header(msgraph_token)).json() + def _get_user_emails(self, roles_graph_data, msgraph_token): batch_endpoint = self._get_batch_endpoint() batch_request_body = self._get_batch_users_by_role_assignments_body(roles_graph_data) headers = self._get_auth_header(msgraph_token) headers["Content-type"] = "application/json" users_graph_data = requests.post(batch_endpoint, json=batch_request_body, headers=headers).json() + return users_graph_data - return roles_graph_data, users_graph_data + def _get_user_emails_from_response(self, users_graph_data): + user_emails = {} + for user_data in users_graph_data["responses"]: + # Handle user endpoint response + if "users" in user_data["body"]["@odata.context"] and user_data["body"]["mail"] is not None: + user_emails[user_data["body"]["id"]] = user_data["body"]["mail"] + # Handle group endpoint response + if "directoryObjects" in user_data["body"]["@odata.context"]: + for group_member in user_data["body"]["value"]: + if group_member["mail"] is not None: + user_emails[group_member["id"]] = group_member["mail"] + return user_emails def get_workspace_role_assignment_details(self, workspace: Workspace): + msgraph_token = self._get_msgraph_token() app_role_ids = {role_name: workspace.properties[role_id] for role_name, role_id in self.WORKSPACE_ROLES_DICT.items()} inverted_app_role_ids = {role_id: role_name for role_name, role_id in app_role_ids.items()} sp_id = workspace.properties["sp_id"] - roles_graph_data, users_graph_data = self._get_user_emails_with_role_asssignment(sp_id) - user_emails = {} - for user_data in users_graph_data["responses"]: - if user_data["body"]["mail"] is not None: - user_emails[user_data["body"]["id"]] = user_data["body"]["mail"] + roles_graph_data = self._get_user_role_assignments(sp_id, msgraph_token) + users_graph_data = self._get_user_emails(roles_graph_data, msgraph_token) + user_emails = self._get_user_emails_from_response(users_graph_data) workspace_role_assignments_details = defaultdict(list) for role_assignment in roles_graph_data["value"]: diff --git a/api_app/tests_ma/test_services/test_aad_access_service.py b/api_app/tests_ma/test_services/test_aad_access_service.py index b0dbe2ee89..d1355ff8c1 100644 --- a/api_app/tests_ma/test_services/test_aad_access_service.py +++ b/api_app/tests_ma/test_services/test_aad_access_service.py @@ -13,14 +13,16 @@ def test_extract_workspace__raises_error_if_client_id_not_available(): access_service.extract_workspace_auth_information(data={}) -@patch("services.aad_authentication.AzureADAuthorization._get_app_auth_info", return_value={"app_role_id_workspace_researcher": "1234"}) +@patch("services.aad_authentication.AzureADAuthorization._get_app_auth_info", + return_value={"app_role_id_workspace_researcher": "1234"}) def test_extract_workspace__raises_error_if_owner_not_in_roles(get_app_auth_info_mock): access_service = AzureADAuthorization() with pytest.raises(AuthConfigValidationError): access_service.extract_workspace_auth_information(data={"client_id": "1234"}) -@patch("services.aad_authentication.AzureADAuthorization._get_app_auth_info", return_value={"app_role_id_workspace_owner": "1234"}) +@patch("services.aad_authentication.AzureADAuthorization._get_app_auth_info", + return_value={"app_role_id_workspace_owner": "1234"}) def test_extract_workspace__raises_error_if_researcher_not_in_roles(get_app_auth_info_mock): access_service = AzureADAuthorization() with pytest.raises(AuthConfigValidationError): @@ -68,54 +70,84 @@ def test_extract_workspace__returns_sp_id_and_roles(get_app_sp_graph_data_mock): @pytest.mark.parametrize('user, workspace, expected_role', [ # user not a member of the workspace app - (User(roleAssignments=[RoleAssignment(resource_id="ab123", role_id="ab124")], id='123', name="test", email="t@t.com"), - Workspace(id='abc', etag="", templateName='template-name', templateVersion='0.1.0', resourcePath="test", - properties={'client_id': '1234', 'sp_id': 'abc127', 'app_role_id_workspace_owner': 'abc128', 'app_role_id_workspace_researcher': 'abc129', 'app_role_id_workspace_airlock_manager': 'abc130'}), + (User(roleAssignments=[RoleAssignment(resource_id="ab123", role_id="ab124")], id='123', + name="test", email="t@t.com"), + Workspace(id='abc', etag="", templateName='template-name', templateVersion='0.1.0', + resourcePath="test", + properties={'client_id': '1234', 'sp_id': 'abc127', + 'app_role_id_workspace_owner': 'abc128', + 'app_role_id_workspace_researcher': 'abc129', + 'app_role_id_workspace_airlock_manager': 'abc130'}), WorkspaceRole.NoRole), # user is member of the workspace app but not in role - (User(roleAssignments=[RoleAssignment(resource_id="ab127", role_id="ab124")], id='123', name="test", email="t@t.com"), - Workspace(id='abc', etag="", templateName='template-name', templateVersion='0.1.0', resourcePath="test", - properties={'client_id': '1234', 'sp_id': 'abc127', 'app_role_id_workspace_owner': 'abc128', 'app_role_id_workspace_researcher': 'abc129', 'app_role_id_workspace_airlock_manager': 'abc130'}), + (User(roleAssignments=[RoleAssignment(resource_id="ab127", role_id="ab124")], id='123', + name="test", email="t@t.com"), + Workspace(id='abc', etag="", templateName='template-name', templateVersion='0.1.0', + resourcePath="test", + properties={'client_id': '1234', 'sp_id': 'abc127', + 'app_role_id_workspace_owner': 'abc128', + 'app_role_id_workspace_researcher': 'abc129', + 'app_role_id_workspace_airlock_manager': 'abc130'}), WorkspaceRole.NoRole), # user has owner role in workspace - (User(roleAssignments=[RoleAssignment(resource_id="abc127", role_id="abc128")], id='123', name="test", email="t@t.com"), - Workspace(id='abc', etag="", templateName='template-name', templateVersion='0.1.0', resourcePath="test", - properties={'client_id': '1234', 'sp_id': 'abc127', 'app_role_id_workspace_owner': 'abc128', 'app_role_id_workspace_researcher': 'abc129', 'app_role_id_workspace_airlock_manager': 'abc130'}), + (User(roleAssignments=[RoleAssignment(resource_id="abc127", role_id="abc128")], id='123', + name="test", email="t@t.com"), + Workspace(id='abc', etag="", templateName='template-name', templateVersion='0.1.0', + resourcePath="test", + properties={'client_id': '1234', 'sp_id': 'abc127', + 'app_role_id_workspace_owner': 'abc128', + 'app_role_id_workspace_researcher': 'abc129', + 'app_role_id_workspace_airlock_manager': 'abc130'}), WorkspaceRole.Owner), # user has researcher role in workspace - (User(roleAssignments=[RoleAssignment(resource_id="abc127", role_id="abc129")], id='123', name="test", email="t@t.com"), - Workspace(id='abc', etag="", templateName='template-name', templateVersion='0.1.0', resourcePath="test", - properties={'client_id': '1234', 'sp_id': 'abc127', 'app_role_id_workspace_owner': 'abc128', 'app_role_id_workspace_researcher': 'abc129', 'app_role_id_workspace_airlock_manager': 'abc130'}), + (User(roleAssignments=[RoleAssignment(resource_id="abc127", role_id="abc129")], id='123', + name="test", email="t@t.com"), + Workspace(id='abc', etag="", templateName='template-name', templateVersion='0.1.0', + resourcePath="test", + properties={'client_id': '1234', 'sp_id': 'abc127', + 'app_role_id_workspace_owner': 'abc128', + 'app_role_id_workspace_researcher': 'abc129', + 'app_role_id_workspace_airlock_manager': 'abc130'}), WorkspaceRole.Researcher), # user has airlock manager role in workspace - (User(roleAssignments=[RoleAssignment(resource_id="abc127", role_id="abc130")], id='123', name="test", email="t@t.com"), - Workspace(id='abc', etag="", templateName='template-name', templateVersion='0.1.0', resourcePath="test", - properties={'client_id': '1234', 'sp_id': 'abc127', 'app_role_id_workspace_owner': 'abc128', 'app_role_id_workspace_researcher': 'abc129', 'app_role_id_workspace_airlock_manager': 'abc130'}), + (User(roleAssignments=[RoleAssignment(resource_id="abc127", role_id="abc130")], id='123', + name="test", email="t@t.com"), + Workspace(id='abc', etag="", templateName='template-name', templateVersion='0.1.0', + resourcePath="test", + properties={'client_id': '1234', 'sp_id': 'abc127', + 'app_role_id_workspace_owner': 'abc128', + 'app_role_id_workspace_researcher': 'abc129', + 'app_role_id_workspace_airlock_manager': 'abc130'}), WorkspaceRole.AirlockManager) ]) @patch("services.aad_authentication.AzureADAuthorization.get_identity_role_assignments") -def test_get_workspace_role_returns_correct_owner(get_identity_role_assignments_mock, user: User, workspace: Workspace, expected_role: WorkspaceRole): - +def test_get_workspace_role_returns_correct_owner(get_identity_role_assignments_mock, user: User, workspace: Workspace, + expected_role: WorkspaceRole): get_identity_role_assignments_mock.return_value = user.roleAssignments access_service = AzureADAuthorization() - actual_role = access_service.get_workspace_role(user, workspace, access_service.get_identity_role_assignments(user.id)) + actual_role = access_service.get_workspace_role(user, workspace, + access_service.get_identity_role_assignments(user.id)) assert actual_role == expected_role -@patch("services.aad_authentication.AzureADAuthorization.get_identity_role_assignments", return_value=[("ab123", "ab124")]) +@patch("services.aad_authentication.AzureADAuthorization.get_identity_role_assignments", + return_value=[("ab123", "ab124")]) def test_raises_auth_config_error_if_workspace_auth_config_is_not_set(_): access_service = AzureADAuthorization() user = User(id='123', name="test", email="t@t.com") - workspace_with_no_auth_config = Workspace(id='abc', etag='', templateName='template-name', templateVersion='0.1.0', resourcePath="test") + workspace_with_no_auth_config = Workspace(id='abc', etag='', templateName='template-name', templateVersion='0.1.0', + resourcePath="test") with pytest.raises(AuthConfigValidationError): - _ = access_service.get_workspace_role(user, workspace_with_no_auth_config, access_service.get_identity_role_assignments(user.id)) + _ = access_service.get_workspace_role(user, workspace_with_no_auth_config, + access_service.get_identity_role_assignments(user.id)) -@patch("services.aad_authentication.AzureADAuthorization.get_identity_role_assignments", return_value=[("ab123", "ab124")]) +@patch("services.aad_authentication.AzureADAuthorization.get_identity_role_assignments", + return_value=[("ab123", "ab124")]) def test_raises_auth_config_error_if_auth_info_has_incorrect_roles(_): access_service = AzureADAuthorization() @@ -129,4 +161,166 @@ def test_raises_auth_config_error_if_auth_info_has_incorrect_roles(_): resourcePath="test") with pytest.raises(AuthConfigValidationError): - _ = access_service.get_workspace_role(user, workspace_with_auth_info_but_no_roles, access_service.get_identity_role_assignments()) + _ = access_service.get_workspace_role(user, workspace_with_auth_info_but_no_roles, + access_service.get_identity_role_assignments()) + + +@patch("services.aad_authentication.AzureADAuthorization._get_user_role_assignments") +@patch("services.aad_authentication.AzureADAuthorization._get_user_emails") +@patch("services.aad_authentication.AzureADAuthorization._get_msgraph_token", return_value="token") +def test_get_workspace_role_assignment_details_with_single_user_returns_user_mail_and_role_assignment(_, users, roles): + access_service = AzureADAuthorization() + + # Build user response + user_principal_id = "user_principal_id" + user_email = "test_user@email.com" + user_response = get_batch_response([user_principal_id], [user_email]) + users.return_value = user_response + + # Build user role assignment response + workspace_owner_role_id = "1234" + roles_response = get_sample_role_response([user_principal_id], [workspace_owner_role_id], ["User"]) + roles.return_value = roles_response + + # Act + role_assignment_details = access_service.get_workspace_role_assignment_details(Workspace( + id="id", + templateName="tre-workspace-base", + templateVersion="0.1.0", + etag="", + properties={'sp_id': 'ab123', 'app_role_id_workspace_owner': workspace_owner_role_id, + 'app_role_id_workspace_researcher': 'ab125', 'app_role_id_workspace_airlock_manager': 'ab130'} + )) + + assert role_assignment_details['WorkspaceOwner'] == [user_email] + + +@patch("services.aad_authentication.AzureADAuthorization._get_user_role_assignments") +@patch("services.aad_authentication.AzureADAuthorization._get_user_emails") +@patch("services.aad_authentication.AzureADAuthorization._get_msgraph_token", return_value="token") +def test_get_workspace_role_assignment_details_with_single_user_with_no_mail_is_not_returned(_, users, roles): + access_service = AzureADAuthorization() + + # Build user response + user_principal_id = "user_principal_id" + user_response = get_batch_response([user_principal_id], [None]) + users.return_value = user_response + + # Build user role assignment response + workspace_owner_role_id = "1234" + roles_response = get_sample_role_response([user_principal_id], [workspace_owner_role_id], ["User"]) + roles.return_value = roles_response + + # Act + role_assignment_details = access_service.get_workspace_role_assignment_details(Workspace( + id="id", + templateName="tre-workspace-base", + templateVersion="0.1.0", + etag="", + properties={'sp_id': 'ab123', 'app_role_id_workspace_owner': workspace_owner_role_id, + 'app_role_id_workspace_researcher': 'ab125', 'app_role_id_workspace_airlock_manager': 'ab130'} + )) + + assert len(role_assignment_details) == 0 + + +@patch("services.aad_authentication.AzureADAuthorization._get_user_role_assignments") +@patch("services.aad_authentication.AzureADAuthorization._get_user_emails") +@patch("services.aad_authentication.AzureADAuthorization._get_msgraph_token", return_value="token") +def test_get_workspace_role_assignment_details_with_only_groups_assigned_are_not_returned(_, users_and_groups, roles): + access_service = AzureADAuthorization() + + # Build group response + group_principal_id = "group_principal_id" + group_response = get_batch_response([group_principal_id], ["group@email.com"]) + users_and_groups.return_value = group_response + + # Build user role assignment response + workspace_owner_role_id = "1234" + roles_response = get_sample_role_response([group_principal_id], [workspace_owner_role_id], ["Group"]) + roles.return_value = roles_response + + # Act + role_assignment_details = access_service.get_workspace_role_assignment_details(Workspace( + id="id", + templateName="tre-workspace-base", + templateVersion="0.1.0", + etag="", + properties={'sp_id': 'ab123', 'app_role_id_workspace_owner': workspace_owner_role_id, + 'app_role_id_workspace_researcher': 'ab125', 'app_role_id_workspace_airlock_manager': 'ab130'} + )) + + assert len(role_assignment_details) == 0 + + +@patch("services.aad_authentication.AzureADAuthorization._get_user_role_assignments") +@patch("services.aad_authentication.AzureADAuthorization._get_user_emails") +@patch("services.aad_authentication.AzureADAuthorization._get_msgraph_token", return_value="token") +def test_get_workspace_role_assignment_details_with_groups_with_multiple_users_assigned_returned_as_expected(_, users_and_groups, roles): + access_service = AzureADAuthorization() + + # Build group response + group_principal_id = "group_principal_id" + + user_principal_id1 = "user_principal_id1" + user_email1 = "test_user1@email.com" + + user_principal_id2 = "user_principal_id2" + user_email2 = "test_user2@email.com" + + user_response = get_batch_response([user_principal_id1, user_principal_id2, group_principal_id], [user_email1, user_email2, "group@email.com"]) + users_and_groups.return_value = user_response + + # Build user role assignment response + workspace_owner_role_id = "1234" + roles_response = get_sample_role_response([group_principal_id, user_principal_id1, user_principal_id2], + [workspace_owner_role_id], + ["Group", "User", "User"]) + roles.return_value = roles_response + + # Act + role_assignment_details = access_service.get_workspace_role_assignment_details(Workspace( + id="id", + templateName="tre-workspace-base", + templateVersion="0.1.0", + etag="", + properties={'sp_id': 'ab123', 'app_role_id_workspace_owner': workspace_owner_role_id, + 'app_role_id_workspace_researcher': 'ab125', 'app_role_id_workspace_airlock_manager': 'ab130'} + )) + + assert len(role_assignment_details) == 0 + + +def get_batch_response(principal_ids, mails): + response_body = {"responses": []} + for principal_id, mail in zip(principal_ids, mails): + response_body["responses"].append(get_sample_user_response(principal_id, mail)) + return response_body + + +def get_sample_user_response(principal_id, mail): + headers = '{"Cache-Control":"no-cache","x-ms-resource-unit":"1","OData-Version":"4.0","Content-Type":"application/json;odata.metadata=minimal;odata.streaming=true;IEEE754Compatible=false;charset=utf-8"}' + user_odata = '@odata.context":"https://graph.microsoft.com/v1.0/$metadata#users(mail,id)/$entity' + user_response_body = {"id": "1", + "status": 200, + "headers": headers, + "body": {"@odata.context": user_odata, "mail": mail, "id": principal_id}} + return user_response_body + + +def get_sample_group_response(principal_id, mail): + headers = '{"Cache-Control":"no-cache","x-ms-resource-unit":"1","OData-Version":"4.0","Content-Type":"application/json;odata.metadata=minimal;odata.streaming=true;IEEE754Compatible=false;charset=utf-8"}' + user_odata = '@odata.context":"https://graph.microsoft.com/v1.0/$metadata#users(mail,id)/$entity' + group_response_body = {"id": "1", + "status": 200, + "headers": headers, + "body": {"@odata.context": user_odata, "mail": mail, "id": principal_id}} + return group_response_body + + +def get_sample_role_response(principal_ids, role_ids, types): + odata_context = '@odata.context":"https://graph.microsoft.com/v1.0/$metadata#servicePrincipals(workspace-client-id))/appRoleAssignedTo(appRoleId,principalId,principalType)' + response = {"@odata.context": odata_context, "value": []} + for principal_id, role_id, principal_type in zip(principal_ids, role_ids, types): + response["value"].append({"appRoleId": role_id, "principalId": principal_id, "principalType": principal_type}) + return response