From 99cbc50707952cad999196b885ad681b287a4ce9 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Wed, 13 Nov 2024 12:16:31 +0000 Subject: [PATCH 1/9] linting --- data_safe_haven/commands/sre.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/data_safe_haven/commands/sre.py b/data_safe_haven/commands/sre.py index 8c3e0b5cdc..2546d792c4 100644 --- a/data_safe_haven/commands/sre.py +++ b/data_safe_haven/commands/sre.py @@ -6,7 +6,10 @@ from data_safe_haven import console from data_safe_haven.config import ContextManager, DSHPulumiConfig, SHMConfig, SREConfig -from data_safe_haven.exceptions import DataSafeHavenConfigError, DataSafeHavenError +from data_safe_haven.exceptions import ( + DataSafeHavenConfigError, + DataSafeHavenError, +) from data_safe_haven.external import AzureSdk, GraphApi from data_safe_haven.functions import current_ip_address, ip_address_in_list from data_safe_haven.infrastructure import SREProjectManager @@ -96,6 +99,7 @@ def deploy( ) # Set Entra options application = graph_api.get_application_by_name(context.entra_application_name) + if not application: msg = f"No Entra application '{context.entra_application_name}' was found. Please redeploy your SHM." raise DataSafeHavenConfigError(msg) From 4480f503d1086efb7b366c55741d074cffd2731e Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Wed, 13 Nov 2024 12:16:54 +0000 Subject: [PATCH 2/9] Directly exit after credentials not confirmed --- data_safe_haven/external/api/credentials.py | 25 ++++++++++++--------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/data_safe_haven/external/api/credentials.py b/data_safe_haven/external/api/credentials.py index bfeb9c3aeb..30be974e9e 100644 --- a/data_safe_haven/external/api/credentials.py +++ b/data_safe_haven/external/api/credentials.py @@ -6,6 +6,7 @@ from typing import Any, ClassVar import jwt +import typer from azure.core.credentials import AccessToken, TokenCredential from azure.core.exceptions import ClientAuthenticationError from azure.identity import ( @@ -144,8 +145,7 @@ def get_credential(self) -> TokenCredential: self.logger.error( "Please authenticate with Azure: run '[green]az login[/]' using [bold]infrastructure administrator[/] credentials." ) - msg = "Error getting account information from Azure CLI." - raise DataSafeHavenAzureError(msg) from exc + raise typer.Exit(code=1) from exc return credential @@ -214,13 +214,18 @@ def callback(verification_uri: str, user_code: str, _: datetime) -> None: raise DataSafeHavenAzureError(msg) from exc # Confirm that these are the desired credentials - self.confirm_credentials_interactive( - "Microsoft Graph API", - user_name=new_auth_record.username, - user_id=new_auth_record._home_account_id.split(".")[0], - tenant_name=new_auth_record._username.split("@")[1], - tenant_id=new_auth_record._tenant_id, - ) - + try: + self.confirm_credentials_interactive( + "Microsoft Graph API", + user_name=new_auth_record.username, + user_id=new_auth_record._home_account_id.split(".")[0], + tenant_name=new_auth_record._username.split("@")[1], + tenant_id=new_auth_record._tenant_id, + ) + except (CredentialUnavailableError, DataSafeHavenValueError) as exc: + self.logger.error( + "Please authenticate with Graph API using [bold]global administrator credentials[/] for your [blue]Entra ID directory[/]." + ) + raise typer.Exit(code=1) from exc # Return the credential return credential From e9372021fafe911b9d8139dfef9b96b531a27bfa Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Fri, 15 Nov 2024 13:05:55 +0000 Subject: [PATCH 3/9] Catch more specific exceptions rather than all exceptions --- data_safe_haven/external/api/graph_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/data_safe_haven/external/api/graph_api.py b/data_safe_haven/external/api/graph_api.py index 7d3b088672..e464061cf1 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -837,7 +837,7 @@ def read_applications(self) -> Sequence[dict[str, Any]]: "value" ] ] - except Exception as exc: + except (DataSafeHavenMicrosoftGraphError, requests.JSONDecodeError) as exc: msg = "Could not load list of applications." raise DataSafeHavenMicrosoftGraphError(msg) from exc From b2311d4f2f0318959281e3de982b30e60c7b4ff4 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Fri, 15 Nov 2024 13:14:40 +0000 Subject: [PATCH 4/9] Tell user how to remove cached Graph API credential --- data_safe_haven/external/api/credentials.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/data_safe_haven/external/api/credentials.py b/data_safe_haven/external/api/credentials.py index 30be974e9e..c8bdfa8236 100644 --- a/data_safe_haven/external/api/credentials.py +++ b/data_safe_haven/external/api/credentials.py @@ -224,7 +224,9 @@ def callback(verification_uri: str, user_code: str, _: datetime) -> None: ) except (CredentialUnavailableError, DataSafeHavenValueError) as exc: self.logger.error( - "Please authenticate with Graph API using [bold]global administrator credentials[/] for your [blue]Entra ID directory[/]." + f"Delete the cached credential file [green]{authentication_record_path}[/] and\n" + "authenticate with Graph API using [bold]global administrator credentials[/] for your [blue]Entra ID directory[/].\n" + ) raise typer.Exit(code=1) from exc # Return the credential From b792c45ed3d8b89648f62b8d6c3ad8303d1b7314 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Fri, 15 Nov 2024 15:24:37 +0000 Subject: [PATCH 5/9] remove newline from error message --- data_safe_haven/external/api/credentials.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/data_safe_haven/external/api/credentials.py b/data_safe_haven/external/api/credentials.py index c8bdfa8236..2dd5be6775 100644 --- a/data_safe_haven/external/api/credentials.py +++ b/data_safe_haven/external/api/credentials.py @@ -225,7 +225,7 @@ def callback(verification_uri: str, user_code: str, _: datetime) -> None: except (CredentialUnavailableError, DataSafeHavenValueError) as exc: self.logger.error( f"Delete the cached credential file [green]{authentication_record_path}[/] and\n" - "authenticate with Graph API using [bold]global administrator credentials[/] for your [blue]Entra ID directory[/].\n" + "authenticate with Graph API using [bold]global administrator credentials[/] for your [blue]Entra ID directory[/]." ) raise typer.Exit(code=1) from exc From 041b5a4d31f88abda180d1eef1ae3a370245e53d Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Fri, 15 Nov 2024 15:25:02 +0000 Subject: [PATCH 6/9] Catch another specific error type --- data_safe_haven/external/api/graph_api.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/data_safe_haven/external/api/graph_api.py b/data_safe_haven/external/api/graph_api.py index e464061cf1..85e3a872d4 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -13,6 +13,7 @@ from data_safe_haven import console from data_safe_haven.exceptions import ( + DataSafeHavenAzureError, DataSafeHavenMicrosoftGraphError, DataSafeHavenValueError, ) @@ -837,7 +838,7 @@ def read_applications(self) -> Sequence[dict[str, Any]]: "value" ] ] - except (DataSafeHavenMicrosoftGraphError, requests.JSONDecodeError) as exc: + except (DataSafeHavenAzureError, DataSafeHavenMicrosoftGraphError, requests.JSONDecodeError) as exc: msg = "Could not load list of applications." raise DataSafeHavenMicrosoftGraphError(msg) from exc From 7ae72ab495e4535f73f3224acf7e351cd0aa2520 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Fri, 15 Nov 2024 15:25:17 +0000 Subject: [PATCH 7/9] Fix tests to reflect change in exception type --- tests/external/api/test_credentials.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/external/api/test_credentials.py b/tests/external/api/test_credentials.py index c0e631e912..f13588b316 100644 --- a/tests/external/api/test_credentials.py +++ b/tests/external/api/test_credentials.py @@ -4,8 +4,8 @@ DeviceCodeCredential, ) +from click.exceptions import Exit from data_safe_haven.directories import config_dir -from data_safe_haven.exceptions import DataSafeHavenAzureError from data_safe_haven.external.api.credentials import ( AzureSdkCredential, DeferredCredential, @@ -37,8 +37,7 @@ def test_confirm_credentials_interactive_fail( DeferredCredential.cache_ = set() credential = AzureSdkCredential(skip_confirmation=False) with pytest.raises( - DataSafeHavenAzureError, - match="Error getting account information from Azure CLI.", + Exit ): credential.get_credential() @@ -62,8 +61,7 @@ def test_decode_token_error( ): credential = AzureSdkCredential(skip_confirmation=True) with pytest.raises( - DataSafeHavenAzureError, - match="Error getting account information from Azure CLI.", + Exit ): credential.decode_token(credential.token) From 2b69d97ec9fd115854f97f028c95959a4c17e6dc Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Fri, 15 Nov 2024 16:11:51 +0000 Subject: [PATCH 8/9] fix linting --- data_safe_haven/external/api/credentials.py | 1 - data_safe_haven/external/api/graph_api.py | 6 +++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/data_safe_haven/external/api/credentials.py b/data_safe_haven/external/api/credentials.py index 2dd5be6775..d9a7cc1c6c 100644 --- a/data_safe_haven/external/api/credentials.py +++ b/data_safe_haven/external/api/credentials.py @@ -226,7 +226,6 @@ def callback(verification_uri: str, user_code: str, _: datetime) -> None: self.logger.error( f"Delete the cached credential file [green]{authentication_record_path}[/] and\n" "authenticate with Graph API using [bold]global administrator credentials[/] for your [blue]Entra ID directory[/]." - ) raise typer.Exit(code=1) from exc # Return the credential diff --git a/data_safe_haven/external/api/graph_api.py b/data_safe_haven/external/api/graph_api.py index 85e3a872d4..c118113cc2 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -838,7 +838,11 @@ def read_applications(self) -> Sequence[dict[str, Any]]: "value" ] ] - except (DataSafeHavenAzureError, DataSafeHavenMicrosoftGraphError, requests.JSONDecodeError) as exc: + except ( + DataSafeHavenAzureError, + DataSafeHavenMicrosoftGraphError, + requests.JSONDecodeError, + ) as exc: msg = "Could not load list of applications." raise DataSafeHavenMicrosoftGraphError(msg) from exc From 111db6fbdab2ccf051d7f595bf87521c98242692 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Fri, 15 Nov 2024 16:12:01 +0000 Subject: [PATCH 9/9] fix linting --- tests/external/api/test_credentials.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/tests/external/api/test_credentials.py b/tests/external/api/test_credentials.py index f13588b316..dcb7e10670 100644 --- a/tests/external/api/test_credentials.py +++ b/tests/external/api/test_credentials.py @@ -3,8 +3,8 @@ AzureCliCredential, DeviceCodeCredential, ) - from click.exceptions import Exit + from data_safe_haven.directories import config_dir from data_safe_haven.external.api.credentials import ( AzureSdkCredential, @@ -36,9 +36,7 @@ def test_confirm_credentials_interactive_fail( ): DeferredCredential.cache_ = set() credential = AzureSdkCredential(skip_confirmation=False) - with pytest.raises( - Exit - ): + with pytest.raises(Exit): credential.get_credential() def test_confirm_credentials_interactive_cache( @@ -60,9 +58,7 @@ def test_decode_token_error( self, mock_azureclicredential_get_token_invalid # noqa: ARG002 ): credential = AzureSdkCredential(skip_confirmation=True) - with pytest.raises( - Exit - ): + with pytest.raises(Exit): credential.decode_token(credential.token)