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

adding potential fix #19

Merged
merged 1 commit into from
Feb 12, 2024
Merged
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
9 changes: 6 additions & 3 deletions api_app/api/routes/workspaces.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,11 @@ def validate_user_has_valid_role_for_user_resource(user, user_resource):

if "ImperialWorkspaceOwner" in user.roles:
return

if ("WorkspaceResearcher" in user.roles or "AirlockManager" in user.roles or "ImperialWorkspaceResearcher" in user.roles or "ImperialWorkspaceOwner" in user.roles) and user_resource.ownerId == user.id:

if "ImperialWorkspaceDataEngineer" in user.roles:
return

if ("WorkspaceResearcher" in user.roles or "AirlockManager" in user.roles or "ImperialWorkspaceResearcher" in user.roles or "ImperialWorkspaceOwner" in user.roles or "ImperialWorkspaceDataEngineer" in user.roles) and user_resource.ownerId == user.id:
return

raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail=strings.ACCESS_USER_IS_NOT_OWNER_OR_RESEARCHER)
Expand Down Expand Up @@ -373,7 +376,7 @@ async def retrieve_user_resources_for_workspace_service(
user_resources = await user_resource_repo.get_user_resources_for_workspace_service(workspace_id, service_id)

# filter only to the user - for researchers
if ("WorkspaceResearcher" in user.roles or "AirlockManager" in user.roles) and ("WorkspaceOwner" not in user.roles or "ImperialWorkspaceResearcher" not in user.roles or "ImperialWorkspaceOwner" not in user.roles):
if ("WorkspaceResearcher" in user.roles or "AirlockManager" in user.roles) and ("WorkspaceOwner" not in user.roles or "ImperialWorkspaceResearcher" not in user.roles or "ImperialWorkspaceOwner" not in user.roles or "ImperialWorkspaceDataEngineer" not in user.roles):
user_resources = [resource for resource in user_resources if resource.ownerId == user.id]

#for user_resource in user_resources:
Expand Down
1 change: 1 addition & 0 deletions api_app/db/migrations/workspaces.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ async def moveAuthInformationToProperties(self) -> bool:
item["properties"]["app_role_id_workspace_researcher"] = item["authInformation"]["roles"]["WorkspaceResearcher"]
item["properties"]["app_role_id_imperial_workspace_researcher"] = item["authInformation"]["roles"]["ImperialWorkspaceResearcher"]
item["properties"]["app_role_id_imperial_workspace_owner"] = item["authInformation"]["roles"]["ImperialWorkspaceOwner"]
item["properties"]["app_role_id_imperial_workspace_dataengineer"] = item["authInformation"]["roles"]["ImperialWorkspaceDataEngineer"]
item["properties"]["app_role_id_workspace_owner"] = item["authInformation"]["roles"]["WorkspaceOwner"]
# cleanup
del item["authInformation"]
Expand Down
2 changes: 1 addition & 1 deletion api_app/models/domain/workspace.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class WorkspaceRole(Enum):
AirlockManager = 3
ImperialResearcher = 4
ImperialOwner = 5

ImperialDataEngineer = 6


class Workspace(Resource):
Expand Down
4 changes: 3 additions & 1 deletion api_app/services/aad_authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class AzureADAuthorization(AccessService):
aad_instance = config.AAD_AUTHORITY_URL

TRE_CORE_ROLES = ['TREAdmin', 'TREUser']
WORKSPACE_ROLES_DICT = {'WorkspaceOwner': 'app_role_id_workspace_owner', 'WorkspaceResearcher': 'app_role_id_workspace_researcher', 'AirlockManager': 'app_role_id_workspace_airlock_manager', 'ImperialWorkspaceResearcher': 'app_role_id_imperial_workspace_researcher' , 'ImperialWorkspaceOwner': 'app_role_id_imperial_workspace_owner'}
WORKSPACE_ROLES_DICT = {'WorkspaceOwner': 'app_role_id_workspace_owner', 'WorkspaceResearcher': 'app_role_id_workspace_researcher', 'AirlockManager': 'app_role_id_workspace_airlock_manager', 'ImperialWorkspaceResearcher': 'app_role_id_imperial_workspace_researcher' , 'ImperialWorkspaceOwner': 'app_role_id_imperial_workspace_owner', 'ImperialWorkspaceDataEngineer': 'app_role_id_imperial_workspace_dataengineer'}

def __init__(self, auto_error: bool = True, require_one_of_roles: Optional[list] = None):
super(AzureADAuthorization, self).__init__(
Expand Down Expand Up @@ -445,6 +445,8 @@ def get_workspace_role(self, user: User, workspace: Workspace, user_role_assignm
return WorkspaceRole.ImperialResearcher
if RoleAssignment(resource_id=workspace_sp_id, role_id=workspace.properties['app_role_id_imperial_workspace_owner']) in user_role_assignments:
return WorkspaceRole.ImperialOwner
if RoleAssignment(resource_id=workspace_sp_id, role_id=workspace.properties['app_role_id_imperial_workspace_dataengineer']) in user_role_assignments:
return WorkspaceRole.ImperialDataEngineer
return WorkspaceRole.NoRole


Expand Down
10 changes: 6 additions & 4 deletions api_app/services/airlock.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ def validate_user_allowed_to_access_storage_account(user: User, airlock_request:
if (airlock_request.status == AirlockRequestStatus.InReview):
allowed_roles = ["AirlockManager", "WorkspaceOwner"]
else:
allowed_roles = ["WorkspaceResearcher", "WorkspaceOwner"]
allowed_roles = ["WorkspaceResearcher", "WorkspaceOwner", "ImperialWorkspaceDataEngineer"]

if not _user_has_one_of_roles(user=user, roles=allowed_roles):
raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail=strings.AIRLOCK_UNAUTHORIZED_TO_SA)
Expand Down Expand Up @@ -351,7 +351,9 @@ def check_email_exists(role_assignment_details: defaultdict(list)):
if "AirlockManager" not in role_assignment_details or not role_assignment_details["AirlockManager"]:
logger.error('Creating an airlock request but the airlock manager does not have an email address.')
raise HTTPException(status_code=status.HTTP_417_EXPECTATION_FAILED, detail=strings.AIRLOCK_NO_AIRLOCK_MANAGER_EMAIL)

if "ImperialWorkspaceDataEngineer" not in role_assignment_details or not role_assignment_details["ImperialWorkspaceDataEngineer"]:
logger.error('Creating an airlock request but the DataEngineer does not have an email address.')
raise HTTPException(status_code=status.HTTP_417_EXPECTATION_FAILED, detail=strings.AIRLOCK_NO_RESEARCHER_EMAIL)

async def get_airlock_requests_by_user_and_workspace(user: User, workspace: Workspace, airlock_request_repo: AirlockRequestRepository,
creator_user_id: Optional[str] = None, type: Optional[AirlockRequestType] = None, status: Optional[AirlockRequestStatus] = None,
Expand All @@ -370,10 +372,10 @@ def get_allowed_actions(request: AirlockRequest, user: User, airlock_request_rep
if can_review_request and "AirlockManager" in user.roles:
allowed_actions.append(AirlockActions.Review)

if can_cancel_request and ("WorkspaceOwner" in user.roles or "WorkspaceResearcher" in user.roles):
if can_cancel_request and ("WorkspaceOwner" in user.roles or "WorkspaceResearcher" in user.roles or "ImperialWorkspaceDataEngineer" in user.roles):
allowed_actions.append(AirlockActions.Cancel)

if can_submit_request and ("WorkspaceOwner" in user.roles or "WorkspaceResearcher" in user.roles):
if can_submit_request and ("WorkspaceOwner" in user.roles or "WorkspaceResearcher" in user.roles or "ImperialWorkspaceDataEngineer" in user.roles):
allowed_actions.append(AirlockActions.Submit)

return allowed_actions
Expand Down
14 changes: 7 additions & 7 deletions api_app/services/authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,25 +33,25 @@ def get_access_service(provider: str = AuthProvider.AAD) -> AccessService:
get_current_workspace_owner_user = AzureADAuthorization(require_one_of_roles=['WorkspaceOwner'])


get_current_workspace_researcher_user = AzureADAuthorization(require_one_of_roles=['WorkspaceResearcher', 'ImperialWorkspaceResearcher', 'ImperialWorkspaceOwner'])
get_current_workspace_researcher_user = AzureADAuthorization(require_one_of_roles=['WorkspaceResearcher', 'ImperialWorkspaceResearcher', 'ImperialWorkspaceOwner', 'ImperialWorkspaceDataEngineer'])


get_current_airlock_manager_user = AzureADAuthorization(require_one_of_roles=['AirlockManager'])


get_current_workspace_owner_or_researcher_user = AzureADAuthorization(require_one_of_roles=['WorkspaceOwner', 'WorkspaceResearcher', 'ImperialWorkspaceResearcher', 'ImperialWorkspaceOwner'])
get_current_workspace_owner_or_researcher_user = AzureADAuthorization(require_one_of_roles=['WorkspaceOwner', 'WorkspaceResearcher', 'ImperialWorkspaceResearcher', 'ImperialWorkspaceOwner', 'ImperialWorkspaceDataEngineer'])


get_current_workspace_owner_or_airlock_manager = AzureADAuthorization(require_one_of_roles=['WorkspaceOwner', 'AirlockManager'])
get_current_workspace_owner_or_airlock_manager = AzureADAuthorization(require_one_of_roles=['WorkspaceOwner', 'AirlockManager', 'ImperialWorkspaceDataEngineer'])


get_current_workspace_owner_or_researcher_user_or_airlock_manager = AzureADAuthorization(require_one_of_roles=['WorkspaceOwner', 'WorkspaceResearcher', 'AirlockManager', 'ImperialWorkspaceResearcher', 'ImperialWorkspaceOwner'])
get_current_workspace_owner_or_researcher_user_or_airlock_manager = AzureADAuthorization(require_one_of_roles=['WorkspaceOwner', 'WorkspaceResearcher', 'AirlockManager', 'ImperialWorkspaceResearcher', 'ImperialWorkspaceOwner', 'ImperialWorkspaceDataEngineer'])


get_current_workspace_owner_or_researcher_user_or_tre_admin = AzureADAuthorization(require_one_of_roles=["TREAdmin", "WorkspaceOwner", "WorkspaceResearcher", "ImperialWorkspaceResearcher", "ImperialWorkspaceOwner"])
get_current_workspace_owner_or_researcher_user_or_tre_admin = AzureADAuthorization(require_one_of_roles=["TREAdmin", "WorkspaceOwner", "WorkspaceResearcher", "ImperialWorkspaceResearcher", "ImperialWorkspaceOwner", "ImperialWorkspaceDataEngineer"])


get_current_workspace_owner_or_researcher_user_or_airlock_manager_or_tre_admin = AzureADAuthorization(require_one_of_roles=["TREAdmin", "WorkspaceOwner", "WorkspaceResearcher", "AirlockManager", "ImperialWorkspaceResearcher", "ImperialWorkspaceOwner"])
get_current_workspace_owner_or_researcher_user_or_airlock_manager_or_tre_admin = AzureADAuthorization(require_one_of_roles=["TREAdmin", "WorkspaceOwner", "WorkspaceResearcher", "AirlockManager", "ImperialWorkspaceResearcher", "ImperialWorkspaceOwner", "ImperialWorkspaceDataEngineer"])


get_current_workspace_owner_or_tre_admin = AzureADAuthorization(require_one_of_roles=["TREAdmin", "WorkspaceOwner", "ImperialWorkspaceOwner"])
get_current_workspace_owner_or_tre_admin = AzureADAuthorization(require_one_of_roles=["TREAdmin", "WorkspaceOwner", "ImperialWorkspaceOwner", "ImperialWorkspaceDataEngineer"])
17 changes: 17 additions & 0 deletions devops/scripts/aad/create_workspace_application.sh
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ source "${DIR}/update_resource_access.sh"
researcherRoleId=$(cat /proc/sys/kernel/random/uuid)
imperialResearcherRoleId=$(cat /proc/sys/kernel/random/uuid)
imperialOwnerRoleId=$(cat /proc/sys/kernel/random/uuid)
imperialDataEngineerRoleId=$(cat /proc/sys/kernel/random/uuid)
ownerRoleId=$(cat /proc/sys/kernel/random/uuid)
airlockManagerRoleId=$(cat /proc/sys/kernel/random/uuid)
userImpersonationScopeId=$(cat /proc/sys/kernel/random/uuid)
Expand All @@ -136,12 +137,14 @@ if [ -n "${existingApp}" ]; then
researcherRoleId=$(echo "$existingApp" | jq -r '.appRoles[] | select(.value == "WorkspaceResearcher").id')
imperialResearcherRoleId=$(echo "$existingApp" | jq -r '.appRoles[] | select(.value == "ImperialWorkspaceResearcher").id')
imperialOwnerRoleId=$(echo "$existingApp" | jq -r '.appRoles[] | select(.value == "ImperialWorkspaceOwner").id')
imperialDataEngineerRoleId=$(echo "$existingApp" | jq -r '.appRoles[] | select(.value == "ImperialWorkspaceDataEngineer").id')
ownerRoleId=$(echo "$existingApp" | jq -r '.appRoles[] | select(.value == "WorkspaceOwner").id')
airlockManagerRoleId=$(echo "$existingApp" | jq -r '.appRoles[] | select(.value == "AirlockManager").id')
userImpersonationScopeId=$(echo "$existingApp" | jq -r '.api.oauth2PermissionScopes[] | select(.value == "user_impersonation").id')
if [[ -z "${researcherRoleId}" ]]; then researcherRoleId=$(cat /proc/sys/kernel/random/uuid); fi
if [[ -z "${imperialResearcherRoleId}" ]]; then imperialResearcherRoleId=$(cat /proc/sys/kernel/random/uuid); fi
if [[ -z "${imperialOwnerRoleId}" ]]; then imperialOwnerRoleId=$(cat /proc/sys/kernel/random/uuid); fi
if [[ -z "${imperialDataEngineerRoleId}" ]]; then imperialDataEngineerRoleId=$(cat /proc/sys/kernel/random/uuid); fi
if [[ -z "${ownerRoleId}" ]]; then ownerRoleId=$(cat /proc/sys/kernel/random/uuid); fi
if [[ -z "${airlockManagerRoleId}" ]]; then airlockManagerRoleId=$(cat /proc/sys/kernel/random/uuid); fi
if [[ -z "${userImpersonationScopeId}" ]]; then userImpersonationScopeId=$(cat /proc/sys/kernel/random/uuid); fi
Expand Down Expand Up @@ -209,6 +212,15 @@ appDefinition=$(jq -c . << JSON
"origin": "Application",
"value": "ImperialWorkspaceOwner"
},
{
"id": "${imperialDataEngineerRoleId}",
"allowedMemberTypes": [ "User", "Application" ],
"description": "Provides Imperial Data Engineer access to the Workspace.",
"displayName": "Imperial Workspace Data Engineer",
"isEnabled": true,
"origin": "Application",
"value": "ImperialWorkspaceDataEngineer"
},
{
"id": "${airlockManagerRoleId}",
"allowedMemberTypes": [ "User", "Application" ],
Expand Down Expand Up @@ -359,6 +371,10 @@ if [[ -n ${automationClientId} ]]; then
"id": "${imperialOwnerRoleId}",
"type": "Role"
},
{
"id": "${imperialDataEngineerRoleId}",
"type": "Role"
},
{
"id": "${airlockManagerRoleId}",
"type": "Role"
Expand All @@ -385,6 +401,7 @@ JSON
grant_admin_consent "${automationSpId}" "${workspaceSpId}" "${researcherRoleId}"
grant_admin_consent "${automationSpId}" "${workspaceSpId}" "${imperialResearcherRoleId}"
grant_admin_consent "${automationSpId}" "${workspaceSpId}" "${imperialOwnerRoleId}"
grant_admin_consent "${automationSpId}" "${workspaceSpId}" "${imperialDataEngineerRoleId}"
az ad app permission grant --id "$automationSpId" --api "$workspaceAppId" --scope "user_impersonation" --only-show-errors
fi
fi
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ public void validateToken(final String token, final UrlJwkProvider jwkProvider)
|| x.equalsIgnoreCase("WorkspaceResearcher")
|| x.equalsIgnoreCase("ImperialWorkspaceResearcher")
|| x.equalsIgnoreCase("ImperialWorkspaceOwner")
|| x.equalsIgnoreCase("ImperialWorkspaceDataEngineer")
|| x.equalsIgnoreCase("AirlockManager"))) {
throw new GuacamoleInvalidCredentialsException(
"User must have a workspace owner or workspace researcher or Airlock Manager role",
Expand Down
2 changes: 1 addition & 1 deletion ui/app/src/components/shared/ResourceContextMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ export const ResourceContextMenu: React.FunctionComponent<ResourceContextMenuPro
wsAuth = true;
break;
case ResourceType.UserResource:
r = [WorkspaceRoleName.WorkspaceOwner, WorkspaceRoleName.WorkspaceResearcher, WorkspaceRoleName.ImperialWorkspaceResearcher, WorkspaceRoleName.AirlockManager, WorkspaceRoleName.ImperialWorkspaceOwner];
r = [WorkspaceRoleName.WorkspaceOwner, WorkspaceRoleName.WorkspaceResearcher, WorkspaceRoleName.ImperialWorkspaceResearcher, WorkspaceRoleName.AirlockManager, WorkspaceRoleName.ImperialWorkspaceOwner, WorkspaceRoleName.ImperialWorkspaceDataEngineer];
wsAuth = true;
break;
case ResourceType.Workspace:
Expand Down
1 change: 1 addition & 0 deletions ui/app/src/models/roleNames.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,5 @@ export enum WorkspaceRoleName {
AirlockManager = "AirlockManager",
ImperialWorkspaceResearcher = "ImperialWorkspaceResearcher",
ImperialWorkspaceOwner = "ImperialWorkspaceOwner"
ImperialWorkspaceDataEngineer = "ImperialWorkspaceDataEngineer"
}
Loading