Merge pull request #1975 from jemrobinson/1814-overwrite-application-…

…secret

Fix EntraApplication resource issues

data_safe_haven/external/api/graph_api.py

@@ -20,7 +20,6 @@
 from data_safe_haven import console
 from data_safe_haven.exceptions import (
     DataSafeHavenMicrosoftGraphError,
-    DataSafeHavenValueError,
 )
 from data_safe_haven.functions import alphanumeric
 from data_safe_haven.logging import get_logger
@@ -266,27 +265,31 @@ def create_application(
     def create_application_secret(
         self, application_name: str, application_secret_name: str
     ) -> str:
-        """Add a secret to an existing Entra application
+        """Add a secret to an existing Entra application, overwriting any existing secret.
 
         Returns:
             str: Contents of newly-created secret
 
         Raises:
-            DataSafeHavenMicrosoftGraphError if the secret could not be created or already exists
+            DataSafeHavenMicrosoftGraphError if the secret could not be created
         """
         try:
             application_json = self.get_application_by_name(application_name)
             if not application_json:
                 msg = f"Could not retrieve application '{application_name}'"
                 raise DataSafeHavenMicrosoftGraphError(msg)
-            # If the secret already exists then raise an exception
-            if "passwordCredentials" in application_json and any(
-                cred["displayName"] == application_secret_name
-                for cred in application_json["passwordCredentials"]
-            ):
-                msg = f"Secret '{application_secret_name}' already exists in application '{application_name}'."
-                raise DataSafeHavenValueError(msg)
-            # Create the application secret if it does not exist
+            # If the secret already exists then remove it
+            if "passwordCredentials" in application_json:
+                for secret in application_json["passwordCredentials"]:
+                    if secret["displayName"] == application_secret_name:
+                        self.logger.debug(
+                            f"Removing pre-existing secret '{secret['displayName']}' from application '{application_name}'."
+                        )
+                        self.http_post(
+                            f"{self.base_endpoint}/applications/{application_json['id']}/removePassword",
+                            json={"keyId": secret["keyId"]},
+                        )
+            # Create the application secret
             self.logger.debug(
                 f"Creating application secret '[green]{application_secret_name}[/]'...",
             )
@@ -303,7 +306,7 @@ def create_application_secret(
                 f"{self.base_endpoint}/applications/{application_json['id']}/addPassword",
                 json=request_json,
             ).json()
-            self.logger.info(
+            self.logger.debug(
                 f"Created application secret '[green]{application_secret_name}[/]'.",
             )
             return str(json_response["secretText"])
@@ -797,14 +800,14 @@ def http_get(self, url: str, **kwargs: Any) -> requests.Response:
                 **kwargs,
             )
         except requests.exceptions.RequestException as exc:
-            msg = f"Could not execute GET request from '{url}'."
+            msg = f"Could not execute GET request to '{url}'."
             raise DataSafeHavenMicrosoftGraphError(msg) from exc
 
         # We do not use response.ok as this allows 3xx codes
         if requests.codes.OK <= response.status_code < requests.codes.MULTIPLE_CHOICES:
             return response
         else:
-            msg = f"Could not execute GET request from '{url}'. Response content received: '{response.content.decode()}'."
+            msg = f"Could not execute GET request to '{url}'. Response content received: '{response.content.decode()}'. Token {self.token}"
             raise DataSafeHavenMicrosoftGraphError(msg)
 
     def http_patch(self, url: str, **kwargs: Any) -> requests.Response:

data_safe_haven/infrastructure/components/dynamic/dsh_resource_provider.py

@@ -43,8 +43,7 @@ def partial_diff(
             delete_before_replace=True,  # delete the existing resource before replacing
         )
 
-    @staticmethod
-    def refresh(props: dict[str, Any]) -> dict[str, Any]:
+    def refresh(self, props: dict[str, Any]) -> dict[str, Any]:
         return dict(**props)
 
     def check(

data_safe_haven/infrastructure/components/dynamic/entra_application.py

@@ -18,7 +18,6 @@ class EntraApplicationProps:
     def __init__(
         self,
         application_name: Input[str],
-        auth_token: Input[str],
         application_role_assignments: Input[list[str]] | None = None,
         application_secret_name: Input[str] | None = None,
         delegated_role_assignments: Input[list[str]] | None = None,
@@ -28,19 +27,21 @@ def __init__(
         self.application_name = application_name
         self.application_role_assignments = application_role_assignments
         self.application_secret_name = application_secret_name
-        self.auth_token = Output.secret(auth_token)
         self.delegated_role_assignments = delegated_role_assignments
         self.public_client_redirect_uri = public_client_redirect_uri
         self.web_redirect_url = web_redirect_url
 
 
 class EntraApplicationProvider(DshResourceProvider):
-    @staticmethod
-    def refresh(props: dict[str, Any]) -> dict[str, Any]:
+    def __init__(self, auth_token: str):
+        self.auth_token = auth_token
+        super().__init__()
+
+    def refresh(self, props: dict[str, Any]) -> dict[str, Any]:
         try:
             outs = dict(**props)
             with suppress(DataSafeHavenMicrosoftGraphError):
-                graph_api = GraphApi(auth_token=outs["auth_token"])
+                graph_api = GraphApi(auth_token=self.auth_token)
                 if json_response := graph_api.get_application_by_name(
                     outs["application_name"]
                 ):
@@ -66,7 +67,7 @@ def create(self, props: dict[str, Any]) -> CreateResult:
         """Create new Entra application."""
         outs = dict(**props)
         try:
-            graph_api = GraphApi(auth_token=props["auth_token"])
+            graph_api = GraphApi(auth_token=self.auth_token)
             request_json = {
                 "displayName": props["application_name"],
                 "signInAudience": "AzureADMyOrg",
@@ -122,7 +123,7 @@ def delete(self, id_: str, props: dict[str, Any]) -> None:
         # Use `id` as a no-op to avoid ARG002 while maintaining function signature
         id(id_)
         try:
-            graph_api = GraphApi(auth_token=props["auth_token"])
+            graph_api = GraphApi(auth_token=self.auth_token)
             graph_api.delete_application(props["application_name"])
         except Exception as exc:
             msg = f"Failed to delete application '{props['application_name']}' from Entra ID."
@@ -137,8 +138,7 @@ def diff(
         """Calculate diff between old and new state"""
         # Use `id` as a no-op to avoid ARG002 while maintaining function signature
         id(id_)
-        # Exclude "auth_token" which should not trigger a diff
-        return self.partial_diff(old_props, new_props, ["auth_token"])
+        return self.partial_diff(old_props, new_props)
 
     def update(
         self,
@@ -150,7 +150,6 @@ def update(
         try:
             # Delete the old application, using the auth token from new_props
             old_props_ = {**old_props}
-            old_props_["auth_token"] = new_props["auth_token"]
             self.delete(id_, old_props_)
             # Create a new application
             updated = self.create(new_props)
@@ -170,10 +169,11 @@ def __init__(
         self,
         name: str,
         props: EntraApplicationProps,
+        auth_token: str,
         opts: ResourceOptions | None = None,
     ):
         super().__init__(
-            EntraApplicationProvider(),
+            EntraApplicationProvider(auth_token),
             name,
             {
                 "application_id": None,

data_safe_haven/infrastructure/components/dynamic/file_share_file.py

@@ -66,8 +66,7 @@ def get_file_client(
             credential=storage_account_key,
         )
 
-    @staticmethod
-    def refresh(props: dict[str, Any]) -> dict[str, Any]:
+    def refresh(self, props: dict[str, Any]) -> dict[str, Any]:
         with suppress(Exception):
             file_client = FileShareFileProvider.get_file_client(
                 props["storage_account_name"],

data_safe_haven/infrastructure/components/dynamic/ssl_certificate.py

@@ -43,8 +43,7 @@ def __init__(
 
 
 class SSLCertificateProvider(DshResourceProvider):
-    @staticmethod
-    def refresh(props: dict[str, Any]) -> dict[str, Any]:
+    def refresh(self, props: dict[str, Any]) -> dict[str, Any]:
         try:
             outs = dict(**props)
             with suppress(DataSafeHavenAzureError):

data_safe_haven/infrastructure/programs/sre/identity.py

@@ -25,7 +25,7 @@ def __init__(
         dns_resource_group_name: Input[str],
         dns_server_ip: Input[str],
         entra_application_name: Input[str],
-        entra_auth_token: Input[str],
+        entra_auth_token: str,
         entra_tenant_id: Input[str],
         location: Input[str],
         networking_resource_group_name: Input[str],
@@ -99,10 +99,10 @@ def __init__(
                 application_name=props.entra_application_name,
                 application_role_assignments=["User.Read.All", "GroupMember.Read.All"],
                 application_secret_name="Apricot Authentication Secret",
-                auth_token=props.entra_auth_token,
                 delegated_role_assignments=["User.Read.All"],
                 public_client_redirect_uri="urn:ietf:wg:oauth:2.0:oob",
             ),
+            auth_token=props.entra_auth_token,
             opts=child_opts,
         )
 

data_safe_haven/infrastructure/programs/sre/remote_desktop.py

@@ -37,7 +37,7 @@ def __init__(
         dns_server_ip: Input[str],
         entra_application_fqdn: Input[str],
         entra_application_name: Input[str],
-        entra_auth_token: Input[str],
+        entra_auth_token: str,
         entra_tenant_id: Input[str],
         ldap_group_filter: Input[str],
         ldap_group_search_base: Input[str],
@@ -135,9 +135,9 @@ def __init__(
             f"{self._name}_entra_application",
             EntraApplicationProps(
                 application_name=props.entra_application_name,
-                auth_token=props.entra_auth_token,
                 web_redirect_url=props.entra_application_url,
             ),
+            auth_token=props.entra_auth_token,
             opts=child_opts,
         )
 

data_safe_haven/infrastructure/project_manager.py

@@ -135,8 +135,9 @@ def stack(self) -> automation.Stack:
                 )
                 self.logger.info(f"Loaded stack [green]{self.stack_name}[/].")
                 # Ensure encrypted key is stored in the Pulumi configuration
-                self.update_dsh_pulumi_config()
+                self.update_dsh_pulumi_encrypted_key()
             except automation.CommandError as exc:
+                self.log_exception(exc)
                 msg = f"Could not load Pulumi stack {self.stack_name}."
                 raise DataSafeHavenPulumiError(msg) from exc
         return self._stack
@@ -228,6 +229,7 @@ def destroy(self) -> None:
                     self._stack.workspace.remove_stack(self.stack_name)
                 self.logger.info(f"Removed Pulumi stack [green]{self.stack_name}[/].")
             except automation.CommandError as exc:
+                self.log_exception(exc)
                 if "no stack named" not in str(exc):
                     msg = "Pulumi stack could not be removed."
                     raise DataSafeHavenPulumiError(msg) from exc
@@ -289,6 +291,11 @@ def install_plugins(self) -> None:
             msg = "Installing Pulumi plugins failed.."
             raise DataSafeHavenPulumiError(msg) from exc
 
+    def log_exception(self, exc: automation.CommandError) -> None:
+        with suppress(KeyError):
+            stderr = str(exc).split("\n")[3].replace(" stderr: ", "")
+            self.log_message(f"Pulumi output: {stderr}")
+
     def log_message(self, message: str) -> None:
         return from_ansi(self.logger, message)
 
@@ -322,6 +329,7 @@ def refresh(self) -> None:
             # Note that we disable parallelisation which can cause deadlock
             self.stack.refresh(parallel=1, **self.pulumi_extra_args)
         except automation.CommandError as exc:
+            self.log_exception(exc)
             msg = "Pulumi refresh failed."
             raise DataSafeHavenPulumiError(msg) from exc
 
@@ -331,6 +339,7 @@ def run_pulumi_command(self, command: str) -> str:
             result = self.stack._run_pulumi_cmd_sync(command.split())
             return str(result.stdout)
         except automation.CommandError as exc:
+            self.log_exception(exc)
             msg = "Failed to run command."
             raise DataSafeHavenPulumiError(msg) from exc
 
@@ -339,6 +348,7 @@ def secret(self, name: str) -> str:
         try:
             return str(self.stack.get_config(name).value)
         except automation.CommandError as exc:
+            self.log_exception(exc)
             msg = f"Secret '{name}' was not found."
             raise DataSafeHavenPulumiError(msg) from exc
 
@@ -352,8 +362,8 @@ def teardown(self) -> None:
         try:
             self.refresh()
             self.destroy()
-            self.update_dsh_pulumi_project()
         except Exception as exc:
+            self.log_exception(exc)
             msg = "Tearing down Pulumi infrastructure failed.."
             raise DataSafeHavenPulumiError(msg) from exc
 
@@ -367,6 +377,7 @@ def update(self) -> None:
             self.evaluate(result.summary.result)
             self.update_dsh_pulumi_project()
         except automation.CommandError as exc:
+            self.log_exception(exc)
             msg = "Pulumi update failed."
             raise DataSafeHavenPulumiError(msg) from exc
 
@@ -377,13 +388,13 @@ def update_dsh_pulumi_project(self) -> None:
         }
         self.pulumi_project.stack_config = all_config_dict
 
-    def update_dsh_pulumi_config(self) -> None:
-        """Update persistent data in the DSHPulumiProject object"""
+    def update_dsh_pulumi_encrypted_key(self) -> None:
+        """Update encrypted key in the DSHPulumiProject object"""
         stack_key = self.stack.workspace.stack_settings(
             stack_name=self.stack_name
         ).encrypted_key
 
-        if self.pulumi_config.encrypted_key is None:
+        if not self.pulumi_config.encrypted_key:
             self.pulumi_config.encrypted_key = stack_key
         elif self.pulumi_config.encrypted_key != stack_key:
             msg = "Stack encrypted key does not match project encrypted key"