[GitHub] [bookkeeper] Ghatage commented on a change in pull request #2368: Config option to allow only secured client connections

[GitBox <gi...@apache.org>]

Ghatage commented on a change in pull request #2368:
URL: https://github.com/apache/bookkeeper/pull/2368#discussion_r448051873



##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/AuthHandler.java
##########
@@ -92,6 +96,24 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
             }
 
             if (authenticated) {
+                if (msg instanceof BookkeeperProtocol.Request) {

Review comment:
       @eolivelli 
   I can make it use EUA as opposed to creating a new Error code.
   However, just using EUA won't work right? The check for older Versions fails here because it tests for message to be an instance of latest version - V3 and it seems incompatible with V2.
   
   If anything I feel we need to add another check here.
   If we are in here, we know that the request is already authenticated, so we can cast `msg` to `BookkeeperProtocol.Request` and do a `.getVersion()` on that. If that works, it seems that the message is of type Request (may even be an earlier version)
   
   Something like this:
   
   ```
              if (authenticated) {
                   if (((BookieProtocol.Request) msg).getProtocolVersion() || msg instanceof BookkeeperProtocol.Request) {
                       Channel c = ctx.channel();
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org