Repository: microsoft/AzureTRE (python)

Issue:
Cannot update workspace with custom address space
**Describe the bug**
Creating a workspace with a custom address space and then updating it will fail with `The custom 'address_space' you requested does not fit in the current network.`  This is because the method `workspaces.validate_address_space()` will check the allocated network on update, find that the network is already allocated (to the current workspace), and return the error. 

The same error arises when creating a workspace and specifying a custom address space which overlaps with a workspace that has been deleted.

**Steps to reproduce**

1. Create a workspace with custom address space
1. Update it without any changes

**Suggested changes**
* Custom address space should not be updateable
* The address space should not be subject to validation on update
* The allocated network validation should not included the deleted workspaces and the current workspace

Pull request:
Update setting of read only fields and include deleted address spaces when assigning new address spaces
# Resolves #3691
## How is this addressed

- Fixed iteration through schema to set read only properties
- Exlplicitly set fields to updatable in templates
- Moduified IP address assignment and validation to reuse those in deleted workspaces.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 769b45c1..87ba3096 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,6 +9,8 @@ FEATURES:
 ENHANCEMENTS:
 
 BUG FIXES:
+* Fix issue where updates fail as read only is not configured consistently on schema fields ([#3691](https://github.com/microsoft/AzureTRE/issues/3691))
+* When geting avaialble address spaces allow those allocated to deleted workspaces to be reassigned ([#3691](https://github.com/microsoft/AzureTRE/issues/3691))
 * Update Python packages, and fix breaking changes ([#3764](https://github.com/microsoft/AzureTRE/issues/3764))
 * Enabling support for more than 20 users/groups in Workspace API ([#3759](https://github.com/microsoft/AzureTRE/pull/3759  ))
 * Airlock Import Review workspace uses dedicated DNS zone to prevent conflict with core ([#3767](https://github.com/microsoft/AzureTRE/pull/3767))
diff --git a/api_app/_version.py b/api_app/_version.py
index 53d3b0d4..9027351f 100644
--- a/api_app/_version.py
+++ b/api_app/_version.py
@@ -1,1 +1,1 @@
-__version__ = "0.16.7"
+__version__ = "0.16.8"
diff --git a/api_app/api/routes/workspaces.py b/api_app/api/routes/workspaces.py
index 8607e9fc..9a7fc9fe 100644
--- a/api_app/api/routes/workspaces.py
+++ b/api_app/api/routes/workspaces.py
@@ -7,7 +7,7 @@ from jsonschema.exceptions import ValidationError
 
 from api.dependencies.database import get_repository
 from api.dependencies.workspaces import get_operation_by_id_from_path, get_workspace_by_id_from_path, get_deployed_workspace_by_id_from_path, get_deployed_workspace_service_by_id_from_path, get_workspace_service_by_id_from_path, get_user_resource_by_id_from_path
-from db.errors import MajorVersionUpdateDenied, TargetTemplateVersionDoesNotExist, UserNotAuthorizedToUseTemplate, VersionDowngradeDenied
+from db.errors import InvalidInput, MajorVersionUpdateDenied, TargetTemplateVersionDoesNotExist, UserNotAuthorizedToUseTemplate, VersionDowngradeDenied
 from db.repositories.operations import OperationRepository
 from db.repositories.resource_templates import ResourceTemplateRepository
 from db.repositories.resources_history import ResourceHistoryRepository
@@ -107,6 +107,8 @@ async def create_workspace(workspace_create: WorkspaceInCreate, response: Respon
     except UserNotAuthorizedToUseTemplate as e:
         logging.exception("User not authorized to use template")
         raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail=str(e))
+    except InvalidInput as e:
+        raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e))
 
     operation = await save_and_deploy_resource(
         resource=workspace,
diff --git a/api_app/db/repositories/workspaces.py b/api_app/db/repositories/workspaces.py
index 42a46c31..23af5310 100644
--- a/api_app/db/repositories/workspaces.py
+++ b/api_app/db/repositories/workspaces.py
@@ -140,11 +140,11 @@ class WorkspaceRepository(ResourceRepository):
         if (address_space is None):
             raise InvalidInput("Missing 'address_space' from properties.")
 
-        allocated_networks = [x.properties["address_space"] for x in await self.get_workspaces()]
+        allocated_networks = [x.properties["address_space"] for x in await self.get_active_workspaces()]
         return is_network_available(allocated_networks, address_space)
 
     async def get_new_address_space(self, cidr_netmask: int = 24):
-        workspaces = await self.get_workspaces()
+        workspaces = await self.get_active_workspaces()
         networks = [[x.properties.get("address_space")] for x in workspaces]
         networks = networks + [x.properties.get("address_spaces", []) for x in workspaces]
         networks = [i for s in networks for i in s if i is not None]
diff --git a/api_app/services/schema_service.py b/api_app/services/schema_service.py
index f01e2038..65e98012 100644
--- a/api_app/services/schema_service.py
+++ b/api_app/services/schema_service.py
@@ -50,9 +50,17 @@ def enrich_template(original_template, extra_properties, is_update: bool = False
     # this will help the UI render fields appropriately and know what it can send in a PATCH
     if is_update:
         for prop in template["properties"].values():
-            if "updateable" not in prop.keys() or prop["updateable"] is not True:
+            if not prop.get("updateable", False):
                 prop["readOnly"] = True
 
+        if "allOf" in template:
+            for conditional_property in template["allOf"]:
+                for condition in ["then", "else"]:
+                    if condition in conditional_property and "properties" in conditional_property[condition]:
+                        for prop in conditional_property[condition]["properties"].values():
+                            if not prop.get("updateable", False):
+                                prop["readOnly"] = True
+
     # if there is an 'allOf' property which is empty, the validator fails - so remove the key
     if "allOf" in template and template["allOf"] is None:
         template.pop("allOf")
diff --git a/templates/workspaces/airlock-import-review/template_schema.json b/templates/workspaces/airlock-import-review/template_schema.json
index be6c1ccf..e05a0d87 100644
--- a/templates/workspaces/airlock-import-review/template_schema.json
+++ b/templates/workspaces/airlock-import-review/template_schema.json
@@ -94,13 +94,15 @@
           "client_id": {
             "type": "string",
             "title": "Application (Client) ID",
-            "description": "The AAD Application Registration ID for the workspace."
+            "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
+            "sensitive": true,
+            "updateable": true
           }
         },
         "required": [
@@ -113,12 +115,14 @@
             "type": "boolean",
             "title": "Create AAD Groups for each workspace role",
             "description": "Create AAD Groups for the workspace roles. If this is set to true, the workspace will create new AAD Groups.",
-            "default": false
+            "default": false,
+            "updateable": true
           },
           "aad_redirect_uris": {
             "type": "array",
             "title": "AAD Redirect URIs",
             "description": "Redirect URIs for the AAD app in Automatic Auth mode",
+            "updateable": true,
             "items": {
               "title": "items",
               "type": "object",
diff --git a/templates/workspaces/base/porter.yaml b/templates/workspaces/base/porter.yaml
index ee0e2705..f7a18024 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: 1.5.1
+version: 1.5.3
 description: "A base Azure TRE workspace"
 dockerfile: Dockerfile.tmpl
 registry: azuretre
diff --git a/templates/workspaces/base/template_schema.json b/templates/workspaces/base/template_schema.json
index 2ae9c9d5..3d6cdf0e 100644
--- a/templates/workspaces/base/template_schema.json
+++ b/templates/workspaces/base/template_schema.json
@@ -81,7 +81,8 @@
             "type": "boolean",
             "title": "Configure Review VMs",
             "description": "Allow TRE to automatically create and delete review VMs for airlock approvals",
-            "default": false
+            "default": false,
+            "updateable": true
           }
         }
       }
@@ -108,6 +109,7 @@
             "title": "Airlock Review Config",
             "default": null,
             "description": "Configuration for Airlock Review feature. Needs to be set up after workspace creation",
+            "updateable": true,
             "properties": {
               "import": {
                 "title": "Import Review Settings",
@@ -206,13 +208,15 @@
           "client_id": {
             "type": "string",
             "title": "Application (Client) ID",
-            "description": "The AAD Application Registration ID for the workspace."
+            "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
+            "sensitive": true,
+            "updateable": true
           }
         },
         "required": [
@@ -225,12 +229,14 @@
             "type": "boolean",
             "title": "Create AAD Groups for each workspace role",
             "description": "Create AAD Groups for the workspace roles. If this is set to true, the workspace will create new AAD Groups.",
-            "default": false
+            "default": false,
+            "updateable": true
           },
           "aad_redirect_uris": {
             "type": "array",
             "title": "AAD Redirect URIs",
             "description": "Redirect URIs for the AAD app in Automatic Auth mode",
+            "updateable": true,
             "items": {
               "title": "items",
               "type": "object",
diff --git a/templates/workspaces/unrestricted/porter.yaml b/templates/workspaces/unrestricted/porter.yaml
index fefa9df1..fd3a0758 100644
--- a/templates/workspaces/unrestricted/porter.yaml
+++ b/templates/workspaces/unrestricted/porter.yaml
@@ -1,7 +1,7 @@
 ---
 schemaVersion: 1.0.0
 name: tre-workspace-unrestricted
-version: 0.11.1
+version: 0.11.4
 description: "A base Azure TRE workspace"
 dockerfile: Dockerfile.tmpl
 registry: azuretre
diff --git a/templates/workspaces/unrestricted/template_schema.json b/templates/workspaces/unrestricted/template_schema.json
index 40e051d5..3fbaa16a 100644
--- a/templates/workspaces/unrestricted/template_schema.json
+++ b/templates/workspaces/unrestricted/template_schema.json
@@ -206,13 +206,15 @@
           "client_id": {
             "type": "string",
             "title": "Application (Client) ID",
-            "description": "The AAD Application Registration ID for the workspace."
+            "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
+            "sensitive": true,
+            "updateable": true
           }
         },
         "required": [
@@ -225,12 +227,14 @@
             "type": "boolean",
             "title": "Create AAD Groups for each workspace role",
             "description": "Create AAD Groups for the workspace roles. If this is set to true, the workspace will create new AAD Groups.",
-            "default": false
+            "default": false,
+            "updateable": true
           },
           "aad_redirect_uris": {
             "type": "array",
             "title": "AAD Redirect URIs",
             "description": "Redirect URIs for the AAD app in Automatic Auth mode",
+            "updateable": true,
             "items": {
               "title": "items",
               "type": "object",


Test patch:
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 7ae495fa..79b9a517 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
@@ -209,16 +209,16 @@ async def test_get_address_space_based_on_size_with_custom_address_space_and_mis
 
 
 @pytest.mark.asyncio
-@patch('db.repositories.workspaces.WorkspaceRepository.get_workspaces')
+@patch('db.repositories.workspaces.WorkspaceRepository.get_active_workspaces')
 @patch('core.config.RESOURCE_LOCATION', "useast2")
 @patch('core.config.TRE_ID', "9876")
 @patch('core.config.CORE_ADDRESS_SPACE', "10.1.0.0/22")
 @patch('core.config.TRE_ADDRESS_SPACE', "10.0.0.0/12")
-async def test_get_address_space_based_on_size_with_address_space_only(get_workspaces_mock, workspace_repo, basic_workspace_request, workspace):
+async def test_get_address_space_based_on_size_with_address_space_only(get_active_workspaces_mock, workspace_repo, basic_workspace_request, workspace):
     workspace_with_address_space = copy.deepcopy(workspace)
     workspace_with_address_space.properties["address_space"] = "10.1.4.0/24"
 
-    get_workspaces_mock.return_value = [workspace_with_address_space]
+    get_active_workspaces_mock.return_value = [workspace_with_address_space]
     workspace_to_create = basic_workspace_request
     address_space = await workspace_repo.get_address_space_based_on_size(workspace_to_create.properties)
 
@@ -226,12 +226,12 @@ async def test_get_address_space_based_on_size_with_address_space_only(get_works
 
 
 @pytest.mark.asyncio
-@patch('db.repositories.workspaces.WorkspaceRepository.get_workspaces')
+@patch('db.repositories.workspaces.WorkspaceRepository.get_active_workspaces')
 @patch('core.config.RESOURCE_LOCATION', "useast2")
 @patch('core.config.TRE_ID', "9876")
 @patch('core.config.CORE_ADDRESS_SPACE', "10.1.0.0/22")
 @patch('core.config.TRE_ADDRESS_SPACE', "10.0.0.0/12")
-async def test_get_address_space_based_on_size_with_address_space_and_address_spaces(get_workspaces_mock, workspace_repo, basic_workspace_request, workspace):
+async def test_get_address_space_based_on_size_with_address_space_and_address_spaces(get_active_workspaces_mock, workspace_repo, basic_workspace_request, workspace):
     workspace_with_address_space = copy.deepcopy(workspace)
     workspace_with_address_space.properties["address_space"] = "10.1.4.0/24"
 
@@ -242,7 +242,7 @@ async def test_get_address_space_based_on_size_with_address_space_and_address_sp
     workspace_with_both.properties["address_spaces"] = ["10.1.7.0/24", "10.1.8.0/24"]
     workspace_with_both.properties["address_space"] = "10.1.7.0/24"
 
-    get_workspaces_mock.return_value = [workspace_with_address_space, workspace_with_address_spaces, workspace_with_both]
+    get_active_workspaces_mock.return_value = [workspace_with_address_space, workspace_with_address_spaces, workspace_with_both]
     workspace_to_create = basic_workspace_request
     address_space = await workspace_repo.get_address_space_based_on_size(workspace_to_create.properties)