Assert the OpenAPI document declares bearer auth on protected paths #80
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/openapi-bearer-assertion"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes task 146. Adds
OpenApiDocumentTests.GetOpenApiDocument_EveryShippedEndpoint_DeclaresSecurityExactlyWhenItsOwnMetadataRequiresAuthorisation, which enumerates every realRouteEndpointthe shipped API maps (viaEndpointDataSource, the same mechanismGroupScopedEndpointAuthorisationTestsalready uses) and compares each one's ownIAuthorizeData/IAllowAnonymousmetadata against whether the generated document declares the bearer security requirement on the matching path/method.Verified against both mutations, then reverted:
AuthenticationSecurityRequirementTransformerforced to emit nothing reddens the new test (and the existing test-only-endpoint test); forced to apply the requirement everywhere reddens the new test and the three existing negative-case tests.On the existing hardcoded path list (
GetOpenApiDocument_TestsMapTheirOwnRoutes_DescribesOnlyTheApisOwnEndpoints): left it as-is. Deriving its expected path set from the sameEndpointDataSourceenumeration would make it tautological — a test route added directly toPlaceMark.Api.Program.csby accident (rather than through a test project'sIStartupFilter, which is the whole point of that test) would appear in both the derived expectation and the document, and the test would pass regardless. The hardcoded list is what actually catches that; the new test's derived approach only works because its assertion is about metadata agreement, not endpoint membership.No new ADR — this extends an existing, established test pattern rather than deciding anything new.
Verdict: mergeable
Circularity. Not trivially circular. The new test independently retrieves metadata via
EndpointDataSource/RouteEndpoint.Metadata(not by calling the transformer, which readscontext.Description.ActionDescriptor.EndpointMetadata) and compares the result to a real generated document. I reproduced both reported mutations (transformer forced to emit nothing / emit unconditionally) and both redden the new test, confirming it's a genuine end-to-end check that a route's document entry tracks its actual metadata.There is a real, narrower circularity though: the
requiresAuthorizationpredicate in the test (IAuthorizeData.Any() && !IAllowAnonymous.Any()) is a hand-copied duplicate of the transformer's own predicate. I mutated the transformer to drop the!IAllowAnonymoushalf (over-declaring on an endpoint carrying both) and the new whole-document test stayed green — only the pre-existingGetOpenApiDocument_EndpointRequiringAuthorizationButAllowingAnonymous_DoesNotDeclareTheSecurityRequirementtest (hardcoded, independent oracle against a purpose-built route) caught it. This isn't a defect introduced by this PR: no shipped endpoint currently combinesRequireAuthorizationwithAllowAnonymous, and ADR-0034 already documents this exact gap and accepts it, proving the precedence rule only via a test-only endpoint. But the new test's doc comment ("the same testAuthenticationSecurityRequirementTransformeritself makes") overstates what it covers — it verifies membership parity (protected vs. not), not the precedence rule between the two attribute types. Worth a one-line comment adjustment so a future reader doesn't assume this test would catch a precedence regression; not blocking.Negative assertion.
expectations.ShouldContain(expectation => !expectation.RequiresAuthorization)only proves the enumeration contains some anonymous route — it doesn't pin down which ones must stay anonymous. Because the expected value is derived from the same live metadata the document is generated from, this test (and the whole-document mechanism generally) cannot catch a business-logic regression where an anonymous-by-design route (e.g./api/auth/login) is accidentally given.RequireAuthorization()— metadata and document would still agree with each other./health/livehas an independent, hardcoded oracle elsewhere in this file (GetOpenApiDocument_UnprotectedProbe_RequiresTheSchemeOnNoOperation); login/register/refresh/logout/OIDC challenge/callback/redeem do not, here or (as far as I checked) anywhere else. Not this PR's job to add that (it's a runtime-behaviour concern, not a doc/reality-parity one), but worth being clear-eyed that "every shipped endpoint" coverage here means doc-vs-metadata agreement, not doc-vs-intent.Enumeration. Verified directly (dumped every
RouteEndpointfrom a liveWebApplicationFactory): the only endpoints inEndpointDataSourceare the 32 real API operations plus/openapi/{documentName}.json, which is excluded. Swagger UI's static assets are served by middleware, notRouteEndpoints, so they never enter the enumeration or the document'spaths— no other exclusion is needed and none is silently missing.Precedence semantics.
IAllowAnonymouswins overIAuthorizeDataregardless of registration order — the transformer's doc comment states this correctly and it matches the framework's actual short-circuit behaviour inAuthorizationMiddleware. The test's predicate encodes the same (correct) rule.No new ADR. Agree — this reads endpoint metadata via
EndpointDataSourcethe same wayGroupScopedEndpointAuthorisationTestsalready does, and asserts a document/metadata relationship ADR-0034 already decided and partially proved. Nothing here is a new architectural decision.Hardcoded path list left alone. Agree with the reasoning. A derived expected-path-set would only ever equal the document's own paths by construction if both were built from the same
EndpointDataSourcewalk — exactly the "route added directly toProgram.cs" failure mode the existing test exists to catch would then pass unnoticed. Keeping it hardcoded is correct.Build and full
PlaceMark.Api.Testssuite (534 tests) clean on 10.0.100; CI green onf27c28e.Rewrote the new test's doc comment (
912ff44) to state what it actually proves — membership parity, mutation-verified both ways — and name the two things it can't catch: theIAllowAnonymousprecedence rule (no shipped route exercises it; covered only by the existing test-only-endpoint tests, ADR-0034's accepted gap) and any specific route staying anonymous (expectation and document derive from the same live metadata, so they move together). Full CI sequence, includingdotnet format --verify-no-changes, is clean.