diff --git a/api_app/_version.py b/api_app/_version.py index 1cdae7a8ac..be2d7c2ffe 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.2.24" +__version__ = "0.2.27" diff --git a/api_app/api/routes/api.py b/api_app/api/routes/api.py index 7178ea31a8..a7e5cae8d2 100644 --- a/api_app/api/routes/api.py +++ b/api_app/api/routes/api.py @@ -93,6 +93,11 @@ async def swagger_ui_redirect(): workspace_swagger_router = APIRouter() +def get_scope(workspace) -> str: + # Cope with the fact that scope id can have api:// at the front. + return f"api://{workspace['properties']['scope_id'].lstrip('api://')}/user_impersonation" + + @workspace_swagger_router.get("/workspaces/{workspace_id}/openapi.json", include_in_schema=False, name="openapi_definitions") async def get_openapi_json(workspace_id: str, request: Request, workspace_repo=Depends(get_repository(WorkspaceRepository))): global openapi_definitions @@ -108,11 +113,9 @@ async def get_openapi_json(workspace_id: str, request: Request, workspace_repo=D ) workspace = workspace_repo.get_workspace_by_id(workspace_id) - ws_app_reg_id = workspace.properties['client_id'] - workspace_scopes = { - f"api://{ws_app_reg_id}/user_impersonation": "List and Get TRE Workspaces" - } - openapi_definitions[workspace_id]['components']['securitySchemes']['oauth2']['flows']['authorizationCode']['scopes'] = workspace_scopes + scope = {get_scope(workspace): "List and Get TRE Workspaces"} + + openapi_definitions[workspace_id]['components']['securitySchemes']['oauth2']['flows']['authorizationCode']['scopes'] = scope # Add an example into every workspace_id path parameter so users don't have to cut and paste them in. for route in openapi_definitions[workspace_id]['paths'].values(): @@ -129,7 +132,7 @@ async def get_openapi_json(workspace_id: str, request: Request, workspace_repo=D async def get_workspace_swagger(workspace_id, request: Request, workspace_repo=Depends(get_repository(WorkspaceRepository))): workspace = workspace_repo.get_workspace_by_id(workspace_id) - ws_app_reg_id = workspace.properties['client_id'] + scope = get_scope(workspace) swagger_ui_html = get_swagger_ui_html( openapi_url="openapi.json", title=request.app.title + " - Swagger UI", @@ -137,7 +140,7 @@ async def get_workspace_swagger(workspace_id, request: Request, workspace_repo=D init_oauth={ "usePkceWithAuthorizationCodeGrant": True, "clientId": config.SWAGGER_UI_CLIENT_ID, - "scopes": ["openid", "offline_access", f"api://{ws_app_reg_id}/user_impersonation"] + "scopes": ["openid", "offline_access", scope] } ) diff --git a/api_app/db/migrations/workspaces.py b/api_app/db/migrations/workspaces.py index b290fd5829..113a0b7dd3 100644 --- a/api_app/db/migrations/workspaces.py +++ b/api_app/db/migrations/workspaces.py @@ -13,22 +13,33 @@ def moveAuthInformationToProperties(self) -> bool: migrated = False for item in self.query(query=WorkspaceRepository.workspaces_query_string()): template_version = semantic_version.Version(item["templateVersion"]) - if (template_version < semantic_version.Version('0.2.5') and "authInformation" in item): - logging.INFO(f'Found workspace {item["id"]} that needs migrating') + updated = False + if (template_version < semantic_version.Version('0.2.7')): # Rename app_id to be client_id - item["properties"]["client_id"] = item["properties"]["app_id"] - del item["properties"]["app_id"] - - # Copy authInformation into properties - item["properties"]["sp_id"] = item["authInformation"]["sp_id"] - item["properties"]["app_role_id_workspace_researcher"] = item["authInformation"]["roles"]["WorkspaceResearcher"] - item["properties"]["app_role_id_workspace_owner"] = item["authInformation"]["roles"]["WorkspaceOwner"] - - # cleanup - del item["authInformation"] - self.update_item_dict(item) - logging.INFO(f'Upgraded authentication info for workspace id {item["id"]}') - migrated = True + if "app_id" in item["properties"]: + item["properties"]["client_id"] = item["properties"]["app_id"] + del item["properties"]["app_id"] + updated = True + + if "scope_id" not in item["properties"]: + item["properties"]["scope_id"] = f"api://{item['properties']['client_id']}" + updated = True + + if "authInformation" in item: + logging.INFO(f'Upgrading authInformation in workspace {item["id"]}') + + # Copy authInformation into properties + item["properties"]["sp_id"] = item["authInformation"]["sp_id"] + item["properties"]["app_role_id_workspace_researcher"] = item["authInformation"]["roles"]["WorkspaceResearcher"] + item["properties"]["app_role_id_workspace_owner"] = item["authInformation"]["roles"]["WorkspaceOwner"] + # cleanup + del item["authInformation"] + updated = True + + if updated: + self.update_item_dict(item) + logging.INFO(f'Upgraded authentication info for workspace id {item["id"]}') + migrated = True return migrated diff --git a/api_app/services/aad_authentication.py b/api_app/services/aad_authentication.py index 881faeaef0..f7a6af5f7e 100644 --- a/api_app/services/aad_authentication.py +++ b/api_app/services/aad_authentication.py @@ -186,6 +186,9 @@ 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 + # This method is called when you create a workspace and you already have an AAD App Registration + # to link it to. You pass in teh client_id and go and get the extra information you need from AAD + # If the client_id is `auto_create`, then these values will be written by Terraform. def _get_app_auth_info(self, client_id: str) -> dict: graph_data = self._get_app_sp_graph_data(client_id) if 'value' not in graph_data or len(graph_data['value']) == 0: @@ -193,7 +196,7 @@ def _get_app_auth_info(self, client_id: str) -> dict: raise AuthConfigValidationError(f"{strings.ACCESS_UNABLE_TO_GET_INFO_FOR_APP} {client_id}") app_info = graph_data['value'][0] - authInfo = {'sp_id': app_info['id']} + authInfo = {'sp_id': app_info['id'], 'scope_id': app_info['servicePrincipalNames'][0]} # Convert the roles into ids (We could have more roles defined in the app than we need.) for appRole in app_info['appRoles']: 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 e4588d184e..a4c9d598c9 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 @@ -43,12 +43,16 @@ def test_extract_workspace__returns_sp_id_and_roles(get_app_sp_graph_data_mock): 'appRoles': [ {'id': '1abc3', 'value': 'WorkspaceResearcher'}, {'id': '1abc4', 'value': 'WorkspaceOwner'}, + ], + 'servicePrincipalNames': [ + "api://tre_ws_1234" ] } ] } expected_auth_info = { "sp_id": "12345", + "scope_id": "api://tre_ws_1234", "app_role_id_workspace_owner": "1abc4", "app_role_id_workspace_researcher": "1abc3" } diff --git a/docs/tre-admins/auth.md b/docs/tre-admins/auth.md index 251ad58964..7e8707d06f 100644 --- a/docs/tre-admins/auth.md +++ b/docs/tre-admins/auth.md @@ -192,7 +192,9 @@ Same as [TRE API](#authentication-tre-api). | API/permission name | Type | Description | Admin consent required | | ------------------- | ---- | ----------- | ---------------------- | | Microsoft Graph/User.Read (`https://graph.microsoft.com/User.Read`) | Delegated | Allows users to sign-in to the app, and allows the app to read the profile of signed-in users. It also allows the app to read basic company information of signed-in users. | No | -| Workspace API/user_impersonation (`api:///user_impersonation`) | Delegated* | Allows the app to access the workspace API on behalf of the user | No | Granted for *[directory name]* | +| Workspace API/user_impersonation (`api:///user_impersonation`) | Delegated* | Allows the app to access the workspace API on behalf of the user | No | Granted for *[directory name]* | + +When the Workspace AAD app is registered by the aad-app-reg.sh, the `Workspace Scope Id` is the same as the Client Id. When the Workspace AAD app is created by the base workspace, the `Workspace Scope Id` will be in this format `api://_ws_` #### App roles diff --git a/e2e_tests/conftest.py b/e2e_tests/conftest.py index 8bddd52106..8ada6f42bb 100644 --- a/e2e_tests/conftest.py +++ b/e2e_tests/conftest.py @@ -46,31 +46,3 @@ async def admin_token(verify) -> str: token = responseJson["access_token"] assert token is not None, "Token not returned" return token if (response.status_code == status.HTTP_200_OK) else None - - -@pytest.fixture -async def workspace_owner_token(verify) -> str: - async with AsyncClient(verify=verify) as client: - - 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.TEST_WORKSPACE_APP_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.TEST_WORKSPACE_APP_ID}&username={config.TEST_USER_NAME}&password={config.TEST_USER_PASSWORD}&scope=api://{config.TEST_WORKSPACE_APP_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 1387cefe1b..d042a0b2b5 100644 --- a/e2e_tests/helpers.py +++ b/e2e_tests/helpers.py @@ -5,7 +5,7 @@ import logging from starlette import status - +from json import JSONDecodeError import config from resources import strings @@ -33,28 +33,28 @@ def get_auth_header(token: str) -> dict: return {'Authorization': f'Bearer {token}'} +def get_full_endpoint(endpoint: str) -> str: + if (config.TRE_URL != ""): + return f"{config.TRE_URL}{endpoint}" + else: + return f"https://{config.TRE_ID}.{config.RESOURCE_LOCATION}.cloudapp.azure.com{endpoint}" + + @asynccontextmanager async def get_template(template_name, endpoint, admin_token, verify): async with AsyncClient(verify=verify) as client: headers = {'Authorization': f'Bearer {admin_token}'} + full_endpoint = get_full_endpoint(endpoint) - response = await client.get(f"https://{config.TRE_ID}.{config.RESOURCE_LOCATION}.cloudapp.azure.com{endpoint}/{template_name}", headers=headers, timeout=TIMEOUT) + response = await client.get(f"{full_endpoint}/{template_name}", headers=headers, timeout=TIMEOUT) yield response -async def post_resource(payload, endpoint, resource_type, token, admin_token, verify, method="POST", wait=True): +async def post_resource(payload, endpoint, access_token, verify, method="POST", wait=True): async with AsyncClient(verify=verify, timeout=30.0) as client: - if resource_type == 'workspace': - auth_headers = get_auth_header(admin_token) - else: - auth_headers = get_auth_header(token) - - if (config.TRE_URL != ""): - full_endpoint = f"{config.TRE_URL}{endpoint}" - else: - full_endpoint = f"https://{config.TRE_ID}.{config.RESOURCE_LOCATION}.cloudapp.azure.com{endpoint}" - LOGGER.info(f'POSTING RESOURCE TO: {full_endpoint}') + full_endpoint = get_full_endpoint(endpoint) + auth_headers = get_auth_header(access_token) if method == "POST": response = await client.post(full_endpoint, headers=auth_headers, json=payload, timeout=TIMEOUT) @@ -72,17 +72,14 @@ async def post_resource(payload, endpoint, resource_type, token, admin_token, ve operation_endpoint = response.headers["Location"] if wait: - await wait_for(check_method, client, operation_endpoint, get_auth_header(token), strings.RESOURCE_STATUS_FAILED) + await wait_for(check_method, client, operation_endpoint, auth_headers, strings.RESOURCE_STATUS_FAILED) return resource_path, resource_id async def get_shared_service_id_by_name(template_name: str, verify, token) -> Optional[dict]: async with AsyncClient(verify=verify, timeout=TIMEOUT) as client: - endpoint = '/api/shared-services' - full_endpoint = f'https://{config.TRE_ID}.{config.RESOURCE_LOCATION}.cloudapp.azure.com{endpoint}' - LOGGER.info(f'URL: {full_endpoint}') - + full_endpoint = get_full_endpoint('/api/shared-services') auth_headers = get_auth_header(token) response = await client.get(full_endpoint, headers=auth_headers, timeout=TIMEOUT) @@ -97,18 +94,13 @@ async def get_shared_service_id_by_name(template_name: str, verify, token) -> Op return matching_shared_services[0] -async def disable_and_delete_resource(endpoint, resource_type, token, admin_token, verify): +async def disable_and_delete_resource(endpoint, access_token, verify): async with AsyncClient(verify=verify, timeout=TIMEOUT) as client: - if resource_type == 'workspace': - auth_headers = get_auth_header(admin_token) - else: - auth_headers = get_auth_header(token) - + full_endpoint = get_full_endpoint(endpoint) + auth_headers = get_auth_header(access_token) auth_headers["etag"] = "*" # for now, send in the wildcard to skip around etag checking - full_endpoint = f'https://{config.TRE_ID}.{config.RESOURCE_LOCATION}.cloudapp.azure.com{endpoint}' - # disable payload = {"isEnabled": False} response = await client.patch(full_endpoint, headers=auth_headers, json=payload, timeout=TIMEOUT) @@ -124,8 +116,7 @@ async def disable_and_delete_resource(endpoint, resource_type, token, admin_toke resource_id = response.json()["operation"]["resourceId"] operation_endpoint = response.headers["Location"] - owner_auth_headers = get_auth_header(token) - await wait_for(delete_done, client, operation_endpoint, owner_auth_headers, strings.RESOURCE_STATUS_DELETING_FAILED) + await wait_for(delete_done, client, operation_endpoint, auth_headers, strings.RESOURCE_STATUS_DELETING_FAILED) return resource_id @@ -157,13 +148,13 @@ async def ping_guacamole_workspace_service(workspace_id, workspace_service_id, t assert (response.status_code == status.HTTP_200_OK), "Guacamole cannot be reached" -async def wait_for(func, client, operation_endoint, headers, failure_state): - done, done_state, message = await func(client, operation_endoint, headers) +async def wait_for(func, client, operation_endpoint, headers, failure_state): + done, done_state, message = await func(client, operation_endpoint, headers) while not done: - LOGGER.info(f'WAITING FOR OP: {operation_endoint}') + LOGGER.info(f'WAITING FOR OP: {operation_endpoint}') await asyncio.sleep(30) - done, done_state, message = await func(client, operation_endoint, headers) + done, done_state, message = await func(client, operation_endpoint, headers) LOGGER.info(f"{done}, {done_state}, {message}") try: assert done_state != failure_state @@ -191,8 +182,9 @@ async def patch_done(client, operation_endpoint, headers): async def check_deployment(client, operation_endpoint, headers): - response = await client.get( - f"https://{config.TRE_ID}.{config.RESOURCE_LOCATION}.cloudapp.azure.com{operation_endpoint}", headers=headers, timeout=TIMEOUT) + full_endpoint = get_full_endpoint(operation_endpoint) + + response = await client.get(full_endpoint, headers=headers, timeout=TIMEOUT) if response.status_code == 200: deployment_status = response.json()["operation"]["status"] message = response.json()["operation"]["message"] @@ -201,3 +193,57 @@ async def check_deployment(client, operation_endpoint, headers): LOGGER.error(f"Non 200 response in check_deployment: {response.status_code}") LOGGER.error(f"Full response: {response}") raise Exception("Non 200 response in check_deployment") + + +async def get_workspace(client, workspace_id: str, headers) -> dict: + full_endpoint = get_full_endpoint(f"/api/workspaces/{workspace_id}") + + response = await client.get(full_endpoint, headers=headers, timeout=TIMEOUT) + if response.status_code == 200: + return response.json()["workspace"] + else: + LOGGER.error(f"Non 200 response in get_workspace: {response.status_code}") + LOGGER.error(f"Full response: {response}") + raise Exception("Non 200 response in get_workspace") + + +async def get_identifier_uri(client, workspace_id: str, auth_headers) -> str: + workspace = await get_workspace(client, workspace_id, auth_headers) + + if ("properties" not in workspace): + raise Exception("Properties not found in workspace.") + + if ("scope_id" not in workspace["properties"]): + raise Exception("Scope Id not found in workspace properties.") + + # Cope with the fact that scope id can have api:// at the front. + return f"api://{workspace['properties']['scope_id'].lstrip('api://')}" + + +async def get_workspace_owner_token(admin_token, workspace_id, verify) -> Optional[str]: + async with AsyncClient(verify=verify) as client: + auth_headers = get_auth_header(admin_token) + scope_uri = await get_identifier_uri(client, workspace_id, auth_headers) + + if config.TEST_ACCOUNT_CLIENT_ID != "" and config.TEST_ACCOUNT_CLIENT_SECRET != "": + # Logging in as an Enterprise Application: 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={scope_uri}/.default" + url = f"https://login.microsoftonline.com/{config.AAD_TENANT_ID}/oauth2/v2.0/token" + + else: + # Logging in as a User: Use Resource Owner Password Credentials flow + payload = f"grant_type=password&resource={workspace_id}&username={config.TEST_USER_NAME}&password={config.TEST_USER_PASSWORD}&scope={scope_uri}/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=auth_headers, content=payload) + try: + responseJson = response.json() + except JSONDecodeError: + raise Exception("Failed to parse response as JSON: {}".format(response.content)) + + if "access_token" not in responseJson: + raise Exception("Failed to get access_token: {}".format(response.content)) + + token = responseJson["access_token"] + + return token if (response.status_code == status.HTTP_200_OK) else None diff --git a/e2e_tests/test_workspace_services.py b/e2e_tests/test_workspace_services.py index 68ca9e7458..baf917bda2 100644 --- a/e2e_tests/test_workspace_services.py +++ b/e2e_tests/test_workspace_services.py @@ -1,7 +1,7 @@ import pytest import config -from helpers import disable_and_delete_resource, post_resource +from helpers import disable_and_delete_resource, get_workspace_owner_token, post_resource from resources import strings @@ -10,7 +10,7 @@ @pytest.mark.extended @pytest.mark.timeout(3000) -async def test_create_guacamole_service_into_base_workspace(admin_token, workspace_owner_token, verify) -> None: +async def test_create_guacamole_service_into_base_workspace(admin_token, verify) -> None: payload = { "templateName": "tre-workspace-base", @@ -22,7 +22,8 @@ async def test_create_guacamole_service_into_base_workspace(admin_token, workspa } } - workspace_path, _ = await post_resource(payload, strings.API_WORKSPACES, 'workspace', workspace_owner_token, admin_token, verify) + workspace_path, workspace_id = await post_resource(payload, strings.API_WORKSPACES, access_token=admin_token, verify=verify) + workspace_owner_token = await get_workspace_owner_token(admin_token=admin_token, workspace_id=workspace_id, verify=verify) service_payload = { "templateName": "tre-service-guacamole", @@ -34,7 +35,7 @@ async def test_create_guacamole_service_into_base_workspace(admin_token, workspa } } - workspace_service_path, workspace_service_id = await post_resource(service_payload, f'/api{workspace_path}/{strings.API_WORKSPACE_SERVICES}', 'workspace_service', workspace_owner_token, None, verify) + workspace_service_path, workspace_service_id = await post_resource(service_payload, f'/api{workspace_path}/{strings.API_WORKSPACE_SERVICES}', workspace_owner_token, verify) # TODO: uncomment after fixing https://github.com/microsoft/AzureTRE/issues/1602 # await ping_guacamole_workspace_service(workspace_id, workspace_service_id, workspace_owner_token, verify) @@ -47,8 +48,56 @@ async def test_create_guacamole_service_into_base_workspace(admin_token, workspa } } - await post_resource(patch_payload, f'/api{workspace_service_path}', 'workspace_service', workspace_owner_token, None, verify, method="PATCH") + await post_resource(patch_payload, f'/api{workspace_service_path}', workspace_owner_token, verify, method="PATCH") - await disable_and_delete_resource(f'/api{workspace_service_path}', 'workspace_service', workspace_owner_token, None, verify) + await disable_and_delete_resource(f'/api{workspace_service_path}', workspace_owner_token, verify) - await disable_and_delete_resource(f'/api{workspace_path}', 'workspace', workspace_owner_token, admin_token, verify) + await disable_and_delete_resource(f'/api{workspace_path}', admin_token, verify) + + +@pytest.mark.extended_aad +@pytest.mark.timeout(3000) +async def test_create_guacamole_service_into_aad_workspace(admin_token, workspace_owner_token, verify) -> None: + """This test will create a Guacamole service but will create a workspace and automatically register the AAD Application""" + + payload = { + "templateName": "tre-workspace-base", + "properties": { + "display_name": "E2E test guacamole service", + "description": "workspace for E2E AAD", + "address_space_size": "small", + "client_id": "auto_create" + } + } + + workspace_path, workspace_id = await post_resource(payload, strings.API_WORKSPACES, access_token=admin_token, verify=verify) + workspace_owner_token = await get_workspace_owner_token(admin_token=admin_token, workspace_id=workspace_id, verify=verify) + + service_payload = { + "templateName": "tre-service-guacamole", + "properties": { + "display_name": "Workspace service test", + "description": "Workspace service for E2E test", + "ws_client_id": f"{config.TEST_WORKSPACE_APP_ID}", + "ws_client_secret": f"{config.TEST_WORKSPACE_APP_SECRET}" + } + } + + workspace_service_path, workspace_service_id = await post_resource(service_payload, f'/api{workspace_path}/{strings.API_WORKSPACE_SERVICES}', workspace_owner_token, verify) + + # TODO: uncomment after fixing https://github.com/microsoft/AzureTRE/issues/1602 + # await ping_guacamole_workspace_service(workspace_id, workspace_service_id, workspace_owner_token, verify) + + # patch the guac service. we'll just update the display_name but this will still force a full deployment run + # and essentially terraform no-op + patch_payload = { + "properties": { + "display_name": "Updated Guac Name", + } + } + + await post_resource(patch_payload, f'/api{workspace_service_path}', workspace_owner_token, verify, method="PATCH") + + await disable_and_delete_resource(f'/api{workspace_service_path}', workspace_owner_token, verify) + + await disable_and_delete_resource(f'/api{workspace_path}', admin_token, verify) diff --git a/scripts/create_aad_assets.sh b/scripts/create_aad_assets.sh index 09eb42fa7e..26a6dda55e 100755 --- a/scripts/create_aad_assets.sh +++ b/scripts/create_aad_assets.sh @@ -19,11 +19,12 @@ if [ "${LOGGED_IN_TENANT_ID}" != "${AAD_TENANT_ID}" ]; then CHANGED_TENANT=1 fi -# Then register an App +# Then register an App. DO NOT put quotes around ${api_app_can_create_other_applications} as this +# will break the aad-app-reg.dh script. ./scripts/aad/aad-app-reg.sh \ --name "${TRE_ID}" \ --swaggerui-redirecturl "https://${TRE_ID}.${LOCATION}.cloudapp.azure.com/api/docs/oauth2-redirect" \ - --admin-consent --automation-account "${api_app_can_create_other_applications}" + --admin-consent --automation-account ${api_app_can_create_other_applications} echo "Please copy the values above into your /templates/core/.env." read -p "Please confirm you have done this? (y/N) " -n 1 -r diff --git a/scripts/db_migrations.py b/scripts/db_migrations.py index 21dae4cacc..461c56fac7 100644 --- a/scripts/db_migrations.py +++ b/scripts/db_migrations.py @@ -92,19 +92,33 @@ def moveAuthInformationToProperties(self, resources_container_name): for item in resources_container.query_items(query='SELECT * FROM c', enable_cross_partition_query=True): template_version = semantic_version.Version(item["templateVersion"]) - if (template_version < semantic_version.Version('2.5.0') and "authInformation" in item): - print(f'Found workspace {item["id"]} that needs migrating') + updated = False + if (template_version < semantic_version.Version('0.2.9') and item["templateName"] == "tre-workspace-base"): # Rename app_id to be client_id - item["properties"]["client_id"] = item["properties"]["app_id"] - del item["properties"]["app_id"] - del item["authInformation"]["app_id"] + if "app_id" in item["properties"]: + item["properties"]["client_id"] = item["properties"]["app_id"] + del item["properties"]["app_id"] + updated = True - # merge authInformation into properties - item["properties"] = {**item["authInformation"], **item["properties"]} - del item["authInformation"] - resources_container.upsert_item(item) - print(f'Upgraded authentication info for workspace id {item["id"]}') + if "scope_id" not in item["properties"]: + item["properties"]["scope_id"] = item["properties"]["client_id"] + updated = True + + if "authInformation" in item: + print(f'Upgrading authInformation in workspace {item["id"]}') + + if "app_id" in item["authInformation"]: + del item["authInformation"]["app_id"] + + # merge authInformation into properties + item["properties"] = {**item["authInformation"], **item["properties"]} + del item["authInformation"] + updated = True + + if updated: + resources_container.upsert_item(item) + print(f'Upgraded authentication for workspace id {item["id"]}') def main(): diff --git a/templates/workspaces/base/.env.sample b/templates/workspaces/base/.env.sample index 5a87f0ed4f..4fe71f963b 100644 --- a/templates/workspaces/base/.env.sample +++ b/templates/workspaces/base/.env.sample @@ -1,26 +1,13 @@ -# These are needed if you are testing at the porter level -export ID="0df3fbe1-38f3-4f2e-9dee-3273351d9b1a" -export ADDRESS_SPACE="10.2.8.0/24" -export SHARED_STORAGE_QUOTA=50 -export KEYVAULT_PURGE_PROTECTION_ENABLED=false -export ENABLE_LOCAL_DEBUGGING=true -export REGISTER_AAD_APPLICATION=true -export CLIENT_ID=$WORKSPACE_APP_CLIENT_ID +TRE_RESOURCE_ID="__CHANGE_ME__" +AUTH_TENANT_ID="__CHANGE_ME__" +AUTH_CLIENT_ID="__CHANGE_ME__" +AUTH_CLIENT_SECRET="__CHANGE_ME__" +ARM_CLIENT_ID="__CHANGE_ME__" +ARM_CLIENT_SECRET="__CHANGE_ME__" +ARM_TENANT_ID="__CHANGE_ME__" +ARM_SUBSCRIPTION_ID="__CHANGE_ME__" -# These are needed if you are testing at the Terraform level. -export TRE_ID=__CHANGE_ME__ -export TF_VAR_tre_id=__CHANGE_ME__ -export TF_VAR_location=westeurope -export TF_VAR_tre_resource_id=0df3fbe1-38f3-4f2e-9dff-3273351d9b1a -export TF_VAR_address_space="10.2.6.0/24" -export TF_VAR_enable_local_debugging="true" -export TF_VAR_purge_protection_enabled="false" -export TF_VAR_register_aad_application="true" -export TF_VAR_client_id=$WORKSPACE_APP_CLIENT_ID - -export TF_VAR_mgmt_resource_group_name=__CHANGE_ME__ -export TF_VAR_mgmt_storage_account_name=__CHANGE_ME__ -export TF_VAR_terraform_state_container_name=tfstate -export TF_VAR_auth_tenant_id="__CHANGE_ME__" -export TF_VAR_auth_client_id="__CHANGE_ME__" -export TF_VAR_auth_client_secret="__CHANGE_ME__" +ADDRESS_SPACE="10.2.8.0/24" +SHARED_STORAGE_QUOTA=50 +ENABLE_LOCAL_DEBUGGING=true +REGISTER_AAD_APPLICATION=false diff --git a/templates/workspaces/base/parameters.json b/templates/workspaces/base/parameters.json index 150f8faae5..6a03b6620f 100755 --- a/templates/workspaces/base/parameters.json +++ b/templates/workspaces/base/parameters.json @@ -58,12 +58,6 @@ "env": "ENABLE_LOCAL_DEBUGGING" } }, - { - "name": "keyvault_purge_protection_enabled", - "source": { - "env": "KEYVAULT_PURGE_PROTECTION_ENABLED" - } - }, { "name": "register_aad_application", "source": { diff --git a/templates/workspaces/base/porter.yaml b/templates/workspaces/base/porter.yaml index c09dcf6f8b..32b0e652f2 100644 --- a/templates/workspaces/base/porter.yaml +++ b/templates/workspaces/base/porter.yaml @@ -1,6 +1,6 @@ --- name: tre-workspace-base -version: 0.2.6 +version: 0.2.13 description: "A base Azure TRE workspace" registry: azuretre @@ -54,9 +54,6 @@ parameters: - name: enable_local_debugging type: boolean default: false - - name: keyvault_purge_protection_enabled - type: boolean - default: true - name: register_aad_application type: boolean default: false @@ -64,6 +61,10 @@ parameters: - name: client_id type: string description: "The client id of the workspace in the identity provider." + - name: scope_id + type: string + default: "" + description: "The Service Principal Name or identifierUri (e.g. api://GUID" - name: sp_id type: string default: "" @@ -93,6 +94,11 @@ outputs: default: "{{ bundle.parameters.client_id }}" applyTo: - install + - name: scope_id + type: string + default: "{{ bundle.parameters.scope_id }}" + applyTo: + - install - name: sp_id default: "{{ bundle.parameters.sp_id }}" type: string @@ -115,14 +121,13 @@ install: shared_storage_quota: "{{ bundle.parameters.shared_storage_quota }}" enable_local_debugging: "{{ bundle.parameters.enable_local_debugging }}" - keyvault_purge_protection_enabled: - "{{ bundle.parameters.keyvault_purge_protection_enabled }}" register_aad_application: "{{ bundle.parameters.register_aad_application }}" auth_client_id: "{{ bundle.credentials.auth_client_id }}" auth_client_secret: "{{ bundle.credentials.auth_client_secret }}" auth_tenant_id: "{{ bundle.credentials.auth_tenant_id }}" client_id: "{{ bundle.parameters.client_id }}" + scope_id: "{{ bundle.parameters.scope_id }}" sp_id: "{{ bundle.parameters.sp_id }}" app_role_id_workspace_owner: "{{ bundle.parameters.app_role_id_workspace_owner }}" app_role_id_workspace_researcher: "{{ bundle.parameters.app_role_id_workspace_researcher }}" @@ -137,6 +142,7 @@ install: - name: app_role_id_workspace_owner - name: app_role_id_workspace_researcher - name: client_id + - name: scope_id - name: sp_id upgrade: @@ -157,8 +163,6 @@ uninstall: shared_storage_quota: "{{ bundle.parameters.shared_storage_quota }}" enable_local_debugging: "{{ bundle.parameters.enable_local_debugging }}" - keyvault_purge_protection_enabled: - "{{ bundle.parameters.keyvault_purge_protection_enabled }}" register_aad_application: "{{ bundle.parameters.register_aad_application }}" auth_client_id: "{{ bundle.credentials.auth_client_id }}" diff --git a/templates/workspaces/base/terraform/.terraform.lock.hcl b/templates/workspaces/base/terraform/.terraform.lock.hcl index dcce8e87d2..5c12a0f59f 100644 --- a/templates/workspaces/base/terraform/.terraform.lock.hcl +++ b/templates/workspaces/base/terraform/.terraform.lock.hcl @@ -22,21 +22,41 @@ provider "registry.terraform.io/hashicorp/azuread" { } provider "registry.terraform.io/hashicorp/azurerm" { - version = "3.1.0" - constraints = "3.1.0" + version = "3.4.0" + constraints = "3.4.0" hashes = [ - "h1:ogkXoVfaEWZgkF+ZxPlKBBHXGENb+4cz9Quj3xjhcC4=", - "zh:39f925413e88c608f5319e01a540505d5ac4a1aa3073ad31823f5e9f8a00aafa", - "zh:3a4d42ee0b1b5ae76949a25b2d2ca11ac98f3ae483b407e56d48ba4bd6f249be", - "zh:5f220c18ebf2f848450b34f1f12a946e3e7b0585c7681eb6135be7e4658d8c0c", - "zh:7e176275fbb87987ef84515a588b4a5a5a692b3c2ae31171cbec0a0c977c8e64", - "zh:8980a4f2aca62b3833c12999abdc090a850a22cb88ef7e80fe6f33d6e688dc78", - "zh:a3c5da6f59afd566b77b789bfc2d4acddc7bf4c777fab9fc2483c1580143e1e0", - "zh:a9db6d2aec9bb17f08f3bac940ae313dd8d9e83726e71b27daeffca39eca7e67", - "zh:db5d31bade0ee02d33f620b266de435e630defd91f0b79398b94163fdaa6a3af", - "zh:e6d22de2fc2cd61a2d72d306d62c212aedc2667564e02915723f92775289cda5", - "zh:e92b6e64601cffbf891558047a6effeaacb1180df993270ecbab2a9f7f964c03", - "zh:fd40d50a64109b26f6db0178ce4a2f78e81a709ad4b1728c7d923b5e5c551cde", + "h1:h78yKGgOFrU/N5ntockxN7XF/ufv47j77+oauO2GKqk=", + "zh:4e9913fc3378436d19150c334e5906eafb83a4af3a270423cb7cdda94b27371f", + "zh:5b3d0cec2a600dc1f6633baa8fc36368c5c330fd7654861edcfa76f760a8f6a9", + "zh:5e0e1f899027bc182f31d996c9611e5ba27a034c848d7b0519b39e559fc4f38d", + "zh:66e3a1383ed6a0370989f6fd6abcfa63ccf6918ae535108595af57b9c20a9257", + "zh:688493baf6a116a399b737d74c11080051aca1ab087e5cddd14cc683b7e45c76", + "zh:9e471d85d52343e3ba778f3a94626d820fbec97bb589a3ac7a6a0939b9387770", + "zh:be1e85635daca1768f26962a4cbbadbf7fd13d9da8f9f188e938beca542c2ad5", + "zh:c00e14b6aa566eb9995cb0e1611a18fb8650d9f35c7636a7643a1b6e22660226", + "zh:c40711e5021838fd879da4c9e6b8f7e72104ada2adf0f3ba22e1cc32c3c54086", + "zh:cc62f8541de8d79577e57664e4f03c1fca893d455e5fb238d20668389c0f09ee", + "zh:cd9cbb5c6e5ceb5fcc7c4d0cab516ff209667d1b539b8c7436bd5e452c6aba8f", + "zh:f569b65999264a9416862bca5cd2a6177d94ccb0424f3a4ef424428912b9cb3c", + ] +} + +provider "registry.terraform.io/hashicorp/null" { + version = "3.1.1" + hashes = [ + "h1:71sNUDvmiJcijsvfXpiLCz0lXIBSsEJjMxljt7hxMhw=", + "zh:063466f41f1d9fd0dd93722840c1314f046d8760b1812fa67c34de0afcba5597", + "zh:08c058e367de6debdad35fc24d97131c7cf75103baec8279aba3506a08b53faf", + "zh:73ce6dff935150d6ddc6ac4a10071e02647d10175c173cfe5dca81f3d13d8afe", + "zh:78d5eefdd9e494defcb3c68d282b8f96630502cac21d1ea161f53cfe9bb483b3", + "zh:8fdd792a626413502e68c195f2097352bdc6a0df694f7df350ed784741eb587e", + "zh:976bbaf268cb497400fd5b3c774d218f3933271864345f18deebe4dcbfcd6afa", + "zh:b21b78ca581f98f4cdb7a366b03ae9db23a73dfa7df12c533d7c19b68e9e72e5", + "zh:b7fc0c1615dbdb1d6fd4abb9c7dc7da286631f7ca2299fb9cd4664258ccfbff4", + "zh:d1efc942b2c44345e0c29bc976594cb7278c38cfb8897b344669eafbc3cddf46", + "zh:e356c245b3cd9d4789bab010893566acace682d7db877e52d40fc4ca34a50924", + "zh:ea98802ba92fcfa8cf12cbce2e9e7ebe999afbf8ed47fa45fc847a098d89468b", + "zh:eff8872458806499889f6927b5d954560f3d74bf20b6043409edf94d26cd906f", ] } diff --git a/templates/workspaces/base/terraform/aad/outputs.tf b/templates/workspaces/base/terraform/aad/outputs.tf index 9383017b57..8c84927878 100644 --- a/templates/workspaces/base/terraform/aad/outputs.tf +++ b/templates/workspaces/base/terraform/aad/outputs.tf @@ -10,6 +10,10 @@ output "client_id" { value = azuread_application.workspace.application_id } +output "scope_id" { + value = "api://${var.workspace_resource_name_suffix}" +} + output "sp_id" { value = azuread_service_principal.workspace.object_id } diff --git a/templates/workspaces/base/terraform/deploy.sh b/templates/workspaces/base/terraform/deploy.sh index e1848f0b80..df5663e9e5 100755 --- a/templates/workspaces/base/terraform/deploy.sh +++ b/templates/workspaces/base/terraform/deploy.sh @@ -13,5 +13,3 @@ terraform init -reconfigure -input=false -backend=true \ -backend-config="container_name=${TF_VAR_terraform_state_container_name}" \ -backend-config="key=${TF_VAR_tre_id}-ws-${TF_VAR_tre_resource_id}" terraform apply -auto-approve - -terraform destroy -auto-approve diff --git a/templates/workspaces/base/terraform/keyvault.tf b/templates/workspaces/base/terraform/keyvault.tf index b689e38ef3..e1b1cb64a2 100644 --- a/templates/workspaces/base/terraform/keyvault.tf +++ b/templates/workspaces/base/terraform/keyvault.tf @@ -3,7 +3,7 @@ resource "azurerm_key_vault" "kv" { location = azurerm_resource_group.ws.location resource_group_name = azurerm_resource_group.ws.name sku_name = "standard" - purge_protection_enabled = var.keyvault_purge_protection_enabled + purge_protection_enabled = true tenant_id = data.azurerm_client_config.current.tenant_id network_acls { @@ -39,7 +39,6 @@ resource "azurerm_private_endpoint" "kvpe" { } } - data "azurerm_user_assigned_identity" "resource_processor_vmss_id" { name = "id-vmss-${var.tre_id}" resource_group_name = "rg-${var.tre_id}" @@ -63,6 +62,19 @@ resource "azurerm_key_vault_access_policy" "deployer" { secret_permissions = ["Get", "List", "Set", "Delete", "Purge"] } +resource "null_resource" "wait_for_dns_vault" { + provisioner "local-exec" { + command = "bash -c \"sleep 120s\"" + on_failure = fail + } + + triggers = { + always_run = azurerm_private_endpoint.kvpe.private_service_connection[0].private_ip_address # only wait on new/changed private IP address + } + + depends_on = [azurerm_private_endpoint.kvpe] +} + resource "azurerm_key_vault_secret" "aad_tenant_id" { name = "auth-tenant-id" value = var.auth_tenant_id @@ -70,6 +82,6 @@ resource "azurerm_key_vault_secret" "aad_tenant_id" { depends_on = [ azurerm_key_vault_access_policy.deployer, azurerm_key_vault_access_policy.resource_processor, - azurerm_private_endpoint.kvpe + null_resource.wait_for_dns_vault ] } diff --git a/templates/workspaces/base/terraform/outputs.tf b/templates/workspaces/base/terraform/outputs.tf index d571084484..5ba52ed6a8 100644 --- a/templates/workspaces/base/terraform/outputs.tf +++ b/templates/workspaces/base/terraform/outputs.tf @@ -2,6 +2,9 @@ output "workspace_resource_name_suffix" { value = local.workspace_resource_name_suffix } +# The following outputs are dependent on an Automatic AAD Workspace Application Registration. +# If we are not creating an App Reg we simple pass back the same values that were already created +# This is necessary so that we don't delete workspace properties output "app_role_id_workspace_owner" { value = var.register_aad_application ? module.aad[0].app_role_workspace_owner_id : var.app_role_id_workspace_owner } @@ -17,3 +20,7 @@ output "client_id" { output "sp_id" { value = var.register_aad_application ? module.aad[0].sp_id : var.sp_id } + +output "scope_id" { + value = var.register_aad_application ? module.aad[0].scope_id : var.scope_id +} diff --git a/templates/workspaces/base/terraform/providers.tf b/templates/workspaces/base/terraform/providers.tf index 509c30eaaa..c5ddaeb448 100644 --- a/templates/workspaces/base/terraform/providers.tf +++ b/templates/workspaces/base/terraform/providers.tf @@ -2,7 +2,7 @@ terraform { required_providers { azurerm = { source = "hashicorp/azurerm" - version = "=3.1.0" + version = "=3.4.0" } azuread = { source = "hashicorp/azuread" @@ -20,8 +20,10 @@ terraform { provider "azurerm" { features { key_vault { - purge_soft_delete_on_destroy = var.keyvault_purge_protection_enabled ? false : true - recover_soft_deleted_key_vaults = false + # Don't purge secrets on destroy (this would fail due to purge protection being enabled on keyvault) + purge_soft_deleted_secrets_on_destroy = false + # When recreating a shared service, recover any previously soft deleted secrets + recover_soft_deleted_secrets = true } } } diff --git a/templates/workspaces/base/terraform/variables.tf b/templates/workspaces/base/terraform/variables.tf index 4f5c4fd2a2..09ee986e51 100644 --- a/templates/workspaces/base/terraform/variables.tf +++ b/templates/workspaces/base/terraform/variables.tf @@ -42,12 +42,6 @@ variable "enable_local_debugging" { description = "This will allow storage account access over the internet. Set to true to allow deploying this from a local machine." } -variable "keyvault_purge_protection_enabled" { - type = bool - default = true - description = "Whether to allow Key Vault to purge the secrets on deletion. You will need False when debugging" -} - variable "register_aad_application" { type = bool default = false @@ -89,6 +83,17 @@ variable "sp_id" { default = "" description = "The Service Principal in the Identity provider to be able to get claims, this is passed in so that we may return it as an output." } +variable "scope_id" { + type = string + default = "" + description = "The Service Principal Name or Identifier URI, this is passed in so that we may return it as an output." +} +variable "workspace_owner_object_id" { + type = string + default = "" + description = "The Object Id of the user that you wish to be the Workspace Owner. E.g. the TEST_AUTOMATION_ACCOUNT." +} + locals { core_vnet = "vnet-${var.tre_id}"