diff --git a/CHANGELOG.md b/CHANGELOG.md index f5f543f338..8f8db140af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ ENHANCEMENTS: * UI is deployed by default ([#2554](https://github.com/microsoft/AzureTRE/pull/2554)) * Remove manual/makefile option to install Gitea/Nexus ([#2573](https://github.com/microsoft/AzureTRE/pull/2573)) * Exact Terraform provider versions in bundles ([#2579](https://github.com/microsoft/AzureTRE/pull/2579)) +* Stabilize E2E tests by issuing the access token prior using it, hence, reducing the change of expired token ([#2572](https://github.com/microsoft/AzureTRE/pull/2572)) BUG FIXES: @@ -32,6 +33,7 @@ BUG FIXES: * Temporary disable AppInsight's private endpoint in base workspace ([#2543](https://github.com/microsoft/AzureTRE/pull/2543)) * Resource Processor execution optimization (`porter show`) for long-standing services ([#2542](https://github.com/microsoft/AzureTRE/pull/2542)) * Move AML Compute deployment to use AzApi Terraform Provider {[#2555]((https://github.com/microsoft/AzureTRE/pull/2555)) +* Invalid token exceptions in the API app are catched, throwing 401 instead of 500 Internal server error ([#2572](https://github.com/microsoft/AzureTRE/pull/2572)) ## 0.4.2 (August 23, 2022) diff --git a/api_app/_version.py b/api_app/_version.py index 7fe0489074..b6f65f35da 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.4.29" +__version__ = "0.4.30" diff --git a/api_app/resources/strings.py b/api_app/resources/strings.py index a12759caad..9063eddc8a 100644 --- a/api_app/resources/strings.py +++ b/api_app/resources/strings.py @@ -99,6 +99,9 @@ AUTH_CONFIGURATION_NOT_AVAILABLE_FOR_WORKSPACE = "Auth configuration not available for workspace" AUTH_UNABLE_TO_VALIDATE_TOKEN = "Unable to decode or validate token" INVALID_AUTH_PROVIDER = "Invalid authentication provider" +INVALID_SIGNATURE = "Invalid token signature" +EXPIRED_SIGNATURE = "Expired token signature" +INVALID_TOKEN = "Invalid token" UNABLE_TO_REPLACE_CURRENT_TEMPLATE = "Unable to replace the existing 'current' template with this name" UNABLE_TO_PROCESS_REQUEST = "Unable to process request" diff --git a/api_app/services/aad_authentication.py b/api_app/services/aad_authentication.py index a4f0c22d22..060313ecc2 100644 --- a/api_app/services/aad_authentication.py +++ b/api_app/services/aad_authentication.py @@ -67,7 +67,18 @@ async def __call__(self, request: Request) -> User: try: decoded_token = self._decode_token(token, config.API_AUDIENCE) except jwt.exceptions.InvalidSignatureError: - logging.debug("Failed to decode using TRE API app registration") + logging.debug("Failed to decode using TRE API app registration (Invalid Signatrue)") + raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail=strings.INVALID_SIGNATURE) + except jwt.exceptions.ExpiredSignatureError: + logging.debug("Failed to decode using TRE API app registration (Expired Signature)") + raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail=strings.EXPIRED_SIGNATURE) + except jwt.exceptions.InvalidTokenError: + # any other token validation exception, we want to catch all of these... + logging.debug("Failed to decode using TRE API app registration (Invalid token)") + raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail=strings.INVALID_TOKEN) + except Exception as e: + # Unexpected token decoding/validation exception. making sure we are not crashing (with 500) + logging.debug(e) pass # Failed to decode token using either app registration diff --git a/e2e_tests/conftest.py b/e2e_tests/conftest.py index 8ada6f42bb..a30039c4f7 100644 --- a/e2e_tests/conftest.py +++ b/e2e_tests/conftest.py @@ -1,11 +1,5 @@ -from json import JSONDecodeError import pytest -from httpx import AsyncClient -from starlette import status - -import config - pytestmark = pytest.mark.asyncio @@ -19,30 +13,3 @@ def verify(pytestconfig): return True elif pytestconfig.getoption("verify").lower() == "false": return False - - -@pytest.fixture -async def admin_token(verify) -> str: - async with AsyncClient(verify=verify) as client: - responseJson = "" - headers = {'Content-Type': "application/x-www-form-urlencoded"} - if config.TEST_ACCOUNT_CLIENT_ID != "" and config.TEST_ACCOUNT_CLIENT_SECRET != "": - # Use Client Credentials flow - payload = f"grant_type=client_credentials&client_id={config.TEST_ACCOUNT_CLIENT_ID}&client_secret={config.TEST_ACCOUNT_CLIENT_SECRET}&scope=api://{config.API_CLIENT_ID}/.default" - url = f"https://login.microsoftonline.com/{config.AAD_TENANT_ID}/oauth2/v2.0/token" - - else: - # Use Resource Owner Password Credentials flow - payload = f"grant_type=password&resource={config.API_CLIENT_ID}&username={config.TEST_USER_NAME}&password={config.TEST_USER_PASSWORD}&scope=api://{config.API_CLIENT_ID}/user_impersonation&client_id={config.TEST_APP_ID}" - url = f"https://login.microsoftonline.com/{config.AAD_TENANT_ID}/oauth2/token" - - response = await client.post(url, headers=headers, content=payload) - try: - responseJson = response.json() - except JSONDecodeError: - assert False, "Failed to parse response as JSON: {}".format(response.content) - - assert "access_token" in responseJson, "Failed to get access_token: {}".format(response.content) - token = responseJson["access_token"] - assert token is not None, "Token not returned" - return token if (response.status_code == status.HTTP_200_OK) else None diff --git a/e2e_tests/helpers.py b/e2e_tests/helpers.py index 6b2ac9c693..ddb237a68c 100644 --- a/e2e_tests/helpers.py +++ b/e2e_tests/helpers.py @@ -1,3 +1,5 @@ +from json import JSONDecodeError + import asyncio from typing import Optional from contextlib import asynccontextmanager @@ -101,3 +103,29 @@ async def check_aad_auth_redirect(endpoint, verify) -> None: valid_redirection_contains = ["login", "microsoftonline", "oauth2", "authorize"] assert all(word in location for word in valid_redirection_contains), "Redirect URL doesn't apper to be valid" + + +async def get_admin_token(verify) -> str: + async with AsyncClient(verify=verify) as client: + responseJson = "" + headers = {'Content-Type': "application/x-www-form-urlencoded"} + if config.TEST_ACCOUNT_CLIENT_ID != "" and config.TEST_ACCOUNT_CLIENT_SECRET != "": + # Use Client Credentials flow + payload = f"grant_type=client_credentials&client_id={config.TEST_ACCOUNT_CLIENT_ID}&client_secret={config.TEST_ACCOUNT_CLIENT_SECRET}&scope=api://{config.API_CLIENT_ID}/.default" + url = f"https://login.microsoftonline.com/{config.AAD_TENANT_ID}/oauth2/v2.0/token" + + else: + # Use Resource Owner Password Credentials flow + payload = f"grant_type=password&resource={config.API_CLIENT_ID}&username={config.TEST_USER_NAME}&password={config.TEST_USER_PASSWORD}&scope=api://{config.API_CLIENT_ID}/user_impersonation&client_id={config.TEST_APP_ID}" + url = f"https://login.microsoftonline.com/{config.AAD_TENANT_ID}/oauth2/token" + + response = await client.post(url, headers=headers, content=payload) + try: + responseJson = response.json() + except JSONDecodeError: + assert False, "Failed to parse response as JSON: {}".format(response.content) + + assert "access_token" in responseJson, "Failed to get access_token: {}".format(response.content) + token = responseJson["access_token"] + assert token is not None, "Token not returned" + return token if (response.status_code == status.HTTP_200_OK) else None diff --git a/e2e_tests/resources/resource.py b/e2e_tests/resources/resource.py index ffd82e4186..c1efc1707f 100644 --- a/e2e_tests/resources/resource.py +++ b/e2e_tests/resources/resource.py @@ -67,8 +67,8 @@ async def disable_and_delete_resource(endpoint, access_token, verify): async def wait_for(func, client, operation_endpoint, headers, failure_states: list): done, done_state, message = await func(client, operation_endpoint, headers) + LOGGER.info(f'WAITING FOR OP: {operation_endpoint}') while not done: - LOGGER.info(f'WAITING FOR OP: {operation_endpoint}') await asyncio.sleep(30) done, done_state, message = await func(client, operation_endpoint, headers) diff --git a/e2e_tests/test_airlock.py b/e2e_tests/test_airlock.py index 9128f8fed8..0140b79835 100644 --- a/e2e_tests/test_airlock.py +++ b/e2e_tests/test_airlock.py @@ -13,6 +13,7 @@ from airlock.request import post_request, get_request, upload_blob_using_sas, wait_for_status from airlock import strings as airlock_strings +from helpers import get_admin_token pytestmark = pytest.mark.asyncio LOGGER = logging.getLogger(__name__) @@ -23,8 +24,9 @@ @pytest.mark.airlock @pytest.mark.extended @pytest.mark.timeout(2000) -async def test_airlock_import_flow(admin_token, verify) -> None: +async def test_airlock_import_flow(verify) -> None: + admin_token = await get_admin_token(verify) if config.TEST_AIRLOCK_WORKSPACE_ID != "": workspace_id = config.TEST_AIRLOCK_WORKSPACE_ID workspace_path = f"/workspaces/{workspace_id}" @@ -125,4 +127,5 @@ async def test_airlock_import_flow(admin_token, verify) -> None: if config.TEST_AIRLOCK_WORKSPACE_ID == "": # 8. delete workspace LOGGER.info("Deleting workspace") + admin_token = await get_admin_token(verify) await disable_and_delete_resource(f'/api{workspace_path}', admin_token, verify) diff --git a/e2e_tests/test_performance.py b/e2e_tests/test_performance.py index 8eacb01e86..0a0e97be2f 100644 --- a/e2e_tests/test_performance.py +++ b/e2e_tests/test_performance.py @@ -5,12 +5,14 @@ from resources.resource import disable_and_delete_resource, post_resource from resources import strings +from helpers import get_admin_token + pytestmark = pytest.mark.asyncio @pytest.mark.performance @pytest.mark.timeout(3000) -async def test_parallel_resource_creations(admin_token, verify) -> None: +async def test_parallel_resource_creations(verify) -> None: """Creates N workspaces in parallel, and creates a workspace service in each, in parallel""" number_workspaces = 2 @@ -28,6 +30,7 @@ async def test_parallel_resource_creations(admin_token, verify) -> None: } } + admin_token = await get_admin_token(verify) task = asyncio.create_task(post_resource(payload=payload, endpoint=strings.API_WORKSPACES, access_token=admin_token, verify=verify)) tasks.append(task) @@ -45,7 +48,7 @@ async def test_parallel_resource_creations(admin_token, verify) -> None: @pytest.mark.skip @pytest.mark.performance @pytest.mark.timeout(3000) -async def test_bulk_updates_to_ensure_each_resource_updated_in_series(admin_token, verify) -> None: +async def test_bulk_updates_to_ensure_each_resource_updated_in_series(verify) -> None: """Optionally creates a workspace and workspace service, then creates N number of VMs in parallel, patches each, and deletes them""" @@ -69,6 +72,7 @@ async def test_bulk_updates_to_ensure_each_resource_updated_in_series(admin_toke } } + admin_token = await get_admin_token(verify) workspace_path, workspace_id = await post_resource(payload, strings.API_WORKSPACES, admin_token, verify) else: workspace_path = f"/workspaces/{config.PERF_TEST_WORKSPACE_ID}" @@ -140,6 +144,7 @@ async def test_bulk_updates_to_ensure_each_resource_updated_in_series(admin_toke await asyncio.gather(*tasks) + admin_token = await get_admin_token(verify) # clear up workspace + service (if we created them) if config.PERF_TEST_WORKSPACE_SERVICE_ID == "": await disable_and_delete_resource(f'/api{workspace_service_path}', workspace_owner_token, verify) diff --git a/e2e_tests/test_shared_service_templates.py b/e2e_tests/test_shared_service_templates.py index 02792e6604..2faafafb6a 100644 --- a/e2e_tests/test_shared_service_templates.py +++ b/e2e_tests/test_shared_service_templates.py @@ -6,6 +6,7 @@ import config from helpers import get_auth_header, get_template from resources import strings +from helpers import get_admin_token shared_service_templates = [ (strings.FIREWALL_SHARED_SERVICE), @@ -15,8 +16,9 @@ @pytest.mark.smoke @pytest.mark.parametrize("template_name", shared_service_templates) -async def test_get_shared_service_templates(template_name, admin_token, verify) -> None: +async def test_get_shared_service_templates(template_name, verify) -> None: async with AsyncClient(verify=verify) as client: + admin_token = await get_admin_token(verify) response = await client.get(f"https://{config.TRE_ID}.{config.RESOURCE_LOCATION}.cloudapp.azure.com{strings.API_SHARED_SERVICE_TEMPLATES}", headers=get_auth_header(admin_token)) template_names = [templates["name"] for templates in response.json()["templates"]] @@ -25,6 +27,7 @@ async def test_get_shared_service_templates(template_name, admin_token, verify) @pytest.mark.smoke @pytest.mark.parametrize("template_name", shared_service_templates) -async def test_get_shared_service_template(template_name, admin_token, verify) -> None: +async def test_get_shared_service_template(template_name, verify) -> None: + admin_token = await get_admin_token(verify) async with get_template(template_name, strings.API_SHARED_SERVICE_TEMPLATES, admin_token, verify) as response: assert (response.status_code == status.HTTP_200_OK), f"GET Request for {template_name} failed" diff --git a/e2e_tests/test_shared_services.py b/e2e_tests/test_shared_services.py index 5526595791..878a789dab 100644 --- a/e2e_tests/test_shared_services.py +++ b/e2e_tests/test_shared_services.py @@ -4,13 +4,13 @@ from resources.resource import disable_and_delete_resource, post_resource from helpers import get_shared_service_id_by_name from resources import strings - +from helpers import get_admin_token LOGGER = logging.getLogger(__name__) @pytest.mark.shared_services -async def test_patch_firewall(admin_token, verify): +async def test_patch_firewall(verify): template_name = strings.FIREWALL_SHARED_SERVICE patch_payload = { @@ -70,6 +70,7 @@ async def test_patch_firewall(admin_token, verify): "templateName": template_name, } + admin_token = await get_admin_token(verify) shared_service_firewall = await get_shared_service_id_by_name( template_name, verify, admin_token ) @@ -93,7 +94,8 @@ async def test_patch_firewall(admin_token, verify): @pytest.mark.shared_services @pytest.mark.timeout(65 * 60) @pytest.mark.parametrize("template_name", shared_service_templates_to_create) -async def test_create_shared_service(template_name, admin_token, verify) -> None: +async def test_create_shared_service(template_name, verify) -> None: + admin_token = await get_admin_token(verify) # Check that the shared service hasn't already been created shared_service = await get_shared_service_id_by_name( template_name, verify, admin_token @@ -122,6 +124,7 @@ async def test_create_shared_service(template_name, admin_token, verify) -> None verify=verify, ) + admin_token = await get_admin_token(verify) await disable_and_delete_resource( f"/api{shared_service_path}", admin_token, verify ) diff --git a/e2e_tests/test_workspace_service_templates.py b/e2e_tests/test_workspace_service_templates.py index e8a0fe5363..1ccb2a8a83 100644 --- a/e2e_tests/test_workspace_service_templates.py +++ b/e2e_tests/test_workspace_service_templates.py @@ -6,7 +6,7 @@ import config from helpers import get_auth_header, get_template from resources import strings - +from helpers import get_admin_token pytestmark = pytest.mark.asyncio @@ -20,8 +20,9 @@ @pytest.mark.smoke @pytest.mark.parametrize("template_name", workspace_service_templates) -async def test_get_workspace_service_templates(template_name, admin_token, verify) -> None: +async def test_get_workspace_service_templates(template_name, verify) -> None: async with AsyncClient(verify=verify) as client: + admin_token = await get_admin_token(verify) response = await client.get(f"https://{config.TRE_ID}.{config.RESOURCE_LOCATION}.cloudapp.azure.com{strings.API_WORKSPACE_SERVICE_TEMPLATES}", headers=get_auth_header(admin_token)) template_names = [templates["name"] for templates in response.json()["templates"]] @@ -30,13 +31,14 @@ async def test_get_workspace_service_templates(template_name, admin_token, verif @pytest.mark.smoke @pytest.mark.parametrize("template_name", workspace_service_templates) -async def test_get_workspace_service_template(template_name, admin_token, verify) -> None: +async def test_get_workspace_service_template(template_name, verify) -> None: + admin_token = await get_admin_token(verify) async with get_template(template_name, strings.API_WORKSPACE_SERVICE_TEMPLATES, admin_token, verify) as response: assert (response.status_code == status.HTTP_200_OK), f"GET Request for {template_name} failed" @pytest.mark.smoke -async def test_create_workspace_service_templates(admin_token, verify) -> None: +async def test_create_workspace_service_templates(verify) -> None: async with AsyncClient(verify=verify) as client: payload = { "name": f"{strings.TEST_WORKSPACE_SERVICE_TEMPLATE}", @@ -53,6 +55,7 @@ async def test_create_workspace_service_templates(admin_token, verify) -> None: } } + admin_token = await get_admin_token(verify) response = await client.post(f"https://{config.TRE_ID}.{config.RESOURCE_LOCATION}.cloudapp.azure.com{strings.API_WORKSPACE_SERVICE_TEMPLATES}", headers=get_auth_header(admin_token), json=payload) assert (response.status_code == status.HTTP_201_CREATED or response.status_code == status.HTTP_409_CONFLICT), "The workspace service template creation service returned unexpected response." diff --git a/e2e_tests/test_workspace_services.py b/e2e_tests/test_workspace_services.py index d1ec73ff84..feca64e510 100644 --- a/e2e_tests/test_workspace_services.py +++ b/e2e_tests/test_workspace_services.py @@ -5,14 +5,14 @@ from resources.workspace import get_workspace_auth_details from resources.resource import disable_and_delete_resource, post_resource from resources import strings - +from helpers import get_admin_token pytestmark = pytest.mark.asyncio @pytest.mark.extended @pytest.mark.timeout(75 * 60) -async def test_create_guacamole_service_into_base_workspace(admin_token, verify) -> None: +async def test_create_guacamole_service_into_base_workspace(verify) -> None: payload = { "templateName": strings.BASE_WORKSPACE, @@ -27,6 +27,7 @@ async def test_create_guacamole_service_into_base_workspace(admin_token, verify) if config.TEST_WORKSPACE_APP_PLAN != "": payload["properties"]["app_service_plan_sku"] = config.TEST_WORKSPACE_APP_PLAN + admin_token = await get_admin_token(verify) workspace_path, workspace_id = await post_resource(payload, strings.API_WORKSPACES, access_token=admin_token, verify=verify) workspace_owner_token, scope_uri = await get_workspace_auth_details(admin_token=admin_token, workspace_id=workspace_id, verify=verify) @@ -67,12 +68,13 @@ async def test_create_guacamole_service_into_base_workspace(admin_token, verify) await disable_and_delete_resource(f'/api{workspace_service_path}', workspace_owner_token, verify) + admin_token = await get_admin_token(verify) await disable_and_delete_resource(f'/api{workspace_path}', admin_token, verify) @pytest.mark.extended_aad @pytest.mark.timeout(75 * 60) -async def test_create_guacamole_service_into_aad_workspace(admin_token, verify) -> None: +async def test_create_guacamole_service_into_aad_workspace(verify) -> None: """This test will create a Guacamole service but will create a workspace and automatically register the AAD Application""" payload = { @@ -87,6 +89,7 @@ async def test_create_guacamole_service_into_aad_workspace(admin_token, verify) if config.TEST_WORKSPACE_APP_PLAN != "": payload["properties"]["app_service_plan_sku"] = config.TEST_WORKSPACE_APP_PLAN + admin_token = await get_admin_token(verify) workspace_path, workspace_id = await post_resource(payload, strings.API_WORKSPACES, access_token=admin_token, verify=verify) workspace_owner_token, scope_uri = await get_workspace_auth_details(admin_token=admin_token, workspace_id=workspace_id, verify=verify) @@ -127,6 +130,7 @@ async def test_create_guacamole_service_into_aad_workspace(admin_token, verify) await disable_and_delete_resource(f'/api{workspace_service_path}', workspace_owner_token, verify) + admin_token = await get_admin_token(verify) await disable_and_delete_resource(f'/api{workspace_path}', admin_token, verify) diff --git a/e2e_tests/test_workspace_templates.py b/e2e_tests/test_workspace_templates.py index e8d6062a8d..d2e29fd39a 100644 --- a/e2e_tests/test_workspace_templates.py +++ b/e2e_tests/test_workspace_templates.py @@ -6,6 +6,7 @@ import config from helpers import get_auth_header, get_template from resources import strings +from helpers import get_admin_token pytestmark = pytest.mark.asyncio @@ -18,8 +19,9 @@ @pytest.mark.smoke @pytest.mark.parametrize("template_name", workspace_templates) -async def test_get_workspace_templates(template_name, admin_token, verify) -> None: +async def test_get_workspace_templates(template_name, verify) -> None: async with AsyncClient(verify=verify) as client: + admin_token = await get_admin_token(verify) response = await client.get(f"https://{config.TRE_ID}.{config.RESOURCE_LOCATION}.cloudapp.azure.com{strings.API_WORKSPACE_TEMPLATES}", headers=get_auth_header(admin_token)) template_names = [templates["name"] for templates in response.json()["templates"]] @@ -28,6 +30,7 @@ async def test_get_workspace_templates(template_name, admin_token, verify) -> No @pytest.mark.smoke @pytest.mark.parametrize("template_name", workspace_templates) -async def test_get_workspace_template(template_name, admin_token, verify) -> None: +async def test_get_workspace_template(template_name, verify) -> None: + admin_token = await get_admin_token(verify) async with get_template(template_name, strings.API_WORKSPACE_TEMPLATES, admin_token, verify) as response: assert (response.status_code == status.HTTP_200_OK), f"GET Request for {template_name} creation failed"