-
Notifications
You must be signed in to change notification settings - Fork 9.1k
Custom Security Schemes #1812
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
Custom Security Schemes #1812
Conversation
Added support for additional security schemes by adding a "custom" value to the security scheme type enumeration and a customType field to distinguish between custom schemes.
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.
Comments as above. Otherwise LGTM.
versions/3.1.0.md
Outdated
|
||
This object MAY be extended with [Specification Extensions](#specificationExtensions). | ||
|
||
Custom schemes may be processed in much the same manner as extensions. Implementations shall accept "custom" as a valid value for the securitySchemeType field. They shall also validate that the customType field is present and populated when the value of securitySchemeType is "custom". Any additional data needed to describe the custom schema shall be provided using [Specification Extensions](#specificationExtensions). |
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.
Instead of shall
, can we use MUST
or SHOULD
as deemed appropriate. Can we also say Validating implementations
as not all tooling is or has a validator. Should securitySchemeType
be a link?
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.
1- type
REQUIRED when securityScheme: custom
2- specificationExtension
is RECOMMENDED
3- no changes about type
to be a link. In that case, it should be url
or uri
?
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.
Implementations SHALL accept "custom" as a valid value. Otherwise they would throw an error when processing a valid OAS document. Requiring that the customType field is present and populated is consistent with the handling of other security scheme specific fields. That doesn't mean that the implementation has to do anything with the values, just check that something is there.
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.
Re: customType as a uri. The current text says that the values for the customType field are not controlled. If they are not controlled, then limiting the values to a URI is an unnecessary restriction. However, if we are open to adding custom security schemes to the OAS register----
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.
Implementations SHALL accept "custom" as a valid value.
Otherwise they would throw an error when processing a valid OAS document.
iiuc SHALL/SHOULD means that a validating implementation may not accept custom
. Am I wrong?
Requiring that the customType field is present and populated
is consistent with the handling of other security scheme specific fields.
So 1) is fine, right?
That doesn't mean that the implementation has to do anything with the values, just check that something is there.
I agree, and that's what current patch states. @MikeRalphson can you make an example of what you expect 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.
They shall also validate that the customType field is present and populated when the value of securitySchemeType is "custom".
This whole sentence is redundant if customType
is REQUIRED
when type
is custom
. It's implicit and we don't repeat ourselves like this elsewhere.
Lots of tools don't validate the input document at all, they just muddle through looking at what they understand. They won't necessarily "throw" on seeing custom
. This is true if they are accepting a 3.1.0
document as a backwards compatible superset of the 3.0.0
they actually understand.
When I said securitySchemeType
to be a link, I meant a link to the relevant anchor within the document.
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.
Our goal is the treat customer Security Schemes the same as any other type of Security Scheme. Therefore: "custom" must be recognized as a valid value for the Security Scheme Type and the customType field is required for custom Security Schemes. I think the changes through line 3182 do this. Line 3185 is informative so it does not have to be written in requirement speak. I'm OK with softening the language in this section.
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.
Suggestion: SHALL we move this conversation out of the "outdated" file, eg here https://github.com/OAI/OpenAPI-Specification/pull/1812/files :)
@darrelmiller as discussed, you volunteered to propose new wording around the extensibility of the |
Having reviewed this PR and the current working of the scheme property on Security Requirement Object, it seems sufficiently clear that the
|
This PR
Added support for additional security schemes by adding a "custom" value to the security scheme type enumeration and a customType field to distinguish between custom schemes.
Note
Split from #1736
@cmheazel pls review.