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

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

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



##########
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:
       +1

##########
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:
       +1




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