Skip to content

Policy config: add client PD block (4078-1)#4226

Open
stevenvegt wants to merge 7 commits intofeature/4078-jwt-bearer-two-vpfrom
4078-1-policy-client-pd
Open

Policy config: add client PD block (4078-1)#4226
stevenvegt wants to merge 7 commits intofeature/4078-jwt-bearer-two-vpfrom
4078-1-policy-client-pd

Conversation

@stevenvegt
Copy link
Copy Markdown
Member

@stevenvegt stevenvegt commented May 1, 2026

Parent PRD

#4078

Summary

First foundational PR of the #4078 chain. The policy config loader now recognizes an optional service_provider PresentationDefinition per credential profile, alongside the existing organization and user. Loaded service_provider PDs are exposed through the existing WalletOwnerMapping under a new WalletOwnerServiceProvider key so consumers added in PR #4227 can pick them up without touching the loader again. No production consumer reads the new key in this PR.

What changed

  • vcr/pe/policy.go — added WalletOwnerServiceProvider = WalletOwnerType("service_provider") next to the existing WalletOwnerOrganization/WalletOwnerUser constants.
  • policy/local.go — added a ServiceProvider *validatingPresentationDefinition field on credentialProfileConfig, threaded it through toWalletOwnerMapping(), and relaxed the load-time check from "at least one of organization or user" to "at least one of organization, service_provider, or user". The new field uses the same validating type as its siblings, so PE schema validation runs on unmarshal.
  • policy/local_test.go — folded the new scenarios into TestStore_LoadFromFile: org-only loading now asserts service_provider is absent from the mapping (covers the unchanged-behavior bullet); new subtests cover combined org+service_provider+user loading, service-provider-only loading, malformed-service-provider rejection, and the new "no PD defined" branch.
  • docs/pages/deployment/policy.rst — operator-facing description of the service_provider block, including the RFC021-vs-RFC-7523 audience caveat.
  • docs/pages/release_notes.rst — entry under Unreleased ▸ New features.

How to review

  1. Start with vcr/pe/policy.go and policy/local.go — the whole functional change is ~10 lines.
  2. Look at policy/local_test.go — every new test fixture is referenced from a subtest in TestStore_LoadFromFile. Note the malformed fixture (policy/test/invalid/invalid_service_provider_pd.json) deliberately omits a valid organization so the schema error can only originate from the service_provider field.
  3. Verify the operator-doc paragraph in docs/pages/deployment/policy.rst is accurate for the protocol semantics — the verifier silently ignores WalletOwnerServiceProvider keys today, which is correct for RFC021 but worth confirming you agree that documenting it (rather than guarding against it) is the right call.
  4. The four other modified consumers of WalletOwnerMapping (under auth/) are intentionally untouched in this PR; PR Two-VP token request flow (4078-2) #4227 wires the client-side flow.

Deviations from spec

  • Naming. The implementation spec called the new block client. That term overloads httpClient/authzenClient and the OAuth client_id form parameter, so the block is named service_provider instead — matching the domain term used in the PRD's prose and the snake_case convention of sibling compound keys (wallet_owner_type, scope_policy, auth.experimental.jwt_bearer_client).
  • Validation relaxed. The spec says the new block is optional and existing files should still load. To make it a first-class block, the load-time check now requires "at least one of organization, service_provider, or user" instead of "organization or user". A profile that defines only service_provider is therefore valid.
  • Verifier-side semantics documented, not guarded. Adding WalletOwnerServiceProvider to WalletOwnerMapping means the existing inbound-RFC021 verifier flow (PEXConsumer.next in auth/api/iam/session.go) will silently skip a service_provider entry if one ever ends up on a profile shared with that flow. This is correct behavior — RFC021 has no service-provider-PD concept — but the asymmetry is documented in the operator guide rather than enforced with a runtime guard.
  • Docs and release notes updated even though the spec did not ask for it; this is a new operator-visible config block.
  • Test layout. The new subtests live inside TestStore_LoadFromFile rather than a separate test function, to avoid splitting the load-success/load-failure responsibility.

Dependencies

None — this is the first PR in the #4078 chain and can be reviewed independently. PRs #4227, #4228, #4229 depend on it.

Design context

Original implementation spec (used during AI-assisted development)

Foundational change. Adds client as a third wallet-owner type alongside organization and user in the policy config. No behavior change for existing callers. (Renamed to service_provider during implementation — see Deviations.)

What to build

  • Extend the policy config loader/schema to recognize a client block per credential profile entry. Look for WalletOwnerType (or equivalent) in policy/local.go and the loader under policy/.
  • The client block must be a valid PE PresentationDefinition (same shape as the existing organization and user).
  • The client block is optional — entries without it must continue to load unchanged.
  • No consumers wire up client in this PR — it's just loaded and made available on the in-memory policy structure.

Modules touched

  • policy/local.go (and tests)
  • Whatever struct holds per-profile PDs (likely a WalletOwnerMapping-shaped type) — extend or update the iteration to include client.
  • Test fixture policy files: add at least one fixture that has a client block to exercise the new path.

Testing

  • Unit test: a policy file with organization + client + user loads, and all three PDs are accessible.
  • Unit test: a policy file with only organization (existing shape) still loads unchanged.
  • Unit test: a malformed client block (e.g. invalid JSON, non-PE structure) returns a clear error.

Acceptance Criteria

  • Policy loader accepts an optional service_provider PD per credential profile.
  • Existing policy files (without service_provider) continue to load with no change in behavior.
  • Unit tests cover present, absent, and malformed cases.
  • No consumer code is changed in this PR — wiring happens in PR 2.

@qltysh
Copy link
Copy Markdown

qltysh Bot commented May 1, 2026

Qlty


Coverage Impact

⬆️ Merging this pull request will increase total coverage on feature/4078-jwt-bearer-two-vp by 0.01%.

Modified Files with Diff Coverage (1)

RatingFile% DiffUncovered Line #s
Coverage rating: C Coverage rating: C
policy/local.go100.0%
Total100.0%
🚦 See full report on Qlty Cloud »

🛟 Help
  • Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

  • Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

  • File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

    • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

stevenvegt and others added 4 commits May 1, 2026 16:27
Adds the WalletOwnerClient constant and threads an optional `client`
PresentationDefinition through the policy config loader. Consumers
iterating WalletOwnerMapping see the new entry only when a profile
declares a `client` block; existing org-only profiles are unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Locks the invariant that the client field uses the validating type, so
a malformed client PD (e.g. missing input_descriptors) is rejected at
load time with the same schema error as organization and user.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Relaxes the load-time check from "at least one of organization or user"
to "at least one of organization, client, or user", so a profile that
only configures the OAuth-client PD loads. The error message is updated
to list all three valid blocks.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Fold the new client-PD subtests into TestStore_LoadFromFile so loading
  is exercised in one place, and assert WalletOwnerMapping contents
  instead of just length so an org-only profile no longer accidentally
  qualifies as a passing absence-of-client check.
- Cover the new "no PD defined" validation branch with a dedicated
  fixture (invalid/no_pds.json), and tighten the malformed-client
  fixture to contain only a malformed client block so the schema error
  can only originate from that field.
- Document the client block in the operator policy guide, including the
  RFC021 vs RFC 7523 audience caveat, and add a release-notes entry.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@stevenvegt stevenvegt marked this pull request as ready for review May 1, 2026 14:58
Comment thread docs/pages/deployment/policy.rst Outdated
The original "client" name overloaded an already-busy term in this
codebase (httpClient, authzenClient, the OAuth client_id form param)
and required a comment to disambiguate. Switching to the domain term
"service_provider" — the role this PD describes in the LSPxNuts
delegation model — removes that ambiguity and matches the snake_case
convention used by sibling compound keys (wallet_owner_type,
scope_policy, auth.experimental.jwt_bearer_client).

Renames the WalletOwnerClient constant, the credentialProfileConfig
field, the JSON config key, the test fixtures and directory, the
operator-doc paragraph, and the release-notes entry.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@qltysh
Copy link
Copy Markdown

qltysh Bot commented May 1, 2026

1 new issue

Tool Category Rule Count
qlty Structure Function with many returns (count = 7): loadFromFile 1

…sn't

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@stevenvegt stevenvegt requested a review from reinkrul May 1, 2026 15:26
stevenvegt added a commit that referenced this pull request May 5, 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>
stevenvegt added a commit that referenced this pull request May 5, 2026
- 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants