Feature/workspace user management #4337

fortunkam · 2025-02-07T15:18:18Z

Resolves #4169 "Enable Workspace Owners to add multiple users and assign WorkspaceResearcher role"

This pull request introduces a new user management feature to the api_app project, along with several related changes. The key changes include the addition of new routes for managing workspace users, updates to the configuration to enable user management, and the creation of new schemas and models to support user roles and assignments.

The User management feature will only be visible if the user is a TreAdmin, the USER_MANAGEMENT_ENABLED flag is enabled and the Workspace has Entra ID/AAD groups enabled.

User Management Feature:

api_app/api/routes/api.py: Added workspace_users to the list of routes and included conditional routing based on the USER_MANAGEMENT_ENABLED configuration. [1] [2]
api_app/api/routes/workspace_users.py: Introduced new routes for getting workspace users, assignable users, workspace roles, assigning users to roles, and removing user assignments.

Configuration Updates:

api_app/core/config.py: Added a new configuration variable USER_MANAGEMENT_ENABLED to toggle user management features.

Schema and Model Additions:

api_app/models/domain/workspace_users.py: Added new models for AssignableUser, Role, AssignedUser, and AssignmentType.
api_app/models/schemas/roles.py: Created a new schema RolesInResponse to represent the roles in a workspace.
api_app/models/schemas/users.py: Updated UsersInResponse to use AssignedUser and added new schemas AssignableUsersInResponse and WorkspaceUserOperationResponse. [1] [2]
api_app/models/schemas/workspace_users.py: Added a new schema UserRoleAssignmentRequest for assigning roles to users.

Other Changes:

api_app/resources/strings.py: Added new string constants for user management API endpoints and messages. [1] [2]
api_app/services/aad_authentication.py: Updated methods to support fetching user details, roles, and assignments from Azure AD, including handling new attributes like userPrincipalName. [1] [2] [3] [4] [5]

Version Update:

api_app/_version.py: Updated the version from 0.21.1 to 0.22.0.

github-actions · 2025-02-07T15:19:09Z

Unit Test Results

617 tests 617 ✅ 7s ⏱️
1 suites 0 💤
1 files 0 ❌

Results for commit 2ce6937.

♻️ This comment has been updated with latest results.

api_app/api/dependencies/database.py

marrobi

Great to see this, couple of comments, will likely give it a go and review further next week. Thanks!

api_app/api/routes/workspaces.py

marrobi · 2025-02-07T15:52:21Z

api_app/services/aad_authentication.py

+        sp_graph_data = self._get_app_sp_graph_data(workspace.properties["client_id"])
+        app_id_to_role_name = {app_role["id"]: app_role["value"] for app_role in sp_graph_data["value"][0]["appRoles"]}
+
+        for group in (item for item in roles_graph_data["value"] if item["principalType"] == PrincipalType.Group.value):


Is this not just going to add the user to a random group that has been assigned to the app role?

the terraform uses:

resource "azuread_group" "workspace_owners" { count = var.create_aad_groups ? 1 : 0 display_name = "${var.workspace_resource_name_suffix} Workspace Owners" owners = [var.workspace_owner_object_id] security_enabled = true }

We should maybe output and store the group IDs as workspace properties? If that property exists we add to the group, if it doesn't we add direct to the app role?

Interesting, I think when we created this originally we made an assumption that there would only be one group assigned to the app role (for each role type) since that is what the "Add groups" option did. We can make that more explicit.

Yes, we need to, who knows what group might get added to the app role, and then unknowingly other people get added to it and end up with access to something they shouldn't.

Do we need to store this in the Cosmos config somewhere or can we just infer the names from convention?

I think we need to add the group IDs in outputs.tf for the workspace, and porter.yaml, then the IDs will be stored in cosmos. I think check for existence of that value - will need to be conditional on groups being enabled - and if is there use the ID of the group.

That make sense?

Looks like there are already some properties being exported I might be able to use create_aad_groups and can then lookup based on the name and the app_role_id_* properties.

Have added an implementation of this, let me know what you think.

marrobi · 2025-02-13T22:28:06Z

api_app/services/aad_authentication.py

+        group_name = group_details[0]
+        workspace_app_role_field = group_details[1]
+
+        for group in [item for item in roles_graph_data["value"] if item["principalType"] == PrincipalType.Group.value]:


Bit confused, might need to talk through it. How does roles_graph_data["value"] contain groups? looks to be getting existing user role assignments?

roles_graph_data contains a list of appRoles assigned to the API, it is a mixture of users and groups

This is the call. https://learn.microsoft.com/en-us/graph/api/serviceprincipal-list-approleassignedto?view=graph-rest-1.0&tabs=http

Ah, got it, then look through those groups by name?

If it works, ok, think using the workspace outputs would be more straightforward.

marrobi · 2025-02-14T10:41:03Z

ui/app/src/components/workspaces/WorkspaceUsersAssignNew.tsx

+      <Stack tokens={{ childrenGap: 20 }} styles={{ root: { paddingTop: 20 } }}>
+        <Stack tokens={{ childrenGap: 10 }} verticalAlign="center">
+          <Label>User</Label>
+          <TextField


Is it possible to have a picker here of some sort, I tried with an email in the tenant and got a 500 back. Didn't dig any deeper.

Originally I had a picker here but it didn't look great with large numbers in the tenant. Ideally what it needs is some sort of search functionality. For external users to a tenant it needs to be added as the user principal name rather than the email address so it is a bit clunky. (e.g. myemail_microsoft.com#EXT#@MyTenant.onmicrosoft.com).

I can readd the name picker as a short term measure and investigate the search functionality as a longer term solution (possibly a new issue)?

Not sure what the picker looks like... got a snip?

Yes, agree, search would work better.

This was the initial attempt, for each user we show the Display Name and the User Principal Name (email address for non-guest users)

It works fine while the GAL is small.

Nice, does it filter as you type or not?

I think add it, and we can maybe modify as we go on. Is it an existing control/component - https://developer.microsoft.com/en-us/fluentui#/controls/web/peoplepicker ?

No it is a combobox with an API, will add it now and take a look at that link to see if I can get it in easily?

This is now replaced with the people picker

jonnyry · 2025-02-14T10:46:42Z

NOTE: To use this feature the API Service principal needs to have 2 more application MS Graph permissions adding. Groups.ReadWrite.All and AppRoleAssignments.ReadWriteAll

Is there any way to use a narrower permission than AppRoleAssignments.ReadWriteAll? IIUC this is a broad permission with the ability to grant any other permission (e.g. global admin).

marrobi · 2025-02-14T11:43:19Z

NOTE: To use this feature the API Service principal needs to have 2 more application MS Graph permissions adding. Groups.ReadWrite.All and AppRoleAssignments.ReadWriteAll

Is there any way to use a narrower permission than AppRoleAssignments.ReadWriteAll? IIUC this is a broad permission with the ability to grant any other permission (e.g. global admin).

The Resource Processor should already have access to these if it created them, maybe it should give the API access to the specific groups and app roles?

fortunkam · 2025-02-14T12:20:39Z

NOTE: To use this feature the API Service principal needs to have 2 more application MS Graph permissions adding. Groups.ReadWrite.All and AppRoleAssignments.ReadWriteAll

Is there any way to use a narrower permission than AppRoleAssignments.ReadWriteAll? IIUC this is a broad permission with the ability to grant any other permission (e.g. global admin).

The Resource Processor should already have access to these if it created them, maybe it should give the API access to the specific groups and app roles?

In theory it should be owner on the groups so should be able to access the data but this was the only way we could get it to work. I think the ms graph layer was getting in the way but it needs more research.

Sync with Main

Added support for multi assignment

Copilot

Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.

api_app/tests_ma/test_api/test_routes/test_workspace_users.py

api_app/tests_ma/test_services/test_aad_access_service.py

…fortunkam/4337-1

Co-authored-by: Copilot <[email protected]>

…m/fortunkam/azuretre into pr/fortunkam/4337-1

…p ids

…rise app only

fortunkam · 2025-02-21T12:53:00Z

Following discussion the User Management feature will now only be available on workspaces with AAD groups enabled.
The additional Graph API permissions are now not required.

…ture/workspace_user_management

Matthew Fortunka and others added 4 commits February 6, 2025 15:01

Added User Management (WIP)

880f7c2

Added config flag to disable user management

b2dde32

Merge branch 'microsoft:main' into feature/workspace_user_management

1addc5e

fixed UI and added unit tests

685c92e

github-actions bot added the external PR from an external contributor label Feb 7, 2025

fortunkam commented Feb 7, 2025

View reviewed changes

api_app/api/dependencies/database.py Outdated Show resolved Hide resolved

added additional permissions to auth setup script

a2484b8

marrobi reviewed Feb 7, 2025

View reviewed changes

api_app/api/routes/workspaces.py Outdated Show resolved Hide resolved

api_app/api/routes/workspaces.py Outdated Show resolved Hide resolved

upped docker version

fef0af9

marrobi reviewed Feb 7, 2025

View reviewed changes

Matthew Fortunka added 3 commits February 13, 2025 10:50

fixed unit tests

75ffaa3

Added more tests

4bb3b36

more tests

b145db9

marrobi reviewed Feb 13, 2025

View reviewed changes

marrobi reviewed Feb 14, 2025

View reviewed changes

fortunkam and others added 10 commits February 14, 2025 12:47

Merge branch 'feature/workspace_user_management' into main

96f849b

Merge pull request #1 from fortunkam/main

b69beb8

Sync with Main

added combo box to select users from

57d5c70

increase UI version number

c1f6b84

fix linting issues

79248b6

more linting issues addressed

c8908e0

more linting fixes

9d195a2

final linting pass

93356b7

added option to deploy

f398b0e

upped version of core for linting

a5eedd4

fortunkam marked this pull request as ready for review February 17, 2025 10:06

Matthew Fortunka added 2 commits February 19, 2025 09:56

Removed extra method

081055c

Moved to use ID instead of email

fee832a

fortunkam marked this pull request as draft February 19, 2025 14:03

Matthew Fortunka added 4 commits February 19, 2025 14:30

refactored to removed unused classes

d18a392

Added support for multi assignment

fixed unit tests

a8ece9b

removed reference to email per PR comment

408144f

added timeouts to graph and REST calls

091ab2a

marrobi requested a review from Copilot February 19, 2025 15:31

Copilot AI reviewed Feb 19, 2025

View reviewed changes

api_app/tests_ma/test_api/test_routes/test_workspace_users.py Outdated Show resolved Hide resolved

api_app/tests_ma/test_services/test_aad_access_service.py Outdated Show resolved Hide resolved

marrobi and others added 12 commits February 19, 2025 15:34

Merge branch 'main' of https://github.com/microsoft/AzureTRE into pr/…

140614d

…fortunkam/4337-1

Update api_app/tests_ma/test_api/test_routes/test_workspace_users.py

38980cf

Co-authored-by: Copilot <[email protected]>

Update api_app/tests_ma/test_services/test_aad_access_service.py

cb30ba7

Co-authored-by: Copilot <[email protected]>

Merge branch 'feature/workspace_user_management' of https://github.co…

836e119

…m/fortunkam/azuretre into pr/fortunkam/4337-1

add core_api_client_id to resource processor env var

acf011c

bump RP version to 0.12.2 and add core_api_client_id to config

3c17ac4

Add core_api_client_id variable, update aad provider, adn output grou…

bf160b7

…p ids

WIP: trying to run appRoleAssignment with Owner permissions on enterp…

ce2669d

…rise app only

Fixed return values

f033b0f

Moved to only allow user management if AAD groups are enabled

149af84

fixed typo

4e166f3

updated unit tests

3b4e239

Matthew Fortunka added 6 commits February 21, 2025 12:54

Merge branch 'main' of https://github.com/microsoft/AzureTRE into fea…

4fecce3

…ture/workspace_user_management

upped version number of base workspace to avoid conflicts

8696094

fix linting errors

9d1169f

fixed missed linting issues

d7f423f

added azuread provider

511df5c

fixed provider issue

b92d765

fortunkam marked this pull request as ready for review February 21, 2025 15:01

increased version of resource processor

2ce6937

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/workspace user management #4337

Feature/workspace user management #4337

fortunkam commented Feb 7, 2025 •

edited

Loading

github-actions bot commented Feb 7, 2025 •

edited

Loading

marrobi left a comment

marrobi Feb 7, 2025

marrobi Feb 7, 2025

fortunkam Feb 7, 2025

marrobi Feb 10, 2025

fortunkam Feb 13, 2025

marrobi Feb 13, 2025

fortunkam Feb 13, 2025

fortunkam Feb 13, 2025

marrobi Feb 13, 2025

fortunkam Feb 14, 2025

fortunkam Feb 14, 2025

marrobi Feb 14, 2025

marrobi Feb 14, 2025

fortunkam Feb 14, 2025

marrobi Feb 14, 2025

fortunkam Feb 14, 2025

fortunkam Feb 14, 2025

marrobi Feb 14, 2025

fortunkam Feb 14, 2025

fortunkam Feb 19, 2025

jonnyry commented Feb 14, 2025

marrobi commented Feb 14, 2025

fortunkam commented Feb 14, 2025

Copilot AI left a comment

fortunkam commented Feb 21, 2025

Feature/workspace user management #4337

Are you sure you want to change the base?

Feature/workspace user management #4337

Conversation

fortunkam commented Feb 7, 2025 • edited Loading

Resolves #4169 "Enable Workspace Owners to add multiple users and assign WorkspaceResearcher role"

github-actions bot commented Feb 7, 2025 • edited Loading

Unit Test Results

marrobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonnyry commented Feb 14, 2025

marrobi commented Feb 14, 2025

fortunkam commented Feb 14, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

fortunkam commented Feb 21, 2025

fortunkam commented Feb 7, 2025 •

edited

Loading

github-actions bot commented Feb 7, 2025 •

edited

Loading