WebSocket proxy should not make a consumer/producer when authorization is failed #448
Conversation
yush1ga
changed the title
| if(!isAuthorized) { | ||
| log.warn("[{}] WebSocket Client [{}] is not authorized on topic {}", session.getRemoteAddress(), role, | ||
| try { | ||
| if(!isAuthorized(authRole)) { |
There was a problem hiding this comment.
actually idea behind keeping isAuthorized async to avoid blocking-thread on authorization check. So, can we keep it async and on future-complete we can call createClient(session) so, we can still keep it async and avoid creation of producer/consumer on authorization failure.
There was a problem hiding this comment.
I returned isAuthorized() to asynchronized and put createClient() in thenApply block.
yush1ga
force-pushed
the
websocket-unauthorized
branch
from
4cdfa3c to
7e6b6a0
Compare
|
|
yush1ga
force-pushed
the
websocket-unauthorized
branch
2 times, most recently
from
5e4aed7 to
dee7e51
Compare
| @@ -81,10 +81,14 @@ public void onWebSocketConnect(Session session) { | |||
| log.warn("[{}] WebSocket Client [{}] is not authorized on topic {}", session.getRemoteAddress(), role, | |||
| topic); | |||
| close(WebSocketError.NotAuthorizedError); | |||
| } else { | |||
| createClient(session); | |||
There was a problem hiding this comment.
do you think it make sense to complete handshake super.onWebSocketConnect(session); after successfully authenticating client, means after calling createClient(session)?
There was a problem hiding this comment.
As mentioned here, onWebSocketConnect seems to be called after handshake is completed and super.onWebSocketConnect(session) seems to only set member variables.
In addition, as you stated in #98, onWebSocketText(..) gets triggered at server only when server releases thread from onWebSocketConnect().
I think we need not move super.onWebSocketConnect(session).
What do you think?
There was a problem hiding this comment.
onWebSocketText(..) gets triggered only when server releases thread from onWebSocketConnect()
It means if we do authorization async then there could be a chance that producer start producing messages even before authorization completed. So, we can keep your previous commit where we do authorization synchronously and it should be faster because we fetch it from ZkCache and get from zk only once.
There was a problem hiding this comment.
I changed isAuthorized into synchronized again.
yush1ga
force-pushed
the
websocket-unauthorized
branch
from
dee7e51 to
bbebf82
Compare
|
@merlimat @saandrews |
### Changes Bump pulsar to 2.8.0-rc-202104202206 and fix the incompatible interface.
Motivation
Now, websocket proxy make an internal consumer/producer by using a super user role even if authorization is failed. (Although websocket connection is closed.)
It causes some problems.
Modifications
isAuthorized()to synchronizedResult
Websocket proxy no longer make consumers/producers for unauthorized users.