Update path transform to make the security key optional

[gandazgul]

What kind of change does this PR introduce? (check at least one)

• Bugfix
• Feature
• Test
• Docs
• Refactor
• Build-related changes
• Other, please describe:

Description:

If the security key is present in a path, Swagger will take that definition over the global security one. Without this change we are always adding this key and disabling security for all paths that don't have the @security tag.

Modified the transform for a path to only set the security key when values are found in the @security tag.

[gandazgul]

Hold on reviewing this, your tests are failing because of nothing I did. So I will submit a different PR to fix those so that this one is clean.

[gandazgul]

Merge this first: #180

[kevinccbsg]

Thanks for the PR

[kevinccbsg]

Thanks, @gandazgul we will try to do a new release asap with your changes 🚀

[gandazgul]

Awesome thank you!

[paulish]

Seems this MR introduces a breaking change: now all methods have security (with explicit @security ... declaration and without it) definition if server security set. Now you need to explicitly add empty @security declaration to remove method security.

[kevinccbsg]

🤔 Thanks @paulish, we will have a look today and we revert or fix the problem.

[kevinccbsg]

@gandazgul we have to revert this as we cannot allow adding security without setting explicitly on each endpoint as OpenAPI does. The only way I would say this would work is by adding a config option to enable this behavior. In that case, this won't affect old versions.

[gandazgul]

No, this is supposed to be the opposite and I have a test for this case. Empty @security removes the security. If you don't specify it it doesn't add it. If you specify it it uses it. Before my fix it was ALWAYS adding security: [] which is what I fixed! Edit: wait what you describe is the way Swagger works. If you add security globally it applies to everything unless you disable it with an empty array. Before this library was always turning it off when added globally this fixes that. It is a breaking change if it always worked the wrong way, you are right about that. Perhaps we should bump the major version. Don't revert it please... I'll have to use patch-package then to make it work like Swagger does.

…

On Thu, Sep 9, 2021, 12:22 PM Kevin Julián Martínez Escobar < ***@***.***> wrote: @gandazgul <https://github.com/gandazgul> we have to revert this as we cannot allow adding security without setting explicitly on each endpoint as OpenAPI does. The only way I would say this would work is by adding a config option to enable this behavior. In that case, this won't affect old versions. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#179 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA2SMVPWHTNWHGBCFZKVETUBDNKXANCNFSM5CZ74WLQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[gandazgul]

Seems this MR introduces a breaking change: now all methods have security (with explicit @security ... declaration and without it) definition if server security set. Now you need to explicitly add empty @security declaration to remove method security.

It might be breaking but it aligns with Swagger

When used on the root level, security applies the specified security schemes globally to all API operations, unless overridden on the operation level.

Global security can be overridden in individual operations to use a different authentication type, different OAuth/OpenID scopes, or no authentication at all and if you see the example disabling global security for a path requires to add security: []

This library before my fix was adding the empty security array to all endpoints which in turn disabled security. Now it doesn't it takes the global one and it can be override locally including disabling by adding an empty @security tag which aligns with how Swagger works.

Edit: adding Swagger docs link on security: https://swagger.io/docs/specification/authentication/

[paulish]

@gandazgul the problem is that before your changes library had another behavior and projects which used this library relied on that old behavior. As a solution (I did not look at the MR core) I would suggest to add a new option to use new behavior which is disabled by default. With a new major release change the default behavior if needed and deprecate the option. In the next major release remove the option at all.

Also such breaking changes must be well documented.

[richiksc]

Specifying @security annotation on each route can become convoluted on large APIs. The change (which has now been reverted, it seems) brought the behavior of the generator in line with the Swagger/OpenAPI spec and how most new users of the library would expect it to behave. This change should be merged back in and either disabled by an option, or better yet a new major version should be released with this breaking change. It is the "proper" behavior for an OpenAPI tool.