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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
