[GitHub] [geode] kirklund commented on a change in pull request #6885: GEODE-9570: make sure re-authentication works with registered interests

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

kirklund commented on a change in pull request #6885:
URL: https://github.com/apache/geode/pull/6885#discussion_r714102448



##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationDUnitTest.java
##########
@@ -416,21 +439,304 @@ public void cqOlderClientWithClientInteractionWillDeliverEventEventually() throw
     });
   }
 
-  private void startClientWithCQ() throws Exception {
+  @Test
+  public void registeredInterestForDefaultInterestPolicy() throws Exception {
+    int serverPort = server.getPort();
+    clientVM = cluster.startClientVM(0, clientVersion,
+        c -> c.withProperty(SECURITY_CLIENT_AUTH_INIT, UpdatableUserAuthInitialize.class.getName())
+            .withCacheSetup(
+                ccf -> ccf.setPoolSubscriptionEnabled(true).setPoolSubscriptionRedundancy(0))
+            .withServerConnection(serverPort));
+
+    clientVM.invoke(() -> {
+      UpdatableUserAuthInitialize.setUser("user1");
+      Region<Object, Object> clientRegion = ClusterStartupRule.getClientCache()
+          .createClientRegionFactory(ClientRegionShortcut.CACHING_PROXY).create("region");
+      clientRegion.registerInterestForAllKeys();
+    });
+
+    Region<Object, Object> region = server.getCache().getRegion("/region");
+    region.put("1", "value1");
+
+    // refresh user before we expire user1, otherwise we might still be using expired
+    // users in some client operations
+    clientVM.invoke(() -> {
+      UpdatableUserAuthInitialize.setUser("user2");
+    });
+
+    getSecurityManager().addExpiredUser("user1");
+    region.put("2", "value2");
+
+    // for new client, a message will be sent to client to trigger re-auth
+    // for old client, server close the proxy, but client have reconnect mechanism which
+    // also triggers re-auth. In both cases, no message loss since old client
+    // will re-register interests with default interest policy
+    clientVM.invoke(() -> {
+      Region<Object, Object> clientRegion =
+          ClusterStartupRule.getClientCache().getRegion("region");
+      await().untilAsserted(
+          () -> assertThat(clientRegion.keySet()).hasSize(2));
+      // but client will reconnect successfully using the 2nd user
+      clientRegion.put("2", "value2");
+    });
+
+    // user1 should not be used to put key2 to the region in any cases
+    assertThat(getSecurityManager().getAuthorizedOps().get("user1"))
+        .doesNotContain("DATA:READ:region:2");
+  }
+
+  @Test
+  public void registeredInterestForInterestPolicyNone() throws Exception {
+    int serverPort = server.getPort();
+    clientVM = cluster.startClientVM(0, clientVersion,
+        c -> c.withProperty(SECURITY_CLIENT_AUTH_INIT, UpdatableUserAuthInitialize.class.getName())
+            .withCacheSetup(
+                ccf -> ccf.setPoolSubscriptionEnabled(true).setPoolSubscriptionRedundancy(0))
+            .withServerConnection(serverPort));
+
+    clientVM.invoke(() -> {
+      UpdatableUserAuthInitialize.setUser("user1");
+      Region<Object, Object> clientRegion = ClusterStartupRule.getClientCache()
+          .createClientRegionFactory(ClientRegionShortcut.CACHING_PROXY).create("region");
+      clientRegion.registerInterestForAllKeys(InterestResultPolicy.NONE);
+    });
+
+    Region<Object, Object> region = server.getCache().getRegion("/region");
+    region.put("1", "value1");
+    clientVM.invoke(() -> {
+      UpdatableUserAuthInitialize.setUser("user2");
+    });
+    getSecurityManager().addExpiredUser("user1");
+
+    region.put("2", "value2");
+
+    // for old client, server close the proxy, client have reconnect mechanism which
+    // also triggers re-auth, clients re-register interest, but with InterestResultPolicy.NONE
+    // there would be message loss
+    if (TestVersion.compare(clientVersion, test_start_version) <= 0) {
+      clientVM.invoke(() -> {
+        Region<Object, Object> clientRegion =
+            ClusterStartupRule.getClientCache().getRegion("region");
+        await().during(10, TimeUnit.SECONDS).untilAsserted(
+            () -> assertThat(clientRegion.keySet().size()).isLessThan(2));
+        // but client will reconnect successfully using the 2nd user
+        clientRegion.put("2", "value2");
+      });
+    } else {
+      // new client would have no message loss
+      clientVM.invoke(() -> {
+        Region<Object, Object> clientRegion =
+            ClusterStartupRule.getClientCache().getRegion("region");
+        await().untilAsserted(
+            () -> assertThat(clientRegion.keySet()).hasSize(2));
+      });
+    }
+
+    // user1 should not be used to put key2 to the region in any cases
+    assertThat(getSecurityManager().getAuthorizedOps().get("user1"))
+        .doesNotContain("DATA:READ:region:2");
+  }
+
+  @Test
+  public void newClient_registeredInterest_slowReAuth_policyDefault() throws Exception {
+    // this test only test the newer client
+    if (TestVersion.compare(clientVersion, test_start_version) <= 0) {
+      return;
+    }
+
+    int serverPort = server.getPort();
+    clientVM = cluster.startClientVM(0, clientVersion,
+        c -> c.withProperty(SECURITY_CLIENT_AUTH_INIT, UpdatableUserAuthInitialize.class.getName())
+            .withPoolSubscription(true)
+            .withServerConnection(serverPort));
+
+    ClientVM client2 = cluster.startClientVM(1, clientVersion,
+        c -> c.withProperty(SECURITY_CLIENT_AUTH_INIT, UpdatableUserAuthInitialize.class.getName())
+            .withPoolSubscription(true)
+            .withServerConnection(serverPort));
+
+    clientVM.invoke(() -> {
+      UpdatableUserAuthInitialize.setUser("user1");
+      Region<Object, Object> region = ClusterStartupRule.getClientCache()
+          .createClientRegionFactory(ClientRegionShortcut.CACHING_PROXY).create("region");
+
+      // this test will succeed because when clients re-connects, it will re-register inteest
+      // a new queue will be created with all the data. Old queue is destroyed.
+      region.registerInterestForAllKeys();
+      UpdatableUserAuthInitialize.setUser("user11");
+      // wait for time longer than server's max time to wait to ree-authenticate
+      UpdatableUserAuthInitialize.setWaitTime(6000);
+    });
+
+    AsyncInvocation invokePut = client2.invokeAsync(() -> {

Review comment:
       Typing here would be `AsyncInvocation<Void>`.

##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/CacheClientProxyTest.java
##########
@@ -120,4 +120,12 @@ public void deliverMessageWhenSubjectIsNull() {
     verify(securityService, never()).bindSubject(subject);
     verify(securityService, never()).postProcess(any(), any(), any(), anyBoolean());
   }
+
+  @Test
+  public void replacingSubjectShouldNotLogout() {
+    proxy = spy(new CacheClientProxy(cache, notifier, socket, id, true, (byte) 1, version, 1L, true,
+        securityService, subject, clock, statsFactory, proxyStatsFactory, dispatcherFactory));
+    proxy.setSubject(mock(Subject.class));
+    verify(subject, never()).logout();
+  }

Review comment:
       Looks like you don't need to use `spy` here. Nothing is overriding or verifying anything on `proxy` itself.

##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationDUnitTest.java
##########
@@ -320,10 +333,20 @@ public void cqOlderClientWillNotReAuthenticate() throws Exception {
     clientVM.invoke(() -> {
       // even user gets refreshed, the old client wouldn't be able to send in the new credentials
       UpdatableUserAuthInitialize.setUser("user2");
-      await().during(6, TimeUnit.SECONDS)
+      await().during(10, TimeUnit.SECONDS)

Review comment:
       Quick question: why does this call need to change from 6 seconds to 10 seconds?

##########
File path: geode-junit/src/main/java/org/apache/geode/security/UpdatableUserAuthInitialize.java
##########
@@ -38,6 +40,16 @@ public Properties getCredentials(Properties securityProps, DistributedMember ser
     credentials.put("security-username", user.get());
     credentials.put("security-password", user.get());
 
+    Long timeToWait = waitTime.get();
+    if (timeToWait < 0) {
+      throw new AuthenticationFailedException("Something wrong happened.");
+    } else if (timeToWait > 0) {
+      try {
+        Thread.sleep(timeToWait);
+      } catch (InterruptedException e) {
+        e.printStackTrace();
+      }

Review comment:
       Would it be better for this test code to `throw new RuntimeException(e)`? Printing the stack trace is usually not a good idea unless you use a logger in some way. In general, the only thing that will `interrupt` a thread is shutting down an `Executor` and we want that to throw some runtime exception and abort out of there fast.




-- 
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: notifications-unsubscribe@geode.apache.org

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