more linting issues addressed

Author: Matthew Fortunka

api_app/api/dependencies/database.py

@@ -69,7 +69,7 @@ async def _get_store_key(cls, credential) -> str:
             ) as cosmosdb_mng_client:
                 database_keys = await cosmosdb_mng_client.database_accounts.list_keys(
                     resource_group_name=RESOURCE_GROUP_NAME,
-                    account_name=COSMOSDB_ACCOUNT_NAME,
+                    account_name=COSMOSDB_ACCOUNT_NAME
                 )
                 primary_master_key = database_keys.primary_master_key
 

api_app/api/routes/workspace_users.py

@@ -1,10 +1,10 @@
-from fastapi import APIRouter, Depends,Response,status
+from fastapi import APIRouter, Depends, Response, status
 from api.dependencies.workspaces import get_workspace_by_id_from_path
 from resources import strings
 from services.authentication import get_access_service
 from models.schemas.users import UsersInResponse, AssignableUsersInResponse
 from models.schemas.roles import RolesInResponse
-from services.authentication import get_current_admin_user,get_current_workspace_owner_or_researcher_user_or_airlock_manager_or_tre_admin
+from services.authentication import get_current_admin_user, get_current_workspace_owner_or_researcher_user_or_airlock_manager_or_tre_admin
 
 workspaces_users_admin_router = APIRouter(dependencies=[Depends(get_current_admin_user)])
 workspaces_users_shared_router = APIRouter(dependencies=[Depends(get_current_workspace_owner_or_researcher_user_or_airlock_manager_or_tre_admin)])
@@ -48,9 +48,7 @@ async def assign_workspace_user(response: Response, user_email: str, role_name:
     return UsersInResponse(users=users)
 
 
-@workspaces_users_admin_router.delete("/workspaces/{workspace_id}/users/assign",
-                               status_code=status.HTTP_202_ACCEPTED,
-                               name=strings.API_REMOVE_WORKSPACE_USER_ASSIGNMENT)
+@workspaces_users_admin_router.delete("/workspaces/{workspace_id}/users/assign", status_code=status.HTTP_202_ACCEPTED, name=strings.API_REMOVE_WORKSPACE_USER_ASSIGNMENT)
 async def remove_workspace_user_assignment(user_email: str,
                                            role_name: str,
                                            workspace=Depends(get_workspace_by_id_from_path)) -> UsersInResponse:

api_app/models/domain/authentication.py

@@ -14,10 +14,12 @@ class User(BaseModel):
     roles: List[str] = Field([])
     roleAssignments: List[RoleAssignment] = Field([])
 
+
 class AssignableUser(BaseModel):
     name: str
     email: str
 
+
 class Role(BaseModel):
     id: str
     value: str

api_app/models/schemas/roles.py

@@ -1,7 +1,7 @@
 from pydantic import BaseModel, Field
 from typing import List
-
 from models.domain.authentication import Role
 
+
 class RolesInResponse(BaseModel):
     roles: List[Role] = Field(..., title="Roles", description="List of roles in a workspace")

api_app/models/schemas/users.py

@@ -27,5 +27,6 @@ class Config:
             }
         }
 
+
 class AssignableUsersInResponse(BaseModel):
     assignable_users: List[AssignableUser] = Field(..., title="Assignable Users", description="List of users assignable to a workspace")

api_app/services/aad_authentication.py

@@ -1,5 +1,4 @@
 import base64
-import json
 from collections import defaultdict
 from enum import Enum
 from typing import List, Optional
@@ -12,7 +11,7 @@
 
 from services.access_service import AccessService, AuthConfigValidationError, UserRoleAssignmentError
 from core import config
-from db.errors import DuplicateEntity, EntityDoesNotExist
+from db.errors import EntityDoesNotExist
 from models.domain.authentication import User, AssignableUser, Role, RoleAssignment
 from models.domain.workspace import Workspace, WorkspaceRole
 from resources import strings
@@ -283,13 +282,13 @@ def _get_users_inc_groups_from_response(self, users_graph_data, roles_graph_data
                     user_email = user_data["body"]["userPrincipalName"]
                     # if user with id does not already exist in users
                     user_roles=self._get_roles_for_principal(user_id, roles_graph_data, app_id_to_role_name)
+
                     if not any(user.id == user_id for user in users):
                         users.append(User(id=user_id, name=user_name, email=user_email, roles=user_roles))
                     else:
                         user = next((user for user in users if user.id == user_id), None)
                         user.roles = list(set(user.roles + user_roles))
 
-
             # Handle group endpoint response
             elif "directoryObjects" in user_data["body"]["@odata.context"]:
                 group_id = user_data["id"]
@@ -299,6 +298,7 @@ def _get_users_inc_groups_from_response(self, users_graph_data, roles_graph_data
                     user_email = group_member["userPrincipalName"]
 
                     group_roles=self._get_roles_for_principal(group_id, roles_graph_data, app_id_to_role_name)
+
                     if not any(user.id == user_id for user in users):
                         users.append(User(id=user_id, name=user_name, email=user_email, roles=group_roles))
                     else:
@@ -339,11 +339,11 @@ def get_workspace_roles(self, workspace: Workspace) -> List[Role]:
 
         for role in graph_data["value"]:
             roles.append(Role(id=role["id"], value=role["value"],
-                            isEnabled=role["isEnabled"],
-                            description=role["description"],
-                            displayName=role["displayName"],
-                            origin=role["origin"],
-                            allowedMemberTypes=role["allowedMemberTypes"]))
+                                isEnabled=role["isEnabled"],
+                                description=role["description"],
+                                displayName=role["displayName"],
+                                origin=role["origin"],
+                                allowedMemberTypes=role["allowedMemberTypes"]))
 
         return roles
 
@@ -359,7 +359,7 @@ def get_workspace_user_emails_by_role_assignment(self, workspace: Workspace):
         return workspace_role_assignments_details
 
     def get_workspace_role_by_name(self, name: str, workspace: Workspace) -> Role:
-        app_roles_endpoint =  f"{MICROSOFT_GRAPH_URL}/v1.0/servicePrincipals/{workspace.properties['sp_id']}/appRoles"
+        app_roles_endpoint = f"{MICROSOFT_GRAPH_URL}/v1.0/servicePrincipals/{workspace.properties['sp_id']}/appRoles"
         graph_data = self._ms_graph_query(app_roles_endpoint, "GET")
 
         for role in graph_data["value"]:
@@ -399,7 +399,7 @@ def _is_workspace_role_group_in_use(self, workspace: Workspace) -> bool:
     def _get_workspace_group_name(self, workspace: Workspace, role: Role) -> tuple:
         tre_id = workspace.properties["tre_id"]
         workspace_id = workspace.properties["workspace_id"]
-        group_suffix = ""
+        group_name = ""
         app_role_id_suffix = ""
         if role.value == "WorkspaceResearcher":
             group_name = "Workspace Researchers"
@@ -487,7 +487,7 @@ def remove_workspace_role_user_assignment(self, user: User,
         if self._is_workspace_role_group_in_use(workspace):
             self._remove_workspace_user_from_application_group(user, workspace, role)
         else:
-            self._remove_workspace_user_from_application(user,role_assignment)
+            self._remove_workspace_user_from_application(user, role_assignment)
 
     def _remove_workspace_user_from_application(self, user: User, role_assignment: dict) -> requests.Response:
         msgraph_token = self._get_msgraph_token()

api_app/services/access_service.py

@@ -9,9 +9,11 @@
 class AuthConfigValidationError(Exception):
     """Raised when the input auth information is invalid"""
 
+
 class UserRoleAssignmentError(Exception):
     """Raised when a user role assignment fails"""
 
+
 class AccessService(OAuth2AuthorizationCodeBearer):
     @abstractmethod
     def extract_workspace_auth_information(self, data: dict) -> dict:

api_app/tests_ma/test_api/conftest.py

@@ -1,7 +1,6 @@
 import pytest
 import pytest_asyncio
 from mock import patch
-from unittest.mock import patch
 
 from fastapi import FastAPI
 from httpx import AsyncClient

api_app/tests_ma/test_api/test_routes/test_workspace_users.py

@@ -1,4 +1,3 @@
-from unittest.mock import AsyncMock
 import pytest
 from mock import patch
 
@@ -24,6 +23,7 @@
 CLIENT_ID = 'f0acf127-a672-a672-a672-a15e5bf9f127'
 OPERATION_ID = '11111111-7265-4b5f-9eae-a1a62928772f'
 
+
 def sample_workspace(workspace_id=WORKSPACE_ID, auth_info: dict = {}) -> Workspace:
     workspace = Workspace(
         id=workspace_id,
@@ -83,14 +83,13 @@ async def test_get_workspace_users_returns_users(self, _, auth_class, app, clien
             assert response.status_code == status.HTTP_200_OK
             assert response.json()["users"] == users
 
-
     @pytest.mark.parametrize("auth_class", ["aad_authentication.AzureADAuthorization"])
     @patch("api.dependencies.workspaces.WorkspaceRepository.get_workspace_by_id", return_value=sample_workspace())
     async def test_assign_workspace_user_assigns_workspace_user(self, get_workspace_by_id_mock, auth_class, app, client):
         with patch(f"services.{auth_class}.get_user_by_email") as get_user_by_email_mock, \
-            patch(f"services.{auth_class}.get_workspace_role_by_name") as get_workspace_role_by_name_mock, \
-            patch(f"services.{auth_class}.assign_workspace_user") as assign_workspace_user_mock, \
-            patch(f"services.{auth_class}.get_workspace_users") as get_workspace_users_mock:
+                patch(f"services.{auth_class}.get_workspace_role_by_name") as get_workspace_role_by_name_mock, \
+                patch(f"services.{auth_class}.assign_workspace_user") as assign_workspace_user_mock, \
+                patch(f"services.{auth_class}.get_workspace_users") as get_workspace_users_mock:
 
             workspace = get_workspace_by_id_mock.return_value
 
@@ -182,7 +181,6 @@ async def test_get_assignable_users_returns_assignable_users(self, get_workspace
             assert response.status_code == status.HTTP_200_OK
             assert response.json()["assignable_users"] == assignable_users
 
-
     @pytest.mark.parametrize("auth_class", ["aad_authentication.AzureADAuthorization"])
     @patch("api.dependencies.workspaces.WorkspaceRepository.get_workspace_by_id", return_value=sample_workspace())
     async def test_get_workspace_roles_returns_workspace_roles(self, get_workspace_by_id_mock, auth_class, app, client):

api_app/tests_ma/test_api/test_routes/test_workspaces.py

@@ -15,7 +15,7 @@
 from db.errors import EntityDoesNotExist
 from db.repositories.workspaces import WorkspaceRepository
 from db.repositories.workspace_services import WorkspaceServiceRepository
-from models.domain.authentication import RoleAssignment, User, Role
+from models.domain.authentication import RoleAssignment
 from models.domain.operation import Operation, OperationStep, Status
 from models.domain.resource import ResourceHistoryItem, ResourceType
 from models.domain.user_resource import UserResource

api_app/tests_ma/test_services/test_aad_access_service.py

@@ -85,6 +85,7 @@ def get_app_sp_graph_data_mock():
         ]
     }
 
+
 @pytest.fixture
 def workspace_with_groups():
     return Workspace(
@@ -105,6 +106,7 @@ def workspace_with_groups():
         }
     )
 
+
 @pytest.fixture
 def workspace_without_groups():
     return Workspace(
@@ -125,14 +127,17 @@ def workspace_without_groups():
         }
     )
 
+
 @pytest.fixture
 def role_owner():
     return Role(id="owner-role-id", value="WorkspaceOwner", isEnabled=True, description="Owner", displayName="Owner", origin="", allowedMemberTypes=[])
 
+
 @pytest.fixture
 def user_without_role():
     return User(id="user1", name="Test User", email="[email protected]", roles=[])
 
+
 @pytest.fixture
 def user_with_role():
     return User(id="user2", name="Test User 2", email="[email protected]", roles=["WorkspaceOwner"])
@@ -654,21 +659,6 @@ def test_get_workspace_role_by_name(mock_ms_graph_query):
 
 @patch("services.aad_authentication.AzureADAuthorization.get_user_by_email")
 def test_get_user_by_email(mock_get_user_by_email):
-    workspace = Workspace(
-        id="abc",
-        etag="",
-        templateName="template-name",
-        templateVersion="0.1.0",
-        resourcePath="test",
-        properties={
-            "client_id": "1234",
-            "sp_id": "abc127",
-            "app_role_id_workspace_owner": "abc128",
-            "app_role_id_workspace_researcher": "abc129",
-            "app_role_id_workspace_airlock_manager": "abc130",
-        },
-    )
-
     mock_get_user_by_email.return_value = User(id="1", name="John Doe", email="[email protected]", roles=["WorkspaceOwner"])
 
     access_service = AzureADAuthorization()
@@ -754,6 +744,7 @@ def get_mock_role_response(principal_roles):
         )
     return response
 
+
 @patch("services.aad_authentication.AzureADAuthorization._is_user_in_role", return_value=True)
 @patch("services.aad_authentication.AzureADAuthorization._is_workspace_role_group_in_use")
 @patch("services.aad_authentication.AzureADAuthorization._assign_workspace_user_to_application_group")
@@ -769,12 +760,13 @@ def test_assign_workspace_user_already_has_role(workspace_role_in_use_mock,
     assert assign_user_to_group_mock.call_count == 0
     assert assign_user_to_role_mock.call_count == 0
 
+
 @patch("services.aad_authentication.AzureADAuthorization._is_user_in_role", return_value=False)
 @patch("services.aad_authentication.AzureADAuthorization._is_workspace_role_group_in_use", return_value=False)
 @patch("services.aad_authentication.AzureADAuthorization._assign_workspace_user_to_application_group")
 @patch("services.aad_authentication.AzureADAuthorization._assign_workspace_user_to_application")
 def test_assign_workspace_user_if_no_groups(assign_user_to_role_mock,assign_user_to_group_mock,
-                                             workspace_without_groups, role_owner,
+                                                workspace_without_groups, role_owner,
                                                 user_with_role):
 
     access_service = AzureADAuthorization()
@@ -785,12 +777,13 @@ def test_assign_workspace_user_if_no_groups(assign_user_to_role_mock,assign_user
     assert assign_user_to_group_mock.call_count == 0
     assert assign_user_to_role_mock.call_count == 1
 
+
 @patch("services.aad_authentication.AzureADAuthorization._is_user_in_role", return_value=False)
 @patch("services.aad_authentication.AzureADAuthorization._is_workspace_role_group_in_use", return_value=True)
 @patch("services.aad_authentication.AzureADAuthorization._assign_workspace_user_to_application_group")
 @patch("services.aad_authentication.AzureADAuthorization._assign_workspace_user_to_application")
 def test_assign_workspace_user_if_groups(assign_user_to_role_mock,assign_user_to_group_mock,
-                                             workspace_without_groups, role_owner,
+                                                workspace_without_groups, role_owner,
                                                 user_with_role):
 
     access_service = AzureADAuthorization()
@@ -801,12 +794,13 @@ def test_assign_workspace_user_if_groups(assign_user_to_role_mock,assign_user_to
     assert assign_user_to_group_mock.call_count == 1
     assert assign_user_to_role_mock.call_count == 0
 
+
 @patch("services.aad_authentication.AzureADAuthorization._is_workspace_role_group_in_use", return_value=False)
 @patch("services.aad_authentication.AzureADAuthorization._remove_workspace_user_from_application_group")
 @patch("services.aad_authentication.AzureADAuthorization._remove_workspace_user_from_application")
 @patch("services.aad_authentication.AzureADAuthorization._get_role_assignment_for_user")
 def test_remove_workspace_user_if_no_groups(get_role_assignment_mock,
-                                            remove_user_to_role_mock,remove_user_to_group_mock,
+                                            remove_user_to_role_mock, remove_user_to_group_mock,
                                              workspace_without_groups, role_owner,
                                                 user_with_role):
 
@@ -819,12 +813,13 @@ def test_remove_workspace_user_if_no_groups(get_role_assignment_mock,
     assert remove_user_to_group_mock.call_count == 0
     assert remove_user_to_role_mock.call_count == 1
 
+
 @patch("services.aad_authentication.AzureADAuthorization._is_workspace_role_group_in_use", return_value=True)
 @patch("services.aad_authentication.AzureADAuthorization._remove_workspace_user_from_application_group")
 @patch("services.aad_authentication.AzureADAuthorization._remove_workspace_user_from_application")
 @patch("services.aad_authentication.AzureADAuthorization._get_role_assignment_for_user")
 def test_remove_workspace_user_if_groups(get_role_assignment_mock,
-                                            remove_user_to_role_mock,remove_user_to_group_mock,
+                                            remove_user_to_role_mock, remove_user_to_group_mock,
                                              workspace_without_groups, role_owner,
                                                 user_with_role):
 
@@ -865,6 +860,7 @@ def test_get_assignable_users_returns_users(_, request_get_mock, mock_headers):
     assert users[0].name == "User 1"
     assert users[0].email == "[email protected]"
 
+
 @patch("services.aad_authentication.AzureADAuthorization._get_msgraph_token", return_value="token")
 @patch("services.aad_authentication.AzureADAuthorization._ms_graph_query")
 @patch("services.aad_authentication.AzureADAuthorization._get_auth_header")
@@ -882,8 +878,8 @@ def test_get_workspace_roles_returns_roles(_, ms_graph_query_mock, mock_headers,
             Role(id=1, value="AirlockManager", isEnabled=True, description="", displayName="Airlock Manager", origin="", allowedMemberTypes=[]).dict(),
             Role(id=2, value="WorkspaceResearcher", isEnabled=True, description="", displayName="Workspace Researcher", origin="", allowedMemberTypes=[]).dict(),
             Role(id=3, value="WorkspaceOwner", isEnabled=True, description="", displayName="Workspace Owner", origin="", allowedMemberTypes=[]).dict(),
-        ]
-    }
+            ]
+        }
     ms_graph_query_mock.return_value = request_get_mock_response
     roles = access_service.get_workspace_roles(workspace_without_groups)