fix: allow anonymous security alternatives#2432
Conversation
Greptile SummaryThis PR fixes
Confidence Score: 5/5Safe to merge; the logic change is a one-liner with correct OpenAPI semantics and is covered by tests. The fix is narrow and correct — an empty security requirement is the standard OpenAPI way to declare an anonymous alternative, and returning nil from DescribeSecurityDefinition is the right response. All changed files are internally consistent: spec, generator logic, generated fixture, and tests all agree. No files require special attention beyond the orphaned bearerAuthContextKey type in the generated file, which is cosmetic.
|
| Filename | Overview |
|---|---|
| pkg/codegen/operations.go | Adds early-return in DescribeSecurityDefinition when an empty security requirement is found, making the whole operation anonymous. Correct OpenAPI semantics; order-dependence of the early-return is noted in a prior thread. |
| internal/test/servers/middleware/fiber/middleware.gen.go | Regenerated: removes BearerAuthScopes const and SetUserValue call from AuthCheck wrapper. bearerAuthContextKey type is now an orphan, but Go does not error on unused types so it compiles fine. |
| internal/test/servers/middleware/fiber/main_test.go | Tests updated to reflect anonymous alternative behaviour; new snapshot test checks generated file omits SetUserValue. assert.True for runtime.Caller and the assert/require pattern discussed in previous thread. |
| internal/test/servers/middleware/fiber/spec.yaml | Adds empty {} security alternative before bearerAuth on /auth-check, triggering the new early-return path. |
| README.md | Adds documentation about empty-security-alternative behaviour; removes stale 'future plan' sentence about server-side validation. |
Reviews (2): Last reviewed commit: "Update Readme" | Re-trigger Greptile
|
This PR kind of relates to #1524 I think that putting the scopes into the generated code was a mistake, and auth middleware should use the OpenAPI spec, rather than the generated code, to make the authentication decision. Our own middleware already doesn't touch the generated scope keys. That being said, the presence of the anonymous scope hides the presence of other required scopes. In an ideal world, you'd have both of those pieces of information available; the required scopes, and the presence of the anonymous scope. I need to think about this a little. I'm honestly leaning towards removing scopes altogether from the generated code (and adding a backward compatibility flag to put them back), and encouraging people never to use this. It was a mistake. |
|
I will fix up and merge this PR, mostly as-is, but add an additional commit myself which will deprecate the API scope keys, and put them behind a compatibility flag. This mistake needs to go away. It leads to insecure, fragile code, even though it was convenient when I originally wrote it many years ago. |
# Conflicts: # internal/test/servers/middleware/fiber/main_test.go
…rnatives
Specs that already list an empty `{}` security alternative alongside named
schemes previously still had the scheme's scopes context key (such as
`BearerAuthScopes`) set by generated middleware. After regenerating, that key
is no longer set for such operations, so authentication middleware that
enforces auth based on its presence will treat those requests as anonymous.
Call this out explicitly in the security documentation.
|
Note to self: Document in the release notes the behavior change around |
Summary
Validation