E2E work with scope_id from workspace properties (#1797)

* E2E work with local API

* Added scope identifier uri into tests

* Hangiver from previous method

* Try adding a sleep into the endpoint dns

* Bump the version

* Wait for the private endpoint

* Bump version

* Forgot to wait on teh sleep

* Bumped

* refactoring bug

* Purge Protection

* Bump version

* PR Comments

* More PR tweaks

* typo

* shell check comment

* Remove purge protection

* _get_app_auth_info

api_app/_version.py

@@ -1 +1 @@
-__version__ = "0.2.24"
+__version__ = "0.2.27"

api_app/api/routes/api.py

@@ -93,6 +93,11 @@ async def swagger_ui_redirect():
 workspace_swagger_router = APIRouter()
 
 
+def get_scope(workspace) -> str:
+    # Cope with the fact that scope id can have api:// at the front.
+    return f"api://{workspace['properties']['scope_id'].lstrip('api://')}/user_impersonation"
+
+
 @workspace_swagger_router.get("/workspaces/{workspace_id}/openapi.json", include_in_schema=False, name="openapi_definitions")
 async def get_openapi_json(workspace_id: str, request: Request, workspace_repo=Depends(get_repository(WorkspaceRepository))):
     global openapi_definitions
@@ -108,11 +113,9 @@ async def get_openapi_json(workspace_id: str, request: Request, workspace_repo=D
         )
 
         workspace = workspace_repo.get_workspace_by_id(workspace_id)
-        ws_app_reg_id = workspace.properties['client_id']
-        workspace_scopes = {
-            f"api://{ws_app_reg_id}/user_impersonation": "List and Get TRE Workspaces"
-        }
-        openapi_definitions[workspace_id]['components']['securitySchemes']['oauth2']['flows']['authorizationCode']['scopes'] = workspace_scopes
+        scope = {get_scope(workspace): "List and Get TRE Workspaces"}
+
+        openapi_definitions[workspace_id]['components']['securitySchemes']['oauth2']['flows']['authorizationCode']['scopes'] = scope
 
         # Add an example into every workspace_id path parameter so users don't have to cut and paste them in.
         for route in openapi_definitions[workspace_id]['paths'].values():
@@ -129,15 +132,15 @@ async def get_openapi_json(workspace_id: str, request: Request, workspace_repo=D
 async def get_workspace_swagger(workspace_id, request: Request, workspace_repo=Depends(get_repository(WorkspaceRepository))):
 
     workspace = workspace_repo.get_workspace_by_id(workspace_id)
-    ws_app_reg_id = workspace.properties['client_id']
+    scope = get_scope(workspace)
     swagger_ui_html = get_swagger_ui_html(
         openapi_url="openapi.json",
         title=request.app.title + " - Swagger UI",
         oauth2_redirect_url="/api/docs/oauth2-redirect",
         init_oauth={
             "usePkceWithAuthorizationCodeGrant": True,
             "clientId": config.SWAGGER_UI_CLIENT_ID,
-            "scopes": ["openid", "offline_access", f"api://{ws_app_reg_id}/user_impersonation"]
+            "scopes": ["openid", "offline_access", scope]
         }
     )
 

api_app/db/migrations/workspaces.py

@@ -13,22 +13,33 @@ def moveAuthInformationToProperties(self) -> bool:
         migrated = False
         for item in self.query(query=WorkspaceRepository.workspaces_query_string()):
             template_version = semantic_version.Version(item["templateVersion"])
-            if (template_version < semantic_version.Version('0.2.5') and "authInformation" in item):
-                logging.INFO(f'Found workspace {item["id"]} that needs migrating')
+            updated = False
+            if (template_version < semantic_version.Version('0.2.7')):
 
                 # Rename app_id to be client_id
-                item["properties"]["client_id"] = item["properties"]["app_id"]
-                del item["properties"]["app_id"]
-
-                # Copy authInformation into properties
-                item["properties"]["sp_id"] = item["authInformation"]["sp_id"]
-                item["properties"]["app_role_id_workspace_researcher"] = item["authInformation"]["roles"]["WorkspaceResearcher"]
-                item["properties"]["app_role_id_workspace_owner"] = item["authInformation"]["roles"]["WorkspaceOwner"]
-
-                # cleanup
-                del item["authInformation"]
-                self.update_item_dict(item)
-                logging.INFO(f'Upgraded authentication info for workspace id {item["id"]}')
-                migrated = True
+                if "app_id" in item["properties"]:
+                    item["properties"]["client_id"] = item["properties"]["app_id"]
+                    del item["properties"]["app_id"]
+                    updated = True
+
+                if "scope_id" not in item["properties"]:
+                    item["properties"]["scope_id"] = f"api://{item['properties']['client_id']}"
+                    updated = True
+
+                if "authInformation" in item:
+                    logging.INFO(f'Upgrading authInformation in workspace {item["id"]}')
+
+                    # Copy authInformation into properties
+                    item["properties"]["sp_id"] = item["authInformation"]["sp_id"]
+                    item["properties"]["app_role_id_workspace_researcher"] = item["authInformation"]["roles"]["WorkspaceResearcher"]
+                    item["properties"]["app_role_id_workspace_owner"] = item["authInformation"]["roles"]["WorkspaceOwner"]
+                    # cleanup
+                    del item["authInformation"]
+                    updated = True
+
+                if updated:
+                    self.update_item_dict(item)
+                    logging.INFO(f'Upgraded authentication info for workspace id {item["id"]}')
+                    migrated = True
 
             return migrated

api_app/services/aad_authentication.py

@@ -186,14 +186,17 @@ def _get_app_sp_graph_data(self, client_id: str) -> dict:
         graph_data = requests.get(sp_endpoint, headers=self._get_auth_header(msgraph_token)).json()
         return graph_data
 
+    # This method is called when you create a workspace and you already have an AAD App Registration
+    # to link it to. You pass in teh client_id and go and get the extra information you need from AAD
+    # If the client_id is `auto_create`, 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:
             logging.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']}
+        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']:

api_app/tests_ma/test_services/test_aad_access_service.py

@@ -43,12 +43,16 @@ def test_extract_workspace__returns_sp_id_and_roles(get_app_sp_graph_data_mock):
                 'appRoles': [
                     {'id': '1abc3', 'value': 'WorkspaceResearcher'},
                     {'id': '1abc4', 'value': 'WorkspaceOwner'},
+                ],
+                '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"
     }

docs/tre-admins/auth.md

@@ -192,7 +192,9 @@ Same as [TRE API](#authentication-tre-api).
 | API/permission name | Type | Description | Admin consent required |
 | ------------------- | ---- | ----------- | ---------------------- |
 | Microsoft Graph/User.Read (`https://graph.microsoft.com/User.Read`) | Delegated | Allows users to sign-in to the app, and allows the app to read the profile of signed-in users. It also allows the app to read basic company information of signed-in users. | No |
-| Workspace API/user_impersonation (`api://<Workspace API Application (client) ID>/user_impersonation`) | Delegated* | Allows the app to access the workspace API on behalf of the user | No | Granted for *[directory name]* |
+| Workspace API/user_impersonation (`api://<Workspace Scope ID>/user_impersonation`) | Delegated* | Allows the app to access the workspace API on behalf of the user | No | Granted for *[directory name]* |
+
+When the Workspace AAD app is registered by the aad-app-reg.sh, the `Workspace Scope Id` is the same as the Client Id. When the Workspace AAD app is created by the base workspace, the `Workspace Scope Id` will be in this format `api://<TRE_ID>_ws_<WORKSPACE_SHORT_IDENTIFIER>`
 
 #### App roles
 

e2e_tests/conftest.py

@@ -46,31 +46,3 @@ async def admin_token(verify) -> str:
         token = responseJson["access_token"]
         assert token is not None, "Token not returned"
         return token if (response.status_code == status.HTTP_200_OK) else None
-
-
-@pytest.fixture
-async def workspace_owner_token(verify) -> str:
-    async with AsyncClient(verify=verify) as client:
-
-        headers = {'Content-Type': "application/x-www-form-urlencoded"}
-        if config.TEST_ACCOUNT_CLIENT_ID != "" and config.TEST_ACCOUNT_CLIENT_SECRET != "":
-            # Use Client Credentials flow
-            payload = f"grant_type=client_credentials&client_id={config.TEST_ACCOUNT_CLIENT_ID}&client_secret={config.TEST_ACCOUNT_CLIENT_SECRET}&scope=api://{config.TEST_WORKSPACE_APP_ID}/.default"
-            url = f"https://login.microsoftonline.com/{config.AAD_TENANT_ID}/oauth2/v2.0/token"
-
-        else:
-            # Use Resource Owner Password Credentials flow
-            payload = f"grant_type=password&resource={config.TEST_WORKSPACE_APP_ID}&username={config.TEST_USER_NAME}&password={config.TEST_USER_PASSWORD}&scope=api://{config.TEST_WORKSPACE_APP_ID}/user_impersonation&client_id={config.TEST_APP_ID}"
-            url = f"https://login.microsoftonline.com/{config.AAD_TENANT_ID}/oauth2/token"
-
-        response = await client.post(url, headers=headers, content=payload)
-        try:
-            responseJson = response.json()
-        except JSONDecodeError:
-            assert False, "Failed to parse response as JSON: {}".format(response.content)
-
-        assert "access_token" in responseJson, "Failed to get access_token: {}".format(response.content)
-        token = responseJson["access_token"]
-        assert token is not None, "Token not returned"
-
-        return token if (response.status_code == status.HTTP_200_OK) else None

e2e_tests/helpers.py

@@ -5,7 +5,7 @@
 import logging
 from starlette import status
 
-
+from json import JSONDecodeError
 import config
 from resources import strings
 
@@ -33,28 +33,28 @@ def get_auth_header(token: str) -> dict:
     return {'Authorization': f'Bearer {token}'}
 
 
+def get_full_endpoint(endpoint: str) -> str:
+    if (config.TRE_URL != ""):
+        return f"{config.TRE_URL}{endpoint}"
+    else:
+        return f"https://{config.TRE_ID}.{config.RESOURCE_LOCATION}.cloudapp.azure.com{endpoint}"
+
+
 @asynccontextmanager
 async def get_template(template_name, endpoint, admin_token, verify):
     async with AsyncClient(verify=verify) as client:
         headers = {'Authorization': f'Bearer {admin_token}'}
+        full_endpoint = get_full_endpoint(endpoint)
 
-        response = await client.get(f"https://{config.TRE_ID}.{config.RESOURCE_LOCATION}.cloudapp.azure.com{endpoint}/{template_name}", headers=headers, timeout=TIMEOUT)
+        response = await client.get(f"{full_endpoint}/{template_name}", headers=headers, timeout=TIMEOUT)
         yield response
 
 
-async def post_resource(payload, endpoint, resource_type, token, admin_token, verify, method="POST", wait=True):
+async def post_resource(payload, endpoint, access_token, verify, method="POST", wait=True):
     async with AsyncClient(verify=verify, timeout=30.0) as client:
 
-        if resource_type == 'workspace':
-            auth_headers = get_auth_header(admin_token)
-        else:
-            auth_headers = get_auth_header(token)
-
-        if (config.TRE_URL != ""):
-            full_endpoint = f"{config.TRE_URL}{endpoint}"
-        else:
-            full_endpoint = f"https://{config.TRE_ID}.{config.RESOURCE_LOCATION}.cloudapp.azure.com{endpoint}"
-        LOGGER.info(f'POSTING RESOURCE TO: {full_endpoint}')
+        full_endpoint = get_full_endpoint(endpoint)
+        auth_headers = get_auth_header(access_token)
 
         if method == "POST":
             response = await client.post(full_endpoint, headers=auth_headers, json=payload, timeout=TIMEOUT)
@@ -72,17 +72,14 @@ async def post_resource(payload, endpoint, resource_type, token, admin_token, ve
         operation_endpoint = response.headers["Location"]
 
         if wait:
-            await wait_for(check_method, client, operation_endpoint, get_auth_header(token), strings.RESOURCE_STATUS_FAILED)
+            await wait_for(check_method, client, operation_endpoint, auth_headers, strings.RESOURCE_STATUS_FAILED)
 
         return resource_path, resource_id
 
 
 async def get_shared_service_id_by_name(template_name: str, verify, token) -> Optional[dict]:
     async with AsyncClient(verify=verify, timeout=TIMEOUT) as client:
-        endpoint = '/api/shared-services'
-        full_endpoint = f'https://{config.TRE_ID}.{config.RESOURCE_LOCATION}.cloudapp.azure.com{endpoint}'
-        LOGGER.info(f'URL: {full_endpoint}')
-
+        full_endpoint = get_full_endpoint('/api/shared-services')
         auth_headers = get_auth_header(token)
 
         response = await client.get(full_endpoint, headers=auth_headers, timeout=TIMEOUT)
@@ -97,18 +94,13 @@ async def get_shared_service_id_by_name(template_name: str, verify, token) -> Op
         return matching_shared_services[0]
 
 
-async def disable_and_delete_resource(endpoint, resource_type, token, admin_token, verify):
+async def disable_and_delete_resource(endpoint, access_token, verify):
     async with AsyncClient(verify=verify, timeout=TIMEOUT) as client:
 
-        if resource_type == 'workspace':
-            auth_headers = get_auth_header(admin_token)
-        else:
-            auth_headers = get_auth_header(token)
-
+        full_endpoint = get_full_endpoint(endpoint)
+        auth_headers = get_auth_header(access_token)
         auth_headers["etag"] = "*"  # for now, send in the wildcard to skip around etag checking
 
-        full_endpoint = f'https://{config.TRE_ID}.{config.RESOURCE_LOCATION}.cloudapp.azure.com{endpoint}'
-
         # disable
         payload = {"isEnabled": False}
         response = await client.patch(full_endpoint, headers=auth_headers, json=payload, timeout=TIMEOUT)
@@ -124,8 +116,7 @@ async def disable_and_delete_resource(endpoint, resource_type, token, admin_toke
         resource_id = response.json()["operation"]["resourceId"]
         operation_endpoint = response.headers["Location"]
 
-        owner_auth_headers = get_auth_header(token)
-        await wait_for(delete_done, client, operation_endpoint, owner_auth_headers, strings.RESOURCE_STATUS_DELETING_FAILED)
+        await wait_for(delete_done, client, operation_endpoint, auth_headers, strings.RESOURCE_STATUS_DELETING_FAILED)
         return resource_id
 
 
@@ -157,13 +148,13 @@ async def ping_guacamole_workspace_service(workspace_id, workspace_service_id, t
         assert (response.status_code == status.HTTP_200_OK), "Guacamole cannot be reached"
 
 
-async def wait_for(func, client, operation_endoint, headers, failure_state):
-    done, done_state, message = await func(client, operation_endoint, headers)
+async def wait_for(func, client, operation_endpoint, headers, failure_state):
+    done, done_state, message = await func(client, operation_endpoint, headers)
     while not done:
-        LOGGER.info(f'WAITING FOR OP: {operation_endoint}')
+        LOGGER.info(f'WAITING FOR OP: {operation_endpoint}')
         await asyncio.sleep(30)
 
-        done, done_state, message = await func(client, operation_endoint, headers)
+        done, done_state, message = await func(client, operation_endpoint, headers)
         LOGGER.info(f"{done}, {done_state}, {message}")
     try:
         assert done_state != failure_state
@@ -191,8 +182,9 @@ async def patch_done(client, operation_endpoint, headers):
 
 
 async def check_deployment(client, operation_endpoint, headers):
-    response = await client.get(
-        f"https://{config.TRE_ID}.{config.RESOURCE_LOCATION}.cloudapp.azure.com{operation_endpoint}", headers=headers, timeout=TIMEOUT)
+    full_endpoint = get_full_endpoint(operation_endpoint)
+
+    response = await client.get(full_endpoint, headers=headers, timeout=TIMEOUT)
     if response.status_code == 200:
         deployment_status = response.json()["operation"]["status"]
         message = response.json()["operation"]["message"]
@@ -201,3 +193,57 @@ async def check_deployment(client, operation_endpoint, headers):
         LOGGER.error(f"Non 200 response in check_deployment: {response.status_code}")
         LOGGER.error(f"Full response: {response}")
         raise Exception("Non 200 response in check_deployment")
+
+
+async def get_workspace(client, workspace_id: str, headers) -> dict:
+    full_endpoint = get_full_endpoint(f"/api/workspaces/{workspace_id}")
+
+    response = await client.get(full_endpoint, headers=headers, timeout=TIMEOUT)
+    if response.status_code == 200:
+        return response.json()["workspace"]
+    else:
+        LOGGER.error(f"Non 200 response in get_workspace: {response.status_code}")
+        LOGGER.error(f"Full response: {response}")
+        raise Exception("Non 200 response in get_workspace")
+
+
+async def get_identifier_uri(client, workspace_id: str, auth_headers) -> str:
+    workspace = await get_workspace(client, workspace_id, auth_headers)
+
+    if ("properties" not in workspace):
+        raise Exception("Properties not found in workspace.")
+
+    if ("scope_id" not in workspace["properties"]):
+        raise Exception("Scope Id not found in workspace properties.")
+
+    # Cope with the fact that scope id can have api:// at the front.
+    return f"api://{workspace['properties']['scope_id'].lstrip('api://')}"
+
+
+async def get_workspace_owner_token(admin_token, workspace_id, verify) -> Optional[str]:
+    async with AsyncClient(verify=verify) as client:
+        auth_headers = get_auth_header(admin_token)
+        scope_uri = await get_identifier_uri(client, workspace_id, auth_headers)
+
+        if config.TEST_ACCOUNT_CLIENT_ID != "" and config.TEST_ACCOUNT_CLIENT_SECRET != "":
+            # Logging in as an Enterprise Application: Use Client Credentials flow
+            payload = f"grant_type=client_credentials&client_id={config.TEST_ACCOUNT_CLIENT_ID}&client_secret={config.TEST_ACCOUNT_CLIENT_SECRET}&scope={scope_uri}/.default"
+            url = f"https://login.microsoftonline.com/{config.AAD_TENANT_ID}/oauth2/v2.0/token"
+
+        else:
+            # Logging in as a User: Use Resource Owner Password Credentials flow
+            payload = f"grant_type=password&resource={workspace_id}&username={config.TEST_USER_NAME}&password={config.TEST_USER_PASSWORD}&scope={scope_uri}/user_impersonation&client_id={config.TEST_APP_ID}"
+            url = f"https://login.microsoftonline.com/{config.AAD_TENANT_ID}/oauth2/token"
+
+        response = await client.post(url, headers=auth_headers, content=payload)
+        try:
+            responseJson = response.json()
+        except JSONDecodeError:
+            raise Exception("Failed to parse response as JSON: {}".format(response.content))
+
+        if "access_token" not in responseJson:
+            raise Exception("Failed to get access_token: {}".format(response.content))
+
+        token = responseJson["access_token"]
+
+        return token if (response.status_code == status.HTTP_200_OK) else None