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/linters/.tflint.hcl b/.github/linters/.tflint.hcl index 623facb90e..6ad0c77c8a 100644 --- a/.github/linters/.tflint.hcl +++ b/.github/linters/.tflint.hcl @@ -1,5 +1,5 @@ config { - call_module_type = "all" + call_module_type = "local" force = false } @@ -36,3 +36,8 @@ rule "terraform_standard_module_structure" { rule "terraform_required_version" { enabled = false } + +# Disabled: Workspace secrets have a normal lifecycle and need to be deleted with the workspace +rule "azurerm_resources_missing_prevent_destroy" { + enabled = false +} diff --git a/.github/linters/.tflint_workspaces.hcl b/.github/linters/.tflint_workspaces.hcl index ddcd381282..72d36d3659 100644 --- a/.github/linters/.tflint_workspaces.hcl +++ b/.github/linters/.tflint_workspaces.hcl @@ -1,12 +1,14 @@ # This is used for TRE tags validation only. config { - call_module_type = "all" + call_module_type = "local" force = false } plugin "azurerm" { enabled = true + version = "0.30.0" + source = "github.com/terraform-linters/tflint-ruleset-azurerm" } rule "terraform_typed_variables" { @@ -17,3 +19,8 @@ rule "azurerm_resource_missing_tags" { enabled = true tags = ["tre_id", "tre_workspace_id"] } + +# Disabled: Workspace secrets have a normal lifecycle and need to be deleted with the workspace +rule "azurerm_resources_missing_prevent_destroy" { + enabled = false +} 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 50bb55942f..65e4968558 100644 --- a/.github/workflows/deploy_tre.yml +++ b/.github/workflows/deploy_tre.yml @@ -53,8 +53,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 7efa2be6ce..54420f6d1c 100644 --- a/.github/workflows/deploy_tre_branch.yml +++ b/.github/workflows/deploy_tre_branch.yml @@ -86,8 +86,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 09e6959c17..c10d57e562 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 @@ -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 }} @@ -913,6 +897,6 @@ jobs: - name: Publish E2E Test Results uses: EnricoMi/publish-unit-test-result-action@v2 with: - junit_files: "artifacts/**/*.xml" + files: "artifacts/**/*.xml" check_name: "E2E Test Results" comment_mode: off diff --git a/.github/workflows/pr_comment_bot.yml b/.github/workflows/pr_comment_bot.yml index e7c4babd5b..cab2d90c0e 100644 --- a/.github/workflows/pr_comment_bot.yml +++ b/.github/workflows/pr_comment_bot.yml @@ -152,7 +152,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: @@ -167,6 +168,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 @@ -183,8 +185,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/CHANGELOG.md b/CHANGELOG.md index d2b4b6f134..79269b1ec2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,13 +2,33 @@ ## 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 3.0.0 (major upgrade from 2.8.0) now creates and manages 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_*`). + + **Migration Guide:** + 1. **Existing Workspaces:** Continue to operate without changes; + 2. **New Workspaces:** + - No `client_secret` parameter needed + - Optionally provide `client_id` to reuse pre-existing application + - Leave `client_id` empty for fully automatic application creation + 3. **Upgrading Workspaces:** + - Only upgrade once you have tested the process in a non-production environment with your own bundles. + - Ensure Application Admin identity owns existing workspace applications + - Run workspace upgrade - Terraform will import and take over secret management + + **Permission Changes:** + - **Removed:** `Directory.Read.All` no longer required + - **Keep (depending on requirements):** `Application.ReadWrite.All` (or `Application.ReadWrite.OwnedBy`), `Group.Create`, `Group.Read.All`, `User.ReadBasic.All`, `DelegatedPermissionGrant.ReadWrite.All` + + ([#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)) +* Clarify cost label time period and aggregation scope in UI tooltips ([#4607](https://github.com/microsoft/AzureTRE/pull/4607)) * Update Porter, AzureCLI, Terraform and its providers across the solution ([#4799](https://github.com/microsoft/AzureTRE/issues/4799)) * Update `api_healthcheck.sh` script with fixed 10-second check intervals and 7-minute timeout for improved API health monitoring ([#4807](https://github.com/microsoft/AzureTRE/issues/4807)) * Update SuperLinter to version 8.3.2 ([#4815](https://github.com/microsoft/AzureTRE/issues/4815)) diff --git a/api_app/_version.py b/api_app/_version.py index 545cb33740..55b3ef6040 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.25.12" +__version__ = "0.26.3" diff --git a/api_app/api/routes/workspace_users.py b/api_app/api/routes/workspace_users.py index b90a6b07ea..3e63e2d6cc 100644 --- a/api_app/api/routes/workspace_users.py +++ b/api_app/api/routes/workspace_users.py @@ -31,7 +31,6 @@ async def get_workspace_roles(workspace=Depends(get_workspace_by_id_from_path), @workspaces_users_admin_router.post("/workspaces/{workspace_id}/users/assign", status_code=status.HTTP_202_ACCEPTED, name=strings.API_ASSIGN_WORKSPACE_USER) async def assign_workspace_user(response: Response, userRoleAssignmentRequest: UserRoleAssignmentRequest, workspace=Depends(get_workspace_by_id_from_path), access_service=Depends(get_access_service)) -> WorkspaceUserOperationResponse: - for user_id in userRoleAssignmentRequest.user_ids: access_service.assign_workspace_user( user_id, diff --git a/api_app/api/routes/workspaces.py b/api_app/api/routes/workspaces.py index 4f8ee42bb2..cc7f90b34a 100644 --- a/api_app/api/routes/workspaces.py +++ b/api_app/api/routes/workspaces.py @@ -24,13 +24,12 @@ from models.schemas.resource_template import ResourceTemplateInformationInList from resources import strings from services.access_service import AuthConfigValidationError -from services.authentication import get_current_admin_user, \ +from services.authentication import extract_auth_information, get_current_admin_user, \ get_access_service, get_current_workspace_owner_user, get_current_workspace_owner_or_researcher_user, get_current_tre_user_or_tre_admin, \ get_current_workspace_owner_or_tre_admin, \ 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,7 +98,6 @@ 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) except (ValidationError, ValueError) as e: diff --git a/api_app/db/repositories/workspaces.py b/api_app/db/repositories/workspaces.py index 013c1d54ae..3a4d43ec00 100644 --- a/api_app/db/repositories/workspaces.py +++ b/api_app/db/repositories/workspaces.py @@ -105,7 +105,6 @@ async def create_workspace_item(self, workspace_input: WorkspaceInCreate, auth_i 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, @@ -113,7 +112,6 @@ async def create_workspace_item(self, workspace_input: WorkspaceInCreate, auth_i resource_spec_parameters = {**workspace_input.properties, **address_space_param, **address_spaces_param, - **auto_app_registration_param, **workspace_owner_param, **auth_info, **self.get_workspace_spec_params(full_workspace_id)} @@ -135,9 +133,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/models/schemas/airlock_request.py b/api_app/models/schemas/airlock_request.py index 0cc1ccbe42..87ddf79e10 100644 --- a/api_app/models/schemas/airlock_request.py +++ b/api_app/models/schemas/airlock_request.py @@ -113,7 +113,7 @@ class AirlockReviewInCreate(BaseModel): class Config: schema_extra = { "example": { - "approval": "True", + "approval": True, "decisionExplanation": "the reason why this request was approved/rejected" } } 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/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/api_app/resources/strings.py b/api_app/resources/strings.py index c54a40ba52..b12d47679c 100644 --- a/api_app/resources/strings.py +++ b/api_app/resources/strings.py @@ -112,6 +112,7 @@ ACCESS_UNABLE_TO_GET_INFO_FOR_APP = "Unable to get app info for app:" ACCESS_UNABLE_TO_GET_ROLE_ASSIGNMENTS_FOR_USER = "Unable to get role assignments for user" ACCESS_UNABLE_TO_GET_ACCOUNT_TYPE = "Unable to look up account type" +ACCESS_MS_GRAPH_QUERY_FAILED = "Microsoft Graph query failed" ACCESS_UNHANDLED_ACCOUNT_TYPE = "Unhandled account type" ACCESS_USER_IS_NOT_OWNER_OR_RESEARCHER = "Workspace Researcher or Owner rights are required" diff --git a/api_app/services/aad_authentication.py b/api_app/services/aad_authentication.py index 4363ea3009..0e59bfa481 100644 --- a/api_app/services/aad_authentication.py +++ b/api_app/services/aad_authentication.py @@ -29,6 +29,16 @@ USER_MANAGEMENT_MINIMUM_BASE_TEMPLATE_VERSION = "2.1.0" +def _is_valid_aad_property(value) -> bool: + if value is None: + return False + if isinstance(value, dict): + return False + if isinstance(value, str) and value.strip() == '': + return False + return True + + class PrincipalType(Enum): User = "User" Group = "Group" @@ -345,8 +355,23 @@ def get_assignable_users(self, filter: str = "", maxResultCount: int = 5) -> Lis return result def get_workspace_roles(self, workspace: Workspace) -> List[Role]: - app_roles_endpoint = f"{MICROSOFT_GRAPH_URL}/v1.0/servicePrincipals/{workspace.properties['sp_id']}/appRoles" - graph_data = self._ms_graph_query(app_roles_endpoint, "GET") + sp_id = workspace.properties.get('sp_id') + if not _is_valid_aad_property(sp_id): + logger.error(f"Workspace {workspace.id} has invalid sp_id: {sp_id!r}. The workspace may not have deployed correctly or is using an older template without automatic AAD registration.") + raise HTTPException( + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + detail=f"Workspace {workspace.id} has invalid service principal configuration. Please check the workspace deployment or upgrade to a template version that supports automatic AAD registration." + ) + + app_roles_endpoint = f"{MICROSOFT_GRAPH_URL}/v1.0/servicePrincipals/{sp_id}/appRoles" + graph_data = self._ms_graph_query(app_roles_endpoint, "GET", raise_on_error=True) + + if "value" not in graph_data: + logger.error(f"MS Graph response missing 'value' for workspace {workspace.id}") + raise HTTPException( + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + detail=strings.ACCESS_MS_GRAPH_QUERY_FAILED + ) roles = [] @@ -362,16 +387,69 @@ def get_workspace_roles(self, workspace: Workspace) -> List[Role]: return roles def assign_workspace_user(self, user_id: str, workspace: Workspace, role_id: str) -> None: - # User already has the role, do nothing - if self._is_user_in_role(user_id, role_id): + """ + Assign a principal to a workspace role. + + If workspace has AAD groups configured, tries group membership first. + Falls back to direct app role assignment if group assignment fails + (e.g., for service principals) or if groups are not configured. + """ + groups_in_use = self._is_workspace_role_group_in_use(workspace) + + if groups_in_use: + # Try group assignment first (works for users) + try: + if workspace.templateName == "tre-workspace-base" and compare_versions(workspace.templateVersion, USER_MANAGEMENT_MINIMUM_BASE_TEMPLATE_VERSION) < 0: + logger.error(f"Unable to assign user {user_id} to group with role {role_id}, Workspace needs to be version {USER_MANAGEMENT_MINIMUM_BASE_TEMPLATE_VERSION} or greater") + raise UserRoleAssignmentError(f"Unable to assign user {user_id} to group with role {role_id}, Workspace needs to be version {USER_MANAGEMENT_MINIMUM_BASE_TEMPLATE_VERSION} or greater") + self._assign_workspace_user_to_application_group(user_id, workspace, role_id) + return + except UserRoleAssignmentError: + # Re-raise UserRoleAssignmentError (version check, group not found, etc.) + raise + except Exception as e: + # Group assignment failed (likely a service principal), fall through to direct assignment + logger.info(f"Group assignment failed for {user_id}: {e}. Using direct app role assignment.") + + # Direct app role assignment for service principals or when no groups configured + return self._assign_principal_to_app_role_direct(user_id, workspace, role_id) + + def _assign_principal_to_app_role_direct(self, principal_id: str, workspace: Workspace, role_id: str) -> None: + """ + Assign a principal directly to an app role via Graph API. + This bypasses group membership and propagates to tokens immediately. + """ + sp_id = workspace.properties.get("sp_id") + if not _is_valid_aad_property(sp_id): + raise UserRoleAssignmentError(f"Workspace {workspace.id} has invalid service principal configuration.") + + url = f"{MICROSOFT_GRAPH_URL}/v1.0/servicePrincipals/{sp_id}/appRoleAssignedTo" + body = { + "principalId": principal_id, + "resourceId": sp_id, + "appRoleId": role_id + } + + msgraph_token = self._get_msgraph_token() + auth_headers = self._get_auth_header(msgraph_token) + response = requests.post(url, json=body, headers=auth_headers, timeout=GRAPH_REQUEST_TIMEOUT) + + if response.status_code == 201: + logger.info(f"Successfully assigned principal {principal_id} directly to app role {role_id}") return - if compare_versions(workspace.templateVersion, USER_MANAGEMENT_MINIMUM_BASE_TEMPLATE_VERSION) < 0: - logger.error(f"Unable to assign user {user_id} to group with role {role_id}, Workspace needs to be version 2.2.0 or greater") - raise UserRoleAssignmentError(f"Unable to assign user {user_id} to group with role {role_id}, Workspace needs to be version 2.2.0 or greater") - if not self._is_workspace_role_group_in_use(workspace): - logger.error(f"Unable to assign user {user_id} to group with role {role_id}, Entra ID groups are not in use on this workspace") - raise UserRoleAssignmentError(f"Unable to assign user {user_id} to group with role {role_id}, Entra ID groups are not in use on this workspace") - return self._assign_workspace_user_to_application_group(user_id, workspace, role_id) + + if response.status_code == 400: + try: + error_data = response.json() + error_message = error_data.get("error", {}).get("message", "") + if "already exist" in error_message: + logger.info(f"Principal {principal_id} already has app role {role_id}") + return + except Exception: + pass + + logger.error(f"Direct app role assignment failed: {response.status_code} - {response.text}") + raise UserRoleAssignmentError(f"Failed to assign principal {principal_id} to role {role_id}: {response.status_code}") 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" @@ -387,13 +465,19 @@ def _get_workspace_group_name(self, workspace: Workspace, role_id: str) -> tuple workspace_id = workspace.properties["workspace_id"] group_name = "" app_role_id_suffix = "" - if workspace.properties["app_role_id_workspace_researcher"] == role_id: + + # Get app role IDs, handling old bundles that may have empty/invalid values + researcher_role = workspace.properties.get("app_role_id_workspace_researcher") + owner_role = workspace.properties.get("app_role_id_workspace_owner") + airlock_role = workspace.properties.get("app_role_id_workspace_airlock_manager") + + if _is_valid_aad_property(researcher_role) and researcher_role == role_id: group_name = "Workspace Researchers" app_role_id_suffix = "workspace_researcher" - elif workspace.properties["app_role_id_workspace_owner"] == role_id: + elif _is_valid_aad_property(owner_role) and owner_role == role_id: group_name = "Workspace Owners" app_role_id_suffix = "workspace_owner" - elif workspace.properties["app_role_id_workspace_airlock_manager"] == role_id: + elif _is_valid_aad_property(airlock_role) and airlock_role == role_id: group_name = "Airlock Managers" app_role_id_suffix = "workspace_airlock_manager" else: @@ -402,7 +486,10 @@ def _get_workspace_group_name(self, workspace: Workspace, role_id: str) -> tuple return (f"{tre_id}-ws-{workspace_id} {group_name}", f"app_role_id_{app_role_id_suffix}") def _assign_workspace_user_to_application_group(self, user_id: str, workspace: Workspace, role_id: str): - roles_graph_data = self._get_user_role_assignments(workspace.properties["sp_id"]) + sp_id = workspace.properties.get("sp_id") + if not _is_valid_aad_property(sp_id): + raise UserRoleAssignmentError(f"Workspace {workspace.id} has invalid service principal configuration. Cannot assign user to role.") + roles_graph_data = self._get_user_role_assignments(sp_id) group_details = self._get_workspace_group_name(workspace, role_id) group_name = group_details[0] workspace_app_role_field = group_details[1] @@ -415,7 +502,10 @@ def _assign_workspace_user_to_application_group(self, user_id: str, workspace: W raise UserRoleAssignmentError(f"Unable to assign user to group with role: {role_id}") def _remove_workspace_user_from_application_group(self, user_id: str, workspace: Workspace, role_id: str): - roles_graph_data = self._get_user_role_assignments(workspace.properties["sp_id"]) + sp_id = workspace.properties.get("sp_id") + if not _is_valid_aad_property(sp_id): + raise UserRoleAssignmentError(f"Workspace {workspace.id} has invalid service principal configuration. Cannot remove user from role.") + roles_graph_data = self._get_user_role_assignments(sp_id) group_details = self._get_workspace_group_name(workspace, role_id) group_name = group_details[0] workspace_app_role_field = group_details[1] @@ -428,8 +518,9 @@ def _remove_workspace_user_from_application_group(self, user_id: str, workspace: def _add_user_to_group(self, user_id: str, group_id: str): url = f"{MICROSOFT_GRAPH_URL}/v1.0/groups/{group_id}/members/$ref" + # Use directoryObjects which works for both users and service principals body = { - "@odata.id": f"{MICROSOFT_GRAPH_URL}/v1.0/users/{user_id}" + "@odata.id": f"{MICROSOFT_GRAPH_URL}/v1.0/directoryObjects/{user_id}" } response = self._ms_graph_query(url, "POST", json=body) @@ -452,9 +543,9 @@ def remove_workspace_role_user_assignment(self, role_id: str, workspace: Workspace ) -> None: - if compare_versions(workspace.templateVersion, USER_MANAGEMENT_MINIMUM_BASE_TEMPLATE_VERSION) < 0: - logger.error(f"Unable to remove user {user_id} from group with role {role_id}, Workspace needs to be version 2.2.0 or greater") - raise UserRoleAssignmentError(f"Unable to remove user {user_id} from group with role {role_id}, Workspace needs to be version 2.2.0 or greater") + if workspace.templateName == "tre-workspace-base" and compare_versions(workspace.templateVersion, USER_MANAGEMENT_MINIMUM_BASE_TEMPLATE_VERSION) < 0: + logger.error(f"Unable to remove user {user_id} from group with role {role_id}, Workspace needs to be version {USER_MANAGEMENT_MINIMUM_BASE_TEMPLATE_VERSION} or greater") + raise UserRoleAssignmentError(f"Unable to remove user {user_id} from group with role {role_id}, Workspace needs to be version {USER_MANAGEMENT_MINIMUM_BASE_TEMPLATE_VERSION} or greater") if not self._is_workspace_role_group_in_use(workspace): logger.error(f"Unable to remove user {user_id} from group with role {role_id}, Entra ID groups are not in use on this workspace") raise UserRoleAssignmentError(f"Unable to remove user {user_id} from group with role {role_id}, Entra ID groups are not in use on this workspace") @@ -480,9 +571,8 @@ 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. + # DEPRECATED: Remove when workspace base bundles < 3.0.0 are no longer supported. + # This method is only needed for auth_type=Manual which requires Directory.Read.All. 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: @@ -499,10 +589,11 @@ def _get_app_auth_info(self, client_id: str) -> dict: return authInfo - def _ms_graph_query(self, url: str, http_method: str, json=None) -> dict: + def _ms_graph_query(self, url: str, http_method: str, json=None, raise_on_error: bool = False) -> dict: msgraph_token = self._get_msgraph_token() auth_headers = self._get_auth_header(msgraph_token) graph_data = {} + original_url = url while True: if not url: break @@ -517,9 +608,17 @@ def _ms_graph_query(self, url: str, http_method: str, json=None) -> dict: graph_data = merge_dict(graph_data, json_response) if '@odata.nextLink' in json_response: url = json_response['@odata.nextLink'] + elif response.status_code == 204: + # 204 No Content is a success response for POST/DELETE operations + pass else: - logger.error(f"MS Graph query to: {url} failed with status code {response.status_code}") - logger.error(f"Full response: {response}") + logger.error(f"MS Graph query to: {original_url} failed with status code {response.status_code}") + logger.error(f"Full response: {response.text}") + if raise_on_error: + raise HTTPException( + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + detail=f"{strings.ACCESS_MS_GRAPH_QUERY_FAILED}: {response.status_code}" + ) return graph_data def _get_role_assignment_graph_data_for_user(self, user_id: str) -> dict: @@ -550,14 +649,27 @@ def _get_identity_type(self, id: str) -> str: return object_info["@odata.type"] + # DEPRECATED: Remove when workspace base bundles < 3.0.0 are no longer supported. + # New bundles handle AAD app registration entirely in Terraform. 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): + # New bundles (v3.0.0+) don't use auth_type - they handle AAD in Terraform + if "auth_type" not in data: + return {} + + if 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": + if data["auth_type"] == "Automatic": + # Old bundles with auth_type=Automatic need register_aad_application=true + # so their Terraform knows to create the AAD app + auth_info["register_aad_application"] = True + else: + # auth_type=Manual - look up existing app via Graph API + logger.warning( + "DEPRECATION: auth_type=Manual requires Application.Read.All permission. " + "Upgrade to base workspace bundle v3.0.0+ which handles AAD registration in Terraform." + ) auth_info = self._get_app_auth_info(data["client_id"]) # Check we've get all our required roles diff --git a/api_app/services/access_service.py b/api_app/services/access_service.py index d38d26ce15..716e9d3cfb 100644 --- a/api_app/services/access_service.py +++ b/api_app/services/access_service.py @@ -15,6 +15,7 @@ class UserRoleAssignmentError(Exception): class AccessService(OAuth2AuthorizationCodeBearer): + # DEPRECATED: Remove when workspace base bundles < 3.0.0 are no longer supported. @abstractmethod def extract_workspace_auth_information(self, data: dict) -> dict: pass diff --git a/api_app/services/authentication.py b/api_app/services/authentication.py index 30b49af194..167beba5c0 100644 --- a/api_app/services/authentication.py +++ b/api_app/services/authentication.py @@ -7,6 +7,8 @@ from services.access_service import AccessService, AuthConfigValidationError +# DEPRECATED: Remove when workspace base bundles < 3.0.0 are no longer supported. +# New bundles handle AAD app registration entirely in Terraform. def extract_auth_information(workspace_creation_properties: dict) -> dict: access_service = get_access_service('AAD') try: 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..d13953d32c 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 e71c98cd12..0d42ba7938 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 @@ -288,18 +288,6 @@ async def test_create_workspace_item_raises_value_error_if_template_is_invalid(m 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/api_app/tests_ma/test_services/test_aad_access_service.py b/api_app/tests_ma/test_services/test_aad_access_service.py index aa5f1650bb..e69e7de362 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 @@ -313,7 +313,7 @@ def test_get_workspace_user_emails_by_role_assignment_with_groups_and_users_assi 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"}) + access_service.extract_workspace_auth_information(data={"auth_type": "Manual", "client_id": "1234"}) @patch( @@ -325,7 +325,7 @@ def test_extract_workspace__raises_error_if_researcher_not_in_roles( ): access_service = AzureADAuthorization() with pytest.raises(AuthConfigValidationError): - access_service.extract_workspace_auth_information(data={"client_id": "1234"}) + access_service.extract_workspace_auth_information(data={"auth_type": "Manual", "client_id": "1234"}) @patch( @@ -337,7 +337,7 @@ def test_extract_workspace__raises_error_if_graph_data_is_invalid( ): access_service = AzureADAuthorization() with pytest.raises(AuthConfigValidationError): - access_service.extract_workspace_auth_information(data={"client_id": "1234"}) + access_service.extract_workspace_auth_information(data={"auth_type": "Manual", "client_id": "1234"}) @patch("services.aad_authentication.AzureADAuthorization._get_app_sp_graph_data") @@ -647,6 +647,33 @@ 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._ms_graph_query") +def test_is_user_in_role_returns_true_when_role_exists(mock_ms_graph_query): + mock_ms_graph_query.return_value = {"value": [{"appRoleId": "role-a"}]} + access_service = AzureADAuthorization() + + assert access_service._is_user_in_role("user-obj", "role-a") is True + mock_ms_graph_query.assert_called_once() + + +@patch("services.aad_authentication.AzureADAuthorization._ms_graph_query") +def test_is_user_in_role_returns_false_when_role_not_exists(mock_ms_graph_query): + mock_ms_graph_query.return_value = {"value": [{"appRoleId": "role-a"}]} + access_service = AzureADAuthorization() + + assert access_service._is_user_in_role("user-obj", "role-b") is False + mock_ms_graph_query.assert_called_once() + + +@patch("services.aad_authentication.AzureADAuthorization._ms_graph_query") +def test_is_user_in_role_handles_empty_value(mock_ms_graph_query): + mock_ms_graph_query.return_value = {"value": []} + access_service = AzureADAuthorization() + + assert access_service._is_user_in_role("user-obj", "role-c") is False + mock_ms_graph_query.assert_called_once() + + def get_mock_batch_response(user_principals, group_principals): response_body = {"responses": []} for user_principal in user_principals: @@ -708,39 +735,43 @@ def get_mock_role_response(principal_roles): return response -@patch("services.aad_authentication.AzureADAuthorization._is_user_in_role", return_value=True) -@patch("services.aad_authentication.AzureADAuthorization._is_workspace_role_group_in_use") -@patch("services.aad_authentication.AzureADAuthorization._assign_workspace_user_to_application_group") -def test_assign_workspace_user_already_has_role(workspace_role_in_use_mock, - assign_user_to_group_mock, - workspace_without_groups, role_owner, - user_with_role): +@patch("services.aad_authentication.AzureADAuthorization._assign_principal_to_app_role_direct") +@patch("services.aad_authentication.AzureADAuthorization._is_workspace_role_group_in_use", return_value=False) +def test_assign_workspace_user_if_no_groups_uses_direct_assignment(workspace_role_in_use_mock, + direct_assign_mock, + workspace_without_groups, role_owner, + user_with_role): + """When no groups configured, should use direct app role assignment.""" access_service = AzureADAuthorization() access_service.assign_workspace_user(user_with_role.id, workspace_without_groups, role_owner.id) - assert workspace_role_in_use_mock.call_count == 0 - assert assign_user_to_group_mock.call_count == 0 - + assert direct_assign_mock.call_count == 1 -@patch("services.aad_authentication.AzureADAuthorization._is_user_in_role", return_value=False) -@patch("services.aad_authentication.AzureADAuthorization._is_workspace_role_group_in_use", return_value=False) -@patch("services.aad_authentication.AzureADAuthorization._assign_workspace_user_to_application_group") -def test_assign_workspace_user_if_no_groups_raises_error(_, __, ___, workspace_without_groups, role_owner, - user_with_role): +@patch("services.aad_authentication.AzureADAuthorization._assign_principal_to_app_role_direct") +@patch("services.aad_authentication.AzureADAuthorization._assign_workspace_user_to_application_group", side_effect=Exception("Group add failed")) +@patch("services.aad_authentication.AzureADAuthorization._is_workspace_role_group_in_use", return_value=True) +def test_assign_workspace_user_fallback_to_direct_on_group_failure(workspace_role_in_use_mock, + group_assign_mock, + direct_assign_mock, + workspace_without_groups, role_owner, + user_with_role): + """When group assignment fails (e.g., for service principals), should fall back to direct assignment.""" access_service = AzureADAuthorization() + access_service.assign_workspace_user(user_with_role.id, workspace_without_groups, role_owner.id) - with pytest.raises(UserRoleAssignmentError): - access_service.assign_workspace_user(user_with_role.id, workspace_without_groups, role_owner.id) + # Should try group first, then fall back to direct + assert group_assign_mock.call_count == 1 + assert direct_assign_mock.call_count == 1 -@patch("services.aad_authentication.AzureADAuthorization._is_user_in_role", return_value=False) @patch("services.aad_authentication.AzureADAuthorization._is_workspace_role_group_in_use", return_value=True) @patch("services.aad_authentication.AzureADAuthorization._assign_workspace_user_to_application_group") -def test_assign_workspace_user_if_groups(_, __, assign_user_to_group_mock, +def test_assign_workspace_user_if_groups(assign_user_to_group_mock, + workspace_role_in_use_mock, workspace_without_groups, role_owner, user_with_role): - + """Users should use group assignment when groups are configured.""" access_service = AzureADAuthorization() access_service.assign_workspace_user(user_with_role.id, workspace_without_groups, role_owner.id) 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/config_schema.json b/config_schema.json index abfaf97217..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": { @@ -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 old mode 100644 new mode 100755 index 4b2ffa8347..a03316c69b --- a/core/terraform/outputs.sh +++ b/core/terraform/outputs.sh @@ -30,12 +30,9 @@ 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 -echo "TEST_WORKSPACE_APP_SECRET='${WORKSPACE_API_CLIENT_SECRET}'" >> ../private.env - # 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 diff --git a/core/version.txt b/core/version.txt index 24d361527f..fd86b3ee91 100644 --- a/core/version.txt +++ b/core/version.txt @@ -1 +1 @@ -__version__ = "0.16.12" +__version__ = "0.17.0" 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 2495731240..698dc4e566 100755 --- a/devops/scripts/aad/create_workspace_application.sh +++ b/devops/scripts/aad/create_workspace_application.sh @@ -8,21 +8,17 @@ 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. -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 +37,15 @@ 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 appObjectId="" declare applicationAdminClientId="" declare applicationAdminObjectId="" +declare currentUserId="" +declare msGraphUri="" + # Initialize parameters specified from command line while [[ $# -gt 0 ]]; do case "$1" in @@ -59,25 +53,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 +65,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 +73,41 @@ 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 +115,22 @@ 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}\"" -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 +# Output the application details +echo +echo "==========================================" +echo "Workspace Application Client ID: ${workspaceAppId}" +echo "==========================================" +echo + diff --git a/devops/scripts/aad/create_workspace_application_legacy.sh b/devops/scripts/aad/create_workspace_application_legacy.sh new file mode 100755 index 0000000000..f3fb6895ac --- /dev/null +++ b/devops/scripts/aad/create_workspace_application_legacy.sh @@ -0,0 +1,377 @@ +#!/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) + +# ============================================================================= +# DEPRECATED: This script is for workspace bundles < v3.0.0 that use auth_type. +# For workspace bundles v3.0.0+, use create_workspace_application.sh instead. +# This script will be removed when bundles < v3.0.0 are no longer supported. +# ============================================================================= + +function show_usage() +{ + cat << USAGE + +DEPRECATED: Use create_workspace_application.sh for workspace bundles v3.0.0+. + +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. + +Usage: $0 [--admin-consent] + +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 + +USAGE + exit 2 +} + +if ! command -v az &> /dev/null; then + echo "This script requires Azure CLI" 1>&2 + exit 1 +fi + +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 + +# 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 applicationAdminClientId="" +declare applicationAdminObjectId="" + +# Initialize parameters specified from command line +while [[ $# -gt 0 ]]; do + case "$1" in + -n|--name) + 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 + ;; + *) + echo "Invalid option: $1." + show_usage + ;; + esac +done + +################################### +# CHECK INCOMMING PARAMETERS # +################################### +if [[ -z "$appName" ]]; then + echo "Please specify the application name." 1>&2 + show_usage +fi +appName="$appName API" +if [[ -z "$applicationAdminClientId" ]]; then + echo "Please specify the client id of the Application Admin." 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) +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" + +# Load in 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 + 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 +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 +{ + "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": [] + } +} +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. + wait_for_new_app_registration "${workspaceAppId}" + + # Update to set the identifier URI. + az ad app update --id "${workspaceAppId}" --identifier-uris "api://${workspaceAppId}" --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 +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 + +echo "workspace_api_client_id=\"${workspaceAppId}\"" +echo "workspace_api_client_secret=\"${spPassword}\"" + +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/create_aad_assets.sh b/devops/scripts/create_aad_assets.sh index ff5727ba37..01260884f6 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 @@ -50,6 +50,7 @@ fi APPLICATION_PERMISSION=$(IFS=,; echo "${APPLICATION_PERMISSIONS[*]}") # Create the identity that is able to administer other applications +# shellcheck disable=SC2153 "$DIR/aad/create_application_administrator.sh" \ --name "${TRE_ID}" \ --admin-consent \ @@ -57,6 +58,7 @@ APPLICATION_PERMISSION=$(IFS=,; echo "${APPLICATION_PERMISSIONS[*]}") --reset-password $RESET_PASSWORDS # Create the identity that is able to automate the testing +# shellcheck disable=SC2153 "$DIR/aad/create_automation_administrator.sh" \ --name "${TRE_ID}" \ --reset-password $RESET_PASSWORDS @@ -74,21 +76,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..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}" <Environment variable name | Description | -| ------------------------- | ----------- | -| `TRE_ID` | A globally unique identifier. `TRE_ID` can be found in the resource names of the Azure TRE instance; for example, a `TRE_ID` of `mytre-dev` will result in a resource group name for Azure TRE instance of `rg-mytre-dev`. This must be less than 12 characters. Allowed characters: lowercase alphanumerics| -| `TRE_URL`| This will be generated for you by populating your `TRE_ID`. This is used so that you can automatically register bundles | +| --- | --- | +| `TRE_ID` | A globally unique identifier. `TRE_ID` can be found in the resource names of the Azure TRE instance; for example, a `TRE_ID` of `mytre-dev` will result in a resource group name for Azure TRE instance of `rg-mytre-dev`. This must be less than 12 characters. Allowed characters: lowercase alphanumerics | +| `TRE_URL` | This will be generated for you by populating your `TRE_ID`. This is used so that you can automatically register bundles | | `CORE_ADDRESS_SPACE` | The address space for the Azure TRE core virtual network. `/22` or larger. | -| `TRE_ADDRESS_SPACE` | The address space for the whole TRE environment virtual network where workspaces networks will be created (can include the core network as well). E.g. `10.0.0.0/12`| +| `TRE_ADDRESS_SPACE` | The address space for the whole TRE environment virtual network where workspaces networks will be created (can include the core network as well). E.g. `10.0.0.0/12` | | `ENABLE_SWAGGER` | Determines whether the Swagger interface for the API will be available. | | `SWAGGER_UI_CLIENT_ID` | Generated when following [pre-deployment steps](./setup-instructions/setup-auth-entities.md) guide. Client ID for swagger client to make requests. | | `AAD_TENANT_ID` | Generated when following [pre-deployment steps](./setup-instructions/setup-auth-entities.md) guide. Tenant id against which auth is performed. | @@ -47,30 +47,29 @@ | `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 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. | +| `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 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. | -| `UI_SITE_NAME` | Optional. Overrides the title text shown in top left corner of portal. Default value is: `Azure TRE` | -| `UI_FOOTER_TEXT` | Optional. Overrides the footer text shown in the bottom left corner of the portal. Default value is `Azure Trusted Research Environment` | +| `UI_SITE_NAME` | Optional. Overrides the title text shown in top left corner of portal. Default value is: `Azure TRE` | +| `UI_FOOTER_TEXT` | Optional. Overrides the footer text shown in the bottom left corner of the portal. Default value is `Azure Trusted Research Environment` | ## For authentication in `/config.yaml` - | Variable | Description | - | -------- | ----------- | - | `APPLICATION_ADMIN_CLIENT_ID`| This client will administer Microsoft Entra ID Applications for TRE | - | `APPLICATION_ADMIN_CLIENT_SECRET`| This client will administer Microsoft Entra ID Applications for TRE | - | `TEST_ACCOUNT_CLIENT_ID`| This will be created by default, but can be disabled by editing `/devops/scripts/create_aad_assets.sh`. This is the user that will run the tests for you | - | `TEST_ACCOUNT_CLIENT_SECRET` | This will be created by default, but can be disabled by editing `/devops/scripts/create_aad_assets.sh`. This is the user that will run the tests for you | - | `API_CLIENT_ID` | API application (client) ID. | - | `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.| +| Variable | Description | +| --- | --- | +| `APPLICATION_ADMIN_CLIENT_ID` | This client will administer Microsoft Entra ID Applications for TRE | +| `APPLICATION_ADMIN_CLIENT_SECRET` | This client will administer Microsoft Entra ID Applications for TRE | +| `TEST_ACCOUNT_CLIENT_ID` | This will be created by default, but can be disabled by editing `/devops/scripts/create_aad_assets.sh`. This is the user that will run the tests for you | +| `TEST_ACCOUNT_CLIENT_SECRET` | This will be created by default, but can be disabled by editing `/devops/scripts/create_aad_assets.sh`. This is the user that will run the tests for you | +| `API_CLIENT_ID` | API application (client) ID. | +| `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 | ## For CI/CD pipelines in github environment secrets - | Variable | Description | - | -------- | ----------- | - | `AZURE_CREDENTIALS`| Credentials used to authorize CI/CD workflows to provision resources for the TRE workspaces and workspace services. This is basically your ARM client credentials in json format. Read more about how to create it and its format [here](./setup-instructions/workflows.md##create-a-service principal-for-provisioning-resources)| +| Variable | Description | +| --- | --- | +| `AZURE_CREDENTIALS` | Credentials used to authorize CI/CD workflows to provision resources for the TRE workspaces and workspace services. This is basically your ARM client credentials in json format. Read more about how to create it and its format in the [workflows documentation](./setup-instructions/workflows.md#create-a-service-principal-for-provisioning-resources) | diff --git a/docs/tre-admins/identities/application_admin.md b/docs/tre-admins/identities/application_admin.md index 0d4da35e9a..8686c4e0fd 100644 --- a/docs/tre-admins/identities/application_admin.md +++ b/docs/tre-admins/identities/application_admin.md @@ -8,12 +8,13 @@ This application does not have any roles defined. ## Microsoft Graph Permissions -| Name | Type* | Admin consent required | TRE usage | -| --- | -- | -----| --------- | +| Name | Type* | Admin consent required | TRE usage | +| --- | --- | --- | --- | | 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 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 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. @@ -28,15 +29,15 @@ This user is currently only used from the Porter bundles hosted on the Resource ``` | Argument | Description | -| -------- | ----------- | +| --- | --- | | `--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 example `Application.ReadWrite.All,Directory.Read.All,Group.Create` | +| `--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 | Variable | Description | Location | -| -------- | ----------- | -------- | -|APPLICATION_ADMIN_CLIENT_ID|The Client Id|`./config.yaml`| -|APPLICATION_ADMIN_CLIENT_SECRET|The client secret|`./config.yaml`| +| --- | --- | --- | +| APPLICATION_ADMIN_CLIENT_ID | The Client Id | `./config.yaml` | +| APPLICATION_ADMIN_CLIENT_SECRET | The client secret | `./config.yaml` | diff --git a/docs/tre-admins/identities/workspace.md b/docs/tre-admins/identities/workspace.md index 8bc5b1989a..a64c9393a9 100644 --- a/docs/tre-admins/identities/workspace.md +++ b/docs/tre-admins/identities/workspace.md @@ -6,17 +6,18 @@ Access to workspaces is also controlled using app registrations - one per worksp ## Application Roles | Display name | Description | Allowed member types | Value | -| ------------ | ----------- | -------------------- | ----- | +| --- | --- | --- | --- | | Workspace Owner | Provides workspace owners access to the Workspace. | Users/Groups,Applications | `WorkspaceOwner` | | Workspace Researcher | Provides researchers access to the Workspace. | Users/Groups,Applications | `WorkspaceResearcher` | | 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| -|openid|Delegated|No|Allows users to sign in to the app with their work or school accounts and allows the app to see basic user profile information.| -|profile|Delegated|No|Used to read the user's profile when creating TRE resources| + +| Name | Type* | Admin consent required | TRE usage | +| --- | --- | --- | --- | +| email | Delegated | No | Used to read the user's email address when creating TRE resources | +| openid | Delegated | No | Allows users to sign in to the app with their work or school accounts and allows the app to see basic user profile information. | +| profile | Delegated | No | Used to read the user's profile when creating TRE resources | '*' 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. @@ -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..fa25f66038 100644 --- a/docs/tre-admins/setup-instructions/cicd-pre-deployment-steps.md +++ b/docs/tre-admins/setup-instructions/cicd-pre-deployment-steps.md @@ -73,11 +73,11 @@ Configure the following secrets in your github environment: Configure the following **variables** in your github environment: |
Variable name
| Description | -| ----------- | ----------- | +| --- | --- | | `LOCATION` | The Azure location (region) for all resources. E.g. `westeurope` | | `TERRAFORM_STATE_CONTAINER_NAME` | Optional. The name of the blob container to hold the Terraform state. Default value is `tfstate`. | | `CORE_ADDRESS_SPACE` | Optional. The address space for the Azure TRE core virtual network. Default value is `10.0.0.0/22`. | -| `TRE_ADDRESS_SPACE` | Optional. The address space for the whole TRE environment virtual network where workspaces networks will be created (can include the core network as well). Default value is `10.0.0.0/16`| +| `TRE_ADDRESS_SPACE` | Optional. The address space for the whole TRE environment virtual network where workspaces networks will be created (can include the core network as well). Default value is `10.0.0.0/16` | | `AZURE_ENVIRONMENT` | Optional. The name of the Azure environment. Supported values are `AzureCloud` and `AzureUSGovernment`. Default value is `AzureCloud`. | | `CORE_APP_SERVICE_PLAN_SKU` | Optional. The SKU used for AppService plan for core infrastructure. Default value is `P1v2`. | | `WORKSPACE_APP_SERVICE_PLAN_SKU` | Optional. The SKU used for AppService plan used in E2E tests. Default value is `P1v2`. | @@ -92,18 +92,16 @@ Configure the following **variables** in your github environment: In a previous [Setup Auth configuration](./setup-auth-entities.md) step authentication configuration was added in `config.yaml` file. Go to this file and add those env vars to your github environment: - | Secret Name | Description | - | -------- | ----------- | - | `AAD_TENANT_ID` | Tenant id against which auth is performed. | - | `APPLICATION_ADMIN_CLIENT_ID`| This client will administer Microsoft Entra ID Applications for TRE | - | `APPLICATION_ADMIN_CLIENT_SECRET`| This client will administer Microsoft Entra ID Applications for TRE | - | `TEST_ACCOUNT_CLIENT_ID`| This will be created by default, but can be disabled by editing `/devops/scripts/create_aad_assets.sh`. This is the user that will run the tests for you | - | `TEST_ACCOUNT_CLIENT_SECRET` | This will be created by default, but can be disabled by editing `/devops/scripts/create_aad_assets.sh`. This is the user that will run the tests for you | - | `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 | - | `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| +| Secret Name | Description | +| --- | --- | +| `AAD_TENANT_ID` | Tenant id against which auth is performed. | +| `APPLICATION_ADMIN_CLIENT_ID` | This client will administer Microsoft Entra ID Applications for TRE | +| `APPLICATION_ADMIN_CLIENT_SECRET` | This client will administer Microsoft Entra ID Applications for TRE | +| `TEST_ACCOUNT_CLIENT_ID` | This will be created by default, but can be disabled by editing `/devops/scripts/create_aad_assets.sh`. This is the user that will run the tests for you | +| `TEST_ACCOUNT_CLIENT_SECRET` | This will be created by default, but can be disabled by editing `/devops/scripts/create_aad_assets.sh`. This is the user that will run the tests for you | +| `API_CLIENT_ID` | API application (client) ID. | +| `API_CLIENT_SECRET` | API application client secret. | +| `SWAGGER_UI_CLIENT_ID` | Swagger (OpenAPI) UI application (client) ID. | !!! 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..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,9 +24,9 @@ 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 - - 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. + - 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. @@ -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 a manual 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-admins/setup-instructions/workflows.md b/docs/tre-admins/setup-instructions/workflows.md index a22e59e4b8..30231dd1f2 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: @@ -119,11 +108,11 @@ Configure additional secrets used in the deployment workflow: Configure variables used in the deployment workflow: |
Variable name
| Description | -| ----------- | ----------- | +| --- | --- | | `LOCATION` | The Azure location (region) for all resources. E.g. `westeurope` | | `TERRAFORM_STATE_CONTAINER_NAME` | Optional. The name of the blob container to hold the Terraform state. Default value is `tfstate`. | | `CORE_ADDRESS_SPACE` | Optional. The address space for the Azure TRE core virtual network. Default value is `10.0.0.0/22`. | -| `TRE_ADDRESS_SPACE` | Optional. The address space for the whole TRE environment virtual network where workspaces networks will be created (can include the core network as well). Default value is `10.0.0.0/16`| +| `TRE_ADDRESS_SPACE` | Optional. The address space for the whole TRE environment virtual network where workspaces networks will be created (can include the core network as well). Default value is `10.0.0.0/16` | | `AZURE_ENVIRONMENT` | Optional. The name of the Azure environment. Supported values are `AzureCloud` and `AzureUSGovernment`. Default value is `AzureCloud`. | | `CORE_APP_SERVICE_PLAN_SKU` | Optional. The SKU used for AppService plan for core infrastructure. Default value is `P1v2`. | | `WORKSPACE_APP_SERVICE_PLAN_SKU` | Optional. The SKU used for AppService plan used in E2E tests. Default value is `P1v2`. | diff --git a/docs/tre-developers/end-to-end-tests.md b/docs/tre-developers/end-to-end-tests.md index b17fcf84dd..dd50f2498b 100644 --- a/docs/tre-developers/end-to-end-tests.md +++ b/docs/tre-developers/end-to-end-tests.md @@ -1,10 +1,5 @@ # End-to-end (E2E) tests -## Prerequisites - -1. Authentication and Authorization configuration set up as noted [here](../tre-admins/auth.md) -1. An Azure Tre deployed environment. - ## Registering bundles to run End-to-end tests End-to-end tests depend on certain bundles to be registered within the TRE API. @@ -17,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/docs/tre-templates/workspaces/base.md b/docs/tre-templates/workspaces/base.md index fadea7a60b..498f507b47 100644 --- a/docs/tre-templates/workspaces/base.md +++ b/docs/tre-templates/workspaces/base.md @@ -19,13 +19,17 @@ 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. +*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: - Key Vault: - Azure Storage: + +## Client secret management +The workspace application password is created with a default validity of approximately 2 years. The secret is stored in the workspace Key Vault as `workspace-client-secret` (with the client ID stored as `workspace-client-id`). + +Workspaces should be upgraded periodically (at least every 2 years) to refresh the password. diff --git a/e2e_tests/.env.sample b/e2e_tests/.env.sample index 504651cfda..2a6d5a5e4b 100644 --- a/e2e_tests/.env.sample +++ b/e2e_tests/.env.sample @@ -11,8 +11,6 @@ RESOURCE_LOCATION=westeurope TEST_APP_ID= 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/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/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 39589e1696..081a7531f0 100644 --- a/e2e_tests/conftest.py +++ b/e2e_tests/conftest.py @@ -1,19 +1,28 @@ -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 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__) 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") @@ -21,10 +30,45 @@ 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'") + + +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( @@ -32,9 +76,12 @@ 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 != "": + # Still need to ensure role assignment for pre-created workspaces + admin_token = await get_admin_token(verify=verify) + await ensure_automation_admin_has_workspace_owner_role(pre_created_workspace_id, admin_token, verify) + await ensure_automation_admin_has_airlock_role(pre_created_workspace_id, admin_token, verify) return f"/workspaces/{pre_created_workspace_id}", pre_created_workspace_id LOGGER.info(f"Creating workspace {template_name}") @@ -46,7 +93,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 != "": @@ -54,7 +102,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 @@ -62,6 +109,13 @@ 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") + + # post_resource with wait=True (the default) ensures the workspace is fully deployed + 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 @@ -108,7 +162,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="Automatic", verify=verify, pre_created_workspace_id=pre_created_workspace_id) yield workspace_path, workspace_id @@ -148,6 +202,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/helpers.py b/e2e_tests/helpers.py index 307fb88933..eb8ee5b218 100644 --- a/e2e_tests/helpers.py +++ b/e2e_tests/helpers.py @@ -1,7 +1,11 @@ import asyncio +import base64 +import functools +import json 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,11 +19,26 @@ azlogger = logging.getLogger("azure") azlogger.setLevel(logging.WARN) +ROLE_ASSIGNMENT_MAX_ATTEMPTS = 30 +ROLE_ASSIGNMENT_SLEEP_SECONDS = 10 +ROLE_VERIFICATION_MAX_ATTEMPTS = 12 +ROLE_VERIFICATION_SLEEP_SECONDS = 5 + class InstallFailedException(Exception): pass +def _is_service_principal_auth() -> bool: + """Check if using service principal (client credentials) authentication.""" + return config.TEST_ACCOUNT_CLIENT_ID != "" and config.TEST_ACCOUNT_CLIENT_SECRET != "" + + +def _has_automation_identity() -> bool: + """Check if any automation identity (service principal or user) is configured.""" + return config.TEST_ACCOUNT_CLIENT_ID != "" or config.TEST_USER_NAME != "" + + def read_workspace_id() -> str: with open('workspace_id.txt', 'r') as f: workspace_id = f.readline() @@ -110,13 +129,26 @@ async def get_admin_token(verify) -> str: def get_token(scope_uri, verify) -> str: - if config.TEST_ACCOUNT_CLIENT_ID != "" and config.TEST_ACCOUNT_CLIENT_SECRET != "": + if _is_service_principal_auth(): # Logging in as an Enterprise Application: Use Client Credentials flow - credential = ClientSecretCredential(config.AAD_TENANT_ID, config.TEST_ACCOUNT_CLIENT_ID, config.TEST_ACCOUNT_CLIENT_SECRET, connection_verify=verify, authority=cloud.get_aad_authority_fqdn()) + credential = ClientSecretCredential( + config.AAD_TENANT_ID, + config.TEST_ACCOUNT_CLIENT_ID, + config.TEST_ACCOUNT_CLIENT_SECRET, + connection_verify=verify, + authority=cloud.get_aad_authority_fqdn() + ) token = credential.get_token(f'{scope_uri}/.default') else: # Logging in as a User: Use Resource Owner Password Credentials flow - credential = UsernamePasswordCredential(config.TEST_APP_ID, config.TEST_USER_NAME, config.TEST_USER_PASSWORD, connection_verify=verify, authority=cloud.get_aad_authority_fqdn(), tenant_id=config.AAD_TENANT_ID) + credential = UsernamePasswordCredential( + config.TEST_APP_ID, + config.TEST_USER_NAME, + config.TEST_USER_PASSWORD, + connection_verify=verify, + authority=cloud.get_aad_authority_fqdn(), + tenant_id=config.AAD_TENANT_ID + ) token = credential.get_token(f'{scope_uri}/user_impersonation') return token.token @@ -125,3 +157,300 @@ 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}" + + +def _decode_jwt_payload(token: str) -> dict: + """Decode a JWT token and return the payload as a dictionary.""" + try: + # JWT format: header.payload.signature + parts = token.split('.') + if len(parts) != 3: + return {} + + # Decode the payload (second part) + payload_b64 = parts[1] + # Add padding if needed + padding = 4 - len(payload_b64) % 4 + if padding != 4: + payload_b64 += '=' * padding + + payload_bytes = base64.urlsafe_b64decode(payload_b64) + return json.loads(payload_bytes.decode('utf-8')) + except Exception as e: + LOGGER.warning("Failed to decode JWT: %s", e) + return {} + + +def _get_roles_from_token(token: str) -> List[str]: + """Extract roles from a JWT token.""" + payload = _decode_jwt_payload(token) + roles = payload.get('roles', []) + if isinstance(roles, list): + return roles + return [] + + +def _extract_scope_uri_from_workspace(workspace: dict) -> Optional[str]: + """Extract the scope URI from a workspace dict for token requests.""" + scope_id = workspace.get("properties", {}).get("scope_id", "") + if not scope_id: + return None + # Cope with the fact that scope_id can have api:// at the front + return f"api://{scope_id.replace('api://', '')}" + + +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 not _has_automation_identity(): + LOGGER.info("No automation admin identity configured; skipping %s assignment", role_name) + return + + workspace = await _wait_for_user_management_configuration(workspace_id, admin_token, verify, role_name) + if workspace is None: + 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(verify) + if user_object_id is None: + LOGGER.warning("Unable to determine automation admin directory object id; skipping %s assignment", role_name) + return + + scope_uri = _extract_scope_uri_from_workspace(workspace) + await _assign_workspace_role_via_api(workspace_id, role_id, user_object_id, admin_token, verify, role_name, scope_uri) + + +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/500/503 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 + + if response.status_code in (status.HTTP_500_INTERNAL_SERVER_ERROR, status.HTTP_503_SERVICE_UNAVAILABLE): + LOGGER.info("Workspace roles endpoint returned %s for %s, likely Entra ID propagation delay (%s/%s)", response.status_code, 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 + + +# Map display names to role values used in tokens +ROLE_DISPLAY_TO_VALUE = { + "Airlock Manager": "AirlockManager", + "WorkspaceOwner": "WorkspaceOwner", + "Workspace Owner": "WorkspaceOwner", + "WorkspaceResearcher": "WorkspaceResearcher", + "Workspace Researcher": "WorkspaceResearcher", +} + + +async def _assign_workspace_role_via_api( + workspace_id: str, + role_id: str, + user_id: str, + admin_token: str, + verify: bool, + role_name: str, + scope_uri: Optional[str] +) -> None: + """ + Assign a user/service principal to an app role via the TRE API. + The API automatically uses direct app role assignment for service principals, + which propagates to tokens immediately (no 5-15 min group membership delay). + + After assignment, verifies that the role appears in newly issued tokens + by polling with retry to handle Entra ID propagation delays. + """ + expected_role_value = ROLE_DISPLAY_TO_VALUE.get(role_name, role_name) + + async with AsyncClient(verify=verify, timeout=TIMEOUT) as client: + headers = get_auth_header(admin_token) + body = { + "user_ids": [user_id], + "role_id": role_id + } + response = await client.post( + get_full_endpoint(f"/api/workspaces/{workspace_id}/users/assign"), + headers=headers, + json=body, + timeout=TIMEOUT + ) + + if response.status_code == status.HTTP_202_ACCEPTED: + LOGGER.info("App role assignment succeeded for %s role in workspace %s", role_name, workspace_id) + else: + LOGGER.warning( + "App role assignment failed for %s role in workspace %s: %s - %s", + role_name, workspace_id, response.status_code, response.text + ) + return # Don't attempt verification if assignment failed + + # Verify the role appears in newly issued tokens + if scope_uri is None: + LOGGER.warning("No workspace scope URI for %s; skipping role verification", workspace_id) + await asyncio.sleep(5) # Fall back to simple wait + return + + await _verify_role_in_token(workspace_id, scope_uri, expected_role_value, role_name, verify) + + +async def _verify_role_in_token( + workspace_id: str, + scope_uri: str, + expected_role_value: str, + role_name: str, + verify: bool +) -> None: + """Poll for fresh tokens until the expected role appears.""" + loop = asyncio.get_running_loop() + for attempt in range(ROLE_VERIFICATION_MAX_ATTEMPTS): + # Get a fresh token for the workspace + try: + token = await loop.run_in_executor(None, functools.partial(get_token, scope_uri, verify)) + roles_in_token = _get_roles_from_token(token) + + if expected_role_value in roles_in_token: + LOGGER.info( + "Verified %s role (%s) appears in token for workspace %s (attempt %s/%s)", + role_name, expected_role_value, workspace_id, attempt + 1, ROLE_VERIFICATION_MAX_ATTEMPTS + ) + return + + LOGGER.info( + "Role %s not yet in token for workspace %s, current roles: %s (attempt %s/%s)", + expected_role_value, workspace_id, roles_in_token, attempt + 1, ROLE_VERIFICATION_MAX_ATTEMPTS + ) + except Exception as e: + LOGGER.warning( + "Error getting token for role verification in workspace %s: %s (attempt %s/%s)", + workspace_id, e, attempt + 1, ROLE_VERIFICATION_MAX_ATTEMPTS + ) + + await asyncio.sleep(ROLE_VERIFICATION_SLEEP_SECONDS) + + LOGGER.warning( + "Role %s never appeared in token for workspace %s after %s attempts", + expected_role_value, workspace_id, ROLE_VERIFICATION_MAX_ATTEMPTS + ) + + +async def _get_automation_admin_object_id(verify: bool) -> Optional[str]: + """Get the directory object ID for the automation admin (user or service principal).""" + loop = asyncio.get_running_loop() + return await loop.run_in_executor(None, functools.partial(_get_directory_object_id_via_graph, verify)) + + +async def _wait_for_user_management_configuration(workspace_id: str, admin_token: str, verify: bool, role_name: str) -> Optional[dict]: + """Poll the workspace until Entra ID role/group data is available. Returns workspace dict or None.""" + 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 workspace + + if _workspace_cannot_support_user_management(workspace): + LOGGER.warning("Workspace %s does not support user management; skipping %s assignment", workspace_id, role_name) + return None + + 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 None + + +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=TIMEOUT, verify=verify) as client: + if _is_service_principal_auth(): + 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") diff --git a/e2e_tests/pytest.ini b/e2e_tests/pytest.ini index 3e3cf490e3..798460e5d7 100644 --- a/e2e_tests/pytest.ini +++ b/e2e_tests/pytest.ini @@ -8,6 +8,7 @@ markers = timeout: used to set test timeout with pytest-timeout airlock: only airlock related workspace_services + manual_app: marks tests for manually created workspace applications (not run automatically) asyncio_mode = auto asyncio_default_fixture_loop_scope = session diff --git a/e2e_tests/resources/deployment.py b/e2e_tests/resources/deployment.py index 85db3c81e0..273b6537d9 100644 --- a/e2e_tests/resources/deployment.py +++ b/e2e_tests/resources/deployment.py @@ -9,7 +9,7 @@ async def delete_done(client, operation_endpoint, headers): delete_terminal_states = [strings.RESOURCE_STATUS_DELETED, strings.RESOURCE_STATUS_DELETING_FAILED] - deployment_status, message, operation_steps = await check_deployment(client, operation_endpoint, headers) + deployment_status, message, operation_steps = await check_deployment(client, operation_endpoint, headers, is_delete=True) return (True, deployment_status, message, operation_steps) if deployment_status in delete_terminal_states else (False, deployment_status, message, operation_steps) @@ -28,7 +28,7 @@ async def patch_done(client, operation_endpoint, headers): @backoff.on_exception(backoff.constant, TimeoutException, # catching all timeout types (Connection, Read, etc.) max_time=90) -async def check_deployment(client, operation_endpoint, headers): +async def check_deployment(client, operation_endpoint, headers, is_delete=False): full_endpoint = get_full_endpoint(operation_endpoint) response = await client.get(full_endpoint, headers=headers, timeout=5.0) @@ -38,11 +38,11 @@ async def check_deployment(client, operation_endpoint, headers): message = response_json["operation"]["message"] operation_steps = stringify_operation_steps(response_json["operation"]["steps"]) return deployment_status, message, operation_steps - elif response.status_code == 404: - # 404 indicates the resource has been deleted (filtered out by get_by_id methods) - # This is a valid state for deletion operations - LOGGER.info(f"Resource not found (404) - treating as deleted: {operation_endpoint}") - return strings.RESOURCE_STATUS_DELETED, "Resource has been deleted", "" + elif response.status_code == 404 and is_delete: + # For delete operations, a 404 means the resource (and its operation endpoint) + # was successfully deleted - treat this as a successful deletion + LOGGER.info(f"Got 404 for delete operation endpoint {operation_endpoint}, treating as successful deletion") + return strings.RESOURCE_STATUS_DELETED, "Resource deleted successfully", "" else: LOGGER.error(f"Non 200 response in check_deployment: {response.status_code}") LOGGER.error(f"Full response: {response}") diff --git a/e2e_tests/test_airlock.py b/e2e_tests/test_airlock.py index 051a5c9d81..c49597db11 100644 --- a/e2e_tests/test_airlock.py +++ b/e2e_tests/test_airlock.py @@ -141,7 +141,7 @@ async def test_airlock_review_vm_flow(setup_test_workspace, setup_test_airlock_i # Approve request LOGGER.info("Approving airlock request") payload = { - "approval": "True", + "approval": True, "decisionExplanation": "the reason why this request was approved/rejected" } request_result = await post_request(payload, f'/api{workspace_path}/requests/{request_id}/review', workspace_owner_token, verify, 200) @@ -173,7 +173,7 @@ async def test_airlock_flow(setup_test_workspace, verify) -> None: # 3. approve request LOGGER.info("Approving airlock request") payload = { - "approval": "True", + "approval": True, "decisionExplanation": "the reason why this request was approved/rejected" } request_result = await post_request(payload, f'/api{workspace_path}/requests/{request_id}/review', workspace_owner_token, verify, 200) 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 7fa0208a7c..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,9 +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}", - "client_secret": f"{config.TEST_WORKSPACE_APP_SECRET}" + "auth_type": "Automatic" } } 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 40de3a637f..cafa3ca42d 100644 --- a/templates/workspaces/base/.env.sample +++ b/templates/workspaces/base/.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 ADDRESS_SPACES="WyIxMC4xLjEwLjAvMjQiXQ==" SHARED_STORAGE_QUOTA=50 diff --git a/templates/workspaces/base/parameters.json b/templates/workspaces/base/parameters.json index d4f408174b..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": { @@ -82,42 +76,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 c970a581d9..2624ff7c96 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.1 +version: 3.0.0 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,36 +80,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" @@ -243,7 +213,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,13 +220,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: ${ bundle.parameters.enable_airlock } @@ -303,7 +266,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,13 +273,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: ${ bundle.parameters.enable_airlock } @@ -351,30 +307,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 +319,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 } @@ -395,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/template_schema.json b/templates/workspaces/base/template_schema.json index c69024b8e8..4c1ce180f1 100644 --- a/templates/workspaces/base/template_schema.json +++ b/templates/workspaces/base/template_schema.json @@ -75,6 +75,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", @@ -244,13 +251,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": [ @@ -259,13 +259,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", @@ -349,7 +342,6 @@ "auth_type", "create_aad_groups", "client_id", - "client_secret", "enable_backup", "enable_airlock", "configure_review_vms", @@ -357,4 +349,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 abfcf62520..045ed40127 100644 --- a/templates/workspaces/base/terraform/.terraform.lock.hcl +++ b/templates/workspaces/base/terraform/.terraform.lock.hcl @@ -80,3 +80,5 @@ provider "registry.terraform.io/hashicorp/random" { "zh:eac7b63e86c749c7d48f527671c7aee5b4e26c10be6ad7232d6860167f99dbb0", ] } + + diff --git a/templates/workspaces/base/terraform/aad/aad.tf b/templates/workspaces/base/terraform/aad/aad.tf index fd3acd0c3b..51da680294 100644 --- a/templates/workspaces/base/terraform/aad/aad.tf +++ b/templates/workspaces/base/terraform/aad/aad.tf @@ -112,8 +112,9 @@ 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 + display_name = "Workspace Password" } resource "azurerm_key_vault_secret" "client_id" { @@ -127,7 +128,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 @@ -135,7 +136,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 +170,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/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 : "" } - - diff --git a/templates/workspaces/base/terraform/aad/variables.tf b/templates/workspaces/base/terraform/aad/variables.tf index a93df28733..23df4a7253 100644 --- a/templates/workspaces/base/terraform/aad/variables.tf +++ b/templates/workspaces/base/terraform/aad/variables.tf @@ -14,19 +14,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/azure-monitor/azure-monitor.tf b/templates/workspaces/base/terraform/azure-monitor/azure-monitor.tf index 08822baa80..a9b1580245 100644 --- a/templates/workspaces/base/terraform/azure-monitor/azure-monitor.tf +++ b/templates/workspaces/base/terraform/azure-monitor/azure-monitor.tf @@ -194,6 +194,7 @@ resource "azurerm_private_endpoint" "azure_monitor_private_endpoint" { depends_on = [ azurerm_monitor_private_link_scoped_service.ampls_app_insights, + azurerm_monitor_private_link_scoped_service.ampls_log_anaytics, ] } diff --git a/templates/workspaces/base/terraform/keyvault.tf b/templates/workspaces/base/terraform/keyvault.tf index 9d3986e052..3ab5bfd897 100644 --- a/templates/workspaces/base/terraform/keyvault.tf +++ b/templates/workspaces/base/terraform/keyvault.tf @@ -102,43 +102,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/variables.tf b/templates/workspaces/base/terraform/variables.tf index b475c0135c..4689042976 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 @@ -93,47 +87,14 @@ 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 = "" 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 } -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 = "" diff --git a/templates/workspaces/base/terraform/workspace.tf b/templates/workspaces/base/terraform/workspace.tf index 8008c545bd..774be28609 100644 --- a/templates/workspaces/base/terraform/workspace.tf +++ b/templates/workspaces/base/terraform/workspace.tf @@ -1,3 +1,21 @@ +data "azuread_application" "existing_workspace" { + count = var.client_id != "" ? 1 : 0 + client_id = var.client_id + + lifecycle { + postcondition { + condition = var.client_id == "" || self.client_id != "" + error_message = "Application with client_id ${var.client_id} not found in tenant. Ensure the application exists and the service principal has permission to read it." + } + } +} + +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}" @@ -36,7 +54,6 @@ module "network" { module "aad" { source = "./aad" tre_workspace_tags = local.tre_workspace_tags - count = var.register_aad_application ? 1 : 0 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 +69,12 @@ module "aad" { ] } +import { + for_each = local.workspace_app_imports + to = module.aad.azuread_application.workspace + id = each.value +} + module "airlock" { count = var.enable_airlock ? 1 : 0 source = "./airlock" 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 <