From 532908327146ed8ee279d11a3ef9ba1a5b6c529a Mon Sep 17 00:00:00 2001 From: Tamir Kamara <26870601+tamirkamara@users.noreply.github.com> Date: Mon, 22 Aug 2022 10:44:16 +0000 Subject: [PATCH 1/7] paging in ms graph queries --- .devcontainer/devcontainer.json | 15 +++++++++ api_app/_version.py | 2 +- api_app/api/routes/workspaces.py | 1 + api_app/services/aad_authentication.py | 43 +++++++++++++++++++------- 4 files changed, 48 insertions(+), 13 deletions(-) diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index 99939f506e..d9f786334b 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -89,6 +89,21 @@ "false" ] }, + { + "name": "E2E Extended AAD", + "type": "python", + "request": "launch", + "module": "pytest", + "justMyCode": true, + "cwd": "${workspaceFolder}/e2e_tests/", + "preLaunchTask": "Copy_env_file_for_e2e_debug", + "args": [ + "-m", + "extended_aad", + "--verify", + "false" + ] + }, { "name": "E2E Shared Services", "type": "python", diff --git a/api_app/_version.py b/api_app/_version.py index ac1552129d..2dd5536049 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.4.17" +__version__ = "0.4.18" diff --git a/api_app/api/routes/workspaces.py b/api_app/api/routes/workspaces.py index 2b394ab664..13cb7d5e5d 100644 --- a/api_app/api/routes/workspaces.py +++ b/api_app/api/routes/workspaces.py @@ -79,6 +79,7 @@ async def retrieve_workspace_by_workspace_id(user=Depends(get_current_tre_user_o if access_service.get_workspace_role(user, workspace, user_role_assignments) != WorkspaceRole.NoRole: return WorkspaceInResponse(workspace=workspace) else: + logging.debug("User doesn't have roles in workspace.") raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail=strings.ACCESS_USER_IS_NOT_OWNER_OR_RESEARCHER) diff --git a/api_app/services/aad_authentication.py b/api_app/services/aad_authentication.py index c8e1dbfcce..6b50075c16 100644 --- a/api_app/services/aad_authentication.py +++ b/api_app/services/aad_authentication.py @@ -282,27 +282,42 @@ def _get_app_auth_info(self, client_id: str) -> dict: return authInfo - def _get_role_assignment_graph_data_for_user(self, user_id: str) -> dict: + def _ms_graph_query(self, url: str, http_method: str, json=None) -> dict: msgraph_token = self._get_msgraph_token() + auth_headers = self._get_auth_header(msgraph_token) + graph_data = {} + while True: + if not url: + break + logging.debug(f"making request to: {url}") + if json: + response = requests.request(method=http_method, url=url, json=json, headers=auth_headers) + else: + response = requests.request(method=http_method, url=url, headers=auth_headers) + url = "" + if response.status_code == 200: + json_response = response.json() + graph_data.update(json_response) + if '@odata.nextLink' in json_response: + url = json_response['@odata.nextLink'] + return graph_data + + def _get_role_assignment_graph_data_for_user(self, user_id: str) -> dict: user_endpoint = f"https://graph.microsoft.com/v1.0/users/{user_id}/appRoleAssignments" - graph_data = requests.get(user_endpoint, headers=self._get_auth_header(msgraph_token)).json() + graph_data = self._ms_graph_query(user_endpoint, "GET") return graph_data def _get_role_assignment_graph_data_for_service_principal(self, principal_id: str) -> dict: - msgraph_token = self._get_msgraph_token() - user_endpoint = f"https://graph.microsoft.com/v1.0/servicePrincipals/{principal_id}/appRoleAssignments" - graph_data = requests.get(user_endpoint, headers=self._get_auth_header(msgraph_token)).json() + svc_principal_endpoint = f"https://graph.microsoft.com/v1.0/servicePrincipals/{principal_id}/appRoleAssignments" + graph_data = self._ms_graph_query(svc_principal_endpoint, "GET") return graph_data def _get_identity_type(self, id: str) -> str: - msgraph_token = self._get_msgraph_token() objects_endpoint = "https://graph.microsoft.com/v1.0/directoryObjects/getByIds" request_body = {"ids": [id], "types": ["user", "servicePrincipal"]} - graph_data = requests.post( - objects_endpoint, - headers=self._get_auth_header(msgraph_token), - json=request_body - ).json() + graph_data = self._ms_graph_query(objects_endpoint, "POST", json=request_body) + + logging.debug(graph_data) if "value" not in graph_data or len(graph_data["value"]) != 1: logging.debug(graph_data) @@ -337,15 +352,19 @@ def get_identity_role_assignments(self, user_id: str) -> List[RoleAssignment]: if identity_type == "#microsoft.graph.user": graph_data = self._get_role_assignment_graph_data_for_user(user_id) elif identity_type == "#microsoft.graph.servicePrincipal": + logging.debug("before _get_role_assignment_graph_data_for_service_principal call") graph_data = self._get_role_assignment_graph_data_for_service_principal(user_id) else: - logging.debug(graph_data) + logging.debug("about to raise: AuthConfigValidationError") raise AuthConfigValidationError(f"{strings.ACCESS_UNHANDLED_ACCOUNT_TYPE} {identity_type}") if 'value' not in graph_data: logging.debug(graph_data) raise AuthConfigValidationError(f"{strings.ACCESS_UNABLE_TO_GET_ROLE_ASSIGNMENTS_FOR_USER} {user_id}") + logging.debug("about to log: graph_data") + logging.debug(graph_data) + return [RoleAssignment(role_assignment['resourceId'], role_assignment['appRoleId']) for role_assignment in graph_data['value']] def get_workspace_role(self, user: User, workspace: Workspace, user_role_assignments: List[RoleAssignment]) -> WorkspaceRole: From 794d93add765ed48a2c73bcfa160c34a3ea5ea6f Mon Sep 17 00:00:00 2001 From: Tamir Kamara <26870601+tamirkamara@users.noreply.github.com> Date: Mon, 22 Aug 2022 10:57:37 +0000 Subject: [PATCH 2/7] update changelog --- CHANGELOG.md | 1 + api_app/services/aad_authentication.py | 5 +---- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 842e6f43c5..c15c191ae0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ BUG FIXES: * Enable route table on the Airlock Processor subnet ([#2414](https://github.com/microsoft/AzureTRE/pull/2414)) * Support for _Standard_ app service plan SKUs ([#2415](https://github.com/microsoft/AzureTRE/pull/2415)) * Fix Azure ML Workspace deletion ([#2452](https://github.com/microsoft/AzureTRE/pull/2452)) +* Get all pages in MS Graph queries ([#2492](https://github.com/microsoft/AzureTRE/pull/2492)) ## 0.4.1 (August 03, 2022) diff --git a/api_app/services/aad_authentication.py b/api_app/services/aad_authentication.py index 6b50075c16..7977dd4153 100644 --- a/api_app/services/aad_authentication.py +++ b/api_app/services/aad_authentication.py @@ -289,7 +289,7 @@ def _ms_graph_query(self, url: str, http_method: str, json=None) -> dict: while True: if not url: break - logging.debug(f"making request to: {url}") + logging.debug(f"Making request to: {url}") if json: response = requests.request(method=http_method, url=url, json=json, headers=auth_headers) else: @@ -352,17 +352,14 @@ def get_identity_role_assignments(self, user_id: str) -> List[RoleAssignment]: if identity_type == "#microsoft.graph.user": graph_data = self._get_role_assignment_graph_data_for_user(user_id) elif identity_type == "#microsoft.graph.servicePrincipal": - logging.debug("before _get_role_assignment_graph_data_for_service_principal call") graph_data = self._get_role_assignment_graph_data_for_service_principal(user_id) else: - logging.debug("about to raise: AuthConfigValidationError") raise AuthConfigValidationError(f"{strings.ACCESS_UNHANDLED_ACCOUNT_TYPE} {identity_type}") if 'value' not in graph_data: logging.debug(graph_data) raise AuthConfigValidationError(f"{strings.ACCESS_UNABLE_TO_GET_ROLE_ASSIGNMENTS_FOR_USER} {user_id}") - logging.debug("about to log: graph_data") logging.debug(graph_data) return [RoleAssignment(role_assignment['resourceId'], role_assignment['appRoleId']) for role_assignment in graph_data['value']] From d2f08968e38104e68fb61e0fa4ac2e4ab82bbc2f Mon Sep 17 00:00:00 2001 From: Tamir Kamara <26870601+tamirkamara@users.noreply.github.com> Date: Mon, 22 Aug 2022 18:53:41 +0000 Subject: [PATCH 3/7] merge dict fix --- api_app/services/aad_authentication.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/api_app/services/aad_authentication.py b/api_app/services/aad_authentication.py index 7977dd4153..07058f30ce 100644 --- a/api_app/services/aad_authentication.py +++ b/api_app/services/aad_authentication.py @@ -297,7 +297,7 @@ def _ms_graph_query(self, url: str, http_method: str, json=None) -> dict: url = "" if response.status_code == 200: json_response = response.json() - graph_data.update(json_response) + graph_data = merge_dict(graph_data, json_response) if '@odata.nextLink' in json_response: url = json_response['@odata.nextLink'] return graph_data @@ -381,3 +381,15 @@ def get_workspace_role(self, user: User, workspace: Workspace, user_role_assignm if RoleAssignment(resource_id=workspace_sp_id, role_id=workspace.properties['app_role_id_workspace_airlock_manager']) in user_role_assignments: return WorkspaceRole.AirlockManager return WorkspaceRole.NoRole + + +def merge_dict(d1, d2): + dd = defaultdict(list) + + for d in (d1, d2): + for key, value in d.items(): + if isinstance(value, list): + dd[key].extend(value) + else: + dd[key].append(value) + return dict(dd) From 1fbfd27812c43e65cfa0fb0cc53647b31e1c1051 Mon Sep 17 00:00:00 2001 From: Tamir Kamara <26870601+tamirkamara@users.noreply.github.com> Date: Mon, 22 Aug 2022 18:54:19 +0000 Subject: [PATCH 4/7] rename sample --- e2e_tests/{.env.tmpl => .env.sample} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename e2e_tests/{.env.tmpl => .env.sample} (100%) diff --git a/e2e_tests/.env.tmpl b/e2e_tests/.env.sample similarity index 100% rename from e2e_tests/.env.tmpl rename to e2e_tests/.env.sample From b91932b755857bcca328c146b11e3163ec305099 Mon Sep 17 00:00:00 2001 From: Tamir Kamara <26870601+tamirkamara@users.noreply.github.com> Date: Tue, 23 Aug 2022 09:28:44 +0000 Subject: [PATCH 5/7] add log --- api_app/services/aad_authentication.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/api_app/services/aad_authentication.py b/api_app/services/aad_authentication.py index 07058f30ce..c76dfec9e0 100644 --- a/api_app/services/aad_authentication.py +++ b/api_app/services/aad_authentication.py @@ -300,6 +300,8 @@ def _ms_graph_query(self, url: str, http_method: str, json=None) -> dict: graph_data = merge_dict(graph_data, json_response) if '@odata.nextLink' in json_response: url = json_response['@odata.nextLink'] + else: + logging.error(f"MS Graph query to: {url} failed with status code {response.status_code}") return graph_data def _get_role_assignment_graph_data_for_user(self, user_id: str) -> dict: From 9c45ba493c9963b5bc5ed577fd4adb22d2adf78f Mon Sep 17 00:00:00 2001 From: Tamir Kamara <26870601+tamirkamara@users.noreply.github.com> Date: Tue, 23 Aug 2022 10:12:02 +0000 Subject: [PATCH 6/7] add log --- api_app/services/aad_authentication.py | 1 + 1 file changed, 1 insertion(+) diff --git a/api_app/services/aad_authentication.py b/api_app/services/aad_authentication.py index c76dfec9e0..b0eff41569 100644 --- a/api_app/services/aad_authentication.py +++ b/api_app/services/aad_authentication.py @@ -302,6 +302,7 @@ def _ms_graph_query(self, url: str, http_method: str, json=None) -> dict: url = json_response['@odata.nextLink'] else: logging.error(f"MS Graph query to: {url} failed with status code {response.status_code}") + logging.error(f"Full response: {response}") return graph_data def _get_role_assignment_graph_data_for_user(self, user_id: str) -> dict: From 583888ad49cc181d8052d1aa8f5288946e6cbbe2 Mon Sep 17 00:00:00 2001 From: Tamir Kamara <26870601+tamirkamara@users.noreply.github.com> Date: Tue, 23 Aug 2022 10:24:49 +0000 Subject: [PATCH 7/7] dump version --- api_app/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api_app/_version.py b/api_app/_version.py index 2dd5536049..8f584e6225 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.4.18" +__version__ = "0.4.19"