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

Wrong Security swagger generation #76

Open
Leogiciel opened this issue Apr 26, 2019 · 7 comments
Open

Wrong Security swagger generation #76

Leogiciel opened this issue Apr 26, 2019 · 7 comments
Assignees
Labels

Comments

@Leogiciel
Copy link

Leogiciel commented Apr 26, 2019

Security decorator needs two params to be generated correctfully.

eg :
@Security("bearer", ["admin","operator"])

In typescript-rest, you only implemented the second parameters (roles), so Swagger can't recognize it and generate swagger.yaml file properly :

security: - '[object Object]': []

instead of

security: - 'bearer': ["admin", "operator"]

@jackghicks
Copy link

@Leogiciel Hi, did you come up with a workaround for this issue by any chance? I'm curious to know how you deal with it, as I have come up against it myself.

@Leogiciel
Copy link
Author

Leogiciel commented May 23, 2019

Yes @jackghicks, you can use our fork of typescript-rest package : https://github.com/nexussmart/typescript-rest

This should do the trick :

image

@jackghicks
Copy link

@Leogiciel thanks for sharing this. have you published yours to npm?
@thiagobustamante if you have a moment, this fix would be good to apply and publish to npm on the main typescript-rest package

for now, I am able to use @Security() without params for most of my application but this will be a problem later, and I'd like to keep my deployment as simple and unsurprising as possible :)

@Leogiciel
Copy link
Author

@jackghicks, I've made the package public : https://www.npmjs.com/package/@dev-volpy/typescript-rest

@jackghicks
Copy link

I've just realized the dichotomy behind this bug. I think the issue is really with the Swagger generator. It seems to me that in the Swagger spec, you need to specify the name of the securityDefinition, together with the scopes (only if oauth2, also it is misnamed variable 'roles' in typescript-rest's Security decorator).

However, in the typescript-rest @Security implementation, the type of authentication is not taken into account during authorization at all, which is why it is missing.

Perhaps the developer intended to allow flexibility here, by omitting the name and making the roles optional too, you can easily specify with @Security() that there must be some form (any valid) of authentication for the decorated routes, or that the scopes, where supplied, must be satisfied, but by any available authentication type. This would be a bit odd, since only oauth2 would allow scopes anyway. If this is the case, then the typescript-rest-swagger generator should respect the securityDefinition defined in swagger.config.json, and generate all the possibilities where the scopes or name has not been supplied.

Alternatively, the name parameter should become required, as in your patch, and perhaps passed through so that it can be used in authorization if required.

All this said, it does seem reasonable to simply leave the name parameter as an unused variable like you have done here. At least temporarily.

@jackghicks
Copy link

In the end I opened a pull request for typescript-rest-swagger, to retroactively add support for the typescript-rest style of @Security decorator to typescript-rest-swagger. In this way, you can continue to use typescript-rest on the main repo, but switch your typescript-rest-swagger dependency to use the patched version. I published it here:
https://www.npmjs.com/package/@thrivesoft/typescript-rest-swagger

Pull request is here: #78

@evelyne24
Copy link

evelyne24 commented Jun 18, 2019

Can you give an example on how to use this patched version with Authorization Bearer pls? I opened a new bug to typescript-rest because the boilerplate project and the documentation need updating. thiagobustamante/typescript-rest#101

I'm not sure how to use your version @jackghicks ? Do you have an example? Is this compatible with typescript-rest 2.2.0?

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

No branches or pull requests

4 participants