From 95619ecfbe459b04ecf82214f49a7b5a4592c2de Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Thu, 14 Nov 2024 15:38:33 +0000 Subject: [PATCH 01/15] Check that a user belongs to the correct SHM domain when registering with SRE --- data_safe_haven/commands/users.py | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/data_safe_haven/commands/users.py b/data_safe_haven/commands/users.py index fe413fa781..d90783f39b 100644 --- a/data_safe_haven/commands/users.py +++ b/data_safe_haven/commands/users.py @@ -146,11 +146,22 @@ def register( # List users users = UserHandler(context, graph_api) - available_usernames = users.get_usernames_entra_id() + # available_usernames = users.get_usernames_entra_id() + available_users = users.entra_users.list() + user_dict = {user.user_principal_name.split('@')[0]: user.user_principal_name.split('@')[1] for user in available_users} usernames_to_register = [] for username in usernames: - if username in available_usernames: - usernames_to_register.append(username) + if username in user_dict.keys(): + user_domain = user_dict[username] + if user_domain != shm_config.shm.fqdn: + logger.error( + f"Username '{username}' belongs to SHM domain '{user_domain}'.\n" + f"SRE '{sre_config.name}' is associated with SHM domain '{shm_config.shm.fqdn}'.\n" + "Users can only be registered to one SHM domain.\n" + "Please use 'dsh users add' to create a new user associated with the current SHM domain." + ) + else: + usernames_to_register.append(username) else: logger.error( f"Username '{username}' does not belong to this Data Safe Haven deployment." From 2943ab281959ce534a8f6171bfc23eb5d668b998 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Thu, 14 Nov 2024 16:03:19 +0000 Subject: [PATCH 02/15] fix linting --- data_safe_haven/commands/users.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/data_safe_haven/commands/users.py b/data_safe_haven/commands/users.py index d90783f39b..6c28096ded 100644 --- a/data_safe_haven/commands/users.py +++ b/data_safe_haven/commands/users.py @@ -148,7 +148,12 @@ def register( users = UserHandler(context, graph_api) # available_usernames = users.get_usernames_entra_id() available_users = users.entra_users.list() - user_dict = {user.user_principal_name.split('@')[0]: user.user_principal_name.split('@')[1] for user in available_users} + user_dict = { + user.user_principal_name.split("@")[0]: user.user_principal_name.split("@")[ + 1 + ] + for user in available_users + } usernames_to_register = [] for username in usernames: if username in user_dict.keys(): From fdcce90075480cc3835b8f44a542c3e5a1485550 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Fri, 15 Nov 2024 10:48:06 +0000 Subject: [PATCH 03/15] get fqdn from stack output --- data_safe_haven/commands/users.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/data_safe_haven/commands/users.py b/data_safe_haven/commands/users.py index 6c28096ded..d479a1393e 100644 --- a/data_safe_haven/commands/users.py +++ b/data_safe_haven/commands/users.py @@ -10,6 +10,7 @@ from data_safe_haven.exceptions import DataSafeHavenError from data_safe_haven.external import GraphApi from data_safe_haven.logging import get_logger +from data_safe_haven.infrastructure import SREProjectManager users_command_group = typer.Typer() @@ -121,8 +122,9 @@ def register( try: shm_config = SHMConfig.from_remote(context) except DataSafeHavenError: - logger.error("Have you deployed the SHM?") - raise + msg = "Have you deployed the SHM?" + logger.error(msg) + raise DataSafeHavenError(msg) # Load Pulumi config pulumi_config = DSHPulumiConfig.from_remote(context) @@ -132,7 +134,13 @@ def register( if sre_config.name not in pulumi_config.project_names: msg = f"Could not load Pulumi settings for '{sre_config.name}'. Have you deployed the SRE?" logger.error(msg) - raise DataSafeHavenError(msg) + raise typer.Exit(1) + + sre_stack = SREProjectManager( + context=context, + config=sre_config, + pulumi_config=pulumi_config, + ) # Load GraphAPI graph_api = GraphApi.from_scopes( @@ -155,13 +163,14 @@ def register( for user in available_users } usernames_to_register = [] + shm_name = sre_stack.output("linked_shm")["name"] for username in usernames: if username in user_dict.keys(): user_domain = user_dict[username] - if user_domain != shm_config.shm.fqdn: + if shm_name not in user_domain: logger.error( f"Username '{username}' belongs to SHM domain '{user_domain}'.\n" - f"SRE '{sre_config.name}' is associated with SHM domain '{shm_config.shm.fqdn}'.\n" + f"SRE '{sre_config.name}' is associated with SHM domain '{shm_name}'.\n" "Users can only be registered to one SHM domain.\n" "Please use 'dsh users add' to create a new user associated with the current SHM domain." ) From 8b89c9c376e3f2c2a9df645e81c5f83a64bf16f6 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Fri, 15 Nov 2024 11:49:34 +0000 Subject: [PATCH 04/15] Add exports from SRE stack --- data_safe_haven/infrastructure/programs/declarative_sre.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/data_safe_haven/infrastructure/programs/declarative_sre.py b/data_safe_haven/infrastructure/programs/declarative_sre.py index 15989bbe7b..9b0a5952ba 100644 --- a/data_safe_haven/infrastructure/programs/declarative_sre.py +++ b/data_safe_haven/infrastructure/programs/declarative_sre.py @@ -420,4 +420,6 @@ def __call__(self) -> None: pulumi.export("data", data.exports) pulumi.export("ldap", ldap_group_names) pulumi.export("remote_desktop", remote_desktop.exports) + pulumi.export("sre_fqdn", networking.sre_fqdn) + pulumi.export("linked_shm", self.context.name) pulumi.export("workspaces", workspaces.exports) From d886b102c5c01f3b0dc8e5ce8f575cf50b19b2be Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Mon, 25 Nov 2024 22:10:07 +0000 Subject: [PATCH 05/15] Clarify error message and check against SRE context --- data_safe_haven/commands/users.py | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/data_safe_haven/commands/users.py b/data_safe_haven/commands/users.py index d479a1393e..01dd79e6c8 100644 --- a/data_safe_haven/commands/users.py +++ b/data_safe_haven/commands/users.py @@ -9,8 +9,8 @@ from data_safe_haven.config import ContextManager, DSHPulumiConfig, SHMConfig, SREConfig from data_safe_haven.exceptions import DataSafeHavenError from data_safe_haven.external import GraphApi -from data_safe_haven.logging import get_logger from data_safe_haven.infrastructure import SREProjectManager +from data_safe_haven.logging import get_logger users_command_group = typer.Typer() @@ -122,9 +122,8 @@ def register( try: shm_config = SHMConfig.from_remote(context) except DataSafeHavenError: - msg = "Have you deployed the SHM?" - logger.error(msg) - raise DataSafeHavenError(msg) + logger.error("Have you deployed the SHM?") + raise typer.Exit(1) # Load Pulumi config pulumi_config = DSHPulumiConfig.from_remote(context) @@ -154,7 +153,6 @@ def register( # List users users = UserHandler(context, graph_api) - # available_usernames = users.get_usernames_entra_id() available_users = users.entra_users.list() user_dict = { user.user_principal_name.split("@")[0]: user.user_principal_name.split("@")[ @@ -163,16 +161,15 @@ def register( for user in available_users } usernames_to_register = [] - shm_name = sre_stack.output("linked_shm")["name"] + shm_name = sre_stack.output("context") for username in usernames: if username in user_dict.keys(): - user_domain = user_dict[username] + user_domain = user_dict[username].split(".")[0] if shm_name not in user_domain: logger.error( - f"Username '{username}' belongs to SHM domain '{user_domain}'.\n" - f"SRE '{sre_config.name}' is associated with SHM domain '{shm_name}'.\n" - "Users can only be registered to one SHM domain.\n" - "Please use 'dsh users add' to create a new user associated with the current SHM domain." + f"Username [green]'{username}'[/green] belongs to SHM context [blue]'{user_domain}'[/blue].\n" + f"SRE [yellow]'{sre_config.name}'[/yellow] belongs to SHM context [blue]'{shm_name}'[/blue].\n" + "The user must belong to the same SHM context as the SRE." ) else: usernames_to_register.append(username) From 01585a1b0c1a283fd594803873be81dead49913b Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Mon, 25 Nov 2024 22:10:54 +0000 Subject: [PATCH 06/15] Fix linting --- data_safe_haven/commands/users.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/data_safe_haven/commands/users.py b/data_safe_haven/commands/users.py index 01dd79e6c8..b3456c8784 100644 --- a/data_safe_haven/commands/users.py +++ b/data_safe_haven/commands/users.py @@ -121,9 +121,9 @@ def register( # Load SHMConfig try: shm_config = SHMConfig.from_remote(context) - except DataSafeHavenError: + except DataSafeHavenError as exc: logger.error("Have you deployed the SHM?") - raise typer.Exit(1) + raise typer.Exit(1) from exc # Load Pulumi config pulumi_config = DSHPulumiConfig.from_remote(context) From d5042b2cd262630f4c6a644c1707fd238e178436 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Mon, 25 Nov 2024 22:11:17 +0000 Subject: [PATCH 07/15] Export context name associated with SRE as part of SRE stack --- data_safe_haven/infrastructure/programs/declarative_sre.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/data_safe_haven/infrastructure/programs/declarative_sre.py b/data_safe_haven/infrastructure/programs/declarative_sre.py index 6ce9c81d1d..d9d6f0b545 100644 --- a/data_safe_haven/infrastructure/programs/declarative_sre.py +++ b/data_safe_haven/infrastructure/programs/declarative_sre.py @@ -424,7 +424,7 @@ def __call__(self) -> None: # Export values for later use pulumi.export("data", data.exports) pulumi.export("ldap", ldap_group_names) + pulumi.export("context", self.context.name) pulumi.export("remote_desktop", remote_desktop.exports) pulumi.export("sre_fqdn", networking.sre_fqdn) - pulumi.export("linked_shm", self.context.name) pulumi.export("workspaces", workspaces.exports) From 757dcf1a3a7f0ea292aa9adb8e694cc8246287ff Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 26 Nov 2024 10:11:03 +0000 Subject: [PATCH 08/15] Use SHM FQDN in error message rather than context name, clarify error message --- data_safe_haven/commands/users.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/data_safe_haven/commands/users.py b/data_safe_haven/commands/users.py index b3456c8784..2db3cf5bd3 100644 --- a/data_safe_haven/commands/users.py +++ b/data_safe_haven/commands/users.py @@ -164,19 +164,19 @@ def register( shm_name = sre_stack.output("context") for username in usernames: if username in user_dict.keys(): - user_domain = user_dict[username].split(".")[0] + user_domain = user_dict[username] if shm_name not in user_domain: logger.error( - f"Username [green]'{username}'[/green] belongs to SHM context [blue]'{user_domain}'[/blue].\n" - f"SRE [yellow]'{sre_config.name}'[/yellow] belongs to SHM context [blue]'{shm_name}'[/blue].\n" - "The user must belong to the same SHM context as the SRE." + f"User [green]'{username}'[/green]'s principal domain name is [blue]'{user_domain}'[/blue].\n" + f"SRE [yellow]'{sre_config.name}'[/yellow] belongs to SHM domain [blue]'{shm_config.shm.fqdn}'[/blue].\n" + "The user's principal domain name must match the domain of the SRE to be registered." ) else: usernames_to_register.append(username) else: logger.error( f"Username '{username}' does not belong to this Data Safe Haven deployment." - " Please use 'dsh users add' to create it." + "Please use 'dsh users add' to create this user." ) users.register(sre_config.name, usernames_to_register) except DataSafeHavenError as exc: From 510a7149983c2cb97f9bd0d5a7d3d4b04a4c7fdb Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 26 Nov 2024 10:15:46 +0000 Subject: [PATCH 09/15] Remove unneeded export of context name --- data_safe_haven/commands/users.py | 3 +-- data_safe_haven/infrastructure/programs/declarative_sre.py | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/data_safe_haven/commands/users.py b/data_safe_haven/commands/users.py index 2db3cf5bd3..e9e96a5283 100644 --- a/data_safe_haven/commands/users.py +++ b/data_safe_haven/commands/users.py @@ -161,11 +161,10 @@ def register( for user in available_users } usernames_to_register = [] - shm_name = sre_stack.output("context") for username in usernames: if username in user_dict.keys(): user_domain = user_dict[username] - if shm_name not in user_domain: + if shm_config.shm.fqdn not in user_domain: logger.error( f"User [green]'{username}'[/green]'s principal domain name is [blue]'{user_domain}'[/blue].\n" f"SRE [yellow]'{sre_config.name}'[/yellow] belongs to SHM domain [blue]'{shm_config.shm.fqdn}'[/blue].\n" diff --git a/data_safe_haven/infrastructure/programs/declarative_sre.py b/data_safe_haven/infrastructure/programs/declarative_sre.py index d9d6f0b545..78467f201b 100644 --- a/data_safe_haven/infrastructure/programs/declarative_sre.py +++ b/data_safe_haven/infrastructure/programs/declarative_sre.py @@ -424,7 +424,6 @@ def __call__(self) -> None: # Export values for later use pulumi.export("data", data.exports) pulumi.export("ldap", ldap_group_names) - pulumi.export("context", self.context.name) pulumi.export("remote_desktop", remote_desktop.exports) pulumi.export("sre_fqdn", networking.sre_fqdn) pulumi.export("workspaces", workspaces.exports) From 309d777298356b8f1070909dd4409c76ee3233b8 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 26 Nov 2024 10:29:38 +0000 Subject: [PATCH 10/15] Remove unnecessary loading of SRE stack --- data_safe_haven/commands/users.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/data_safe_haven/commands/users.py b/data_safe_haven/commands/users.py index e9e96a5283..5dab2e7591 100644 --- a/data_safe_haven/commands/users.py +++ b/data_safe_haven/commands/users.py @@ -9,7 +9,6 @@ from data_safe_haven.config import ContextManager, DSHPulumiConfig, SHMConfig, SREConfig from data_safe_haven.exceptions import DataSafeHavenError from data_safe_haven.external import GraphApi -from data_safe_haven.infrastructure import SREProjectManager from data_safe_haven.logging import get_logger users_command_group = typer.Typer() @@ -135,12 +134,6 @@ def register( logger.error(msg) raise typer.Exit(1) - sre_stack = SREProjectManager( - context=context, - config=sre_config, - pulumi_config=pulumi_config, - ) - # Load GraphAPI graph_api = GraphApi.from_scopes( scopes=["Group.ReadWrite.All", "GroupMember.ReadWrite.All"], From bd045eaf8c33fdcf86905906cb3a31a2f49a5328 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 26 Nov 2024 12:52:18 +0000 Subject: [PATCH 11/15] user preferred_name instead of directly accessing principal name --- data_safe_haven/commands/users.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/data_safe_haven/commands/users.py b/data_safe_haven/commands/users.py index 5dab2e7591..53473f58ed 100644 --- a/data_safe_haven/commands/users.py +++ b/data_safe_haven/commands/users.py @@ -148,9 +148,7 @@ def register( users = UserHandler(context, graph_api) available_users = users.entra_users.list() user_dict = { - user.user_principal_name.split("@")[0]: user.user_principal_name.split("@")[ - 1 - ] + user.preferred_username.split("@")[0]: user.preferred_username.split("@")[1] for user in available_users } usernames_to_register = [] From 469f1d6ac57bb81e8e4bebb7b4a84d897c7af163 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 26 Nov 2024 15:40:04 +0000 Subject: [PATCH 12/15] better formatted error message --- data_safe_haven/commands/users.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/data_safe_haven/commands/users.py b/data_safe_haven/commands/users.py index 53473f58ed..b3a90c0664 100644 --- a/data_safe_haven/commands/users.py +++ b/data_safe_haven/commands/users.py @@ -158,14 +158,14 @@ def register( if shm_config.shm.fqdn not in user_domain: logger.error( f"User [green]'{username}'[/green]'s principal domain name is [blue]'{user_domain}'[/blue].\n" - f"SRE [yellow]'{sre_config.name}'[/yellow] belongs to SHM domain [blue]'{shm_config.shm.fqdn}'[/blue].\n" + f"SRE [yellow]'{sre}'[/yellow] belongs to SHM domain [blue]'{shm_config.shm.fqdn}'[/blue].\n" "The user's principal domain name must match the domain of the SRE to be registered." ) else: usernames_to_register.append(username) else: logger.error( - f"Username '{username}' does not belong to this Data Safe Haven deployment." + f"Username '{username}' does not belong to this Data Safe Haven deployment.\n" "Please use 'dsh users add' to create this user." ) users.register(sre_config.name, usernames_to_register) From 2abd14b27984ac2df81b3da60ffdf8d537927019 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 26 Nov 2024 15:40:52 +0000 Subject: [PATCH 13/15] add test for mismatched domain --- tests/commands/conftest.py | 13 +++++++++++++ tests/commands/test_users.py | 20 ++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/tests/commands/conftest.py b/tests/commands/conftest.py index d675398bfc..de60eb29d0 100644 --- a/tests/commands/conftest.py +++ b/tests/commands/conftest.py @@ -1,6 +1,8 @@ from pytest import fixture from typer.testing import CliRunner +from data_safe_haven.administration.users.entra_users import EntraUsers +from data_safe_haven.administration.users.research_user import ResearchUser from data_safe_haven.config import ( Context, ContextManager, @@ -260,3 +262,14 @@ def tmp_contexts_none(tmp_path, context_yaml): with open(config_file_path, "w") as f: f.write(context_yaml) return tmp_path + + +@fixture +def mock_entra_user_list(mocker): + test_user = ResearchUser( + given_name="Harry", + surname="Lime", + sam_account_name="harry.lime", + user_principal_name="harry.lime@acme.testing", + ) + mocker.patch.object(EntraUsers, "list", return_value=[test_user]) diff --git a/tests/commands/test_users.py b/tests/commands/test_users.py index c1b183c922..5c11e29cc9 100644 --- a/tests/commands/test_users.py +++ b/tests/commands/test_users.py @@ -52,6 +52,26 @@ def test_invalid_shm( assert result.exit_code == 1 assert "Have you deployed the SHM?" in result.stdout + def test_mismatched_domain( + self, + mock_graphapi_get_credential, # noqa: ARG002 + mock_pulumi_config_no_key_from_remote, # noqa: ARG002 + mock_shm_config_from_remote, # noqa: ARG002 + mock_sre_config_from_remote, # noqa: ARG002 + mock_entra_user_list, # noqa: ARG002 + runner, + tmp_contexts, # noqa: ARG002 + ): + result = runner.invoke( + users_command_group, ["register", "-u", "harry.lime", "sandbox"] + ) + + assert result.exit_code == 0 + assert ( + "principal domain name must match the domain of the SRE to be registered" + in result.stdout + ) + def test_invalid_sre( self, mock_pulumi_config_from_remote, # noqa: ARG002 From a1b067fcbb7219ac1a148de0e3ffa70d3dbb8849 Mon Sep 17 00:00:00 2001 From: Matt Craddock Date: Thu, 28 Nov 2024 12:09:50 +0000 Subject: [PATCH 14/15] Update data_safe_haven/commands/users.py Co-authored-by: Jim Madge --- data_safe_haven/commands/users.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/data_safe_haven/commands/users.py b/data_safe_haven/commands/users.py index b3a90c0664..661e3e91f0 100644 --- a/data_safe_haven/commands/users.py +++ b/data_safe_haven/commands/users.py @@ -153,8 +153,7 @@ def register( } usernames_to_register = [] for username in usernames: - if username in user_dict.keys(): - user_domain = user_dict[username] + if user_domain:=user_dict.get(username) if shm_config.shm.fqdn not in user_domain: logger.error( f"User [green]'{username}'[/green]'s principal domain name is [blue]'{user_domain}'[/blue].\n" From 2d148498131bcb68e2226f971bebcfdf6ba6ba9e Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Thu, 28 Nov 2024 12:13:08 +0000 Subject: [PATCH 15/15] add missing colon --- data_safe_haven/commands/users.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/data_safe_haven/commands/users.py b/data_safe_haven/commands/users.py index 661e3e91f0..80fb1a356b 100644 --- a/data_safe_haven/commands/users.py +++ b/data_safe_haven/commands/users.py @@ -153,7 +153,7 @@ def register( } usernames_to_register = [] for username in usernames: - if user_domain:=user_dict.get(username) + if user_domain := user_dict.get(username): if shm_config.shm.fqdn not in user_domain: logger.error( f"User [green]'{username}'[/green]'s principal domain name is [blue]'{user_domain}'[/blue].\n"