From 4b47173b87336271dc6da5b1e46efdedc828e784 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Wed, 14 Feb 2024 15:43:26 +0000 Subject: [PATCH 01/18] :sparkles: Allow AzureADApplication to use public_client redirect as well as web. Also allow AzureADApplication to set up an application secret --- data_safe_haven/external/api/graph_api.py | 2 +- .../components/dynamic/azuread_application.py | 46 ++++++++++++++----- .../stacks/sre/remote_desktop.py | 2 +- 3 files changed, 37 insertions(+), 13 deletions(-) diff --git a/data_safe_haven/external/api/graph_api.py b/data_safe_haven/external/api/graph_api.py index 8edfa063a1..cf3be8d3db 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -261,7 +261,7 @@ def create_application( raise DataSafeHavenMicrosoftGraphError(msg) from exc def create_application_secret( - self, application_secret_name: str, application_name: str + self, application_name: str, application_secret_name: str ) -> str: """Add a secret to an existing AzureAD application diff --git a/data_safe_haven/infrastructure/components/dynamic/azuread_application.py b/data_safe_haven/infrastructure/components/dynamic/azuread_application.py index 253231804b..89aa7904d4 100644 --- a/data_safe_haven/infrastructure/components/dynamic/azuread_application.py +++ b/data_safe_haven/infrastructure/components/dynamic/azuread_application.py @@ -18,12 +18,16 @@ class AzureADApplicationProps: def __init__( self, application_name: Input[str], - application_url: Input[str], auth_token: Input[str], + application_secret_name: Input[str] | None = None, + public_client_redirect_url: Input[str] | None = None, + web_redirect_url: Input[str] | None = None, ) -> None: self.application_name = application_name - self.application_url = application_url + self.application_secret_name = application_secret_name self.auth_token = auth_token + self.public_client_redirect_url = public_client_redirect_url + self.web_redirect_url = web_redirect_url class AzureADApplicationProvider(DshResourceProvider): @@ -50,19 +54,33 @@ def create(self, props: dict[str, Any]) -> CreateResult: outs = dict(**props) try: graph_api = GraphApi(auth_token=props["auth_token"], disable_logging=True) + request_json = { + "displayName": props["application_name"], + "signInAudience": "AzureADMyOrg", + } + if props.get("web_redirect_url", None): + request_json["web"] = { + "redirectUris": [props["web_redirect_url"]], + "implicitGrantSettings": {"enableIdTokenIssuance": True}, + } + if props.get("public_client_redirect_url", None): + request_json["publicClient"] = { + "redirectUris": [props["public_client_redirect_url"]], + } json_response = graph_api.create_application( props["application_name"], - request_json={ - "displayName": props["application_name"], - "web": { - "redirectUris": [props["application_url"]], - "implicitGrantSettings": {"enableIdTokenIssuance": True}, - }, - "signInAudience": "AzureADMyOrg", - }, + request_json=request_json, ) outs["object_id"] = json_response["id"] outs["application_id"] = json_response["appId"] + + if props.get("application_secret_name", None): + outs["application_secret"] = graph_api.create_application_secret( + props["application_name"], + props["application_secret_name"], + ) + else: + outs["application_secret"] = "" except Exception as exc: msg = f"Failed to create application [green]{props['application_name']}[/] in AzureAD.\n{exc}" raise DataSafeHavenMicrosoftGraphError(msg) from exc @@ -116,6 +134,7 @@ def update( class AzureADApplication(Resource): application_id: Output[str] + application_secret: Output[str] object_id: Output[str] _resource_type_name = "dsh:common:AzureADApplication" # set resource type @@ -128,6 +147,11 @@ def __init__( super().__init__( AzureADApplicationProvider(), name, - {"application_id": None, "object_id": None, **vars(props)}, + { + "application_id": None, + "application_secret": None, + "object_id": None, + **vars(props), + }, opts, ) diff --git a/data_safe_haven/infrastructure/stacks/sre/remote_desktop.py b/data_safe_haven/infrastructure/stacks/sre/remote_desktop.py index 0ea737e7d5..27263ab814 100644 --- a/data_safe_haven/infrastructure/stacks/sre/remote_desktop.py +++ b/data_safe_haven/infrastructure/stacks/sre/remote_desktop.py @@ -136,8 +136,8 @@ def __init__( f"{self._name}_aad_application", AzureADApplicationProps( application_name=props.aad_application_name, - application_url=props.aad_application_url, auth_token=props.aad_auth_token, + web_redirect_url=props.aad_application_url, ), opts=child_opts, ) From fde6eafbbc0bcf3c6ef2516a8ddbd27098b05718 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Mon, 19 Feb 2024 14:37:33 +0000 Subject: [PATCH 02/18] :recycle: Grant permissions programmatically rather than requiring manual consent --- data_safe_haven/external/api/graph_api.py | 43 +++++++++-------------- 1 file changed, 16 insertions(+), 27 deletions(-) diff --git a/data_safe_haven/external/api/graph_api.py b/data_safe_haven/external/api/graph_api.py index cf3be8d3db..aeaff7117b 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -225,35 +225,24 @@ def create_application( self.logger.info( f"Created new application '[green]{json_response['displayName']}[/]'.", ) + + # Ensure that the application service principal exists + application_sp = self.ensure_application_service_principal(application_name) + # Grant admin consent for the requested scopes if application_scopes or delegated_scopes: - application_id = self.get_id_from_application_name(application_name) - application_sp = self.get_service_principal_by_name(application_name) - if not ( - application_sp - and self.read_application_permissions(application_sp["id"]) - ): - self.logger.info( - f"Application [green]{application_name}[/] has requested permissions" - " that need administrator approval." - ) - self.logger.info( - "Please sign-in with [bold]global administrator[/] credentials for the" - " Azure Active Directory where your users are stored." - ) - self.logger.info( - "To sign in, use a web browser to open the page" - f" [green]https://login.microsoftonline.com/{self.tenant_id}/adminconsent?client_id=" - f"{application_id}&redirect_uri=https://login.microsoftonline.com/common/oauth2/nativeclient[/]" - " and follow the instructions." - ) - while True: - if application_sp := self.get_service_principal_by_name( - application_name - ): - if self.read_application_permissions(application_sp["id"]): - break - time.sleep(10) + for scope in application_scopes: + self.grant_application_role_permissions(application_name, scope) + for scope in delegated_scopes: + self.grant_delegated_role_permissions(application_name, scope) + while True: + if application_sp := self.get_service_principal_by_name( + application_name + ): + if self.read_application_permissions(application_sp["id"]): + break + time.sleep(10) + # Return JSON representation of the AzureAD application return json_response except Exception as exc: From 83d97d3204d27413e92bbfb5a763c82c38ee4025 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Thu, 15 Feb 2024 15:34:34 +0000 Subject: [PATCH 03/18] :sparkles: Add GraphAPI function for granting application role permissions --- data_safe_haven/external/api/graph_api.py | 45 ++++++++++++++++++- .../components/dynamic/azuread_application.py | 27 +++++++---- 2 files changed, 63 insertions(+), 9 deletions(-) diff --git a/data_safe_haven/external/api/graph_api.py b/data_safe_haven/external/api/graph_api.py index aeaff7117b..4308baf8b0 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -44,6 +44,9 @@ class GraphApi: """Interface to the Microsoft Graph REST API""" linux_schema = "extj8xolrvw_linux" # this is the "Extension with Properties for Linux User and Groups" extension + application_ids: ClassVar[dict[str, str]] = { + "Microsoft Graph": "00000003-0000-0000-c000-000000000000", + } role_template_ids: ClassVar[dict[str, str]] = { "Global Administrator": "62e90394-69f5-4237-9190-012177145e10" } @@ -52,6 +55,7 @@ class GraphApi: "Domain.Read.All": "dbb9058a-0e50-45d7-ae91-66909b5d4664", "Group.Read.All": "5b567255-7703-4780-807c-7be8301ae99b", "Group.ReadWrite.All": "62a82d76-70ea-41e2-9197-370581804d09", + "GroupMember.Read.All": "98830695-27a2-44f7-8c18-0c3ebc9698f6", "User.Read.All": "df021288-bdef-4463-88db-98f22de89214", "User.ReadWrite.All": "741f803b-c850-494e-b5df-cde7c675a1ca", "UserAuthenticationMethod.ReadWrite.All": "50483e42-d915-4231-9639-7fdb7fd190e5", @@ -213,7 +217,7 @@ def create_application( if scopes: request_json["requiredResourceAccess"] = [ { - "resourceAppId": "00000003-0000-0000-c000-000000000000", # Microsoft Graph: https://graph.microsoft.com + "resourceAppId": self.application_ids["Microsoft Graph"], "resourceAccess": scopes, } ] @@ -568,6 +572,45 @@ def get_id_from_username(self, username: str) -> str | None: except (DataSafeHavenMicrosoftGraphError, StopIteration): return None + def grant_application_role_permissions( + self, application_name: str, application_role_name: str + ) -> None: + """ + Grant permissions for a particular role to an application. + See https://learn.microsoft.com/en-us/graph/permissions-grant-via-msgraph + + Raises: + DataSafeHavenMicrosoftGraphError if the secret could not be created or already exists + """ + try: + microsoft_graph_sp = self.get_service_principal_by_name("Microsoft Graph") + if not microsoft_graph_sp: + msg = f"Could not find Microsoft Graph service principal." + raise DataSafeHavenMicrosoftGraphError(msg) + application_sp = self.get_service_principal_by_name(application_name) + if not application_sp: + msg = f"Could not find application service principal." + raise DataSafeHavenMicrosoftGraphError(msg) + # Create the application secret if it does not exist + self.logger.debug( + f"Assigning application role '[green]{application_role_name}[/]' to '{application_name}'...", + ) + request_json = { + "principalId": application_sp["id"], + "resourceId": microsoft_graph_sp["id"], + "appRoleId": self.uuid_application[application_role_name], + } + self.http_post( + f"{self.base_endpoint}/servicePrincipals/{microsoft_graph_sp['id']}/appRoleAssignments", + json=request_json, + ) + self.logger.info( + f"Assigned application role '[green]{application_role_name}[/]' to '{application_name}'.", + ) + except Exception as exc: + msg = f"Could not assign application role '{application_role_name}'.\n{exc}" + raise DataSafeHavenMicrosoftGraphError(msg) from exc + def http_delete(self, url: str, **kwargs: Any) -> requests.Response: """Make an HTTP DELETE request diff --git a/data_safe_haven/infrastructure/components/dynamic/azuread_application.py b/data_safe_haven/infrastructure/components/dynamic/azuread_application.py index 89aa7904d4..8a0acd9979 100644 --- a/data_safe_haven/infrastructure/components/dynamic/azuread_application.py +++ b/data_safe_haven/infrastructure/components/dynamic/azuread_application.py @@ -19,14 +19,16 @@ 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, - public_client_redirect_url: Input[str] | None = None, + public_client_redirect_uri: Input[str] | None = None, web_redirect_url: Input[str] | None = None, ) -> None: self.application_name = application_name + self.application_role_assignments = application_role_assignments self.application_secret_name = application_secret_name self.auth_token = auth_token - self.public_client_redirect_url = public_client_redirect_url + self.public_client_redirect_uri = public_client_redirect_uri self.web_redirect_url = web_redirect_url @@ -58,29 +60,38 @@ def create(self, props: dict[str, Any]) -> CreateResult: "displayName": props["application_name"], "signInAudience": "AzureADMyOrg", } + # Add a web redirection URL if requested if props.get("web_redirect_url", None): request_json["web"] = { "redirectUris": [props["web_redirect_url"]], "implicitGrantSettings": {"enableIdTokenIssuance": True}, } - if props.get("public_client_redirect_url", None): + # Add a public client redirection URL if requested + if props.get("public_client_redirect_uri", None): request_json["publicClient"] = { - "redirectUris": [props["public_client_redirect_url"]], + "redirectUris": [props["public_client_redirect_uri"]], } json_response = graph_api.create_application( props["application_name"], + application_scopes=props.get("application_role_assignments", []), request_json=request_json, ) outs["object_id"] = json_response["id"] outs["application_id"] = json_response["appId"] - if props.get("application_secret_name", None): - outs["application_secret"] = graph_api.create_application_secret( + # Grant any requested application role permissions + for role_name in props.get("application_role_assignments", []): + graph_api.grant_application_role_permissions(outs["application_name"], role_name) + + # Attach an application secret if requested + outs["application_secret"] = ( + graph_api.create_application_secret( props["application_name"], props["application_secret_name"], ) - else: - outs["application_secret"] = "" + if props.get("application_secret_name", None) + else "" + ) except Exception as exc: msg = f"Failed to create application [green]{props['application_name']}[/] in AzureAD.\n{exc}" raise DataSafeHavenMicrosoftGraphError(msg) from exc From 06fc7350235541803cb777772b6a515710e5e83e Mon Sep 17 00:00:00 2001 From: James Robinson Date: Mon, 19 Feb 2024 14:35:28 +0000 Subject: [PATCH 04/18] :sparkles: Add GraphAPI function for granting delegated application role permissions --- data_safe_haven/external/api/graph_api.py | 79 +++++++++++++++++++++-- 1 file changed, 73 insertions(+), 6 deletions(-) diff --git a/data_safe_haven/external/api/graph_api.py b/data_safe_haven/external/api/graph_api.py index 4308baf8b0..bfe7165afc 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -583,24 +583,38 @@ def grant_application_role_permissions( DataSafeHavenMicrosoftGraphError if the secret could not be created or already exists """ try: + # Get service principals for Microsoft Graph and this application microsoft_graph_sp = self.get_service_principal_by_name("Microsoft Graph") if not microsoft_graph_sp: - msg = f"Could not find Microsoft Graph service principal." + msg = "Could not find Microsoft Graph service principal." raise DataSafeHavenMicrosoftGraphError(msg) application_sp = self.get_service_principal_by_name(application_name) if not application_sp: - msg = f"Could not find application service principal." + msg = "Could not find application service principal." raise DataSafeHavenMicrosoftGraphError(msg) - # Create the application secret if it does not exist + # Check whether permission is already granted + app_role_id = self.uuid_application[application_role_name] + response = self.http_get( + f"{self.base_endpoint}/servicePrincipals/{microsoft_graph_sp['id']}/appRoleAssignedTo", + ) + for application in response.json().get("value", []): + if (application["appRoleId"] == app_role_id) and ( + application["principalDisplayName"] == application_name + ): + self.logger.debug( + f"Application role '[green]{application_role_name}[/]' already assigned to '{application_name}'.", + ) + return + # Otherwise grant permissions for this role to the application self.logger.debug( f"Assigning application role '[green]{application_role_name}[/]' to '{application_name}'...", ) request_json = { "principalId": application_sp["id"], "resourceId": microsoft_graph_sp["id"], - "appRoleId": self.uuid_application[application_role_name], + "appRoleId": app_role_id, } - self.http_post( + response = self.http_post( f"{self.base_endpoint}/servicePrincipals/{microsoft_graph_sp['id']}/appRoleAssignments", json=request_json, ) @@ -608,7 +622,60 @@ def grant_application_role_permissions( f"Assigned application role '[green]{application_role_name}[/]' to '{application_name}'.", ) except Exception as exc: - msg = f"Could not assign application role '{application_role_name}'.\n{exc}" + msg = f"Could not assign application role '{application_role_name}' to application '{application_name}'.\n{exc}" + raise DataSafeHavenMicrosoftGraphError(msg) from exc + + def grant_delegated_role_permissions( + self, application_name: str, application_role_name: str + ) -> None: + """ + Grant permissions for a particular role to an application. + See https://learn.microsoft.com/en-us/graph/permissions-grant-via-msgraph + + Raises: + DataSafeHavenMicrosoftGraphError if the secret could not be created or already exists + """ + try: + # Get service principals for Microsoft Graph and this application + microsoft_graph_sp = self.get_service_principal_by_name("Microsoft Graph") + if not microsoft_graph_sp: + msg = "Could not find Microsoft Graph service principal." + raise DataSafeHavenMicrosoftGraphError(msg) + application_sp = self.get_service_principal_by_name(application_name) + if not application_sp: + msg = "Could not find application service principal." + raise DataSafeHavenMicrosoftGraphError(msg) + # Check whether permission is already granted + response = self.http_get( + f"{self.base_endpoint}/oauth2PermissionGrants", + ) + for application in response.json().get("value", []): + if (application["clientId"] == application_sp["id"]) and ( + application_role_name.lower() in application["scope"].lower() + ): + self.logger.debug( + f"Delegated permission '[green]{application_role_name}[/]' already assigned to '{application_name}'.", + ) + return + # Otherwise grant delegated permissions for this role to the application + self.logger.debug( + f"Assigning delegated role '[green]{application_role_name}[/]' to '{application_name}'...", + ) + request_json = { + "clientId": application_sp["id"], + "consentType": "AllPrincipals", + "resourceId": microsoft_graph_sp["id"], + "scope": application_role_name, + } + response = self.http_post( + f"{self.base_endpoint}/oauth2PermissionGrants", + json=request_json, + ) + self.logger.info( + f"Assigned delegated role '[green]{application_role_name}[/]' to '{application_name}'.", + ) + except Exception as exc: + msg = f"Could not assign delegated role '{application_role_name}' to application '{application_name}'.\n{exc}" raise DataSafeHavenMicrosoftGraphError(msg) from exc def http_delete(self, url: str, **kwargs: Any) -> requests.Response: From 1c1443cebf049ca9fdbe2815d8f2525339fc8cb5 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Mon, 19 Feb 2024 14:38:06 +0000 Subject: [PATCH 05/18] :sparkles: Add service principal creation --- data_safe_haven/external/api/graph_api.py | 40 ++++++++++++++++++- .../components/dynamic/azuread_application.py | 4 +- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/data_safe_haven/external/api/graph_api.py b/data_safe_haven/external/api/graph_api.py index bfe7165afc..d927d4d8a0 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -316,7 +316,6 @@ def create_group(self, group_name: str, group_id: str) -> None: self.logger.debug( f"Creating AzureAD group '[green]{group_name}[/]'...", ) - endpoint = f"{self.base_endpoint}/groups" request_json = { "displayName": group_name, "groupTypes": [], @@ -325,7 +324,7 @@ def create_group(self, group_name: str, group_id: str) -> None: "securityEnabled": True, } json_response = self.http_post( - endpoint, + f"{self.base_endpoint}/groups", json=request_json, ).json() # Add Linux group name and ID @@ -346,6 +345,43 @@ def create_group(self, group_name: str, group_id: str) -> None: msg = f"Could not create AzureAD group '{group_name}'.\n{exc}" raise DataSafeHavenMicrosoftGraphError(msg) from exc + def ensure_application_service_principal( + self, application_name: str + ) -> dict[str, Any]: + """Create a service principal for an AzureAD application if it does not already exist + + Raises: + DataSafeHavenMicrosoftGraphError if the service principal could not be created + """ + try: + # Return existing service principal if there is one + application_sp = self.get_service_principal_by_name(application_name) + if not application_sp: + # Otherwise we need to try + self.logger.debug( + f"Creating service principal for application '[green]{application_name}[/]'...", + ) + application_json = self.get_application_by_name(application_name) + if not application_json: + msg = f"Could not retrieve application '{application_name}'" + raise DataSafeHavenMicrosoftGraphError(msg) + self.http_post( + f"{self.base_endpoint}/servicePrincipals", + json={"appId": application_json["appId"]}, + ).json() + self.logger.info( + f"Created service principal for application '[green]{application_name}[/]'.", + ) + application_sp = self.get_service_principal_by_name(application_name) + if not application_sp: + raise DataSafeHavenMicrosoftGraphError( + f"service principal for application '[green]{application_name}[/]' not found." + ) + return application_sp + except Exception as exc: + msg = f"Could not create service principal for application '[green]{application_name}[/]'.\n{exc}" + raise DataSafeHavenMicrosoftGraphError(msg) from exc + def create_token_administrator(self) -> str: """Create an access token for a global administrator diff --git a/data_safe_haven/infrastructure/components/dynamic/azuread_application.py b/data_safe_haven/infrastructure/components/dynamic/azuread_application.py index 8a0acd9979..a5bc972270 100644 --- a/data_safe_haven/infrastructure/components/dynamic/azuread_application.py +++ b/data_safe_haven/infrastructure/components/dynamic/azuread_application.py @@ -81,7 +81,9 @@ def create(self, props: dict[str, Any]) -> CreateResult: # Grant any requested application role permissions for role_name in props.get("application_role_assignments", []): - graph_api.grant_application_role_permissions(outs["application_name"], role_name) + graph_api.grant_application_role_permissions( + outs["application_name"], role_name + ) # Attach an application secret if requested outs["application_secret"] = ( From c3db73fa37e0f0f45ef43b8e7f441df988fe64c1 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Tue, 20 Feb 2024 10:33:05 +0000 Subject: [PATCH 06/18] :sparkles: Add delegated_role_assignments option to AzureADApplication. --- data_safe_haven/external/api/graph_api.py | 65 ++++++++++--------- .../components/dynamic/azuread_application.py | 11 +++- 2 files changed, 46 insertions(+), 30 deletions(-) diff --git a/data_safe_haven/external/api/graph_api.py b/data_safe_haven/external/api/graph_api.py index d927d4d8a0..2dc819f7b2 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -374,9 +374,8 @@ def ensure_application_service_principal( ) application_sp = self.get_service_principal_by_name(application_name) if not application_sp: - raise DataSafeHavenMicrosoftGraphError( - f"service principal for application '[green]{application_name}[/]' not found." - ) + msg = f"service principal for application '[green]{application_name}[/]' not found." + raise DataSafeHavenMicrosoftGraphError(msg) return application_sp except Exception as exc: msg = f"Could not create service principal for application '[green]{application_name}[/]'.\n{exc}" @@ -681,32 +680,40 @@ def grant_delegated_role_permissions( if not application_sp: msg = "Could not find application service principal." raise DataSafeHavenMicrosoftGraphError(msg) - # Check whether permission is already granted - response = self.http_get( - f"{self.base_endpoint}/oauth2PermissionGrants", - ) - for application in response.json().get("value", []): - if (application["clientId"] == application_sp["id"]) and ( - application_role_name.lower() in application["scope"].lower() - ): - self.logger.debug( - f"Delegated permission '[green]{application_role_name}[/]' already assigned to '{application_name}'.", - ) - return - # Otherwise grant delegated permissions for this role to the application + # Check existing permissions + response = self.http_get(f"{self.base_endpoint}/oauth2PermissionGrants") self.logger.debug( f"Assigning delegated role '[green]{application_role_name}[/]' to '{application_name}'...", ) - request_json = { - "clientId": application_sp["id"], - "consentType": "AllPrincipals", - "resourceId": microsoft_graph_sp["id"], - "scope": application_role_name, - } - response = self.http_post( - f"{self.base_endpoint}/oauth2PermissionGrants", - json=request_json, + # If there are existing permissions then we need to patch + application = next( + ( + app + for app in response.json().get("value", []) + if app["clientId"] == application_sp["id"] + ), + None, ) + if application: + request_json = { + "scope": f"{application['scope']} {application_role_name}" + } + response = self.http_patch( + f"{self.base_endpoint}/oauth2PermissionGrants/{application['id']}", + json=request_json, + ) + # Otherwise we need to make a new delegation request + else: + request_json = { + "clientId": application_sp["id"], + "consentType": "AllPrincipals", + "resourceId": microsoft_graph_sp["id"], + "scope": application_role_name, + } + response = self.http_post( + f"{self.base_endpoint}/oauth2PermissionGrants", + json=request_json, + ) self.logger.info( f"Assigned delegated role '[green]{application_role_name}[/]' to '{application_name}'.", ) @@ -739,7 +746,7 @@ def http_delete(self, url: str, **kwargs: Any) -> requests.Response: return response raise DataSafeHavenInternalError(response.content) except Exception as exc: - msg = f"Could not execute DELETE request.\n{exc}" + msg = f"Could not execute DELETE request to '{url}'.\n{exc}" raise DataSafeHavenMicrosoftGraphError(msg) from exc def http_get(self, url: str, **kwargs: Any) -> requests.Response: @@ -767,7 +774,7 @@ def http_get(self, url: str, **kwargs: Any) -> requests.Response: return response raise DataSafeHavenInternalError(response.content) except Exception as exc: - msg = f"Could not execute GET request.\n{exc}" + msg = f"Could not execute GET request from '{url}'.\n{exc}" raise DataSafeHavenMicrosoftGraphError(msg) from exc def http_patch(self, url: str, **kwargs: Any) -> requests.Response: @@ -795,7 +802,7 @@ def http_patch(self, url: str, **kwargs: Any) -> requests.Response: return response raise DataSafeHavenInternalError(response.content) except Exception as exc: - msg = f"Could not execute PATCH request.\n{exc}" + msg = f"Could not execute PATCH request to '{url}'.\n{exc}" raise DataSafeHavenMicrosoftGraphError(msg) from exc def http_post(self, url: str, **kwargs: Any) -> requests.Response: @@ -824,7 +831,7 @@ def http_post(self, url: str, **kwargs: Any) -> requests.Response: return response raise DataSafeHavenInternalError(response.content) except Exception as exc: - msg = f"Could not execute POST request.\n{exc}" + msg = f"Could not execute POST request to '{url}'.\n{exc}" raise DataSafeHavenMicrosoftGraphError(msg) from exc def read_applications(self) -> Sequence[dict[str, Any]]: diff --git a/data_safe_haven/infrastructure/components/dynamic/azuread_application.py b/data_safe_haven/infrastructure/components/dynamic/azuread_application.py index a5bc972270..9cc50fcf7d 100644 --- a/data_safe_haven/infrastructure/components/dynamic/azuread_application.py +++ b/data_safe_haven/infrastructure/components/dynamic/azuread_application.py @@ -21,13 +21,15 @@ def __init__( 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, public_client_redirect_uri: Input[str] | None = None, web_redirect_url: Input[str] | None = None, ) -> None: self.application_name = application_name self.application_role_assignments = application_role_assignments self.application_secret_name = application_secret_name - self.auth_token = auth_token + 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 @@ -74,6 +76,7 @@ def create(self, props: dict[str, Any]) -> CreateResult: json_response = graph_api.create_application( props["application_name"], application_scopes=props.get("application_role_assignments", []), + delegated_scopes=props.get("delegated_role_assignments", []), request_json=request_json, ) outs["object_id"] = json_response["id"] @@ -85,6 +88,12 @@ def create(self, props: dict[str, Any]) -> CreateResult: outs["application_name"], role_name ) + # Grant any requested delegated role permissions + for role_name in props.get("delegated_role_assignments", []): + graph_api.grant_delegated_role_permissions( + outs["application_name"], role_name + ) + # Attach an application secret if requested outs["application_secret"] = ( graph_api.create_application_secret( From 8b220e4f034131b9e5f5c29bccbec01099c112df Mon Sep 17 00:00:00 2001 From: James Robinson Date: Mon, 8 Apr 2024 10:26:09 +0100 Subject: [PATCH 07/18] :alien: Use Enum when setting TXT record --- data_safe_haven/external/api/azure_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/data_safe_haven/external/api/azure_api.py b/data_safe_haven/external/api/azure_api.py index 68a47975fc..58180393ac 100644 --- a/data_safe_haven/external/api/azure_api.py +++ b/data_safe_haven/external/api/azure_api.py @@ -227,7 +227,7 @@ def ensure_dns_txt_record( parameters=RecordSet( ttl=30, txt_records=[TxtRecord(value=[record_value])] ), - record_type="TXT", + record_type=RecordType.TXT, relative_record_set_name=record_name, resource_group_name=resource_group_name, zone_name=zone_name, From f84350ae35b44d15154b76c03a3cbad7a6ad6743 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Mon, 8 Apr 2024 10:31:51 +0100 Subject: [PATCH 08/18] :rotating_light: Minor linting fix in graph_api.create_application --- 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 2dc819f7b2..b356150f71 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -231,7 +231,7 @@ def create_application( ) # Ensure that the application service principal exists - application_sp = self.ensure_application_service_principal(application_name) + self.ensure_application_service_principal(application_name) # Grant admin consent for the requested scopes if application_scopes or delegated_scopes: From d2ffb3973fb4e44a9fd2c2f19e40e52db973be59 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Mon, 8 Apr 2024 22:30:20 +0100 Subject: [PATCH 09/18] :loud_sound: Improve errors when creating/updating users in GraphAPI --- data_safe_haven/external/api/graph_api.py | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/data_safe_haven/external/api/graph_api.py b/data_safe_haven/external/api/graph_api.py index b356150f71..81eb0fc2b6 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -476,20 +476,20 @@ def create_user( DataSafeHavenMicrosoftGraphError if the user could not be created """ username = request_json["mailNickname"] + final_verb = "create/update" try: # Check whether user already exists user_id = self.get_id_from_username(username) - final_verb = "" if user_id: self.logger.debug( f"Updating AzureAD user '[green]{username}[/]'...", ) - final_verb = "Updated" + final_verb = "Update" else: self.logger.debug( f"Creating AzureAD user '[green]{username}[/]'...", ) - final_verb = "Created" + final_verb = "Create" # If they do not then create them endpoint = f"{self.base_endpoint}/users" json_response = self.http_post( @@ -504,8 +504,9 @@ def create_user( json={"emailAddress": email_address}, ) except DataSafeHavenMicrosoftGraphError as exc: - if "already exists" not in str(exc): - raise + if "already registered" not in str(exc): + msg = f"Invalid email address '{email_address}'.\n{exc}" + raise DataSafeHavenMicrosoftGraphError(msg) from exc # Set the authentication phone number try: self.http_post( @@ -513,18 +514,19 @@ def create_user( json={"phoneNumber": phone_number, "phoneType": "mobile"}, ) except DataSafeHavenMicrosoftGraphError as exc: - if "already exists" not in str(exc): - raise + if "already registered" not in str(exc): + msg = f"Invalid phone number '{phone_number}'.\n{exc}" + raise DataSafeHavenMicrosoftGraphError(msg) from exc # Ensure user is enabled self.http_patch( f"{self.base_endpoint}/users/{user_id}", json={"accountEnabled": True}, ) self.logger.info( - f"{final_verb} AzureAD user '[green]{username}[/]'.", + f"{final_verb}d AzureAD user '[green]{username}[/]'.", ) except DataSafeHavenMicrosoftGraphError as exc: - msg = f"Could not create/update user {username}.\n{exc}" + msg = f"Could not {final_verb.lower()} user {username}.\n{exc}" raise DataSafeHavenMicrosoftGraphError(msg) from exc def delete_application( From 0c035ee0e30ee327bdbd5f4ec4d035d777a6dd27 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Mon, 8 Apr 2024 22:31:00 +0100 Subject: [PATCH 10/18] :bug: Extract username from userPrincipalName instead of trying to use mailNickname --- 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 81eb0fc2b6..148b5c6db7 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -603,7 +603,7 @@ def get_id_from_username(self, username: str) -> str | None: next( user for user in self.read_users() - if user["mailNickname"] == username + if user["userPrincipalName"].split("@")[0] == username )["id"] ) except (DataSafeHavenMicrosoftGraphError, StopIteration): From ed95da259882f1e4e94ff1dce68a3f5dbaf3345c Mon Sep 17 00:00:00 2001 From: James Robinson Date: Mon, 8 Apr 2024 22:31:19 +0100 Subject: [PATCH 11/18] :sparkles: Add remove_user function to GraphAPI --- data_safe_haven/external/api/graph_api.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/data_safe_haven/external/api/graph_api.py b/data_safe_haven/external/api/graph_api.py index 148b5c6db7..2a108ca4cc 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -983,6 +983,26 @@ def read_users( msg = f"Could not load list of users.\n{exc}" raise DataSafeHavenMicrosoftGraphError(msg) from exc + def remove_user( + self, + username: str, + ) -> None: + """Remove a user from AzureAD + + Raises: + DataSafeHavenMicrosoftGraphError if the user could not be removed + """ + try: + user_id = self.get_id_from_username(username) + # Attempt to remove user from group + self.http_delete( + f"{self.base_endpoint}/users/{user_id}", + ) + return + except Exception as exc: + msg = f"Could not remove user '{username}'.\n{exc}" + raise DataSafeHavenMicrosoftGraphError(msg) from exc + def remove_user_from_group( self, username: str, From bd9ee418d027c2df03bcf95dedf1bf48837e4f21 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Mon, 8 Apr 2024 22:33:04 +0100 Subject: [PATCH 12/18] :coffin: Remove unused AzureAD linux schema --- .../administration/users/azure_ad_users.py | 62 +------------------ data_safe_haven/external/api/graph_api.py | 20 +----- 2 files changed, 3 insertions(+), 79 deletions(-) diff --git a/data_safe_haven/administration/users/azure_ad_users.py b/data_safe_haven/administration/users/azure_ad_users.py index 29a7586e95..477234dccd 100644 --- a/data_safe_haven/administration/users/azure_ad_users.py +++ b/data_safe_haven/administration/users/azure_ad_users.py @@ -46,16 +46,8 @@ def add(self, new_users: Sequence[ResearchUser]) -> None: request_json, user.email_address, user.phone_number ) self.logger.info( - f"Ensured user '{user.preferred_username}' exists in AzureAD" + f"Ensured user '[green]{user.preferred_username}[/]' exists in AzureAD" ) - # Decorate all users with the Linux schema - self.set_user_attributes() - # # Ensure that all users belong to an associated group the same name and UID - # for user in self.list(): - # self.graph_api.create_group(user.username, user.uid_number) - # self.graph_api.add_user_to_group(user.username, user.username) - # # Also add the user to the research users group - # self.graph_api.add_user_to_group(user.username, self.researchers_group_name) def list(self) -> Sequence[ResearchUser]: user_list = self.graph_api.read_users() @@ -113,55 +105,3 @@ def set(self, users: Sequence[ResearchUser]) -> None: users_to_add = [user for user in users if user not in self.list()] self.add(users_to_add) - def set_user_attributes(self) -> None: - """Ensure that all users have Linux attributes""" - # next_uid = max( - # [int(user.uid_number) + 1 if user.uid_number else 0 for user in self.users] - # + [10000] - # ) - # for user in self.users: - # try: - # # Get username from userPrincipalName - # username = user.user_principal_name.split("@")[0] - # if not user.homedir: - # user.homedir = f"/home/{username}" - # self.logger.debug( - # f"Added homedir {user.homedir} to user {user.preferred_username}" - # ) - # if not user.shell: - # user.shell = "/bin/bash" - # self.logger.debug( - # f"Added shell {user.shell} to user {user.preferred_username}" - # ) - # if not user.uid_number: - # # Set UID to the next unused one - # user.uid_number = next_uid - # next_uid += 1 - # self.logger.debug( - # f"Added uid {user.uid_number} to user {user.preferred_username}" - # ) - # if not user.username: - # user.username = username - # self.logger.debug( - # f"Added username {user.username} to user {user.preferred_username}" - # ) - # # Ensure that the remote user matches the local model - # patch_json = { - # GraphApi.linux_schema: { - # "gidnumber": user.uid_number, - # "homedir": user.homedir, - # "shell": user.shell, - # "uid": user.uid_number, - # "user": user.username, - # } - # } - # self.graph_api.http_patch( - # f"{self.graph_api.base_endpoint}/users/{user.azure_oid}", - # json=patch_json, - # ) - # self.logger.debug(f"Set Linux attributes for user {user.preferred_username}.") - # except Exception as exc: - # self.logger.error( - # f"Failed to set Linux attributes for user {user.preferred_username}.\n{str(exc)}" - # ) - pass diff --git a/data_safe_haven/external/api/graph_api.py b/data_safe_haven/external/api/graph_api.py index 2a108ca4cc..ed681320ff 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -43,7 +43,6 @@ def __del__(self) -> None: class GraphApi: """Interface to the Microsoft Graph REST API""" - linux_schema = "extj8xolrvw_linux" # this is the "Extension with Properties for Linux User and Groups" extension application_ids: ClassVar[dict[str, str]] = { "Microsoft Graph": "00000003-0000-0000-c000-000000000000", } @@ -301,7 +300,7 @@ def create_application_secret( msg = f"Could not create application secret '{application_secret_name}'.\n{exc}" raise DataSafeHavenMicrosoftGraphError(msg) from exc - def create_group(self, group_name: str, group_id: str) -> None: + def create_group(self, group_name: str) -> None: """Create an AzureAD group if it does not already exist Raises: @@ -323,21 +322,10 @@ def create_group(self, group_name: str, group_id: str) -> None: "mailNickname": group_name, "securityEnabled": True, } - json_response = self.http_post( + self.http_post( f"{self.base_endpoint}/groups", json=request_json, ).json() - # Add Linux group name and ID - patch_json = { - self.linux_schema: { - "group": group_name, - "gid": group_id, - } - } - self.http_patch( - f"{self.base_endpoint}/groups/{json_response['id']}", - json=patch_json, - ) self.logger.info( f"Created AzureAD group '[green]{group_name}[/]'.", ) @@ -958,7 +946,6 @@ def read_users( "surname", "telephoneNumber", "userPrincipalName", - self.linux_schema, ] ) users: Sequence[dict[str, Any]] @@ -975,9 +962,6 @@ def read_users( user["isGlobalAdmin"] = any( user["id"] == admin["id"] for admin in administrators ) - for key, value in user.get(self.linux_schema, {}).items(): - user[key] = value - user[self.linux_schema] = {} return users except Exception as exc: msg = f"Could not load list of users.\n{exc}" From 67307be2568bd4f853fcc02b52c7b40f470b3be5 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Mon, 8 Apr 2024 22:34:31 +0100 Subject: [PATCH 13/18] :sparkles: Added missing methods to AzureAD users --- .../administration/users/azure_ad_users.py | 45 ++++++++++--------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/data_safe_haven/administration/users/azure_ad_users.py b/data_safe_haven/administration/users/azure_ad_users.py index 477234dccd..e487e3025f 100644 --- a/data_safe_haven/administration/users/azure_ad_users.py +++ b/data_safe_haven/administration/users/azure_ad_users.py @@ -3,6 +3,7 @@ from collections.abc import Sequence from typing import Any +from data_safe_haven.exceptions import DataSafeHavenMicrosoftGraphError from data_safe_haven.external import GraphApi from data_safe_haven.functions import password from data_safe_haven.utility import LoggingSingleton @@ -76,32 +77,34 @@ def list(self) -> Sequence[ResearchUser]: ) ] + def register(self, sre_name: str, usernames: Sequence[str]) -> None: + """Add usernames to SRE security group""" + group_name = f"Data Safe Haven SRE {sre_name} Users" + for username in usernames: + self.graph_api.add_user_to_group(username, group_name) + def remove(self, users: Sequence[ResearchUser]) -> None: - """Disable a list of users in AzureAD""" - # for user_to_remove in users: - # matched_users = [user for user in self.users if user == user_to_remove] - # if not matched_users: - # continue - # user = matched_users[0] - # try: - # if self.graph_api.remove_user_from_group( - # user.username, self.researchers_group_name - # ): - # self.logger.info( - # f"Removed '{user.preferred_username}' from group '{self.researchers_group_name}'" - # ) - # else: - # raise DataSafeHavenMicrosoftGraphError - # except DataSafeHavenMicrosoftGraphError: - # self.logger.error( - # f"Unable to remove '{user.preferred_username}' from group '{self.researchers_group_name}'" - # ) - pass + """Remove list of users from AzureAD""" + for user_to_remove in users: + matched_users = [user for user in self.list() if user == user_to_remove] + if not matched_users: + continue + user = matched_users[0] + try: + self.graph_api.remove_user(user.username) + self.logger.info(f"Removed '{user.preferred_username}'.") + except DataSafeHavenMicrosoftGraphError: + self.logger.error(f"Unable to remove '{user.preferred_username}'.") def set(self, users: Sequence[ResearchUser]) -> None: - """Set Guacamole users to specified list""" + """Set AzureAD users to specified list""" users_to_remove = [user for user in self.list() if user not in users] self.remove(users_to_remove) users_to_add = [user for user in users if user not in self.list()] self.add(users_to_add) + def unregister(self, sre_name: str, usernames: Sequence[str]) -> None: + """Remove usernames from SRE security group""" + group_name = f"Data Safe Haven SRE {sre_name}" + for username in usernames: + self.graph_api.remove_user_from_group(username, group_name) From 05d192a78c3799b063a0a4381049a3ec38931c4f Mon Sep 17 00:00:00 2001 From: James Robinson Date: Tue, 9 Apr 2024 16:15:22 +0100 Subject: [PATCH 14/18] :bug: Ensure that GraphAPI token has correct permissions for granting application permissions --- data_safe_haven/commands/deploy.py | 7 ++- data_safe_haven/external/api/graph_api.py | 43 +++++++++++++++++-- .../components/dynamic/azuread_application.py | 30 ++++++++----- 3 files changed, 64 insertions(+), 16 deletions(-) diff --git a/data_safe_haven/commands/deploy.py b/data_safe_haven/commands/deploy.py index e1ddbdbfb0..355effc4da 100644 --- a/data_safe_haven/commands/deploy.py +++ b/data_safe_haven/commands/deploy.py @@ -115,7 +115,12 @@ def sre( # part of a Pulumi declarative command graph_api = GraphApi( tenant_id=config.shm.aad_tenant_id, - default_scopes=["Application.ReadWrite.All", "Group.ReadWrite.All"], + default_scopes=[ + "Application.ReadWrite.All", + "AppRoleAssignment.ReadWrite.All", + "Directory.ReadWrite.All", + "Group.ReadWrite.All", + ], ) # Initialise Pulumi stack diff --git a/data_safe_haven/external/api/graph_api.py b/data_safe_haven/external/api/graph_api.py index ed681320ff..de4fb1bb99 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -50,6 +50,8 @@ class GraphApi: "Global Administrator": "62e90394-69f5-4237-9190-012177145e10" } uuid_application: ClassVar[dict[str, str]] = { + "Application.ReadWrite.All": "1bfefb4e-e0b5-418b-a88f-73c46d2cc8e9", + "AppRoleAssignment.ReadWrite.All": "06b708a9-e830-4db3-a914-8e69da51d44f", "Directory.Read.All": "7ab1d382-f21e-4acd-a863-ba3e13f7da61", "Domain.Read.All": "dbb9058a-0e50-45d7-ae91-66909b5d4664", "Group.Read.All": "5b567255-7703-4780-807c-7be8301ae99b", @@ -597,6 +599,25 @@ def get_id_from_username(self, username: str) -> str | None: except (DataSafeHavenMicrosoftGraphError, StopIteration): return None + def grant_role_permissions( + self, + application_name: str, + *, + application_role_assignments: Sequence[str], + delegated_role_assignments: Sequence[str], + ) -> None: + """Grant admin permission for requested application roles""" + # Ensure that the application has a service principal + self.ensure_application_service_principal(application_name) + + # Grant any requested application role permissions + for role_name in application_role_assignments: + self.grant_application_role_permissions(application_name, role_name) + + # Grant any requested delegated role permissions + for role_name in delegated_role_assignments: + self.grant_delegated_role_permissions(application_name, role_name) + def grant_application_role_permissions( self, application_name: str, application_role_name: str ) -> None: @@ -1000,10 +1021,24 @@ def remove_user_from_group( try: user_id = self.get_id_from_username(username) group_id = self.get_id_from_groupname(group_name) - # Attempt to remove user from group - self.http_delete( - f"{self.base_endpoint}/groups/{group_id}/members/{user_id}/$ref", - ) + # Check whether user is in group + json_response = self.http_get( + f"{self.base_endpoint}/groups/{group_id}/members", + ).json() + # Remove user from group if it is a member + if user_id in [ + group_member["id"] for group_member in json_response["value"] + ]: + self.http_delete( + f"{self.base_endpoint}/groups/{group_id}/members/{user_id}/$ref", + ) + self.logger.info( + f"Removed [green]'{username}'[/] from group [green]'{group_name}'[/]." + ) + else: + self.logger.info( + f"User [green]'{username}'[/] does not belong to group [green]'{group_name}'[/]." + ) except Exception as exc: msg = ( f"Could not remove user '{username}' from group '{group_name}'.\n{exc}" diff --git a/data_safe_haven/infrastructure/components/dynamic/azuread_application.py b/data_safe_haven/infrastructure/components/dynamic/azuread_application.py index 9cc50fcf7d..e24c3f25d7 100644 --- a/data_safe_haven/infrastructure/components/dynamic/azuread_application.py +++ b/data_safe_haven/infrastructure/components/dynamic/azuread_application.py @@ -48,6 +48,17 @@ def refresh(props: dict[str, Any]) -> dict[str, Any]: ): outs["object_id"] = json_response["id"] outs["application_id"] = json_response["appId"] + + # Ensure that requested role permissions have been granted + graph_api.grant_role_permissions( + outs["application_name"], + application_role_assignments=props.get( + "application_role_assignments", [] + ), + delegated_role_assignments=props.get( + "delegated_role_assignments", [] + ), + ) return outs except Exception as exc: msg = f"Failed to refresh application [green]{props['application_name']}[/] in AzureAD.\n{exc}" @@ -82,17 +93,14 @@ def create(self, props: dict[str, Any]) -> CreateResult: outs["object_id"] = json_response["id"] outs["application_id"] = json_response["appId"] - # Grant any requested application role permissions - for role_name in props.get("application_role_assignments", []): - graph_api.grant_application_role_permissions( - outs["application_name"], role_name - ) - - # Grant any requested delegated role permissions - for role_name in props.get("delegated_role_assignments", []): - graph_api.grant_delegated_role_permissions( - outs["application_name"], role_name - ) + # Grant requested role permissions + graph_api.grant_role_permissions( + outs["application_name"], + application_role_assignments=props.get( + "application_role_assignments", [] + ), + delegated_role_assignments=props.get("delegated_role_assignments", []), + ) # Attach an application secret if requested outs["application_secret"] = ( From 40f7bf32e2716a7a4f5807088839b16bd14f6830 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Thu, 11 Apr 2024 14:16:27 +0100 Subject: [PATCH 15/18] :recycle: Simplify user removal logic following suggestion from @JimMadge --- data_safe_haven/administration/users/azure_ad_users.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/data_safe_haven/administration/users/azure_ad_users.py b/data_safe_haven/administration/users/azure_ad_users.py index e487e3025f..06f648987c 100644 --- a/data_safe_haven/administration/users/azure_ad_users.py +++ b/data_safe_haven/administration/users/azure_ad_users.py @@ -85,11 +85,10 @@ def register(self, sre_name: str, usernames: Sequence[str]) -> None: def remove(self, users: Sequence[ResearchUser]) -> None: """Remove list of users from AzureAD""" - for user_to_remove in users: - matched_users = [user for user in self.list() if user == user_to_remove] - if not matched_users: - continue - user = matched_users[0] + for user in filter( + lambda existing_user: any(existing_user == user for user in users), + self.list(), + ): try: self.graph_api.remove_user(user.username) self.logger.info(f"Removed '{user.preferred_username}'.") From f6271513a0e1a315f562f11af51edea40f1ac232 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Thu, 11 Apr 2024 14:18:44 +0100 Subject: [PATCH 16/18] :bug: Remove infinite loop following suggestion from @JimMadge Co-authored-by: Jim Madge --- data_safe_haven/external/api/graph_api.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/data_safe_haven/external/api/graph_api.py b/data_safe_haven/external/api/graph_api.py index de4fb1bb99..ea6db2dac3 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -240,13 +240,20 @@ def create_application( self.grant_application_role_permissions(application_name, scope) for scope in delegated_scopes: self.grant_delegated_role_permissions(application_name, scope) - while True: + attempts = 0 + max_attempts = 5 + while attempts < max_attempts: if application_sp := self.get_service_principal_by_name( application_name ): if self.read_application_permissions(application_sp["id"]): break time.sleep(10) + attempts += 1 + + if attempts == max_attempts: + msg = "Maximum attempts to validate service principle permissions exceeded" + raise DataSafeHavenMicrosoftGraphError(msg) # Return JSON representation of the AzureAD application return json_response From c6fa9392abff9b6acc8a7cf2537626b1946fb15a Mon Sep 17 00:00:00 2001 From: James Robinson Date: Thu, 11 Apr 2024 14:39:16 +0100 Subject: [PATCH 17/18] :loud_sound: Improve docstrings for application role granting --- data_safe_haven/external/api/graph_api.py | 32 ++++++++++++++++++----- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/data_safe_haven/external/api/graph_api.py b/data_safe_haven/external/api/graph_api.py index ea6db2dac3..fa1d64f3ce 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -613,7 +613,19 @@ def grant_role_permissions( application_role_assignments: Sequence[str], delegated_role_assignments: Sequence[str], ) -> None: - """Grant admin permission for requested application roles""" + """ + Grant roles to the service principal associated with an application and give admin approval to these roles + + These can be either application or delegated roles. + + - Application roles allow the application to perform an action itself. + - Delegated roles allow the application to ask a user for permission to perform an action. + + See https://learn.microsoft.com/en-us/graph/permissions-grant-via-msgraph for more details. + + Raises: + DataSafeHavenMicrosoftGraphError if one or more roles could not be assigned. + """ # Ensure that the application has a service principal self.ensure_application_service_principal(application_name) @@ -629,11 +641,14 @@ def grant_application_role_permissions( self, application_name: str, application_role_name: str ) -> None: """ - Grant permissions for a particular role to an application. - See https://learn.microsoft.com/en-us/graph/permissions-grant-via-msgraph + Assign a named application role to the service principal associated with an application. + Additionally provide Global Admin approval for the application to hold this role. + Application roles allow the application to perform an action itself. + + See https://learn.microsoft.com/en-us/graph/permissions-grant-via-msgraph for more details. Raises: - DataSafeHavenMicrosoftGraphError if the secret could not be created or already exists + DataSafeHavenMicrosoftGraphError if one or more roles could not be assigned. """ try: # Get service principals for Microsoft Graph and this application @@ -682,11 +697,14 @@ def grant_delegated_role_permissions( self, application_name: str, application_role_name: str ) -> None: """ - Grant permissions for a particular role to an application. - See https://learn.microsoft.com/en-us/graph/permissions-grant-via-msgraph + Assign a named delegated role to the service principal associated with an application. + Additionally provide Global Admin approval for the application to hold this role. + Delegated roles allow the application to ask a user for permission to perform an action. + + See https://learn.microsoft.com/en-us/graph/permissions-grant-via-msgraph for more details. Raises: - DataSafeHavenMicrosoftGraphError if the secret could not be created or already exists + DataSafeHavenMicrosoftGraphError if one or more roles could not be assigned. """ try: # Get service principals for Microsoft Graph and this application From 10af228252b54efd67f45071932106f401b73273 Mon Sep 17 00:00:00 2001 From: James Robinson Date: Thu, 11 Apr 2024 14:40:24 +0100 Subject: [PATCH 18/18] :recycle: Improve code style/efficiency following suggestion from @JimMadge Co-authored-by: Jim Madge --- data_safe_haven/external/api/graph_api.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/data_safe_haven/external/api/graph_api.py b/data_safe_haven/external/api/graph_api.py index fa1d64f3ce..adf3705c3b 100644 --- a/data_safe_haven/external/api/graph_api.py +++ b/data_safe_haven/external/api/graph_api.py @@ -658,7 +658,7 @@ def grant_application_role_permissions( raise DataSafeHavenMicrosoftGraphError(msg) application_sp = self.get_service_principal_by_name(application_name) if not application_sp: - msg = "Could not find application service principal." + msg = f"Could not find application service principal for application {application_name}." raise DataSafeHavenMicrosoftGraphError(msg) # Check whether permission is already granted app_role_id = self.uuid_application[application_role_name] @@ -1051,9 +1051,9 @@ def remove_user_from_group( f"{self.base_endpoint}/groups/{group_id}/members", ).json() # Remove user from group if it is a member - if user_id in [ + if user_id in ( group_member["id"] for group_member in json_response["value"] - ]: + ): self.http_delete( f"{self.base_endpoint}/groups/{group_id}/members/{user_id}/$ref", )