-
Notifications
You must be signed in to change notification settings - Fork 3k
Use proper priority to deploy Vert.x Filter used for legacy websockets #48883
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
Conversation
We should never use the same priority value that Authorization or Authentication use as that can cause problems to other extensions - Fixes: quarkusio#48812
Status for workflow
|
final IndexView index = indexBuildItem.getIndex(); | ||
WebsocketClientProcessor.registerCodersForReflection(reflection, index.getAnnotations(SERVER_ENDPOINT)); | ||
|
||
int priority = 1 + FilterBuildItem.AUTHORIZATION; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is (old) WebSocket so it shouldn't matter and I don't know anything about it, but for record, I think it would be better if this was the opposite - 1-FilterBuildItem.AUTHORIZATION
because this priority is multiplied by -1
and it means that this handler will run before the authorization handler. If they have some HTTP permissions, they will not run before WS handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to open a PR in that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd have to analyze why it worked this way before (why was the priority same), how the legacy WS works etc. I don't think I'll do that. I just tried to provide information, I have no problem with keeping what you have.
We should never use the same priority value that
Authorization or Authentication use as that can
cause problems to other extensions