Refactor authz resource tree by paulineribeyre · Pull Request #103 · uc-cdis/gen3-workflow

paulineribeyre · 2026-02-26T22:46:14Z

Link to JIRA ticket if there is one:

New Features

Breaking Changes

Refactor authz resource tree. See https://github.com/uc-cdis/gen3-workflow/blob/master/docs/authorization.md
Rename /storage/info to /storage/setup since it now also does user authz setup in addition to bucket setup

Bug Fixes

Improvements

Dependency updates

Deployment changes

github-actions · 2026-02-26T22:47:03Z

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

coveralls · 2026-02-27T23:10:28Z

Pull Request Test Coverage Report for Build 22594355466

Details

0 of 0 changed or added relevant lines in 0 files are covered.
40 unchanged lines in 4 files lost coverage.
Overall coverage decreased (-0.2%) to 88.102%

Files with Coverage Reduction	New Missed Lines	%
auth.py	3	89.89%
routes/storage.py	7	86.0%
routes/ga4gh_tes.py	13	86.71%
routes/s3.py	17	84.83%

Totals
Change from base Build 22507255459:	-0.2%
Covered Lines:	659
Relevant Lines:	748

💛 - Coveralls

nss10

Left some comments and questions. Rest looks great. :tada

nss10 · 2026-02-27T22:15:14Z

gen3workflow/routes/storage.py

    token_claims = await auth.get_token_claims()
    user_id = token_claims.get("sub")
+    await auth.authorize(
+        "delete", [f"/services/workflow/gen3-workflow/tasks/{user_id}"]


I think you meant

Suggested change

"delete", [f"/services/workflow/gen3-workflow/tasks/{user_id}"]

"delete", [f"/services/workflow/gen3-workflow/storage/{user_id}"]

nss10 · 2026-02-27T22:15:58Z

gen3workflow/routes/storage.py

    token_claims = await auth.get_token_claims()
    user_id = token_claims.get("sub")
+    await auth.authorize(
+        "delete", [f"/services/workflow/gen3-workflow/tasks/{user_id}"]


Same as above!

Suggested change

"delete", [f"/services/workflow/gen3-workflow/tasks/{user_id}"]

"delete", [f"/services/workflow/gen3-workflow/storage/{user_id}"]

nss10 · 2026-02-27T22:52:08Z

gen3workflow/auth.py

+        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}"


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 tasks_path and storage_path instead of resource_path{1,2}?

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.

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

nss10 · 2026-02-27T22:54:43Z

gen3workflow/aws_utils.py

        _, 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."


I always felt, the GET /storage/info is doing more than what its name says. This is better :D

nss10 · 2026-02-27T23:16:57Z

tests/test_misc.py

+@pytest.mark.parametrize(
+    "access_token_patcher", [{"user_id": NEW_TEST_USER_ID}], indirect=True
+)
 async def test_storage_info(


Suggested change

async def test_storage_info(

async def test_storage_setup(

nss10 · 2026-02-27T23:23:37Z

tests/test_misc.py

+        "/storage/setup", headers={"Authorization": f"bearer {TEST_USER_TOKEN}"}
    )
    bucket_name = res.json()["bucket"]



Can we also do an assert to mock_arborist_request after line 307 to ensure that the arborist call with the correct path is also being hit

method="POST", path=f"/resource/services/workflow/gen3-workflow/storage", body=f'{{"name":"{NEW_TEST_USER_ID}","description":"Represents task storage owned by user \'test-username-{NEW_TEST_USER_ID}\'"}}', authorized=True, )

nss10 · 2026-02-27T23:36:03Z

gen3workflow/routes/s3.py

+    # S3 bucket. It could be supported by hitting the "GET task" endpoint to get the list of
+    # files for a specific task that a user has access to in another user's bucket.


Okay, this is a little confusing to me.

It could be supported by hitting the "GET task" endpoint to get the list of
# files for a specific task that a user has access to in another user's bucket

Did you mean, It could be supported in the future by hitting the "GET task", or am I misunderstanding this?

Yes in the future

nss10 · 2026-02-27T23:41:50Z

tests/conftest.py

                        "logs": [{"system_logs": ["blah"]}],
                        "tags": {
-                            "_AUTHZ": f"/users/OTHER_USER/gen3-workflow/tasks/TASK_ID_PLACEHOLDER"
+                            "_AUTHZ": f"/services/workflow/gen3-workflow/tasks/OTHER_USER/TASK_ID_PLACEHOLDER"


Nothing important, just an unnecessary f-string format

Suggested change

"_AUTHZ": f"/services/workflow/gen3-workflow/tasks/OTHER_USER/TASK_ID_PLACEHOLDER"

"_AUTHZ": "/services/workflow/gen3-workflow/tasks/OTHER_USER/TASK_ID_PLACEHOLDER"

nss10 · 2026-02-27T23:45:50Z

docs/authorization.md


-## Other Gen3-Workflow functionality
- To download inputs and upload outputs, the Funnel workers need `create` access to resource `/services/workflow/gen3-workflow/tasks` on service `gen3-workflow`, like end-users.
- To empty or delete their own S3 bucket, a user needs `delete` access to the resource `/services/workflow/gen3-workflow/user-bucket` on the `gen3-workflow` service -- a special privilege useful for automated testing but not intended for the average user.


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.

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]

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

nss10 · 2026-02-27T23:53:40Z

gen3workflow/auth.py

-                {
-                    "id": "gen3-workflow-deleter",
-                    "action": {"service": "gen3-workflow", "method": "delete"},
+                    "id": "gen3_workflow_admin_action",


Shouldn't we only give gen3_workflow_reader_action and gen3_workflow_creator_action instead of gen3_workflow_admin_action to all the users?

Users should be able to cancel tasks and delete inputs/outputs from s3 to lower costs for example

github-actions · 2026-03-02T20:51:29Z

filepath	error	SUBTOTAL
tests/test_gen3_workflow.py	13	13
TOTAL	13	13

Please find the detailed integration test report here

Please find the Github Action logs here

paulineribeyre added 23 commits February 23, 2026 13:38

print body

55c5e7d

parse 'STREAMING-UNSIGNED-PAYLOAD-TRAILER' body

3bab856

fix multipart upload

ecea86d

deps update

18a5a1a

revert kms change for now; clean up

b38f3ba

log body

30f6701

test STREAMING-UNSIGNED-PAYLOAD-TRAILER

34e1bb2

test STREAMING-UNSIGNED-PAYLOAD-TRAILER

54f8afe

fix header case

9772135

fix: SyntaxWarning: invalid escape sequence '\Z'

5fb7502

fix header case

bd54b42

fix header case

eaa5b28

fix header case

8365cf3

fix header case

79bbb79

clean up

411e264

simplify

80a5d46

fix error: Too little data for declared Content-Length

612b597

fix: Header value must be str or bytes, not int

76025d3

debug

a2324c1

fix: SignatureDoesNotMatch

188b0a7

clean up

599f1ef

clean up

74c5206

reject ls all

82629a6

Refactor authz resource tree

ce0b098

paulineribeyre force-pushed the authz-refactor branch from 1e10af2 to ce0b098 Compare February 26, 2026 22:51

fix graph

1402e9e

paulineribeyre force-pushed the authz-refactor branch from 4ed49d4 to 1402e9e Compare February 26, 2026 22:56

paulineribeyre requested a review from nss10 February 26, 2026 22:59

fix auto authz access

9525160

paulineribeyre added 3 commits February 27, 2026 13:52

rename endpoint for clarity

1e47ab9

fix s3 auth

1f7b86f

more renaming

f5ae28f

This was referenced Feb 27, 2026

Rename storage setup uc-cdis/gen3-code-vigil#484

Merged

Rename storage endpoint uc-cdis/funnel-gen3-plugin#11

Merged

paulineribeyre changed the base branch from debug-chunk to master February 27, 2026 23:04

pull master

ee929fb

nss10 reviewed Feb 28, 2026

View reviewed changes

nss10 requested changes Feb 28, 2026

View reviewed changes

small fixes

fab6149

paulineribeyre requested a review from nss10 March 2, 2026 20:28

nss10 approved these changes Mar 2, 2026

View reviewed changes

paulineribeyre merged commit 3720331 into master Mar 2, 2026
12 of 13 checks passed

paulineribeyre deleted the authz-refactor branch March 2, 2026 21:10

paulineribeyre mentioned this pull request Mar 3, 2026

MIDRC-1228 Multipart upload support #104

Merged

	"delete", [f"/services/workflow/gen3-workflow/tasks/{user_id}"]
	"delete", [f"/services/workflow/gen3-workflow/storage/{user_id}"]

		# S3 bucket. It could be supported by hitting the "GET task" endpoint to get the list of
		# files for a specific task that a user has access to in another user's bucket.

	"_AUTHZ": f"/services/workflow/gen3-workflow/tasks/OTHER_USER/TASK_ID_PLACEHOLDER"
	"_AUTHZ": "/services/workflow/gen3-workflow/tasks/OTHER_USER/TASK_ID_PLACEHOLDER"

Conversation

paulineribeyre commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New Features

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

Uh oh!

github-actions bot commented Feb 26, 2026

Uh oh!

coveralls commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 22594355466

Details

💛 - Coveralls

Uh oh!

nss10 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

paulineribeyre commented Feb 26, 2026 •

edited

Loading

coveralls commented Feb 27, 2026 •

edited

Loading