API binding: accept service_provider_subject_id on request-service-access-token (4078-3)#4228
Open
stevenvegt wants to merge 10 commits into4078-2-two-vp-flowfrom
Open
API binding: accept service_provider_subject_id on request-service-access-token (4078-3)#4228stevenvegt wants to merge 10 commits into4078-2-two-vp-flowfrom
stevenvegt wants to merge 10 commits into4078-2-two-vp-flowfrom
Conversation
|
Coverage Impact ⬆️ Merging this pull request will increase total coverage on Modified Files with Diff Coverage (3)
🤖 Increase coverage with AI coding...🚦 See full report on Qlty Cloud » 🛟 Help
|
This was referenced May 1, 2026
Threads the new optional OpenAPI field through the handler to the IAM client's existing serviceProviderSubjectID parameter (added in #4227). The two-VP dispatch and feature gating already live in the IAM client; the handler's job here is just to forward the value. Named service_provider_subject_id rather than client_id (which the PRD body suggested) to avoid overloading with the OAuth client_id form parameter and to match the convention adopted in #4226 (service_provider PD block) and the internal serviceProviderSubjectID parameter. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ssToken @reinkrul: in the two-VP context, the parameter is specifically the organization (HCP) subject — distinguishing it from the service-provider subject in the same signature makes the call sites self-documenting. The dispatcher and the single-VP path keep their generic subjectID since they don't share a signature with the SP subject. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous constructor took 9 positional arguments, including two booleans separated by a duration. A struct makes the call site self-documenting and means future config additions (e.g. the grant-types-enabled list tracked under #4231) become a one-line struct change instead of an N-call-site signature break. The only call site is auth/auth.go. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The experimental-flag gate in RequestServiceAccessToken returns synchronously before any HTTP call, so the TLS server fixture set up by createClientServerTestContext was dead weight for this one test. createClientTestContext gives just the client and the mocks, no TLS server, no JSON metadata fixtures. The other two failure-mode tests (AS-doesn't-advertise, no service_provider PD) genuinely need the AS metadata HTTP fixture and correctly stay on createClientServerTestContext. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the existing single-VP error tests (BuildSubmission returning pe.ErrNoCredentials, DID-method mismatch) for the second wallet trip in the jwt-bearer flow: - SP wallet returns DIDs whose methods are not in the AS's DIDMethodsSupported list — filterDIDsByMethods returns ErrPreconditionFailed. - SP wallet has matching DIDs but BuildSubmission cannot satisfy the service_provider PD — pe.ErrNoCredentials must propagate so the API layer can map it to 412 Precondition Failed. Carry-over from #4227 self-review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The OpenAPI field is *string; an explicit empty string would have routed
into the two-VP flow with a meaningless subject and surfaced as a
misleading 412 'did method mismatch' from a downstream ListDIDs("")
call. Reject empty up front with a clear InvalidInputError, and check
that a non-nil subject actually exists locally — same treatment as the
path-param subjectID.
Adds three sub-tests under "service_provider_subject_id":
- ok - threads through to IAM client (also tightened to assert the
response body, not just NoError)
- empty string is rejected up front
- unknown subject returns 400
Carry-over from #4228 self-review.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Rephrase ClientConfig doc: 'all required unless noted' was inaccurate for bool/duration zero-valued scalars. Now distinguishes interface fields (required) from scalars (zero value is valid). - Add release-notes entry for the new service_provider_subject_id API field, parallel to the existing #4226 / #4227 lines. - Pin DIDMethodsSupported explicitly in the SP method-mismatch test so the assertion doesn't silently flip if the default fixture changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
stevenvegt
force-pushed
the
4078-3-api-client-id
branch
from
acc5ffd to
fbbd56c
Compare
stevenvegt
changed the title
…docs Adds a new top-level section to docs/pages/deployment/policy.rst that explains, for policy authors and node operators: - when the two-VP RFC 7523 jwt-bearer flow triggers (all four conditions: experimental flag, service_provider_subject_id body field, AS metadata advertises jwt-bearer, profile has a service_provider PD) - how VP1 (organization) and VP2 (service_provider) are built and which subject's wallet/PD each uses - cross-VP binding via shared field.id, with a worked example showing a delegating_hcp constraint in both PDs that ties VP2's delegation credential to VP1's HCP issuer - the credential_selection map and how server-captured entries are additively merged with EHR-supplied ones - the four-step required configuration The section is prefaced by a warning that the entire mechanism is experimental and subject to change while the underlying OAuth profile stabilises. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
stevenvegt
requested review from
gerardsn,
reinkrul and
woutslakhorst
as code owners
stevenvegt
changed the base branch from
feature/4078-jwt-bearer-two-vp
to
4078-2-two-vp-flow
4 new issues
|
reinkrul
approved these changes
May 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Parent PRD
#4078
Summary
Adds the optional
service_provider_subject_idbody field onPOST /internal/auth/v2/{subjectID}/request-service-access-token. When present, the handler routes the request into the experimental RFC 7523 two-VPjwt-bearerflow added in #4227; when absent, the existing single-VPvp_token-bearerflow runs unchanged. Also addresses all four deferred carry-overs from #4227's self-review.What changed
API surface (
docs/_static/auth/v2.yaml)service_provider_subject_idstring added toServiceAccessTokenRequest. Description marks it experimental and explains the four conditions that must hold for the two-VP flow to engage.auth/api/iam/generated.go,e2e-tests/browser/client/iam/generated.go).Handler (
auth/api/iam/api.go)request.Body.ServiceProviderSubjectIdto the IAM client'sserviceProviderSubjectIDparameter (added in Two-VP token request flow (4078-2) #4227, previously alwaysnil).InvalidInputError), and the SP subject must exist locally (subjectExistscheck, mirroring the path-paramsubjectIDtreatment).oauth.UnsupportedGrantTypethat the existing error mapping renders as a 4xx.Carry-overs from #4227
*stringsentinel fromClient.RequestServiceAccessTokensubjectID→organizationSubjectIDinsiderequestJwtBearerAccessTokenfor clarity (commit8b710172).iam.NewClientconstructor shapeClientConfigstruct (commite13789f6). Future config additions (e.g. the grant-types-enabled list under #4231) become a one-line struct change.createClientTestContext(commit034f0b28). The other two failure tests genuinely need the AS metadata HTTP fixture and stay on the heavier context.ListDIDsempty /BuildSubmissionErrNoCredentials43f12f69).Documentation (
docs/pages/deployment/policy.rst)New top-level section "Two-VP flow and cross-VP binding (experimental)" with a sphinx warning banner and four subsections covering: when the flow triggers, how VP1 and VP2 are built, cross-VP binding via shared
field.idwith a workeddelegating_hcpexample, thecredential_selectionmap (EHR-supplied vs server-captured), and the four-step required configuration. Release notes entry added.How to review
auth/api/iam/api.go:726-742— the handler, ~15 lines added. The validation block is the only behavioural change; everything else is a one-line thread-through at line 795.auth/api/iam/api_test.go"service_provider_subject_id" subtests — three sibling tests cover threads-through (with response body asserted), empty-string rejected, unknown subject returns 400. The absent-field path is implicitly covered by every existing single-VP test.auth/client/iam/openid4vp.go:73-99— the newClientConfigstruct +NewClient. Pure refactor; the existing 9 positional args map 1:1 to fields. Single call site updated inauth/auth.go.auth/client/iam/openid4vp_test.go— two new SP-side failure-mode subtests, plus the lighter-context conversion of the feature-disabled test.docs/pages/deployment/policy.rst— the new docs section is the only place an operator or PD author can read about the two-VP mechanism end to end. Worth a careful pass for accuracy.Decisions to look at
service_provider_subject_idrather than the PRD'sclient_id. Same reasoning as Policy config: add client PD block (4078-1) #4226 renamingclient→service_provider: avoids overloading with the OAuthclient_idform parameter (RFC 6749), which the two-VP flow specifically does NOT send (RFC 7521 §4.2).*stringpointer to""would have routed into the two-VP flow and surfaced as a misleading 412 "did method mismatch" downstream. Now an explicitInvalidInputErrorwith the message"service_provider_subject_id is optional and cannot be empty: omit the field or set a non-empty value".ClientConfigdoc comment. Distinguishes interface-typed required fields (will nil-pan on first use if missing) from scalar fields where the zero value is meaningful and supported. Validation is intentionally not added insideNewClient(kept consistent with the previous positional signature; out of scope for this PR).Deviations from spec
client_id→service_provider_subject_id(see "Decisions to look at" above).oauth.UnsupportedGrantTypewith the messagejwt-bearer two-VP flow requires auth.experimental.jwt_bearer_client = true, which the existing error-mapping already surfaces as a 4xx. Duplicating the check at the API layer would duplicate the message and add a branch to test for no functional gain.subjectExistsfor the SP subject) — flagged as the one major issue in this PR's self-review and addressed in commiteda026e8rather than deferred.Dependencies
Stacked on #4227 (the two-VP flow internals). Both #4226 and #4227 are now APPROVED and awaiting merge; this PR merges after them.
#4229 (integration test) depends on this PR.
Related issues
auth.experimental.jwt_bearer_clientboolean flag with a list-stylegranttypesenabledconfig and drive AS metadata'sgrant_types_supportedfrom the same source. Filed during Two-VP token request flow (4078-2) #4227 review; the boolean flag stays in this PR and Two-VP token request flow (4078-2) #4227 and goes away under Replace experimental jwt-bearer client flag with a grant-types-enabled config #4231.Design context
field.idmechanism the new docs section explains for operators.service_providerpolicy PD block this flow consults.Original implementation spec (used during AI-assisted development)
Public surface. After this PR, EHR callers can opt in to the two-VP flow by passing
client_id.What to build
OpenAPI spec
docs/_static/auth/v2.yaml. Add an optionalclient_idstring field to theServiceAccessTokenRequestschema.jwt-bearerflow when the AS supports it and aclientPD is configured.auth/api/iam/generated.go(ande2e-tests/browser/client/iam/generated.goif affected).Wiring
request-service-access-token, threadclient_idfrom the request body toRequestRFC021AccessToken's SP-subject parameter (added in PR 2).auth.experimental.jwt_bearer_client = falseandclient_idis present in the request body, return HTTP 400 with a clear "feature disabled" error message.Acceptance Criteria (original)
client_idfield, marked experimental.auth/api/iam/generated.goregenerated.client_idto internal client.client_idpresent → 400. (Skipped — see Deviations.)