Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleaner exit when user credentials are incorrect #2296

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
6 changes: 5 additions & 1 deletion data_safe_haven/commands/sre.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
26 changes: 16 additions & 10 deletions data_safe_haven/external/api/credentials.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should raise this exception anywhere except from Typer commands.

This module shouldn't depend on Typer.

return credential


Expand Down Expand Up @@ -214,13 +214,19 @@ 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(
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
return credential
7 changes: 6 additions & 1 deletion data_safe_haven/external/api/graph_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

from data_safe_haven import console
from data_safe_haven.exceptions import (
DataSafeHavenAzureError,
DataSafeHavenMicrosoftGraphError,
DataSafeHavenValueError,
)
Expand Down Expand Up @@ -837,7 +838,11 @@ def read_applications(self) -> Sequence[dict[str, Any]]:
"value"
]
]
except Exception as exc:
except (
DataSafeHavenAzureError,
DataSafeHavenMicrosoftGraphError,
requests.JSONDecodeError,
) as exc:
msg = "Could not load list of applications."
raise DataSafeHavenMicrosoftGraphError(msg) from exc

Expand Down
12 changes: 3 additions & 9 deletions tests/external/api/test_credentials.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
AzureCliCredential,
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,
Expand Down Expand Up @@ -36,10 +36,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.",
):
with pytest.raises(Exit):
credential.get_credential()

def test_confirm_credentials_interactive_cache(
Expand All @@ -61,10 +58,7 @@ def test_decode_token_error(
self, mock_azureclicredential_get_token_invalid # noqa: ARG002
):
credential = AzureSdkCredential(skip_confirmation=True)
with pytest.raises(
DataSafeHavenAzureError,
match="Error getting account information from Azure CLI.",
):
with pytest.raises(Exit):
credential.decode_token(credential.token)


Expand Down
Loading