Skip to content

Conversation

@webholik
Copy link
Contributor

@webholik webholik commented Sep 4, 2023

If we encounter extra API keys in the security rule set we will add them as extra header/query params in the request.

@webholik webholik requested a review from VShingala September 4, 2023 04:43
@webholik webholik changed the title [IMPORT-820] Add extra API keys as headers or query params Add extra API keys as headers or query params Sep 4, 2023
Copy link
Member

@VShingala VShingala left a comment

Choose a reason for hiding this comment

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

If we make changes into collection generation flow, we also have to take care of this in validateTransactions flow, otherwise users will have issues in that flow where these headers will be removed in suggestions.


if (_.isObject(security) && _.isEmpty(security)) {
helper = {
authHelper = {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we using authHelper here specifically if it will anyways get assigned helper in line no. 1300.

if (_.get(apiKey, 'apikey.2.value') === 'header') {
headers.push(keyValuePair);
}
else {
Copy link
Member

Choose a reason for hiding this comment

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

let's use else if ( === query ) { condition specifically to represent this better here. As API key can be mentioned inside cookie as well.

const securitySet = _.get(openapi, 'security', []);
let count = 0;

_.forEach(securitySet, (security) => {
Copy link
Member

Choose a reason for hiding this comment

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

This is not how you should iterate for multiple API keys.

Take a look at this article: there are combinations of AND and OR in case when multiple keys are mentioned.

https://swagger.io/docs/specification/authentication/api-keys/?sbsearch=API%20Keys

@VShingala
Copy link
Member

@webholik Let's address the comments and make the related changes.

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.

3 participants