Remove need to provide client secret when creating workspace and remove Directory.Read.All Dependency from automation admin

.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 }}" \

.github/linters/.tflint.hcl

@@ -1,5 +1,5 @@
 config {
-  call_module_type = "all"
+  call_module_type = "local"
   force = false
 }
 
@@ -36,3 +36,8 @@ rule "terraform_standard_module_structure" {
 rule "terraform_required_version" {
   enabled = false
 }
+
+# Disabled: Workspace secrets have a normal lifecycle and need to be deleted with the workspace
+rule "azurerm_resources_missing_prevent_destroy" {
+  enabled = false
+}

.github/linters/.tflint_workspaces.hcl

@@ -1,12 +1,14 @@
 # This is used for TRE tags validation only.
 
 config {
-  call_module_type = "all"
+  call_module_type = "local"
   force = false
 }
 
 plugin "azurerm" {
     enabled = true
+    version = "0.30.0"
+    source  = "github.com/terraform-linters/tflint-ruleset-azurerm"
 }
 
 rule "terraform_typed_variables" {
@@ -17,3 +19,8 @@ rule "azurerm_resource_missing_tags" {
   enabled = true
   tags = ["tre_id", "tre_workspace_id"]
 }
+
+# Disabled: Workspace secrets have a normal lifecycle and need to be deleted with the workspace
+rule "azurerm_resources_missing_prevent_destroy" {
+  enabled = false
+}

.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`;

.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({

.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 }}

.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) }}

.github/workflows/deploy_tre_reusable.yml

@@ -76,12 +76,6 @@ on: # yamllint disable-line rule:truthy
       TEST_APP_ID:
         description: ""
         required: true
-      TEST_WORKSPACE_APP_ID:
-        description: ""
-        required: true
-      TEST_WORKSPACE_APP_SECRET:
-        description: ""
-        required: true
       TEST_ACCOUNT_CLIENT_ID:
         description: ""
         required: true
@@ -163,12 +157,6 @@ jobs:
           if [ "${{ secrets.TEST_APP_ID }}" == '' ]; then
             echo "Missing secret: TEST_APP_ID" && exit 1
           fi
-          if [ "${{ secrets.TEST_WORKSPACE_APP_ID }}" == '' ]; then
-            echo "Missing secret: TEST_WORKSPACE_APP_ID" && exit 1
-          fi
-          if [ "${{ secrets.TEST_WORKSPACE_APP_SECRET }}" == '' ]; then
-            echo "Missing secret: TEST_WORKSPACE_APP_SECRET" && exit 1
-          fi
           if [ "${{ secrets.TEST_ACCOUNT_CLIENT_ID }}" == '' ]; then
             echo "Missing secret: TEST_ACCOUNT_CLIENT_ID" && exit 1
           fi
@@ -815,8 +803,6 @@ jobs:
           API_CLIENT_ID: "${{ secrets.API_CLIENT_ID }}"
           AAD_TENANT_ID: "${{ secrets.AAD_TENANT_ID }}"
           TEST_APP_ID: "${{ secrets.TEST_APP_ID }}"
-          TEST_WORKSPACE_APP_ID: "${{ secrets.TEST_WORKSPACE_APP_ID }}"
-          TEST_WORKSPACE_APP_SECRET: "${{ secrets.TEST_WORKSPACE_APP_SECRET }}"
           TEST_ACCOUNT_CLIENT_ID: "${{ secrets.TEST_ACCOUNT_CLIENT_ID }}"
           TEST_ACCOUNT_CLIENT_SECRET: "${{ secrets.TEST_ACCOUNT_CLIENT_SECRET }}"
           TRE_ID: ${{ secrets.TRE_ID }}
@@ -859,8 +845,6 @@ jobs:
           API_CLIENT_ID: "${{ secrets.API_CLIENT_ID }}"
           AAD_TENANT_ID: "${{ secrets.AAD_TENANT_ID }}"
           TEST_APP_ID: "${{ secrets.TEST_APP_ID }}"
-          TEST_WORKSPACE_APP_ID: "${{ secrets.TEST_WORKSPACE_APP_ID }}"
-          TEST_WORKSPACE_APP_SECRET: "${{ secrets.TEST_WORKSPACE_APP_SECRET }}"
           TEST_ACCOUNT_CLIENT_ID: "${{ secrets.TEST_ACCOUNT_CLIENT_ID }}"
           TEST_ACCOUNT_CLIENT_SECRET: "${{ secrets.TEST_ACCOUNT_CLIENT_SECRET }}"
           TRE_ID: ${{ secrets.TRE_ID }}
@@ -913,6 +897,6 @@ jobs:
       - name: Publish E2E Test Results
         uses: EnricoMi/publish-unit-test-result-action@v2
         with:
-          junit_files: "artifacts/**/*.xml"
+          files: "artifacts/**/*.xml"
           check_name: "E2E Test Results"
           comment_mode: off

.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) }}

CHANGELOG.md

@@ -2,13 +2,33 @@
 ## 0.27.0 (Unreleased)
 **BREAKING CHANGES**
 * Fix missing arguments for airlock manager requests - change in API contract  ([#4544](https://github.com/microsoft/AzureTRE/issues/4544))
-* Clarify cost label time period and aggregation scope in UI tooltips ([#4607](https://github.com/microsoft/AzureTRE/pull/4607))
+
+* Base workspace bundle 3.0.0 (major upgrade from 2.8.0) now creates and manages the workspace Microsoft Entra application secret automatically and removes the manual identity passthrough parameters (`client_secret`, `register_aad_application`, `scope_id`, `sp_id`, `app_role_id_*`).
+
+  **Migration Guide:**
+  1. **Existing Workspaces:** Continue to operate without changes;
+  2. **New Workspaces:**
+     - No `client_secret` parameter needed
+     - Optionally provide `client_id` to reuse pre-existing application
+     - Leave `client_id` empty for fully automatic application creation
+  3. **Upgrading Workspaces:**
+     - Only upgrade once you have tested the process in a non-production environment with your own bundles.
+     - Ensure Application Admin identity owns existing workspace applications
+     - Run workspace upgrade - Terraform will import and take over secret management
+
+  **Permission Changes:**
+  - **Removed:** `Directory.Read.All` no longer required
+  - **Keep (depending on requirements):** `Application.ReadWrite.All` (or `Application.ReadWrite.OwnedBy`), `Group.Create`, `Group.Read.All`, `User.ReadBasic.All`, `DelegatedPermissionGrant.ReadWrite.All`
+
+  ([#4775](https://github.com/microsoft/AzureTRE/pull/4775))
+
 
 ENHANCEMENTS:
 * Upgrade Guacamole to v1.6.0 with Java 17 and other security updates ([#4754](https://github.com/microsoft/AzureTRE/pull/4754))
 * API: Replace HTTP_422_UNPROCESSABLE_ENTITY response with HTTP_422_UNPROCESSABLE_CONTENT as per RFC 9110 ([#4742](https://github.com/microsoft/AzureTRE/issues/4742))
 * Change Group.ReadWrite.All permission to Group.Create for AUTO_WORKSPACE_GROUP_CREATION ([#4772](https://github.com/microsoft/AzureTRE/issues/4772))
 * Make workspace shared storage quota updateable ([#4314](https://github.com/microsoft/AzureTRE/issues/4314))
+* Clarify cost label time period and aggregation scope in UI tooltips ([#4607](https://github.com/microsoft/AzureTRE/pull/4607))
 * Update Porter, AzureCLI, Terraform and its providers across the solution ([#4799](https://github.com/microsoft/AzureTRE/issues/4799))
 * Update `api_healthcheck.sh` script with fixed 10-second check intervals and 7-minute timeout for improved API health monitoring ([#4807](https://github.com/microsoft/AzureTRE/issues/4807))
 * Update SuperLinter to version 8.3.2 ([#4815](https://github.com/microsoft/AzureTRE/issues/4815))

api_app/_version.py

@@ -1 +1 @@
-__version__ = "0.25.12"
+__version__ = "0.26.3"

api_app/api/routes/workspace_users.py

@@ -31,7 +31,6 @@ async def get_workspace_roles(workspace=Depends(get_workspace_by_id_from_path),
 
 @workspaces_users_admin_router.post("/workspaces/{workspace_id}/users/assign", status_code=status.HTTP_202_ACCEPTED, name=strings.API_ASSIGN_WORKSPACE_USER)
 async def assign_workspace_user(response: Response, userRoleAssignmentRequest: UserRoleAssignmentRequest, workspace=Depends(get_workspace_by_id_from_path), access_service=Depends(get_access_service)) -> WorkspaceUserOperationResponse:
-
     for user_id in userRoleAssignmentRequest.user_ids:
         access_service.assign_workspace_user(
             user_id,

api_app/api/routes/workspaces.py

@@ -24,13 +24,12 @@
 from models.schemas.resource_template import ResourceTemplateInformationInList
 from resources import strings
 from services.access_service import AuthConfigValidationError
-from services.authentication import get_current_admin_user, \
+from services.authentication import extract_auth_information, get_current_admin_user, \
     get_access_service, get_current_workspace_owner_user, get_current_workspace_owner_or_researcher_user, get_current_tre_user_or_tre_admin, \
     get_current_workspace_owner_or_tre_admin, \
     get_current_workspace_owner_or_researcher_user_or_airlock_manager, \
     get_current_workspace_owner_or_airlock_manager, \
     get_current_workspace_owner_or_researcher_user_or_airlock_manager_or_tre_admin
-from services.authentication import extract_auth_information
 from services.azure_resource_status import get_azure_resource_status
 from azure.cosmos.exceptions import CosmosAccessConditionFailedError
 from .resource_helpers import cascaded_update_resource, delete_validation, enrich_resource_with_available_upgrades, get_identity_role_assignments, save_and_deploy_resource, construct_location_header, send_uninstall_message, \
@@ -99,7 +98,6 @@ async def retrieve_workspace_scope_id_by_workspace_id(workspace=Depends(get_work
 @workspaces_core_router.post("/workspaces", status_code=status.HTTP_202_ACCEPTED, response_model=OperationInResponse, name=strings.API_CREATE_WORKSPACE, dependencies=[Depends(get_current_admin_user)])
 async def create_workspace(workspace_create: WorkspaceInCreate, response: Response, user=Depends(get_current_admin_user), workspace_repo=Depends(get_repository(WorkspaceRepository)), resource_template_repo=Depends(get_repository(ResourceTemplateRepository)), operations_repo=Depends(get_repository(OperationRepository)), resource_history_repo=Depends(get_repository(ResourceHistoryRepository))) -> OperationInResponse:
     try:
-        # TODO: This requires Directory.ReadAll ( Application.Read.All ) to be enabled in the Azure AD application to enable a users workspaces to be listed. This should be made optional.
         auth_info = extract_auth_information(workspace_create.properties)
         workspace, resource_template = await workspace_repo.create_workspace_item(workspace_create, auth_info, user.id, user.roles)
     except (ValidationError, ValueError) as e:

api_app/db/repositories/workspaces.py

@@ -105,15 +105,13 @@ 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,
         # so dict.update can't work. Priorities from right to left.
         resource_spec_parameters = {**workspace_input.properties,
                                     **address_space_param,
                                     **address_spaces_param,
-                                    **auto_app_registration_param,
                                     **workspace_owner_param,
                                     **auth_info,
                                     **self.get_workspace_spec_params(full_workspace_id)}
@@ -135,9 +133,6 @@ def get_workspace_owner(self, workspace_properties: dict, workspace_owner_object
         user_defined_workspace_owner_object_id = workspace_properties.get("workspace_owner_object_id")
         return workspace_owner_object_id if user_defined_workspace_owner_object_id is None else user_defined_workspace_owner_object_id
 
-    def automatically_create_application_registration(self, workspace_properties: dict) -> bool:
-        return True if ("auth_type" in workspace_properties and workspace_properties["auth_type"] == "Automatic") else False
-
     async def get_address_space_based_on_size(self, workspace_properties: dict):
         # Default the address space to 'small' if not supplied.
         address_space_size = workspace_properties.get("address_space_size", "small").lower()

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"
             }
         }

api_app/models/schemas/workspace.py

@@ -84,7 +84,6 @@ class Config:
                     "description": "workspace description",
                     "auth_type": "Manual",
                     "client_id": "<WORKSPACE_CLIENT_ID>",
-                    "client_secret": "<WORKSPACE_CLIENT_SECRET>",
                     "address_space_size": "small"
                 }
             }

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",

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"