Skip to content

openapi: the oauth2 flows rule is split between the loader and the compiler #803

Closed

Description

@OmarAlJarrah

What happens

An oauth2 scheme that declares no flow is checked in two places, and each sees only part of the case. The spec marks flows REQUIRED on an oauth2 scheme (3.0.3, 3.1.1 and 3.2.0), and every field of the OAuth Flows Object is optional, so the spellings differ in validity:

Spelling Valid Reported by on main
flows absent no the parser library's required-field rule: an error, located by line and column
flows: {}, or a flows object holding only keys the model does not name yes nothing (#646)
openapi: 3.1.0
info: {title: t, version: "1"}
paths: {}
components:
  securitySchemes:
    s:
      type: oauth2
error openapi/validation/validation-required-field absent.yaml:7:7: `securityScheme.flows` is required for type=oauth2

Exit 1. In the IR the diagnostic's provenance is {"source": 0, "position": {"line": 7, "column": 7}}, with no pointer. Replace type: oauth2 with {type: oauth2, flows: {}} and morphic validate is silent and exits 0. Checked at 210283f.

The finding is located by position because validationDiag locates every library finding through locate(verr.Node), and Provenance.Position is the documented locator for a finding made on a raw node, before a pointer exists. That is by design; the cost is that nothing sites the report at the declaration a reader would edit.

Why it matters

Neither stage can hold the whole rule. The library sees only the absent key. The compiler sees only the lowered flow list, so it cannot tell the spellings apart from the IR alone, though the lowering can (GetFlows() is nil for an absent key and non-nil for {}).

Closing #646 in the compiler while leaving the library's finding in place reports the absent spelling twice: an error and a warning for one defect, in two locator kinds, with an exit code that depends on the spelling. The change open for #646 (#727) takes that shape.

The repo already has the pattern for a rule both stages could check. compilerOwned drops the library's operationId-uniqueness finding because the compiler judges it once it has seen every claim (#502), so there is one report, placed by the compiler.

Proposal (draft)

Hand the whole flows rule to the compiler, as compilerOwned does for operationId uniqueness:

  1. The loader drops the library's flows-required finding for an oauth2 scheme.
  2. lowerSecurityScheme reports every spelling once, at the declaration (decl, not a $ref entry), with a message that does not embed the entry's name. Aliases of one declaration then collapse in compile.Diags, as TestLowerSecuritySchemes_ARefdEntryRecordsFieldsWhereTheyAreWritten already pins for other fields.
  3. Severity follows validity: absent is an error (a REQUIRED field is missing), and a present but flowless object is a warning (valid, but it states no way to obtain a token).

Points to settle before implementing:

  • Identifying the finding. The library's error is a plain *errors.errorString under the generic rule validation-required-field; nothing in it names the field, and matching its message is out. A node-shaped predicate works: the finding's Node is the scheme's mapping node (checked: a !!map with keys [type] for the repro above), so "a required-field finding whose node is a mapping with type: oauth2 and no flows key" identifies it. That ties the drop to library behavior, so it needs a canary: a test that compiles the absent spelling and fails if the library's finding survives the drop or the compiler's report is missing.
  • Other documents. dropped applies "wherever the library makes it, the source or a document a $ref reads". A flowless scheme declared in another file would lose the library's finding, so the compiler must report it there against the right source. openapi: a finding inside an externally referenced document is reported as an unresolved reference at another file's position #537 and openapi: an object an external reference brings in is not validated as the source is #545 are the history of findings from externally referenced documents.
  • Stable codes. The absent spelling's code would change from openapi/validation/validation-required-field to the compiler's. Diagnostic codes are meant to be stable, so decide whether that is acceptable or the compiler should reuse the library's code for this spelling.
  • oauth2MetadataUrl. Whether a 3.2 scheme with flows: {} plus a metadata URL is reported is openapi: an OAuth2 scheme with no flows compiles clean #646's question. This issue's scope is that the answer is applied once, in one place.

An alternative, or a follow-up: point every loader finding. A JSON pointer can be derived from verr.Node; overlay.attribute and defs' positions.pointerOf each resolve a node to a pointer already, but there is no shared helper. That would site every required-field finding (apiKey name, http scheme and the rest) by pointer. It removes the missing pointer for all rules but not the duplication, changes every loader finding in the goldens, and contradicts what Provenance.Position documents today, so it needs its own decision on whether it replaces this proposal or follows it.

Expected

  • One report per defect, whatever the spelling.
  • Absent flows: an error at the declaration's pointer. flows: {} or only unknown keys: a warning at the same pointer. A flows object holding a flow: nothing.
  • A conformance spec with a golden covers each spelling, with an alias declared before its target, so the harness oracles reach it.
  • A canary pins the library finding's identity.

Metadata

Metadata

Assignees

No one assigned

    Labels

    type:featureNew capability or enhancement

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions