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

User username as VM username rather than random ID #3770

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
FEATURES:

ENHANCEMENTS:
- Make user details avialble to resource processor, and update Windows and Linux VMs to use them. ([#4905](https://github.com/microsoft/AzureTRE/pull/3770))
* Disable storage account cross tenant replication ([#4116](https://github.com/microsoft/AzureTRE/pull/4116))
* Key Vaults should use RBAC instead of access policies for access control ([#4000](https://github.com/microsoft/AzureTRE/issues/4000))
* Split log entries with [Log chunk X of Y] for better readability. ([#3992](https://github.com/microsoft/AzureTRE/issues/3992))
Expand Down
2 changes: 1 addition & 1 deletion api_app/_version.py
Original file line number Diff line number Diff line change
@@ -1 +1 @@
__version__ = "0.19.4"
__version__ = "0.19.5"
1 change: 1 addition & 0 deletions api_app/models/domain/authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
class User(BaseModel):
id: str
name: str
username: str
email: str = Field(None)
roles: List[str] = Field([])
roleAssignments: List[RoleAssignment] = Field([])
9 changes: 8 additions & 1 deletion api_app/models/domain/resource.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from enum import Enum
from typing import Optional, Union, List
from pydantic import BaseModel, Field, validator
from models.domain.authentication import User
from models.domain.azuretremodel import AzureTREModel
from models.domain.request_action import RequestAction
from resources import strings
Expand Down Expand Up @@ -50,14 +51,20 @@ class Resource(AzureTREModel):
etag: str = Field(title="_etag", description="eTag of the document", alias="_etag")
resourcePath: str = ""
resourceVersion: int = 0
user: dict = {}
user: Optional[User]
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": {
Copy link
Preview

Copilot AI Dec 13, 2024

Choose a reason for hiding this comment

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

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.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
"id": self.user.id,
"name": self.user.name,
"email": self.user.email,
"username": self.user.username,
},
"id": self.id,
"name": self.templateName,
"version": self.templateVersion,
Expand Down
22 changes: 13 additions & 9 deletions api_app/services/aad_authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ def _get_user_from_token(decoded_token: dict) -> User:
return User(id=user_id,
name=decoded_token.get('name', ''),
email=decoded_token.get('email', ''),
username=decoded_token.get('preferred_username', ''),
roles=decoded_token.get('roles', []))

def _decode_token(self, token: str, ws_app_reg_id: str) -> dict:
Expand Down Expand Up @@ -227,11 +228,11 @@ def _get_batch_endpoint() -> str:

@staticmethod
def _get_users_endpoint(user_object_id) -> str:
return "/users/" + user_object_id + "?$select=displayName,mail,id"
return "/users/" + user_object_id + "?$select=displayName,mail,id,userPrincipalName"

@staticmethod
def _get_group_members_endpoint(group_object_id) -> str:
return "/groups/" + group_object_id + "/transitiveMembers?$select=displayName,mail,id"
return "/groups/" + group_object_id + "/transitiveMembers?$select=displayName,mail,id,userPrincipalName"

def _get_app_sp_graph_data(self, client_id: str) -> dict:
msgraph_token = self._get_msgraph_token()
Expand Down Expand Up @@ -276,23 +277,26 @@ def _get_users_inc_groups_from_response(self, users_graph_data, roles_graph_data
# Handle user endpoint response
user_id = user_data["body"]["id"]
user_name = user_data["body"]["displayName"]
user_username = user_data["body"]["userPrincipalName"]

if "users" in user_data["body"]["@odata.context"]:
user_email = user_data["body"]["mail"]
# if user with id does not already exist in users
if not any(user.id == user_id for user in users):
users.append(User(id=user_id, name=user_name, email=user_email, roles=self._get_roles_for_principal(user_id, roles_graph_data, app_id_to_role_name)))
users.append(User(id=user_id, name=user_name, email=user_email, username=user_username, roles=self._get_roles_for_principal(user_id, roles_graph_data, app_id_to_role_name)))

# Handle group endpoint response
elif "directoryObjects" in user_data["body"]["@odata.context"]:
group_id = user_data["id"]
for group_member in user_data["body"]["value"]:
user_id = group_member["id"]
user_name = group_member["displayName"]
user_email = group_member["mail"]

if not any(user.id == user_id for user in users):
users.append(User(id=user_id, name=user_name, email=user_email, roles=self._get_roles_for_principal(group_id, roles_graph_data, app_id_to_role_name)))
if group_member["@odata.type"] == "#microsoft.graph.user":
user_id = group_member["id"]
user_name = group_member["displayName"]
user_email = group_member["mail"]
user_username = group_member["userPrincipalName"]

if not any(user.id == user_id for user in users):
users.append(User(id=user_id, name=user_name, email=user_email, username=user_username, roles=self._get_roles_for_principal(group_id, roles_graph_data, app_id_to_role_name)))

return users

Expand Down
2 changes: 1 addition & 1 deletion api_app/tests_ma/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ def multi_step_resource_template(basic_shared_service_template) -> ResourceTempl

@pytest.fixture
def test_user():
return User(id="user-id", name="test user", email="[email protected]")
return User(id="user-id", name="test user", email="[email protected]", username="testuser")


@pytest.fixture
Expand Down
1 change: 1 addition & 0 deletions api_app/tests_ma/test_api/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ def create_test_user() -> User:
return User(
id="user-guid-here",
name="Test User",
username="testuser",
email="[email protected]",
roles=[],
roleAssignments=[]
Expand Down
6 changes: 4 additions & 2 deletions api_app/tests_ma/test_api/test_routes/test_workspaces.py
Original file line number Diff line number Diff line change
Expand Up @@ -1661,14 +1661,16 @@ async def test_get_workspace_users_returns_users(self, _, auth_class, app, clien
"name": "John Doe",
"email": "[email protected]",
"roles": ["WorkspaceOwner", "WorkspaceResearcher"],
'roleAssignments': []
"roleAssignments": [],
"username": "johndoe"
},
{
"id": "456",
"name": "Jane Smith",
"email": "[email protected]",
"roles": ["WorkspaceResearcher"],
'roleAssignments': []
"roleAssignments": [],
"username": "janesmith"
}
]
get_workspace_users_mock.return_value = users
Expand Down
8 changes: 4 additions & 4 deletions api_app/tests_ma/test_models/test_resource.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ def test_resource_is_enabled_returns_correct_value(resource, expected):
assert resource.isEnabled == expected


def test_user_resource_get_resource_request_message_payload_augments_payload_with_extra_params():
def test_user_resource_get_resource_request_message_payload_augments_payload_with_extra_params(test_user):
owner_id = "abc"
workspace_id = "123"
parent_service_id = "abcdef"

user_resource = UserResource(id="123", templateName="user-template", templateVersion="1.0", etag="", ownerId=owner_id, workspaceId=workspace_id, parentWorkspaceServiceId=parent_service_id, resourcePath="test")
user_resource = UserResource(id="123", templateName="user-template", templateVersion="1.0", etag="", ownerId=owner_id, workspaceId=workspace_id, parentWorkspaceServiceId=parent_service_id, resourcePath="test", user=test_user)

message_payload = user_resource.get_resource_request_message_payload(OPERATION_ID, STEP_ID, RequestAction.Install)

Expand All @@ -36,9 +36,9 @@ def test_user_resource_get_resource_request_message_payload_augments_payload_wit
assert message_payload["parentWorkspaceServiceId"] == parent_service_id


def test_workspace_service_get_resource_request_message_payload_augments_payload_with_extra_params():
def test_workspace_service_get_resource_request_message_payload_augments_payload_with_extra_params(test_user):
workspace_id = "123"
workspace_service = WorkspaceService(id="123", templateName="service-template", templateVersion="1.0", etag="", workspaceId=workspace_id, resourcePath="test")
workspace_service = WorkspaceService(id="123", templateName="service-template", templateVersion="1.0", etag="", workspaceId=workspace_id, resourcePath="test", user=test_user)

message_payload = workspace_service.get_resource_request_message_payload(OPERATION_ID, STEP_ID, RequestAction.Install)

Expand Down
22 changes: 12 additions & 10 deletions api_app/tests_ma/test_service_bus/test_resource_request_sender.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
try_update_with_retries,
update_resource_for_step,
)
from tests_ma.test_api.conftest import create_test_user
from tests_ma.test_service_bus.test_deployment_status_update import (
create_sample_operation,
)
Expand All @@ -25,7 +24,8 @@
pytestmark = pytest.mark.asyncio


def create_test_resource():
@pytest.fixture
def test_resource(test_user):
return Resource(
id=str(uuid.uuid4()),
resourceType=ResourceType.Workspace,
Expand All @@ -34,6 +34,7 @@ def create_test_resource():
etag="",
properties={"testParameter": "testValue"},
resourcePath="test",
user=test_user
)


Expand All @@ -52,22 +53,23 @@ async def test_resource_request_message_generated_correctly(
operations_repo_mock,
resource_history_repo_mock,
request_action,
multi_step_resource_template
multi_step_resource_template,
test_resource,
test_user
):
service_bus_client_mock().get_queue_sender().send_messages = AsyncMock()
resource = create_test_resource()
operation = create_sample_operation(resource.id, request_action)
operation = create_sample_operation(test_resource.id, request_action)
operations_repo_mock.create_operation_item.return_value = operation
resource_repo.get_resource_by_id.return_value = resource
resource_repo.get_resource_by_id.return_value = test_resource
resource_template_repo.get_template_by_name_and_version.return_value = multi_step_resource_template

resource_repo.patch_resource.return_value = (resource, multi_step_resource_template)
resource_repo.patch_resource.return_value = (test_resource, multi_step_resource_template)

await send_resource_request_message(
resource=resource,
resource=test_resource,
operations_repo=operations_repo_mock,
resource_repo=resource_repo,
user=create_test_user(),
user=test_user,
resource_template_repo=resource_template_repo,
resource_history_repo=resource_history_repo_mock,
action=request_action
Expand All @@ -80,7 +82,7 @@ async def test_resource_request_message_generated_correctly(
sent_message = args[0]
assert sent_message.correlation_id == operation.id
sent_message_as_json = json.loads(str(sent_message))
assert sent_message_as_json["id"] == resource.id
assert sent_message_as_json["id"] == test_resource.id
assert sent_message_as_json["action"] == request_action


Expand Down
Loading
Loading