User username as VM username rather than random ID #3770
Conversation
marrobi
changed the title
Unit Test Results596 tests 596 ✅ 6s ⏱️ Results for commit 1be0132. ♻️ This comment has been updated with latest results. |
|
@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. |
|
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. |
|
/test-extended |
|
🤖 pr-bot 🤖 🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/11401103181 (with refid (in response to this comment from @marrobi) |
Copilot
AI
left a comment
Copilot
AI
left a comment
There was a problem hiding this comment.
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"]
| updatedWhen: float = 0 | ||
|
|
||
| def get_resource_request_message_payload(self, operation_id: str, step_id: str, action: RequestAction) -> dict: | ||
| payload = { | ||
| "operationId": operation_id, | ||
| "stepId": step_id, | ||
| "action": action, | ||
| "user": { |
There was a problem hiding this comment.
The 'user' field is Optional[User], but there is no null check before accessing its attributes. This could lead to an AttributeError if 'user' is None. Add a null check before accessing 'user' attributes.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
Fixes #1148 #3693
This pull request includes several changes to the
api_appto enhance user details handling and update various components to utilize these details. The most important changes include adding theusernamefield to theUsermodel, updating theResourcemodel to include user details, and modifying the authentication service to handle the new user details.