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

Check that a user belongs to the correct SHM domain when registering with an SRE #2292

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions data_safe_haven/commands/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,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
raise typer.Exit(1) from exc

# Load Pulumi config
pulumi_config = DSHPulumiConfig.from_remote(context)
Expand All @@ -132,7 +132,7 @@ 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)

# Load GraphAPI
graph_api = GraphApi.from_scopes(
Expand All @@ -146,15 +146,26 @@ def register(

# List users
users = UserHandler(context, graph_api)
available_usernames = users.get_usernames_entra_id()
available_users = users.entra_users.list()
JimMadge marked this conversation as resolved.
Show resolved Hide resolved
user_dict = {
user.preferred_username.split("@")[0]: user.preferred_username.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 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"
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."
)
Comment on lines +158 to +162
Copy link
Member

Choose a reason for hiding this comment

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

I think we've had problems with multi-line log messages before (although we might strip those characters). Could you check the output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this in the log file:
image

Seems fine to me

Copy link
Member

Choose a reason for hiding this comment

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

The problem there is we have lines in the log file which are not in the log format which will make it difficult to parse.

We could,

  • Join the lines in the log file
  • Make each of these a different message
  • Put this output to stdout but a shorter summary to the log

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."
f"Username '{username}' does not belong to this Data Safe Haven deployment.\n"
"Please use 'dsh users add' to create this user."
Comment on lines +167 to +168
Copy link
Member

Choose a reason for hiding this comment

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

Also here

)
users.register(sre_config.name, usernames_to_register)
except DataSafeHavenError as exc:
Expand Down
13 changes: 13 additions & 0 deletions tests/commands/conftest.py
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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="[email protected]",
)
mocker.patch.object(EntraUsers, "list", return_value=[test_user])
20 changes: 20 additions & 0 deletions tests/commands/test_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading