Do not flatten securities #15
Conversation
lag-of-death
changed the title
| expect(translateToSecurities([[basicSecurity, implicitSecurity, apiSecurity]])).toMatchSnapshot(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Please add some tests that will verify the OR case (return array of more than one arrays)
| [ | ||
| { | ||
| api_key: [], | ||
| }, |
There was a problem hiding this comment.
Looks good but needs some more tests: something that can prove we support both AND & OR cases.
There was a problem hiding this comment.
@chris-miaskowski @lag-of-death is there anything left to address here?
There was a problem hiding this comment.
@lottamus, no, the test was added:
#15 (comment)
chris-miaskowski
left a comment
chris-miaskowski
left a comment
There was a problem hiding this comment.
Hey @lag-of-death looks good in general but missing some tests. I'm happy to approve when you add them.
|
@chris-miaskowski, the tests are added :) |
There was a problem hiding this comment.
Dropped some thought and comments, although I kind of remember that the security definitions were flattened for some reason that I can't really recall now. I think @marbemac probably knows/was the author of such choice.
I also think this is likely a breaking change and might have some consequences on Studio as well //cc @lottamus ?
|
thanks for the feedback @XVincentX! I've addressed most of it and left some comments for the rest. Please take a look :) |
This was just an oversight on my part. Looks good to me! @lottamus could you create an issue to update elements for stoplightio/types#50? @pytlesk4 could you do a quick investigation of updating analyzer to the latest types package (that includes the PR linked above), and if there are any problems then perhaps create an issue to tackle it soon? |
There was a problem hiding this comment.
Looks ok to me except one question about keeping the security array keys. I believe we might need a way to figure out what the original security array looks like?
Also I find it a bit weird that we're mapping to an array of arrays...wouldn't an easier interface look something like this that would also have the benefit of preserving the security key: 🤷♂
getSecurities(
spec: Partial<OpenAPIObject>,
operation: Partial<OperationObject>,
): Array<Dictionary<SecuritySchemeObject, SecurityString>>[
{
"api_key": {
in: 'header',
name: 'api_key',
type: 'apiKey',
},
"petstore-oauth2": {
authorizationUrl: 'http://swagger.io/api/oauth/dialog',
flow: 'implicit',
scopes: {
'read:pets': 'read your pets',
'write:pets': 'modify pets in your account',
},
type: 'oauth2',
},
},
{
"api_key": {
in: 'header',
name: 'api_key',
type: 'apiKey',
},
"basic-auth": {},
}
]| name: 'api_key', | ||
| type: 'apiKey', | ||
| }, | ||
| { |
There was a problem hiding this comment.
Is there any reason you can think of that we might need the security array's key here? Currently there is nothing tying this oauth2 object back to the security array. Does that make sense?
| { | |
| { | |
| securityKey: 'petstore_auth', |
There was a problem hiding this comment.
@lottamus, I think that given https://github.com/stoplightio/http-spec/pull/15/files#diff-f5ac86d0dd4d4fa1390aca28ac1fbc59L107-L112 it's OK to just not add this securityKey? If it was not a problem before, it shouldn't be a problem now. We can address it later if it turns out to be an issue
I find it a lot easier to work with arrays instead of objects, that's why such a structure |
|
🎉 This PR is included in version 2.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Changes
Security array is not flattened now, so that
andrelation between security schemes can be respected (the relation in described in the links under Specs section below).The issue: stoplightio/prism#509
Specs:
OAS3: https://swagger.io/docs/specification/authentication/#multiple
OAS2: https://swagger.io/docs/specification/2-0/authentication/#multiple