-
Notifications
You must be signed in to change notification settings - Fork 50
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
Include the scheme when using http security #10
Conversation
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.
Hi @tommyengstrom! Thanks for your PR. It's great, modulo two very minor points.
Sorry for taking so long to review it. And thanks for using openapi3
:)
@@ -838,8 +838,17 @@ data OAuth2Flows = OAuth2Flows | |||
, _oAuth2FlowsAuthorizationCode :: Maybe (OAuth2Flow OAuth2AuthorizationCodeFlow) | |||
} deriving (Eq, Show, Generic, Data, Typeable) | |||
|
|||
data HttpSchemeType | |||
= HttpSchemeBearer | |||
| HttpSchemeBasic |
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.
The spec (https://swagger.io/specification/#security-scheme-object) for scheme
property with http
type says:
The values used SHOULD be registered in the IANA Authentication Scheme registry.
So I'd say we should either add other options as well (Digest
, HOBA
, ...) or at least provide HttpSchemeCustom Text
constructor.
Also, for Bearer
scheme the spec mentions an optional field bearerFormat
:
A hint to the client to identify how the bearer token is formatted. Bearer tokens are usually generated by an authorization server, so this information is primarily for documentation purposes.
Please add it to the HttpSchemeBearer
constructor as Maybe Text
field.
src/Data/OpenApi/Internal.hs
Outdated
= HttpSchemeBearer | ||
| HttpSchemeBasic | ||
deriving (Eq, Show, Generic, Data, Typeable) | ||
instance ToJSON HttpSchemeType where |
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.
Please move these instances lower in the file, near instances for other security schemes.
Hey I'll have a look at this next week. A bit busy right now, but I intend to do it. |
Hi @tommyengstrom! Did you have a chance to finish this PR? |
Sorry for the delay. Not sure this is what you wanted. I dropped the To/FromJSON instances for |
Thanks! |
Using https://editor.swagger.io/ to view the API it was complaining about the
scheme
property not being included when using http auth.I used this with
servant-auth
and an orphan instance ofHasOpenApi
: