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

fix: fix examples for the new "security at operation level" feature #768

Merged
merged 55 commits into from
Apr 26, 2022

Conversation

sekharbans-ebay
Copy link

@sekharbans-ebay sekharbans-ebay commented Apr 22, 2022


title: "Fix examples for the new "security at operation level" feature"


Related issue(s):
#584

Rewords the examples and also introduces the server reference to the channel.

lejenome and others added 30 commits March 14, 2022 13:20
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
sekharbans-ebay and others added 7 commits April 25, 2022 00:27
Co-authored-by: Sergio Moya <1083296+smoya@users.noreply.github.com>
Co-authored-by: Sergio Moya <1083296+smoya@users.noreply.github.com>
Co-authored-by: Sergio Moya <1083296+smoya@users.noreply.github.com>
Co-authored-by: Sergio Moya <1083296+smoya@users.noreply.github.com>
Co-authored-by: Sergio Moya <1083296+smoya@users.noreply.github.com>
Co-authored-by: Sergio Moya <1083296+smoya@users.noreply.github.com>
Co-authored-by: Sergio Moya <1083296+smoya@users.noreply.github.com>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

1 similar comment
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@derberg
Copy link
Member

derberg commented Apr 25, 2022

This example introduces below on each channel:

servers:
     - test_oauth 

then what is the point of having test server if non of channels are available on it?
Why in this example we need to link all the channels with test_oauth?

Comment on lines 56 to 57
streetlights_auth:
- streetlights:write
- streetlights:write
Copy link
Member

Choose a reason for hiding this comment

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

Security requirement object should be in the form of array, not object.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 75 to 76
streetlights_auth:
- streetlights:read
Copy link
Member

Choose a reason for hiding this comment

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

Security requirement object should be in the form of array, not object.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 95 to 96
streetlights_auth:
- streetlights:read
Copy link
Member

Choose a reason for hiding this comment

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

Security requirement object should be in the form of array, not object.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines 114 to 115
streetlights_auth:
- streetlights:read
Copy link
Member

Choose a reason for hiding this comment

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

Security requirement object should be in the form of array, not object.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

@sekharbans-ebay
Copy link
Author

sekharbans-ebay commented Apr 25, 2022

This example introduces below on each channel:

servers:
     - test_oauth 

then what is the point of having test server if non of channels are available on it? Why in this example we need to link all the channels with test_oauth?

Yes. Removed it.

Second thoughts, i acted in haste :). Restored it now. Added reference to 'test' server to smartylighting.streetlights.1.0.event.{streetlightId}.lighting.measured
Havent provided security at the channel/operation level since the test server has only one security method (therefore implied) - and also to point out operation level security is optional.

@sekharbans-ebay
Copy link
Author

sekharbans-ebay commented Apr 25, 2022

This example introduces below on each channel:

servers:
     - test_oauth 

then what is the point of having test server if non of channels are available on it? Why in this example we need to link all the channels with test_oauth?

Added reference to 'test' server to smartylighting.streetlights.1.0.event.{streetlightId}.lighting.measured
Havent provided security at the channel/operation level since the test server has only one security method (therefore implied) - and also to point out operation level security is optional.

@derberg
Copy link
Member

derberg commented Apr 25, 2022

but now, we have only test_oauth, so why someone would explicitly link to it from each channel?

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sekharbans-ebay
Copy link
Author

Added reference to 'test' server to smartylighting.streetlights.1.0.event.{streetlightId}.lighting.measured
Havent provided security at the channel/operation level since the test server has only one security method (therefore implied) - and also to point out operation level security is optional.

Yes! occurred to me as well - so i restored test. This is a better example now.
I added reference to 'test' server to smartylighting.streetlights.1.0.event.{streetlightId}.lighting.measured
Havent provided security at the channel/operation level since the test server has only one security method (therefore implied) - and also to point out operation level security is optional.

@smoya
Copy link
Member

smoya commented Apr 26, 2022

Added reference to 'test' server to smartylighting.streetlights.1.0.event.{streetlightId}.lighting.measured
Havent provided security at the channel/operation level since the test server has only one security method (therefore implied) - and also to point out operation level security is optional.

Yes! occurred to me as well - so i restored test. This is a better example now. I added reference to 'test' server to smartylighting.streetlights.1.0.event.{streetlightId}.lighting.measured Havent provided security at the channel/operation level since the test server has only one security method (therefore implied) - and also to point out operation level security is optional.

Feel free to drop: I think it makes more sense now. However, the last changes made me ask myself if security requirements at operation level security should contain only requirements that are declared in the server security. If that's not a requirement, maybe makes sense to add security requirements also in the smartylighting.streetlights.1.0.event.{streetlightId}.lighting.measured channel, so we demystify? I would also like to know the opinion from @derberg

@smoya
Copy link
Member

smoya commented Apr 26, 2022

@dalelane @derberg @fmvilas we will need the last review on this. It's just the last pending required work on 2.4.0.

Copy link
Collaborator

@dalelane dalelane left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

looks much better,

also I do not think adding security on operation related to test make sense in case of saslScram security + it could suggest that security on operation level is always needed, which is not the case

:shipit:

@smoya
Copy link
Member

smoya commented Apr 26, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit 5856082 into asyncapi:2022-04-release Apr 26, 2022
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.4.0-2022-04-release.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.4.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

9 participants