User username as VM username rather than random ID

[marrobi]

Fixes #1148 #3693

This pull request includes several changes to the api_app to enhance user details handling and update various components to utilize these details. The most important changes include adding the username field to the User model, updating the Resource model to include user details, and modifying the authentication service to handle the new user details.

[github-actions]

Unit Test Results

596 tests   596 ✅  6s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit bbc14af.

♻️ This comment has been updated with latest results.

[marrobi]

@tamirkamara @damoodamoo welcome your thoughts on this from a design perspective, got a few asks for this. Think should have a "user object":

"user": {
 "name": "blah",
 "email": "blah",
 "username": "blah" 
}

In the payload rather than how I've done it in this PR? The RP would need to understand where to look.

[tanya-borisova]

Personally, I think it's fine without a user object, as this avoids some code in the RP that would have to unpack it

[damoodamoo]

I would say it would be nicer to pass a user object rather than as individual fields, since it will only grow over time. But - not a big deal and agree with @tanya-borisova that it would mean more code in the RP.

[marrobi]

/test-extended

[github-actions]

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/11401103181 (with refid 323e78ec)

(in response to this comment from @marrobi)

[Copilot]

Copilot reviewed 11 out of 25 changed files in this pull request and generated 1 comment.

Files not reviewed (14)

• templates/workspace_services/guacamole/user_resources/guacamole-azure-linuxvm/terraform/linuxvm.tf: Language not supported
• templates/workspace_services/guacamole/user_resources/guacamole-azure-linuxvm/terraform/locals.tf: Language not supported
• templates/workspace_services/guacamole/user_resources/guacamole-azure-linuxvm/terraform/outputs.tf: Language not supported
• templates/workspace_services/guacamole/user_resources/guacamole-azure-linuxvm/terraform/variables.tf: Language not supported
• api_app/tests_ma/test_service_bus/test_resource_request_sender.py: Evaluated as low risk
• api_app/tests_ma/test_services/test_aad_access_service.py: Evaluated as low risk
• resource_processor/_version.py: Evaluated as low risk
• api_app/tests_ma/test_api/test_routes/test_workspaces.py: Evaluated as low risk
• api_app/tests_ma/test_api/conftest.py: Evaluated as low risk
• api_app/tests_ma/conftest.py: Evaluated as low risk
• api_app/tests_ma/test_models/test_resource.py: Evaluated as low risk
• resource_processor/tests_rp/test_logging.py: Evaluated as low risk
• api_app/models/domain/authentication.py: Evaluated as low risk
• CHANGELOG.md: Evaluated as low risk

Comments suppressed due to low confidence (1)

api_app/services/aad_authentication.py:276

• The new 'username' field in the '_get_users_inc_groups_from_response' method is not covered by tests.

+                user_username = user_data["body"]["userPrincipalName"]

[jonnyry]

@marrobi

Am I right in thinking it uses the whole userPrincipalName as a Windows/Linux username?

e.g.

[email protected] for internal users
username_externaldomain.com#EXT#@contoso.onmicrosoft.com for users from external tenants

or is there some parsing I've missed.

The screenshot you posted is showing only the prefix #1148 (comment)

[jonnyry]

Seperate point - this is something that was raised in our UAT by a researcher. Keen to see this merged.

[marrobi]

@marrobi

Am I right in thinking it uses the whole userPrincipalName as a Windows/Linux username?

e.g.

[email protected] for internal users username_externaldomain.com#EXT#@contoso.onmicrosoft.com for users from external tenants

or is there some parsing I've missed.

The screenshot you posted is showing only the prefix #1148 (comment)

In locals.tf:

admin_username = element(split("@", var.user_username), 0)

Just the prefix, as @ is not allowed in a username. After the @ is the machine name/domain name.

[jonnyry]

@marrobi
Am I right in thinking it uses the whole userPrincipalName as a Windows/Linux username?
e.g.
[email protected] for internal users username_externaldomain.com#EXT#@contoso.onmicrosoft.com for users from external tenants
or is there some parsing I've missed.
The screenshot you posted is showing only the prefix #1148 (comment)

In locals.tf:

admin_username = element(split("@", var.user_username), 0)

Just the prefix, as @ is not allowed in a username. After the @ is the machine name/domain name.

Ah missed that. Thanks.

[jonnyry]

Does it need ignore_changes on the admin_user on the VM resources?

Changing admin_user for existing machines is a destructive action.

From the TF docs:

admin_username - (Required) The username of the local administrator used for the Virtual Machine. Changing this forces a new resource to be created.

[marrobi]

Does it need ignore_changes on the admin_user on the VM resources?

Changing admin_user for existing machines is a destructive action.

From the TF docs:

admin_username - (Required) The username of the local administrator used for the Virtual Machine. Changing this forces a new resource to be created.

Good idea. Feel free to pull the PR and iterate/push changes. I was going to look into this - #3770 (comment) at some point too. User should become required, but not sure if that is backwards compatible.

[marrobi]

/test-extended

[github-actions]

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/13167786178 (with refid 323e78ec)

(in response to this comment from @marrobi)

[marrobi]

Tests pass, but the API is broken. Will take another look.

This is as existing resource are missing username field.

I might revisit this and use the ownerId on the user resource and terraform AD provider to get the user name.

[marrobi]

/test-destroy-env

[github-actions]

Destroying PR test environment (RG: rg-tre323e78ec)... (run: https://github.com/microsoft/AzureTRE/actions/runs/13168231234)

[github-actions]

PR test environment destroy complete (RG: rg-tre323e78ec)