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

api_app/_version.py

@@ -1 +1 @@
-__version__ = "0.25.4"
+__version__ = "0.25.5"

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

api_app/db/repositories/workspaces.py

@@ -89,7 +89,7 @@ async def is_workspace_storage_account_available(self, workspace_id: str) -> boo
         )
         return availability_result.name_available
 
-    async def create_workspace_item(self, workspace_input: WorkspaceInCreate, auth_info: dict, workspace_owner_object_id: str, user_roles: List[str]) -> Tuple[Workspace, ResourceTemplate]:
+    async def create_workspace_item(self, workspace_input: WorkspaceInCreate, workspace_owner_object_id: str, user_roles: List[str]) -> Tuple[Workspace, ResourceTemplate]:
 
         full_workspace_id = str(uuid.uuid4())
 
@@ -114,7 +114,6 @@ async def create_workspace_item(self, workspace_input: WorkspaceInCreate, auth_i
                                     **address_spaces_param,
                                     **auto_app_registration_param,
                                     **workspace_owner_param,
-                                    **auth_info,
                                     **self.get_workspace_spec_params(full_workspace_id)}
 
         workspace = Workspace(

api_app/services/aad_authentication.py

@@ -480,25 +480,6 @@ def _get_batch_users_by_role_assignments_body(self, roles_graph_data):
 
         return request_body
 
-    # This method is called when you create a workspace and you already have an AAD App Registration
-    # to link it to. You pass in the client_id and go and get the extra information you need from AAD
-    # If the auth_type is `Automatic`, then these values will be written by Terraform.
-    def _get_app_auth_info(self, client_id: str) -> dict:
-        graph_data = self._get_app_sp_graph_data(client_id)
-        if 'value' not in graph_data or len(graph_data['value']) == 0:
-            logger.debug(graph_data)
-            raise AuthConfigValidationError(f"{strings.ACCESS_UNABLE_TO_GET_INFO_FOR_APP} {client_id}")
-
-        app_info = graph_data['value'][0]
-        authInfo = {'sp_id': app_info['id'], 'scope_id': app_info['servicePrincipalNames'][0]}
-
-        # Convert the roles into ids (We could have more roles defined in the app than we need.)
-        for appRole in app_info['appRoles']:
-            if appRole['value'] in self.WORKSPACE_ROLES_DICT.keys():
-                authInfo[self.WORKSPACE_ROLES_DICT[appRole['value']]] = appRole['id']
-
-        return authInfo
-
     def _ms_graph_query(self, url: str, http_method: str, json=None) -> dict:
         msgraph_token = self._get_msgraph_token()
         auth_headers = self._get_auth_header(msgraph_token)
@@ -550,23 +531,6 @@ def _get_identity_type(self, id: str) -> str:
 
         return object_info["@odata.type"]
 
-    def extract_workspace_auth_information(self, data: dict) -> dict:
-        if ("auth_type" not in data) or (data["auth_type"] != "Automatic" and "client_id" not in data):
-            raise AuthConfigValidationError(strings.ACCESS_PLEASE_SUPPLY_CLIENT_ID)
-
-        auth_info = {}
-        # The user may want us to create the AAD workspace app and therefore they
-        # don't know the client_id yet.
-        if data["auth_type"] != "Automatic":
-            auth_info = self._get_app_auth_info(data["client_id"])
-
-            # Check we've get all our required roles
-            for role in self.WORKSPACE_ROLES_DICT.items():
-                if role[1] not in auth_info:
-                    raise AuthConfigValidationError(f"{strings.ACCESS_APP_IS_MISSING_ROLE} {role[0]}")
-
-        return auth_info
-
     def get_identity_role_assignments(self, user_id: str) -> List[RoleAssignment]:
         identity_type = self._get_identity_type(user_id)
         if identity_type == "#microsoft.graph.user":

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

api_app/services/authentication.py

@@ -7,14 +7,6 @@
 from services.access_service import AccessService, AuthConfigValidationError
 
 
-def extract_auth_information(workspace_creation_properties: dict) -> dict:
-    access_service = get_access_service('AAD')
-    try:
-        return access_service.extract_workspace_auth_information(workspace_creation_properties)
-    except AuthConfigValidationError as e:
-        raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e))
-
-
 def get_access_service(provider: str = AuthProvider.AAD) -> AccessService:
     if provider == AuthProvider.AAD:
         return AzureADAuthorization()

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
 
 

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,17 +479,16 @@ 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)
 
         assert response.status_code == status.HTTP_503_SERVICE_UNAVAILABLE
         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
 

api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py

@@ -117,7 +117,7 @@ async def test_create_workspace_item_creates_a_workspace_with_the_right_values(m
     validate_input_mock.return_value = basic_resource_template
     new_cidr_mock.return_value = "1.2.3.4/24"
 
-    workspace, _ = await workspace_repo.create_workspace_item(workspace_to_create, {}, "test_object_id", ["test_role"])
+    workspace, _ = await workspace_repo.create_workspace_item(workspace_to_create, "test_object_id", ["test_role"])
 
     assert workspace.templateName == workspace_to_create.templateName
     assert workspace.resourceType == ResourceType.Workspace
@@ -186,7 +186,7 @@ async def test_create_workspace_item_creates_a_workspace_with_custom_address_spa
     mock_is_workspace_storage_account_available.return_value.return_value = False
     validate_input_mock.return_value = basic_resource_template
 
-    workspace, _ = await workspace_repo.create_workspace_item(workspace_to_create, {}, "test_object_id", ["test_role"])
+    workspace, _ = await workspace_repo.create_workspace_item(workspace_to_create, "test_object_id", ["test_role"])
 
     assert workspace.properties["address_space"] == workspace_to_create.properties["address_space"]
 
@@ -208,7 +208,7 @@ async def test_create_workspace_item_throws_exception_with_bad_custom_address_sp
     validate_input_mock.return_value = basic_resource_template
 
     with pytest.raises(InvalidInput):
-        await workspace_repo.create_workspace_item(workspace_to_create, {}, "test_object_id", ["test_role"])
+        await workspace_repo.create_workspace_item(workspace_to_create, "test_object_id", ["test_role"])
 
 
 @pytest.mark.asyncio
@@ -273,7 +273,7 @@ async def test_create_workspace_item_raises_value_error_if_template_is_invalid(m
     validate_input_mock.side_effect = ValueError
 
     with pytest.raises(ValueError):
-        await workspace_repo.create_workspace_item(workspace_input, {}, "test_object_id", ["test_role"])
+        await workspace_repo.create_workspace_item(workspace_input, "test_object_id", ["test_role"])
 
 
 def test_automatically_create_application_registration_returns_true(workspace_repo):

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

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.

config_schema.json

@@ -210,11 +210,6 @@
           "description": "Workspace AD Application. This will be created for you for future use - when creating workspaces.",
           "type": "string",
           "pattern": "^[{]?[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}[}]?$"
-        },
-        "workspace_api_client_secret": {
-          "description": "Workspace AD Application secret. This will be created for you for future use - when creating workspaces.",
-          "type": "string",
-          "minLength": 11
         }
       },
       "required": [

core/terraform/outputs.sh

@@ -33,7 +33,6 @@ fi
 # Add a few extra values to the file to help us (i.e. for local debugging api_app and resource processor)
 # shellcheck disable=SC2129
 echo "TEST_WORKSPACE_APP_ID='${WORKSPACE_API_CLIENT_ID}'" >> ../private.env
-echo "TEST_WORKSPACE_APP_SECRET='${WORKSPACE_API_CLIENT_SECRET}'" >> ../private.env
 
 # These next ones from Check Dependencies
 echo "SUBSCRIPTION_ID='${SUB_ID}'" >> ../private.env