:fix: remove client id set on connect

Signed-off-by: riccardomodanese <[email protected]>

broker/artemis/plugin/src/main/java/org/eclipse/kapua/broker/artemis/plugin/security/SecurityPlugin.java

@@ -14,7 +14,6 @@
 
 import com.codahale.metrics.Timer.Context;
 import com.fasterxml.jackson.core.JsonProcessingException;
-import com.google.common.base.Strings;
 import io.netty.handler.ssl.SslHandler;
 import org.apache.activemq.artemis.api.core.ActiveMQException;
 import org.apache.activemq.artemis.api.core.ActiveMQExceptionType;
@@ -56,7 +55,6 @@
 import javax.security.cert.X509Certificate;
 import java.security.cert.Certificate;
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicInteger;
 
 /**
  * Kapua Artemis security plugin implementation (authentication/authorization)
@@ -65,10 +63,6 @@ public class SecurityPlugin implements ActiveMQSecurityManager5 {
 
     protected static Logger logger = LoggerFactory.getLogger(SecurityPlugin.class);
 
-    //this class should be a singleton but just to be sure let's use the atomic integer
-    private static final AtomicInteger INDEX = new AtomicInteger();
-    private String clientIdPrefix = "internal-client-id-";
-
     private final LoginMetric loginMetric;
     private final PublishMetric publishMetric;
     private final SubscribeMetric subscribeMetric;
@@ -104,19 +98,7 @@ public Subject authenticate(String username, String password, RemotingConnection
         logger.debug("### authenticate user: {} - clientId: {} - remoteIP: {} - connectionId: {} - securityDomain: {}",
                 username, remotingConnection.getClientID(), remotingConnection.getTransportConnection().getRemoteAddress(), connectionId, securityDomain);
         String clientIp = remotingConnection.getTransportConnection().getRemoteAddress();
-        String clientId = remotingConnection.getClientID();
-        //set a random client id value if not set by the client
-        //from JMS 2 specs "Although setting client ID remains mandatory when creating an unshared durable subscription, it is optional when creating a shared durable subscription."
-        if (Strings.isNullOrEmpty(clientId)) {
-            clientId = clientIdPrefix + INDEX.getAndIncrement();
-            logger.info("Updated empty client id to: {}", clientId);
-        }
-        //leave the clientId validation to the DeviceCreator. Here just check for / or ::
-        //ArgumentValidator.match(clientId, DeviceValidationRegex.CLIENT_ID, "deviceCreator.clientId");
-        if (clientId != null && (clientId.contains("/") || clientId.contains("::"))) {
-            //TODO look for the right exception mapped to MQTT invalid client id error code
-            throw new SecurityException("Invalid Client Id!");
-        }
+        String clientId = extractAndValidateClientId(remotingConnection);
         SessionContext sessionContext = serverContext.getSecurityContext().getSessionContextWithCacheFallback(connectionId);
         if (sessionContext != null && sessionContext.getPrincipal() != null) {
             logger.debug("### authenticate user (cache found): {} - clientId: {} - remoteIP: {} - connectionId: {}", username, clientId, remotingConnection.getTransportConnection().getRemoteAddress(), connectionId);
@@ -143,6 +125,17 @@ public Subject authenticate(String username, String password, RemotingConnection
         }
     }
 
+    private String extractAndValidateClientId(RemotingConnection remotingConnection) {
+        String clientId = remotingConnection.getClientID();
+        //leave the clientId validation to the DeviceCreator. Here just check for / or ::
+        //ArgumentValidator.match(clientId, DeviceValidationRegex.CLIENT_ID, "deviceCreator.clientId");
+        if (clientId != null && (clientId.contains("/") || clientId.contains("::"))) {
+            //TODO look for the right exception mapped to MQTT invalid client id error code
+            throw new SecurityException("Invalid Client Id!");
+        }
+        return clientId;
+    }
+
     private Subject authenticateInternalConn(ConnectionInfo connectionInfo, String connectionId, String username, String password, RemotingConnection remotingConnection) {
         loginMetric.getInternalConnector().getAttempt().inc();
         String usernameToCompare = SystemSetting.getInstance().getString(SystemSettingKey.BROKER_INTERNAL_CONNECTOR_USERNAME);
@@ -155,12 +148,10 @@ private Subject authenticateInternalConn(ConnectionInfo connectionInfo, String c
             logger.info("Authenticate internal: user: {} - clientId: {} - connectionIp: {} - connectionId: {} - remoteIP: {} - isOpen: {}",
                     username, connectionInfo.getClientId(), connectionInfo.getClientIp(), remotingConnection.getID(),
                     remotingConnection.getTransportConnection().getRemoteAddress(), remotingConnection.getTransportConnection().isOpen());
-            //TODO double check why the client id is null once coming from AMQP connection (the Kapua connection factory with custom client id generation is called)
             KapuaPrincipal kapuaPrincipal = buildInternalKapuaPrincipal(getAdminAccountInfo().getId(), username, connectionInfo.getClientId());
-            //auto generate client id if null. It shouldn't be null but in some case the one from JMS connection is.
-            String clientId = connectionInfo.getClientId();
+            //from JMS 2 specs "Although setting client ID remains mandatory when creating an unshared durable subscription, it is optional when creating a shared durable subscription."
             //update client id with account|clientId (see pattern)
-            String fullClientId = Utils.getFullClientId(getAdminAccountInfo().getId(), clientId);
+            String fullClientId = Utils.getFullClientId(getAdminAccountInfo().getId(), connectionInfo.getClientId());
             remotingConnection.setClientID(fullClientId);
             Subject subject = buildInternalSubject(kapuaPrincipal);
             SessionContext sessionContext = new SessionContext(kapuaPrincipal, getAdminAccountInfo().getName(), connectionInfo,