You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/09/15 16:56:17 UTC
[GitHub] [geode] kirklund commented on a change in pull request #6865: GEODE-9458: Enhance function execution testing auth expiry
kirklund commented on a change in pull request #6865:
URL: https://github.com/apache/geode/pull/6865#discussion_r709355045
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationFunctionDUnitTest.java
##########
@@ -65,77 +76,327 @@
return Arrays.asList(CURRENT_VERSION, RELEASE_VERSION);
}
- private MemberVM serverVM;
- private ClientVM clientVM;
+ private MemberVM serverVM0;
+ private MemberVM serverVM1;
+ private MemberVM serverVM2;
@Rule
- public ClusterStartupRule lsRule = new ClusterStartupRule();
+ public ClusterStartupRule lsRule = new ClusterStartupRule(4);
+
+ @Rule
+ public ClientCacheRule clientCacheRule = new ClientCacheRule();
@Before
- public void setup() throws Exception {
- Properties properties = new Properties();
- properties.setProperty(SECURITY_MANAGER, ExpirableSecurityManager.class.getName());
- properties.setProperty(ConfigurationProperties.SERIALIZABLE_OBJECT_FILTER,
+ public void setup() {
+ MemberVM locatorVM =
+ lsRule.startLocatorVM(0, l -> l.withSecurityManager(ExpirableSecurityManager.class));
+ int locatorPort = locatorVM.getPort();
+
+ Properties serverProperties = new Properties();
+ serverProperties.setProperty(SECURITY_MANAGER, ExpirableSecurityManager.class.getName());
+ serverProperties.setProperty(ConfigurationProperties.SERIALIZABLE_OBJECT_FILTER,
Review comment:
I would go ahead and `import static` the `SERIALIZABLE_OBJECT_FILTER` to match `SECURITY_MANAGER`. I generally use `import static` as much as possible for any statics that read better using it and only skip doing this for anything that reads more clearly without it.
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationFunctionDUnitTest.java
##########
@@ -65,77 +76,327 @@
return Arrays.asList(CURRENT_VERSION, RELEASE_VERSION);
}
- private MemberVM serverVM;
- private ClientVM clientVM;
+ private MemberVM serverVM0;
+ private MemberVM serverVM1;
+ private MemberVM serverVM2;
@Rule
- public ClusterStartupRule lsRule = new ClusterStartupRule();
+ public ClusterStartupRule lsRule = new ClusterStartupRule(4);
+
+ @Rule
+ public ClientCacheRule clientCacheRule = new ClientCacheRule();
@Before
- public void setup() throws Exception {
- Properties properties = new Properties();
- properties.setProperty(SECURITY_MANAGER, ExpirableSecurityManager.class.getName());
- properties.setProperty(ConfigurationProperties.SERIALIZABLE_OBJECT_FILTER,
+ public void setup() {
+ MemberVM locatorVM =
+ lsRule.startLocatorVM(0, l -> l.withSecurityManager(ExpirableSecurityManager.class));
+ int locatorPort = locatorVM.getPort();
+
+ Properties serverProperties = new Properties();
+ serverProperties.setProperty(SECURITY_MANAGER, ExpirableSecurityManager.class.getName());
+ serverProperties.setProperty(ConfigurationProperties.SERIALIZABLE_OBJECT_FILTER,
"org.apache.geode.management.internal.security.TestFunctions*");
- serverVM = lsRule.startServerVM(0, properties);
+ serverProperties.setProperty(GROUPS, "group");
+ serverProperties.setProperty(USER_NAME, "test");
+ serverProperties.setProperty(PASSWORD, "test");
- serverVM.invoke(() -> {
+ serverVM0 = lsRule.startServerVM(1, serverProperties, locatorPort);
+ serverVM1 = lsRule.startServerVM(2, serverProperties, locatorPort);
+ serverVM2 = lsRule.startServerVM(3, serverProperties, locatorPort);
+
+ VMProvider.invokeInEveryMember(() -> {
Objects.requireNonNull(ClusterStartupRule.getCache())
.createRegionFactory(RegionShortcut.REPLICATE).create("region");
- });
- int serverPort = serverVM.getPort();
- clientVM = lsRule.startClientVM(1, clientVersion, c1 -> c1
+ Objects.requireNonNull(ClusterStartupRule.getCache())
+ .createRegionFactory(RegionShortcut.PARTITION).create("partitionRegion");
+ }, serverVM0, serverVM1, serverVM2);
+
+ VMProvider.invokeInEveryMember(() -> writeFunction = new TestFunctions.WriteFunction(),
+ serverVM0, serverVM1, serverVM2);
+
+ clientCacheRule
.withProperty(SECURITY_CLIENT_AUTH_INIT, UpdatableUserAuthInitialize.class.getName())
.withPoolSubscription(true)
- .withServerConnection(serverPort));
+ .withLocatorConnection(locatorPort);
+ }
- VMProvider.invokeInEveryMember(() -> writeFunction = new TestFunctions.WriteFunction(),
- serverVM, clientVM);
+ @Test
+ public void clientShouldReAuthenticateWhenCredentialExpiredAndFunctionExecutionOnServerSucceed()
+ throws Exception {
+ ClientCache clientCache = clientCacheRule.createCache();
+ UpdatableUserAuthInitialize.setUser("data1");
+ writeFunction = new TestFunctions.WriteFunction();
+
+ ResultCollector rc = onServer(clientCache.getDefaultPool()).execute(writeFunction);
+ assertThat(((ArrayList) rc.getResult()).get(0))
+ .isEqualTo(TestFunctions.WriteFunction.SUCCESS_OUTPUT);
+
+ // expire the current user
+ VMProvider.invokeInEveryMember(() -> getSecurityManager().addExpiredUser("data1"),
+ serverVM0, serverVM1, serverVM2);
+
+ // do a second function execution, if this is successful, it means new credentials are provided
+ UpdatableUserAuthInitialize.setUser("data2");
+ rc = onServer(clientCache.getDefaultPool()).execute(writeFunction);
+ assertThat(((ArrayList) rc.getResult()).get(0))
+ .isEqualTo(TestFunctions.WriteFunction.SUCCESS_OUTPUT);
+
+ // all function invocation authorizations are recorded
+ List<Object> resultsVM0 = collectSecurityManagerResults(serverVM0);
+ List<Object> resultsVM1 = collectSecurityManagerResults(serverVM1);
+ List<Object> resultsVM2 = collectSecurityManagerResults(serverVM2);
+
+ Set<String> combinedExpiredUsers = combineExpiredUsers(resultsVM0, resultsVM1, resultsVM2);
+
+ assertThat(combinedExpiredUsers.size()).isEqualTo(1);
+ assertThat(combinedExpiredUsers.contains("data1")).isTrue();
+ Map<String, List<String>> authorizedOps = collectVMOps(1, resultsVM0, resultsVM1, resultsVM2);
+
+ assertThat(authorizedOps.get("data1")).asList().hasSize(1);
+ assertThat(authorizedOps.get("data1")).asList().containsExactly("DATA:WRITE");
+ assertThat(authorizedOps.get("data2")).asList().hasSize(1);
+ assertThat(authorizedOps.get("data2")).asList().containsExactly("DATA:WRITE");
+
+ Map<String, List<String>> unauthorizedOps = collectVMOps(2, resultsVM0, resultsVM1, resultsVM2);
+
+ assertThat(unauthorizedOps.get("data1")).asList().hasSize(1);
+ assertThat(unauthorizedOps.get("data1")).asList().containsExactly("DATA:WRITE");
}
@Test
- public void clientShouldReAuthenticateWhenCredentialExpiredAndFunctionExecutionSucceed() {
- clientVM.invoke(() -> {
- ClientCache clientCache = ClusterStartupRule.getClientCache();
- assertThat(clientCache).isNotNull();
- UpdatableUserAuthInitialize.setUser("data1");
- ResultCollector rc = onServer(clientCache.getDefaultPool()).execute(writeFunction);
- assertThat(((ArrayList) rc.getResult()).get(0))
- .isEqualTo(TestFunctions.WriteFunction.SUCCESS_OUTPUT);
- });
+ public void clientShouldReAuthenticateWhenCredentialExpiredAndFunctionExecutionOnServersSucceed()
+ throws Exception {
+ ClientCache clientCache = clientCacheRule.createCache();
+ UpdatableUserAuthInitialize.setUser("data1");
+ writeFunction = new TestFunctions.WriteFunction();
+
+ ResultCollector rc = onServers(clientCache.getDefaultPool()).execute(writeFunction);
+ assertThat(((ArrayList) rc.getResult()).get(0))
+ .isEqualTo(TestFunctions.WriteFunction.SUCCESS_OUTPUT);
// expire the current user
- serverVM.invoke(() -> getSecurityManager().addExpiredUser("data1"));
+ VMProvider.invokeInEveryMember(() -> getSecurityManager().addExpiredUser("data1"),
+ serverVM0, serverVM1, serverVM2);
// do a second function execution, if this is successful, it means new credentials are provided
- clientVM.invoke(() -> {
- ClientCache clientCache = ClusterStartupRule.getClientCache();
- assertThat(clientCache).isNotNull();
- UpdatableUserAuthInitialize.setUser("data2");
- ResultCollector rc = onServer(clientCache.getDefaultPool()).execute(writeFunction);
- assertThat(((ArrayList) rc.getResult()).get(0))
- .isEqualTo(TestFunctions.WriteFunction.SUCCESS_OUTPUT);
- });
+ UpdatableUserAuthInitialize.setUser("data2");
+ rc = onServers(clientCache.getDefaultPool()).execute(writeFunction);
+ assertThat(((ArrayList) rc.getResult()).get(0))
+ .isEqualTo(TestFunctions.WriteFunction.SUCCESS_OUTPUT);
+
+ // all function invocation authorizations are recorded
+ List<Object> resultsVM0 = collectSecurityManagerResults(serverVM0);
+ List<Object> resultsVM1 = collectSecurityManagerResults(serverVM1);
+ List<Object> resultsVM2 = collectSecurityManagerResults(serverVM2);
+
+ Set<String> combinedExpiredUsers = combineExpiredUsers(resultsVM0, resultsVM1, resultsVM2);
+
+ assertThat(combinedExpiredUsers.size()).isEqualTo(1);
+ assertThat(combinedExpiredUsers.contains("data1")).isTrue();
+ Map<String, List<String>> authorizedOps = collectVMOps(1, resultsVM0, resultsVM1, resultsVM2);
+
+ assertThat(authorizedOps.get("data1")).asList().hasSize(3);
+ assertThat(authorizedOps.get("data1")).asList().containsExactly("DATA:WRITE", "DATA:WRITE",
+ "DATA:WRITE");
+ assertThat(authorizedOps.get("data2")).asList().hasSize(3);
+ assertThat(authorizedOps.get("data2")).asList().containsExactly("DATA:WRITE", "DATA:WRITE",
+ "DATA:WRITE");
+
+ Map<String, List<String>> unauthorizedOps = collectVMOps(2, resultsVM0, resultsVM1, resultsVM2);
+
+ assertThat(unauthorizedOps.get("data1")).asList().hasSize(3);
+ assertThat(unauthorizedOps.get("data1")).asList().containsExactly("DATA:WRITE", "DATA:WRITE",
+ "DATA:WRITE");
+ }
+
+ @Test
+ public void clientShouldReAuthenticateWhenCredentialExpiredAndFunctionExecutionOnRegionSucceed()
+ throws Exception {
+ ClientCache clientCache = clientCacheRule.createCache();
+ UpdatableUserAuthInitialize.setUser("data1");
+ Region<Object, Object> region =
+ clientCache.createClientRegionFactory(ClientRegionShortcut.PROXY).create("region");
+ writeFunction = new TestFunctions.WriteFunction();
+
+ ResultCollector rc = onRegion(region).execute(writeFunction);
+ assertThat(((ArrayList) rc.getResult()).get(0))
+ .isEqualTo(TestFunctions.WriteFunction.SUCCESS_OUTPUT);
+
+ // expire the current user
+ VMProvider.invokeInEveryMember(() -> getSecurityManager().addExpiredUser("data1"),
+ serverVM0, serverVM1, serverVM2);
+
+ // do a second function execution, if this is successful, it means new credentials are provided
+ UpdatableUserAuthInitialize.setUser("data2");
+ rc = onRegion(region).execute(writeFunction);
+ assertThat(((ArrayList) rc.getResult()).get(0))
+ .isEqualTo(TestFunctions.WriteFunction.SUCCESS_OUTPUT);
+
+ // all function invocation authorizations are recorded
+ List<Object> resultsVM0 = collectSecurityManagerResults(serverVM0);
+ List<Object> resultsVM1 = collectSecurityManagerResults(serverVM1);
+ List<Object> resultsVM2 = collectSecurityManagerResults(serverVM2);
- // all put operation succeeded
- serverVM.invoke(() -> {
+ Set<String> combinedExpiredUsers = combineExpiredUsers(resultsVM0, resultsVM1, resultsVM2);
+
+ assertThat(combinedExpiredUsers.size()).isEqualTo(1);
+ assertThat(combinedExpiredUsers.contains("data1")).isTrue();
Review comment:
These two assertions are used in several places. It would be better if you use the `Collection` assertion everywhere instead of the `boolean` assertion:
```
assertThat(combinedExpiredUsers).hasSize(1);
assertThat(combinedExpiredUsers).contains("data1");
```
Or:
```
assertThat(combinedExpiredUsers)
.hasSize(1)
.contains("data1");
```
Or:
```
assertThat(combinedExpiredUsers).containsExactly("data1");
```
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationFunctionDUnitTest.java
##########
@@ -65,77 +76,327 @@
return Arrays.asList(CURRENT_VERSION, RELEASE_VERSION);
}
- private MemberVM serverVM;
- private ClientVM clientVM;
+ private MemberVM serverVM0;
+ private MemberVM serverVM1;
+ private MemberVM serverVM2;
@Rule
- public ClusterStartupRule lsRule = new ClusterStartupRule();
+ public ClusterStartupRule lsRule = new ClusterStartupRule(4);
Review comment:
Not sure what `ls` refers to in `lsRule`. Maybe rename it to `clusterRule`.
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationFunctionDUnitTest.java
##########
@@ -65,77 +76,327 @@
return Arrays.asList(CURRENT_VERSION, RELEASE_VERSION);
}
- private MemberVM serverVM;
- private ClientVM clientVM;
+ private MemberVM serverVM0;
+ private MemberVM serverVM1;
+ private MemberVM serverVM2;
@Rule
- public ClusterStartupRule lsRule = new ClusterStartupRule();
+ public ClusterStartupRule lsRule = new ClusterStartupRule(4);
+
+ @Rule
+ public ClientCacheRule clientCacheRule = new ClientCacheRule();
@Before
- public void setup() throws Exception {
- Properties properties = new Properties();
- properties.setProperty(SECURITY_MANAGER, ExpirableSecurityManager.class.getName());
- properties.setProperty(ConfigurationProperties.SERIALIZABLE_OBJECT_FILTER,
+ public void setup() {
+ MemberVM locatorVM =
+ lsRule.startLocatorVM(0, l -> l.withSecurityManager(ExpirableSecurityManager.class));
+ int locatorPort = locatorVM.getPort();
+
+ Properties serverProperties = new Properties();
+ serverProperties.setProperty(SECURITY_MANAGER, ExpirableSecurityManager.class.getName());
+ serverProperties.setProperty(ConfigurationProperties.SERIALIZABLE_OBJECT_FILTER,
"org.apache.geode.management.internal.security.TestFunctions*");
- serverVM = lsRule.startServerVM(0, properties);
+ serverProperties.setProperty(GROUPS, "group");
+ serverProperties.setProperty(USER_NAME, "test");
+ serverProperties.setProperty(PASSWORD, "test");
- serverVM.invoke(() -> {
+ serverVM0 = lsRule.startServerVM(1, serverProperties, locatorPort);
+ serverVM1 = lsRule.startServerVM(2, serverProperties, locatorPort);
+ serverVM2 = lsRule.startServerVM(3, serverProperties, locatorPort);
+
+ VMProvider.invokeInEveryMember(() -> {
Objects.requireNonNull(ClusterStartupRule.getCache())
.createRegionFactory(RegionShortcut.REPLICATE).create("region");
- });
- int serverPort = serverVM.getPort();
- clientVM = lsRule.startClientVM(1, clientVersion, c1 -> c1
+ Objects.requireNonNull(ClusterStartupRule.getCache())
+ .createRegionFactory(RegionShortcut.PARTITION).create("partitionRegion");
+ }, serverVM0, serverVM1, serverVM2);
+
+ VMProvider.invokeInEveryMember(() -> writeFunction = new TestFunctions.WriteFunction(),
+ serverVM0, serverVM1, serverVM2);
+
+ clientCacheRule
.withProperty(SECURITY_CLIENT_AUTH_INIT, UpdatableUserAuthInitialize.class.getName())
.withPoolSubscription(true)
- .withServerConnection(serverPort));
+ .withLocatorConnection(locatorPort);
+ }
- VMProvider.invokeInEveryMember(() -> writeFunction = new TestFunctions.WriteFunction(),
- serverVM, clientVM);
+ @Test
+ public void clientShouldReAuthenticateWhenCredentialExpiredAndFunctionExecutionOnServerSucceed()
+ throws Exception {
+ ClientCache clientCache = clientCacheRule.createCache();
+ UpdatableUserAuthInitialize.setUser("data1");
+ writeFunction = new TestFunctions.WriteFunction();
+
+ ResultCollector rc = onServer(clientCache.getDefaultPool()).execute(writeFunction);
+ assertThat(((ArrayList) rc.getResult()).get(0))
+ .isEqualTo(TestFunctions.WriteFunction.SUCCESS_OUTPUT);
Review comment:
You should always try to cast to an interface rather than a concrete implementation:
```
List<?> result = (List) rc.getResult();
assertThat(result.get(0))
.isEqualTo(TestFunctions.WriteFunction.SUCCESS_OUTPUT);
```
--
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