Feat/dpop by Avantol13 · Pull Request #314 · uc-cdis/gen3sdk-python

Avantol13 · 2026-08-20T21:10:08Z

Depends on:

Feat/dpop authutils#113
Feat/dpop fence#1372
⚠️ Merge those first and then update authutils here!

New Features

DPoP support (CLI & SDK)
Nextflow run support utilizing DPoP (CLI & SDK)

Breaking Changes

3.13 only due to required updates from upstream dependencies, including authutils and other Gen3 libraries (this matches the Python version used and required by other Gen3 services)

Bug Fixes

Improvements

Unit tests parallelized, down to ~7s from ~40s locally (w/ the new tests included) - looks like existing runs would take ~30s before these new tests and on this branch they take ~17s, so still almost half the time with many new tests added
Rip out coupling to indexd by actually mocking in unit tests

Dependency updates

Deployment changes

…elize tests

github-actions · 2026-08-20T21:11:20Z

The style in this PR agrees with black. ✔️

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

github-actions · 2026-08-20T21:37:25Z

Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_dbgap.py	4	0	1	5
tests/test_ras_passport.py	0	0	2	2
tests/test_data_upload.py	8	0	1	9
tests/test_graph_submit_and_query.py	12	1	1	14
tests/test_presigned_url.py	7	0	0	7
tests/test_centralized_auth.py	5	0	0	5
tests/test_google_data_access.py	1	0	0	1
tests/test_audit_service.py	1	0	0	1
tests/test_drs_endpoint.py	2	0	0	2
tests/test_gen3_sdk.py	1	0	0	1
TOTAL	41	1	5	47

Test summary after rerunning failed integration tests

filepath	passed	SUBTOTAL
tests/test_graph_submit_and_query.py	1	1
TOTAL	1	1

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

paulineribeyre · 2026-09-10T20:34:16Z

+        """
+        is_s3 = service == _SERVICE_S3
+        not_forwarded = (
+            _HEADERS_NOT_FORWARDED_TO_S3 if is_s3 else _HEADERS_NOT_FORWARDED


This is a bit redundant since _HEADERS_NOT_FORWARDED_TO_S3 is just _HEADERS_NOT_FORWARDED except "authorization", and the "authorization" header is overwritten anyway a couple lines later if "not is_s3".

Could we just remove "authorization" from _HEADERS_NOT_FORWARDED and get rid of _HEADERS_NOT_FORWARDED_TO_S3?

yeah, good call. I cleaned it up and added a check b/c of case-sensitivity causing duplicates

paulineribeyre · 2026-09-10T20:46:41Z

+        else:
+            logging.warning(
+                f"Refusing to proxy {path}: only /ga4gh/tes and /s3 paths are "
+                "proxied. Check the endpoints your pipeline is configured with."


The paths start with either /ga4gh/tes/v1 or /workflows, see config here.

Plus, the S3 endpoint is also exposed at the app root (see here).

So a request to Gen3 S3 could start with /workflows or /workflows/s3 or /ga4gh/tes/v1 or /ga4gh/tes/v1/s3 (although the last 2 are not documented or used). But never just /s3.

I'm not sure what a reliable way to identify S3 requests while maintaining root S3 endpoint support would be. We could list all the non-S3 routes but that's not very future-proof.

For now to unblock my testing, i made this change, which assumes S3 requests are non-root.

Edit: I see the proxy is accepting /s3 requests and forwarding them to /workflows/s3, and DPOP_PROTECTED_PATHS in gen3-workflow matches that, so maybe I misunderstood the intent. We can discuss it when you're back!

the proxy itself has its own endpoints and then translates those to the real Gen3 Workflow, and I'd rather keep them separate and have the mapping in the proxy itself. We can control in the Nextflow config what local proxy endpoint to hit for S3 and TES respectively (and the auto-generated config should already be doing that). e.g. this should've worked out of the box

in your edit: /ga4gh/tes and /s3 are the proxy's own local namespace, not commons paths. the commons paths only appear in TES_ENDPOINT / S3_ENDPOINT, and the generated nextflow config points nextflow at the local ones.

round trip for S3: nextflow hits 127.0.0.1:port/s3/bucket/key, proxy strips /s3 and appends to {commons}/workflows/s3, signs htu over {commons}/workflows/s3/bucket/key.

gen3-workflow should rebuild that same string to check.

What was the config that sent /workflows/... at the proxy? e.g. why didn't what was written work out of the box?

if it was a hand-written pipeline config or the auto-generated config... we could fix that instead.

re: your changes, if we want to keep this and support commons paths matching locally - I think it needs some updates either way. It looks like this will happen:

/workflows/s3/bucket/key.txt -> ('https://cx/workflows/s3/bucket/key.txt', 's3') /workflows/bucket/key.txt -> ('https://cx/ga4gh/tes/bucket/key.txt', 'tes') /s3/bucket/key.txt -> None

the middle one is the root-mounted S3 case you raised - after /workflows/ matches, anything that isn't /s3/ falls through to the TES base, so it goes to
{commons}/ga4gh/tes/<bucket>/<key> and since the service isn't s3 we replace the
SigV4 header with Authorization: DPoP <token>. and the last one is what the
generated config sends, so generated-config runs 404.

But... I still am not convinced we need to change this. The local proxy and nextflow config can be opinionated - we can choose to only support /s3 -> /workflows/s3 instead of all 4 options the service itself supports.

My integration tests do not use the path that updates the config automatically. I see that I misconfigured aws.client.endpoint (<proxy>/workflows/s3 instead of <proxy>/s3), that's probably where the issue came from. I'll fix that and test - no need to change anything if that works 👍

Maybe a bit of documentation/docstring about this endpoint mapping would be nice though, so the next reader isn't confused like I was?

Ok that was only part of the issue - all the existing TES tests were also pointing at /workflows/s3, which i didn't change when i updated them to use the dpop proxy.

Co-authored-by: Pauline Ribeyre <4224001+paulineribeyre@users.noreply.github.com>

github-actions · 2026-09-14T16:12:06Z

Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_graph_submit_and_query.py	12	1	1	14
tests/test_data_upload.py	8	0	1	9
tests/test_presigned_url.py	8	0	0	8
tests/test_centralized_auth.py	5	0	0	5
tests/test_audit_service.py	1	0	0	1
tests/test_dbgap.py	4	0	1	5
tests/test_google_data_access.py	1	0	0	1
tests/test_gen3_sdk.py	1	0	0	1
tests/test_ras_passport.py	0	0	2	2
TOTAL	40	1	5	46

Test summary after rerunning failed integration tests

filepath	passed	SUBTOTAL
tests/test_graph_submit_and_query.py	1	1
TOTAL	1	1

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

github-actions · 2026-09-14T17:21:40Z

Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_graph_submit_and_query.py	12	1	1	14
tests/test_data_upload.py	8	0	1	9
tests/test_presigned_url.py	8	0	0	8
tests/test_centralized_auth.py	5	0	0	5
tests/test_dbgap.py	4	0	1	5
tests/test_google_data_access.py	1	0	0	1
tests/test_audit_service.py	1	0	0	1
tests/test_gen3_sdk.py	1	0	0	1
tests/test_ras_passport.py	0	0	2	2
TOTAL	40	1	5	46

Test summary after rerunning failed integration tests

filepath	passed	SUBTOTAL
tests/test_graph_submit_and_query.py	1	1
TOTAL	1	1

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

… match the known env vars for tokens so other processes/users cannot just hit it without also sending auth

nss10

Great work. Left some comments, questions and suggestions.

nss10 · 2026-09-28T01:18:59Z

+    # A client acting on behalf of a user appends the user ID to its token.
+    return candidate.split(";userId=")[0] or None


@paulineribeyre -- Do we still need this here? Now that we are moving away from client accessing S3 bucket on users' behalf?

we don't need it anymore

I'll remove - from what I understand we don't need it

nss10 · 2026-09-28T03:44:08Z

 gen3users = "*"
+joserfc = ">=1.7.3"
+
+authutils = {git = "https://github.com/uc-cdis/authutils.git", rev = "feat/dpop"}


Reminder to pin it to master, after authutils PR is merged

nss10 · 2026-09-28T04:24:08Z

+        self.records: dict[str, dict] = {}
+        self.bundles: dict[str, dict] = {}


Should reads and writes to these dicts be thread safe? Since mutliple threads could "technically" write in parallel. I don't think it is an issue with the current use case though.

agreed it's not an issue today. in-process mode is single threaded, and the threaded HTTP mode only serves download_object_manifest, which reads. if a test ever starts writing concurrently over HTTP, we'd maybe need to revisit but I don't expect we'd need / want a fake indexd server for that (we'd probably just patch at the requests / httpx2 call instead of handling this way). All of this was to remove the weird dependency on indexd for unit tests

nss10 · 2026-09-28T22:26:07Z

+    def test_no_requested_lifetime_skips_the_check(self, ec_key, requests_mock):
+        """Without an explicit lifetime the server picks one, so nothing is checked."""
+        token, _ = _exchange(ec_key, api_key=_api_key_expiring_in(-60))
+
+        assert token == TASK_TOKEN
+        assert requests_mock.called


This seems a little off to me — maybe I'm missing something. There is no restriction on fetching a TASK_TOKEN with an already-expired API key as long as no explicit task_token_expiration is provided?

At dpop.py#L920-921 we simply skip the check when task_token_expiration is None. Is this a missed edge case, or are we intentionally deferring to the server to reject the expired API key?

great catch, that was a legit missed edge case. fixing

Co-authored-by: Sai Shanmukha Narumanchi <nss10@outlook.com>

Avantol13

thank you for the comprehensive review!! I know this was a lot of new code and changes

Avantol13 · 2026-09-30T16:12:14Z

+    def test_challenge_carrying_only_a_nonce_header_is_retried(
+        self, ec_key, requests_mock
+    ):
+        """A refusal with a nonce but no explanation is still read as a nonce demand."""
+        requests_mock.post(
+            TOKEN_ENDPOINT,
+            [
+                {
+                    "status_code": 401,
+                    "text": "",
+                    "headers": {"DPoP-Nonce": "as-nonce-2"},
+                },
+                {"status_code": 200, "json": {"access_token": TASK_TOKEN}},
+            ],
+        )
+
+        assert _exchange(ec_key) == (TASK_TOKEN, "as-nonce-2")


deliberate. per RFC 9449 the server signals with error: use_dpop_nonce, but servers can attach a DPoP-Nonce header to every response on a DPoP endpoint, so the header alone doesn't mean "retry with this nonce". the rule in _is_dpop_nonce_error is: if the body explains the failure, trust the body. only when the body is empty is the header the only signal left, so we treat it as a nonce demand. the unrelated error test has a body saying something else, so retrying would just burn the budget and bury the real error. I'll update the docstrings so this is more clear

Avantol13 · 2026-09-30T16:29:53Z

+    def test_no_requested_lifetime_skips_the_check(self, ec_key, requests_mock):
+        """Without an explicit lifetime the server picks one, so nothing is checked."""
+        token, _ = _exchange(ec_key, api_key=_api_key_expiring_in(-60))
+
+        assert token == TASK_TOKEN
+        assert requests_mock.called


great catch, that was a legit missed edge case. fixing

Avantol13 · 2026-09-30T16:36:04Z

+        self.records: dict[str, dict] = {}
+        self.bundles: dict[str, dict] = {}


agreed it's not an issue today. in-process mode is single threaded, and the threaded HTTP mode only serves download_object_manifest, which reads. if a test ever starts writing concurrently over HTTP, we'd maybe need to revisit but I don't expect we'd need / want a fake indexd server for that (we'd probably just patch at the requests / httpx2 call instead of handling this way). All of this was to remove the weird dependency on indexd for unit tests

Avantol13 · 2026-09-30T16:41:25Z

+    @pytest.mark.parametrize(
+        "body,expected",
+        [
+            # Whichever field fence puts its explanation in, it is quoted back.
+            pytest.param({"json": {"message": "no can do"}}, "no can do", id="message"),
+            pytest.param(
+                {"json": {"error_description": "no can do"}},
+                "no can do",
+                id="error_description",
+            ),
+            pytest.param({"json": {"detail": "no can do"}}, "no can do", id="detail"),
+            pytest.param({"json": {"error": "no can do"}}, "no can do", id="error"),
+            # An HTML or plain-text body is included rather than dropped.
+            pytest.param(
+                {"text": "<html>bad gateway</html>"}, "bad gateway", id="html"
+            ),
+            # A JSON error with no field this SDK knows about is reported as-is.
+            pytest.param({"json": {"weird": ["shape"]}}, '"weird"', id="unknown_shape"),
+            # JSON that is not an object at all still has to survive the trip.
+            pytest.param({"json": ["no can do"]}, "no can do", id="json_array"),
+        ],
+    )
+    def test_server_explanation_is_surfaced(
+        self, ec_key, requests_mock, body, expected
+    ):
+        """Whatever the server said reaches the caller."""


good callout, I split it up a bit more, lmk what you think

github-actions · 2026-09-30T18:32:07Z

Integration Tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_graph_submit_and_query.py	12	1	1	14
tests/test_presigned_url.py	8	0	0	8
tests/test_dbgap.py	4	0	1	5
tests/test_centralized_auth.py	5	0	0	5
tests/test_audit_service.py	1	0	0	1
tests/test_google_data_access.py	1	0	0	1
tests/test_gen3_sdk.py	1	0	0	1
tests/test_ras_passport.py	0	0	2	2
TOTAL	32	1	4	37

Please find the detailed integration test report here

Please find the Github Action logs here

github-actions · 2026-09-30T18:32:20Z

Failure Analysis

Please find the detailed test analysis report here

github-actions · 2026-10-01T14:39:32Z

Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_graph_submit_and_query.py	12	1	1	14
TOTAL	12	1	1	14

Test summary after rerunning failed integration tests

filepath	passed	SUBTOTAL
tests/test_graph_submit_and_query.py	1	1
TOTAL	1	1

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

nss10 · 2026-10-02T15:44:53Z

Approved PR -- but these comments need to be addressed before merging.

paulineribeyre · 2026-10-05T17:15:53Z

@@ -0,0 +1,284 @@
+# Nextflow with the Gen3 DPoP Proxy


Nextflow workflows are only a subset of what can be done with the DPoP proxy. I would use TES instead of Nextflow as the primary example, and restructure the doc to something like this

# Using the Gen3 DPoP Proxy ## Overview ## Running the proxy on its own ## Using it from Python ## Nextlow integration ### <existing sections> ## Security properties ## Troubleshooting

adding a table of contents would also be nice

updated in bc49cab

github-actions · 2026-10-05T18:51:01Z

Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_graph_submit_and_query.py	12	1	1	14
TOTAL	12	1	1	14

Test summary after rerunning failed integration tests

filepath	passed	SUBTOTAL
tests/test_graph_submit_and_query.py	1	1
TOTAL	1	1

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

paulineribeyre

lgtm, i did not review the tests

github-actions · 2026-10-06T15:08:52Z

Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_graph_submit_and_query.py	12	1	1	14
TOTAL	12	1	1	14

Test summary after rerunning failed integration tests

filepath	passed	SUBTOTAL
tests/test_graph_submit_and_query.py	1	1
TOTAL	1	1

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

github-actions · 2026-10-07T18:20:26Z

Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_graph_submit_and_query.py	12	1	1	14
TOTAL	12	1	1	14

Test summary after rerunning failed integration tests

filepath	passed	SUBTOTAL
tests/test_graph_submit_and_query.py	1	1
TOTAL	1	1

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

Avantol13 and others added 5 commits August 5, 2026 14:45

feat(dpop): initial dpop support, no tests yet

3523d8b

Apply automatic documentation changes

dea09cf

feat(tests): dpop tests, rip out indexd dependency by mocking, parall…

6b2dc66

…elize tests

Apply automatic documentation changes

a8c56a2

Merge branch 'master' into feat/dpop

104eaff

paulineribeyre reviewed Sep 10, 2026

View reviewed changes

Avantol13 and others added 2 commits September 14, 2026 10:32

Update docs/howto/nextflow.md

a3b1c10

Co-authored-by: Pauline Ribeyre <4224001+paulineribeyre@users.noreply.github.com>

Merge branch 'master' into feat/dpop

c1642b2

feat(headers): cleanup header forwarding

0064eba

fix(dpop): make the proxy itself require task token to be present and…

99bbea4

… match the known env vars for tokens so other processes/users cannot just hit it without also sending auth

paulineribeyre requested review from nss10 and paulineribeyre September 16, 2026 16:15

nss10 reviewed Sep 28, 2026

View reviewed changes

Avantol13 and others added 5 commits September 30, 2026 10:41

Update gen3/dpop.py

7b479dc

Co-authored-by: Sai Shanmukha Narumanchi <nss10@outlook.com>

Update tests/dpop/test_dpop.py

7869a6e

Co-authored-by: Sai Shanmukha Narumanchi <nss10@outlook.com>

Merge branch 'master' into feat/dpop

fa50edd

feat(review): address comments, clarify some code and docs

b1f5f36

chore(tests): update based on review

f824bce

Avantol13 commented Sep 30, 2026

View reviewed changes

chore(deps): relock

97c352d

PlanXCyborg added the TestGraphSubmitAndQuery label Oct 1, 2026

nss10 previously approved these changes Oct 2, 2026

View reviewed changes

paulineribeyre reviewed Oct 5, 2026

View reviewed changes

chore(docs): update based on review

bc49cab

Avantol13 dismissed nss10’s stale review via bc49cab October 5, 2026 18:34

paulineribeyre previously approved these changes Oct 5, 2026

View reviewed changes

chore(dpop): don't support ;userid

249442d

Avantol13 dismissed paulineribeyre’s stale review via 249442d October 6, 2026 14:50

chore(deps): authutils to actual release

11ed493

paulineribeyre force-pushed the feat/dpop branch from 9492c93 to 11ed493 Compare October 7, 2026 17:30

chore(deps): authutils to actual release

ed6977c

		# A client acting on behalf of a user appends the user ID to its token.
		return candidate.split(";userId=")[0] or None

		self.records: dict[str, dict] = {}
		self.bundles: dict[str, dict] = {}

Conversation

Avantol13 commented Aug 20, 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 Aug 20, 2026

Uh oh!

github-actions Bot commented Aug 20, 2026

Integration Tests

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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 Sep 14, 2026

Integration Tests

Uh oh!

github-actions Bot commented Sep 14, 2026

Integration Tests

Uh oh!

nss10 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nss10 Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Avantol13 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Avantol13 commented Aug 20, 2026 •

edited

Loading

paulineribeyre Sep 10, 2026 •

edited

Loading

nss10 Sep 28, 2026 •

edited

Loading