-
Notifications
You must be signed in to change notification settings - Fork 1
Refactor authz resource tree #103
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
Changes from 29 commits
55c5e7d
3bab856
ecea86d
18a5a1a
b38f3ba
30f6701
34e1bb2
54f8afe
9772135
5fb7502
bd54b42
eaa5b28
8365cf3
79bbb79
411e264
80a5d46
612b597
76025d3
a2324c1
188b0a7
599f1ef
74c5206
82629a6
ce0b098
1402e9e
9525160
1e47ab9
1f7b86f
f5ae28f
ee929fb
fab6149
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 |
|---|---|---|
|
|
@@ -118,43 +118,52 @@ async def authorize( | |
|
|
||
| return authorized | ||
|
|
||
| async def grant_user_access_to_their_own_tasks( | ||
| async def grant_user_access_to_their_own_data( | ||
| self, username: str, user_id: str | ||
| ) -> None: | ||
| """ | ||
| Ensure the specified user exists in Arborist and has a policy granting them access to their | ||
| own Gen3Workflow tasks ("read" and "delete" access to resource "/users/<user ID>/gen3-workflow/tasks" for service "gen3-workflow"). | ||
| own Gen3Workflow tasks and bucket storage. | ||
| Args: | ||
| username (str): The user's Gen3 username | ||
| user_id (str): The user's unique Gen3 ID | ||
| """ | ||
| logger.info(f"Ensuring user '{user_id}' has access to their own tasks") | ||
| resource_path = f"/users/{user_id}/gen3-workflow/tasks" | ||
| if await self.authorize(method="read", resources=[resource_path], throw=False): | ||
| # if the user already has access to their own tasks, return early | ||
| logger.info( | ||
| f"Ensuring user '{user_id}' has access to their own tasks and storage" | ||
| ) | ||
| resource_path1 = f"/services/workflow/gen3-workflow/tasks/{user_id}" | ||
|
Contributor
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. I know I'm nit picking right now, but, if it is not too much trouble, can we update the function to have less code duplication and variable names like Maybe something like base = "/services/workflow/gen3-workflow"
resources_to_create = [
(
f"{base}/tasks",
tasks_path,
f"Represents workflow tasks owned by user '{username}'",
),
(
f"{base}/storage",
storage_path,
f"Represents task storage owned by user '{username}'",
),
]
for parent_path, resource_path, description in resources_to_create:
logger.debug("Attempting to create resource '%s' in Arborist", resource_path)
await self.arborist_client.create_resource(
parent_path,
{"name": user_id, "description": description},
create_parents=True,
)Again, I understand this is nitpicking, and we may not update this function at all in the future, so it is fine if you choose to stick to your design.
Collaborator
Author
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. IMO sometimes trying too hard to reduce code duplication just makes the code more convoluted and harder to read. Your version is not really shorter, and i think it's hard to understand at a glance, so i'll stick to mine |
||
| if await self.authorize(method="read", resources=[resource_path1], throw=False): | ||
| # if the user already has access to their own data, return early | ||
| return | ||
|
|
||
| logger.debug(f"Attempting to create resource '{resource_path}' in Arborist") | ||
| parent_path = f"/users/{user_id}/gen3-workflow" | ||
| parent_path = "/services/workflow/gen3-workflow/tasks" | ||
| logger.debug(f"Attempting to create resource '{resource_path1}' in Arborist") | ||
| resource = { | ||
| "name": "tasks", | ||
| "name": user_id, | ||
| "description": f"Represents workflow tasks owned by user '{username}'", | ||
| } | ||
| await self.arborist_client.create_resource( | ||
| parent_path, resource, create_parents=True | ||
| ) | ||
|
|
||
| role_id = "gen3-workflow_task_owner" | ||
| resource_path2 = f"/services/workflow/gen3-workflow/storage/{user_id}" | ||
| parent_path = "/services/workflow/gen3-workflow/storage" | ||
| logger.debug(f"Attempting to create resource '{resource_path2}' in Arborist") | ||
| resource = { | ||
| "name": user_id, | ||
| "description": f"Represents task storage owned by user '{username}'", | ||
| } | ||
| await self.arborist_client.create_resource( | ||
| parent_path, resource, create_parents=True | ||
| ) | ||
|
|
||
| role_id = "gen3_workflow_admin" | ||
| role = { | ||
| "id": role_id, | ||
| "permissions": [ | ||
| { | ||
| "id": "gen3-workflow-reader", | ||
| "action": {"service": "gen3-workflow", "method": "read"}, | ||
| }, | ||
| { | ||
| "id": "gen3-workflow-deleter", | ||
| "action": {"service": "gen3-workflow", "method": "delete"}, | ||
| "id": "gen3_workflow_admin_action", | ||
|
Contributor
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. Shouldn't we only give
Collaborator
Author
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. Users should be able to cancel tasks and delete inputs/outputs from s3 to lower costs for example |
||
| "action": {"service": "gen3-workflow", "method": "*"}, | ||
| }, | ||
| ], | ||
| } | ||
|
|
@@ -168,13 +177,13 @@ async def grant_user_access_to_their_own_tasks( | |
| ) | ||
| await self.arborist_client.create_role(role) | ||
|
|
||
| policy_id = f"gen3-workflow_task_owner_sub-{user_id}" | ||
| policy_id = f"gen3_workflow_user_sub_{user_id}" | ||
| logger.debug(f"Attempting to create policy '{policy_id}' in Arborist") | ||
| policy = { | ||
| "id": policy_id, | ||
| "description": f"policy created by gen3-workflow for user '{username}'", | ||
| "role_ids": [role_id], | ||
| "resource_paths": [resource_path], | ||
| "resource_paths": [resource_path1, resource_path2], | ||
| } | ||
| await self.arborist_client.create_policy(policy, skip_if_exists=True) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -226,7 +226,7 @@ def create_iam_role_for_bucket_access(user_id: str) -> str: | |
| if config["KMS_ENCRYPTION_ENABLED"]: | ||
| _, kms_key_arn = get_existing_kms_key_for_bucket(bucket_name) | ||
| if not kms_key_arn: | ||
| err_msg = "Bucket misconfigured. Hit the `GET /storage/info` endpoint and try again." | ||
| err_msg = "Bucket misconfigured. Hit the `GET /storage/setup` endpoint and try again." | ||
|
Contributor
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. I always felt, the |
||
| logger.error( | ||
| f"No existing KMS key found for bucket '{bucket_name}'. {err_msg}" | ||
| ) | ||
|
|
||
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.
Do you think we don't need to specify this any more -- -- a special privilege useful for automated testing but not intended for the average user.
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.
Is this going to be a general user feature? We aren't sure about the scalability of this action. (here)[https://github.com/uc-cdis/gen3-workflow/blob/f5ae28fd6a7157fdbd0ba0fe74c9722ef7a1239a/gen3workflow/aws_utils.py#L427C5-L428C37]
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.
Thanks I forgot about that
With the refactor, it's not a special privilege anymore since i got rid of the "user-bucket" resource. You need the same access as when you use the s3 endpoint to delete a file, and all users have that access.
Since end-users can access the endpoint and have permission to delete files, stating in the docs that it shouldn't be used doesn't help - end users don't read those docs. We should just update that code when we have a chance, it's a low priority https://ctds-planx.atlassian.net/browse/MIDRC-1233