Skip to content

[#4253 7/8] feat(policy,discovery): fatal load-time PD validation (Policy 7)#4290

Draft
stevenvegt wants to merge 1 commit into
4253-2-validatorsfrom
4253-7-loadtime-validation
Draft

[#4253 7/8] feat(policy,discovery): fatal load-time PD validation (Policy 7)#4290
stevenvegt wants to merge 1 commit into
4253-2-validatorsfrom
4253-7-loadtime-validation

Conversation

@stevenvegt
Copy link
Copy Markdown
Member

Parent PRD

#4253

Item 7 of 8. Depends on item #2 (#4281, pe.Validate) — branched off 4253-2-validators. Independent of the #3/#4/#5 chain.

Summary

Wire pe.Validate (Policy 7) into the two places that load operator-owned PresentationDefinitions at startup — the local policy backend and discovery service definitions — and make an inconsistent PD fatal at boot. This matches the existing precedent (a PD that fails JSON-schema validation already prevents startup) and the PRD's "fail to load" intent.

Why fatal at boot

  • Operator-owned PDs load once at Configure (startup) into memory; request-time serving (FindCredentialProfile) reads memory, never disk. So load time is the natural and only place to validate them.
  • A structurally invalid PD already fails boot today (via v2.Validate in validatingPresentationDefinition.UnmarshalJSON). Making the semantic same-id check fatal too keeps both classes of invalid policy consistent: the node refuses to run with a broken policy file.
  • Inconsistent PDs are not a caller concern: they never surface in the 4xx range. The only request-time effect of a genuinely conflicting same-id PD comes from Policy 3 (always-on), which yields ErrNoCredentials with a MatchReport explanation — but with fatal boot, such a node never starts serving in the first place.

Backward-compatibility caveat: a same-id PD with conflicting filters currently works (same-id is decorative today — independent per-descriptor matching). After this change it fails to boot. Operators must audit deployed PDs before upgrading; the release notes (#8) carry the audit checklist. This is the breaking edge the PRD's impact assessment already calls out.

Implementation Spec

policy/local.go

In loadFromFile's per-scope loop (lines 177-191), after the existing profile checks, validate each non-nil PD with pe.Validate:

  • Organization, ServiceProvider, User (the three *validatingPresentationDefinition slots).
  • On *pe.PDValidationError, return an error naming the scope, the wallet-owner, and the file, wrapping the typed error (whose message aggregates every conflicting field id), e.g.:
    presentation definition for scope %q (%s) is invalid (file=%s): %w.
  • The error propagates loadFromFile -> loadFromDirectory -> Configure, so the node refuses to start.

(Hooked here rather than in UnmarshalJSON so the message carries scope/owner/file context; UnmarshalJSON only knows the bare PD.)

discovery/definition.go

In ParseServiceDefinition, after the unmarshal (line 72), call pe.Validate(definition.PresentationDefinition):

  • On error return invalid presentation definition in service definition %q: %w with definition.ID.
  • Propagates through loadDefinitions -> Configure, fatal at startup (same as the existing schema validation at line 66).

Testing

  • policy: a policy file whose profile has a same-id type/const conflict fails loadFromFile; the error names the field id, the scope, and the wallet-owner. A consistent same-id PD loads fine.
  • discovery: a service definition with an inconsistent PD fails ParseServiceDefinition naming the service id; a consistent one parses.
  • Confirm a structurally-valid-but-semantically-consistent PD (e.g. same id, same type/const, different paths) is not rejected.

Acceptance Criteria

  • policy/local.go validates each non-nil profile PD at load via pe.Validate; failure is fatal and names scope + owner + file.
  • discovery/definition.go validates the service-definition PD in ParseServiceDefinition; failure is fatal and names the service id.
  • Both surface the aggregated *pe.PDValidationError detail; consistent PDs (incl. path-only same-id differences) load fine.
  • go build ./... and go test ./policy/... ./discovery/... pass.

@qltysh
Copy link
Copy Markdown
Contributor

qltysh Bot commented May 27, 2026

Qlty


Coverage Impact

This PR will not change total coverage.

🚦 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.

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.

1 participant