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

Closed
wants to merge 20 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ FEATURES:
* Add support for customer-managed keys encryption. Core support ([#4141](https://github.com/microsoft/AzureTRE/issues/4142), [#4144](https://github.com/microsoft/AzureTRE/issues/4144)), Base workspace ([#4161](https://github.com/microsoft/AzureTRE/pull/4161)), other templates ([#4145](https://github.com/microsoft/AzureTRE/issues/4145))

ENHANCEMENTS:
- Make user details available 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.20.4"
__version__ = "0.21.0"
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 StrEnum
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": {
"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