-
Notifications
You must be signed in to change notification settings - Fork 2
(MIDRC-1276) Add pre-filtering in funnel query for listing tasks #164
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
base: master
Are you sure you want to change the base?
Changes from 5 commits
a20510c
63f36a0
346f747
10d8e68
448b4db
4802165
5c7f73b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |||||
| import json | ||||||
| import re | ||||||
|
|
||||||
| from fastapi import APIRouter, Depends, HTTPException, Request | ||||||
| from fastapi import APIRouter, Depends, HTTPException, Query, Request | ||||||
| from gen3authz.client.arborist.errors import ArboristError | ||||||
| from starlette.status import ( | ||||||
| HTTP_200_OK, | ||||||
|
|
@@ -37,6 +37,13 @@ | |||||
| TAGS_HIDDEN_FROM_USER.remove("_AUTHZ") | ||||||
|
|
||||||
|
|
||||||
| def get_authz_string_for_user(user_id: str) -> str: | ||||||
| """ | ||||||
| Get the authz_resource string for a user | ||||||
| """ | ||||||
| return f"/services/workflow/gen3-workflow/tasks/{user_id}/TASK_ID_PLACEHOLDER" | ||||||
|
|
||||||
|
|
||||||
| async def get_request_body(request: Request) -> dict: | ||||||
| """ | ||||||
| Extract the body from a FastAPI request | ||||||
|
|
@@ -161,9 +168,7 @@ async def create_task(request: Request, auth=Depends(Auth)) -> dict: | |||||
| ) | ||||||
| logger.error(err_msg) | ||||||
| raise HTTPException(HTTP_400_BAD_REQUEST, err_msg) | ||||||
| authz_resource = ( | ||||||
| f"/services/workflow/gen3-workflow/tasks/{user_id}/TASK_ID_PLACEHOLDER" | ||||||
| ) | ||||||
| authz_resource = get_authz_string_for_user(user_id) | ||||||
| body["tags"]["_AUTHZ"] = authz_resource | ||||||
|
|
||||||
| if config["EKS_CLUSTER_NAME"]: | ||||||
|
|
@@ -238,7 +243,13 @@ def apply_view_to_task(view: str, task: dict) -> dict: | |||||
|
|
||||||
| @router.get("/tasks", status_code=HTTP_200_OK) | ||||||
| @router.get("/tasks/", status_code=HTTP_200_OK, include_in_schema=False) | ||||||
| async def list_tasks(request: Request, auth=Depends(Auth)) -> dict: | ||||||
| async def list_tasks( | ||||||
| request: Request, | ||||||
| auth=Depends(Auth), | ||||||
| all: str = Query( | ||||||
| None, description="Get all TES tasks without pre-filtering on _AUTHZ" | ||||||
| ), | ||||||
| ) -> dict: | ||||||
| """ | ||||||
| List the user's GA4GH TES tasks | ||||||
| """ | ||||||
|
|
@@ -262,7 +273,18 @@ async def list_tasks(request: Request, auth=Depends(Auth)) -> dict: | |||||
| requested_view = query_params.get("view") | ||||||
| query_params["view"] = "FULL" | ||||||
|
|
||||||
| # get all the tasks, regardless of access | ||||||
| if all is None: | ||||||
| query_params["tag_key"] = "_AUTHZ" | ||||||
| # get the user_id and construct an authz value | ||||||
| token_claims = await auth.get_token_claims() | ||||||
| user_id = token_claims.get("sub") | ||||||
| if not user_id: | ||||||
| err_msg = "No user sub in token" | ||||||
| logger.error(err_msg) | ||||||
| raise HTTPException(HTTP_401_UNAUTHORIZED, err_msg) | ||||||
| authz_resource = get_authz_string_for_user(user_id) | ||||||
| query_params["tag_value"] = authz_resource | ||||||
|
|
||||||
| url = f"{config['TES_SERVER_URL']}/tasks" | ||||||
| res = await make_tes_server_request( | ||||||
| request.app.async_client, "get", url, params=query_params | ||||||
|
|
@@ -293,6 +315,7 @@ async def list_tasks(request: Request, auth=Depends(Auth)) -> dict: | |||||
| raise HTTPException(e.code, e.message) | ||||||
|
|
||||||
| # filter out tasks the current user does not have access to | ||||||
| # this should be redundant if all is None but is included in case pre-filtering fails | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| listed_tasks["tasks"] = [ | ||||||
|
Comment on lines
318
to
320
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should not need to get the user's access and filter the tasks when we're already filtering on the I'd say let's leave it in, but add a comment stating this^? |
||||||
| apply_view_to_task(requested_view, task) | ||||||
| for task in listed_tasks.get("tasks", []) | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description should be user-friendly because that's what goes into the auto-generated swagger doc. Maybe something like "If true, retrieves all the TES tasks you may have access to, instead of just your own. Default: false"?