[GitHub] [activemq-artemis] brusdev commented on a change in pull request #3849: ARTEMIS-3569 - balancer role_name local target, matches role of authe…

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

brusdev commented on a change in pull request #3849:
URL: https://github.com/apache/activemq-artemis/pull/3849#discussion_r750300024



##########
File path: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java
##########
@@ -1126,10 +1127,12 @@ public AMQConnectionContext getContext() {
       @Override
       public Response processAddConnection(ConnectionInfo info) throws Exception {
          try {
-            if (transportConnection.getRedirectTo() != null && protocolManager.getRedirectHandler()
-               .redirect(OpenWireConnection.this, info)) {
-               shutdown(true);
-               return null;
+            if (transportConnection.getRedirectTo() != null) {
+               protocolManager.validateUser(OpenWireConnection.this, info);

Review comment:
       I don't see others calling addConnection. Why not anticipating `protocolManager.validateUser` at the beginning of processAddConnection and removing it from addConnection to avoid the double execution?

##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ActiveMQPacketHandler.java
##########
@@ -175,6 +171,11 @@ private void handleCreateSession(final CreateSessionMessage request) {
             activeMQPrincipal = connection.getDefaultActiveMQPrincipal();
          }
 
+         if (connection.getTransportConnection().getRedirectTo() != null) {
+            server.validateUser(activeMQPrincipal == null ? request.getUsername() : activeMQPrincipal.getUserName(), activeMQPrincipal == null ? request.getPassword() : activeMQPrincipal.getPassword(), connection, protocolManager.getSecurityDomain());

Review comment:
       Why not anticipating server.validateUser and passing the validatedUser parameter to createSession to avoid the double execution?

##########
File path: pom.xml
##########
@@ -206,7 +206,7 @@
 
       <activemq-surefire-argline>-Dorg.apache.commons.logging.Log=org.apache.activemq.artemis.logs.JBossLoggingApacheLoggerBridge -Dorg.apache.activemq.artemis.utils.RetryRule.retry=${retryTests} -Dbrokerconfig.maxDiskUsage=100 -Dorg.apache.activemq.artemis.core.remoting.impl.netty.TransportConstants.DEFAULT_QUIET_PERIOD=0 -Dorg.apache.activemq.artemis.core.remoting.impl.netty.TransportConstants.DEFAULT_SHUTDOWN_TIMEOUT=0 -Djava.util.logging.manager=org.jboss.logmanager.LogManager
          -Dlogging.configuration="file:${activemq.basedir}/tests/config/${logging.config}"
-         -Djava.library.path="${activemq.basedir}/target/bin/lib/linux-x86_64:${activemq.basedir}/target/bin/lib/linux-i686" -Djgroups.bind_addr=localhost -Dorg.apache.activemq.artemis.api.core.UDPBroadcastEndpointFactory.localBindAddress=localhost
+         -Djava.library.path="${activemq.basedir}/target/bin/lib/linux-x86_64:${activemq.basedir}/target/bin/lib/linux-i686" -Djgroups.bind_addr=localhost -Dorg.apache.activemq.artemis.api.core.UDPBroadcastEndpointFactory.localBindAddress=0.0.0.0

Review comment:
       why do you need this change?




-- 
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.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

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