-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make auth infer opt in #2460
Make auth infer opt in #2460
Conversation
return securitySchemes; | ||
|
||
// Default implementation, currently only supports JWT Bearer scheme | ||
return authenticationSchemes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize that this commit removed the code that would've prevented the Bearer
auth scheme from being generated if there was already one provided with options.SecuritySchemes
. Perhaps we should add that back here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combining the use of options.SecuritySchemes
with the infer behaviour (either default or via SecuritySchemesSelector
), with trumping rules etc, feels like a recipe for disaster. So, for this PR, I've simplified the approach so that you either set the security schemes explicitly (via options.SecuritySchemes
) OR you "opt-in" to the infer behavior (via InferSecuritySchemes()
). So, you can use one approach or the other not a combination of both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And of course, in both cases schema filters may be applied for any final tweaks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fair! I think the schema filters ordering is the most important bit here.
Will this be shipped in a patch or minor release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will ship as a minor version release
Fixes #2459 by making the new functionality opt-in. Opt-in is invoked via config method
InferSecuritySchemes()
, which also accepts an optionalFunc
to apply custom mapping from authentication/authorization state in ASP.NET Core to Open API security schemes.