-
Notifications
You must be signed in to change notification settings - Fork 12
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
Feature - add operation filters for Shared Access Key and Certificate authentications in OpenAPI docs #172
Feature - add operation filters for Shared Access Key and Certificate authentications in OpenAPI docs #172
Conversation
… authentications in OpenAPI docs
A new preview package for You can pull it locally via the CLI: PM> Install-Package Arcus.WebApi.All -Version 20200714.0.0-PR-172 -Source https://www.myget.org/F/arcus/api/v3/index.json |
src/Arcus.WebApi.OpenApi.Extensions/CertificateAuthenticationOperationFilter.cs
Outdated
Show resolved
Hide resolved
src/Arcus.WebApi.OpenApi.Extensions/CertificateAuthenticationOperationFilter.cs
Outdated
Show resolved
Hide resolved
src/Arcus.WebApi.OpenApi.Extensions/SharedAccessKeyAuthenticationOperationFilter.cs
Outdated
Show resolved
Hide resolved
src/Arcus.WebApi.OpenApi.Extensions/SharedAccessKeyAuthenticationOperationFilter.cs
Outdated
Show resolved
Hide resolved
src/Arcus.WebApi.OpenApi.Extensions/SharedAccessKeyAuthenticationOperationFilter.cs
Show resolved
Hide resolved
…perationFilter.cs Co-authored-by: Tom Kerkhove <kerkhove.tom@gmail.com>
Co-authored-by: Tom Kerkhove <kerkhove.tom@gmail.com>
A new preview package for You can pull it locally via the CLI: PM> Install-Package Arcus.WebApi.All -Version 20200720.0.0-PR-172 -Source https://www.myget.org/F/arcus/api/v3/index.json |
…ijnmoreels/arcus.webapi into feature/sak-cert-auth-openapi
A new preview package for You can pull it locally via the CLI: PM> Install-Package Arcus.WebApi.All -Version 20200727.0.0-PR-172 -Source https://www.myget.org/F/arcus/api/v3/index.json |
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.
LGTM - Just adding a concern with respect to the config side of things for the security definition name.
For example:
swaggerGenerationOptions.OperationFilter<CustomSecurityRequirementsOperationFilter<SharedAccessKeyAuthenticationAttribute>>(ApiKeySecurityDefinitionId);
swaggerGenerationOptions.AddSecurityDefinition(ApiKeySecurityDefinitionId, new OpenApiSecurityScheme
{
Name = "X-API-Key",
Type = SecuritySchemeType.ApiKey,
In = ParameterLocation.Header
});
In = ParameterLocation.Header | ||
}); | ||
|
||
setupAction.OperationFilter<SharedAccessKeyAuthenticationOperationFilter>(); |
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.
Are we assuming people use sharedaccesskey
& X-API-Key
or is this configurable?
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.
Made it configurabe.
|
||
setupAction.OperationFilter<OAuthAuthorizeOperationFilter>(new object[] {new [] {"myApiScope1", "myApiScope2"}); | ||
setupAction.OperationFilter<CertificateAuthenticationOperationFilter>(); |
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.
Same here, are we assuming certificate
or is it configurable? What happens if I use a different name?
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.
Made it configurable.
#if NETCOREAPP3_1 | ||
var scheme = new OpenApiSecurityScheme | ||
{ | ||
Scheme = "certificate", |
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.
This is hardcoded but must match what was defined as a security requirement, we should make this configurable
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.
Made it configurable. Thanks!
#if NETCOREAPP3_1 | ||
var scheme = new OpenApiSecurityScheme | ||
{ | ||
Scheme = "sharedaccesskey", |
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.
This is hardcoded but must match what was defined as a security requirement, we should make this configurable
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.
Made it configurable. Thanks!
Hmm, that isn't the case with the OAuth operation filter we have. Is it bc we don't have these shared access key and certificate operation filters by default in the security scheme? |
I think we should go that route because you can assign any name to the security scheme depending of what you want. We can make it optional and use those as defaults, but allow them to specify something else if they want to maybe? |
Yes, that seems appropriate. |
A new preview package for You can pull it locally via the CLI: PM> Install-Package Arcus.WebApi.All -Version 20200827.0.0-PR-172 -Source https://www.myget.org/F/arcus/api/v3/index.json |
Making our two authentication mechanisms available on operation level in the OpenAPI docs by providing two custom implemented
IOperationFilter
instances.Closes #170
Closes #169