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

[Fixed] leaf node subscription permission negotiation. #2091

Merged
merged 3 commits into from
Apr 9, 2021

Conversation

matthiashanel
Copy link
Contributor

On connect all subscription where sent by the soliciting leaf node.
If creds contains sub deny permissions, the leaf node would be
disconnected.
This waits for the permissions to be exchanged and checks permissions
before sending subscriptions.

Signed-off-by: Matthias Hanel mh@synadia.com

split up leafNodeFinishConnectProcess into two functions.
When expecting multiple messages in a row, use a buffer to store everything after the left most match.
This can be used a lot more. But there are unrelated tests that rely on the old behavior. (will address separately)

@matthiashanel matthiashanel requested a review from kozlovic April 9, 2021 03:31
On connect all subscription where sent by the soliciting leaf node.
If creds contains sub deny permissions, the leaf node would be
disconnected.
This waits for the permissions to be exchanged and checks permissions
before sending subscriptions.

Signed-off-by: Matthias Hanel <mh@synadia.com>
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Some changes, but the important one would be to make sure that leafNodeFinishConnectProcess() is never invoked more than once, which right now we could be at risk to do in sendPermsInfo() ends-up be called more than once.

incorporate review comments

Signed-off-by: Matthias Hanel <mh@synadia.com>
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Just a final change I think.

Signed-off-by: Matthias Hanel <mh@synadia.com>
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM

@matthiashanel matthiashanel merged commit da4430f into master Apr 9, 2021
@matthiashanel matthiashanel deleted the permissions branch April 9, 2021 20:53
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.

2 participants