diff --git a/.github/actions/devcontainer_run_command/action.yml b/.github/actions/devcontainer_run_command/action.yml index b0364585a1..d666070b70 100644 --- a/.github/actions/devcontainer_run_command/action.yml +++ b/.github/actions/devcontainer_run_command/action.yml @@ -30,12 +30,6 @@ inputs: TEST_ACCOUNT_CLIENT_SECRET: description: "The Test Automation Account Client Secret used to interact with the API." required: false - TEST_WORKSPACE_APP_ID: - description: "The Test Workspace application Id used to interact with the API." - required: false - TEST_WORKSPACE_APP_SECRET: - description: "The Test Workspace application secret used to interact with the API." - required: false TRE_ID: description: "The TRE Id." required: false @@ -273,8 +267,6 @@ runs: -e TRE_ID="${{ inputs.TRE_ID }}" \ -e TF_VAR_tre_id="${{ inputs.TRE_ID }}" \ -e TRE_URL="${{ env.TRE_URL }}" \ - -e TEST_WORKSPACE_APP_ID="${{ inputs.TEST_WORKSPACE_APP_ID }}" \ - -e TEST_WORKSPACE_APP_SECRET="${{ inputs.TEST_WORKSPACE_APP_SECRET }}" \ -e TEST_APP_ID="${{ inputs.TEST_APP_ID }}" \ -e TEST_ACCOUNT_CLIENT_ID="${{ inputs.TEST_ACCOUNT_CLIENT_ID }}" \ -e TEST_ACCOUNT_CLIENT_SECRET="${{ inputs.TEST_ACCOUNT_CLIENT_SECRET }}" \ diff --git a/.github/scripts/build.js b/.github/scripts/build.js index 1014402991..0a30e81a72 100644 --- a/.github/scripts/build.js +++ b/.github/scripts/build.js @@ -114,6 +114,15 @@ async function getCommandFromComment({ core, context, github }) { break; } + case "/test-manual-app": + { + const runTests = await handleTestCommand({ core, github }, parts, "manual app tests", runId, { number: prNumber, authorUsername: prAuthorUsername, repoOwner, repoName, headSha: prHeadSha, refId: prRefId, details: pr }, { username: commentUsername, link: commentLink }); + if (runTests) { + command = "run-tests-manual-app"; + } + break; + } + case "/test-force-approve": { command = "test-force-approve"; @@ -250,6 +259,7 @@ You can use the following commands:     /test-extended - build, deploy and run smoke & extended tests on a PR     /test-extended-aad - build, deploy and run smoke & extended AAD tests on a PR     /test-shared-services - test the deployment of shared services on a PR build +    /test-manual-app - run the manual workspace application test suite on a PR build     /test-force-approve - force approval of the PR tests (i.e. skip the deployment checks)     /test-destroy-env - delete the validation environment for a PR (e.g. to enable testing a deployment from a clean start after previous tests)     /help - show this help`; diff --git a/.github/scripts/build.test.js b/.github/scripts/build.test.js index 5a9cd8456b..5a36e41281 100644 --- a/.github/scripts/build.test.js +++ b/.github/scripts/build.test.js @@ -412,6 +412,32 @@ describe('getCommandFromComment', () => { }); }); + describe(`for '/test-manual-app'`, () => { + test(`should set command to 'run-tests-manual-app'`, async () => { + const context = createCommentContext({ + username: 'admin', + body: '/test-manual-app', + }); + await getCommandFromComment({ core, context, github }); + expect(outputFor(mockCoreSetOutput, 'command')).toBe('run-tests-manual-app'); + }); + + test(`should add comment with run link`, async () => { + const context = createCommentContext({ + username: 'admin', + body: '/test-manual-app', + pullRequestNumber: PR_NUMBER.UPSTREAM_NON_DOCS_CHANGES, + }); + await getCommandFromComment({ core, context, github }); + expect(mockGithubRestIssuesCreateComment).toHaveComment({ + owner: 'someOwner', + repo: 'someRepo', + issue_number: PR_NUMBER.UPSTREAM_NON_DOCS_CHANGES, + bodyMatcher: /Running manual app tests: https:\/\/github.com\/someOwner\/someRepo\/actions\/runs\/11112222 \(with refid `291ae84f`\)/, + }); + }); + }); + describe(`for '/test-shared-services'`, () => { test(`should set command to 'run-tests-shared-services'`, async () => { const context = createCommentContext({ diff --git a/.github/workflows/deploy_tre.yml b/.github/workflows/deploy_tre.yml index 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..ae6c81a335 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 }} 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 d52d77954a..0b926e79bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,13 +2,32 @@ ## 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; upgrade at your convenience + 2. **Upgrading Workspaces:** + - Ensure Application Admin identity owns existing workspace applications + - Run workspace upgrade - Terraform will import and take over secret management + 3. **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 + + **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)) BUG FIXES: diff --git a/api_app/_version.py b/api_app/_version.py index c2a0df2076..7c4a9591e1 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.25.9" +__version__ = "0.26.0" diff --git a/api_app/api/routes/workspaces.py b/api_app/api/routes/workspaces.py index 4f8ee42bb2..11f5f7a386 100644 --- a/api_app/api/routes/workspaces.py +++ b/api_app/api/routes/workspaces.py @@ -30,7 +30,6 @@ get_current_workspace_owner_or_researcher_user_or_airlock_manager, \ get_current_workspace_owner_or_airlock_manager, \ get_current_workspace_owner_or_researcher_user_or_airlock_manager_or_tre_admin -from services.authentication import extract_auth_information from services.azure_resource_status import get_azure_resource_status from azure.cosmos.exceptions import CosmosAccessConditionFailedError from .resource_helpers import cascaded_update_resource, delete_validation, enrich_resource_with_available_upgrades, get_identity_role_assignments, save_and_deploy_resource, construct_location_header, send_uninstall_message, \ @@ -99,9 +98,7 @@ async def retrieve_workspace_scope_id_by_workspace_id(workspace=Depends(get_work @workspaces_core_router.post("/workspaces", status_code=status.HTTP_202_ACCEPTED, response_model=OperationInResponse, name=strings.API_CREATE_WORKSPACE, dependencies=[Depends(get_current_admin_user)]) async def create_workspace(workspace_create: WorkspaceInCreate, response: Response, user=Depends(get_current_admin_user), workspace_repo=Depends(get_repository(WorkspaceRepository)), resource_template_repo=Depends(get_repository(ResourceTemplateRepository)), operations_repo=Depends(get_repository(OperationRepository)), resource_history_repo=Depends(get_repository(ResourceHistoryRepository))) -> OperationInResponse: try: - # TODO: This requires Directory.ReadAll ( Application.Read.All ) to be enabled in the Azure AD application to enable a users workspaces to be listed. This should be made optional. - auth_info = extract_auth_information(workspace_create.properties) - workspace, resource_template = await workspace_repo.create_workspace_item(workspace_create, auth_info, user.id, user.roles) + workspace, resource_template = await workspace_repo.create_workspace_item(workspace_create, user.id, user.roles) except (ValidationError, ValueError) as e: logger.exception("Failed to create workspace model instance") raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) diff --git a/api_app/db/repositories/workspaces.py b/api_app/db/repositories/workspaces.py index 5be9cab979..008bb99c04 100644 --- a/api_app/db/repositories/workspaces.py +++ b/api_app/db/repositories/workspaces.py @@ -89,7 +89,7 @@ async def is_workspace_storage_account_available(self, workspace_id: str) -> boo ) return availability_result.name_available - async def create_workspace_item(self, workspace_input: WorkspaceInCreate, auth_info: dict, workspace_owner_object_id: str, user_roles: List[str]) -> Tuple[Workspace, ResourceTemplate]: + async def create_workspace_item(self, workspace_input: WorkspaceInCreate, workspace_owner_object_id: str, user_roles: List[str]) -> Tuple[Workspace, ResourceTemplate]: full_workspace_id = str(uuid.uuid4()) @@ -104,7 +104,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, @@ -112,9 +111,7 @@ 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)} workspace = Workspace( @@ -134,9 +131,6 @@ def get_workspace_owner(self, workspace_properties: dict, workspace_owner_object user_defined_workspace_owner_object_id = workspace_properties.get("workspace_owner_object_id") return workspace_owner_object_id if user_defined_workspace_owner_object_id is None else user_defined_workspace_owner_object_id - def automatically_create_application_registration(self, workspace_properties: dict) -> bool: - return True if ("auth_type" in workspace_properties and workspace_properties["auth_type"] == "Automatic") else False - async def get_address_space_based_on_size(self, workspace_properties: dict): # Default the address space to 'small' if not supplied. address_space_size = workspace_properties.get("address_space_size", "small").lower() diff --git a/api_app/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..14763f71b3 100644 --- a/api_app/services/aad_authentication.py +++ b/api_app/services/aad_authentication.py @@ -346,7 +346,14 @@ def get_assignable_users(self, filter: str = "", maxResultCount: int = 5) -> Lis 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") + 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 = [] @@ -374,9 +381,26 @@ def assign_workspace_user(self, user_id: str, workspace: Workspace, role_id: str return self._assign_workspace_user_to_application_group(user_id, workspace, role_id) def _is_user_in_role(self, user_id: str, role_id: str) -> bool: - user_app_role_query = f"{MICROSOFT_GRAPH_URL}/v1.0/users/{user_id}/appRoleAssignments" - user_app_roles = self._ms_graph_query(user_app_role_query, "GET") - return any(r for r in user_app_roles["value"] if r["appRoleId"] == role_id) + try: + identity_type = self._get_identity_type(user_id) + except AuthConfigValidationError as exc: + logger.warning("Unable to determine identity type for %s: %s", user_id, exc) + return False + + if identity_type == "#microsoft.graph.user": + graph_data = self._get_role_assignment_graph_data_for_user(user_id) + elif identity_type == "#microsoft.graph.servicePrincipal": + graph_data = self._get_role_assignment_graph_data_for_service_principal(user_id) + else: + logger.warning("Unsupported identity type %s for %s", identity_type, user_id) + return False + + assignments = graph_data.get("value") + if assignments is None: + logger.warning("Role assignment response missing 'value' for %s", user_id) + return False + + return any(role.get("appRoleId") == role_id for role in assignments) def _is_workspace_role_group_in_use(self, workspace: Workspace) -> bool: aad_groups_in_user = workspace.properties["create_aad_groups"] @@ -480,29 +504,11 @@ def _get_batch_users_by_role_assignments_body(self, roles_graph_data): return request_body - # This method is called when you create a workspace and you already have an AAD App Registration - # to link it to. You pass in the client_id and go and get the extra information you need from AAD - # If the auth_type is `Automatic`, then these values will be written by Terraform. - def _get_app_auth_info(self, client_id: str) -> dict: - graph_data = self._get_app_sp_graph_data(client_id) - if 'value' not in graph_data or len(graph_data['value']) == 0: - logger.debug(graph_data) - raise AuthConfigValidationError(f"{strings.ACCESS_UNABLE_TO_GET_INFO_FOR_APP} {client_id}") - - app_info = graph_data['value'][0] - authInfo = {'sp_id': app_info['id'], 'scope_id': app_info['servicePrincipalNames'][0]} - - # Convert the roles into ids (We could have more roles defined in the app than we need.) - for appRole in app_info['appRoles']: - if appRole['value'] in self.WORKSPACE_ROLES_DICT.keys(): - authInfo[self.WORKSPACE_ROLES_DICT[appRole['value']]] = appRole['id'] - - return authInfo - - def _ms_graph_query(self, url: str, http_method: str, json=None) -> dict: + 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 @@ -518,8 +524,13 @@ def _ms_graph_query(self, url: str, http_method: str, json=None) -> dict: if '@odata.nextLink' in json_response: url = json_response['@odata.nextLink'] 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,23 +561,6 @@ def _get_identity_type(self, id: str) -> str: return object_info["@odata.type"] - def extract_workspace_auth_information(self, data: dict) -> dict: - if ("auth_type" not in data) or (data["auth_type"] != "Automatic" and "client_id" not in data): - raise AuthConfigValidationError(strings.ACCESS_PLEASE_SUPPLY_CLIENT_ID) - - auth_info = {} - # The user may want us to create the AAD workspace app and therefore they - # don't know the client_id yet. - if data["auth_type"] != "Automatic": - auth_info = self._get_app_auth_info(data["client_id"]) - - # Check we've get all our required roles - for role in self.WORKSPACE_ROLES_DICT.items(): - if role[1] not in auth_info: - raise AuthConfigValidationError(f"{strings.ACCESS_APP_IS_MISSING_ROLE} {role[0]}") - - return auth_info - def get_identity_role_assignments(self, user_id: str) -> List[RoleAssignment]: identity_type = self._get_identity_type(user_id) if identity_type == "#microsoft.graph.user": diff --git a/api_app/services/access_service.py b/api_app/services/access_service.py index d38d26ce15..c909fc3b46 100644 --- a/api_app/services/access_service.py +++ b/api_app/services/access_service.py @@ -15,10 +15,6 @@ class UserRoleAssignmentError(Exception): class AccessService(OAuth2AuthorizationCodeBearer): - @abstractmethod - def extract_workspace_auth_information(self, data: dict) -> dict: - pass - @abstractmethod def get_identity_role_assignments(self, user_id: str) -> dict: pass diff --git a/api_app/services/authentication.py b/api_app/services/authentication.py index 30b49af194..18efa223cf 100644 --- a/api_app/services/authentication.py +++ b/api_app/services/authentication.py @@ -4,15 +4,7 @@ from models.schemas.workspace import AuthProvider from resources import strings from services.aad_authentication import AzureADAuthorization -from services.access_service import AccessService, AuthConfigValidationError - - -def extract_auth_information(workspace_creation_properties: dict) -> dict: - access_service = get_access_service('AAD') - try: - return access_service.extract_workspace_auth_information(workspace_creation_properties) - except AuthConfigValidationError as e: - raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) +from services.access_service import AccessService def get_access_service(provider: str = AuthProvider.AAD) -> AccessService: diff --git a/api_app/tests_ma/test_api/test_routes/test_workspace_users.py b/api_app/tests_ma/test_api/test_routes/test_workspace_users.py index 64fb2a7d68..de9cc40aaa 100644 --- a/api_app/tests_ma/test_api/test_routes/test_workspace_users.py +++ b/api_app/tests_ma/test_api/test_routes/test_workspace_users.py @@ -24,7 +24,7 @@ OPERATION_ID = '11111111-7265-4b5f-9eae-a1a62928772f' -def sample_workspace(workspace_id=WORKSPACE_ID, auth_info: dict = {}) -> Workspace: +def sample_workspace(workspace_id=WORKSPACE_ID) -> Workspace: workspace = Workspace( id=workspace_id, templateName="tre-workspace-base", @@ -39,8 +39,7 @@ def sample_workspace(workspace_id=WORKSPACE_ID, auth_info: dict = {}) -> Workspa updatedWhen=FAKE_CREATE_TIMESTAMP, user=create_admin_user() ) - if auth_info: - workspace.properties = {**auth_info} + return workspace diff --git a/api_app/tests_ma/test_api/test_routes/test_workspaces.py b/api_app/tests_ma/test_api/test_routes/test_workspaces.py index 5e97af84f2..e775824c7b 100644 --- a/api_app/tests_ma/test_api/test_routes/test_workspaces.py +++ b/api_app/tests_ma/test_api/test_routes/test_workspaces.py @@ -436,8 +436,7 @@ async def test_get_workspace_history_returns_empty_list_when_no_history(self, ac @patch("api.routes.resource_helpers.send_resource_request_message", return_value=sample_resource_operation(resource_id=WORKSPACE_ID, operation_id=OPERATION_ID)) @patch("api.routes.workspaces.WorkspaceRepository.save_item") @patch("api.routes.workspaces.WorkspaceRepository.create_workspace_item") - @patch("api.routes.workspaces.extract_auth_information") - async def test_post_workspaces_creates_workspace(self, _, create_workspace_item, __, ___, resource_template_repo, app, client, workspace_input, basic_resource_template): + async def test_post_workspaces_creates_workspace(self, create_workspace_item, __, ___, resource_template_repo, app, client, workspace_input, basic_resource_template): resource_template_repo.return_value = basic_resource_template create_workspace_item.return_value = [sample_workspace(), basic_resource_template] response = await client.post(app.url_path_for(strings.API_CREATE_WORKSPACE), json=workspace_input) @@ -451,8 +450,7 @@ async def test_post_workspaces_creates_workspace(self, _, create_workspace_item, @patch("api.routes.workspaces.WorkspaceRepository.save_item") @patch("api.routes.workspaces.WorkspaceRepository.create_workspace_item") @patch("api.routes.workspaces.WorkspaceRepository._validate_resource_parameters") - @patch("api.routes.workspaces.extract_auth_information") - async def test_post_workspaces_calls_db_and_service_bus(self, _, __, create_workspace_item, save_item_mock, send_resource_request_message_mock, resource_template_repo, app, client, workspace_input, basic_resource_template): + async def test_post_workspaces_calls_db_and_service_bus(self, __, create_workspace_item, save_item_mock, send_resource_request_message_mock, resource_template_repo, app, client, workspace_input, basic_resource_template): resource_template_repo.return_value = basic_resource_template create_workspace_item.return_value = [sample_workspace(), basic_resource_template] await client.post(app.url_path_for(strings.API_CREATE_WORKSPACE), json=workspace_input) @@ -466,8 +464,7 @@ async def test_post_workspaces_calls_db_and_service_bus(self, _, __, create_work @patch("api.routes.workspaces.WorkspaceRepository.save_item") @patch("api.routes.workspaces.WorkspaceRepository.create_workspace_item") @patch("api.routes.workspaces.WorkspaceRepository._validate_resource_parameters") - @patch("api.routes.workspaces.extract_auth_information") - async def test_post_workspaces_returns_202_on_successful_create(self, _, __, create_workspace_item, ____, _____, resource_template_repo, app, client, workspace_input, basic_resource_template): + async def test_post_workspaces_returns_202_on_successful_create(self, __, create_workspace_item, ____, _____, resource_template_repo, app, client, workspace_input, basic_resource_template): resource_template_repo.return_value = basic_resource_template create_workspace_item.return_value = [sample_workspace(), basic_resource_template] response = await client.post(app.url_path_for(strings.API_CREATE_WORKSPACE), json=workspace_input) @@ -482,8 +479,7 @@ async def test_post_workspaces_returns_202_on_successful_create(self, _, __, cre @patch("api.routes.workspaces.WorkspaceRepository.save_item") @patch("api.routes.workspaces.WorkspaceRepository.create_workspace_item", return_value=[sample_workspace(), sample_resource_template()]) @patch("api.routes.workspaces.WorkspaceRepository._validate_resource_parameters") - @patch("api.routes.workspaces.extract_auth_information") - async def test_post_workspaces_returns_503_if_service_bus_call_fails(self, _, __, ___, ____, _____, delete_item_mock, resource_template_repo, app, client, workspace_input, basic_resource_template): + async def test_post_workspaces_returns_503_if_service_bus_call_fails(self, __, ___, ____, _____, delete_item_mock, resource_template_repo, app, client, workspace_input, basic_resource_template): resource_template_repo.return_value = basic_resource_template response = await client.post(app.url_path_for(strings.API_CREATE_WORKSPACE), json=workspace_input) @@ -491,8 +487,8 @@ async def test_post_workspaces_returns_503_if_service_bus_call_fails(self, _, __ delete_item_mock.assert_called_once_with(WORKSPACE_ID) # [POST] /workspaces/ - @patch("api.routes.workspaces.WorkspaceRepository.validate_input_against_template", side_effect=ValueError) - async def test_post_workspaces_returns_400_if_template_does_not_exist(self, _, app, client, workspace_input): + @patch("api.routes.workspaces.WorkspaceRepository.create_workspace_item", side_effect=ValueError) + async def test_post_workspaces_returns_400_if_template_does_not_exist(self, mock_create, app, client, workspace_input): response = await client.post(app.url_path_for(strings.API_CREATE_WORKSPACE), json=workspace_input) assert response.status_code == status.HTTP_400_BAD_REQUEST diff --git a/api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py b/api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py index 9abbc96727..66f2c8e061 100644 --- a/api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py +++ b/api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py @@ -117,7 +117,7 @@ async def test_create_workspace_item_creates_a_workspace_with_the_right_values(m validate_input_mock.return_value = basic_resource_template new_cidr_mock.return_value = "1.2.3.4/24" - workspace, _ = await workspace_repo.create_workspace_item(workspace_to_create, {}, "test_object_id", ["test_role"]) + workspace, _ = await workspace_repo.create_workspace_item(workspace_to_create, "test_object_id", ["test_role"]) assert workspace.templateName == workspace_to_create.templateName assert workspace.resourceType == ResourceType.Workspace @@ -185,7 +185,7 @@ async def test_create_workspace_item_creates_a_workspace_with_custom_address_spa mock_is_workspace_storage_account_available.return_value.return_value = False validate_input_mock.return_value = basic_resource_template - workspace, _ = await workspace_repo.create_workspace_item(workspace_to_create, {}, "test_object_id", ["test_role"]) + workspace, _ = await workspace_repo.create_workspace_item(workspace_to_create, "test_object_id", ["test_role"]) assert workspace.properties["address_space"] == workspace_to_create.properties["address_space"] @@ -207,7 +207,7 @@ async def test_create_workspace_item_throws_exception_with_bad_custom_address_sp validate_input_mock.return_value = basic_resource_template with pytest.raises(InvalidInput): - await workspace_repo.create_workspace_item(workspace_to_create, {}, "test_object_id", ["test_role"]) + await workspace_repo.create_workspace_item(workspace_to_create, "test_object_id", ["test_role"]) @pytest.mark.asyncio @@ -272,19 +272,7 @@ async def test_create_workspace_item_raises_value_error_if_template_is_invalid(m validate_input_mock.side_effect = ValueError with pytest.raises(ValueError): - await workspace_repo.create_workspace_item(workspace_input, {}, "test_object_id", ["test_role"]) - - -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 + await workspace_repo.create_workspace_item(workspace_input, "test_object_id", ["test_role"]) def test_workspace_owner_is_set_if_not_present_in_workspace_properties(workspace_repo): 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..a02f17e86b 100644 --- a/api_app/tests_ma/test_services/test_aad_access_service.py +++ b/api_app/tests_ma/test_services/test_aad_access_service.py @@ -145,12 +145,6 @@ def user_with_role(): return User(id="user2", name="Test User 2", email="test2@example.com", roles=["WorkspaceOwner"]) -def test_extract_workspace__raises_error_if_client_id_not_available(): - access_service = AzureADAuthorization() - with pytest.raises(AuthConfigValidationError): - access_service.extract_workspace_auth_information(data={"auth_type": "Manual"}) - - @patch("services.aad_authentication.AzureADAuthorization._get_app_sp_graph_data") @patch("services.aad_authentication.AzureADAuthorization._get_user_role_assignments") @patch("services.aad_authentication.AzureADAuthorization._get_user_details") @@ -306,71 +300,6 @@ def test_get_workspace_user_emails_by_role_assignment_with_groups_and_users_assi assert "test_user4@email.com" in role_assignment_details["WorkspaceOwner"] -@patch( - "services.aad_authentication.AzureADAuthorization._get_app_auth_info", - return_value={"app_role_id_workspace_researcher": "1234"}, -) -def test_extract_workspace__raises_error_if_owner_not_in_roles(get_app_auth_info_mock): - access_service = AzureADAuthorization() - with pytest.raises(AuthConfigValidationError): - access_service.extract_workspace_auth_information(data={"client_id": "1234"}) - - -@patch( - "services.aad_authentication.AzureADAuthorization._get_app_auth_info", - return_value={"app_role_id_workspace_owner": "1234"}, -) -def test_extract_workspace__raises_error_if_researcher_not_in_roles( - get_app_auth_info_mock, -): - access_service = AzureADAuthorization() - with pytest.raises(AuthConfigValidationError): - access_service.extract_workspace_auth_information(data={"client_id": "1234"}) - - -@patch( - "services.aad_authentication.AzureADAuthorization._get_app_sp_graph_data", - return_value={}, -) -def test_extract_workspace__raises_error_if_graph_data_is_invalid( - get_app_sp_graph_data_mock, -): - access_service = AzureADAuthorization() - with pytest.raises(AuthConfigValidationError): - access_service.extract_workspace_auth_information(data={"client_id": "1234"}) - - -@patch("services.aad_authentication.AzureADAuthorization._get_app_sp_graph_data") -def test_extract_workspace__returns_sp_id_and_roles(get_app_sp_graph_data_mock): - get_app_sp_graph_data_mock.return_value = { - "value": [ - { - "id": "12345", - "appRoles": [ - {"id": "1abc3", "value": "WorkspaceResearcher"}, - {"id": "1abc4", "value": "WorkspaceOwner"}, - {"id": "1abc5", "value": "AirlockManager"}, - ], - "servicePrincipalNames": ["api://tre_ws_1234"], - } - ] - } - expected_auth_info = { - "sp_id": "12345", - "scope_id": "api://tre_ws_1234", - "app_role_id_workspace_owner": "1abc4", - "app_role_id_workspace_researcher": "1abc3", - "app_role_id_workspace_airlock_manager": "1abc5", - } - - access_service = AzureADAuthorization() - actual_auth_info = access_service.extract_workspace_auth_information( - data={"auth_type": "Manual", "client_id": "1234"} - ) - - assert actual_auth_info == expected_auth_info - - @pytest.mark.parametrize( "user, workspace, expected_role", [ @@ -647,6 +576,38 @@ def test_get_role_assignment_for_user(mock_get_role_assignment_data_for_user): assert role == mock_user_data["value"][0] +@patch("services.aad_authentication.AzureADAuthorization._get_role_assignment_graph_data_for_user") +@patch("services.aad_authentication.AzureADAuthorization._get_identity_type", return_value="#microsoft.graph.user") +def test_is_user_in_role_with_user_identity(mock_get_identity_type, mock_get_role_assignments): + mock_get_role_assignments.return_value = {"value": [{"appRoleId": "role-a"}]} + access_service = AzureADAuthorization() + + assert access_service._is_user_in_role("user-obj", "role-a") is True + mock_get_identity_type.assert_called_once_with("user-obj") + mock_get_role_assignments.assert_called_once() + + +@patch("services.aad_authentication.AzureADAuthorization._get_role_assignment_graph_data_for_service_principal") +@patch("services.aad_authentication.AzureADAuthorization._get_identity_type", return_value="#microsoft.graph.servicePrincipal") +def test_is_user_in_role_with_service_principal(mock_get_identity_type, mock_get_role_assignments): + mock_get_role_assignments.return_value = {"value": [{"appRoleId": "role-b"}]} + access_service = AzureADAuthorization() + + assert access_service._is_user_in_role("sp-obj", "role-b") is True + mock_get_identity_type.assert_called_once_with("sp-obj") + mock_get_role_assignments.assert_called_once() + + +@patch("services.aad_authentication.AzureADAuthorization._get_identity_type", return_value="#microsoft.graph.servicePrincipal") +@patch("services.aad_authentication.AzureADAuthorization._get_role_assignment_graph_data_for_service_principal", return_value={}) +def test_is_user_in_role_handles_missing_graph_value(mock_get_assignments, mock_get_identity_type): + access_service = AzureADAuthorization() + + assert access_service._is_user_in_role("sp-obj", "role-c") is False + mock_get_identity_type.assert_called_once_with("sp-obj") + mock_get_assignments.assert_called_once() + + def get_mock_batch_response(user_principals, group_principals): response_body = {"responses": []} for user_principal in user_principals: diff --git a/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 index 4b2ffa8347..a03316c69b 100644 --- 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/wait_for_new_app_registration.sh b/devops/scripts/aad/wait_for_new_app_registration.sh index 8a2a0eaea2..d47d8effc6 100755 --- a/devops/scripts/aad/wait_for_new_app_registration.sh +++ b/devops/scripts/aad/wait_for_new_app_registration.sh @@ -25,6 +25,4 @@ function wait_for_new_app_registration() echo "Failed" exit 1 fi - - echo "App registration \"${clientId}\" found." } diff --git a/devops/scripts/create_aad_assets.sh b/devops/scripts/create_aad_assets.sh index ff5727ba37..e6583f43ae 100755 --- a/devops/scripts/create_aad_assets.sh +++ b/devops/scripts/create_aad_assets.sh @@ -29,15 +29,15 @@ APPLICATION_PERMISSIONS=() APPLICATION_PERMISSIONS+=("Application.ReadWrite.OwnedBy") if [ "${AUTO_WORKSPACE_APP_REGISTRATION:-}" == true ]; then - APPLICATION_PERMISSIONS+=("Application.ReadWrite.All" "Directory.Read.All") + APPLICATION_PERMISSIONS+=("Application.ReadWrite.All") fi if [ "${AUTO_WORKSPACE_GROUP_CREATION:-}" == true ]; then - APPLICATION_PERMISSIONS+=("Group.Create") + APPLICATION_PERMISSIONS+=("Group.Create" "Group.Read.All" "User.ReadBasic.All") fi if [ "${AUTO_GRANT_WORKSPACE_CONSENT:-}" == true ]; then - APPLICATION_PERMISSIONS+=("Application.ReadWrite.All" "DelegatedPermissionGrant.ReadWrite.All") + APPLICATION_PERMISSIONS+=("DelegatedPermissionGrant.ReadWrite.All") fi # Check if the array contains more than 1 item @@ -74,21 +74,6 @@ APPLICATION_PERMISSION=$(IFS=,; echo "${APPLICATION_PERMISSIONS[*]}") --reset-password $RESET_PASSWORDS \ --custom-domain "${CUSTOM_DOMAIN}" -if [ "${AUTO_WORKSPACE_APP_REGISTRATION:=false}" == false ]; then - # Load the new values back in - # This is because we want the SWAGGER_UI_CLIENT_ID - # shellcheck disable=SC1091 - . "$DIR/load_and_validate_env.sh" - - "$DIR/aad/create_workspace_application.sh" \ - --name "${TRE_ID} - workspace 1" \ - --admin-consent \ - --ux-clientid "${SWAGGER_UI_CLIENT_ID}" \ - --automation-clientid "${TEST_ACCOUNT_CLIENT_ID}" \ - --application-admin-clientid "${APPLICATION_ADMIN_CLIENT_ID}" \ - --reset-password $RESET_PASSWORDS -fi - if [ "${CHANGED_TENANT}" -ne 0 ]; then echo "Attempting to sign you back into ${LOGGED_IN_TENANT_ID}." diff --git a/devops/scripts/setup_local_debugging.sh b/devops/scripts/setup_local_debugging.sh index 2bf70a63d0..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}" <_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..746371688e 100644 --- a/docs/tre-admins/setup-instructions/cicd-pre-deployment-steps.md +++ b/docs/tre-admins/setup-instructions/cicd-pre-deployment-steps.md @@ -102,8 +102,6 @@ In a previous [Setup Auth configuration](./setup-auth-entities.md) step authenti | `API_CLIENT_ID` | API application (client) ID. | | `API_CLIENT_SECRET` | API application client secret. | | `SWAGGER_UI_CLIENT_ID` | Swagger (OpenAPI) UI application (client) ID. | - | `TEST_WORKSPACE_APP_ID`| Each workspace is secured behind it's own AD Application. Use the value of `WORKSPACE_API_CLIENT_ID` created in the `/config.yaml` env file | - | `TEST_WORKSPACE_APP_SECRET`| Each workspace is secured behind it's own AD Application. This is the secret for that application. Use the value of `WORKSPACE_API_CLIENT_SECRET` created in the `/config.yaml` env file| !!! info See [Environment variables](../environment-variables.md) for full details of the deployment related variables. diff --git a/docs/tre-admins/setup-instructions/installing-base-workspace.md b/docs/tre-admins/setup-instructions/installing-base-workspace.md index f1b8dea6b4..f1877335e1 100644 --- a/docs/tre-admins/setup-instructions/installing-base-workspace.md +++ b/docs/tre-admins/setup-instructions/installing-base-workspace.md @@ -22,17 +22,12 @@ As explained in the [auth guide](../auth.md), every workspace has a correspondin ```bash ./devops/scripts/aad/create_workspace_application.sh \ --name "${TRE_ID} - workspace 1" \ - --admin-consent \ - --ux-clientid "${SWAGGER_UI_CLIENT_ID}" \ - --automation-clientid "${TEST_ACCOUNT_CLIENT_ID}" \ --application-admin-clientid "${APPLICATION_ADMIN_CLIENT_ID}" ``` !!! caution If you're using a separate tenant for Microsoft Entra ID app registrations to the one where you've deployed the TRE infrastructure resources, ensure you've signed into that tenant in the `az cli` before running the above command. See **Using a separate Microsoft Entra ID tenant** in [Setup Auth configuration](setup-auth-entities.md) for more details. -Running the script will report `workspace_api_client_id` and `workspace_api_client_secret` for the generated app. Add these under the authenrication section in `/config.yaml` so that automated testing will work. You also need to use `workspace_api_client_id` in the POST body below. - ### Create workspace using the API Go to `https:///api/docs` and use POST `/api/workspaces` with the sample body to create a base workspace. @@ -43,7 +38,6 @@ Go to `https:///api/docs` and use POST `/api/workspaces` with th "display_name": "manual-from-swagger", "description": "workspace for team X", "client_id":"", - "client_secret":"", "address_space_size": "medium", "workspace_subscription_id": "" } diff --git a/docs/tre-admins/setup-instructions/ui-install-base-workspace.md b/docs/tre-admins/setup-instructions/ui-install-base-workspace.md index 25b4e2c213..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..49bf3b2faf 100644 --- a/docs/tre-admins/setup-instructions/workflows.md +++ b/docs/tre-admins/setup-instructions/workflows.md @@ -90,17 +90,6 @@ Configure the E2E Test repository secrets | `TEST_USER_NAME` | The username of the E2E Test User | | `TEST_USER_PASSWORD` | The password of the E2E Test User | -### Create a workspace app registration for setting up workspaces (for the E2E tests) - -Follow the [instructions to create a workspace app registration](../auth.md#workspaces) (used for the E2E tests) - and make the E2E test user a **WorkspaceOwner** for the app registration. - -Configure the TEST_WORKSPACE_APP_ID repository secret - -|
Secret name
| Description | -| ----------- | ----------- | -| `TEST_WORKSPACE_APP_ID` | The application (client) ID of the Workspaces app. | -| `TEST_WORKSPACE_APP_SECRET` | The application (client) secret of the Workspaces app. | - ### Configure repository/environment secrets Configure additional secrets used in the deployment workflow: diff --git a/docs/tre-developers/end-to-end-tests.md b/docs/tre-developers/end-to-end-tests.md index 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..11c6c32054 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,11 @@ 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_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 +92,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 +101,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 +108,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 +161,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 +201,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..d3d1ff35a4 100644 --- a/e2e_tests/helpers.py +++ b/e2e_tests/helpers.py @@ -1,7 +1,9 @@ import asyncio +import functools from typing import List, Optional from contextlib import asynccontextmanager from httpx import AsyncClient, Timeout, Response +import httpx import logging from starlette import status from azure.identity import ClientSecretCredential, UsernamePasswordCredential @@ -15,6 +17,11 @@ azlogger = logging.getLogger("azure") azlogger.setLevel(logging.WARN) +GRAPH_TIMEOUT = Timeout(10, read=30) +_AUTOMATION_ADMIN_OBJECT_ID: Optional[str] = None +ROLE_ASSIGNMENT_MAX_ATTEMPTS = 30 +ROLE_ASSIGNMENT_SLEEP_SECONDS = 10 + class InstallFailedException(Exception): pass @@ -125,3 +132,212 @@ def get_token(scope_uri, verify) -> str: def assert_status(response: Response, expected_status: List[int] = [200], message_prefix: str = "Unexpected HTTP Status"): assert response.status_code in expected_status, \ f"{message_prefix}. Expected: {expected_status}. Actual: {response.status_code}. Response text: {response.text}" + + +async def ensure_automation_admin_has_airlock_role(workspace_id: str, admin_token: str, verify: bool) -> None: + await _ensure_automation_admin_has_role(workspace_id, admin_token, verify, role_name="Airlock Manager") + + +async def ensure_automation_admin_has_workspace_owner_role(workspace_id: str, admin_token: str, verify: bool) -> None: + await _ensure_automation_admin_has_role(workspace_id, admin_token, verify, role_name="WorkspaceOwner") + + +async def _ensure_automation_admin_has_role(workspace_id: str, admin_token: str, verify: bool, role_name: str) -> None: + """Assign the automation admin identity to a workspace role via the TRE API.""" + if workspace_id == "": + LOGGER.info("Workspace ID missing; skipping %s assignment", role_name) + return + + if config.TEST_ACCOUNT_CLIENT_ID == "" and config.TEST_USER_NAME == "": + LOGGER.info("No automation admin identity configured; skipping %s assignment", role_name) + return + + if not await _wait_for_user_management_configuration(workspace_id, admin_token, verify, role_name): + return + + role_id = await _get_workspace_role_id(workspace_id, admin_token, verify, role_name=role_name) + if role_id is None: + LOGGER.warning("%s role not found for workspace %s; skipping assignment", role_name, workspace_id) + return + + user_object_id = await _get_automation_admin_object_id(workspace_id, admin_token, verify) + if user_object_id is None: + LOGGER.warning("Unable to determine automation admin directory object id; skipping %s assignment", role_name) + return + + await _assign_workspace_role_via_api(workspace_id, role_id, user_object_id, admin_token, verify, role_name) + + +async def _get_workspace_role_id(workspace_id: str, admin_token: str, verify: bool, role_name: str) -> Optional[str]: + """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 + + +async def _assign_workspace_role_via_api(workspace_id: str, role_id: str, user_id: str, admin_token: str, verify: bool, role_name: str) -> None: + payload = { + "role_id": role_id, + "user_ids": [user_id] + } + + async with AsyncClient(verify=verify, timeout=TIMEOUT) as client: + headers = get_auth_header(admin_token) + response = await client.post( + get_full_endpoint(f"/api/workspaces/{workspace_id}/users/assign"), + headers=headers, + json=payload, + timeout=TIMEOUT + ) + assert_status(response, [status.HTTP_202_ACCEPTED], f"Failed to assign {role_name} role for workspace {workspace_id}") + + +async def _get_automation_admin_object_id(workspace_id: str, admin_token: str, verify: bool) -> Optional[str]: + global _AUTOMATION_ADMIN_OBJECT_ID + if _AUTOMATION_ADMIN_OBJECT_ID: + return _AUTOMATION_ADMIN_OBJECT_ID + + if config.TEST_USER_NAME != "": + user_id = await _get_assignable_user_id(workspace_id, admin_token, verify, config.TEST_USER_NAME) + if user_id: + _AUTOMATION_ADMIN_OBJECT_ID = user_id + return user_id + LOGGER.warning("Assignable user lookup failed for %s; falling back to Graph", config.TEST_USER_NAME) + + loop = asyncio.get_running_loop() + directory_id = await loop.run_in_executor(None, functools.partial(_get_directory_object_id_via_graph, verify)) + if directory_id: + _AUTOMATION_ADMIN_OBJECT_ID = directory_id + return directory_id + + +async def _get_assignable_user_id(workspace_id: str, admin_token: str, verify: bool, user_principal_name: str) -> Optional[str]: + params = { + "filter": user_principal_name, + "maxResultCount": 50 + } + + async with AsyncClient(verify=verify, timeout=TIMEOUT) as client: + headers = get_auth_header(admin_token) + response = await client.get( + get_full_endpoint(f"/api/workspaces/{workspace_id}/assignable-users"), + headers=headers, + params=params, + timeout=TIMEOUT + ) + assert_status(response, [status.HTTP_200_OK], f"Failed to get assignable users for workspace {workspace_id}") + + for user in response.json().get("assignableUsers", []): + upn = user.get("userPrincipalName", "").lower() + if upn == user_principal_name.lower(): + return user.get("id") + + return None + + +async def _wait_for_user_management_configuration(workspace_id: str, admin_token: str, verify: bool, role_name: str) -> bool: + """Poll the workspace until Entra ID role/group data is available.""" + for attempt in range(ROLE_ASSIGNMENT_MAX_ATTEMPTS): + workspace = await _get_workspace_details(workspace_id, admin_token, verify) + if _workspace_supports_user_management(workspace): + return True + + if _workspace_cannot_support_user_management(workspace): + LOGGER.warning("Workspace %s does not support user management; skipping %s assignment", workspace_id, role_name) + return False + + LOGGER.info("Waiting for workspace %s user management config (%s/%s)", workspace_id, attempt + 1, ROLE_ASSIGNMENT_MAX_ATTEMPTS) + await asyncio.sleep(ROLE_ASSIGNMENT_SLEEP_SECONDS) + + LOGGER.warning("Workspace %s never exposed user management config; skipping %s assignment", workspace_id, role_name) + return False + + +async def _get_workspace_details(workspace_id: str, admin_token: str, verify: bool) -> dict: + async with AsyncClient(verify=verify, timeout=TIMEOUT) as client: + headers = get_auth_header(admin_token) + response = await client.get(get_full_endpoint(f"/api/workspaces/{workspace_id}"), headers=headers, timeout=TIMEOUT) + assert_status(response, [status.HTTP_200_OK], f"Failed to get workspace {workspace_id}") + return response.json().get("workspace", {}) + + +def _workspace_supports_user_management(workspace: dict) -> bool: + props = workspace.get("properties", {}) + has_groups = props.get("create_aad_groups") is True + required_fields = [ + "sp_id", + "app_role_id_workspace_owner", + "app_role_id_workspace_researcher", + "app_role_id_workspace_airlock_manager" + ] + return has_groups and all(field in props for field in required_fields) + + +def _workspace_cannot_support_user_management(workspace: dict) -> bool: + props = workspace.get("properties", {}) + deployment_status = workspace.get("deploymentStatus", "") + groups_flag = props.get("create_aad_groups") + return deployment_status == "deployed" and not groups_flag + + +def _get_directory_object_id_via_graph(verify: bool) -> Optional[str]: + graph_resource = cloud.get_ms_graph_resource() + graph_base_url = f"{graph_resource}/v1.0" + token = get_token(graph_resource, verify) + headers = { + "Authorization": f"Bearer {token}", + "Content-Type": "application/json" + } + + identity_id: Optional[str] = None + with httpx.Client(headers=headers, timeout=GRAPH_TIMEOUT, verify=verify) as client: + if config.TEST_ACCOUNT_CLIENT_ID != "": + identity_id = _get_service_principal_id(client, graph_base_url, config.TEST_ACCOUNT_CLIENT_ID) + elif config.TEST_USER_NAME != "": + identity_id = _get_user_object_id(client, graph_base_url, config.TEST_USER_NAME) + + return identity_id + + +def _get_service_principal_id(client: httpx.Client, base_url: str, app_id: str) -> Optional[str]: + resp = client.get(f"{base_url}/servicePrincipals", params={"$filter": f"appId eq '{app_id}'"}) + resp.raise_for_status() + values = resp.json().get("value", []) + if not values: + return None + return values[0]["id"] + + +def _get_user_object_id(client: httpx.Client, base_url: str, user_principal_name: str) -> Optional[str]: + if user_principal_name == "": + return None + + resp = client.get(f"{base_url}/users/{user_principal_name}", params={"$select": "id"}) + if resp.status_code == status.HTTP_404_NOT_FOUND: + return None + + resp.raise_for_status() + return resp.json().get("id") 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 affdeef01a..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,6 +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 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/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 <