Skip to content

Add support for SecuritySchemesSelector and default implementation #2442

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

Merged

Conversation

captainsafia
Copy link
Collaborator

Addresses part of dotnet/aspnetcore#39761 and part of #1661.

We've been exploring the possibility of automatically generating OpenApiSecuritySchemes by examining authentication/authorization state in an ASP.NET Core app and auto-generating the appropriate annotations.

  • Adds an IAsyncSwaggerProvider interface and implementations of GetSwaggerAsync to SwaggerGenerator to support making async calls to the authentication system
  • Adds a new SecuritySchemesSelector to SwaggerGeneratorOptions for deriving OpenApiSecuritySchemes from AuthenticationSchemes

@captainsafia captainsafia marked this pull request as ready for review June 14, 2022 19:16
@captainsafia
Copy link
Collaborator Author

@domaindrivendev Thoughts on this? Let me know if you'd like more context on the change.


public async Task<OpenApiDocument> GetSwaggerAsync(string documentName, string host = null, string basePath = null)
{
var (applicableApiDescriptions, swaggerDoc, schemaRepository) = GetSwaggerDocumentWithoutFilters(documentName, host, basePath);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reasoning for pulling the document filter processing out of the shared method? The first 7 lines of both GetSwaggerAsync and GetSwagger look identical so just wondering why all of that can't be pulled up into the shared method? And, if there's a good reason, might be good to put a comment there because it's not clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this as a bit of a "premature optimization" in the event that support for async filters is fully pursued per #1661. I've removed it since it is not explicitly required for this change.

@@ -102,5 +108,28 @@ private string DefaultSortKeySelector(ApiDescription apiDescription)
{
return TagsSelector(apiDescription).First();
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to see this mapping of ASP.NET schemes to Open API schemes in the SwaggerGenerator rather than a config helper. Can you elaborate on the usecase for exposing this as an option at all?

Also, I generally try to avoid functions that mutate parameters (see line 121). I think it would be better to have this method accept the ASP.NET schemes only and return the corresponding OpenAPI schemes. So, the option is just "bring your own" mapping of ASP.NET to Open API schemes. But again, I would avoid exposing this option unless there's a strong need vs just adding schemes via the SecuritySchemes property that's already on this class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But again, I would avoid exposing this option unless there's a strong need vs just adding schemes via the SecuritySchemes property that's already on this class.

I had actually started off by just setting the SecuritySchemes property directly but figured that following the selector pattern used for other properties. I do agree that it's probably more straightforward to just set the property internally, particularly in order to avoid the nastiness with passing in a mutated parameter to the selector.

@domaindrivendev
Copy link
Owner

@captainsafia - added some comments

@captainsafia
Copy link
Collaborator Author

@captainsafia - added some comments

I've addressed some of the feedback here. This work emerged as a result of explorations that we've been making into seeing whether it is feasible to introspect all the registered authentication schemes on an ASP.NET Core application and automatically generated the OpenApiSecurityScheme definitions for them. Turns out, this is a little gnarly, largely because authentication configuration is difficult to introspect in an ASP.NET Core app. We opted to support just the JWT-bearer based authentication which we've made easier to introspect by reading options from from config (as of .NET 7 Preview 5).

@domaindrivendev domaindrivendev merged commit b2f185d into domaindrivendev:master Jul 13, 2022
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.

2 participants