DonalEvans commented on a change in pull request #7116: URL: https://github.com/apache/geode/pull/7116#discussion_r752749909
########## File path: geode-core/src/distributedTest/java/org/apache/geode/security/SecurityManagerAvailabilityDUnitTest.java ########## @@ -0,0 +1,164 @@ +/* + * 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; + + +import static org.apache.geode.distributed.ConfigurationProperties.SECURITY_CLIENT_AUTH_INIT; +import static org.apache.geode.distributed.ConfigurationProperties.SECURITY_MANAGER; +import static org.apache.geode.security.SecurityManager.PASSWORD; +import static org.apache.geode.security.SecurityManager.USER_NAME; +import static org.apache.geode.security.TestIntegratedSecurityService.FAIL_THROWABLE; +import static org.apache.geode.security.TestIntegratedSecurityService.FAIL_TIMES; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Properties; +import java.util.concurrent.atomic.AtomicInteger; + +import org.apache.shiro.UnavailableSecurityManagerException; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import org.apache.geode.cache.Region; +import org.apache.geode.cache.RegionShortcut; +import org.apache.geode.cache.client.ClientCache; +import org.apache.geode.cache.client.ClientRegionShortcut; +import org.apache.geode.internal.cache.InternalCache; +import org.apache.geode.test.dunit.rules.ClientVM; +import org.apache.geode.test.dunit.rules.ClusterStartupRule; +import org.apache.geode.test.dunit.rules.MemberVM; +import org.apache.geode.test.junit.categories.SecurityTest; +import org.apache.geode.test.junit.rules.ClientCacheRule; +import org.apache.geode.test.junit.rules.VMProvider; + +@Category({SecurityTest.class}) +public class SecurityManagerAvailabilityDUnitTest { + @Rule + public ClusterStartupRule clusterStartupRule = new ClusterStartupRule(); + + @Rule + public ClientCacheRule clientCacheRule = new ClientCacheRule(); + + private MemberVM serverVM0; + private MemberVM serverVM1; + private ClientVM clientVM; + + private static final AtomicInteger ZE_COUNT = new AtomicInteger(); + + @Before + public void setup() throws Exception { + MemberVM locatorVM = + clusterStartupRule.startLocatorVM(0, + l -> l.withSecurityManager(ExpirableSecurityManager.class) + .withSystemProperty("org.apache.geode.internal.security.SecurityServiceFactory", + TestSecurityServiceFactory.class.getName())); + int locatorPort = locatorVM.getPort(); + + Properties serverProperties = new Properties(); + serverProperties.setProperty(SECURITY_MANAGER, ExpirableSecurityManager.class.getName()); + serverProperties.setProperty(USER_NAME, "test"); + serverProperties.setProperty(PASSWORD, "test"); Review comment: These properties are never used in the test, so they can be removed. ########## File path: geode-core/src/distributedTest/java/org/apache/geode/security/TestIntegratedSecurityService.java ########## @@ -0,0 +1,101 @@ +/* + * 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; + +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationTargetException; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import org.apache.logging.log4j.Logger; +import org.apache.shiro.subject.Subject; + +import org.apache.geode.internal.classloader.ClassPathLoader; +import org.apache.geode.internal.security.IntegratedSecurityService; +import org.apache.geode.internal.security.shiro.SecurityManagerProvider; +import org.apache.geode.logging.internal.log4j.api.LogService; + +/** + * IntegratedSecurityService extension that allows for manipulation of behavior for testing + * handling of rare occurrences of responses of the security service in a more predictable way. + */ +public class TestIntegratedSecurityService extends IntegratedSecurityService { + private static final Logger logger = LogService.getLogger(); + + public static final String FAIL_INTERVAL = "fail_interval"; + public static final String CALL_COUNT = "call_count"; Review comment: These constants are never used outside of this class, so are they necessary? ########## File path: geode-core/src/distributedTest/java/org/apache/geode/security/SecurityManagerAvailabilityDUnitTest.java ########## @@ -0,0 +1,164 @@ +/* + * 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; + + +import static org.apache.geode.distributed.ConfigurationProperties.SECURITY_CLIENT_AUTH_INIT; +import static org.apache.geode.distributed.ConfigurationProperties.SECURITY_MANAGER; +import static org.apache.geode.security.SecurityManager.PASSWORD; +import static org.apache.geode.security.SecurityManager.USER_NAME; +import static org.apache.geode.security.TestIntegratedSecurityService.FAIL_THROWABLE; +import static org.apache.geode.security.TestIntegratedSecurityService.FAIL_TIMES; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Properties; +import java.util.concurrent.atomic.AtomicInteger; + +import org.apache.shiro.UnavailableSecurityManagerException; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import org.apache.geode.cache.Region; +import org.apache.geode.cache.RegionShortcut; +import org.apache.geode.cache.client.ClientCache; +import org.apache.geode.cache.client.ClientRegionShortcut; +import org.apache.geode.internal.cache.InternalCache; +import org.apache.geode.test.dunit.rules.ClientVM; +import org.apache.geode.test.dunit.rules.ClusterStartupRule; +import org.apache.geode.test.dunit.rules.MemberVM; +import org.apache.geode.test.junit.categories.SecurityTest; +import org.apache.geode.test.junit.rules.ClientCacheRule; +import org.apache.geode.test.junit.rules.VMProvider; + +@Category({SecurityTest.class}) +public class SecurityManagerAvailabilityDUnitTest { + @Rule + public ClusterStartupRule clusterStartupRule = new ClusterStartupRule(); + + @Rule + public ClientCacheRule clientCacheRule = new ClientCacheRule(); + + private MemberVM serverVM0; + private MemberVM serverVM1; + private ClientVM clientVM; + + private static final AtomicInteger ZE_COUNT = new AtomicInteger(); + + @Before + public void setup() throws Exception { + MemberVM locatorVM = + clusterStartupRule.startLocatorVM(0, + l -> l.withSecurityManager(ExpirableSecurityManager.class) + .withSystemProperty("org.apache.geode.internal.security.SecurityServiceFactory", + TestSecurityServiceFactory.class.getName())); + int locatorPort = locatorVM.getPort(); + + Properties serverProperties = new Properties(); + serverProperties.setProperty(SECURITY_MANAGER, ExpirableSecurityManager.class.getName()); + serverProperties.setProperty(USER_NAME, "test"); + serverProperties.setProperty(PASSWORD, "test"); + + serverVM0 = clusterStartupRule.startServerVM(1, + s -> s.withSecurityManager(ExpirableSecurityManager.class) + .withCredential("test", "test") + .withConnectionToLocator(locatorPort) + .withSystemProperty("org.apache.geode.internal.security.SecurityServiceFactory", + TestSecurityServiceFactory.class.getName())); + serverVM1 = clusterStartupRule.startServerVM(2, + s -> s.withSecurityManager(ExpirableSecurityManager.class) + .withCredential("test", "test") + .withConnectionToLocator(locatorPort) + .withSystemProperty("org.apache.geode.internal.security.SecurityServiceFactory", + TestSecurityServiceFactory.class.getName())); + + VMProvider.invokeInEveryMember(() -> { + InternalCache cache = ClusterStartupRule.getCache(); + TestIntegratedSecurityService securityService = + (TestIntegratedSecurityService) cache.getSecurityService(); + securityService.setGetSubjectFailConditions(50, UnavailableSecurityManagerException.class); + cache.createRegionFactory(RegionShortcut.REPLICATE).create("region"); + }, serverVM0, serverVM1); + + int svm0 = serverVM0.getPort(); + int svm1 = serverVM1.getPort(); + clientVM = clusterStartupRule.startClientVM(3, + c -> c.withServerConnection(svm0, svm1) + .withPoolSubscription(true) + .withProperty(SECURITY_CLIENT_AUTH_INIT, UpdatableUserAuthInitialize.class.getName())); + } + + @Test + public void confirmSecurityManagerAvailabilityExceptionDoesNotStopClientPutOperations() { + Integer tries = 1000; + List<Object> clientResult = clientVM.invoke(() -> { + ClientCache clientCache = ClusterStartupRule.getClientCache(); + UpdatableUserAuthInitialize.setUser("data1"); + Region<Object, Object> region = + clientCache.createClientRegionFactory(ClientRegionShortcut.PROXY).create("region"); + + List<Object> returns = new ArrayList<>(2); + returns.add(new Exception("no exception")); + ZE_COUNT.set(0); + while (ZE_COUNT.get() < tries) { + try { + int key = ZE_COUNT.incrementAndGet(); Review comment: Is it necessary to use an AtomicInteger here? It seems like you could just have: ``` int count = 0; while (count < tries) { try { count++; ``` and then on line 135 have: ``` returns.add(count); ``` and remove the `ZE_COUNT` field from the class. ########## File path: geode-core/src/distributedTest/java/org/apache/geode/security/SecurityManagerAvailabilityDUnitTest.java ########## @@ -0,0 +1,164 @@ +/* + * 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; + + +import static org.apache.geode.distributed.ConfigurationProperties.SECURITY_CLIENT_AUTH_INIT; +import static org.apache.geode.distributed.ConfigurationProperties.SECURITY_MANAGER; +import static org.apache.geode.security.SecurityManager.PASSWORD; +import static org.apache.geode.security.SecurityManager.USER_NAME; +import static org.apache.geode.security.TestIntegratedSecurityService.FAIL_THROWABLE; +import static org.apache.geode.security.TestIntegratedSecurityService.FAIL_TIMES; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Properties; +import java.util.concurrent.atomic.AtomicInteger; + +import org.apache.shiro.UnavailableSecurityManagerException; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import org.apache.geode.cache.Region; +import org.apache.geode.cache.RegionShortcut; +import org.apache.geode.cache.client.ClientCache; +import org.apache.geode.cache.client.ClientRegionShortcut; +import org.apache.geode.internal.cache.InternalCache; +import org.apache.geode.test.dunit.rules.ClientVM; +import org.apache.geode.test.dunit.rules.ClusterStartupRule; +import org.apache.geode.test.dunit.rules.MemberVM; +import org.apache.geode.test.junit.categories.SecurityTest; +import org.apache.geode.test.junit.rules.ClientCacheRule; +import org.apache.geode.test.junit.rules.VMProvider; + +@Category({SecurityTest.class}) +public class SecurityManagerAvailabilityDUnitTest { + @Rule + public ClusterStartupRule clusterStartupRule = new ClusterStartupRule(); + + @Rule + public ClientCacheRule clientCacheRule = new ClientCacheRule(); + + private MemberVM serverVM0; + private MemberVM serverVM1; + private ClientVM clientVM; + + private static final AtomicInteger ZE_COUNT = new AtomicInteger(); + + @Before + public void setup() throws Exception { + MemberVM locatorVM = + clusterStartupRule.startLocatorVM(0, + l -> l.withSecurityManager(ExpirableSecurityManager.class) + .withSystemProperty("org.apache.geode.internal.security.SecurityServiceFactory", + TestSecurityServiceFactory.class.getName())); + int locatorPort = locatorVM.getPort(); + + Properties serverProperties = new Properties(); + serverProperties.setProperty(SECURITY_MANAGER, ExpirableSecurityManager.class.getName()); + serverProperties.setProperty(USER_NAME, "test"); + serverProperties.setProperty(PASSWORD, "test"); + + serverVM0 = clusterStartupRule.startServerVM(1, + s -> s.withSecurityManager(ExpirableSecurityManager.class) + .withCredential("test", "test") + .withConnectionToLocator(locatorPort) + .withSystemProperty("org.apache.geode.internal.security.SecurityServiceFactory", + TestSecurityServiceFactory.class.getName())); + serverVM1 = clusterStartupRule.startServerVM(2, + s -> s.withSecurityManager(ExpirableSecurityManager.class) + .withCredential("test", "test") + .withConnectionToLocator(locatorPort) + .withSystemProperty("org.apache.geode.internal.security.SecurityServiceFactory", + TestSecurityServiceFactory.class.getName())); + + VMProvider.invokeInEveryMember(() -> { + InternalCache cache = ClusterStartupRule.getCache(); + TestIntegratedSecurityService securityService = + (TestIntegratedSecurityService) cache.getSecurityService(); + securityService.setGetSubjectFailConditions(50, UnavailableSecurityManagerException.class); + cache.createRegionFactory(RegionShortcut.REPLICATE).create("region"); + }, serverVM0, serverVM1); + + int svm0 = serverVM0.getPort(); + int svm1 = serverVM1.getPort(); + clientVM = clusterStartupRule.startClientVM(3, + c -> c.withServerConnection(svm0, svm1) + .withPoolSubscription(true) + .withProperty(SECURITY_CLIENT_AUTH_INIT, UpdatableUserAuthInitialize.class.getName())); + } + + @Test + public void confirmSecurityManagerAvailabilityExceptionDoesNotStopClientPutOperations() { + Integer tries = 1000; Review comment: This can be an `int`. ########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java ########## @@ -1099,6 +1099,30 @@ public void lockDiskStore(String diskStoreName) { doLockDiskStore(diskStoreName); } + /** + * Having this method to return a SecurityServiceFactory allows users to replace the factory + * for testing purposes. + * + * In a test set the system property "org.apache.geode.internal.security.SecurityServiceFactory" + * to the class of choice ie. [Classname].class.getName() and in this class either implement + * SecurityServiceFactory or override the default implementation to create the desired behavior. Review comment: I'm really not a fan of the idea of introducing a system property solely to support testing. Is there any other approach that could be used here? ########## File path: geode-core/src/distributedTest/java/org/apache/geode/security/SecurityManagerAvailabilityDUnitTest.java ########## @@ -0,0 +1,164 @@ +/* + * 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; + + +import static org.apache.geode.distributed.ConfigurationProperties.SECURITY_CLIENT_AUTH_INIT; +import static org.apache.geode.distributed.ConfigurationProperties.SECURITY_MANAGER; +import static org.apache.geode.security.SecurityManager.PASSWORD; +import static org.apache.geode.security.SecurityManager.USER_NAME; +import static org.apache.geode.security.TestIntegratedSecurityService.FAIL_THROWABLE; +import static org.apache.geode.security.TestIntegratedSecurityService.FAIL_TIMES; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Properties; +import java.util.concurrent.atomic.AtomicInteger; + +import org.apache.shiro.UnavailableSecurityManagerException; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import org.apache.geode.cache.Region; +import org.apache.geode.cache.RegionShortcut; +import org.apache.geode.cache.client.ClientCache; +import org.apache.geode.cache.client.ClientRegionShortcut; +import org.apache.geode.internal.cache.InternalCache; +import org.apache.geode.test.dunit.rules.ClientVM; +import org.apache.geode.test.dunit.rules.ClusterStartupRule; +import org.apache.geode.test.dunit.rules.MemberVM; +import org.apache.geode.test.junit.categories.SecurityTest; +import org.apache.geode.test.junit.rules.ClientCacheRule; +import org.apache.geode.test.junit.rules.VMProvider; + +@Category({SecurityTest.class}) +public class SecurityManagerAvailabilityDUnitTest { + @Rule + public ClusterStartupRule clusterStartupRule = new ClusterStartupRule(); + + @Rule + public ClientCacheRule clientCacheRule = new ClientCacheRule(); + + private MemberVM serverVM0; + private MemberVM serverVM1; + private ClientVM clientVM; + + private static final AtomicInteger ZE_COUNT = new AtomicInteger(); + + @Before + public void setup() throws Exception { + MemberVM locatorVM = + clusterStartupRule.startLocatorVM(0, + l -> l.withSecurityManager(ExpirableSecurityManager.class) + .withSystemProperty("org.apache.geode.internal.security.SecurityServiceFactory", + TestSecurityServiceFactory.class.getName())); + int locatorPort = locatorVM.getPort(); + + Properties serverProperties = new Properties(); + serverProperties.setProperty(SECURITY_MANAGER, ExpirableSecurityManager.class.getName()); + serverProperties.setProperty(USER_NAME, "test"); + serverProperties.setProperty(PASSWORD, "test"); + + serverVM0 = clusterStartupRule.startServerVM(1, + s -> s.withSecurityManager(ExpirableSecurityManager.class) + .withCredential("test", "test") + .withConnectionToLocator(locatorPort) + .withSystemProperty("org.apache.geode.internal.security.SecurityServiceFactory", + TestSecurityServiceFactory.class.getName())); + serverVM1 = clusterStartupRule.startServerVM(2, + s -> s.withSecurityManager(ExpirableSecurityManager.class) + .withCredential("test", "test") + .withConnectionToLocator(locatorPort) + .withSystemProperty("org.apache.geode.internal.security.SecurityServiceFactory", + TestSecurityServiceFactory.class.getName())); + + VMProvider.invokeInEveryMember(() -> { + InternalCache cache = ClusterStartupRule.getCache(); + TestIntegratedSecurityService securityService = + (TestIntegratedSecurityService) cache.getSecurityService(); + securityService.setGetSubjectFailConditions(50, UnavailableSecurityManagerException.class); + cache.createRegionFactory(RegionShortcut.REPLICATE).create("region"); + }, serverVM0, serverVM1); + + int svm0 = serverVM0.getPort(); + int svm1 = serverVM1.getPort(); + clientVM = clusterStartupRule.startClientVM(3, + c -> c.withServerConnection(svm0, svm1) + .withPoolSubscription(true) + .withProperty(SECURITY_CLIENT_AUTH_INIT, UpdatableUserAuthInitialize.class.getName())); + } + + @Test + public void confirmSecurityManagerAvailabilityExceptionDoesNotStopClientPutOperations() { + Integer tries = 1000; + List<Object> clientResult = clientVM.invoke(() -> { + ClientCache clientCache = ClusterStartupRule.getClientCache(); + UpdatableUserAuthInitialize.setUser("data1"); + Region<Object, Object> region = + clientCache.createClientRegionFactory(ClientRegionShortcut.PROXY).create("region"); + + List<Object> returns = new ArrayList<>(2); + returns.add(new Exception("no exception")); + ZE_COUNT.set(0); + while (ZE_COUNT.get() < tries) { + try { + int key = ZE_COUNT.incrementAndGet(); + region.put(key, "value" + key); + } catch (Throwable e) { + Throwable cause = e.getCause(); + Throwable causeCause = cause != null ? cause.getCause() : null; + if (cause instanceof UnavailableSecurityManagerException + || causeCause instanceof UnavailableSecurityManagerException) { + returns.set(0, e); + break; + } + } + } + returns.add(ZE_COUNT.get()); + return returns; + }); + + Map<String, Object> resultMap0 = serverVM0.invoke(() -> { + InternalCache cache = ClusterStartupRule.getCache(); + TestIntegratedSecurityService securityService = + (TestIntegratedSecurityService) cache.getSecurityService(); + return securityService.getGetSubjectFailInformation(); + }); + + Map<String, Object> resultMap1 = serverVM1.invoke(() -> { + InternalCache cache = ClusterStartupRule.getCache(); + TestIntegratedSecurityService securityService = + (TestIntegratedSecurityService) cache.getSecurityService(); + return securityService.getGetSubjectFailInformation(); + }); + + assertThat((Integer) resultMap0.get(FAIL_TIMES)).isGreaterThan(0); + assertThat((String) resultMap0.get(FAIL_THROWABLE)).isNotEmpty(); + assertThat((Integer) resultMap1.get(FAIL_TIMES)).isGreaterThan(0); + assertThat((String) resultMap1.get(FAIL_THROWABLE)).isNotEmpty(); + + Throwable error = (Throwable) clientResult.get(0); + + assertThat(error instanceof Exception).isTrue(); + assertThat(error.getMessage()).isEqualTo("no exception"); Review comment: These might be better as: ``` assertThat(error).isInstanceOf(Exception.class); assertThat(error).hasMessage("no exception"); ``` Also, I think that the first assertion is always true, since the exception can either be the `Exception` set on line 119 or some other exception with `UnavailableSecurityManagerException` as a cause, set on line 130, so this assertion isn't really telling us much. When I reverted the fix and reran the test, this assertion did not fail (although the one on the message contents did). ########## File path: geode-core/src/distributedTest/java/org/apache/geode/security/TestIntegratedSecurityService.java ########## @@ -0,0 +1,101 @@ +/* + * 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; + +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationTargetException; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import org.apache.logging.log4j.Logger; +import org.apache.shiro.subject.Subject; + +import org.apache.geode.internal.classloader.ClassPathLoader; +import org.apache.geode.internal.security.IntegratedSecurityService; +import org.apache.geode.internal.security.shiro.SecurityManagerProvider; +import org.apache.geode.logging.internal.log4j.api.LogService; + +/** + * IntegratedSecurityService extension that allows for manipulation of behavior for testing + * handling of rare occurrences of responses of the security service in a more predictable way. + */ +public class TestIntegratedSecurityService extends IntegratedSecurityService { + private static final Logger logger = LogService.getLogger(); + + public static final String FAIL_INTERVAL = "fail_interval"; + public static final String CALL_COUNT = "call_count"; + public static final String FAIL_TIMES = "fail_times"; + public static final String FAIL_THROWABLE = "fail_throwable"; + + private final AtomicInteger getSubjectFailInterval = new AtomicInteger(0); + private final AtomicInteger getSubjectCallCount = new AtomicInteger(0); + private final AtomicInteger getSubjectTimesFailed = new AtomicInteger(0); + private final AtomicReference<String> getSubjectFailThrowable = new AtomicReference<>(""); + + TestIntegratedSecurityService(SecurityManagerProvider provider, PostProcessor postProcessor) { + super(provider, postProcessor); + } + + @Override + public Subject getSubject() { + Subject subject = super.getSubject(); + if (!getSubjectFailThrowable.get().isEmpty() + && getSubjectCallCount.get() > getSubjectFailInterval.get()) { + getSubjectTimesFailed.incrementAndGet(); + logger.info("about to throw induced exception on call count: {}", getSubjectCallCount.get()); + getSubjectCallCount.set(0); + try { + Class throwableClass = ClassPathLoader.getLatest().forName(getSubjectFailThrowable.get()); + Constructor constructor = throwableClass.getConstructor(String.class); + throw (RuntimeException) constructor.newInstance("forced exception for test"); Review comment: Warnings here can be fixed by using: ``` Class<? extends RuntimeException> throwableClass = uncheckedCast(ClassPathLoader.getLatest().forName(getSubjectFailThrowable.get())); Constructor<? extends RuntimeException> constructor = throwableClass.getConstructor(String.class); throw constructor.newInstance("forced exception for test"); ``` ########## File path: geode-core/src/distributedTest/java/org/apache/geode/security/SecurityManagerAvailabilityDUnitTest.java ########## @@ -0,0 +1,164 @@ +/* + * 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; + + +import static org.apache.geode.distributed.ConfigurationProperties.SECURITY_CLIENT_AUTH_INIT; +import static org.apache.geode.distributed.ConfigurationProperties.SECURITY_MANAGER; +import static org.apache.geode.security.SecurityManager.PASSWORD; +import static org.apache.geode.security.SecurityManager.USER_NAME; +import static org.apache.geode.security.TestIntegratedSecurityService.FAIL_THROWABLE; +import static org.apache.geode.security.TestIntegratedSecurityService.FAIL_TIMES; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Properties; +import java.util.concurrent.atomic.AtomicInteger; + +import org.apache.shiro.UnavailableSecurityManagerException; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import org.apache.geode.cache.Region; +import org.apache.geode.cache.RegionShortcut; +import org.apache.geode.cache.client.ClientCache; +import org.apache.geode.cache.client.ClientRegionShortcut; +import org.apache.geode.internal.cache.InternalCache; +import org.apache.geode.test.dunit.rules.ClientVM; +import org.apache.geode.test.dunit.rules.ClusterStartupRule; +import org.apache.geode.test.dunit.rules.MemberVM; +import org.apache.geode.test.junit.categories.SecurityTest; +import org.apache.geode.test.junit.rules.ClientCacheRule; +import org.apache.geode.test.junit.rules.VMProvider; + +@Category({SecurityTest.class}) +public class SecurityManagerAvailabilityDUnitTest { + @Rule + public ClusterStartupRule clusterStartupRule = new ClusterStartupRule(); + + @Rule + public ClientCacheRule clientCacheRule = new ClientCacheRule(); + + private MemberVM serverVM0; + private MemberVM serverVM1; + private ClientVM clientVM; + + private static final AtomicInteger ZE_COUNT = new AtomicInteger(); + + @Before + public void setup() throws Exception { + MemberVM locatorVM = + clusterStartupRule.startLocatorVM(0, + l -> l.withSecurityManager(ExpirableSecurityManager.class) + .withSystemProperty("org.apache.geode.internal.security.SecurityServiceFactory", + TestSecurityServiceFactory.class.getName())); + int locatorPort = locatorVM.getPort(); + + Properties serverProperties = new Properties(); + serverProperties.setProperty(SECURITY_MANAGER, ExpirableSecurityManager.class.getName()); + serverProperties.setProperty(USER_NAME, "test"); + serverProperties.setProperty(PASSWORD, "test"); + + serverVM0 = clusterStartupRule.startServerVM(1, + s -> s.withSecurityManager(ExpirableSecurityManager.class) + .withCredential("test", "test") + .withConnectionToLocator(locatorPort) + .withSystemProperty("org.apache.geode.internal.security.SecurityServiceFactory", + TestSecurityServiceFactory.class.getName())); + serverVM1 = clusterStartupRule.startServerVM(2, + s -> s.withSecurityManager(ExpirableSecurityManager.class) + .withCredential("test", "test") + .withConnectionToLocator(locatorPort) + .withSystemProperty("org.apache.geode.internal.security.SecurityServiceFactory", + TestSecurityServiceFactory.class.getName())); + + VMProvider.invokeInEveryMember(() -> { + InternalCache cache = ClusterStartupRule.getCache(); + TestIntegratedSecurityService securityService = + (TestIntegratedSecurityService) cache.getSecurityService(); + securityService.setGetSubjectFailConditions(50, UnavailableSecurityManagerException.class); + cache.createRegionFactory(RegionShortcut.REPLICATE).create("region"); + }, serverVM0, serverVM1); + + int svm0 = serverVM0.getPort(); + int svm1 = serverVM1.getPort(); Review comment: Could these be renamed to something more descriptive like "serverVM0Port" and "serverVM1Port"? ########## File path: geode-core/src/distributedTest/java/org/apache/geode/security/SecurityManagerAvailabilityDUnitTest.java ########## @@ -0,0 +1,164 @@ +/* + * 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; + + +import static org.apache.geode.distributed.ConfigurationProperties.SECURITY_CLIENT_AUTH_INIT; +import static org.apache.geode.distributed.ConfigurationProperties.SECURITY_MANAGER; +import static org.apache.geode.security.SecurityManager.PASSWORD; +import static org.apache.geode.security.SecurityManager.USER_NAME; +import static org.apache.geode.security.TestIntegratedSecurityService.FAIL_THROWABLE; +import static org.apache.geode.security.TestIntegratedSecurityService.FAIL_TIMES; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Properties; +import java.util.concurrent.atomic.AtomicInteger; + +import org.apache.shiro.UnavailableSecurityManagerException; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import org.apache.geode.cache.Region; +import org.apache.geode.cache.RegionShortcut; +import org.apache.geode.cache.client.ClientCache; +import org.apache.geode.cache.client.ClientRegionShortcut; +import org.apache.geode.internal.cache.InternalCache; +import org.apache.geode.test.dunit.rules.ClientVM; +import org.apache.geode.test.dunit.rules.ClusterStartupRule; +import org.apache.geode.test.dunit.rules.MemberVM; +import org.apache.geode.test.junit.categories.SecurityTest; +import org.apache.geode.test.junit.rules.ClientCacheRule; +import org.apache.geode.test.junit.rules.VMProvider; + +@Category({SecurityTest.class}) +public class SecurityManagerAvailabilityDUnitTest { + @Rule + public ClusterStartupRule clusterStartupRule = new ClusterStartupRule(); + + @Rule + public ClientCacheRule clientCacheRule = new ClientCacheRule(); + + private MemberVM serverVM0; + private MemberVM serverVM1; + private ClientVM clientVM; + + private static final AtomicInteger ZE_COUNT = new AtomicInteger(); + + @Before + public void setup() throws Exception { + MemberVM locatorVM = + clusterStartupRule.startLocatorVM(0, + l -> l.withSecurityManager(ExpirableSecurityManager.class) + .withSystemProperty("org.apache.geode.internal.security.SecurityServiceFactory", + TestSecurityServiceFactory.class.getName())); + int locatorPort = locatorVM.getPort(); + + Properties serverProperties = new Properties(); + serverProperties.setProperty(SECURITY_MANAGER, ExpirableSecurityManager.class.getName()); + serverProperties.setProperty(USER_NAME, "test"); + serverProperties.setProperty(PASSWORD, "test"); + + serverVM0 = clusterStartupRule.startServerVM(1, + s -> s.withSecurityManager(ExpirableSecurityManager.class) + .withCredential("test", "test") + .withConnectionToLocator(locatorPort) + .withSystemProperty("org.apache.geode.internal.security.SecurityServiceFactory", + TestSecurityServiceFactory.class.getName())); + serverVM1 = clusterStartupRule.startServerVM(2, + s -> s.withSecurityManager(ExpirableSecurityManager.class) + .withCredential("test", "test") + .withConnectionToLocator(locatorPort) + .withSystemProperty("org.apache.geode.internal.security.SecurityServiceFactory", + TestSecurityServiceFactory.class.getName())); + + VMProvider.invokeInEveryMember(() -> { + InternalCache cache = ClusterStartupRule.getCache(); + TestIntegratedSecurityService securityService = + (TestIntegratedSecurityService) cache.getSecurityService(); + securityService.setGetSubjectFailConditions(50, UnavailableSecurityManagerException.class); + cache.createRegionFactory(RegionShortcut.REPLICATE).create("region"); + }, serverVM0, serverVM1); + + int svm0 = serverVM0.getPort(); + int svm1 = serverVM1.getPort(); + clientVM = clusterStartupRule.startClientVM(3, + c -> c.withServerConnection(svm0, svm1) + .withPoolSubscription(true) + .withProperty(SECURITY_CLIENT_AUTH_INIT, UpdatableUserAuthInitialize.class.getName())); + } + + @Test + public void confirmSecurityManagerAvailabilityExceptionDoesNotStopClientPutOperations() { + Integer tries = 1000; + List<Object> clientResult = clientVM.invoke(() -> { + ClientCache clientCache = ClusterStartupRule.getClientCache(); + UpdatableUserAuthInitialize.setUser("data1"); + Region<Object, Object> region = + clientCache.createClientRegionFactory(ClientRegionShortcut.PROXY).create("region"); + + List<Object> returns = new ArrayList<>(2); + returns.add(new Exception("no exception")); + ZE_COUNT.set(0); + while (ZE_COUNT.get() < tries) { + try { + int key = ZE_COUNT.incrementAndGet(); + region.put(key, "value" + key); + } catch (Throwable e) { + Throwable cause = e.getCause(); + Throwable causeCause = cause != null ? cause.getCause() : null; + if (cause instanceof UnavailableSecurityManagerException + || causeCause instanceof UnavailableSecurityManagerException) { + returns.set(0, e); + break; + } Review comment: Currently this catch block will ignore any exception other than `UnavailableSecurityManagerException`. It would be best to add: ``` } else { throw e; } ``` so that if some unexpected exception is thrown from the call to `region.put()` then we know about it. -- 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]
