From 8a391ecdd3857948501dae034dd5ea6ac259b343 Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Tue, 25 Nov 2025 17:14:01 +0000 Subject: [PATCH 01/29] Use Group.Create instead of Group.ReadWrite.All for group creation Fixes #4772 --- CHANGELOG.md | 1 + config.sample.yaml | 3 +-- devops/scripts/create_aad_assets.sh | 2 +- docs/tre-admins/environment-variables.md | 2 +- docs/tre-admins/identities/application_admin.md | 4 ++-- templates/workspaces/base/porter.yaml | 2 +- templates/workspaces/base/terraform/aad/aad.tf | 6 +++--- 7 files changed, 10 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6cbf1b01da..c0f4732a48 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ ENHANCEMENTS: * Upgrade Guacamole to v1.6.0 with Java 17 and other security updates ([#4754](https://github.com/microsoft/AzureTRE/pull/4754)) * API: Replace HTTP_422_UNPROCESSABLE_ENTITY response with HTTP_422_UNPROCESSABLE_CONTENT as per RFC 9110 ([#4742](https://github.com/microsoft/AzureTRE/issues/4742)) +* Change Group.ReadWrite.All permission to Group.Create for AUTO_WORKSPACE_GROUP_CREATION ([#4772](https://github.com/microsoft/AzureTRE/issues/4772)) BUG FIXES: * Fix circular dependancy in base workspace. ([#4756](https://github.com/microsoft/AzureTRE/pull/4756)) diff --git a/config.sample.yaml b/config.sample.yaml index 8d689cadfd..95fedf2835 100644 --- a/config.sample.yaml +++ b/config.sample.yaml @@ -80,7 +80,7 @@ authentication: # When this is true, create Workspaces will also create an AAD Application automatically. # When this is false, the AAD Application will need creating manually. auto_workspace_app_registration: true - # Setting AUTO_WORKSPACE_GROUP_CREATION to true will create an identity with `Group.ReadWrite.All` + # Setting AUTO_WORKSPACE_GROUP_CREATION to true will create an identity with `Group.Create` auto_workspace_group_creation: false # Setting this to true will remove the need for users to manually grant consent when creating new workspaces. # The identity will be granted Application.ReadWrite.All and DelegatedPermissionGrant.ReadWrite.All permissions. @@ -101,7 +101,6 @@ ui_config: ui_site_name: "Azure TRE" # Footer text shown in the bottom left hand corner of the TRE portal ui_footer_text: "Azure Trusted Research Environment" - #developer_settings: # Locks will not be added to stateful resources so they can be easily removed # stateful_resources_locked: false diff --git a/devops/scripts/create_aad_assets.sh b/devops/scripts/create_aad_assets.sh index f7c80f92ef..ff5727ba37 100755 --- a/devops/scripts/create_aad_assets.sh +++ b/devops/scripts/create_aad_assets.sh @@ -33,7 +33,7 @@ if [ "${AUTO_WORKSPACE_APP_REGISTRATION:-}" == true ]; then fi if [ "${AUTO_WORKSPACE_GROUP_CREATION:-}" == true ]; then - APPLICATION_PERMISSIONS+=("Group.ReadWrite.All") + APPLICATION_PERMISSIONS+=("Group.Create") fi if [ "${AUTO_GRANT_WORKSPACE_CONSENT:-}" == true ]; then diff --git a/docs/tre-admins/environment-variables.md b/docs/tre-admins/environment-variables.md index ae5929b0a7..2275dd15c6 100644 --- a/docs/tre-admins/environment-variables.md +++ b/docs/tre-admins/environment-variables.md @@ -48,7 +48,7 @@ | `CUSTOM_DOMAIN` | Optional. Custom domain name to access the Azure TRE portal. See [Custom domain name](custom-domain.md). | | `ENABLE_CMK_ENCRYPTION` | Optional. Default is `false`, if set to `true` customer-managed key encryption will be enabled for all supported resources. | | `AUTO_WORKSPACE_APP_REGISTRATION`| Set to `false` by default. Setting this to `true` grants the `Application.ReadWrite.All` and `Directory.Read.All` permission to the *Application Admin* identity. This identity is used to manage other Microsoft Entra ID applications that it owns, e.g. Workspaces. If you do not set this, the identity will have `Application.ReadWrite.OwnedBy` permission. [Further information on Application Admin can be found here](./identities/application_admin.md). | -| `AUTO_WORKSPACE_GROUP_CREATION`| Set to `false` by default. Setting this to `true` grants the `Group.ReadWrite.All` permission to the *Application Admin* identity. This identity can then create security groups aligned to each applciation role. Microsoft Entra ID licencing implications need to be considered as Group assignment is a premium feature. [You can read mode about Group Assignment here](https://docs.microsoft.com/en-us/azure/architecture/multitenant-identity/app-roles#roles-using-azure-ad-app-roles). | +| `AUTO_WORKSPACE_GROUP_CREATION`| Set to `false` by default. Setting this to `true` grants the `Group.Create` permission to the *Application Admin* identity. This identity can then create security groups aligned to each applciation role. | | `AUTO_GRANT_WORKSPACE_CONSENT`| Default of `false`. Setting this to `true` will remove the need for users to manually grant consent when creating new workspaces. The identity will be granted `Application.ReadWrite.All` and `DelegatedPermissionGrant.ReadWrite.All` permissions. | | `USER_MANAGEMENT_ENABLED` | If set to `true`, TRE Admins will be able to assign and de-assign users to workspaces via the UI (Requires Entra ID groups to be enabled on the workspace and the workspace template version to be 2.2.0 or greater). | | `PRIVATE_AGENT_SUBNET_ID` | Optional. Vnet exception is enabled for the provided runner agent subnet id, enabling access to private resources like TRE key vault. | diff --git a/docs/tre-admins/identities/application_admin.md b/docs/tre-admins/identities/application_admin.md index a12fccc412..bf96eba124 100644 --- a/docs/tre-admins/identities/application_admin.md +++ b/docs/tre-admins/identities/application_admin.md @@ -13,7 +13,7 @@ This application does not have any roles defined. | Application.ReadWrite.OwnedBy | Application | Yes | This user has `Application.ReadWrite.OwnedBy` as a minimum permission for it to function. If the tenant is managed by a customer administrator, then this user must be added to the **Owners** of every workspace that is created. This will allow TRE to manage the Microsoft Entra ID Application. This will be a manual process for the Tenant Admin. | | Application.ReadWrite.All | Application | Yes | This permission is required to create workspace applications and administer any applications in the tenant. This is needed if the Microsoft Entra ID Administrator has delegated Microsoft Entra ID administrative operations to the TRE. There will be no need for the Tenant Admin to manually create workspace applications in the Tenant. | | Directory.Read.All | Application | Yes | This permission is required to read User details from Microsoft Entra ID. This is needed if the Microsoft Entra ID Administrator has delegated Microsoft Entra ID administrative operations to the TRE. | -| Group.ReadWrite.All | Application | Yes | This permission is required to create and update Microsoft Entra ID groups. This is requried if Microsoft Entra ID groups are to be created automatically by the TRE. | +| Group.Create | Application | Yes | This permission is required to create and update Microsoft Entra ID groups. This is requried if Microsoft Entra ID groups are to be created automatically by the TRE. | | DelegatedPermissionGrant.ReadWrite.All | Application | Yes | This permssion is required to remove the need for users to manually grant consent when creating new workspaces. | '*' See the difference between [delegated and application permission](https://docs.microsoft.com/graph/auth/auth-concepts#delegated-and-application-permissions) types. See [Microsoft Graph permissions reference](https://docs.microsoft.com/graph/permissions-reference) for more details. @@ -31,7 +31,7 @@ This user is currently only used from the Porter bundles hosted on the Resource | -------- | ----------- | | `--name` | This is used to put a friendly name to the Application that can be seen in the portal. It is typical to use the name of your TRE instance. | | `--admin-consent` | If you have the appropriate permission to grant admin consent, then pass in this argument. If you do not, you will have to ask an Microsoft Entra ID Admin to consent after you have created the identity. Consent is required for this permission. | -| `--application-permission` | This is a comma seperated list of the permissions that need to be assigned. For exampler `Application.ReadWrite.All,Directory.Read.All,Group.ReadWrite.All` | +| `--application-permission` | This is a comma seperated list of the permissions that need to be assigned. For exampler `Application.ReadWrite.All,Directory.Read.All,Group.Create` | | `--reset-password` | Optional, default is 0. When run in a headless fashion, 1 is passed in to always reset the password. | ## Environment Variables diff --git a/templates/workspaces/base/porter.yaml b/templates/workspaces/base/porter.yaml index 9fd970c1e6..eb58e93a9d 100644 --- a/templates/workspaces/base/porter.yaml +++ b/templates/workspaces/base/porter.yaml @@ -1,7 +1,7 @@ --- schemaVersion: 1.0.0 name: tre-workspace-base -version: 2.7.1 +version: 2.8.0 description: "A base Azure TRE workspace" dockerfile: Dockerfile.tmpl registry: azuretre diff --git a/templates/workspaces/base/terraform/aad/aad.tf b/templates/workspaces/base/terraform/aad/aad.tf index 618d1c9305..fd3acd0c3b 100644 --- a/templates/workspaces/base/terraform/aad/aad.tf +++ b/templates/workspaces/base/terraform/aad/aad.tf @@ -143,21 +143,21 @@ resource "azuread_app_role_assignment" "workspace_owner" { resource "azuread_group" "workspace_owners" { count = var.create_aad_groups ? 1 : 0 display_name = "${var.workspace_resource_name_suffix} Workspace Owners" - owners = [var.workspace_owner_object_id, data.azuread_service_principal.core_api.object_id] + owners = [var.workspace_owner_object_id, data.azuread_service_principal.core_api.object_id, data.azuread_client_config.current.object_id] security_enabled = true } resource "azuread_group" "workspace_researchers" { count = var.create_aad_groups ? 1 : 0 display_name = "${var.workspace_resource_name_suffix} Workspace Researchers" - owners = [var.workspace_owner_object_id, data.azuread_service_principal.core_api.object_id] + owners = [var.workspace_owner_object_id, data.azuread_service_principal.core_api.object_id, data.azuread_client_config.current.object_id] security_enabled = true } resource "azuread_group" "workspace_airlock_managers" { count = var.create_aad_groups ? 1 : 0 display_name = "${var.workspace_resource_name_suffix} Airlock Managers" - owners = [var.workspace_owner_object_id, data.azuread_service_principal.core_api.object_id] + owners = [var.workspace_owner_object_id, data.azuread_service_principal.core_api.object_id, data.azuread_client_config.current.object_id] security_enabled = true } From 481daf801377ce2f894526a58cf73dbc93054eb0 Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Wed, 26 Nov 2025 00:04:20 +0000 Subject: [PATCH 02/29] Deployment works, permetations need testing, and docs updating. --- api_app/_version.py | 2 +- api_app/api/routes/workspaces.py | 5 +- api_app/db/repositories/workspaces.py | 3 +- api_app/services/aad_authentication.py | 36 -- api_app/services/access_service.py | 4 - api_app/services/authentication.py | 8 - .../test_routes/test_workspace_users.py | 5 +- .../test_api/test_routes/test_workspaces.py | 16 +- .../test_workpaces_repository.py | 8 +- .../test_services/test_aad_access_service.py | 71 ---- .../aad/create_workspace_application.sh | 325 +++--------------- .../aad/wait_for_new_app_registration.sh | 2 - templates/workspaces/base/porter.yaml | 41 +-- .../workspaces/base/template_schema.json | 24 +- .../base/terraform/.terraform.lock.hcl | 20 ++ .../workspaces/base/terraform/aad/aad.tf | 25 +- .../base/terraform/aad/providers.tf | 4 + .../base/terraform/aad/variables.tf | 10 +- .../workspaces/base/terraform/keyvault.tf | 40 --- .../workspaces/base/terraform/outputs.tf | 21 +- .../workspaces/base/terraform/providers.tf | 4 + .../workspaces/base/terraform/variables.tf | 12 - .../workspaces/base/terraform/workspace.tf | 34 +- 23 files changed, 169 insertions(+), 551 deletions(-) diff --git a/api_app/_version.py b/api_app/_version.py index 4e32d0c791..a5e1b0f66d 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.25.4" +__version__ = "0.25.5" diff --git a/api_app/api/routes/workspaces.py b/api_app/api/routes/workspaces.py index 4f8ee42bb2..11f5f7a386 100644 --- a/api_app/api/routes/workspaces.py +++ b/api_app/api/routes/workspaces.py @@ -30,7 +30,6 @@ get_current_workspace_owner_or_researcher_user_or_airlock_manager, \ get_current_workspace_owner_or_airlock_manager, \ get_current_workspace_owner_or_researcher_user_or_airlock_manager_or_tre_admin -from services.authentication import extract_auth_information from services.azure_resource_status import get_azure_resource_status from azure.cosmos.exceptions import CosmosAccessConditionFailedError from .resource_helpers import cascaded_update_resource, delete_validation, enrich_resource_with_available_upgrades, get_identity_role_assignments, save_and_deploy_resource, construct_location_header, send_uninstall_message, \ @@ -99,9 +98,7 @@ async def retrieve_workspace_scope_id_by_workspace_id(workspace=Depends(get_work @workspaces_core_router.post("/workspaces", status_code=status.HTTP_202_ACCEPTED, response_model=OperationInResponse, name=strings.API_CREATE_WORKSPACE, dependencies=[Depends(get_current_admin_user)]) async def create_workspace(workspace_create: WorkspaceInCreate, response: Response, user=Depends(get_current_admin_user), workspace_repo=Depends(get_repository(WorkspaceRepository)), resource_template_repo=Depends(get_repository(ResourceTemplateRepository)), operations_repo=Depends(get_repository(OperationRepository)), resource_history_repo=Depends(get_repository(ResourceHistoryRepository))) -> OperationInResponse: try: - # TODO: This requires Directory.ReadAll ( Application.Read.All ) to be enabled in the Azure AD application to enable a users workspaces to be listed. This should be made optional. - auth_info = extract_auth_information(workspace_create.properties) - workspace, resource_template = await workspace_repo.create_workspace_item(workspace_create, auth_info, user.id, user.roles) + workspace, resource_template = await workspace_repo.create_workspace_item(workspace_create, user.id, user.roles) except (ValidationError, ValueError) as e: logger.exception("Failed to create workspace model instance") raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) diff --git a/api_app/db/repositories/workspaces.py b/api_app/db/repositories/workspaces.py index 5be9cab979..3b8917ded6 100644 --- a/api_app/db/repositories/workspaces.py +++ b/api_app/db/repositories/workspaces.py @@ -89,7 +89,7 @@ async def is_workspace_storage_account_available(self, workspace_id: str) -> boo ) return availability_result.name_available - async def create_workspace_item(self, workspace_input: WorkspaceInCreate, auth_info: dict, workspace_owner_object_id: str, user_roles: List[str]) -> Tuple[Workspace, ResourceTemplate]: + async def create_workspace_item(self, workspace_input: WorkspaceInCreate, workspace_owner_object_id: str, user_roles: List[str]) -> Tuple[Workspace, ResourceTemplate]: full_workspace_id = str(uuid.uuid4()) @@ -114,7 +114,6 @@ async def create_workspace_item(self, workspace_input: WorkspaceInCreate, auth_i **address_spaces_param, **auto_app_registration_param, **workspace_owner_param, - **auth_info, **self.get_workspace_spec_params(full_workspace_id)} workspace = Workspace( diff --git a/api_app/services/aad_authentication.py b/api_app/services/aad_authentication.py index fcf34f08ef..52cb6e74d7 100644 --- a/api_app/services/aad_authentication.py +++ b/api_app/services/aad_authentication.py @@ -480,25 +480,6 @@ def _get_batch_users_by_role_assignments_body(self, roles_graph_data): return request_body - # This method is called when you create a workspace and you already have an AAD App Registration - # to link it to. You pass in the client_id and go and get the extra information you need from AAD - # If the auth_type is `Automatic`, 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: - logger.debug(graph_data) - 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'], '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']: - if appRole['value'] in self.WORKSPACE_ROLES_DICT.keys(): - authInfo[self.WORKSPACE_ROLES_DICT[appRole['value']]] = appRole['id'] - - return authInfo - 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) @@ -550,23 +531,6 @@ def _get_identity_type(self, id: str) -> str: return object_info["@odata.type"] - def extract_workspace_auth_information(self, data: dict) -> dict: - if ("auth_type" not in data) or (data["auth_type"] != "Automatic" and "client_id" not in data): - raise AuthConfigValidationError(strings.ACCESS_PLEASE_SUPPLY_CLIENT_ID) - - auth_info = {} - # The user may want us to create the AAD workspace app and therefore they - # don't know the client_id yet. - if data["auth_type"] != "Automatic": - auth_info = self._get_app_auth_info(data["client_id"]) - - # Check we've get all our required roles - for role in self.WORKSPACE_ROLES_DICT.items(): - if role[1] not in auth_info: - raise AuthConfigValidationError(f"{strings.ACCESS_APP_IS_MISSING_ROLE} {role[0]}") - - return auth_info - def get_identity_role_assignments(self, user_id: str) -> List[RoleAssignment]: identity_type = self._get_identity_type(user_id) if identity_type == "#microsoft.graph.user": diff --git a/api_app/services/access_service.py b/api_app/services/access_service.py index d38d26ce15..c909fc3b46 100644 --- a/api_app/services/access_service.py +++ b/api_app/services/access_service.py @@ -15,10 +15,6 @@ class UserRoleAssignmentError(Exception): class AccessService(OAuth2AuthorizationCodeBearer): - @abstractmethod - def extract_workspace_auth_information(self, data: dict) -> dict: - pass - @abstractmethod def get_identity_role_assignments(self, user_id: str) -> dict: pass diff --git a/api_app/services/authentication.py b/api_app/services/authentication.py index 30b49af194..d82bd7892f 100644 --- a/api_app/services/authentication.py +++ b/api_app/services/authentication.py @@ -7,14 +7,6 @@ from services.access_service import AccessService, AuthConfigValidationError -def extract_auth_information(workspace_creation_properties: dict) -> dict: - access_service = get_access_service('AAD') - try: - return access_service.extract_workspace_auth_information(workspace_creation_properties) - except AuthConfigValidationError as e: - raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) - - def get_access_service(provider: str = AuthProvider.AAD) -> AccessService: if provider == AuthProvider.AAD: return AzureADAuthorization() diff --git a/api_app/tests_ma/test_api/test_routes/test_workspace_users.py b/api_app/tests_ma/test_api/test_routes/test_workspace_users.py index 64fb2a7d68..de9cc40aaa 100644 --- a/api_app/tests_ma/test_api/test_routes/test_workspace_users.py +++ b/api_app/tests_ma/test_api/test_routes/test_workspace_users.py @@ -24,7 +24,7 @@ OPERATION_ID = '11111111-7265-4b5f-9eae-a1a62928772f' -def sample_workspace(workspace_id=WORKSPACE_ID, auth_info: dict = {}) -> Workspace: +def sample_workspace(workspace_id=WORKSPACE_ID) -> Workspace: workspace = Workspace( id=workspace_id, templateName="tre-workspace-base", @@ -39,8 +39,7 @@ def sample_workspace(workspace_id=WORKSPACE_ID, auth_info: dict = {}) -> Workspa updatedWhen=FAKE_CREATE_TIMESTAMP, user=create_admin_user() ) - if auth_info: - workspace.properties = {**auth_info} + return workspace diff --git a/api_app/tests_ma/test_api/test_routes/test_workspaces.py b/api_app/tests_ma/test_api/test_routes/test_workspaces.py index 5e97af84f2..e775824c7b 100644 --- a/api_app/tests_ma/test_api/test_routes/test_workspaces.py +++ b/api_app/tests_ma/test_api/test_routes/test_workspaces.py @@ -436,8 +436,7 @@ async def test_get_workspace_history_returns_empty_list_when_no_history(self, ac @patch("api.routes.resource_helpers.send_resource_request_message", return_value=sample_resource_operation(resource_id=WORKSPACE_ID, operation_id=OPERATION_ID)) @patch("api.routes.workspaces.WorkspaceRepository.save_item") @patch("api.routes.workspaces.WorkspaceRepository.create_workspace_item") - @patch("api.routes.workspaces.extract_auth_information") - async def test_post_workspaces_creates_workspace(self, _, create_workspace_item, __, ___, resource_template_repo, app, client, workspace_input, basic_resource_template): + async def test_post_workspaces_creates_workspace(self, create_workspace_item, __, ___, resource_template_repo, app, client, workspace_input, basic_resource_template): resource_template_repo.return_value = basic_resource_template create_workspace_item.return_value = [sample_workspace(), basic_resource_template] response = await client.post(app.url_path_for(strings.API_CREATE_WORKSPACE), json=workspace_input) @@ -451,8 +450,7 @@ async def test_post_workspaces_creates_workspace(self, _, create_workspace_item, @patch("api.routes.workspaces.WorkspaceRepository.save_item") @patch("api.routes.workspaces.WorkspaceRepository.create_workspace_item") @patch("api.routes.workspaces.WorkspaceRepository._validate_resource_parameters") - @patch("api.routes.workspaces.extract_auth_information") - async def test_post_workspaces_calls_db_and_service_bus(self, _, __, create_workspace_item, save_item_mock, send_resource_request_message_mock, resource_template_repo, app, client, workspace_input, basic_resource_template): + async def test_post_workspaces_calls_db_and_service_bus(self, __, create_workspace_item, save_item_mock, send_resource_request_message_mock, resource_template_repo, app, client, workspace_input, basic_resource_template): resource_template_repo.return_value = basic_resource_template create_workspace_item.return_value = [sample_workspace(), basic_resource_template] await client.post(app.url_path_for(strings.API_CREATE_WORKSPACE), json=workspace_input) @@ -466,8 +464,7 @@ async def test_post_workspaces_calls_db_and_service_bus(self, _, __, create_work @patch("api.routes.workspaces.WorkspaceRepository.save_item") @patch("api.routes.workspaces.WorkspaceRepository.create_workspace_item") @patch("api.routes.workspaces.WorkspaceRepository._validate_resource_parameters") - @patch("api.routes.workspaces.extract_auth_information") - async def test_post_workspaces_returns_202_on_successful_create(self, _, __, create_workspace_item, ____, _____, resource_template_repo, app, client, workspace_input, basic_resource_template): + async def test_post_workspaces_returns_202_on_successful_create(self, __, create_workspace_item, ____, _____, resource_template_repo, app, client, workspace_input, basic_resource_template): resource_template_repo.return_value = basic_resource_template create_workspace_item.return_value = [sample_workspace(), basic_resource_template] response = await client.post(app.url_path_for(strings.API_CREATE_WORKSPACE), json=workspace_input) @@ -482,8 +479,7 @@ async def test_post_workspaces_returns_202_on_successful_create(self, _, __, cre @patch("api.routes.workspaces.WorkspaceRepository.save_item") @patch("api.routes.workspaces.WorkspaceRepository.create_workspace_item", return_value=[sample_workspace(), sample_resource_template()]) @patch("api.routes.workspaces.WorkspaceRepository._validate_resource_parameters") - @patch("api.routes.workspaces.extract_auth_information") - async def test_post_workspaces_returns_503_if_service_bus_call_fails(self, _, __, ___, ____, _____, delete_item_mock, resource_template_repo, app, client, workspace_input, basic_resource_template): + async def test_post_workspaces_returns_503_if_service_bus_call_fails(self, __, ___, ____, _____, delete_item_mock, resource_template_repo, app, client, workspace_input, basic_resource_template): resource_template_repo.return_value = basic_resource_template response = await client.post(app.url_path_for(strings.API_CREATE_WORKSPACE), json=workspace_input) @@ -491,8 +487,8 @@ async def test_post_workspaces_returns_503_if_service_bus_call_fails(self, _, __ delete_item_mock.assert_called_once_with(WORKSPACE_ID) # [POST] /workspaces/ - @patch("api.routes.workspaces.WorkspaceRepository.validate_input_against_template", side_effect=ValueError) - async def test_post_workspaces_returns_400_if_template_does_not_exist(self, _, app, client, workspace_input): + @patch("api.routes.workspaces.WorkspaceRepository.create_workspace_item", side_effect=ValueError) + async def test_post_workspaces_returns_400_if_template_does_not_exist(self, mock_create, app, client, workspace_input): response = await client.post(app.url_path_for(strings.API_CREATE_WORKSPACE), json=workspace_input) assert response.status_code == status.HTTP_400_BAD_REQUEST diff --git a/api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py b/api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py index a72b2d85e1..8a29d47506 100644 --- a/api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py +++ b/api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py @@ -117,7 +117,7 @@ async def test_create_workspace_item_creates_a_workspace_with_the_right_values(m validate_input_mock.return_value = basic_resource_template new_cidr_mock.return_value = "1.2.3.4/24" - workspace, _ = await workspace_repo.create_workspace_item(workspace_to_create, {}, "test_object_id", ["test_role"]) + workspace, _ = await workspace_repo.create_workspace_item(workspace_to_create, "test_object_id", ["test_role"]) assert workspace.templateName == workspace_to_create.templateName assert workspace.resourceType == ResourceType.Workspace @@ -186,7 +186,7 @@ async def test_create_workspace_item_creates_a_workspace_with_custom_address_spa mock_is_workspace_storage_account_available.return_value.return_value = False validate_input_mock.return_value = basic_resource_template - workspace, _ = await workspace_repo.create_workspace_item(workspace_to_create, {}, "test_object_id", ["test_role"]) + workspace, _ = await workspace_repo.create_workspace_item(workspace_to_create, "test_object_id", ["test_role"]) assert workspace.properties["address_space"] == workspace_to_create.properties["address_space"] @@ -208,7 +208,7 @@ async def test_create_workspace_item_throws_exception_with_bad_custom_address_sp validate_input_mock.return_value = basic_resource_template with pytest.raises(InvalidInput): - await workspace_repo.create_workspace_item(workspace_to_create, {}, "test_object_id", ["test_role"]) + await workspace_repo.create_workspace_item(workspace_to_create, "test_object_id", ["test_role"]) @pytest.mark.asyncio @@ -273,7 +273,7 @@ async def test_create_workspace_item_raises_value_error_if_template_is_invalid(m validate_input_mock.side_effect = ValueError with pytest.raises(ValueError): - await workspace_repo.create_workspace_item(workspace_input, {}, "test_object_id", ["test_role"]) + await workspace_repo.create_workspace_item(workspace_input, "test_object_id", ["test_role"]) def test_automatically_create_application_registration_returns_true(workspace_repo): 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 64b80ada56..16730fb55a 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 @@ -145,12 +145,6 @@ def user_with_role(): return User(id="user2", name="Test User 2", email="test2@example.com", roles=["WorkspaceOwner"]) -def test_extract_workspace__raises_error_if_client_id_not_available(): - access_service = AzureADAuthorization() - with pytest.raises(AuthConfigValidationError): - access_service.extract_workspace_auth_information(data={"auth_type": "Manual"}) - - @patch("services.aad_authentication.AzureADAuthorization._get_app_sp_graph_data") @patch("services.aad_authentication.AzureADAuthorization._get_user_role_assignments") @patch("services.aad_authentication.AzureADAuthorization._get_user_details") @@ -306,71 +300,6 @@ def test_get_workspace_user_emails_by_role_assignment_with_groups_and_users_assi assert "test_user4@email.com" in role_assignment_details["WorkspaceOwner"] -@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"}, -) -def test_extract_workspace__raises_error_if_researcher_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_sp_graph_data", - return_value={}, -) -def test_extract_workspace__raises_error_if_graph_data_is_invalid( - get_app_sp_graph_data_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_sp_graph_data") -def test_extract_workspace__returns_sp_id_and_roles(get_app_sp_graph_data_mock): - get_app_sp_graph_data_mock.return_value = { - "value": [ - { - "id": "12345", - "appRoles": [ - {"id": "1abc3", "value": "WorkspaceResearcher"}, - {"id": "1abc4", "value": "WorkspaceOwner"}, - {"id": "1abc5", "value": "AirlockManager"}, - ], - "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", - "app_role_id_workspace_airlock_manager": "1abc5", - } - - access_service = AzureADAuthorization() - actual_auth_info = access_service.extract_workspace_auth_information( - data={"auth_type": "Manual", "client_id": "1234"} - ) - - assert actual_auth_info == expected_auth_info - - @pytest.mark.parametrize( "user, workspace, expected_role", [ diff --git a/devops/scripts/aad/create_workspace_application.sh b/devops/scripts/aad/create_workspace_application.sh index 2495731240..23d4e9d1ff 100755 --- a/devops/scripts/aad/create_workspace_application.sh +++ b/devops/scripts/aad/create_workspace_application.sh @@ -8,21 +8,18 @@ function show_usage() { cat << USAGE -Utility script for creating a workspace TRE. You would typically have one of these per workspace -for a security boundary. -You must be logged in using Azure CLI with sufficient privileges to modify Azure Active Directory to run this script. +Utility script for pre-creating the Workspace API Azure AD application registration +and corresponding service principal so that Terraform can import and configure it +without requiring Application.ReadWrite.All permissions. -Usage: $0 [--admin-consent] +Usage: $0 --name --application-admin-clientid Options: - -n,--name Required. The prefix for the app (registration) names e.g., "TRE". - -u,--ux-clientid Required. The client ID of the UX must be provided. - -y,--application-admin-clientid Required. The client ID of the Application Administrator that will be able to update this application. - e.g. updating a redirect URI. - -a,--admin-consent Optional, but recommended. Grants admin consent for the app registrations, when this flag is set. - Requires directory admin privileges to the Azure AD in question. - -z,--automation-clientid Optional, the client ID of the automation account can be added to the TRE workspace. - -r,--reset-password Optional, switch to automatically reset the password. Default 0 + -n,--name Required. Prefix for the Workspace API app registration name. + The script appends " API" to keep naming consistent with Terraform. + -y,--application-admin-clientid + Required. Client ID of the application administrator (typically the TRE Core API + app registration) that must be added as an owner of the workspace application. USAGE exit 2 @@ -41,17 +38,16 @@ fi # Get the directory that this script is in DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" -declare resetPassword=0 -declare spPassword="" -declare grantAdminConsent=0 -declare currentUserId="" -declare uxClientId="" -declare msGraphUri="" declare appName="" -declare automationClientId="" +declare workspaceAppId="" +declare workspaceSpId="" +declare appObjectId="" declare applicationAdminClientId="" declare applicationAdminObjectId="" +declare currentUserId="" +declare msGraphUri="" + # Initialize parameters specified from command line while [[ $# -gt 0 ]]; do case "$1" in @@ -59,25 +55,9 @@ while [[ $# -gt 0 ]]; do appName=$2 shift 2 ;; - -a|--admin-consent) - grantAdminConsent=1 - shift 1 - ;; -y|--application-admin-clientid) - applicationAdminClientId=$2 - shift 2 - ;; - -u|--ux-clientid) - uxClientId=$2 - shift 2 - ;; - -z|--automation-clientid) - automationClientId=$2 - shift 2 - ;; - -r|--reset-password) - resetPassword=$2 - shift 2 + applicationAdminClientId=$2 + shift 2 ;; *) echo "Invalid option: $1." @@ -87,7 +67,7 @@ while [[ $# -gt 0 ]]; do done ################################### -# CHECK INCOMMING PARAMETERS # +# CHECK INCOMING PARAMETERS # ################################### if [[ -z "$appName" ]]; then echo "Please specify the application name." 1>&2 @@ -95,161 +75,43 @@ if [[ -z "$appName" ]]; then fi appName="$appName API" if [[ -z "$applicationAdminClientId" ]]; then - echo "Please specify the client id of the Application Admin." 1>&2 - show_usage + echo "Please specify the application administrator client ID." 1>&2 + show_usage fi -applicationAdminObjectId=$(az ad sp show --id "${applicationAdminClientId}" --query id -o tsv --only-show-errors) + currentUserId=$(az ad signed-in-user show --query 'id' --output tsv --only-show-errors) +applicationAdminObjectId=$(az ad sp show --id "$applicationAdminClientId" --query id -o tsv --only-show-errors) msGraphUri="$(az cloud show --query endpoints.microsoftGraphResourceId --output tsv)/v1.0" tenant=$(az rest -m get -u "${msGraphUri}/domains" -o json | jq -r '.value[] | select(.isDefault == true) | .id') -echo -e "\e[96mCreating a Workspace Application in the \"${tenant}\" Azure AD tenant.\e[0m" +echo -e "\e[96mEnsuring Workspace Application exists in the \"${tenant}\" Azure AD tenant.\e[0m" -# Load in helper functions +# Load helper functions # shellcheck disable=SC1091 source "${DIR}/get_existing_app.sh" # shellcheck disable=SC1091 -source "${DIR}/grant_admin_consent.sh" -# shellcheck disable=SC1091 source "${DIR}/wait_for_new_app_registration.sh" # shellcheck disable=SC1091 source "${DIR}/create_or_update_service_principal.sh" -# shellcheck disable=SC1091 -source "${DIR}/get_msgraph_access.sh" -# shellcheck disable=SC1091 -source "${DIR}/update_resource_access.sh" -# Default of new UUIDs -researcherRoleId=$(cat /proc/sys/kernel/random/uuid) -ownerRoleId=$(cat /proc/sys/kernel/random/uuid) -airlockManagerRoleId=$(cat /proc/sys/kernel/random/uuid) -userImpersonationScopeId=$(cat /proc/sys/kernel/random/uuid) -appObjectId="" - -# Get an existing object if its been created before. -existingApp=$(get_existing_app --name "${appName}") || null -if [ -n "${existingApp}" ]; then +# Look for an existing app registration +existingApp=$(get_existing_app --name "${appName}") +if [[ -n ${existingApp} ]]; then appObjectId=$(echo "${existingApp}" | jq -r '.id') - - researcherRoleId=$(echo "$existingApp" | jq -r '.appRoles[] | select(.value == "WorkspaceResearcher").id') - ownerRoleId=$(echo "$existingApp" | jq -r '.appRoles[] | select(.value == "WorkspaceOwner").id') - airlockManagerRoleId=$(echo "$existingApp" | jq -r '.appRoles[] | select(.value == "AirlockManager").id') - userImpersonationScopeId=$(echo "$existingApp" | jq -r '.api.oauth2PermissionScopes[] | select(.value == "user_impersonation").id') - if [[ -z "${researcherRoleId}" ]]; then researcherRoleId=$(cat /proc/sys/kernel/random/uuid); fi - if [[ -z "${ownerRoleId}" ]]; then ownerRoleId=$(cat /proc/sys/kernel/random/uuid); fi - if [[ -z "${airlockManagerRoleId}" ]]; then airlockManagerRoleId=$(cat /proc/sys/kernel/random/uuid); fi - if [[ -z "${userImpersonationScopeId}" ]]; then userImpersonationScopeId=$(cat /proc/sys/kernel/random/uuid); fi + workspaceAppId=$(echo "${existingApp}" | jq -r '.appId') + echo "Found existing app registration (AppId: ${workspaceAppId})" fi -# Get the Required Resource Scope/Role -msGraphAppId="00000003-0000-0000-c000-000000000000" -msGraphObjectId=$(az ad sp show --id ${msGraphAppId} --query "id" --output tsv --only-show-errors) -msGraphEmailScopeId="64a6cdd6-aab1-4aaf-94b8-3cc8405e90d0" -msGraphOpenIdScopeId="37f7f235-527c-4136-accd-4a02d197296e" -msGraphProfileScopeId="14dad69e-099b-42c9-810b-d002981feec1" - -appDefinition=$(jq -c . << JSON +if [[ -z ${workspaceAppId} ]]; then + echo "Creating \"${appName}\" app registration." + appDefinition=$(jq -c . << JSON { - "displayName": "${appName}", - "api": { - "requestedAccessTokenVersion": 2, - "oauth2PermissionScopes": [ - { - "adminConsentDescription": "Allow the app to access the Workspace API on behalf of the signed-in user.", - "adminConsentDisplayName": "Access the Workspace API on behalf of signed-in user", - "id": "${userImpersonationScopeId}", - "isEnabled": true, - "type": "User", - "userConsentDescription": "Allow the app to access the Workspace API on your behalf.", - "userConsentDisplayName": "Access the Workspace API", - "value": "user_impersonation" - } - ] - }, - "appRoles": [ - { - "id": "${ownerRoleId}", - "allowedMemberTypes": [ "User", "Application" ], - "description": "Provides workspace owners access to the Workspace.", - "displayName": "Workspace Owner", - "isEnabled": true, - "origin": "Application", - "value": "WorkspaceOwner" - }, - { - "id": "${researcherRoleId}", - "allowedMemberTypes": [ "User", "Application" ], - "description": "Provides researchers access to the Workspace.", - "displayName": "Workspace Researcher", - "isEnabled": true, - "origin": "Application", - "value": "WorkspaceResearcher" - }, - { - "id": "${airlockManagerRoleId}", - "allowedMemberTypes": [ "User", "Application" ], - "description": "Provides airlock managers access to the Workspace and ability to review airlock requests", - "displayName": "Airlock Manager", - "isEnabled": true, - "origin": "Application", - "value": "AirlockManager" - }], - "signInAudience": "AzureADMyOrg", - "requiredResourceAccess": [ - { - "resourceAppId": "${msGraphAppId}", - "resourceAccess": [ - { - "id": "${msGraphEmailScopeId}", - "type": "Scope" - }, - { - "id": "${msGraphOpenIdScopeId}", - "type": "Scope" - }, - { - "id": "${msGraphProfileScopeId}", - "type": "Scope" - } - ] - } - ], - "web":{ - "implicitGrantSettings":{ - "enableIdTokenIssuance":true, - "enableAccessTokenIssuance":true - } - }, - "optionalClaims": { - "idToken": [ - { - "name": "ipaddr", - "source": null, - "essential": false, - "additionalProperties": [] - }, - { - "name": "email", - "source": null, - "essential": false, - "additionalProperties": [] - } - ], - "accessToken": [], - "saml2Token": [] - } + "displayName": "${appName}", + "signInAudience": "AzureADMyOrg" } JSON ) -# Is the Workspace app already registered? -if [[ -n ${appObjectId} ]]; then - echo "Updating \"${appName}\" with ObjectId \"${appObjectId}\"." - az rest --method PATCH --uri "${msGraphUri}/applications/${appObjectId}" --headers Content-Type=application/json --body "${appDefinition}" - workspaceAppId=$(az ad app show --id "${appObjectId}" --query "appId" --output tsv --only-show-errors) - echo "Workspace app registration with AppId \"${workspaceAppId}\" updated." -else - echo "Creating \"${appName}\" app registration." workspaceAppId=$(az rest --method POST --uri "${msGraphUri}/applications" --headers Content-Type=application/json --body "${appDefinition}" --output tsv --query "appId") # Poll until the app registration is found in the listing. @@ -257,113 +119,24 @@ else # Update to set the identifier URI. az ad app update --id "${workspaceAppId}" --identifier-uris "api://${workspaceAppId}" --only-show-errors + appObjectId=$(az ad app show --id "${workspaceAppId}" --query "id" --output tsv --only-show-errors) fi -# Make the current user an owner of the application. -az ad app owner add --id "${workspaceAppId}" --owner-object-id "$currentUserId" --only-show-errors -az ad app owner add --id "${workspaceAppId}" --owner-object-id "$applicationAdminObjectId" --only-show-errors - -# Create a Service Principal for the app. -spPassword=$(create_or_update_service_principal "${workspaceAppId}" "${resetPassword}") -workspaceSpId=$(az ad sp list --filter "appId eq '${workspaceAppId}'" --query '[0].id' --output tsv --only-show-errors) - -# needed to make the API permissions change effective, this must be done after SP creation... -echo -echo "Running 'az ad app permission grant' to make changes effective." -az ad app permission grant --id "$workspaceSpId" --api "$msGraphObjectId" --scope "email openid profile" --only-show-errors - -# The UX (which was created as part of the API) needs to also have access to this Workspace -echo "Searching for existing UX application (${uxClientId})." -existingUXApp=$(get_existing_app --id "${uxClientId}") -uxObjectId=$(echo "${existingUXApp}" | jq -r .id) - -# This is the new API Access we require. -uxWorkspaceAccess=$(jq -c .requiredResourceAccess << JSON -{ - "requiredResourceAccess": [ - { - "resourceAccess": [ - { - "id": "${userImpersonationScopeId}", - "type": "Scope" - } - ], - "resourceAppId": "${workspaceAppId}" - } - ] -} -JSON -) - -# Utility function to add the required permissions. -update_resource_access "$msGraphUri" "${uxObjectId}" "${workspaceAppId}" "${uxWorkspaceAccess}" - -echo "Grant UX delegated access '${appName}' (Client ID ${uxClientId})" -az ad app permission grant --id "${uxClientId}" --api "${workspaceAppId}" --scope "user_impersonation" --only-show-errors - -if [[ -n ${automationClientId} ]]; then - echo "Searching for existing Automation application (${automationClientId})." - existingAutomationApp=$(get_existing_app --id "${automationClientId}") - - automationAppObjectId=$(echo "${existingAutomationApp}" | jq -r .id) - automationAppName=$(echo "${existingAutomationApp}" | jq -r .displayName) - echo "Found '${automationAppName}' with ObjectId: '${automationAppObjectId}'" - - # This is the new API Access we require. - automationWorkspaceAccess=$(jq -c .requiredResourceAccess << JSON -{ - "requiredResourceAccess": [ - { - "resourceAccess": [ - { - "id": "${userImpersonationScopeId}", - "type": "Scope" - }, - { - "id": "${ownerRoleId}", - "type": "Role" - }, - { - "id": "${researcherRoleId}", - "type": "Role" - }, - { - "id": "${airlockManagerRoleId}", - "type": "Role" - } - ], - "resourceAppId": "${workspaceAppId}" - } - ] -} -JSON -) - - # Utility function to add the required permissions. - update_resource_access "$msGraphUri" "${automationAppObjectId}" "${workspaceAppId}" "${automationWorkspaceAccess}" - - # Grant admin consent for the delegated workspace scopes - if [[ $grantAdminConsent -eq 1 ]]; then - echo "Granting admin consent for \"${automationAppName}\" (Client ID ${automationClientId})" - automationSpId=$(az ad sp list --filter "appId eq '${automationClientId}'" --query '[0].id' --output tsv --only-show-errors) - echo "Found Service Principal \"$automationSpId\" for \"${automationAppName}\"." - - grant_admin_consent "${automationSpId}" "${workspaceSpId}" "${ownerRoleId}" - grant_admin_consent "${automationSpId}" "${workspaceSpId}" "${airlockManagerRoleId}" - grant_admin_consent "${automationSpId}" "${workspaceSpId}" "${researcherRoleId}" - az ad app permission grant --id "$automationSpId" --api "$workspaceAppId" --scope "user_impersonation" --only-show-errors - fi +if [[ -z ${appObjectId} ]]; then + appObjectId=$(az ad app show --id "${workspaceAppId}" --query "id" --output tsv --only-show-errors) fi -# Set outputs in configuration file -yq -i ".authentication.workspace_api_client_id |= \"${workspaceAppId}\"" config.yaml -yq -i ".authentication.workspace_api_client_secret |= \"${spPassword}\"" config.yaml +# Make the current user and the application administrator owners so Terraform can update later. +az ad app owner add --id "${workspaceAppId}" --owner-object-id "$currentUserId" --only-show-errors || true +az ad app owner add --id "${workspaceAppId}" --owner-object-id "$applicationAdminObjectId" --only-show-errors || true -echo "workspace_api_client_id=\"${workspaceAppId}\"" -echo "workspace_api_client_secret=\"${spPassword}\"" +# Create a Service Principal for the app (without exposing credentials). +create_or_update_service_principal "${workspaceAppId}" 0 >/dev/null + +# Output the application details +echo +echo "==========================================" +echo "Workspace Application Client ID: ${workspaceAppId}" +echo "==========================================" +echo -if [[ $grantAdminConsent -eq 0 ]]; then - echo "NOTE: Make sure the API permissions of the app registrations have admin consent granted." - echo "Run this script with flag -a to grant admin consent or configure the registrations in Azure Portal." - echo "See APP REGISTRATIONS in documentation for more information." -fi diff --git a/devops/scripts/aad/wait_for_new_app_registration.sh b/devops/scripts/aad/wait_for_new_app_registration.sh index 8a2a0eaea2..d47d8effc6 100755 --- a/devops/scripts/aad/wait_for_new_app_registration.sh +++ b/devops/scripts/aad/wait_for_new_app_registration.sh @@ -25,6 +25,4 @@ function wait_for_new_app_registration() echo "Failed" exit 1 fi - - echo "App registration \"${clientId}\" found." } diff --git a/templates/workspaces/base/porter.yaml b/templates/workspaces/base/porter.yaml index eb58e93a9d..1239aec1f4 100644 --- a/templates/workspaces/base/porter.yaml +++ b/templates/workspaces/base/porter.yaml @@ -1,7 +1,7 @@ --- schemaVersion: 1.0.0 name: tre-workspace-base -version: 2.8.0 +version: 2.8.3 description: "A base Azure TRE workspace" dockerfile: Dockerfile.tmpl registry: azuretre @@ -64,10 +64,6 @@ parameters: - name: enable_local_debugging type: boolean default: false - - name: register_aad_application - type: boolean - default: false - description: "Whether this bundle should register the workspace in AAD" - name: create_aad_groups type: boolean default: true @@ -84,12 +80,6 @@ parameters: description: "The client id of the workspace in the identity provider. This value is typically provided to you when you create the ws application" - - name: client_secret - type: string - description: - "The client secret of the workspace in the identity provider. This value is typically provided to you - when you create the ws application" - default: "" - name: ui_client_id type: string default: "" @@ -243,7 +233,6 @@ install: address_spaces: ${ bundle.parameters.address_spaces } shared_storage_quota: ${ bundle.parameters.shared_storage_quota } enable_local_debugging: ${ bundle.parameters.enable_local_debugging } - register_aad_application: ${ bundle.parameters.register_aad_application } create_aad_groups: ${ bundle.parameters.create_aad_groups } core_api_client_id: ${ bundle.parameters.core_api_client_id } auth_client_id: ${ bundle.credentials.auth_client_id } @@ -251,7 +240,6 @@ install: auth_tenant_id: ${ bundle.credentials.auth_tenant_id } workspace_owner_object_id: ${ bundle.parameters.workspace_owner_object_id } client_id: ${ bundle.parameters.client_id } - client_secret: ${ bundle.parameters.client_secret } ui_client_id: ${ bundle.parameters.ui_client_id } scope_id: ${ bundle.parameters.scope_id } sp_id: ${ bundle.parameters.sp_id } @@ -303,7 +291,6 @@ upgrade: address_spaces: ${ bundle.parameters.address_spaces } shared_storage_quota: ${ bundle.parameters.shared_storage_quota } enable_local_debugging: ${ bundle.parameters.enable_local_debugging } - register_aad_application: ${ bundle.parameters.register_aad_application } create_aad_groups: ${ bundle.parameters.create_aad_groups } core_api_client_id: ${ bundle.parameters.core_api_client_id } auth_client_id: ${ bundle.credentials.auth_client_id } @@ -311,7 +298,6 @@ upgrade: auth_tenant_id: ${ bundle.credentials.auth_tenant_id } workspace_owner_object_id: ${ bundle.parameters.workspace_owner_object_id } client_id: ${ bundle.parameters.client_id } - client_secret: ${ bundle.parameters.client_secret } ui_client_id: ${ bundle.parameters.ui_client_id } scope_id: ${ bundle.parameters.scope_id } sp_id: ${ bundle.parameters.sp_id } @@ -351,30 +337,6 @@ upgrade: - name: workspace_owners_group_id - name: workspace_researchers_group_id - name: workspace_airlock_managers_group_id - - az: - description: "Set Azure Cloud Environment" - arguments: - - cloud - - set - flags: - name: ${ bundle.parameters.azure_environment } - - az: - description: "AAD Application Admin Login" - arguments: - - login - flags: - service-principal: "" - username: "${ bundle.credentials.auth_client_id }" - password: "${ bundle.credentials.auth_client_secret }" - tenant: "${ bundle.credentials.auth_tenant_id }" - allow-no-subscriptions: "" - - exec: - description: "Update workspace app redirect urls" - command: ./update_redirect_urls.sh - flags: - workspace-api-client-id: "${ bundle.parameters.client_id }" - aad-redirect-uris-b64: "${ bundle.parameters.aad_redirect_uris }" - register-aad-application: "${ bundle.parameters.register_aad_application }" uninstall: - terraform: @@ -387,7 +349,6 @@ uninstall: address_spaces: ${ bundle.parameters.address_spaces } shared_storage_quota: ${ bundle.parameters.shared_storage_quota } enable_local_debugging: ${ bundle.parameters.enable_local_debugging } - register_aad_application: ${ bundle.parameters.register_aad_application } create_aad_groups: ${ bundle.parameters.create_aad_groups } core_api_client_id: ${ bundle.parameters.core_api_client_id } auth_client_id: ${ bundle.credentials.auth_client_id } diff --git a/templates/workspaces/base/template_schema.json b/templates/workspaces/base/template_schema.json index 08a0434164..96fecd3cab 100644 --- a/templates/workspaces/base/template_schema.json +++ b/templates/workspaces/base/template_schema.json @@ -74,6 +74,13 @@ ], "updateable": true }, + "create_aad_groups": { + "type": "boolean", + "title": "Create AAD Groups for each workspace role (Required for user management)", + "description": "Create AAD Groups for the workspace roles. If this is set to true, the workspace will create new AAD Groups.", + "default": true, + "updateable": true + }, "enable_backup": { "type": "boolean", "title": "Enable Backup", @@ -243,13 +250,6 @@ "title": "Application (Client) ID", "description": "The AAD Application Registration ID for the workspace.", "updateable": true - }, - "client_secret": { - "type": "string", - "title": "Application (Client) Secret", - "description": "The AAD Application Registration secret for the workspace. This value will be stored in the Workspace Key Vault.", - "sensitive": true, - "updateable": true } }, "required": [ @@ -258,13 +258,6 @@ }, "else": { "properties": { - "create_aad_groups": { - "type": "boolean", - "title": "Create AAD Groups for each workspace role (Required for user management)", - "description": "Create AAD Groups for the workspace roles. If this is set to true, the workspace will create new AAD Groups.", - "default": false, - "updateable": true - }, "aad_redirect_uris": { "type": "array", "title": "AAD Redirect URIs", @@ -348,7 +341,6 @@ "auth_type", "create_aad_groups", "client_id", - "client_secret", "enable_backup", "enable_airlock", "configure_review_vms", @@ -356,4 +348,4 @@ "*" ] } -} \ No newline at end of file +} diff --git a/templates/workspaces/base/terraform/.terraform.lock.hcl b/templates/workspaces/base/terraform/.terraform.lock.hcl index 1edfb23bd0..1db3592643 100644 --- a/templates/workspaces/base/terraform/.terraform.lock.hcl +++ b/templates/workspaces/base/terraform/.terraform.lock.hcl @@ -81,3 +81,23 @@ provider "registry.terraform.io/hashicorp/random" { "zh:eac7b63e86c749c7d48f527671c7aee5b4e26c10be6ad7232d6860167f99dbb0", ] } + +provider "registry.terraform.io/hashicorp/time" { + version = "0.11.0" + constraints = ">= 0.11.0, 0.11.0" + hashes = [ + "h1:JijCYFGrDkwZ1PeeBnHOXf0qZcWrVkQ0hR8hA4Fk+eA=", + "zh:0260e2b7b4b68b443d4d6011a5bfe82a183adcdfa5088bc976a2f6974dff2de7", + "zh:246eb0e770161b18dccd68c60d8d4595843b1d6c38b208cd593f0452509acdc7", + "zh:2e46651b87bfd825eae8b23368ede64224741822f5ae6ff8caeecd5e9bf943cf", + "zh:3d9b565d744f7ee70164eae678480d95ffced1a599baaf833a82819105b99655", + "zh:78d5eefdd9e494defcb3c68d282b8f96630502cac21d1ea161f53cfe9bb483b3", + "zh:7bb38270dc962b1f185722637cc2d459ced29e9aeec031421816bcaeb9b06846", + "zh:7c30e2c01d408cea9dc4336c27f9d75a67c53974f773fec28ce145b8f4c07cd8", + "zh:8a09dfde3224f44ba859d675cd8e8abeeb1c88b2f6eda5722486bbe5034e746b", + "zh:9327865ae07e312d90727f2814d55ea5a7b92552f1166961dae114620d8847f1", + "zh:9668f39dd4d99fcedc4c46461ec89d83fbe30dd06f67c47f76cf4ffdf2a78867", + "zh:b52dfeb5a736dc7d7e3e830bde85374f46f6ff82dff04a94a8e5ca826d695c89", + "zh:bcb2a08d6d1d18a2331195812b5ab963fe2fc3ee8f6584047dc2831820dcc93e", + ] +} diff --git a/templates/workspaces/base/terraform/aad/aad.tf b/templates/workspaces/base/terraform/aad/aad.tf index fd3acd0c3b..f50b1a4260 100644 --- a/templates/workspaces/base/terraform/aad/aad.tf +++ b/templates/workspaces/base/terraform/aad/aad.tf @@ -9,6 +9,15 @@ resource "random_uuid" "app_role_workspace_owner_id" {} resource "random_uuid" "app_role_workspace_researcher_id" {} resource "random_uuid" "app_role_workspace_airlock_manager_id" {} +locals { + workspace_sp_password_rotation_days = 30 + workspace_sp_password_validity_days = 180 +} + +resource "time_rotating" "workspace_sp_password" { + rotation_days = local.workspace_sp_password_rotation_days +} + resource "azuread_application" "workspace" { display_name = var.workspace_resource_name_suffix identifier_uris = ["api://${var.workspace_resource_name_suffix}"] @@ -114,6 +123,14 @@ resource "azuread_service_principal_delegated_permission_grant" "ui" { resource "azuread_service_principal_password" "workspace" { service_principal_id = azuread_service_principal.workspace.id + end_date = timeadd( + time_rotating.workspace_sp_password.rotation_rfc3339, + format("%dh", local.workspace_sp_password_validity_days * 24) + ) + + rotate_when_changed = { + rotation = time_rotating.workspace_sp_password.rotation_rfc3339 + } } resource "azurerm_key_vault_secret" "client_id" { @@ -135,7 +152,7 @@ resource "azurerm_key_vault_secret" "client_secret" { } resource "azuread_app_role_assignment" "workspace_owner" { - app_role_id = azuread_service_principal.workspace.app_role_ids["WorkspaceOwner"] + app_role_id = azuread_application.workspace.app_role_ids["WorkspaceOwner"] principal_object_id = var.workspace_owner_object_id resource_object_id = azuread_service_principal.workspace.object_id } @@ -169,21 +186,21 @@ resource "azuread_group_member" "workspace_owner" { resource "azuread_app_role_assignment" "workspace_owners_group" { count = var.create_aad_groups ? 1 : 0 - app_role_id = azuread_service_principal.workspace.app_role_ids["WorkspaceOwner"] + app_role_id = azuread_application.workspace.app_role_ids["WorkspaceOwner"] principal_object_id = azuread_group.workspace_owners[count.index].object_id resource_object_id = azuread_service_principal.workspace.object_id } resource "azuread_app_role_assignment" "workspace_researchers_group" { count = var.create_aad_groups ? 1 : 0 - app_role_id = azuread_service_principal.workspace.app_role_ids["WorkspaceResearcher"] + app_role_id = azuread_application.workspace.app_role_ids["WorkspaceResearcher"] principal_object_id = azuread_group.workspace_researchers[count.index].object_id resource_object_id = azuread_service_principal.workspace.object_id } resource "azuread_app_role_assignment" "workspace_airlock_managers_group" { count = var.create_aad_groups ? 1 : 0 - app_role_id = azuread_service_principal.workspace.app_role_ids["AirlockManager"] + app_role_id = azuread_application.workspace.app_role_ids["AirlockManager"] principal_object_id = azuread_group.workspace_airlock_managers[count.index].object_id resource_object_id = azuread_service_principal.workspace.object_id } diff --git a/templates/workspaces/base/terraform/aad/providers.tf b/templates/workspaces/base/terraform/aad/providers.tf index 0da1fc977c..c2062a7e31 100644 --- a/templates/workspaces/base/terraform/aad/providers.tf +++ b/templates/workspaces/base/terraform/aad/providers.tf @@ -13,5 +13,9 @@ terraform { source = "hashicorp/random" version = ">= 3.7.2" } + time = { + source = "hashicorp/time" + version = ">= 0.11.0" + } } } diff --git a/templates/workspaces/base/terraform/aad/variables.tf b/templates/workspaces/base/terraform/aad/variables.tf index a93df28733..7503fe46e8 100644 --- a/templates/workspaces/base/terraform/aad/variables.tf +++ b/templates/workspaces/base/terraform/aad/variables.tf @@ -1,3 +1,7 @@ +variable "client_id" { + type = string + default = "" +} variable "key_vault_id" { type = string } @@ -14,19 +18,15 @@ variable "aad_redirect_uris_b64" { type = string # list of objects like [{"name": "my uri 1", "value": "https://..."}, {}] } variable "create_aad_groups" { - type = string + type = bool } - variable "ui_client_id" { type = string } - variable "auto_grant_workspace_consent" { type = bool default = false } - variable "core_api_client_id" { type = string } - diff --git a/templates/workspaces/base/terraform/keyvault.tf b/templates/workspaces/base/terraform/keyvault.tf index 3ddec760c9..27518d7dcb 100644 --- a/templates/workspaces/base/terraform/keyvault.tf +++ b/templates/workspaces/base/terraform/keyvault.tf @@ -103,43 +103,3 @@ resource "azurerm_key_vault_secret" "aad_tenant_id" { lifecycle { ignore_changes = [tags] } } - -# This secret only gets written if Terraform is not responsible for -# registering the AAD Application -resource "azurerm_key_vault_secret" "client_id" { - name = "workspace-client-id" - value = var.client_id - key_vault_id = azurerm_key_vault.kv.id - count = var.register_aad_application ? 0 : 1 - tags = local.tre_workspace_tags - depends_on = [ - azurerm_role_assignment.keyvault_deployer_ws_role, - azurerm_role_assignment.keyvault_resourceprocessor_ws_role, - terraform_data.wait_for_dns_vault - ] - - lifecycle { ignore_changes = [tags] } -} - -data "azurerm_key_vault_secret" "client_secret" { - count = var.client_secret == local.redacted_senstive_value ? 1 : 0 - name = "workspace-client-secret" - key_vault_id = azurerm_key_vault.kv.id -} - -# This secret only gets written if Terraform is not responsible for -# registering the AAD Application -resource "azurerm_key_vault_secret" "client_secret" { - name = "workspace-client-secret" - value = var.client_secret == local.redacted_senstive_value ? data.azurerm_key_vault_secret.client_secret[0].value : var.client_secret - key_vault_id = azurerm_key_vault.kv.id - count = var.register_aad_application ? 0 : 1 - tags = local.tre_workspace_tags - depends_on = [ - azurerm_role_assignment.keyvault_deployer_ws_role, - azurerm_role_assignment.keyvault_resourceprocessor_ws_role, - terraform_data.wait_for_dns_vault - ] - - lifecycle { ignore_changes = [tags] } -} diff --git a/templates/workspaces/base/terraform/outputs.tf b/templates/workspaces/base/terraform/outputs.tf index 2bc0c2c716..5305d4a9c9 100644 --- a/templates/workspaces/base/terraform/outputs.tf +++ b/templates/workspaces/base/terraform/outputs.tf @@ -2,31 +2,28 @@ 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 + value = module.aad.app_role_workspace_owner_id } output "app_role_id_workspace_researcher" { - value = var.register_aad_application ? module.aad[0].app_role_workspace_researcher_id : var.app_role_id_workspace_researcher + value = module.aad.app_role_workspace_researcher_id } output "app_role_id_workspace_airlock_manager" { - value = var.register_aad_application ? module.aad[0].app_role_workspace_airlock_manager_id : var.app_role_id_workspace_airlock_manager + value = module.aad.app_role_workspace_airlock_manager_id } output "client_id" { - value = var.register_aad_application ? module.aad[0].client_id : var.client_id + value = module.aad.client_id } output "sp_id" { - value = var.register_aad_application ? module.aad[0].sp_id : var.sp_id + value = module.aad.sp_id } output "scope_id" { - value = var.register_aad_application ? module.aad[0].scope_id : var.scope_id + value = module.aad.scope_id } output "backup_vault_name" { @@ -46,13 +43,13 @@ output "log_analytics_workspace_name" { } output "workspace_owners_group_id" { - value = var.register_aad_application ? module.aad[0].workspace_owners_group_id : "" + value = module.aad.workspace_owners_group_id } output "workspace_researchers_group_id" { - value = var.register_aad_application ? module.aad[0].workspace_researchers_group_id : "" + value = module.aad.workspace_researchers_group_id } output "workspace_airlock_managers_group_id" { - value = var.register_aad_application ? module.aad[0].workspace_airlock_managers_group_id : "" + value = module.aad.workspace_airlock_managers_group_id } diff --git a/templates/workspaces/base/terraform/providers.tf b/templates/workspaces/base/terraform/providers.tf index cf04a61767..44f12a6dbc 100644 --- a/templates/workspaces/base/terraform/providers.tf +++ b/templates/workspaces/base/terraform/providers.tf @@ -12,6 +12,10 @@ terraform { source = "Azure/azapi" version = "= 2.3.0" } + time = { + source = "hashicorp/time" + version = "= 0.11.0" + } } backend "azurerm" {} diff --git a/templates/workspaces/base/terraform/variables.tf b/templates/workspaces/base/terraform/variables.tf index b475c0135c..aefecb90b7 100644 --- a/templates/workspaces/base/terraform/variables.tf +++ b/templates/workspaces/base/terraform/variables.tf @@ -47,12 +47,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 "register_aad_application" { - type = bool - default = false - description = "Create an AAD application automatically for the Workspace." -} - variable "create_aad_groups" { type = bool default = false @@ -115,12 +109,6 @@ variable "client_id" { default = "" description = "The client id of the workspace in the identity provider, this is passed in so that we may return it as an output." } -variable "client_secret" { - type = string - default = "" - sensitive = true - description = "The client secret of the workspace in the identity provider, this is passed in so that we may return it as an output." -} variable "ui_client_id" { type = string } diff --git a/templates/workspaces/base/terraform/workspace.tf b/templates/workspaces/base/terraform/workspace.tf index 8008c545bd..d15386113c 100644 --- a/templates/workspaces/base/terraform/workspace.tf +++ b/templates/workspaces/base/terraform/workspace.tf @@ -1,3 +1,23 @@ +data "azuread_application" "existing_workspace" { + count = var.client_id != "" ? 1 : 0 + client_id = var.client_id +} + +data "azuread_service_principal" "existing_workspace" { + count = var.client_id != "" ? 1 : 0 + client_id = var.client_id +} + +locals { + workspace_app_imports = var.client_id != "" ? { + existing = format("/applications/%s", data.azuread_application.existing_workspace[0].object_id) + } : {} + + workspace_sp_imports = var.client_id != "" ? { + existing = format("/servicePrincipals/%s", data.azuread_service_principal.existing_workspace[0].object_id) + } : {} +} + resource "azurerm_resource_group" "ws" { location = var.location name = "rg-${local.workspace_resource_name_suffix}" @@ -36,7 +56,7 @@ module "network" { module "aad" { source = "./aad" tre_workspace_tags = local.tre_workspace_tags - count = var.register_aad_application ? 1 : 0 + client_id = var.client_id key_vault_id = azurerm_key_vault.kv.id workspace_resource_name_suffix = local.workspace_resource_name_suffix workspace_owner_object_id = var.workspace_owner_object_id @@ -52,6 +72,18 @@ module "aad" { ] } +import { + for_each = local.workspace_app_imports + to = module.aad.azuread_application.workspace + id = each.value +} + +import { + for_each = local.workspace_sp_imports + to = module.aad.azuread_service_principal.workspace + id = each.value +} + module "airlock" { count = var.enable_airlock ? 1 : 0 source = "./airlock" From 6904b72009c67377af19a0b529c3c94b0f3f17c7 Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Wed, 26 Nov 2025 09:26:33 +0000 Subject: [PATCH 03/29] Update scripts and docs. --- config_schema.json | 5 - core/terraform/outputs.sh | 1 - ...tomation_admin_to_workspace_application.sh | 141 ++++++++++++++++++ .../aad/create_workspace_application.sh | 12 +- devops/scripts/create_aad_assets.sh | 15 -- devops/scripts/setup_local_debugging.sh | 1 - docs/tre-admins/auth.md | 1 - docs/tre-admins/environment-variables.md | 1 - docs/tre-admins/identities/workspace.md | 22 +-- .../cicd-pre-deployment-steps.md | 1 - .../installing-base-workspace.md | 6 - .../ui-install-base-workspace.md | 12 +- docs/tre-developers/end-to-end-tests.md | 12 ++ 13 files changed, 162 insertions(+), 68 deletions(-) create mode 100755 devops/scripts/aad/add_automation_admin_to_workspace_application.sh diff --git a/config_schema.json b/config_schema.json index abfaf97217..99ca3c9171 100644 --- a/config_schema.json +++ b/config_schema.json @@ -210,11 +210,6 @@ "description": "Workspace AD Application. This will be created for you for future use - when creating workspaces.", "type": "string", "pattern": "^[{]?[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}[}]?$" - }, - "workspace_api_client_secret": { - "description": "Workspace AD Application secret. This will be created for you for future use - when creating workspaces.", - "type": "string", - "minLength": 11 } }, "required": [ diff --git a/core/terraform/outputs.sh b/core/terraform/outputs.sh index 4b2ffa8347..cbbe3b351f 100644 --- a/core/terraform/outputs.sh +++ b/core/terraform/outputs.sh @@ -33,7 +33,6 @@ fi # Add a few extra values to the file to help us (i.e. for local debugging api_app and resource processor) # shellcheck disable=SC2129 echo "TEST_WORKSPACE_APP_ID='${WORKSPACE_API_CLIENT_ID}'" >> ../private.env -echo "TEST_WORKSPACE_APP_SECRET='${WORKSPACE_API_CLIENT_SECRET}'" >> ../private.env # These next ones from Check Dependencies echo "SUBSCRIPTION_ID='${SUB_ID}'" >> ../private.env diff --git a/devops/scripts/aad/add_automation_admin_to_workspace_application.sh b/devops/scripts/aad/add_automation_admin_to_workspace_application.sh new file mode 100755 index 0000000000..9023a78436 --- /dev/null +++ b/devops/scripts/aad/add_automation_admin_to_workspace_application.sh @@ -0,0 +1,141 @@ +#!/bin/bash +set -euo pipefail +# Use this for debug only +# set -o xtrace +# AZURE_CORE_OUTPUT=jsonc # force CLI output to JSON for the script (user can still change default for interactive usage in the dev container) + +function show_usage() +{ + cat << USAGE + +Utility script for adding the Automation Admin Application as an owner to the Workspace Application. +This is required for the end-to-end tests to be able to create and manage workspaces. +You must be logged in using Azure CLI with sufficient privileges to modify Azure Active Directory to run this script. + +Usage: $0 --workspace-application-clientid + +Options: + -w,--workspace-application-clientid Required. Client ID of the workspace application to add automation admin to. + +Note: The automation admin client ID is automatically read from config.yaml (authentication.test_account_client_id). +The automation admin will be assigned the WorkspaceOwner role. + +USAGE + exit 2 +} + +if ! command -v az &> /dev/null; then + echo "This script requires Azure CLI" 1>&2 + exit 1 +fi + +if ! command -v yq &> /dev/null; then + echo "This script requires yq" 1>&2 + exit 1 +fi + +# Get the directory that this script is in +DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" + +declare workspaceAppClientId="" +declare automationAdminClientId="" +declare appRole="WorkspaceOwner" +declare msGraphUri="" + +# Initialize parameters specified from command line +while [[ $# -gt 0 ]]; do + case "$1" in + -w|--workspace-application-clientid) + workspaceAppClientId=$2 + shift 2 + ;; + *) + echo "Invalid option: $1." + show_usage + ;; + esac +done + +################################### +# CHECK INCOMING PARAMETERS # +################################### +if [[ $(az account list --only-show-errors -o json | jq 'length') -eq 0 ]]; then + echo "Please run az login -t --allow-no-subscriptions" + exit 1 +fi + +if [[ -z "$workspaceAppClientId" ]]; then + echo "Please specify the workspace application client ID" 1>&2 + show_usage +fi + +# Read automation admin client ID from config.yaml +if [[ -f "${DIR}/../../../config.yaml" ]]; then + automationAdminClientId=$(yq '.authentication.test_account_client_id' "${DIR}/../../../config.yaml") + if [[ "$automationAdminClientId" == "null" || -z "$automationAdminClientId" ]]; then + echo "Could not find test_account_client_id in config.yaml. Please ensure the automation admin has been created." 1>&2 + echo "Run: make auth" 1>&2 + exit 1 + fi + echo "Using automation admin client ID from config.yaml: ${automationAdminClientId}" +else + echo "config.yaml not found. Please ensure you are running this script from the TRE root directory." 1>&2 + exit 1 +fi + +msGraphUri="$(az cloud show --query endpoints.microsoftGraphResourceId --output tsv)/v1.0" +tenant=$(az rest -m get -u "${msGraphUri}/domains" -o json | jq -r '.value[] | select(.isDefault == true) | .id') + +echo -e "\e[96mAdding Automation Admin to Workspace Application in the \"${tenant}\" Azure AD tenant.\e[0m" + +# Load helper functions +# shellcheck disable=SC1091 +source "${DIR}/grant_admin_consent.sh" + +# Get the service principal object IDs +echo "Getting automation admin service principal..." +automationAdminSpObjectId=$(az ad sp show --id "$automationAdminClientId" --query id -o tsv --only-show-errors) +if [[ -z "$automationAdminSpObjectId" ]]; then + echo "Could not find service principal for automation admin client ID: $automationAdminClientId" 1>&2 + exit 1 +fi + +echo "Getting workspace application service principal..." +workspaceSpObjectId=$(az ad sp show --id "$workspaceAppClientId" --query id -o tsv --only-show-errors) +if [[ -z "$workspaceSpObjectId" ]]; then + echo "Could not find service principal for workspace client ID: $workspaceAppClientId" 1>&2 + exit 1 +fi + +# Get the app role ID for the specified role +echo "Getting app role ID for role: $appRole" +workspaceApp=$(az ad app show --id "$workspaceAppClientId" --query "appRoles[?value=='$appRole']" -o json --only-show-errors) +if [[ $(echo "$workspaceApp" | jq length) -eq 0 ]]; then + echo "Could not find app role '$appRole' in workspace application $workspaceAppClientId" 1>&2 + echo "Available app roles:" + az ad app show --id "$workspaceAppClientId" --query "appRoles[].{value:value,displayName:displayName}" -o table --only-show-errors + exit 1 +fi + +appRoleId=$(echo "$workspaceApp" | jq -r '.[0].id') +echo "Found app role ID: $appRoleId" + +# Check if the role assignment already exists +echo "Checking if role assignment already exists..." +existing_assignment=$(az rest --method GET \ + --uri "${msGraphUri}/servicePrincipals/${automationAdminSpObjectId}/appRoleAssignments" -o json \ + | jq -r ".value | map(select(.appRoleId==\"${appRoleId}\" and .resourceId==\"${workspaceSpObjectId}\")) | length") + +if [[ "$existing_assignment" == "1" ]]; then + echo "Role assignment already exists. Automation Admin already has the $appRole role for this workspace application." + exit 0 +fi + +# Grant the app role assignment +echo "Assigning $appRole role to Automation Admin..." +grant_admin_consent "$automationAdminSpObjectId" "$workspaceSpObjectId" "$appRoleId" + +echo -e "\e[92mSuccessfully assigned $appRole role to Automation Admin for workspace application.\e[0m" +echo "Automation Admin Service Principal: $automationAdminSpObjectId" +echo "Workspace Application Service Principal: $workspaceSpObjectId" +echo "App Role: $appRole ($appRoleId)" diff --git a/devops/scripts/aad/create_workspace_application.sh b/devops/scripts/aad/create_workspace_application.sh index 23d4e9d1ff..996e081025 100755 --- a/devops/scripts/aad/create_workspace_application.sh +++ b/devops/scripts/aad/create_workspace_application.sh @@ -9,17 +9,17 @@ function show_usage() cat << USAGE Utility script for pre-creating the Workspace API Azure AD application registration -and corresponding service principal so that Terraform can import and configure it -without requiring Application.ReadWrite.All permissions. +and corresponding service principal. Usage: $0 --name --application-admin-clientid Options: - -n,--name Required. Prefix for the Workspace API app registration name. - The script appends " API" to keep naming consistent with Terraform. + -n,--name + Required. Prefix for the Workspace API app registration name. + The script appends " API" to keep naming consistent with Terraform. -y,--application-admin-clientid - Required. Client ID of the application administrator (typically the TRE Core API - app registration) that must be added as an owner of the workspace application. + Required. Client ID of the application administrator (typically the TRE Core API + app registration) that must be added as an owner of the workspace application. USAGE exit 2 diff --git a/devops/scripts/create_aad_assets.sh b/devops/scripts/create_aad_assets.sh index ff5727ba37..90f72b75f7 100755 --- a/devops/scripts/create_aad_assets.sh +++ b/devops/scripts/create_aad_assets.sh @@ -74,21 +74,6 @@ APPLICATION_PERMISSION=$(IFS=,; echo "${APPLICATION_PERMISSIONS[*]}") --reset-password $RESET_PASSWORDS \ --custom-domain "${CUSTOM_DOMAIN}" -if [ "${AUTO_WORKSPACE_APP_REGISTRATION:=false}" == false ]; then - # Load the new values back in - # This is because we want the SWAGGER_UI_CLIENT_ID - # shellcheck disable=SC1091 - . "$DIR/load_and_validate_env.sh" - - "$DIR/aad/create_workspace_application.sh" \ - --name "${TRE_ID} - workspace 1" \ - --admin-consent \ - --ux-clientid "${SWAGGER_UI_CLIENT_ID}" \ - --automation-clientid "${TEST_ACCOUNT_CLIENT_ID}" \ - --application-admin-clientid "${APPLICATION_ADMIN_CLIENT_ID}" \ - --reset-password $RESET_PASSWORDS -fi - if [ "${CHANGED_TENANT}" -ne 0 ]; then echo "Attempting to sign you back into ${LOGGED_IN_TENANT_ID}." diff --git a/devops/scripts/setup_local_debugging.sh b/devops/scripts/setup_local_debugging.sh index 2bf70a63d0..1d18ffc25f 100755 --- a/devops/scripts/setup_local_debugging.sh +++ b/devops/scripts/setup_local_debugging.sh @@ -173,7 +173,6 @@ AAD_TENANT_ID=${AAD_TENANT_ID} APPLICATION_ADMIN_CLIENT_ID=${APPLICATION_ADMIN_CLIENT_ID} APPLICATION_ADMIN_CLIENT_SECRET=${APPLICATION_ADMIN_CLIENT_SECRET} TEST_WORKSPACE_APP_ID=${WORKSPACE_API_CLIENT_ID} -TEST_WORKSPACE_APP_SECRET=${WORKSPACE_API_CLIENT_SECRET} EOF # copy porter configuration to porter home diff --git a/docs/tre-admins/auth.md b/docs/tre-admins/auth.md index c8c1fce669..96ed0f64bd 100644 --- a/docs/tre-admins/auth.md +++ b/docs/tre-admins/auth.md @@ -37,7 +37,6 @@ The contents of your authentication section in `config.yaml` file should contain | `API_CLIENT_SECRET` | API application client secret. | | `SWAGGER_UI_CLIENT_ID` | Swagger (OpenAPI) UI application (client) ID. | | `WORKSPACE_API_CLIENT_ID` | Each workspace is secured behind it's own AD Application| - | `WORKSPACE_API_CLIENT_SECRET` | Each workspace is secured behind it's own AD Application. This is the secret for that application.| ### Using a separate Microsoft Entra ID tenant diff --git a/docs/tre-admins/environment-variables.md b/docs/tre-admins/environment-variables.md index 2275dd15c6..22bfa8d494 100644 --- a/docs/tre-admins/environment-variables.md +++ b/docs/tre-admins/environment-variables.md @@ -67,7 +67,6 @@ | `API_CLIENT_SECRET` | API application client secret. | | `SWAGGER_UI_CLIENT_ID` | Swagger (OpenAPI) UI application (client) ID. | | `WORKSPACE_API_CLIENT_ID` | Each workspace is secured behind it's own AD Application| - | `WORKSPACE_API_CLIENT_SECRET` | Each workspace is secured behind it's own AD Application. This is the secret for that application.| ## For CI/CD pipelines in github environment secrets diff --git a/docs/tre-admins/identities/workspace.md b/docs/tre-admins/identities/workspace.md index 8bc5b1989a..65bd142391 100644 --- a/docs/tre-admins/identities/workspace.md +++ b/docs/tre-admins/identities/workspace.md @@ -12,6 +12,7 @@ Access to workspaces is also controlled using app registrations - one per worksp | Airlock Manager | Provides airlock managers access to the Workspace and ability to review airlock requests. | Users/Groups,Applications | `AirlockManager` | ## Microsoft Graph Permissions + | Name | Type* | Admin consent required | TRE usage | | --- | -- | -----| --------- | |email|Delegated|No|Used to read the user's email address when creating TRE resources| @@ -28,40 +29,19 @@ There are two mechanisms for creating Workspace Applications - Manually by your Microsoft Entra ID Tenant Admin (default) - Automatically by TRE. Please see this [guide](./application_admin.md) if you wish this to be automatic. -!!! caution - By default, the app registration for a workspace is not created by the [API](../../tre-developers/api.md). One needs to be present before using the API to provision a new workspace. If you ran `make auth`, a workspace AD application was created for you. If you wish to create another, the same script can be used to create the **Workspace Application**. - Example on how to run the script: ```bash ./devops/scripts/aad/create_workspace_application.sh \ --name "${TRE_ID} - workspace 11" \ - --admin-consent \ - --ux-clientid "${SWAGGER_UI_CLIENT_ID}" \ - --automation-clientid "${TEST_ACCOUNT_CLIENT_ID}" \ --application-admin-clientid "${APPLICATION_ADMIN_CLIENT_ID}" ``` | Argument | Description | | -------- | ----------- | | `--name` | The name of the application. This will be suffixed with 'API' by the script. | -| `--ux-clientid` | This value is one of the outputs when you first ran the script. It is mandatory if you use admin-consent. | -| `--admin-consent` | Grants admin consent for the app registrations. This is required for them to function properly, but requires Microsoft Entra ID admin privileges. | | `--automation-clientid` | This is an optional parameter but will grant the Automation App (created in step 1) permission to the new workspace app. | | `--application-admin-clientid` | This is a required parameter , and should be a client id that will be added to the Owners of the Microsoft Entra ID Application so that it can be administered within TRE. | -| `--reset-password` | Optional, default is 0. When run in a headless fashion, 1 is passed in to always reset the password. | - - -!!! caution - The script will create an app password (client secret) for the workspace and write to `/config.yaml` under the authentication section. These values are only shown once, if you lose them, the script will create new secrets if run again. - -If you do not wish to grant the Automation App permission to your workspace, just remove the `--automation-clientid` from the command. - -## Environment Variables -| Variable | Description | Location | -| -------- | ----------- | -------- | -|WORKSPACE_API_CLIENT_ID|The Client Id|`./config.yaml`| -|WORKSPACE_API_CLIENT_SECRET|The client secret|`./config.yaml`| ## Comments When the Workspace Microsoft Entra ID app is registered by running `make auth`, the `Workspace Scope Id` is the same as the Client Id. When the Workspace Microsoft Entra ID app is created by the base workspace, the `Workspace Scope Id` will be in this format `api://_ws_` diff --git a/docs/tre-admins/setup-instructions/cicd-pre-deployment-steps.md b/docs/tre-admins/setup-instructions/cicd-pre-deployment-steps.md index c7492f5f35..88b124be8e 100644 --- a/docs/tre-admins/setup-instructions/cicd-pre-deployment-steps.md +++ b/docs/tre-admins/setup-instructions/cicd-pre-deployment-steps.md @@ -103,7 +103,6 @@ In a previous [Setup Auth configuration](./setup-auth-entities.md) step authenti | `API_CLIENT_SECRET` | API application client secret. | | `SWAGGER_UI_CLIENT_ID` | Swagger (OpenAPI) UI application (client) ID. | | `TEST_WORKSPACE_APP_ID`| Each workspace is secured behind it's own AD Application. Use the value of `WORKSPACE_API_CLIENT_ID` created in the `/config.yaml` env file | - | `TEST_WORKSPACE_APP_SECRET`| Each workspace is secured behind it's own AD Application. This is the secret for that application. Use the value of `WORKSPACE_API_CLIENT_SECRET` created in the `/config.yaml` env file| !!! info See [Environment variables](../environment-variables.md) for full details of the deployment related variables. diff --git a/docs/tre-admins/setup-instructions/installing-base-workspace.md b/docs/tre-admins/setup-instructions/installing-base-workspace.md index f1b8dea6b4..f1877335e1 100644 --- a/docs/tre-admins/setup-instructions/installing-base-workspace.md +++ b/docs/tre-admins/setup-instructions/installing-base-workspace.md @@ -22,17 +22,12 @@ As explained in the [auth guide](../auth.md), every workspace has a correspondin ```bash ./devops/scripts/aad/create_workspace_application.sh \ --name "${TRE_ID} - workspace 1" \ - --admin-consent \ - --ux-clientid "${SWAGGER_UI_CLIENT_ID}" \ - --automation-clientid "${TEST_ACCOUNT_CLIENT_ID}" \ --application-admin-clientid "${APPLICATION_ADMIN_CLIENT_ID}" ``` !!! caution If you're using a separate tenant for Microsoft Entra ID app registrations to the one where you've deployed the TRE infrastructure resources, ensure you've signed into that tenant in the `az cli` before running the above command. See **Using a separate Microsoft Entra ID tenant** in [Setup Auth configuration](setup-auth-entities.md) for more details. -Running the script will report `workspace_api_client_id` and `workspace_api_client_secret` for the generated app. Add these under the authenrication section in `/config.yaml` so that automated testing will work. You also need to use `workspace_api_client_id` in the POST body below. - ### Create workspace using the API Go to `https:///api/docs` and use POST `/api/workspaces` with the sample body to create a base workspace. @@ -43,7 +38,6 @@ Go to `https:///api/docs` and use POST `/api/workspaces` with th "display_name": "manual-from-swagger", "description": "workspace for team X", "client_id":"", - "client_secret":"", "address_space_size": "medium", "workspace_subscription_id": "" } diff --git a/docs/tre-admins/setup-instructions/ui-install-base-workspace.md b/docs/tre-admins/setup-instructions/ui-install-base-workspace.md index 25b4e2c213..c74f919e3f 100644 --- a/docs/tre-admins/setup-instructions/ui-install-base-workspace.md +++ b/docs/tre-admins/setup-instructions/ui-install-base-workspace.md @@ -43,24 +43,16 @@ Workspace can be easily created via AzureTRE UI. Open a browser and navigate to: Workspace is now ready to use. -## Creating an Application Client for base workspace +## Creating an manuall Entra ID Application for the workspace -As explained in the [auth guide](../auth.md), every workspace has a corresponding app registration which if you haven't run `make auth`; can be created using the helper script `./devops/scripts/aad/create_workspace_application.sh`. For example: +If you have not configured automatic application registration creation as explained in the [auth guide](../auth.md), every workspace has a corresponding app registration which if you haven't run `make auth`; can be created using the helper script `./devops/scripts/aad/create_workspace_application.sh`. For example: ```bash ./devops/scripts/aad/create_workspace_application.sh \ --name "${TRE_ID} - workspace 1" \ - --admin-consent \ - --ux-clientid "${SWAGGER_UI_CLIENT_ID}" \ - --automation-clientid "${TEST_ACCOUNT_CLIENT_ID}" \ --application-admin-clientid "${APPLICATION_ADMIN_CLIENT_ID}" ``` -!!! caution - If you're using a separate tenant for Microsoft Entra ID app registrations to the one where you've deployed the TRE infrastructure resources, ensure you've signed into that tenant in the `az cli` before running the above command. See **Using a separate Microsoft Entra ID tenant** in [Setup Auth configuration](./setup-auth-entities.md) for more details. - -Running the script will report `WORKSPACE_API_CLIENT_ID` and `WORKSPACE_API_CLIENT_SECRET` for the generated app. Set these under authentication section in `config.yaml` so that automated testing will work. You also need to use `WORKSPACE_API_CLIENT_ID` and `WORKSPACE_API_CLIENT_SECRET` in the form. - ## Next steps * [Installing a workspace service & user resources](./ui-install-ws-and-ur.md) diff --git a/docs/tre-developers/end-to-end-tests.md b/docs/tre-developers/end-to-end-tests.md index b17fcf84dd..8aa6ace8f1 100644 --- a/docs/tre-developers/end-to-end-tests.md +++ b/docs/tre-developers/end-to-end-tests.md @@ -5,6 +5,18 @@ 1. Authentication and Authorization configuration set up as noted [here](../tre-admins/auth.md) 1. An Azure Tre deployed environment. +## Add Automation Admin Application as Owner of Workspace Application +For the end-to-end tests to be able to create and manage workspaces, the Automation Admin Application created as part of the auth setup needs to be added as an Owner of the Workspace Application(s) used by the TRE. + +You can do this using the Azure Portal, or by using the helper script `./devops/scripts/aad/add_automation_admin_to_workspace_application.sh`. For example: + +```bash + ./devops/scripts/aad/add_automation_admin_to_workspace_application.sh \ + --workspace-application-clientid "${WORKSPACE_API_CLIENT_ID}" +``` + +The script automatically reads the automation admin client ID from `config.yaml` (authentication.test_account_client_id). + ## Registering bundles to run End-to-end tests End-to-end tests depend on certain bundles to be registered within the TRE API. From c755f306a388b50ea9e99ebe4fcf696255175962 Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Wed, 26 Nov 2025 13:57:35 +0000 Subject: [PATCH 04/29] Remove need for Directory.Read.All --- config.sample.yaml | 4 ++-- .../scripts/aad/create_workspace_application.sh | 8 +------- devops/scripts/create_aad_assets.sh | 6 +++--- docs/tre-admins/auth.md | 4 ++-- docs/tre-admins/environment-variables.md | 4 ++-- docs/tre-admins/identities/application_admin.md | 5 +++-- templates/workspaces/base/porter.yaml | 2 +- templates/workspaces/base/terraform/aad/aad.tf | 6 +++--- templates/workspaces/base/terraform/workspace.tf | 15 --------------- 9 files changed, 17 insertions(+), 37 deletions(-) diff --git a/config.sample.yaml b/config.sample.yaml index 95fedf2835..10eb022066 100644 --- a/config.sample.yaml +++ b/config.sample.yaml @@ -76,11 +76,11 @@ authentication: # Setting AUTO_WORKSPACE_APP_REGISTRATION to false will: # create an identity with `Application.ReadWrite.OwnedBy`. # Setting AUTO_WORKSPACE_APP_REGISTRATION to true will: - # create an identity with `Application.ReadWrite.All` and `Directory.Read.All`. + # create an identity with `Application.ReadWrite.All`. # When this is true, create Workspaces will also create an AAD Application automatically. # When this is false, the AAD Application will need creating manually. auto_workspace_app_registration: true - # Setting AUTO_WORKSPACE_GROUP_CREATION to true will create an identity with `Group.Create` + # Setting AUTO_WORKSPACE_GROUP_CREATION to true will create an identity with `Group.Create`, `Group.Read.All` and `User.ReadBasic.All` permissions. auto_workspace_group_creation: false # Setting this to true will remove the need for users to manually grant consent when creating new workspaces. # The identity will be granted Application.ReadWrite.All and DelegatedPermissionGrant.ReadWrite.All permissions. diff --git a/devops/scripts/aad/create_workspace_application.sh b/devops/scripts/aad/create_workspace_application.sh index 996e081025..698dc4e566 100755 --- a/devops/scripts/aad/create_workspace_application.sh +++ b/devops/scripts/aad/create_workspace_application.sh @@ -8,8 +8,7 @@ function show_usage() { cat << USAGE -Utility script for pre-creating the Workspace API Azure AD application registration -and corresponding service principal. +Utility script for pre-creating the Workspace API Azure AD application registration. Usage: $0 --name --application-admin-clientid @@ -40,7 +39,6 @@ DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" declare appName="" declare workspaceAppId="" -declare workspaceSpId="" declare appObjectId="" declare applicationAdminClientId="" declare applicationAdminObjectId="" @@ -91,8 +89,6 @@ echo -e "\e[96mEnsuring Workspace Application exists in the \"${tenant}\" Azure source "${DIR}/get_existing_app.sh" # shellcheck disable=SC1091 source "${DIR}/wait_for_new_app_registration.sh" -# shellcheck disable=SC1091 -source "${DIR}/create_or_update_service_principal.sh" # Look for an existing app registration existingApp=$(get_existing_app --name "${appName}") @@ -130,8 +126,6 @@ fi az ad app owner add --id "${workspaceAppId}" --owner-object-id "$currentUserId" --only-show-errors || true az ad app owner add --id "${workspaceAppId}" --owner-object-id "$applicationAdminObjectId" --only-show-errors || true -# Create a Service Principal for the app (without exposing credentials). -create_or_update_service_principal "${workspaceAppId}" 0 >/dev/null # Output the application details echo diff --git a/devops/scripts/create_aad_assets.sh b/devops/scripts/create_aad_assets.sh index 90f72b75f7..e6583f43ae 100755 --- a/devops/scripts/create_aad_assets.sh +++ b/devops/scripts/create_aad_assets.sh @@ -29,15 +29,15 @@ APPLICATION_PERMISSIONS=() APPLICATION_PERMISSIONS+=("Application.ReadWrite.OwnedBy") if [ "${AUTO_WORKSPACE_APP_REGISTRATION:-}" == true ]; then - APPLICATION_PERMISSIONS+=("Application.ReadWrite.All" "Directory.Read.All") + APPLICATION_PERMISSIONS+=("Application.ReadWrite.All") fi if [ "${AUTO_WORKSPACE_GROUP_CREATION:-}" == true ]; then - APPLICATION_PERMISSIONS+=("Group.Create") + APPLICATION_PERMISSIONS+=("Group.Create" "Group.Read.All" "User.ReadBasic.All") fi if [ "${AUTO_GRANT_WORKSPACE_CONSENT:-}" == true ]; then - APPLICATION_PERMISSIONS+=("Application.ReadWrite.All" "DelegatedPermissionGrant.ReadWrite.All") + APPLICATION_PERMISSIONS+=("DelegatedPermissionGrant.ReadWrite.All") fi # Check if the array contains more than 1 item diff --git a/docs/tre-admins/auth.md b/docs/tre-admins/auth.md index 96ed0f64bd..c14c268536 100644 --- a/docs/tre-admins/auth.md +++ b/docs/tre-admins/auth.md @@ -14,8 +14,8 @@ The automation utilises a `make` command, which reads a few environment variable |TRE_ID|This is used to build up the name of the identities| |AAD_TENANT_ID|The tenant id of where your Microsoft Entra ID identities will be placed. This can be different to the tenant where your Azure resources are created.| | LOCATION | Where your Azure assets will be provisioned (eg. westeurope). This is used to add a redirect URI from the Swagger UI to the API Application.| -|AUTO_WORKSPACE_APP_REGISTRATION| Default of `false`. Setting this to true grants the `Application.ReadWrite.All` and `Directory.Read.All` permission to the *Application Admin* identity. This identity is used to manage other Microsoft Entra ID applications that it owns, e.g. Workspaces. If you do not set this, the identity will have `Application.ReadWrite.OwnedBy`. Further information can be found [here](./identities/application_admin.md).| -|AUTO_WORKSPACE_GROUP_CREATION| Set to `false` by default. Setting this to `true` grants the `Group.ReadWrite.All` permission to the *Application Admin* identity. This identity can then create security groups aligned to each applciation role. Microsoft Entra ID licencing implications need to be considered as Group assignment is a premium feature. [You can read mode about Group Assignment here](https://docs.microsoft.com/en-us/azure/architecture/multitenant-identity/app-roles#roles-using-azure-ad-app-roles).| +|AUTO_WORKSPACE_APP_REGISTRATION| Default of `false`. Setting this to true grants the `Application.ReadWrite.All` permission to the *Application Admin* identity. This identity is used to manage other Microsoft Entra ID applications that it owns, e.g. Workspaces. If you do not set this, the identity will have `Application.ReadWrite.OwnedBy`. Further information can be found [here](./identities/application_admin.md).| +|AUTO_WORKSPACE_GROUP_CREATION| Set to `false` by default. Setting this to `true` grants the `Group.Create`, `Group.Read.All` and `User.ReadBasic.All` permission to the *Application Admin* identity. This identity can then create and manage security groups aligned to each applciation role. |AUTO_GRANT_WORKSPACE_CONSENT| Default of `false`. Setting this to `true` will remove the need for users to manually grant consent when creating new workspaces. The identity will be granted `Application.ReadWrite.All` and `DelegatedPermissionGrant.ReadWrite.All` permissions. | ## Create Authentication assets diff --git a/docs/tre-admins/environment-variables.md b/docs/tre-admins/environment-variables.md index 22bfa8d494..a59fab5fec 100644 --- a/docs/tre-admins/environment-variables.md +++ b/docs/tre-admins/environment-variables.md @@ -47,8 +47,8 @@ | `BASTION_SKU` | Optional. The SKU of the Azure Bastion instance. Default value is `Basic`. Allowed values [`Developer`, `Standard`, `Basic`, `Premium`]. See [Azure Bastion SKU feature comparison](https://learn.microsoft.com/en-us/azure/bastion/bastion-overview#sku). | | `CUSTOM_DOMAIN` | Optional. Custom domain name to access the Azure TRE portal. See [Custom domain name](custom-domain.md). | | `ENABLE_CMK_ENCRYPTION` | Optional. Default is `false`, if set to `true` customer-managed key encryption will be enabled for all supported resources. | -| `AUTO_WORKSPACE_APP_REGISTRATION`| Set to `false` by default. Setting this to `true` grants the `Application.ReadWrite.All` and `Directory.Read.All` permission to the *Application Admin* identity. This identity is used to manage other Microsoft Entra ID applications that it owns, e.g. Workspaces. If you do not set this, the identity will have `Application.ReadWrite.OwnedBy` permission. [Further information on Application Admin can be found here](./identities/application_admin.md). | -| `AUTO_WORKSPACE_GROUP_CREATION`| Set to `false` by default. Setting this to `true` grants the `Group.Create` permission to the *Application Admin* identity. This identity can then create security groups aligned to each applciation role. | +| `AUTO_WORKSPACE_APP_REGISTRATION`| Set to `false` by default. Setting this to `true` grants the `Application.ReadWrite.All` permission to the *Application Admin* identity. This identity is used to manage other Microsoft Entra ID applications that it owns, e.g. Workspaces. If you do not set this, the identity will have `Application.ReadWrite.OwnedBy` permission. [Further information on Application Admin can be found here](./identities/application_admin.md). | +| `AUTO_WORKSPACE_GROUP_CREATION`| Set to `false` by default. Setting this to `true` grants the `Group.Create`, `Group.Read.All` and `User.ReadBasic.All` permission to the *Application Admin* identity. This identity can then create security groups aligned to each applciation role. | | `AUTO_GRANT_WORKSPACE_CONSENT`| Default of `false`. Setting this to `true` will remove the need for users to manually grant consent when creating new workspaces. The identity will be granted `Application.ReadWrite.All` and `DelegatedPermissionGrant.ReadWrite.All` permissions. | | `USER_MANAGEMENT_ENABLED` | If set to `true`, TRE Admins will be able to assign and de-assign users to workspaces via the UI (Requires Entra ID groups to be enabled on the workspace and the workspace template version to be 2.2.0 or greater). | | `PRIVATE_AGENT_SUBNET_ID` | Optional. Vnet exception is enabled for the provided runner agent subnet id, enabling access to private resources like TRE key vault. | diff --git a/docs/tre-admins/identities/application_admin.md b/docs/tre-admins/identities/application_admin.md index bf96eba124..3f3f11a35f 100644 --- a/docs/tre-admins/identities/application_admin.md +++ b/docs/tre-admins/identities/application_admin.md @@ -12,8 +12,9 @@ This application does not have any roles defined. | --- | -- | -----| --------- | | Application.ReadWrite.OwnedBy | Application | Yes | This user has `Application.ReadWrite.OwnedBy` as a minimum permission for it to function. If the tenant is managed by a customer administrator, then this user must be added to the **Owners** of every workspace that is created. This will allow TRE to manage the Microsoft Entra ID Application. This will be a manual process for the Tenant Admin. | | Application.ReadWrite.All | Application | Yes | This permission is required to create workspace applications and administer any applications in the tenant. This is needed if the Microsoft Entra ID Administrator has delegated Microsoft Entra ID administrative operations to the TRE. There will be no need for the Tenant Admin to manually create workspace applications in the Tenant. | -| Directory.Read.All | Application | Yes | This permission is required to read User details from Microsoft Entra ID. This is needed if the Microsoft Entra ID Administrator has delegated Microsoft Entra ID administrative operations to the TRE. | | Group.Create | Application | Yes | This permission is required to create and update Microsoft Entra ID groups. This is requried if Microsoft Entra ID groups are to be created automatically by the TRE. | +| Group.Read.All | Application | Yes | This permission is required to read Microsoft Entra ID groups. This is requried if Microsoft Entra ID groups are to be created automatically by the TRE. | +| User.ReadBasic.All | Application | Yes | This permission is required to read basic user information in Microsoft Entra ID. This is requried if Microsoft Entra ID groups are to be created automatically by the TRE. | | DelegatedPermissionGrant.ReadWrite.All | Application | Yes | This permssion is required to remove the need for users to manually grant consent when creating new workspaces. | '*' See the difference between [delegated and application permission](https://docs.microsoft.com/graph/auth/auth-concepts#delegated-and-application-permissions) types. See [Microsoft Graph permissions reference](https://docs.microsoft.com/graph/permissions-reference) for more details. @@ -31,7 +32,7 @@ This user is currently only used from the Porter bundles hosted on the Resource | -------- | ----------- | | `--name` | This is used to put a friendly name to the Application that can be seen in the portal. It is typical to use the name of your TRE instance. | | `--admin-consent` | If you have the appropriate permission to grant admin consent, then pass in this argument. If you do not, you will have to ask an Microsoft Entra ID Admin to consent after you have created the identity. Consent is required for this permission. | -| `--application-permission` | This is a comma seperated list of the permissions that need to be assigned. For exampler `Application.ReadWrite.All,Directory.Read.All,Group.Create` | +| `--application-permission` | This is a comma seperated list of the permissions that need to be assigned. For exampler `Application.ReadWrite.All,Group.Create,Group.Read.All,User.ReadBasic.All` | | `--reset-password` | Optional, default is 0. When run in a headless fashion, 1 is passed in to always reset the password. | ## Environment Variables diff --git a/templates/workspaces/base/porter.yaml b/templates/workspaces/base/porter.yaml index 1239aec1f4..3ede3573a1 100644 --- a/templates/workspaces/base/porter.yaml +++ b/templates/workspaces/base/porter.yaml @@ -1,7 +1,7 @@ --- schemaVersion: 1.0.0 name: tre-workspace-base -version: 2.8.3 +version: 2.8.5 description: "A base Azure TRE workspace" dockerfile: Dockerfile.tmpl registry: azuretre diff --git a/templates/workspaces/base/terraform/aad/aad.tf b/templates/workspaces/base/terraform/aad/aad.tf index f50b1a4260..c2fddeab51 100644 --- a/templates/workspaces/base/terraform/aad/aad.tf +++ b/templates/workspaces/base/terraform/aad/aad.tf @@ -121,8 +121,8 @@ resource "azuread_service_principal_delegated_permission_grant" "ui" { claim_values = ["user_impersonation"] } -resource "azuread_service_principal_password" "workspace" { - service_principal_id = azuread_service_principal.workspace.id +resource "azuread_application_password" "workspace" { + application_id = azuread_application.workspace.id end_date = timeadd( time_rotating.workspace_sp_password.rotation_rfc3339, format("%dh", local.workspace_sp_password_validity_days * 24) @@ -144,7 +144,7 @@ resource "azurerm_key_vault_secret" "client_id" { resource "azurerm_key_vault_secret" "client_secret" { name = "workspace-client-secret" - value = azuread_service_principal_password.workspace.value + value = azuread_application_password.workspace.value key_vault_id = var.key_vault_id tags = var.tre_workspace_tags diff --git a/templates/workspaces/base/terraform/workspace.tf b/templates/workspaces/base/terraform/workspace.tf index d15386113c..053738c11e 100644 --- a/templates/workspaces/base/terraform/workspace.tf +++ b/templates/workspaces/base/terraform/workspace.tf @@ -3,19 +3,10 @@ data "azuread_application" "existing_workspace" { client_id = var.client_id } -data "azuread_service_principal" "existing_workspace" { - count = var.client_id != "" ? 1 : 0 - client_id = var.client_id -} - locals { workspace_app_imports = var.client_id != "" ? { existing = format("/applications/%s", data.azuread_application.existing_workspace[0].object_id) } : {} - - workspace_sp_imports = var.client_id != "" ? { - existing = format("/servicePrincipals/%s", data.azuread_service_principal.existing_workspace[0].object_id) - } : {} } resource "azurerm_resource_group" "ws" { @@ -78,12 +69,6 @@ import { id = each.value } -import { - for_each = local.workspace_sp_imports - to = module.aad.azuread_service_principal.workspace - id = each.value -} - module "airlock" { count = var.enable_airlock ? 1 : 0 source = "./airlock" From 9fd75cfeac5c5eb3939f52350a6af20c1f6fd3d2 Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Wed, 26 Nov 2025 17:39:27 +0000 Subject: [PATCH 05/29] Rotating secret --- .../ui-install-base-workspace.md | 2 +- templates/workspaces/base/porter.yaml | 2 +- .../workspaces/base/terraform/aad/aad.tf | 53 ++++++++++++++++--- .../workspaces/base/terraform/aad/outputs.tf | 2 - 4 files changed, 48 insertions(+), 11 deletions(-) diff --git a/docs/tre-admins/setup-instructions/ui-install-base-workspace.md b/docs/tre-admins/setup-instructions/ui-install-base-workspace.md index c74f919e3f..0ba6a66b37 100644 --- a/docs/tre-admins/setup-instructions/ui-install-base-workspace.md +++ b/docs/tre-admins/setup-instructions/ui-install-base-workspace.md @@ -43,7 +43,7 @@ Workspace can be easily created via AzureTRE UI. Open a browser and navigate to: Workspace is now ready to use. -## Creating an manuall Entra ID Application for the workspace +## Creating a manual Entra ID Application for the workspace If you have not configured automatic application registration creation as explained in the [auth guide](../auth.md), every workspace has a corresponding app registration which if you haven't run `make auth`; can be created using the helper script `./devops/scripts/aad/create_workspace_application.sh`. For example: diff --git a/templates/workspaces/base/porter.yaml b/templates/workspaces/base/porter.yaml index 3ede3573a1..5ff1df9beb 100644 --- a/templates/workspaces/base/porter.yaml +++ b/templates/workspaces/base/porter.yaml @@ -1,7 +1,7 @@ --- schemaVersion: 1.0.0 name: tre-workspace-base -version: 2.8.5 +version: 2.8.8 description: "A base Azure TRE workspace" dockerfile: Dockerfile.tmpl registry: azuretre diff --git a/templates/workspaces/base/terraform/aad/aad.tf b/templates/workspaces/base/terraform/aad/aad.tf index c2fddeab51..0719c4a19b 100644 --- a/templates/workspaces/base/terraform/aad/aad.tf +++ b/templates/workspaces/base/terraform/aad/aad.tf @@ -10,14 +10,20 @@ resource "random_uuid" "app_role_workspace_researcher_id" {} resource "random_uuid" "app_role_workspace_airlock_manager_id" {} locals { - workspace_sp_password_rotation_days = 30 - workspace_sp_password_validity_days = 180 + workspace_sp_password_rotation_days = 30 + workspace_sp_password_validity_days = 180 + workspace_sp_password_rotation_offset_days = 15 } -resource "time_rotating" "workspace_sp_password" { +resource "time_rotating" "workspace_sp_password_primary" { rotation_days = local.workspace_sp_password_rotation_days } +resource "time_rotating" "workspace_sp_password_secondary" { + rotation_days = local.workspace_sp_password_rotation_days + rfc3339 = timeadd(time_rotating.workspace_sp_password_primary.rotation_rfc3339, "${local.workspace_sp_password_rotation_offset_days * 24}h") +} + resource "azuread_application" "workspace" { display_name = var.workspace_resource_name_suffix identifier_uris = ["api://${var.workspace_resource_name_suffix}"] @@ -121,18 +127,51 @@ resource "azuread_service_principal_delegated_permission_grant" "ui" { claim_values = ["user_impersonation"] } -resource "azuread_application_password" "workspace" { +resource "azuread_application_password" "workspace_primary" { + application_id = azuread_application.workspace.id + display_name = "Primary Password" + end_date = timeadd( + time_rotating.workspace_sp_password_primary.rotation_rfc3339, + format("%dh", local.workspace_sp_password_validity_days * 24) + ) + + rotate_when_changed = { + rotation = time_rotating.workspace_sp_password_primary.rotation_rfc3339 + } + + lifecycle { + create_before_destroy = true + } +} + +resource "azuread_application_password" "workspace_secondary" { application_id = azuread_application.workspace.id + display_name = "Secondary Password" end_date = timeadd( - time_rotating.workspace_sp_password.rotation_rfc3339, + time_rotating.workspace_sp_password_secondary.rotation_rfc3339, format("%dh", local.workspace_sp_password_validity_days * 24) ) rotate_when_changed = { - rotation = time_rotating.workspace_sp_password.rotation_rfc3339 + rotation = time_rotating.workspace_sp_password_secondary.rotation_rfc3339 + } + + lifecycle { + create_before_destroy = true } } +locals { + # Use timecmp function to compare timestamps + # timecmp returns 1 if first timestamp is later, -1 if earlier, 0 if equal + primary_is_newer = timecmp(time_rotating.workspace_sp_password_primary.rotation_rfc3339, time_rotating.workspace_sp_password_secondary.rotation_rfc3339) > 0 + + # Use the newer password as the current password + current_password = local.primary_is_newer ? azuread_application_password.workspace_primary.value : azuread_application_password.workspace_secondary.value + # Keep the older password as backup (not stored in Key Vault) + backup_password = local.primary_is_newer ? azuread_application_password.workspace_secondary.value : azuread_application_password.workspace_primary.value +} + resource "azurerm_key_vault_secret" "client_id" { name = "workspace-client-id" value = azuread_application.workspace.client_id @@ -144,7 +183,7 @@ resource "azurerm_key_vault_secret" "client_id" { resource "azurerm_key_vault_secret" "client_secret" { name = "workspace-client-secret" - value = azuread_application_password.workspace.value + value = local.current_password key_vault_id = var.key_vault_id tags = var.tre_workspace_tags diff --git a/templates/workspaces/base/terraform/aad/outputs.tf b/templates/workspaces/base/terraform/aad/outputs.tf index 4d00df5349..7770d9cf74 100644 --- a/templates/workspaces/base/terraform/aad/outputs.tf +++ b/templates/workspaces/base/terraform/aad/outputs.tf @@ -33,5 +33,3 @@ output "workspace_researchers_group_id" { output "workspace_airlock_managers_group_id" { value = var.create_aad_groups ? azuread_group.workspace_airlock_managers[0].object_id : "" } - - From 1b9c09098c494003e3463f41ba131686d7cc79f8 Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Thu, 27 Nov 2025 10:43:16 +0000 Subject: [PATCH 06/29] Update docs/tre-admins/auth.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- docs/tre-admins/auth.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/tre-admins/auth.md b/docs/tre-admins/auth.md index c14c268536..d170719aad 100644 --- a/docs/tre-admins/auth.md +++ b/docs/tre-admins/auth.md @@ -15,7 +15,7 @@ The automation utilises a `make` command, which reads a few environment variable |AAD_TENANT_ID|The tenant id of where your Microsoft Entra ID identities will be placed. This can be different to the tenant where your Azure resources are created.| | LOCATION | Where your Azure assets will be provisioned (eg. westeurope). This is used to add a redirect URI from the Swagger UI to the API Application.| |AUTO_WORKSPACE_APP_REGISTRATION| Default of `false`. Setting this to true grants the `Application.ReadWrite.All` permission to the *Application Admin* identity. This identity is used to manage other Microsoft Entra ID applications that it owns, e.g. Workspaces. If you do not set this, the identity will have `Application.ReadWrite.OwnedBy`. Further information can be found [here](./identities/application_admin.md).| -|AUTO_WORKSPACE_GROUP_CREATION| Set to `false` by default. Setting this to `true` grants the `Group.Create`, `Group.Read.All` and `User.ReadBasic.All` permission to the *Application Admin* identity. This identity can then create and manage security groups aligned to each applciation role. +|AUTO_WORKSPACE_GROUP_CREATION| Set to `false` by default. Setting this to `true` grants the `Group.Create`, `Group.Read.All` and `User.ReadBasic.All` permission to the *Application Admin* identity. This identity can then create and manage security groups aligned to each application role. |AUTO_GRANT_WORKSPACE_CONSENT| Default of `false`. Setting this to `true` will remove the need for users to manually grant consent when creating new workspaces. The identity will be granted `Application.ReadWrite.All` and `DelegatedPermissionGrant.ReadWrite.All` permissions. | ## Create Authentication assets From ccf5aeec54bfd9a568465a5a3a4afb17715a80bf Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Thu, 27 Nov 2025 10:44:06 +0000 Subject: [PATCH 07/29] Update docs/tre-admins/identities/application_admin.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- docs/tre-admins/identities/application_admin.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/tre-admins/identities/application_admin.md b/docs/tre-admins/identities/application_admin.md index 3f3f11a35f..236a43d01b 100644 --- a/docs/tre-admins/identities/application_admin.md +++ b/docs/tre-admins/identities/application_admin.md @@ -12,9 +12,9 @@ This application does not have any roles defined. | --- | -- | -----| --------- | | Application.ReadWrite.OwnedBy | Application | Yes | This user has `Application.ReadWrite.OwnedBy` as a minimum permission for it to function. If the tenant is managed by a customer administrator, then this user must be added to the **Owners** of every workspace that is created. This will allow TRE to manage the Microsoft Entra ID Application. This will be a manual process for the Tenant Admin. | | Application.ReadWrite.All | Application | Yes | This permission is required to create workspace applications and administer any applications in the tenant. This is needed if the Microsoft Entra ID Administrator has delegated Microsoft Entra ID administrative operations to the TRE. There will be no need for the Tenant Admin to manually create workspace applications in the Tenant. | -| Group.Create | Application | Yes | This permission is required to create and update Microsoft Entra ID groups. This is requried if Microsoft Entra ID groups are to be created automatically by the TRE. | -| Group.Read.All | Application | Yes | This permission is required to read Microsoft Entra ID groups. This is requried if Microsoft Entra ID groups are to be created automatically by the TRE. | -| User.ReadBasic.All | Application | Yes | This permission is required to read basic user information in Microsoft Entra ID. This is requried if Microsoft Entra ID groups are to be created automatically by the TRE. | +| Group.Create | Application | Yes | This permission is required to create and update Microsoft Entra ID groups. This is required if Microsoft Entra ID groups are to be created automatically by the TRE. | +| Group.Read.All | Application | Yes | This permission is required to read Microsoft Entra ID groups. This is required if Microsoft Entra ID groups are to be created automatically by the TRE. | +| User.ReadBasic.All | Application | Yes | This permission is required to read basic user information in Microsoft Entra ID. This is required if Microsoft Entra ID groups are to be created automatically by the TRE. | | DelegatedPermissionGrant.ReadWrite.All | Application | Yes | This permssion is required to remove the need for users to manually grant consent when creating new workspaces. | '*' See the difference between [delegated and application permission](https://docs.microsoft.com/graph/auth/auth-concepts#delegated-and-application-permissions) types. See [Microsoft Graph permissions reference](https://docs.microsoft.com/graph/permissions-reference) for more details. From f93845e09b239c62ef9f2204ee703c4eeaeb7a4d Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Thu, 27 Nov 2025 10:44:18 +0000 Subject: [PATCH 08/29] Update docs/tre-admins/environment-variables.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- docs/tre-admins/environment-variables.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/tre-admins/environment-variables.md b/docs/tre-admins/environment-variables.md index a59fab5fec..f7ac04f7f1 100644 --- a/docs/tre-admins/environment-variables.md +++ b/docs/tre-admins/environment-variables.md @@ -48,7 +48,7 @@ | `CUSTOM_DOMAIN` | Optional. Custom domain name to access the Azure TRE portal. See [Custom domain name](custom-domain.md). | | `ENABLE_CMK_ENCRYPTION` | Optional. Default is `false`, if set to `true` customer-managed key encryption will be enabled for all supported resources. | | `AUTO_WORKSPACE_APP_REGISTRATION`| Set to `false` by default. Setting this to `true` grants the `Application.ReadWrite.All` permission to the *Application Admin* identity. This identity is used to manage other Microsoft Entra ID applications that it owns, e.g. Workspaces. If you do not set this, the identity will have `Application.ReadWrite.OwnedBy` permission. [Further information on Application Admin can be found here](./identities/application_admin.md). | -| `AUTO_WORKSPACE_GROUP_CREATION`| Set to `false` by default. Setting this to `true` grants the `Group.Create`, `Group.Read.All` and `User.ReadBasic.All` permission to the *Application Admin* identity. This identity can then create security groups aligned to each applciation role. | +| `AUTO_WORKSPACE_GROUP_CREATION`| Set to `false` by default. Setting this to `true` grants the `Group.Create`, `Group.Read.All` and `User.ReadBasic.All` permission to the *Application Admin* identity. This identity can then create security groups aligned to each application role. | | `AUTO_GRANT_WORKSPACE_CONSENT`| Default of `false`. Setting this to `true` will remove the need for users to manually grant consent when creating new workspaces. The identity will be granted `Application.ReadWrite.All` and `DelegatedPermissionGrant.ReadWrite.All` permissions. | | `USER_MANAGEMENT_ENABLED` | If set to `true`, TRE Admins will be able to assign and de-assign users to workspaces via the UI (Requires Entra ID groups to be enabled on the workspace and the workspace template version to be 2.2.0 or greater). | | `PRIVATE_AGENT_SUBNET_ID` | Optional. Vnet exception is enabled for the provided runner agent subnet id, enabling access to private resources like TRE key vault. | From b79940fe99001342f420c3900d57ce5609bd9448 Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Thu, 27 Nov 2025 10:45:01 +0000 Subject: [PATCH 09/29] Update docs/tre-admins/identities/application_admin.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- docs/tre-admins/identities/application_admin.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/tre-admins/identities/application_admin.md b/docs/tre-admins/identities/application_admin.md index 236a43d01b..f83ea90ba6 100644 --- a/docs/tre-admins/identities/application_admin.md +++ b/docs/tre-admins/identities/application_admin.md @@ -32,7 +32,7 @@ This user is currently only used from the Porter bundles hosted on the Resource | -------- | ----------- | | `--name` | This is used to put a friendly name to the Application that can be seen in the portal. It is typical to use the name of your TRE instance. | | `--admin-consent` | If you have the appropriate permission to grant admin consent, then pass in this argument. If you do not, you will have to ask an Microsoft Entra ID Admin to consent after you have created the identity. Consent is required for this permission. | -| `--application-permission` | This is a comma seperated list of the permissions that need to be assigned. For exampler `Application.ReadWrite.All,Group.Create,Group.Read.All,User.ReadBasic.All` | +| `--application-permission` | This is a comma separated list of the permissions that need to be assigned. For exampler `Application.ReadWrite.All,Group.Create,Group.Read.All,User.ReadBasic.All` | | `--reset-password` | Optional, default is 0. When run in a headless fashion, 1 is passed in to always reset the password. | ## Environment Variables From 173ebd1b0f163bec56785f053e2afd0ca88cc775 Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Thu, 27 Nov 2025 10:49:44 +0000 Subject: [PATCH 10/29] Update templates/workspaces/base/terraform/aad/aad.tf Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- templates/workspaces/base/terraform/aad/aad.tf | 2 -- 1 file changed, 2 deletions(-) diff --git a/templates/workspaces/base/terraform/aad/aad.tf b/templates/workspaces/base/terraform/aad/aad.tf index 0719c4a19b..a2265182b2 100644 --- a/templates/workspaces/base/terraform/aad/aad.tf +++ b/templates/workspaces/base/terraform/aad/aad.tf @@ -168,8 +168,6 @@ locals { # Use the newer password as the current password current_password = local.primary_is_newer ? azuread_application_password.workspace_primary.value : azuread_application_password.workspace_secondary.value - # Keep the older password as backup (not stored in Key Vault) - backup_password = local.primary_is_newer ? azuread_application_password.workspace_secondary.value : azuread_application_password.workspace_primary.value } resource "azurerm_key_vault_secret" "client_id" { From 430453a8cae3453443f15a042582957cb87aeecf Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Thu, 27 Nov 2025 11:03:23 +0000 Subject: [PATCH 11/29] Update PR review comments. --- CHANGELOG.md | 1 + api_app/_version.py | 2 +- api_app/models/schemas/workspace.py | 1 - core/version.txt | 2 +- docs/tre-admins/identities/application_admin.md | 4 ++-- docs/tre-templates/workspaces/base.md | 6 ++++-- 6 files changed, 9 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index edcbaa8586..de15ad836f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ ENHANCEMENTS: * API: Replace HTTP_422_UNPROCESSABLE_ENTITY response with HTTP_422_UNPROCESSABLE_CONTENT as per RFC 9110 ([#4742](https://github.com/microsoft/AzureTRE/issues/4742)) * Change Group.ReadWrite.All permission to Group.Create for AUTO_WORKSPACE_GROUP_CREATION ([#4772](https://github.com/microsoft/AzureTRE/issues/4772)) * Make workspace shared storage quota updateable ([#4314](https://github.com/microsoft/AzureTRE/issues/4314)) +* Automate workspace application client secret creation and rotation, removing the need to supply a secret during workspace creation ([#4775](https://github.com/microsoft/AzureTRE/pull/4775)) BUG FIXES: * Fix circular dependancy in base workspace. ([#4756](https://github.com/microsoft/AzureTRE/pull/4756)) diff --git a/api_app/_version.py b/api_app/_version.py index a5e1b0f66d..7c4a9591e1 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.25.5" +__version__ = "0.26.0" diff --git a/api_app/models/schemas/workspace.py b/api_app/models/schemas/workspace.py index 94c6bf861e..1a2a27f6a6 100644 --- a/api_app/models/schemas/workspace.py +++ b/api_app/models/schemas/workspace.py @@ -84,7 +84,6 @@ class Config: "description": "workspace description", "auth_type": "Manual", "client_id": "", - "client_secret": "", "address_space_size": "small" } } diff --git a/core/version.txt b/core/version.txt index bac41ffe7b..fd86b3ee91 100644 --- a/core/version.txt +++ b/core/version.txt @@ -1 +1 @@ -__version__ = "0.16.11" +__version__ = "0.17.0" diff --git a/docs/tre-admins/identities/application_admin.md b/docs/tre-admins/identities/application_admin.md index f83ea90ba6..cfed6bf487 100644 --- a/docs/tre-admins/identities/application_admin.md +++ b/docs/tre-admins/identities/application_admin.md @@ -15,7 +15,7 @@ This application does not have any roles defined. | Group.Create | Application | Yes | This permission is required to create and update Microsoft Entra ID groups. This is required if Microsoft Entra ID groups are to be created automatically by the TRE. | | Group.Read.All | Application | Yes | This permission is required to read Microsoft Entra ID groups. This is required if Microsoft Entra ID groups are to be created automatically by the TRE. | | User.ReadBasic.All | Application | Yes | This permission is required to read basic user information in Microsoft Entra ID. This is required if Microsoft Entra ID groups are to be created automatically by the TRE. | -| DelegatedPermissionGrant.ReadWrite.All | Application | Yes | This permssion is required to remove the need for users to manually grant consent when creating new workspaces. | +| DelegatedPermissionGrant.ReadWrite.All | Application | Yes | This permission is required to remove the need for users to manually grant consent when creating new workspaces. | '*' See the difference between [delegated and application permission](https://docs.microsoft.com/graph/auth/auth-concepts#delegated-and-application-permissions) types. See [Microsoft Graph permissions reference](https://docs.microsoft.com/graph/permissions-reference) for more details. @@ -32,7 +32,7 @@ This user is currently only used from the Porter bundles hosted on the Resource | -------- | ----------- | | `--name` | This is used to put a friendly name to the Application that can be seen in the portal. It is typical to use the name of your TRE instance. | | `--admin-consent` | If you have the appropriate permission to grant admin consent, then pass in this argument. If you do not, you will have to ask an Microsoft Entra ID Admin to consent after you have created the identity. Consent is required for this permission. | -| `--application-permission` | This is a comma separated list of the permissions that need to be assigned. For exampler `Application.ReadWrite.All,Group.Create,Group.Read.All,User.ReadBasic.All` | +| `--application-permission` | This is a comma separated list of the permissions that need to be assigned. For example `Application.ReadWrite.All,Group.Create,Group.Read.All,User.ReadBasic.All` | | `--reset-password` | Optional, default is 0. When run in a headless fashion, 1 is passed in to always reset the password. | ## Environment Variables diff --git a/docs/tre-templates/workspaces/base.md b/docs/tre-templates/workspaces/base.md index fadea7a60b..489f5cedc5 100644 --- a/docs/tre-templates/workspaces/base.md +++ b/docs/tre-templates/workspaces/base.md @@ -19,8 +19,7 @@ When deploying a workspace the following properties need to be configured. | Property | Options | Description | | -------- | ------- | ----------- | -| `client_id` | Valid client ID of the Workspace App Registration. | The OpenID client ID which should be submitted to the OpenID service when necessary. This value is typically provided to you by the OpenID service when OpenID credentials are generated for your application. | -| `client_secret` | Valid client secret. | +| `client_id` | Valid client ID of the Workspace App Registration. | Required only when `auth_type` is set to `Manual`. Leave empty (default) to allow the TRE to create and manage the workspace application automatically. | ## Azure Trusted Services *Azure Trusted Services* are allowed to connect to both the key vault and storage account provsioned within the workspace. If this is undesirable additonal resources without this setting configured can be deployed. @@ -29,3 +28,6 @@ Further details around which Azure services are allowed to connect can be found - Key Vault: - Azure Storage: + +## Client secret rotation +When `auth_type` is set to `Automatic`, the base workspace bundle provisions the Microsoft Entra application and manages its secrets for you. Two secrets (primary and secondary) are created, rotated every 30 days, and offset by 15 days so that one secret is always valid during rollover. Each credential remains valid for 180 days, and the currently active secret is synced to the workspace key vault as `workspace-client-secret` (with the client ID stored as `workspace-client-id`). Re-running the workspace deployment (for example as part of an upgrade) automatically refreshes the secret whenever the rotation schedule requires it—no manual input is needed. From 994d1f9d31a7e9a2a5d9ec3cc7a6c1023f70809a Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Thu, 27 Nov 2025 11:35:59 +0000 Subject: [PATCH 12/29] Update after linting feedback. --- CHANGELOG.md | 1 + api_app/services/authentication.py | 2 +- .../ui-install-base-workspace.md | 2 +- docs/tre-templates/workspaces/base.md | 5 ++- templates/workspaces/base/.env.sample | 5 --- templates/workspaces/base/parameters.json | 30 --------------- templates/workspaces/base/porter.yaml | 37 +------------------ .../workspaces/base/terraform/variables.tf | 27 -------------- 8 files changed, 8 insertions(+), 101 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index de15ad836f..65be0ee5d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ **BREAKING CHANGES** * Fix missing arguments for airlock manager requests - change in API contract ([#4544](https://github.com/microsoft/AzureTRE/issues/4544)) * Clarify cost label time period and aggregation scope in UI tooltips ([#4607](https://github.com/microsoft/AzureTRE/pull/4607)) +* Base workspace bundle 4.0.0 removes the legacy manual identity passthrough parameters (scope_id, sp_id, app_role_id_*)—workspaces now rely on generated outputs instead. ([#4775](https://github.com/microsoft/AzureTRE/pull/4775)) ENHANCEMENTS: * Upgrade Guacamole to v1.6.0 with Java 17 and other security updates ([#4754](https://github.com/microsoft/AzureTRE/pull/4754)) diff --git a/api_app/services/authentication.py b/api_app/services/authentication.py index d82bd7892f..18efa223cf 100644 --- a/api_app/services/authentication.py +++ b/api_app/services/authentication.py @@ -4,7 +4,7 @@ from models.schemas.workspace import AuthProvider from resources import strings from services.aad_authentication import AzureADAuthorization -from services.access_service import AccessService, AuthConfigValidationError +from services.access_service import AccessService def get_access_service(provider: str = AuthProvider.AAD) -> AccessService: diff --git a/docs/tre-admins/setup-instructions/ui-install-base-workspace.md b/docs/tre-admins/setup-instructions/ui-install-base-workspace.md index 0ba6a66b37..c68259797b 100644 --- a/docs/tre-admins/setup-instructions/ui-install-base-workspace.md +++ b/docs/tre-admins/setup-instructions/ui-install-base-workspace.md @@ -26,7 +26,7 @@ Workspace can be easily created via AzureTRE UI. Open a browser and navigate to: - General information such as name and description - [Optional] Update values for Shared Storage Quota, App Service Plan (SKU) and Address space if needed - - Workspace Authentication Type - this determines whether you'd like TRE to create an app registration for the workspace automatically, or whether you with to provide an existing one that you've created manually. To read about how to create it manually read the [Creating an Application Client for base workspace](#creating-an-application-client-for-base-workspace) section below. + - Workspace Authentication Type - this determines whether you'd like TRE to create an app registration for the workspace automatically, or whether you wish to provide an existing one that you've created manually. To read about how to create it manually read the [Creating a manual Entra ID application for the workspace](#creating-a-manual-entra-id-application-for-the-workspace) section below. 1. After filling the details press submit. diff --git a/docs/tre-templates/workspaces/base.md b/docs/tre-templates/workspaces/base.md index 489f5cedc5..11cdb55dae 100644 --- a/docs/tre-templates/workspaces/base.md +++ b/docs/tre-templates/workspaces/base.md @@ -30,4 +30,7 @@ Further details around which Azure services are allowed to connect can be found - Azure Storage: ## Client secret rotation -When `auth_type` is set to `Automatic`, the base workspace bundle provisions the Microsoft Entra application and manages its secrets for you. Two secrets (primary and secondary) are created, rotated every 30 days, and offset by 15 days so that one secret is always valid during rollover. Each credential remains valid for 180 days, and the currently active secret is synced to the workspace key vault as `workspace-client-secret` (with the client ID stored as `workspace-client-id`). Re-running the workspace deployment (for example as part of an upgrade) automatically refreshes the secret whenever the rotation schedule requires it—no manual input is needed. +Two secrets (primary and secondary) are created, rotated every 30 days, and offset by 15 days so that one secret is always valid during rollover. +Each credential remains valid for 180 days, and the currently active secret is synced to the workspace key vault as `workspace-client-secret` (with the client ID stored as `workspace-client-id`). + +Re-running the workspace deployment (for example as part of an upgrade) automatically refreshes the secret whenever the rotation schedule requires it—no manual input is needed. diff --git a/templates/workspaces/base/.env.sample b/templates/workspaces/base/.env.sample index 40de3a637f..aba393f753 100644 --- a/templates/workspaces/base/.env.sample +++ b/templates/workspaces/base/.env.sample @@ -19,11 +19,6 @@ WORKSPACE_OWNER_OBJECT_ID="__CHANGE_ME__" # Used by Porter, aka TRE_RESOURCE_ID ID="MadeUp123" -SP_ID="" -SCOPE_ID="api://ws_0001" -APP_ROLE_ID_WORKSPACE_OWNER="" -APP_ROLE_ID_WORKSPACE_RESEARCHER="" -APP_ROLE_ID_WORKSPACE_AIRLOCK_MANAGER="" # Complex types are base 64 encoded by resource processor ADDRESS_SPACES="WyIxMC4xLjEwLjAvMjQiXQ==" SHARED_STORAGE_QUOTA=50 diff --git a/templates/workspaces/base/parameters.json b/templates/workspaces/base/parameters.json index d4f408174b..3b24ef4ee4 100755 --- a/templates/workspaces/base/parameters.json +++ b/templates/workspaces/base/parameters.json @@ -82,42 +82,12 @@ "env": "CLIENT_SECRET" } }, - { - "name": "scope_id", - "source": { - "env": "SCOPE_ID" - } - }, { "name": "workspace_owner_object_id", "source": { "env": "WORKSPACE_OWNER_OBJECT_ID" } }, - { - "name": "sp_id", - "source": { - "env": "SP_ID" - } - }, - { - "name": "app_role_id_workspace_owner", - "source": { - "env": "APP_ROLE_ID_WORKSPACE_OWNER" - } - }, - { - "name": "app_role_id_workspace_researcher", - "source": { - "env": "APP_ROLE_ID_WORKSPACE_RESEARCHER" - } - }, - { - "name": "app_role_id_workspace_airlock_manager", - "source": { - "env": "APP_ROLE_ID_WORKSPACE_AIRLOCK_MANAGER" - } - }, { "name": "aad_redirect_uris", "source": { diff --git a/templates/workspaces/base/porter.yaml b/templates/workspaces/base/porter.yaml index 54481f5c65..1ffa55d2f6 100644 --- a/templates/workspaces/base/porter.yaml +++ b/templates/workspaces/base/porter.yaml @@ -1,7 +1,7 @@ --- schemaVersion: 1.0.0 name: tre-workspace-base -version: 3.0.0 +version: 4.0.0 description: "A base Azure TRE workspace" dockerfile: Dockerfile.tmpl registry: azuretre @@ -84,26 +84,6 @@ parameters: type: string default: "" description: "The client ID of the UI" - - name: scope_id - type: string - default: "" - description: "The Service Principal Name or identifierUri (e.g. api://GUID" - - name: sp_id - type: string - default: "" - description: "The Service Principal in the Identity provider to be able to get claims" - - name: app_role_id_workspace_owner - type: string - default: "" - description: "The id of the application role WorkspaceOwner in the identity provider" - - name: app_role_id_workspace_researcher - type: string - default: "" - description: "The id of the application role WorkspaceResearcher in the identity provider" - - name: app_role_id_workspace_airlock_manager - type: string - default: "" - description: "The id of the application role AirlockManager in the identity provider" - name: aad_redirect_uris type: string description: "List of redirect URIs in {name:value} format" @@ -241,11 +221,6 @@ install: workspace_owner_object_id: ${ bundle.parameters.workspace_owner_object_id } client_id: ${ bundle.parameters.client_id } ui_client_id: ${ bundle.parameters.ui_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 } - app_role_id_workspace_airlock_manager: ${ bundle.parameters.app_role_id_workspace_airlock_manager } aad_redirect_uris_b64: ${ bundle.parameters.aad_redirect_uris } app_service_plan_sku: ${ bundle.parameters.app_service_plan_sku } enable_airlock: ${ bundle.parameters.enable_airlock } @@ -299,11 +274,6 @@ upgrade: workspace_owner_object_id: ${ bundle.parameters.workspace_owner_object_id } client_id: ${ bundle.parameters.client_id } ui_client_id: ${ bundle.parameters.ui_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 } - app_role_id_workspace_airlock_manager: ${ bundle.parameters.app_role_id_workspace_airlock_manager } aad_redirect_uris_b64: ${ bundle.parameters.aad_redirect_uris } app_service_plan_sku: ${ bundle.parameters.app_service_plan_sku } enable_airlock: ${ bundle.parameters.enable_airlock } @@ -356,12 +326,7 @@ uninstall: auth_tenant_id: ${ bundle.credentials.auth_tenant_id } workspace_owner_object_id: ${ bundle.parameters.workspace_owner_object_id } client_id: ${ bundle.parameters.client_id } - scope_id: ${ bundle.parameters.scope_id } ui_client_id: ${ bundle.parameters.ui_client_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 } - app_role_id_workspace_airlock_manager: ${ bundle.parameters.app_role_id_workspace_airlock_manager } aad_redirect_uris_b64: ${ bundle.parameters.aad_redirect_uris } app_service_plan_sku: ${ bundle.parameters.app_service_plan_sku } enable_airlock: ${ bundle.parameters.enable_airlock } diff --git a/templates/workspaces/base/terraform/variables.tf b/templates/workspaces/base/terraform/variables.tf index aefecb90b7..4689042976 100644 --- a/templates/workspaces/base/terraform/variables.tf +++ b/templates/workspaces/base/terraform/variables.tf @@ -87,23 +87,6 @@ variable "enable_backup" { description = "Enable backups for the workspace" } -# These variables are only passed in if you are not registering an AAD -# application as they need passing back out -variable "app_role_id_workspace_owner" { - type = string - default = "" - description = "The id of the application role WorkspaceOwner in the identity provider, this is passed in so that we may return it as an output." -} -variable "app_role_id_workspace_researcher" { - type = string - default = "" - description = "The id of the application role WorkspaceResearcher in the identity provider, this is passed in so that we may return it as an output." -} -variable "app_role_id_workspace_airlock_manager" { - type = string - default = "" - description = "The id of the application role AirlockManager in the identity provider, this is passed in so that we may return it as an output." -} variable "client_id" { type = string default = "" @@ -112,16 +95,6 @@ variable "client_id" { variable "ui_client_id" { type = string } -variable "sp_id" { - type = string - 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 = "" From a86b286240e27adb4360ea73404963f567a5908d Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Thu, 27 Nov 2025 15:39:29 +0000 Subject: [PATCH 13/29] Remove unused auth variables. --- CHANGELOG.md | 9 ++- api_app/db/repositories/workspaces.py | 5 -- .../test_workpaces_repository.py | 14 ---- config_schema.json | 2 +- .../airlock-import-review/.env.sample | 13 +--- .../airlock-import-review/parameters.json | 42 ---------- .../airlock-import-review/porter.yaml | 76 +------------------ .../template_schema.json | 8 -- templates/workspaces/base/.env.sample | 8 +- templates/workspaces/base/parameters.json | 6 -- templates/workspaces/base/porter.yaml | 2 +- .../workspaces/base/update_redirect_urls.sh | 12 +-- templates/workspaces/unrestricted/.env.sample | 13 +--- .../workspaces/unrestricted/parameters.json | 42 ---------- templates/workspaces/unrestricted/porter.yaml | 76 +------------------ .../unrestricted/template_schema.json | 8 -- 16 files changed, 17 insertions(+), 319 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 65be0ee5d0..c599a42a51 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,15 +2,18 @@ ## 0.27.0 (Unreleased) **BREAKING CHANGES** * Fix missing arguments for airlock manager requests - change in API contract ([#4544](https://github.com/microsoft/AzureTRE/issues/4544)) -* Clarify cost label time period and aggregation scope in UI tooltips ([#4607](https://github.com/microsoft/AzureTRE/pull/4607)) -* Base workspace bundle 4.0.0 removes the legacy manual identity passthrough parameters (scope_id, sp_id, app_role_id_*)—workspaces now rely on generated outputs instead. ([#4775](https://github.com/microsoft/AzureTRE/pull/4775)) + +* Base workspace bundle 4.0.0 (major upgrade from 2.8.0) now creates and rotates the workspace Microsoft Entra application secret automatically and removes the manual identity passthrough parameters (`client_secret`, `register_aad_application`, `scope_id`, `sp_id`, `app_role_id_*`). + - Existing workspaces that relied on manually managed secrets continue to operate without interruption; upgrade them at your own pace. + - The automation admin (`APPLICATION_ADMIN_CLIENT_ID`) no longer needs the `Directory.Read.All` Microsoft Graph permission; keep the documented `Application.ReadWrite.*`, `Group.*`, `User.ReadBasic.All`, and `DelegatedPermissionGrant.ReadWrite.All` permissions in place. ([#4775](https://github.com/microsoft/AzureTRE/pull/4775)) + ENHANCEMENTS: * Upgrade Guacamole to v1.6.0 with Java 17 and other security updates ([#4754](https://github.com/microsoft/AzureTRE/pull/4754)) * API: Replace HTTP_422_UNPROCESSABLE_ENTITY response with HTTP_422_UNPROCESSABLE_CONTENT as per RFC 9110 ([#4742](https://github.com/microsoft/AzureTRE/issues/4742)) * Change Group.ReadWrite.All permission to Group.Create for AUTO_WORKSPACE_GROUP_CREATION ([#4772](https://github.com/microsoft/AzureTRE/issues/4772)) * Make workspace shared storage quota updateable ([#4314](https://github.com/microsoft/AzureTRE/issues/4314)) -* Automate workspace application client secret creation and rotation, removing the need to supply a secret during workspace creation ([#4775](https://github.com/microsoft/AzureTRE/pull/4775)) +* Clarify cost label time period and aggregation scope in UI tooltips ([#4607](https://github.com/microsoft/AzureTRE/pull/4607)) BUG FIXES: * Fix circular dependancy in base workspace. ([#4756](https://github.com/microsoft/AzureTRE/pull/4756)) diff --git a/api_app/db/repositories/workspaces.py b/api_app/db/repositories/workspaces.py index 3b8917ded6..008bb99c04 100644 --- a/api_app/db/repositories/workspaces.py +++ b/api_app/db/repositories/workspaces.py @@ -104,7 +104,6 @@ async def create_workspace_item(self, workspace_input: WorkspaceInCreate, worksp address_space_param = {"address_space": intial_address_space} address_spaces_param = {"address_spaces": [intial_address_space]} - auto_app_registration_param = {"register_aad_application": self.automatically_create_application_registration(workspace_input.properties)} workspace_owner_param = {"workspace_owner_object_id": self.get_workspace_owner(workspace_input.properties, workspace_owner_object_id)} # we don't want something in the input to overwrite the system parameters, @@ -112,7 +111,6 @@ async def create_workspace_item(self, workspace_input: WorkspaceInCreate, worksp resource_spec_parameters = {**workspace_input.properties, **address_space_param, **address_spaces_param, - **auto_app_registration_param, **workspace_owner_param, **self.get_workspace_spec_params(full_workspace_id)} @@ -133,9 +131,6 @@ def get_workspace_owner(self, workspace_properties: dict, workspace_owner_object user_defined_workspace_owner_object_id = workspace_properties.get("workspace_owner_object_id") return workspace_owner_object_id if user_defined_workspace_owner_object_id is None else user_defined_workspace_owner_object_id - def automatically_create_application_registration(self, workspace_properties: dict) -> bool: - return True if ("auth_type" in workspace_properties and workspace_properties["auth_type"] == "Automatic") else False - async def get_address_space_based_on_size(self, workspace_properties: dict): # Default the address space to 'small' if not supplied. address_space_size = workspace_properties.get("address_space_size", "small").lower() diff --git a/api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py b/api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py index 8a29d47506..da62c4d40f 100644 --- a/api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py +++ b/api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py @@ -274,20 +274,6 @@ async def test_create_workspace_item_raises_value_error_if_template_is_invalid(m with pytest.raises(ValueError): await workspace_repo.create_workspace_item(workspace_input, "test_object_id", ["test_role"]) - - -def test_automatically_create_application_registration_returns_true(workspace_repo): - dictToTest = {"auth_type": "Automatic"} - - assert workspace_repo.automatically_create_application_registration(dictToTest) is True - - -def test_automatically_create_application_registration_returns_false(workspace_repo): - dictToTest = {"client_id": "12345"} - - assert workspace_repo.automatically_create_application_registration(dictToTest) is False - - def test_workspace_owner_is_set_if_not_present_in_workspace_properties(workspace_repo): dictToTest = {} expected_object_id = "Expected" diff --git a/config_schema.json b/config_schema.json index 99ca3c9171..38dd054b60 100644 --- a/config_schema.json +++ b/config_schema.json @@ -164,7 +164,7 @@ "type": "boolean" }, "auto_workspace_group_creation": { - "description": "This identity can create security groups aligned to each applciation role. Read more about it here: docs/tre-admins/auth.md", + "description": "This identity can create security groups aligned to each application role. Read more about it here: docs/tre-admins/auth.md", "type": "boolean" }, "auto_grant_workspace_consent": { diff --git a/templates/workspaces/airlock-import-review/.env.sample b/templates/workspaces/airlock-import-review/.env.sample index c89893b33f..e81954d1ed 100644 --- a/templates/workspaces/airlock-import-review/.env.sample +++ b/templates/workspaces/airlock-import-review/.env.sample @@ -4,26 +4,17 @@ ARM_TENANT_ID="__CHANGE_ME__" ARM_SUBSCRIPTION_ID="__CHANGE_ME__" AUTH_TENANT_ID="__CHANGE_ME__" -# These are passed in if Terraform will create the Workspace Microsoft Entra ID Application -REGISTER_AAD_APPLICATION=true +# Workspace Microsoft Entra ID application details are managed automatically. CREATE_AAD_GROUPS=true AUTH_CLIENT_ID="__CHANGE_ME__" AUTH_CLIENT_SECRET="__CHANGE_ME__" WORKSPACE_OWNER_OBJECT_ID="__CHANGE_ME__" -# These are passed in if you register the Workspace Microsoft Entra ID Application before hand -# REGISTER_AAD_APPLICATION=false +# Provide CLIENT_ID only if you need to reuse an existing application registration. # CLIENT_ID="__CHANGE_ME__" -# CLIENT_SECRET="__CHANGE_ME__" -# WORKSPACE_OWNER_OBJECT_ID="" # Used by Porter, aka TRE_RESOURCE_ID ID="MadeUp123" -SP_ID="" -SCOPE_ID="api://ws_0001" -APP_ROLE_ID_WORKSPACE_OWNER="" -APP_ROLE_ID_WORKSPACE_RESEARCHER="" -APP_ROLE_ID_WORKSPACE_AIRLOCK_MANAGER="" # Complex types are base 64 encoded by resource processor (["10.1.10.0/24"], in this case) ADDRESS_SPACES="WyIxMC4xLjEwLjAvMjQiXQ==" ENABLE_LOCAL_DEBUGGING=true diff --git a/templates/workspaces/airlock-import-review/parameters.json b/templates/workspaces/airlock-import-review/parameters.json index 56dd8ec0de..46f22e4888 100755 --- a/templates/workspaces/airlock-import-review/parameters.json +++ b/templates/workspaces/airlock-import-review/parameters.json @@ -52,12 +52,6 @@ "env": "ENABLE_LOCAL_DEBUGGING" } }, - { - "name": "register_aad_application", - "source": { - "env": "REGISTER_AAD_APPLICATION" - } - }, { "name": "create_aad_groups", "source": { @@ -70,48 +64,12 @@ "env": "CLIENT_ID" } }, - { - "name": "client_secret", - "source": { - "env": "CLIENT_SECRET" - } - }, - { - "name": "scope_id", - "source": { - "env": "SCOPE_ID" - } - }, { "name": "workspace_owner_object_id", "source": { "env": "WORKSPACE_OWNER_OBJECT_ID" } }, - { - "name": "sp_id", - "source": { - "env": "SP_ID" - } - }, - { - "name": "app_role_id_workspace_owner", - "source": { - "env": "APP_ROLE_ID_WORKSPACE_OWNER" - } - }, - { - "name": "app_role_id_workspace_researcher", - "source": { - "env": "APP_ROLE_ID_WORKSPACE_RESEARCHER" - } - }, - { - "name": "app_role_id_workspace_airlock_manager", - "source": { - "env": "APP_ROLE_ID_WORKSPACE_AIRLOCK_MANAGER" - } - }, { "name": "aad_redirect_uris", "source": { diff --git a/templates/workspaces/airlock-import-review/porter.yaml b/templates/workspaces/airlock-import-review/porter.yaml index bcd0e0b8b4..6934cf921a 100644 --- a/templates/workspaces/airlock-import-review/porter.yaml +++ b/templates/workspaces/airlock-import-review/porter.yaml @@ -1,7 +1,7 @@ --- schemaVersion: 1.0.0 name: tre-workspace-airlock-import-review -version: 0.14.7 +version: 1.0.0 description: "A workspace to do Airlock Data Import Reviews for Azure TRE" dockerfile: Dockerfile.tmpl registry: azuretre @@ -61,10 +61,6 @@ parameters: - name: enable_local_debugging type: boolean default: false - - name: register_aad_application - type: boolean - default: false - description: "Whether this bundle should register the workspace in AAD" - name: create_aad_groups type: boolean default: true @@ -81,36 +77,10 @@ parameters: description: "The client id of the workspace in the identity provider. This value is typically provided to you when you create the ws application" - - name: client_secret - type: string - description: - "The client secret of the workspace in the identity provider. This value is typically provided to you - when you create the ws application" - default: "" - name: ui_client_id type: string default: "" description: "The client ID of the UI" - - name: scope_id - type: string - default: "" - description: "The Service Principal Name or identifierUri (e.g. api://GUID" - - name: sp_id - type: string - default: "" - description: "The Service Principal in the Identity provider to be able to get claims" - - name: app_role_id_workspace_owner - type: string - default: "" - description: "The id of the application role WorkspaceOwner in the identity provider" - - name: app_role_id_workspace_researcher - type: string - default: "" - description: "The id of the application role WorkspaceResearcher in the identity provider" - - name: app_role_id_workspace_airlock_manager - type: string - default: "" - description: "The id of the application role AirlockManager in the identity provider" - name: aad_redirect_uris type: string description: "List of redirect URIs in {name:value} format" @@ -193,7 +163,6 @@ install: location: ${ bundle.parameters.azure_location } address_spaces: ${ bundle.parameters.address_spaces } enable_local_debugging: ${ bundle.parameters.enable_local_debugging } - register_aad_application: ${ bundle.parameters.register_aad_application } create_aad_groups: ${ bundle.parameters.create_aad_groups } core_api_client_id: ${ bundle.parameters.core_api_client_id } auth_client_id: ${ bundle.credentials.auth_client_id } @@ -201,13 +170,7 @@ install: auth_tenant_id: ${ bundle.credentials.auth_tenant_id } workspace_owner_object_id: ${ bundle.parameters.workspace_owner_object_id } client_id: ${ bundle.parameters.client_id } - client_secret: ${ bundle.parameters.client_secret } ui_client_id: ${ bundle.parameters.ui_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 } - app_role_id_workspace_airlock_manager: ${ bundle.parameters.app_role_id_workspace_airlock_manager } aad_redirect_uris_b64: ${ bundle.parameters.aad_redirect_uris } app_service_plan_sku: ${ bundle.parameters.app_service_plan_sku } enable_airlock: false @@ -242,7 +205,6 @@ upgrade: location: ${ bundle.parameters.azure_location } address_spaces: ${ bundle.parameters.address_spaces } enable_local_debugging: ${ bundle.parameters.enable_local_debugging } - register_aad_application: ${ bundle.parameters.register_aad_application } create_aad_groups: ${ bundle.parameters.create_aad_groups } core_api_client_id: ${ bundle.parameters.core_api_client_id } auth_client_id: ${ bundle.credentials.auth_client_id } @@ -250,13 +212,7 @@ upgrade: auth_tenant_id: ${ bundle.credentials.auth_tenant_id } workspace_owner_object_id: ${ bundle.parameters.workspace_owner_object_id } client_id: ${ bundle.parameters.client_id } - client_secret: ${ bundle.parameters.client_secret } ui_client_id: ${ bundle.parameters.ui_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 } - app_role_id_workspace_airlock_manager: ${ bundle.parameters.app_role_id_workspace_airlock_manager } aad_redirect_uris_b64: ${ bundle.parameters.aad_redirect_uris } app_service_plan_sku: ${ bundle.parameters.app_service_plan_sku } enable_airlock: false @@ -281,30 +237,6 @@ upgrade: - name: workspace_owners_group_id - name: workspace_researchers_group_id - name: workspace_airlock_managers_group_id - - az: - description: "Set Azure Cloud Environment" - arguments: - - cloud - - set - flags: - name: ${ bundle.parameters.azure_environment } - - az: - description: "AAD Application Admin Login" - arguments: - - login - flags: - service-principal: "" - username: "${ bundle.credentials.auth_client_id }" - password: "${ bundle.credentials.auth_client_secret }" - tenant: "${ bundle.credentials.auth_tenant_id }" - allow-no-subscriptions: "" - - exec: - description: "Update workspace app redirect urls" - command: ./update_redirect_urls.sh - flags: - workspace-api-client-id: "${ bundle.parameters.client_id }" - aad-redirect-uris-b64: "${ bundle.parameters.aad_redirect_uris }" - register-aad-application: "${ bundle.parameters.register_aad_application }" uninstall: - terraform: @@ -315,7 +247,6 @@ uninstall: location: ${ bundle.parameters.azure_location } address_spaces: ${ bundle.parameters.address_spaces } enable_local_debugging: ${ bundle.parameters.enable_local_debugging } - register_aad_application: ${ bundle.parameters.register_aad_application } create_aad_groups: ${ bundle.parameters.create_aad_groups } core_api_client_id: ${ bundle.parameters.core_api_client_id } auth_client_id: ${ bundle.credentials.auth_client_id } @@ -323,12 +254,7 @@ uninstall: auth_tenant_id: ${ bundle.credentials.auth_tenant_id } workspace_owner_object_id: ${ bundle.parameters.workspace_owner_object_id } client_id: ${ bundle.parameters.client_id } - scope_id: ${ bundle.parameters.scope_id } ui_client_id: ${ bundle.parameters.ui_client_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 } - app_role_id_workspace_airlock_manager: ${ bundle.parameters.app_role_id_workspace_airlock_manager } aad_redirect_uris_b64: ${ bundle.parameters.aad_redirect_uris } app_service_plan_sku: ${ bundle.parameters.app_service_plan_sku } enable_airlock: false diff --git a/templates/workspaces/airlock-import-review/template_schema.json b/templates/workspaces/airlock-import-review/template_schema.json index 09d7d6be87..0c5e3b8c13 100644 --- a/templates/workspaces/airlock-import-review/template_schema.json +++ b/templates/workspaces/airlock-import-review/template_schema.json @@ -98,13 +98,6 @@ "title": "Application (Client) ID", "description": "The AAD Application Registration ID for the workspace.", "updateable": true - }, - "client_secret": { - "type": "string", - "title": "Application (Client) Secret", - "description": "The AAD Application Registration secret for the workspace. This value will be stored in the Workspace Key Vault.", - "sensitive": true, - "updateable": true } }, "required": [ @@ -177,7 +170,6 @@ "auth_type", "create_aad_groups", "client_id", - "client_secret", "*" ] } diff --git a/templates/workspaces/base/.env.sample b/templates/workspaces/base/.env.sample index aba393f753..cafa3ca42d 100644 --- a/templates/workspaces/base/.env.sample +++ b/templates/workspaces/base/.env.sample @@ -4,18 +4,14 @@ ARM_TENANT_ID="__CHANGE_ME__" ARM_SUBSCRIPTION_ID="__CHANGE_ME__" AUTH_TENANT_ID="__CHANGE_ME__" -# These are passed in if Terraform will create the Workspace Microsoft Entra ID Application -REGISTER_AAD_APPLICATION=true +# Workspace Microsoft Entra ID application details are managed automatically. CREATE_AAD_GROUPS=true AUTH_CLIENT_ID="__CHANGE_ME__" AUTH_CLIENT_SECRET="__CHANGE_ME__" WORKSPACE_OWNER_OBJECT_ID="__CHANGE_ME__" -# These are passed in if you register the Workspace Microsoft Entra ID Application before hand -# REGISTER_AAD_APPLICATION=false +# Provide CLIENT_ID only if you need to reuse an existing application registration. # CLIENT_ID="__CHANGE_ME__" -# CLIENT_SECRET="__CHANGE_ME__" -# WORKSPACE_OWNER_OBJECT_ID="" # Used by Porter, aka TRE_RESOURCE_ID ID="MadeUp123" diff --git a/templates/workspaces/base/parameters.json b/templates/workspaces/base/parameters.json index 3b24ef4ee4..20fc693c66 100755 --- a/templates/workspaces/base/parameters.json +++ b/templates/workspaces/base/parameters.json @@ -58,12 +58,6 @@ "env": "ENABLE_LOCAL_DEBUGGING" } }, - { - "name": "register_aad_application", - "source": { - "env": "REGISTER_AAD_APPLICATION" - } - }, { "name": "create_aad_groups", "source": { diff --git a/templates/workspaces/base/porter.yaml b/templates/workspaces/base/porter.yaml index 1ffa55d2f6..79f4579761 100644 --- a/templates/workspaces/base/porter.yaml +++ b/templates/workspaces/base/porter.yaml @@ -1,7 +1,7 @@ --- schemaVersion: 1.0.0 name: tre-workspace-base -version: 4.0.0 +version: 3.0.0 description: "A base Azure TRE workspace" dockerfile: Dockerfile.tmpl registry: azuretre diff --git a/templates/workspaces/base/update_redirect_urls.sh b/templates/workspaces/base/update_redirect_urls.sh index 109d62b501..cb67f2b5ac 100755 --- a/templates/workspaces/base/update_redirect_urls.sh +++ b/templates/workspaces/base/update_redirect_urls.sh @@ -7,11 +7,10 @@ set -o pipefail function usage() { cat < Date: Thu, 27 Nov 2025 16:13:17 +0000 Subject: [PATCH 14/29] fix linting --- .../test_db/test_repositories/test_workpaces_repository.py | 2 ++ .../setup-instructions/ui-install-base-workspace.md | 4 ++-- templates/workspaces/base/terraform/aad/variables.tf | 4 ---- templates/workspaces/base/terraform/providers.tf | 4 ---- templates/workspaces/base/terraform/workspace.tf | 1 - templates/workspaces/unrestricted/porter.yaml | 4 ++-- 6 files changed, 6 insertions(+), 13 deletions(-) diff --git a/api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py b/api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py index da62c4d40f..97847685d9 100644 --- a/api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py +++ b/api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py @@ -274,6 +274,8 @@ async def test_create_workspace_item_raises_value_error_if_template_is_invalid(m with pytest.raises(ValueError): await workspace_repo.create_workspace_item(workspace_input, "test_object_id", ["test_role"]) + + def test_workspace_owner_is_set_if_not_present_in_workspace_properties(workspace_repo): dictToTest = {} expected_object_id = "Expected" diff --git a/docs/tre-admins/setup-instructions/ui-install-base-workspace.md b/docs/tre-admins/setup-instructions/ui-install-base-workspace.md index c68259797b..54aedeb8e6 100644 --- a/docs/tre-admins/setup-instructions/ui-install-base-workspace.md +++ b/docs/tre-admins/setup-instructions/ui-install-base-workspace.md @@ -24,8 +24,8 @@ Workspace can be easily created via AzureTRE UI. Open a browser and navigate to: 1. Fill in the details for your workspace: - - General information such as name and description - - [Optional] Update values for Shared Storage Quota, App Service Plan (SKU) and Address space if needed + - General information such as name and description + - [Optional] Update values for Shared Storage Quota, App Service Plan (SKU) and Address space if needed - Workspace Authentication Type - this determines whether you'd like TRE to create an app registration for the workspace automatically, or whether you wish to provide an existing one that you've created manually. To read about how to create it manually read the [Creating a manual Entra ID application for the workspace](#creating-a-manual-entra-id-application-for-the-workspace) section below. 1. After filling the details press submit. diff --git a/templates/workspaces/base/terraform/aad/variables.tf b/templates/workspaces/base/terraform/aad/variables.tf index 7503fe46e8..23df4a7253 100644 --- a/templates/workspaces/base/terraform/aad/variables.tf +++ b/templates/workspaces/base/terraform/aad/variables.tf @@ -1,7 +1,3 @@ -variable "client_id" { - type = string - default = "" -} variable "key_vault_id" { type = string } diff --git a/templates/workspaces/base/terraform/providers.tf b/templates/workspaces/base/terraform/providers.tf index 44f12a6dbc..cf04a61767 100644 --- a/templates/workspaces/base/terraform/providers.tf +++ b/templates/workspaces/base/terraform/providers.tf @@ -12,10 +12,6 @@ terraform { source = "Azure/azapi" version = "= 2.3.0" } - time = { - source = "hashicorp/time" - version = "= 0.11.0" - } } backend "azurerm" {} diff --git a/templates/workspaces/base/terraform/workspace.tf b/templates/workspaces/base/terraform/workspace.tf index 053738c11e..7197b3792c 100644 --- a/templates/workspaces/base/terraform/workspace.tf +++ b/templates/workspaces/base/terraform/workspace.tf @@ -47,7 +47,6 @@ module "network" { module "aad" { source = "./aad" tre_workspace_tags = local.tre_workspace_tags - client_id = var.client_id key_vault_id = azurerm_key_vault.kv.id workspace_resource_name_suffix = local.workspace_resource_name_suffix workspace_owner_object_id = var.workspace_owner_object_id diff --git a/templates/workspaces/unrestricted/porter.yaml b/templates/workspaces/unrestricted/porter.yaml index cb341629be..65c18b4df5 100644 --- a/templates/workspaces/unrestricted/porter.yaml +++ b/templates/workspaces/unrestricted/porter.yaml @@ -1,7 +1,7 @@ --- schemaVersion: 1.0.0 name: tre-workspace-unrestricted -version: 1.0.0 +version: 2.0.0 description: "A base Azure TRE workspace" dockerfile: Dockerfile.tmpl registry: azuretre @@ -95,7 +95,7 @@ parameters: - name: aad_redirect_uris type: string description: "List of redirect URIs in {name:value} format" - default: "W10=" # b64 for [] + default: "W10=" # b64 for [] - name: app_service_plan_sku type: string description: "The SKU used when deploying an Azure App Service Plan" From 24dfd9795e87c1e7581b79e57b562f8f6cd27ea4 Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Thu, 27 Nov 2025 16:21:25 +0000 Subject: [PATCH 15/29] Update e2e tests --- api_app/models/schemas/workspace_template.py | 1 - e2e_tests/conftest.py | 6 ++---- e2e_tests/test_performance.py | 3 +-- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/api_app/models/schemas/workspace_template.py b/api_app/models/schemas/workspace_template.py index bc20955217..f57b0b6330 100644 --- a/api_app/models/schemas/workspace_template.py +++ b/api_app/models/schemas/workspace_template.py @@ -18,7 +18,6 @@ def get_sample_workspace_template_object(template_name: str = "tre-workspace-bas "display_name": Property(type="string"), "description": Property(type="string"), "client_id": Property(type="string"), - "client_secret": Property(type="string"), "address_space_size": Property( type="string", default="small", diff --git a/e2e_tests/conftest.py b/e2e_tests/conftest.py index 39589e1696..a11548fbdb 100644 --- a/e2e_tests/conftest.py +++ b/e2e_tests/conftest.py @@ -32,8 +32,7 @@ async def create_or_get_test_workspace( verify: bool, template_name: str = resource_strings.BASE_WORKSPACE, pre_created_workspace_id: str = "", - client_id: str = "", - client_secret: str = "") -> Tuple[str, str]: + client_id: str = "") -> Tuple[str, str]: if pre_created_workspace_id != "": return f"/workspaces/{pre_created_workspace_id}", pre_created_workspace_id @@ -54,7 +53,6 @@ async def create_or_get_test_workspace( if auth_type == "Manual": payload["properties"]["client_id"] = client_id - payload["properties"]["client_secret"] = client_secret admin_token = await get_admin_token(verify=verify) # TODO: Temp fix to solve creation of workspaces - https://github.com/microsoft/AzureTRE/issues/2986 @@ -108,7 +106,7 @@ async def setup_test_workspace(verify) -> Tuple[str, str, str]: pre_created_workspace_id = config.TEST_WORKSPACE_ID # Set up - uses a pre created app reg as has appropriate roles assigned workspace_path, workspace_id = await create_or_get_test_workspace( - auth_type="Manual", verify=verify, pre_created_workspace_id=pre_created_workspace_id, client_id=config.TEST_WORKSPACE_APP_ID, client_secret=config.TEST_WORKSPACE_APP_SECRET) + auth_type="Manual", verify=verify, pre_created_workspace_id=pre_created_workspace_id, client_id=config.TEST_WORKSPACE_APP_ID) yield workspace_path, workspace_id diff --git a/e2e_tests/test_performance.py b/e2e_tests/test_performance.py index 7fa0208a7c..72d8574c6e 100644 --- a/e2e_tests/test_performance.py +++ b/e2e_tests/test_performance.py @@ -68,8 +68,7 @@ async def test_bulk_updates_to_ensure_each_resource_updated_in_series(verify) -> "description": "", "address_space_size": "small", "auth_type": "Manual", - "client_id": f"{config.TEST_WORKSPACE_APP_ID}", - "client_secret": f"{config.TEST_WORKSPACE_APP_SECRET}" + "client_id": f"{config.TEST_WORKSPACE_APP_ID}" } } From c79a9e0411fedb222f712747ea24416b58cf96e0 Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Thu, 27 Nov 2025 16:24:00 +0000 Subject: [PATCH 16/29] Update CHANGELOG.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c599a42a51..22f5070bfc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ **BREAKING CHANGES** * Fix missing arguments for airlock manager requests - change in API contract ([#4544](https://github.com/microsoft/AzureTRE/issues/4544)) -* Base workspace bundle 4.0.0 (major upgrade from 2.8.0) now creates and rotates the workspace Microsoft Entra application secret automatically and removes the manual identity passthrough parameters (`client_secret`, `register_aad_application`, `scope_id`, `sp_id`, `app_role_id_*`). +* Base workspace bundle 3.0.0 (major upgrade from 2.8.0) now creates and rotates the workspace Microsoft Entra application secret automatically and removes the manual identity passthrough parameters (`client_secret`, `register_aad_application`, `scope_id`, `sp_id`, `app_role_id_*`). - Existing workspaces that relied on manually managed secrets continue to operate without interruption; upgrade them at your own pace. - The automation admin (`APPLICATION_ADMIN_CLIENT_ID`) no longer needs the `Directory.Read.All` Microsoft Graph permission; keep the documented `Application.ReadWrite.*`, `Group.*`, `User.ReadBasic.All`, and `DelegatedPermissionGrant.ReadWrite.All` permissions in place. ([#4775](https://github.com/microsoft/AzureTRE/pull/4775)) From d7032511f5900696dc5533c41ad5c3c304e1d86d Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Thu, 27 Nov 2025 16:24:09 +0000 Subject: [PATCH 17/29] Update docs/tre-templates/workspaces/base.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- docs/tre-templates/workspaces/base.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/tre-templates/workspaces/base.md b/docs/tre-templates/workspaces/base.md index 11cdb55dae..72bb9ad2c8 100644 --- a/docs/tre-templates/workspaces/base.md +++ b/docs/tre-templates/workspaces/base.md @@ -22,7 +22,7 @@ When deploying a workspace the following properties need to be configured. | `client_id` | Valid client ID of the Workspace App Registration. | Required only when `auth_type` is set to `Manual`. Leave empty (default) to allow the TRE to create and manage the workspace application automatically. | ## Azure Trusted Services -*Azure Trusted Services* are allowed to connect to both the key vault and storage account provsioned within the workspace. If this is undesirable additonal resources without this setting configured can be deployed. +*Azure Trusted Services* are allowed to connect to both the key vault and storage account provsioned within the workspace. If this is undesirable additional resources without this setting configured can be deployed. Further details around which Azure services are allowed to connect can be found below: From da6a64fa864ae888079cf6949ed427e19e7d8f79 Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Thu, 27 Nov 2025 16:24:39 +0000 Subject: [PATCH 18/29] Remove debreciated parameter. --- docs/tre-admins/identities/workspace.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/tre-admins/identities/workspace.md b/docs/tre-admins/identities/workspace.md index 65bd142391..4d96c87bb2 100644 --- a/docs/tre-admins/identities/workspace.md +++ b/docs/tre-admins/identities/workspace.md @@ -40,7 +40,6 @@ Example on how to run the script: | Argument | Description | | -------- | ----------- | | `--name` | The name of the application. This will be suffixed with 'API' by the script. | -| `--automation-clientid` | This is an optional parameter but will grant the Automation App (created in step 1) permission to the new workspace app. | | `--application-admin-clientid` | This is a required parameter , and should be a client id that will be added to the Owners of the Microsoft Entra ID Application so that it can be administered within TRE. | ## Comments From e0c4bb8fe60939cfe498ac78b2453511ee43a18d Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Thu, 27 Nov 2025 16:26:15 +0000 Subject: [PATCH 19/29] simplify import --- templates/workspaces/base/terraform/workspace.tf | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/templates/workspaces/base/terraform/workspace.tf b/templates/workspaces/base/terraform/workspace.tf index 7197b3792c..6b1e1b597b 100644 --- a/templates/workspaces/base/terraform/workspace.tf +++ b/templates/workspaces/base/terraform/workspace.tf @@ -3,12 +3,6 @@ data "azuread_application" "existing_workspace" { client_id = var.client_id } -locals { - workspace_app_imports = var.client_id != "" ? { - existing = format("/applications/%s", data.azuread_application.existing_workspace[0].object_id) - } : {} -} - resource "azurerm_resource_group" "ws" { location = var.location name = "rg-${local.workspace_resource_name_suffix}" @@ -63,9 +57,9 @@ module "aad" { } import { - for_each = local.workspace_app_imports - to = module.aad.azuread_application.workspace - id = each.value + count = var.client_id != "" ? 1 : 0 + to = module.aad.azuread_application.workspace + id = format("/applications/%s", data.azuread_application.existing_workspace[0].object_id) } module "airlock" { From f883ce12a05dc44b23711de549734e55bcc4b8e3 Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Thu, 27 Nov 2025 17:03:12 +0000 Subject: [PATCH 20/29] Remove more unused vars --- core/terraform/outputs.sh | 4 ---- devops/scripts/setup_local_debugging.sh | 3 --- e2e_tests/.env.sample | 1 - templates/workspaces/base/terraform/workspace.tf | 12 +++++++++--- 4 files changed, 9 insertions(+), 11 deletions(-) diff --git a/core/terraform/outputs.sh b/core/terraform/outputs.sh index cbbe3b351f..47e89dd505 100644 --- a/core/terraform/outputs.sh +++ b/core/terraform/outputs.sh @@ -30,10 +30,6 @@ if [ -f ../.env ]; then source ../.env fi -# Add a few extra values to the file to help us (i.e. for local debugging api_app and resource processor) -# shellcheck disable=SC2129 -echo "TEST_WORKSPACE_APP_ID='${WORKSPACE_API_CLIENT_ID}'" >> ../private.env - # These next ones from Check Dependencies echo "SUBSCRIPTION_ID='${SUB_ID}'" >> ../private.env echo "AZURE_SUBSCRIPTION_ID='${SUB_ID}'" >> ../private.env diff --git a/devops/scripts/setup_local_debugging.sh b/devops/scripts/setup_local_debugging.sh index 1d18ffc25f..3a7e36b910 100755 --- a/devops/scripts/setup_local_debugging.sh +++ b/devops/scripts/setup_local_debugging.sh @@ -162,8 +162,6 @@ sed -i '/ARM_CLIENT_SECRET/d' "${private_env_path}" sed -i '/AAD_TENANT_ID/d' "${private_env_path}" sed -i '/APPLICATION_ADMIN_CLIENT_ID/d' "${private_env_path}" sed -i '/APPLICATION_ADMIN_CLIENT_SECRET/d' "${private_env_path}" -sed -i '/TEST_WORKSPACE_APP_ID/d' "${private_env_path}" -sed -i '/TEST_WORKSPACE_APP_SECRET/d' "${private_env_path}" # Append them to the TRE file so that the Resource Processor can use them tee -a "${private_env_path}" < TEST_USER_PASSWORD= TEST_WORKSPACE_APP_ID= -TEST_WORKSPACE_APP_SECRET= # TODO: move to RP default with https://github.com/microsoft/AzureTRE/pull/2634 WORKSPACE_APP_SERVICE_PLAN_SKU="P1v2" diff --git a/templates/workspaces/base/terraform/workspace.tf b/templates/workspaces/base/terraform/workspace.tf index 6b1e1b597b..7197b3792c 100644 --- a/templates/workspaces/base/terraform/workspace.tf +++ b/templates/workspaces/base/terraform/workspace.tf @@ -3,6 +3,12 @@ data "azuread_application" "existing_workspace" { client_id = var.client_id } +locals { + workspace_app_imports = var.client_id != "" ? { + existing = format("/applications/%s", data.azuread_application.existing_workspace[0].object_id) + } : {} +} + resource "azurerm_resource_group" "ws" { location = var.location name = "rg-${local.workspace_resource_name_suffix}" @@ -57,9 +63,9 @@ module "aad" { } import { - count = var.client_id != "" ? 1 : 0 - to = module.aad.azuread_application.workspace - id = format("/applications/%s", data.azuread_application.existing_workspace[0].object_id) + for_each = local.workspace_app_imports + to = module.aad.azuread_application.workspace + id = each.value } module "airlock" { From 3a9d8d8fbcd7840aa831838459ee0d30c40798d0 Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Thu, 27 Nov 2025 17:53:24 +0000 Subject: [PATCH 21/29] fix spelling --- docs/tre-templates/workspaces/base.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/tre-templates/workspaces/base.md b/docs/tre-templates/workspaces/base.md index 72bb9ad2c8..0ace446356 100644 --- a/docs/tre-templates/workspaces/base.md +++ b/docs/tre-templates/workspaces/base.md @@ -22,7 +22,7 @@ When deploying a workspace the following properties need to be configured. | `client_id` | Valid client ID of the Workspace App Registration. | Required only when `auth_type` is set to `Manual`. Leave empty (default) to allow the TRE to create and manage the workspace application automatically. | ## Azure Trusted Services -*Azure Trusted Services* are allowed to connect to both the key vault and storage account provsioned within the workspace. If this is undesirable additional resources without this setting configured can be deployed. +*Azure Trusted Services* are allowed to connect to both the key vault and storage account provisioned within the workspace. If this is undesirable additional resources without this setting configured can be deployed. Further details around which Azure services are allowed to connect can be found below: From 83c0f4a2de594926addd745fa265147c862034bb Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Fri, 28 Nov 2025 12:21:45 +0000 Subject: [PATCH 22/29] Update e2e tests given roles arent preconfigured in app reg. --- api_app/_version.py | 2 +- api_app/services/aad_authentication.py | 23 +- .../test_services/test_aad_access_service.py | 32 +++ docs/tre-developers/end-to-end-tests.md | 17 +- e2e_tests/cloud.py | 5 + e2e_tests/conftest.py | 21 +- e2e_tests/helpers.py | 199 ++++++++++++++++++ 7 files changed, 278 insertions(+), 21 deletions(-) diff --git a/api_app/_version.py b/api_app/_version.py index 7c4a9591e1..025f4c5d0b 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.26.0" +__version__ = "0.26.1" diff --git a/api_app/services/aad_authentication.py b/api_app/services/aad_authentication.py index 52cb6e74d7..98f4918050 100644 --- a/api_app/services/aad_authentication.py +++ b/api_app/services/aad_authentication.py @@ -374,9 +374,26 @@ def assign_workspace_user(self, user_id: str, workspace: Workspace, role_id: str return self._assign_workspace_user_to_application_group(user_id, workspace, role_id) def _is_user_in_role(self, user_id: str, role_id: str) -> bool: - user_app_role_query = f"{MICROSOFT_GRAPH_URL}/v1.0/users/{user_id}/appRoleAssignments" - user_app_roles = self._ms_graph_query(user_app_role_query, "GET") - return any(r for r in user_app_roles["value"] if r["appRoleId"] == role_id) + try: + identity_type = self._get_identity_type(user_id) + except AuthConfigValidationError as exc: + logger.warning("Unable to determine identity type for %s: %s", user_id, exc) + return False + + if identity_type == "#microsoft.graph.user": + graph_data = self._get_role_assignment_graph_data_for_user(user_id) + elif identity_type == "#microsoft.graph.servicePrincipal": + graph_data = self._get_role_assignment_graph_data_for_service_principal(user_id) + else: + logger.warning("Unsupported identity type %s for %s", identity_type, user_id) + return False + + assignments = graph_data.get("value") + if assignments is None: + logger.warning("Role assignment response missing 'value' for %s", user_id) + return False + + return any(role.get("appRoleId") == role_id for role in assignments) def _is_workspace_role_group_in_use(self, workspace: Workspace) -> bool: aad_groups_in_user = workspace.properties["create_aad_groups"] 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 16730fb55a..d69d71f1df 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 @@ -574,6 +574,38 @@ def test_get_role_assignment_for_user(mock_get_role_assignment_data_for_user): assert role == mock_user_data["value"][0] +@patch("services.aad_authentication.AzureADAuthorization._get_role_assignment_graph_data_for_user") +@patch("services.aad_authentication.AzureADAuthorization._get_identity_type", return_value="#microsoft.graph.user") +def test_is_user_in_role_with_user_identity(mock_get_identity_type, mock_get_role_assignments): + mock_get_role_assignments.return_value = {"value": [{"appRoleId": "role-a"}]} + access_service = AzureADAuthorization() + + assert access_service._is_user_in_role("user-obj", "role-a") is True + mock_get_identity_type.assert_called_once_with("user-obj") + mock_get_role_assignments.assert_called_once() + + +@patch("services.aad_authentication.AzureADAuthorization._get_role_assignment_graph_data_for_service_principal") +@patch("services.aad_authentication.AzureADAuthorization._get_identity_type", return_value="#microsoft.graph.servicePrincipal") +def test_is_user_in_role_with_service_principal(mock_get_identity_type, mock_get_role_assignments): + mock_get_role_assignments.return_value = {"value": [{"appRoleId": "role-b"}]} + access_service = AzureADAuthorization() + + assert access_service._is_user_in_role("sp-obj", "role-b") is True + mock_get_identity_type.assert_called_once_with("sp-obj") + mock_get_role_assignments.assert_called_once() + + +@patch("services.aad_authentication.AzureADAuthorization._get_identity_type", return_value="#microsoft.graph.servicePrincipal") +@patch("services.aad_authentication.AzureADAuthorization._get_role_assignment_graph_data_for_service_principal", return_value={}) +def test_is_user_in_role_handles_missing_graph_value(mock_get_assignments, mock_get_identity_type): + access_service = AzureADAuthorization() + + assert access_service._is_user_in_role("sp-obj", "role-c") is False + mock_get_identity_type.assert_called_once_with("sp-obj") + mock_get_assignments.assert_called_once() + + def get_mock_batch_response(user_principals, group_principals): response_body = {"responses": []} for user_principal in user_principals: diff --git a/docs/tre-developers/end-to-end-tests.md b/docs/tre-developers/end-to-end-tests.md index 8aa6ace8f1..7cc5e3d936 100644 --- a/docs/tre-developers/end-to-end-tests.md +++ b/docs/tre-developers/end-to-end-tests.md @@ -1,21 +1,18 @@ # End-to-end (E2E) tests -## Prerequisites +## Create Workspace Application for E2E tests -1. Authentication and Authorization configuration set up as noted [here](../tre-admins/auth.md) -1. An Azure Tre deployed environment. +End-to-end tests require a Microsoft Entra ID application to represent the workspace API of the workspaces created during testing. This application must be created manually prior to running the tests. -## Add Automation Admin Application as Owner of Workspace Application -For the end-to-end tests to be able to create and manage workspaces, the Automation Admin Application created as part of the auth setup needs to be added as an Owner of the Workspace Application(s) used by the TRE. - -You can do this using the Azure Portal, or by using the helper script `./devops/scripts/aad/add_automation_admin_to_workspace_application.sh`. For example: +Example on how to run the script: ```bash - ./devops/scripts/aad/add_automation_admin_to_workspace_application.sh \ - --workspace-application-clientid "${WORKSPACE_API_CLIENT_ID}" + ./devops/scripts/aad/create_workspace_application.sh \ + --name "Workspace Application for E2E Tests" \ + --application-admin-clientid "${APPLICATION_ADMIN_CLIENT_ID}" ``` -The script automatically reads the automation admin client ID from `config.yaml` (authentication.test_account_client_id). +The Workspace Application ID then needs adding to the `e2e_tests/.env` file under the `TEST_WORKSPACE_APP_ID` property. ## Registering bundles to run End-to-end tests diff --git a/e2e_tests/cloud.py b/e2e_tests/cloud.py index a1d065552c..d7197f9843 100644 --- a/e2e_tests/cloud.py +++ b/e2e_tests/cloud.py @@ -8,3 +8,8 @@ def get_cloud() -> cloud.Cloud: def get_aad_authority_fqdn() -> str: return urlparse(get_cloud().endpoints.active_directory).netloc + + +def get_ms_graph_resource() -> str: + """Return the Microsoft Graph resource ID for the active cloud.""" + return get_cloud().endpoints.microsoft_graph_resource_id.rstrip("/") diff --git a/e2e_tests/conftest.py b/e2e_tests/conftest.py index a11548fbdb..ffb2df0109 100644 --- a/e2e_tests/conftest.py +++ b/e2e_tests/conftest.py @@ -8,7 +8,7 @@ from resources.resource import post_resource, disable_and_delete_resource from resources.workspace import get_workspace_auth_details from resources import strings as resource_strings -from helpers import get_admin_token +from helpers import get_admin_token, ensure_automation_admin_has_airlock_role, ensure_automation_admin_has_workspace_owner_role LOGGER = logging.getLogger(__name__) @@ -28,11 +28,11 @@ def verify(pytestconfig): async def create_or_get_test_workspace( - auth_type: str, - verify: bool, - template_name: str = resource_strings.BASE_WORKSPACE, - pre_created_workspace_id: str = "", - client_id: str = "") -> Tuple[str, str]: + auth_type: str, + verify: bool, + template_name: str = resource_strings.BASE_WORKSPACE, + pre_created_workspace_id: str = "", + client_id: str = "") -> Tuple[str, str]: if pre_created_workspace_id != "": return f"/workspaces/{pre_created_workspace_id}", pre_created_workspace_id @@ -45,7 +45,8 @@ async def create_or_get_test_workspace( "display_name": f"E2E {description} workspace ({auth_type} AAD)", "description": f"{template_name} test workspace for E2E tests", "auth_type": auth_type, - "address_space_size": "small" + "address_space_size": "small", + "create_aad_groups": True } } if config.TEST_WORKSPACE_APP_PLAN != "": @@ -60,6 +61,12 @@ async def create_or_get_test_workspace( workspace_path, workspace_id = await post_resource(payload, resource_strings.API_WORKSPACES, access_token=admin_token, verify=verify) LOGGER.info(f"Workspace {workspace_id} {template_name} created") + + await ensure_automation_admin_has_airlock_role(workspace_id, admin_token, verify) + + if auth_type == "Manual" and client_id != "": + await ensure_automation_admin_has_workspace_owner_role(workspace_id, admin_token, verify) + return workspace_path, workspace_id diff --git a/e2e_tests/helpers.py b/e2e_tests/helpers.py index 307fb88933..7696714468 100644 --- a/e2e_tests/helpers.py +++ b/e2e_tests/helpers.py @@ -1,7 +1,9 @@ import asyncio +import functools from typing import List, Optional from contextlib import asynccontextmanager from httpx import AsyncClient, Timeout, Response +import httpx import logging from starlette import status from azure.identity import ClientSecretCredential, UsernamePasswordCredential @@ -15,6 +17,11 @@ azlogger = logging.getLogger("azure") azlogger.setLevel(logging.WARN) +GRAPH_TIMEOUT = Timeout(10, read=30) +_AUTOMATION_ADMIN_OBJECT_ID: Optional[str] = None +ROLE_ASSIGNMENT_MAX_ATTEMPTS = 30 +ROLE_ASSIGNMENT_SLEEP_SECONDS = 10 + class InstallFailedException(Exception): pass @@ -125,3 +132,195 @@ def get_token(scope_uri, verify) -> str: def assert_status(response: Response, expected_status: List[int] = [200], message_prefix: str = "Unexpected HTTP Status"): assert response.status_code in expected_status, \ f"{message_prefix}. Expected: {expected_status}. Actual: {response.status_code}. Response text: {response.text}" + + +async def ensure_automation_admin_has_airlock_role(workspace_id: str, admin_token: str, verify: bool) -> None: + await _ensure_automation_admin_has_role(workspace_id, admin_token, verify, role_name="Airlock Manager") + + +async def ensure_automation_admin_has_workspace_owner_role(workspace_id: str, admin_token: str, verify: bool) -> None: + await _ensure_automation_admin_has_role(workspace_id, admin_token, verify, role_name="WorkspaceOwner") + + +async def _ensure_automation_admin_has_role(workspace_id: str, admin_token: str, verify: bool, role_name: str) -> None: + """Assign the automation admin identity to a workspace role via the TRE API.""" + if workspace_id == "": + LOGGER.info("Workspace ID missing; skipping %s assignment", role_name) + return + + if config.TEST_ACCOUNT_CLIENT_ID == "" and config.TEST_USER_NAME == "": + LOGGER.info("No automation admin identity configured; skipping %s assignment", role_name) + return + + if not await _wait_for_user_management_configuration(workspace_id, admin_token, verify, role_name): + return + + role_id = await _get_workspace_role_id(workspace_id, admin_token, verify, role_name=role_name) + if role_id is None: + LOGGER.warning("%s role not found for workspace %s; skipping assignment", role_name, workspace_id) + return + + user_object_id = await _get_automation_admin_object_id(workspace_id, admin_token, verify) + if user_object_id is None: + LOGGER.warning("Unable to determine automation admin directory object id; skipping %s assignment", role_name) + return + + await _assign_workspace_role_via_api(workspace_id, role_id, user_object_id, admin_token, verify, role_name) + + +async def _get_workspace_role_id(workspace_id: str, admin_token: str, verify: bool, role_name: str) -> Optional[str]: + async with AsyncClient(verify=verify, timeout=TIMEOUT) as client: + headers = get_auth_header(admin_token) + response = await client.get(get_full_endpoint(f"/api/workspaces/{workspace_id}/roles"), headers=headers, timeout=TIMEOUT) + assert_status(response, [status.HTTP_200_OK], f"Failed to get workspace roles for {workspace_id}") + + for role in response.json().get("roles", []): + if role.get("displayName") == role_name: + return role.get("id") + + return None + + +async def _assign_workspace_role_via_api(workspace_id: str, role_id: str, user_id: str, admin_token: str, verify: bool, role_name: str) -> None: + payload = { + "role_id": role_id, + "user_ids": [user_id] + } + + async with AsyncClient(verify=verify, timeout=TIMEOUT) as client: + headers = get_auth_header(admin_token) + response = await client.post( + get_full_endpoint(f"/api/workspaces/{workspace_id}/users/assign"), + headers=headers, + json=payload, + timeout=TIMEOUT + ) + assert_status(response, [status.HTTP_202_ACCEPTED], f"Failed to assign {role_name} role for workspace {workspace_id}") + + +async def _get_automation_admin_object_id(workspace_id: str, admin_token: str, verify: bool) -> Optional[str]: + global _AUTOMATION_ADMIN_OBJECT_ID + if _AUTOMATION_ADMIN_OBJECT_ID: + return _AUTOMATION_ADMIN_OBJECT_ID + + if config.TEST_USER_NAME != "": + user_id = await _get_assignable_user_id(workspace_id, admin_token, verify, config.TEST_USER_NAME) + if user_id: + _AUTOMATION_ADMIN_OBJECT_ID = user_id + return user_id + LOGGER.warning("Assignable user lookup failed for %s; falling back to Graph", config.TEST_USER_NAME) + + loop = asyncio.get_running_loop() + directory_id = await loop.run_in_executor(None, functools.partial(_get_directory_object_id_via_graph, verify)) + if directory_id: + _AUTOMATION_ADMIN_OBJECT_ID = directory_id + return directory_id + + +async def _get_assignable_user_id(workspace_id: str, admin_token: str, verify: bool, user_principal_name: str) -> Optional[str]: + params = { + "filter": user_principal_name, + "maxResultCount": 50 + } + + async with AsyncClient(verify=verify, timeout=TIMEOUT) as client: + headers = get_auth_header(admin_token) + response = await client.get( + get_full_endpoint(f"/api/workspaces/{workspace_id}/assignable-users"), + headers=headers, + params=params, + timeout=TIMEOUT + ) + assert_status(response, [status.HTTP_200_OK], f"Failed to get assignable users for workspace {workspace_id}") + + for user in response.json().get("assignableUsers", []): + upn = user.get("userPrincipalName", "").lower() + if upn == user_principal_name.lower(): + return user.get("id") + + return None + + +async def _wait_for_user_management_configuration(workspace_id: str, admin_token: str, verify: bool, role_name: str) -> bool: + """Poll the workspace until Entra ID role/group data is available.""" + for attempt in range(ROLE_ASSIGNMENT_MAX_ATTEMPTS): + workspace = await _get_workspace_details(workspace_id, admin_token, verify) + if _workspace_supports_user_management(workspace): + return True + + if _workspace_cannot_support_user_management(workspace): + LOGGER.warning("Workspace %s does not support user management; skipping %s assignment", workspace_id, role_name) + return False + + LOGGER.info("Waiting for workspace %s user management config (%s/%s)", workspace_id, attempt + 1, ROLE_ASSIGNMENT_MAX_ATTEMPTS) + await asyncio.sleep(ROLE_ASSIGNMENT_SLEEP_SECONDS) + + LOGGER.warning("Workspace %s never exposed user management config; skipping %s assignment", workspace_id, role_name) + return False + + +async def _get_workspace_details(workspace_id: str, admin_token: str, verify: bool) -> dict: + async with AsyncClient(verify=verify, timeout=TIMEOUT) as client: + headers = get_auth_header(admin_token) + response = await client.get(get_full_endpoint(f"/api/workspaces/{workspace_id}"), headers=headers, timeout=TIMEOUT) + assert_status(response, [status.HTTP_200_OK], f"Failed to get workspace {workspace_id}") + return response.json().get("workspace", {}) + + +def _workspace_supports_user_management(workspace: dict) -> bool: + props = workspace.get("properties", {}) + has_groups = props.get("create_aad_groups") is True + required_fields = [ + "sp_id", + "app_role_id_workspace_owner", + "app_role_id_workspace_researcher", + "app_role_id_workspace_airlock_manager" + ] + return has_groups and all(field in props for field in required_fields) + + +def _workspace_cannot_support_user_management(workspace: dict) -> bool: + props = workspace.get("properties", {}) + deployment_status = workspace.get("deploymentStatus", "") + groups_flag = props.get("create_aad_groups") + return deployment_status == "deployed" and not groups_flag + + +def _get_directory_object_id_via_graph(verify: bool) -> Optional[str]: + graph_resource = cloud.get_ms_graph_resource() + graph_base_url = f"{graph_resource}/v1.0" + token = get_token(graph_resource, verify) + headers = { + "Authorization": f"Bearer {token}", + "Content-Type": "application/json" + } + + identity_id: Optional[str] = None + with httpx.Client(headers=headers, timeout=GRAPH_TIMEOUT, verify=verify) as client: + if config.TEST_ACCOUNT_CLIENT_ID != "": + identity_id = _get_service_principal_id(client, graph_base_url, config.TEST_ACCOUNT_CLIENT_ID) + elif config.TEST_USER_NAME != "": + identity_id = _get_user_object_id(client, graph_base_url, config.TEST_USER_NAME) + + return identity_id + + +def _get_service_principal_id(client: httpx.Client, base_url: str, app_id: str) -> Optional[str]: + resp = client.get(f"{base_url}/servicePrincipals", params={"$filter": f"appId eq '{app_id}'"}) + resp.raise_for_status() + values = resp.json().get("value", []) + if not values: + return None + return values[0]["id"] + + +def _get_user_object_id(client: httpx.Client, base_url: str, user_principal_name: str) -> Optional[str]: + if user_principal_name == "": + return None + + resp = client.get(f"{base_url}/users/{user_principal_name}", params={"$select": "id"}) + if resp.status_code == status.HTTP_404_NOT_FOUND: + return None + + resp.raise_for_status() + return resp.json().get("id") From e45ef5b46672467095912ae554261dafb2c80796 Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Fri, 28 Nov 2025 12:26:06 +0000 Subject: [PATCH 23/29] fix linting --- core/terraform/outputs.sh | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/core/terraform/outputs.sh b/core/terraform/outputs.sh index 47e89dd505..a03316c69b 100644 --- a/core/terraform/outputs.sh +++ b/core/terraform/outputs.sh @@ -31,6 +31,8 @@ if [ -f ../.env ]; then fi # These next ones from Check Dependencies -echo "SUBSCRIPTION_ID='${SUB_ID}'" >> ../private.env -echo "AZURE_SUBSCRIPTION_ID='${SUB_ID}'" >> ../private.env -echo "AZURE_TENANT_ID='${TENANT_ID}'" >> ../private.env +{ + echo "SUBSCRIPTION_ID='${SUB_ID}'" + echo "AZURE_SUBSCRIPTION_ID='${SUB_ID}'" + echo "AZURE_TENANT_ID='${TENANT_ID}'" +} >> ../private.env From bcddf1c449a276215e0f70eee79468c04fbff59b Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Fri, 28 Nov 2025 13:04:16 +0000 Subject: [PATCH 24/29] fix lint --- e2e_tests/conftest.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/e2e_tests/conftest.py b/e2e_tests/conftest.py index ffb2df0109..2c3e7096a9 100644 --- a/e2e_tests/conftest.py +++ b/e2e_tests/conftest.py @@ -21,10 +21,12 @@ def pytest_addoption(parser): @pytest.fixture(scope="session") def verify(pytestconfig): - if pytestconfig.getoption("verify").lower() == "true": + option_value = pytestconfig.getoption("verify").lower() + if option_value == "true": return True - elif pytestconfig.getoption("verify").lower() == "false": + if option_value == "false": return False + raise ValueError("--verify must be 'true' or 'false'") async def create_or_get_test_workspace( From cb9d51d7748d15e1dc39bf6342e7fd64f1d3d711 Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Fri, 28 Nov 2025 13:08:11 +0000 Subject: [PATCH 25/29] Fix linting --- e2e_tests/conftest.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/e2e_tests/conftest.py b/e2e_tests/conftest.py index 2c3e7096a9..c454e4c3a2 100644 --- a/e2e_tests/conftest.py +++ b/e2e_tests/conftest.py @@ -30,11 +30,11 @@ def verify(pytestconfig): async def create_or_get_test_workspace( - auth_type: str, - verify: bool, - template_name: str = resource_strings.BASE_WORKSPACE, - pre_created_workspace_id: str = "", - client_id: str = "") -> Tuple[str, str]: + auth_type: str, + verify: bool, + template_name: str = resource_strings.BASE_WORKSPACE, + pre_created_workspace_id: str = "", + client_id: str = "") -> Tuple[str, str]: if pre_created_workspace_id != "": return f"/workspaces/{pre_created_workspace_id}", pre_created_workspace_id From b6c60d1d5a3242e28c728b3e8d3de192919bc9a9 Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Fri, 28 Nov 2025 16:08:17 +0000 Subject: [PATCH 26/29] Remove TEST_WORKSAPCE_ID from tests --- .../devcontainer_run_command/action.yml | 8 --- .github/scripts/build.js | 10 +++ .github/scripts/build.test.js | 26 ++++++++ .github/workflows/deploy_tre.yml | 2 - .github/workflows/deploy_tre_branch.yml | 2 - .github/workflows/deploy_tre_reusable.yml | 18 +----- .github/workflows/pr_comment_bot.yml | 6 +- .../cicd-pre-deployment-steps.md | 1 - .../setup-instructions/workflows.md | 11 ---- docs/tre-developers/end-to-end-tests.md | 24 +++---- docs/tre-developers/github-pr-bot-commands.md | 4 ++ e2e_tests/.env.sample | 1 - e2e_tests/config.py | 2 - e2e_tests/conftest.py | 64 +++++++++++++++++-- e2e_tests/test_manual_workspace.py | 21 ++++++ e2e_tests/test_performance.py | 6 +- 16 files changed, 137 insertions(+), 69 deletions(-) create mode 100644 e2e_tests/test_manual_workspace.py diff --git a/.github/actions/devcontainer_run_command/action.yml b/.github/actions/devcontainer_run_command/action.yml index b0364585a1..d666070b70 100644 --- a/.github/actions/devcontainer_run_command/action.yml +++ b/.github/actions/devcontainer_run_command/action.yml @@ -30,12 +30,6 @@ inputs: TEST_ACCOUNT_CLIENT_SECRET: description: "The Test Automation Account Client Secret used to interact with the API." required: false - TEST_WORKSPACE_APP_ID: - description: "The Test Workspace application Id used to interact with the API." - required: false - TEST_WORKSPACE_APP_SECRET: - description: "The Test Workspace application secret used to interact with the API." - required: false TRE_ID: description: "The TRE Id." required: false @@ -273,8 +267,6 @@ runs: -e TRE_ID="${{ inputs.TRE_ID }}" \ -e TF_VAR_tre_id="${{ inputs.TRE_ID }}" \ -e TRE_URL="${{ env.TRE_URL }}" \ - -e TEST_WORKSPACE_APP_ID="${{ inputs.TEST_WORKSPACE_APP_ID }}" \ - -e TEST_WORKSPACE_APP_SECRET="${{ inputs.TEST_WORKSPACE_APP_SECRET }}" \ -e TEST_APP_ID="${{ inputs.TEST_APP_ID }}" \ -e TEST_ACCOUNT_CLIENT_ID="${{ inputs.TEST_ACCOUNT_CLIENT_ID }}" \ -e TEST_ACCOUNT_CLIENT_SECRET="${{ inputs.TEST_ACCOUNT_CLIENT_SECRET }}" \ diff --git a/.github/scripts/build.js b/.github/scripts/build.js index 1014402991..0a30e81a72 100644 --- a/.github/scripts/build.js +++ b/.github/scripts/build.js @@ -114,6 +114,15 @@ async function getCommandFromComment({ core, context, github }) { break; } + case "/test-manual-app": + { + const runTests = await handleTestCommand({ core, github }, parts, "manual app tests", runId, { number: prNumber, authorUsername: prAuthorUsername, repoOwner, repoName, headSha: prHeadSha, refId: prRefId, details: pr }, { username: commentUsername, link: commentLink }); + if (runTests) { + command = "run-tests-manual-app"; + } + break; + } + case "/test-force-approve": { command = "test-force-approve"; @@ -250,6 +259,7 @@ You can use the following commands:     /test-extended - build, deploy and run smoke & extended tests on a PR     /test-extended-aad - build, deploy and run smoke & extended AAD tests on a PR     /test-shared-services - test the deployment of shared services on a PR build +    /test-manual-app - run the manual workspace application test suite on a PR build     /test-force-approve - force approval of the PR tests (i.e. skip the deployment checks)     /test-destroy-env - delete the validation environment for a PR (e.g. to enable testing a deployment from a clean start after previous tests)     /help - show this help`; diff --git a/.github/scripts/build.test.js b/.github/scripts/build.test.js index 5a9cd8456b..5a36e41281 100644 --- a/.github/scripts/build.test.js +++ b/.github/scripts/build.test.js @@ -412,6 +412,32 @@ describe('getCommandFromComment', () => { }); }); + describe(`for '/test-manual-app'`, () => { + test(`should set command to 'run-tests-manual-app'`, async () => { + const context = createCommentContext({ + username: 'admin', + body: '/test-manual-app', + }); + await getCommandFromComment({ core, context, github }); + expect(outputFor(mockCoreSetOutput, 'command')).toBe('run-tests-manual-app'); + }); + + test(`should add comment with run link`, async () => { + const context = createCommentContext({ + username: 'admin', + body: '/test-manual-app', + pullRequestNumber: PR_NUMBER.UPSTREAM_NON_DOCS_CHANGES, + }); + await getCommandFromComment({ core, context, github }); + expect(mockGithubRestIssuesCreateComment).toHaveComment({ + owner: 'someOwner', + repo: 'someRepo', + issue_number: PR_NUMBER.UPSTREAM_NON_DOCS_CHANGES, + bodyMatcher: /Running manual app tests: https:\/\/github.com\/someOwner\/someRepo\/actions\/runs\/11112222 \(with refid `291ae84f`\)/, + }); + }); + }); + describe(`for '/test-shared-services'`, () => { test(`should set command to 'run-tests-shared-services'`, async () => { const context = createCommentContext({ diff --git a/.github/workflows/deploy_tre.yml b/.github/workflows/deploy_tre.yml index 021b7a7e9b..733aeb21fd 100644 --- a/.github/workflows/deploy_tre.yml +++ b/.github/workflows/deploy_tre.yml @@ -50,8 +50,6 @@ jobs: MGMT_STORAGE_ACCOUNT_NAME: ${{ secrets.MGMT_STORAGE_ACCOUNT_NAME }} SWAGGER_UI_CLIENT_ID: ${{ secrets.SWAGGER_UI_CLIENT_ID }} TEST_APP_ID: ${{ secrets.TEST_APP_ID }} - TEST_WORKSPACE_APP_ID: ${{ secrets.TEST_WORKSPACE_APP_ID }} - TEST_WORKSPACE_APP_SECRET: "${{ secrets.TEST_WORKSPACE_APP_SECRET }}" TEST_ACCOUNT_CLIENT_ID: "${{ secrets.TEST_ACCOUNT_CLIENT_ID }}" TEST_ACCOUNT_CLIENT_SECRET: "${{ secrets.TEST_ACCOUNT_CLIENT_SECRET }}" TRE_ID: ${{ secrets.TRE_ID }} diff --git a/.github/workflows/deploy_tre_branch.yml b/.github/workflows/deploy_tre_branch.yml index a294880fb9..10a390310b 100644 --- a/.github/workflows/deploy_tre_branch.yml +++ b/.github/workflows/deploy_tre_branch.yml @@ -81,8 +81,6 @@ jobs: MGMT_STORAGE_ACCOUNT_NAME: ${{ format('tre{0}mgmt', needs.prepare-not-main.outputs.refid) }} SWAGGER_UI_CLIENT_ID: ${{ secrets.SWAGGER_UI_CLIENT_ID }} TEST_APP_ID: ${{ secrets.TEST_APP_ID }} - TEST_WORKSPACE_APP_ID: ${{ secrets.TEST_WORKSPACE_APP_ID }} - TEST_WORKSPACE_APP_SECRET: ${{ secrets.TEST_WORKSPACE_APP_SECRET }} TEST_ACCOUNT_CLIENT_ID: "${{ secrets.TEST_ACCOUNT_CLIENT_ID }}" TEST_ACCOUNT_CLIENT_SECRET: "${{ secrets.TEST_ACCOUNT_CLIENT_SECRET }}" TRE_ID: ${{ format('tre{0}', needs.prepare-not-main.outputs.refid) }} diff --git a/.github/workflows/deploy_tre_reusable.yml b/.github/workflows/deploy_tre_reusable.yml index 85f10d5c9f..a49758d1a9 100644 --- a/.github/workflows/deploy_tre_reusable.yml +++ b/.github/workflows/deploy_tre_reusable.yml @@ -76,12 +76,6 @@ on: # yamllint disable-line rule:truthy TEST_APP_ID: description: "" required: true - TEST_WORKSPACE_APP_ID: - description: "" - required: true - TEST_WORKSPACE_APP_SECRET: - description: "" - required: true TEST_ACCOUNT_CLIENT_ID: description: "" required: true @@ -163,12 +157,6 @@ jobs: if [ "${{ secrets.TEST_APP_ID }}" == '' ]; then echo "Missing secret: TEST_APP_ID" && exit 1 fi - if [ "${{ secrets.TEST_WORKSPACE_APP_ID }}" == '' ]; then - echo "Missing secret: TEST_WORKSPACE_APP_ID" && exit 1 - fi - if [ "${{ secrets.TEST_WORKSPACE_APP_SECRET }}" == '' ]; then - echo "Missing secret: TEST_WORKSPACE_APP_SECRET" && exit 1 - fi if [ "${{ secrets.TEST_ACCOUNT_CLIENT_ID }}" == '' ]; then echo "Missing secret: TEST_ACCOUNT_CLIENT_ID" && exit 1 fi @@ -301,7 +289,7 @@ jobs: [ build-and-push-api, build-and-push-resource-processor, - build-and-push-airlock-processor + build-and-push-airlock-processor, ] steps: @@ -815,8 +803,6 @@ jobs: API_CLIENT_ID: "${{ secrets.API_CLIENT_ID }}" AAD_TENANT_ID: "${{ secrets.AAD_TENANT_ID }}" TEST_APP_ID: "${{ secrets.TEST_APP_ID }}" - TEST_WORKSPACE_APP_ID: "${{ secrets.TEST_WORKSPACE_APP_ID }}" - TEST_WORKSPACE_APP_SECRET: "${{ secrets.TEST_WORKSPACE_APP_SECRET }}" TEST_ACCOUNT_CLIENT_ID: "${{ secrets.TEST_ACCOUNT_CLIENT_ID }}" TEST_ACCOUNT_CLIENT_SECRET: "${{ secrets.TEST_ACCOUNT_CLIENT_SECRET }}" TRE_ID: ${{ secrets.TRE_ID }} @@ -859,8 +845,6 @@ jobs: API_CLIENT_ID: "${{ secrets.API_CLIENT_ID }}" AAD_TENANT_ID: "${{ secrets.AAD_TENANT_ID }}" TEST_APP_ID: "${{ secrets.TEST_APP_ID }}" - TEST_WORKSPACE_APP_ID: "${{ secrets.TEST_WORKSPACE_APP_ID }}" - TEST_WORKSPACE_APP_SECRET: "${{ secrets.TEST_WORKSPACE_APP_SECRET }}" TEST_ACCOUNT_CLIENT_ID: "${{ secrets.TEST_ACCOUNT_CLIENT_ID }}" TEST_ACCOUNT_CLIENT_SECRET: "${{ secrets.TEST_ACCOUNT_CLIENT_SECRET }}" TRE_ID: ${{ secrets.TRE_ID }} diff --git a/.github/workflows/pr_comment_bot.yml b/.github/workflows/pr_comment_bot.yml index e7b3609622..0c6521818d 100644 --- a/.github/workflows/pr_comment_bot.yml +++ b/.github/workflows/pr_comment_bot.yml @@ -147,7 +147,8 @@ jobs: needs.pr_comment.outputs.command == 'run-tests' || needs.pr_comment.outputs.command == 'run-tests-extended' || needs.pr_comment.outputs.command == 'run-tests-extended-aad' || - needs.pr_comment.outputs.command == 'run-tests-shared-services' + needs.pr_comment.outputs.command == 'run-tests-shared-services' || + needs.pr_comment.outputs.command == 'run-tests-manual-app' name: Deploy PR uses: ./.github/workflows/deploy_tre_reusable.yml permissions: @@ -162,6 +163,7 @@ jobs: ${{ (needs.pr_comment.outputs.command == 'run-tests-extended' && 'extended') || (needs.pr_comment.outputs.command == 'run-tests-extended-aad' && 'extended_aad') || (needs.pr_comment.outputs.command == 'run-tests-shared-services' && 'shared_services') || + (needs.pr_comment.outputs.command == 'run-tests-manual-app' && 'manual_app') || (needs.pr_comment.outputs.command == 'run-tests' && '') }} environmentName: CICD E2E_TESTS_NUMBER_PROCESSES: 1 @@ -178,8 +180,6 @@ jobs: MGMT_STORAGE_ACCOUNT_NAME: ${{ format('tre{0}mgmt', needs.pr_comment.outputs.prRefId) }} SWAGGER_UI_CLIENT_ID: ${{ secrets.SWAGGER_UI_CLIENT_ID }} TEST_APP_ID: ${{ secrets.TEST_APP_ID }} - TEST_WORKSPACE_APP_ID: ${{ secrets.TEST_WORKSPACE_APP_ID }} - TEST_WORKSPACE_APP_SECRET: "${{ secrets.TEST_WORKSPACE_APP_SECRET }}" TEST_ACCOUNT_CLIENT_ID: "${{ secrets.TEST_ACCOUNT_CLIENT_ID }}" TEST_ACCOUNT_CLIENT_SECRET: "${{ secrets.TEST_ACCOUNT_CLIENT_SECRET }}" TRE_ID: ${{ format('tre{0}', needs.pr_comment.outputs.prRefId) }} diff --git a/docs/tre-admins/setup-instructions/cicd-pre-deployment-steps.md b/docs/tre-admins/setup-instructions/cicd-pre-deployment-steps.md index 88b124be8e..746371688e 100644 --- a/docs/tre-admins/setup-instructions/cicd-pre-deployment-steps.md +++ b/docs/tre-admins/setup-instructions/cicd-pre-deployment-steps.md @@ -102,7 +102,6 @@ In a previous [Setup Auth configuration](./setup-auth-entities.md) step authenti | `API_CLIENT_ID` | API application (client) ID. | | `API_CLIENT_SECRET` | API application client secret. | | `SWAGGER_UI_CLIENT_ID` | Swagger (OpenAPI) UI application (client) ID. | - | `TEST_WORKSPACE_APP_ID`| Each workspace is secured behind it's own AD Application. Use the value of `WORKSPACE_API_CLIENT_ID` created in the `/config.yaml` env file | !!! info See [Environment variables](../environment-variables.md) for full details of the deployment related variables. diff --git a/docs/tre-admins/setup-instructions/workflows.md b/docs/tre-admins/setup-instructions/workflows.md index a22e59e4b8..49bf3b2faf 100644 --- a/docs/tre-admins/setup-instructions/workflows.md +++ b/docs/tre-admins/setup-instructions/workflows.md @@ -90,17 +90,6 @@ Configure the E2E Test repository secrets | `TEST_USER_NAME` | The username of the E2E Test User | | `TEST_USER_PASSWORD` | The password of the E2E Test User | -### Create a workspace app registration for setting up workspaces (for the E2E tests) - -Follow the [instructions to create a workspace app registration](../auth.md#workspaces) (used for the E2E tests) - and make the E2E test user a **WorkspaceOwner** for the app registration. - -Configure the TEST_WORKSPACE_APP_ID repository secret - -|
Secret name
| Description | -| ----------- | ----------- | -| `TEST_WORKSPACE_APP_ID` | The application (client) ID of the Workspaces app. | -| `TEST_WORKSPACE_APP_SECRET` | The application (client) secret of the Workspaces app. | - ### Configure repository/environment secrets Configure additional secrets used in the deployment workflow: diff --git a/docs/tre-developers/end-to-end-tests.md b/docs/tre-developers/end-to-end-tests.md index 7cc5e3d936..dd50f2498b 100644 --- a/docs/tre-developers/end-to-end-tests.md +++ b/docs/tre-developers/end-to-end-tests.md @@ -1,19 +1,5 @@ # End-to-end (E2E) tests -## Create Workspace Application for E2E tests - -End-to-end tests require a Microsoft Entra ID application to represent the workspace API of the workspaces created during testing. This application must be created manually prior to running the tests. - -Example on how to run the script: - -```bash - ./devops/scripts/aad/create_workspace_application.sh \ - --name "Workspace Application for E2E Tests" \ - --application-admin-clientid "${APPLICATION_ADMIN_CLIENT_ID}" -``` - -The Workspace Application ID then needs adding to the `e2e_tests/.env` file under the `TEST_WORKSPACE_APP_ID` property. - ## Registering bundles to run End-to-end tests End-to-end tests depend on certain bundles to be registered within the TRE API. @@ -26,6 +12,16 @@ When running tests locally, use the `prepare-for-e2e` Makefile target: make prepare-for-e2e ``` +## Manually created workspace application for targeted tests + +Most E2E suites now rely on automatically created workspace applications, so you no longer need to provision a manual app registration for standard runs. + +The `test_manually_created_application_owner_token` test (included in the `extended` marker set) exercises the manual-authentication flow. Its fixture automatically runs `devops/scripts/aad/create_workspace_application.sh` to create or reuse a workspace application before deploying the test workspace. + +Ensure `az` CLI is installed, you are logged in to the correct tenant (`az login -t `), and `APPLICATION_ADMIN_CLIENT_ID` (the application admin app registration) is configured so the script can add the necessary owner. + +Run `make test-e2e-custom SELECTOR='manual_app'` to exercise the same flow. + ## Debugging the End-to-End tests Use the "Run and Debug" panel within Visual Studio Code, select "E2E Extended", "E2E Smoke" or "E2E Performance" in the drop down box and click play. diff --git a/docs/tre-developers/github-pr-bot-commands.md b/docs/tre-developers/github-pr-bot-commands.md index a896230c48..a14eaf274a 100644 --- a/docs/tre-developers/github-pr-bot-commands.md +++ b/docs/tre-developers/github-pr-bot-commands.md @@ -48,6 +48,10 @@ You can use the full or short form of the SHA, but it must be at least 7 charact As with `/test`, this command works on PRs from forks, and makes the deployment secrets available. Before running tests on a PR, run the same checks on the PR code as for `/test`. +### `/test-manual-app []` + +Runs the manual workspace application regression suite (currently the tests marked with the `manual_app` marker). Trigger this when you need to validate changes that affect manually created workspace applications or their authentication flow. + ### `/test-destroy-env` When running `/test` multiple times on a PR, the same TRE ID and environment are used by default. The `/test-destroy-env` command destroys a previously created validation environment, allowing you to re-run `/test` with a clean starting point. diff --git a/e2e_tests/.env.sample b/e2e_tests/.env.sample index 288c440949..2a6d5a5e4b 100644 --- a/e2e_tests/.env.sample +++ b/e2e_tests/.env.sample @@ -11,7 +11,6 @@ RESOURCE_LOCATION=westeurope TEST_APP_ID= TEST_USER_PASSWORD= -TEST_WORKSPACE_APP_ID= # TODO: move to RP default with https://github.com/microsoft/AzureTRE/pull/2634 WORKSPACE_APP_SERVICE_PLAN_SKU="P1v2" diff --git a/e2e_tests/config.py b/e2e_tests/config.py index 31ccb34c37..53be4592ad 100644 --- a/e2e_tests/config.py +++ b/e2e_tests/config.py @@ -16,8 +16,6 @@ AAD_TENANT_ID: str = config("AAD_TENANT_ID", default="") TEST_ACCOUNT_CLIENT_ID: str = config("TEST_ACCOUNT_CLIENT_ID", default="") TEST_ACCOUNT_CLIENT_SECRET: str = config("TEST_ACCOUNT_CLIENT_SECRET", default="") -TEST_WORKSPACE_APP_ID: str = config("TEST_WORKSPACE_APP_ID", default="") -TEST_WORKSPACE_APP_SECRET: str = config("TEST_WORKSPACE_APP_SECRET", default="") TEST_WORKSPACE_APP_PLAN: str = config("WORKSPACE_APP_SERVICE_PLAN_SKU", default="") # Set workspace id of an existing workspace to skip creation of a workspace during E2E tests diff --git a/e2e_tests/conftest.py b/e2e_tests/conftest.py index c454e4c3a2..7c8be33e57 100644 --- a/e2e_tests/conftest.py +++ b/e2e_tests/conftest.py @@ -1,9 +1,14 @@ -import random -import pytest import asyncio +import functools +import logging +import random +import re +import subprocess +from pathlib import Path from typing import Tuple + import config -import logging +import pytest from resources.resource import post_resource, disable_and_delete_resource from resources.workspace import get_workspace_auth_details @@ -14,6 +19,10 @@ LOGGER = logging.getLogger(__name__) pytestmark = pytest.mark.asyncio(loop_scope="session") +_MANUALLY_CREATED_APP_NAME_PREFIX = "TRE E2E Manual Workspace" +_CREATE_MANUAL_APP_SCRIPT = Path(__file__).resolve().parents[1] / "devops" / "scripts" / "aad" / "create_workspace_application.sh" +_MANUAL_APP_CLIENT_ID_PATTERN = re.compile(r"Workspace Application Client ID:\s*([0-9a-f-]+)") + def pytest_addoption(parser): parser.addoption("--verify", action="store", default="true") @@ -29,6 +38,39 @@ def verify(pytestconfig): raise ValueError("--verify must be 'true' or 'false'") +def _run_manual_app_script(command: list[str]) -> str: + completed = subprocess.run(command, check=True, capture_output=True, text=True) + return completed.stdout + completed.stderr + + +async def _provision_manually_created_application_client_id() -> str: + if not _CREATE_MANUAL_APP_SCRIPT.exists(): + raise FileNotFoundError(f"Unable to locate create_workspace_application.sh at {_CREATE_MANUAL_APP_SCRIPT}") + + application_admin_client_id = config.API_CLIENT_ID + if application_admin_client_id == "": + raise ValueError("API_CLIENT_ID must be set to provision a manually created workspace application") + + command = [ + str(_CREATE_MANUAL_APP_SCRIPT), + "--name", + _MANUALLY_CREATED_APP_NAME_PREFIX, + "--application-admin-clientid", + application_admin_client_id + ] + + loop = asyncio.get_running_loop() + output = await loop.run_in_executor(None, functools.partial(_run_manual_app_script, command)) + + match = _MANUAL_APP_CLIENT_ID_PATTERN.search(output) + if not match: + raise RuntimeError(f"Unable to parse workspace application client ID from script output: {output}") + + client_id = match.group(1) + LOGGER.info("Ensured manually created workspace application exists: %s", client_id) + return client_id + + async def create_or_get_test_workspace( auth_type: str, verify: bool, @@ -115,7 +157,7 @@ async def setup_test_workspace(verify) -> Tuple[str, str, str]: pre_created_workspace_id = config.TEST_WORKSPACE_ID # Set up - uses a pre created app reg as has appropriate roles assigned workspace_path, workspace_id = await create_or_get_test_workspace( - auth_type="Manual", verify=verify, pre_created_workspace_id=pre_created_workspace_id, client_id=config.TEST_WORKSPACE_APP_ID) + auth_type="Automatic", verify=verify, pre_created_workspace_id=pre_created_workspace_id) yield workspace_path, workspace_id @@ -155,6 +197,20 @@ async def setup_test_aad_workspace(verify) -> Tuple[str, str, str]: await clean_up_test_workspace(pre_created_workspace_id=pre_created_workspace_id, workspace_path=workspace_path, verify=verify) +@pytest.fixture(scope="session") +async def setup_manually_created_application_workspace(verify) -> Tuple[str, str]: + client_id = await _provision_manually_created_application_client_id() + + workspace_path, workspace_id = await create_or_get_test_workspace( + auth_type="Manual", + verify=verify, + client_id=client_id) + + yield workspace_path, workspace_id + + await clean_up_test_workspace(pre_created_workspace_id="", workspace_path=workspace_path, verify=verify) + + async def get_workspace_owner_token(workspace_id, verify): admin_token = await get_admin_token(verify=verify) workspace_owner_token, _ = await get_workspace_auth_details(admin_token=admin_token, workspace_id=workspace_id, verify=verify) diff --git a/e2e_tests/test_manual_workspace.py b/e2e_tests/test_manual_workspace.py new file mode 100644 index 0000000000..46c4163837 --- /dev/null +++ b/e2e_tests/test_manual_workspace.py @@ -0,0 +1,21 @@ +import pytest + +from helpers import get_admin_token +from resources.workspace import get_workspace_auth_details + +pytestmark = pytest.mark.asyncio(loop_scope="session") + + +@pytest.mark.manual_app +async def test_manually_created_application_workspace(setup_manually_created_application_workspace, verify) -> None: + """Ensure a manually created workspace application continues to work for manual auth flows.""" + workspace_path, workspace_id = setup_manually_created_application_workspace + + admin_token = await get_admin_token(verify=verify) + workspace_owner_token, scope_uri = await get_workspace_auth_details( + admin_token=admin_token, + workspace_id=workspace_id, + verify=verify) + + assert workspace_owner_token, "Expected a workspace owner token for manually created app" + assert scope_uri.startswith("api://"), "Scope URI should be an api:// identifier" diff --git a/e2e_tests/test_performance.py b/e2e_tests/test_performance.py index 72d8574c6e..a823e3e769 100644 --- a/e2e_tests/test_performance.py +++ b/e2e_tests/test_performance.py @@ -27,8 +27,7 @@ async def test_parallel_resource_creations(verify) -> None: "display_name": f'Perf Test Workspace {i}', "description": "workspace for perf test", "address_space_size": "small", - "auth_type": "Manual", - "client_id": f"{config.TEST_WORKSPACE_APP_ID}" + "auth_type": "Automatic" } } @@ -67,8 +66,7 @@ async def test_bulk_updates_to_ensure_each_resource_updated_in_series(verify) -> "display_name": "E2E test guacamole service", "description": "", "address_space_size": "small", - "auth_type": "Manual", - "client_id": f"{config.TEST_WORKSPACE_APP_ID}" + "auth_type": "Automatic" } } From 1e4acdbb52d3f7960a1cfb6621cdfa393f5e4d0c Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Fri, 28 Nov 2025 17:29:55 +0000 Subject: [PATCH 27/29] Add retry loop when getting workspace role IDs --- e2e_tests/helpers.py | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/e2e_tests/helpers.py b/e2e_tests/helpers.py index 7696714468..62c6bab45a 100644 --- a/e2e_tests/helpers.py +++ b/e2e_tests/helpers.py @@ -169,16 +169,28 @@ async def _ensure_automation_admin_has_role(workspace_id: str, admin_token: str, async def _get_workspace_role_id(workspace_id: str, admin_token: str, verify: bool, role_name: str) -> Optional[str]: + """Retrieve a workspace role ID, retrying on 404 to allow time for Entra ID role propagation.""" async with AsyncClient(verify=verify, timeout=TIMEOUT) as client: headers = get_auth_header(admin_token) - response = await client.get(get_full_endpoint(f"/api/workspaces/{workspace_id}/roles"), headers=headers, timeout=TIMEOUT) - assert_status(response, [status.HTTP_200_OK], f"Failed to get workspace roles for {workspace_id}") - - for role in response.json().get("roles", []): - if role.get("displayName") == role_name: - return role.get("id") - - return None + + for attempt in range(ROLE_ASSIGNMENT_MAX_ATTEMPTS): + response = await client.get(get_full_endpoint(f"/api/workspaces/{workspace_id}/roles"), headers=headers, timeout=TIMEOUT) + + if response.status_code == status.HTTP_404_NOT_FOUND: + LOGGER.info("Workspace roles not yet available for %s (%s/%s)", workspace_id, attempt + 1, ROLE_ASSIGNMENT_MAX_ATTEMPTS) + await asyncio.sleep(ROLE_ASSIGNMENT_SLEEP_SECONDS) + continue + + assert_status(response, [status.HTTP_200_OK], f"Failed to get workspace roles for {workspace_id}") + + for role in response.json().get("roles", []): + if role.get("displayName") == role_name: + return role.get("id") + + return None + + LOGGER.warning("Workspace roles never became available for %s after %s attempts", workspace_id, ROLE_ASSIGNMENT_MAX_ATTEMPTS) + return None async def _assign_workspace_role_via_api(workspace_id: str, role_id: str, user_id: str, admin_token: str, verify: bool, role_name: str) -> None: From d4ab72dade0d21734a3838808871f22b75daada2 Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Fri, 28 Nov 2025 17:30:29 +0000 Subject: [PATCH 28/29] format --- e2e_tests/helpers.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/e2e_tests/helpers.py b/e2e_tests/helpers.py index 62c6bab45a..8a3f6bd058 100644 --- a/e2e_tests/helpers.py +++ b/e2e_tests/helpers.py @@ -172,23 +172,23 @@ async def _get_workspace_role_id(workspace_id: str, admin_token: str, verify: bo """Retrieve a workspace role ID, retrying on 404 to allow time for Entra ID role propagation.""" async with AsyncClient(verify=verify, timeout=TIMEOUT) as client: headers = get_auth_header(admin_token) - + for attempt in range(ROLE_ASSIGNMENT_MAX_ATTEMPTS): response = await client.get(get_full_endpoint(f"/api/workspaces/{workspace_id}/roles"), headers=headers, timeout=TIMEOUT) - + if response.status_code == status.HTTP_404_NOT_FOUND: LOGGER.info("Workspace roles not yet available for %s (%s/%s)", workspace_id, attempt + 1, ROLE_ASSIGNMENT_MAX_ATTEMPTS) await asyncio.sleep(ROLE_ASSIGNMENT_SLEEP_SECONDS) continue - + assert_status(response, [status.HTTP_200_OK], f"Failed to get workspace roles for {workspace_id}") - + for role in response.json().get("roles", []): if role.get("displayName") == role_name: return role.get("id") - + return None - + LOGGER.warning("Workspace roles never became available for %s after %s attempts", workspace_id, ROLE_ASSIGNMENT_MAX_ATTEMPTS) return None From c56c3ab76c6c55799cbc62755bd363308e9f975c Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Thu, 8 Jan 2026 10:50:05 +0000 Subject: [PATCH 29/29] Udate to 2.2.27-3ubuntu2.5 --- core/terraform/resource_processor/vmss_porter/cloud-config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/terraform/resource_processor/vmss_porter/cloud-config.yaml b/core/terraform/resource_processor/vmss_porter/cloud-config.yaml index d7f509e974..6415a14f71 100644 --- a/core/terraform/resource_processor/vmss_porter/cloud-config.yaml +++ b/core/terraform/resource_processor/vmss_porter/cloud-config.yaml @@ -20,7 +20,7 @@ packages: - [containerd.io, 1.7.27-1] - [docker-compose, 1.29.2-1] - [azure-cli, 2.74.0-1~jammy] - - [gnupg2, 2.2.27-3ubuntu2.4] + - [gnupg2, 2.2.27-3ubuntu2.5] - [pass, 1.7.4-5] # create the docker group