GEODE-9792: avoid sending in multiple authentication request on the s…

…ame connection by different threads. (apache#7067)

geode-core/src/main/java/org/apache/geode/cache/client/internal/ConnectionFactoryImpl.java

@@ -153,12 +153,20 @@ void authenticateIfRequired(Connection conn) {
     }
 
     ServerLocation server = conn.getServer();
-    if (!server.getRequiresCredentials() || server.getUserId() != -1) {
+    if (!server.getRequiresCredentials()) {
       return;
     }
 
-    Long uniqueID = AuthenticateUserOp.executeOn(conn, pool);
-    server.setUserId(uniqueID);
+    // make this block synchronized so that when a AuthenticateUserOp is already in progress,
+    // another thread won't try to authenticate again.
+    synchronized (server) {
+      if (server.getUserId() != -1) {
+        return;
+      }
+      Long uniqueID = AuthenticateUserOp.executeOn(conn, pool);
+      server.setUserId(uniqueID);
+    }
+
     if (logger.isDebugEnabled()) {
       logger.debug("CFI.authenticateIfRequired() Completed authentication on {}", conn);
     }

geode-core/src/test/java/org/apache/geode/cache/client/internal/ConnectionFactoryImplTest.java

@@ -18,16 +18,23 @@
 package org.apache.geode.cache.client.internal;
 
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.doCallRealMethod;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import java.util.concurrent.CompletableFuture;
+
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
 
 import org.apache.geode.CancelCriterion;
 import org.apache.geode.distributed.internal.ServerLocation;
+import org.apache.geode.test.junit.rules.ExecutorServiceRule;
 
 public class ConnectionFactoryImplTest {
   private ConnectionFactoryImpl factory;
@@ -100,4 +107,32 @@ public void authenticateIfRequired() {
     verify(pool).executeOn(any(Connection.class), any(Op.class));
     verify(server).setUserId(123L);
   }
+
+  @Rule
+  public ExecutorServiceRule executor = new ExecutorServiceRule();
+
+  @Test
+  public void getSetServerIdShouldBeSynchronizedToAvoidExtraLoginRequest() throws Exception {
+    when(pool.isUsedByGateway()).thenReturn(false);
+    when(pool.getMultiuserAuthentication()).thenReturn(false);
+    when(pool.executeOn(any(Connection.class), any())).thenReturn(123L);
+    when(server.getRequiresCredentials()).thenReturn(true);
+    when(connection.getServer()).thenReturn(server);
+
+    doCallRealMethod().when(server).setUserId(anyLong());
+    doCallRealMethod().when(server).getUserId();
+    // set up the initial userId to be -1
+    server.setUserId(-1);
+
+    CompletableFuture<Void> future1 =
+        executor.runAsync(() -> factory.authenticateIfRequired(connection));
+    CompletableFuture<Void> future2 =
+        executor.runAsync(() -> factory.authenticateIfRequired(connection));
+    future1.get();
+    future2.get();
+
+    // setUserId should only be called once by this two threads. The other time
+    // is called by the test setup.
+    verify(server, times(2)).setUserId(anyLong());
+  }
 }

geode-cq/src/distributedTest/java/org/apache/geode/security/MultiUserAPIDUnitTest.java

@@ -15,11 +15,13 @@
 package org.apache.geode.security;
 
 import static org.apache.geode.cache.Region.SEPARATOR;
+import static org.apache.geode.distributed.ConfigurationProperties.SECURITY_CLIENT_AUTH_INIT;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import java.util.Properties;
 
+import org.junit.After;
 import org.junit.BeforeClass;
 import org.junit.ClassRule;
 import org.junit.Rule;
@@ -37,6 +39,8 @@
 import org.apache.geode.examples.SimpleSecurityManager;
 import org.apache.geode.pdx.JSONFormatter;
 import org.apache.geode.pdx.PdxInstance;
+import org.apache.geode.security.templates.TrackableUserPasswordAuthInit;
+import org.apache.geode.security.templates.UserPasswordAuthInit;
 import org.apache.geode.test.dunit.rules.ClusterStartupRule;
 import org.apache.geode.test.dunit.rules.MemberVM;
 import org.apache.geode.test.junit.rules.ClientCacheRule;
@@ -209,14 +213,25 @@ public void noCredentialCanCreateCacheWithMultiUser() throws Exception {
 
   @Test
   public void jsonFormatterOnTheClientWithSingleUser() throws Exception {
-    client.withCredential("data", "data").withMultiUser(false)
+    client.withProperty(SECURITY_CLIENT_AUTH_INIT, TrackableUserPasswordAuthInit.class.getName())
+        .withProperty(UserPasswordAuthInit.USER_NAME, "data")
+        .withProperty(UserPasswordAuthInit.PASSWORD, "data")
+        .withMultiUser(false)
         .withServerConnection(server.getPort()).createCache();
     Region region = client.createProxyRegion("region");
 
     // with single user, the static method in JSONFormatter can be used
     String json = "{\"key\" : \"value\"}";
     PdxInstance value = JSONFormatter.fromJSON(json);
     region.put("key", value);
+
+    // make sure the client only needs to authenticate once
+    assertThat(TrackableUserPasswordAuthInit.timeInitialized.get()).isEqualTo(1);
+  }
+
+  @After
+  public void after() throws Exception {
+    TrackableUserPasswordAuthInit.reset();
   }
 
   @Test

geode-junit/src/main/java/org/apache/geode/security/templates/TrackableUserPasswordAuthInit.java

@@ -0,0 +1,38 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ *
+ */
+
+package org.apache.geode.security.templates;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.geode.LogWriter;
+import org.apache.geode.security.AuthenticationFailedException;
+
+public class TrackableUserPasswordAuthInit extends UserPasswordAuthInit {
+  public static AtomicInteger timeInitialized = new AtomicInteger(0);
+
+  public static void reset() {
+    timeInitialized.set(0);
+  }
+
+  @Override
+  public void init(final LogWriter systemLogWriter, final LogWriter securityLogWriter)
+      throws AuthenticationFailedException {
+    super.init(systemLogWriter, securityLogWriter);
+    timeInitialized.incrementAndGet();
+  }
+}