Skip to content
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

missing cookie apiKey authentication mechanism #208

Closed
matteomazza91 opened this issue Jun 3, 2018 · 9 comments
Closed

missing cookie apiKey authentication mechanism #208

matteomazza91 opened this issue Jun 3, 2018 · 9 comments

Comments

@matteomazza91
Copy link
Contributor

Description

Security Scheme Object defines that apiKey can be in "query", "header" or "cookie".
Actually the generator allows only "query" and "header" as shown by the following snippet:

if (SecurityScheme.Type.APIKEY.equals(securityScheme.getType())) {
            cs.isBasic = cs.isOAuth = false;
            cs.isApiKey = true;
            cs.keyParamName = securityScheme.getName();
            cs.isKeyInHeader = securityScheme.getIn() == SecurityScheme.In.HEADER;
            cs.isKeyInQuery = !cs.isKeyInHeader;
} 
Suggest a fix/enhancement

I had already proposed a PR to the swagger-codegen project to solve this issue.
I can start working on it right now.

@wing328
Copy link
Member

wing328 commented Jun 3, 2018

@matteomazza91 thanks for offering help on this. Please open a PR when you've time and we'll review and get it merged into master quickly.

@wing328 wing328 added this to the 3.0.1 milestone Jun 3, 2018
@jimschubert
Copy link
Member

Do we need to support this only conditionally for 3.0 specs? I may be misreading the linked PR, but it seems this implicitly adds cookie auth support for 2.0 specs as well.

@matteomazza91
Copy link
Contributor Author

you're right. It never checks the version of openAPI. This is correct only for valid input.
I think that this kind of check should be placed in the parser: if the input is not valid, the parser should raise a warning or an error. What do you think about that?

@jimschubert
Copy link
Member

I'm on mobile (which makes search difficult), but I believe we don't explicitly validate the input spec. There are times that a spec may be invalid, but user defined templates don't care. An example could be a missing baseUrl that someone has hardcoded in their custom template.

This may not be an issue if we comment that it's a known edge case in the code. If we're not validating input by default, we may want to change that behavior and provide an option to skip validation. Again, I didn't look so this may already be in place.

@matteomazza91
Copy link
Contributor Author

ok, so possible solutions:

  1. don't check the version for cookie auth

    • valid openApi specs are correctly processed
    • invalid openApi 2 specs can generate cookie auth ("it's not a bug, it's a feature!" cit. )
  2. check the version for cookie auth

    • completely correct for the standard
  3. implement the validation and make it enabled or not with an option

    • most time expensive solution (maybe can be a new issue). I don't think to be able to do this in a reasonable time

@jimschubert
Copy link
Member

I'd like to avoid the first option. The second option would work, but I'd be concerned that this would lead to lots of version conditionals throughout.

So I think the last option is best. I looked briefly and didn't see spec validation on the generate command or on CodegenConfigurator, so it seems this needs to be added (it wasn't a big deal when most users were 2.0 specs). I can look at that tonight or tomorrow. I don't think this needs to be a blocker on merging the addition, and I think a comment that it assumes a validation step prior to generation would remove any confusion.

@wing328
Copy link
Member

wing328 commented Jun 11, 2018

Closing this as #240 has been merged.

@wing328 wing328 closed this as completed Jun 11, 2018
@adamdecaf
Copy link
Contributor

What version was this deployed in? I'm running v3.3.0 and had to use in: header rather than in: cookie.

https://github.com/moov-io/api/commit/34d9c167708df3aaee1e0274dd61271c469d2178#diff-fe030a7c1568b3decf599edf399be7f4R2025

You can clone/fork that project and just run make to build inside a docker image. I can simplify it if needed.

@adamdecaf
Copy link
Contributor

Previously the localVarHeaderParams["{{keyParamName}}"] = key line wasn't being added. The mentioned PR looks to make isKeyInCookie the same as isKeyInHeader for this case, but I'm not totally sure.

https://github.com/moov-io/go-client/pull/2/files#diff-6a8fb16a6861d334f925cfca38f19a85R454

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants