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/08/10 17:36:08 UTC
[GitHub] [geode] jmelchio opened a new pull request #6755: GEODE-9460: Add testing for mutli-user scenarios
jmelchio opened a new pull request #6755:
URL: https://github.com/apache/geode/pull/6755
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
--
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
[GitHub] [geode] jinmeiliao commented on a change in pull request #6755: GEODE-9460: Add testing for mutli-user scenarios
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #6755:
URL: https://github.com/apache/geode/pull/6755#discussion_r686362051
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationDUnitTest.java
##########
@@ -109,4 +116,92 @@ public void clientShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed()
assertThat(region.size()).isEqualTo(2);
}
+ @Test
+ public void userShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed() throws Exception {
+ int serverPort = server.getPort();
+ ClientVM clientVM = lsRule.startClientVM(0, clientVersion,
+ c -> c.withMultiUser(true)
+ .withProperty(SECURITY_CLIENT_AUTH_INIT, UpdatableUserAuthInitialize.class.getName())
+ .withPoolSubscription(true)
+ .withServerConnection(serverPort));
+
+ clientVM.invoke(() -> {
+ UpdatableUserAuthInitialize.setUser("user1");
+ ClientCache clientCache = ClusterStartupRule.getClientCache();
+ assert clientCache != null;
+ clientCache.createClientRegionFactory(ClientRegionShortcut.PROXY).create("region");
+ Properties userSecurityProperties = new Properties();
+ userSecurityProperties.put("security-username", "serviceUser0");
+ userSecurityProperties.put("security-password", "serviceUser0");
+ RegionService regionService = clientCache.createAuthenticatedView(userSecurityProperties);
+ Region<Object, Object> region = regionService.getRegion("/region");
+ region.put(0, "value0");
+ });
+
+ clientVM.invoke(() -> {
+ ClientCache clientCache = ClusterStartupRule.getClientCache();
+ Properties userSecurityProperties = new Properties();
+ userSecurityProperties.put("security-username", "serviceUser1");
+ userSecurityProperties.put("security-password", "serviceUser1");
+ assert clientCache != null;
+ RegionService regionService = clientCache.createAuthenticatedView(userSecurityProperties);
+ Region<Object, Object> region = regionService.getRegion("/region");
+ region.put(1, "value1");
+ regionService.close();
Review comment:
I would think user would continue to use same regionService and expect the next put to succeed. (the whole expire user and re-authenticate happens behind the scene and the client is doing operations like nothing happened).
--
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
[GitHub] [geode] jinmeiliao merged pull request #6755: GEODE-9460: Add testing for mutli-user scenarios
Posted by GitBox <gi...@apache.org>.
jinmeiliao merged pull request #6755:
URL: https://github.com/apache/geode/pull/6755
--
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
[GitHub] [geode] kirklund commented on a change in pull request #6755: GEODE-9460: Add testing for mutli-user scenarios
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6755:
URL: https://github.com/apache/geode/pull/6755#discussion_r688171662
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationDUnitTest.java
##########
@@ -85,9 +86,10 @@ public void clientShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed()
clientVM.invoke(() -> {
ClientCache clientCache = ClusterStartupRule.getClientCache();
UpdatableUserAuthInitialize.setUser("user1");
- ClientRegionFactory clientRegionFactory =
+ assert clientCache != null;
Review comment:
Please use actual Assertions in tests instead of Java asserts.
--
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
[GitHub] [geode] jmelchio commented on a change in pull request #6755: GEODE-9460: Add testing for mutli-user scenarios
Posted by GitBox <gi...@apache.org>.
jmelchio commented on a change in pull request #6755:
URL: https://github.com/apache/geode/pull/6755#discussion_r689540682
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationDUnitTest.java
##########
@@ -85,9 +86,10 @@ public void clientShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed()
clientVM.invoke(() -> {
ClientCache clientCache = ClusterStartupRule.getClientCache();
UpdatableUserAuthInitialize.setUser("user1");
- ClientRegionFactory clientRegionFactory =
+ assert clientCache != null;
Review comment:
The reason for the java assert is not to test something but to get rid of a warning in the gutter. I assume that this is a valid use of java asserts?
--
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
[GitHub] [geode] mhansonp commented on a change in pull request #6755: GEODE-9460: Add testing for mutli-user scenarios
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6755:
URL: https://github.com/apache/geode/pull/6755#discussion_r687953303
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationDUnitTest.java
##########
@@ -109,4 +114,82 @@ public void clientShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed()
assertThat(region.size()).isEqualTo(2);
}
+ @Test
+ public void userShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed() throws Exception {
+ int serverPort = server.getPort();
+ ClientVM clientVM = lsRule.startClientVM(0, clientVersion,
+ c -> c.withMultiUser(true)
+ .withProperty(SECURITY_CLIENT_AUTH_INIT, UpdatableUserAuthInitialize.class.getName())
+ .withPoolSubscription(true)
+ .withServerConnection(serverPort));
+
+ clientVM.invoke(() -> {
+ UpdatableUserAuthInitialize.setUser("serviceUser0");
+ UpdatableUserAuthInitialize.setAttempts(0);
+ UpdatableUserAuthInitialize.setAlternate(false);
+ ClientCache clientCache = ClusterStartupRule.getClientCache();
+ assert clientCache != null;
+ clientCache.createClientRegionFactory(ClientRegionShortcut.PROXY).create("region");
+ Properties userSecurityProperties = new Properties();
+ userSecurityProperties.put("security-username", "serviceUser0");
+ userSecurityProperties.put("security-password", "serviceUser0");
+ RegionService regionService = clientCache.createAuthenticatedView(userSecurityProperties);
+ Region<Object, Object> region = regionService.getRegion("/region");
+ region.put(0, "value0");
+ regionService.close();
+ });
+
Review comment:
Why are splitting this blocks for the same VM?
--
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
[GitHub] [geode] mhansonp commented on a change in pull request #6755: GEODE-9460: Add testing for mutli-user scenarios
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6755:
URL: https://github.com/apache/geode/pull/6755#discussion_r692552257
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationDUnitTest.java
##########
@@ -85,9 +86,10 @@ public void clientShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed()
clientVM.invoke(() -> {
ClientCache clientCache = ClusterStartupRule.getClientCache();
UpdatableUserAuthInitialize.setUser("user1");
- ClientRegionFactory clientRegionFactory =
+ assert clientCache != null;
Review comment:
I'm of the mindset that an assertThat(clientCache).isNotNull(); would be better than what is there. That is pretty much the standard in tests. Whether it is to clear a warning or not.
--
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
[GitHub] [geode] jmelchio commented on a change in pull request #6755: GEODE-9460: Add testing for mutli-user scenarios
Posted by GitBox <gi...@apache.org>.
jmelchio commented on a change in pull request #6755:
URL: https://github.com/apache/geode/pull/6755#discussion_r686874410
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationDUnitTest.java
##########
@@ -109,4 +116,92 @@ public void clientShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed()
assertThat(region.size()).isEqualTo(2);
}
+ @Test
+ public void userShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed() throws Exception {
+ int serverPort = server.getPort();
+ ClientVM clientVM = lsRule.startClientVM(0, clientVersion,
+ c -> c.withMultiUser(true)
+ .withProperty(SECURITY_CLIENT_AUTH_INIT, UpdatableUserAuthInitialize.class.getName())
+ .withPoolSubscription(true)
+ .withServerConnection(serverPort));
+
+ clientVM.invoke(() -> {
+ UpdatableUserAuthInitialize.setUser("user1");
+ ClientCache clientCache = ClusterStartupRule.getClientCache();
+ assert clientCache != null;
+ clientCache.createClientRegionFactory(ClientRegionShortcut.PROXY).create("region");
+ Properties userSecurityProperties = new Properties();
+ userSecurityProperties.put("security-username", "serviceUser0");
+ userSecurityProperties.put("security-password", "serviceUser0");
+ RegionService regionService = clientCache.createAuthenticatedView(userSecurityProperties);
+ Region<Object, Object> region = regionService.getRegion("/region");
+ region.put(0, "value0");
+ });
+
+ clientVM.invoke(() -> {
+ ClientCache clientCache = ClusterStartupRule.getClientCache();
+ Properties userSecurityProperties = new Properties();
+ userSecurityProperties.put("security-username", "serviceUser1");
+ userSecurityProperties.put("security-password", "serviceUser1");
+ assert clientCache != null;
+ RegionService regionService = clientCache.createAuthenticatedView(userSecurityProperties);
+ Region<Object, Object> region = regionService.getRegion("/region");
+ region.put(1, "value1");
+ regionService.close();
Review comment:
I agree. I'll see if I can find reuse the same `regionService` from one `clientVM` invocation to another. If you have ideas on how to accomplish this I'm open to suggestions.
--
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
[GitHub] [geode] mhansonp commented on a change in pull request #6755: GEODE-9460: Add testing for mutli-user scenarios
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6755:
URL: https://github.com/apache/geode/pull/6755#discussion_r692552789
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationDUnitTest.java
##########
@@ -109,4 +114,82 @@ public void clientShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed()
assertThat(region.size()).isEqualTo(2);
}
+ @Test
+ public void userShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed() throws Exception {
+ int serverPort = server.getPort();
+ ClientVM clientVM = lsRule.startClientVM(0, clientVersion,
+ c -> c.withMultiUser(true)
+ .withProperty(SECURITY_CLIENT_AUTH_INIT, UpdatableUserAuthInitialize.class.getName())
+ .withPoolSubscription(true)
+ .withServerConnection(serverPort));
+
+ clientVM.invoke(() -> {
+ UpdatableUserAuthInitialize.setUser("serviceUser0");
+ UpdatableUserAuthInitialize.setAttempts(0);
+ UpdatableUserAuthInitialize.setAlternate(false);
+ ClientCache clientCache = ClusterStartupRule.getClientCache();
+ assert clientCache != null;
+ clientCache.createClientRegionFactory(ClientRegionShortcut.PROXY).create("region");
+ Properties userSecurityProperties = new Properties();
+ userSecurityProperties.put("security-username", "serviceUser0");
+ userSecurityProperties.put("security-password", "serviceUser0");
+ RegionService regionService = clientCache.createAuthenticatedView(userSecurityProperties);
+ Region<Object, Object> region = regionService.getRegion("/region");
+ region.put(0, "value0");
+ regionService.close();
+ });
+
Review comment:
One note that is most likely irrelevant in this case is that splitting it takes more time for context switching.
--
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
[GitHub] [geode] jmelchio commented on a change in pull request #6755: GEODE-9460: Add testing for mutli-user scenarios
Posted by GitBox <gi...@apache.org>.
jmelchio commented on a change in pull request #6755:
URL: https://github.com/apache/geode/pull/6755#discussion_r688057095
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationDUnitTest.java
##########
@@ -109,4 +114,82 @@ public void clientShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed()
assertThat(region.size()).isEqualTo(2);
}
+ @Test
+ public void userShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed() throws Exception {
+ int serverPort = server.getPort();
+ ClientVM clientVM = lsRule.startClientVM(0, clientVersion,
+ c -> c.withMultiUser(true)
+ .withProperty(SECURITY_CLIENT_AUTH_INIT, UpdatableUserAuthInitialize.class.getName())
+ .withPoolSubscription(true)
+ .withServerConnection(serverPort));
+
+ clientVM.invoke(() -> {
+ UpdatableUserAuthInitialize.setUser("serviceUser0");
+ UpdatableUserAuthInitialize.setAttempts(0);
+ UpdatableUserAuthInitialize.setAlternate(false);
+ ClientCache clientCache = ClusterStartupRule.getClientCache();
+ assert clientCache != null;
+ clientCache.createClientRegionFactory(ClientRegionShortcut.PROXY).create("region");
+ Properties userSecurityProperties = new Properties();
+ userSecurityProperties.put("security-username", "serviceUser0");
+ userSecurityProperties.put("security-password", "serviceUser0");
+ RegionService regionService = clientCache.createAuthenticatedView(userSecurityProperties);
+ Region<Object, Object> region = regionService.getRegion("/region");
+ region.put(0, "value0");
+ regionService.close();
+ });
+
Review comment:
> I am confused about the repetition of the `clientvm blocks rather than continuing it.
You're right, in most cases they can be continued. I organized it this way as logical steps but unless we add a user to the expired user list in between there is no need for separation of the blocks. I will change 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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #6755: GEODE-9460: Add testing for mutli-user scenarios
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6755:
URL: https://github.com/apache/geode/pull/6755#discussion_r692552328
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationDUnitTest.java
##########
@@ -98,7 +103,8 @@ public void clientShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed()
clientVM.invoke(() -> {
UpdatableUserAuthInitialize.setUser("user2");
ClientCache clientCache = ClusterStartupRule.getClientCache();
- Region region = clientCache.getRegion("region");
+ assert clientCache != null;
Review comment:
same here and elsewhere...
--
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
[GitHub] [geode] jmelchio commented on a change in pull request #6755: GEODE-9460: Add testing for mutli-user scenarios
Posted by GitBox <gi...@apache.org>.
jmelchio commented on a change in pull request #6755:
URL: https://github.com/apache/geode/pull/6755#discussion_r686970570
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationDUnitTest.java
##########
@@ -109,4 +116,92 @@ public void clientShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed()
assertThat(region.size()).isEqualTo(2);
}
+ @Test
+ public void userShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed() throws Exception {
+ int serverPort = server.getPort();
+ ClientVM clientVM = lsRule.startClientVM(0, clientVersion,
+ c -> c.withMultiUser(true)
+ .withProperty(SECURITY_CLIENT_AUTH_INIT, UpdatableUserAuthInitialize.class.getName())
+ .withPoolSubscription(true)
+ .withServerConnection(serverPort));
+
+ clientVM.invoke(() -> {
+ UpdatableUserAuthInitialize.setUser("user1");
+ ClientCache clientCache = ClusterStartupRule.getClientCache();
+ assert clientCache != null;
+ clientCache.createClientRegionFactory(ClientRegionShortcut.PROXY).create("region");
+ Properties userSecurityProperties = new Properties();
+ userSecurityProperties.put("security-username", "serviceUser0");
+ userSecurityProperties.put("security-password", "serviceUser0");
+ RegionService regionService = clientCache.createAuthenticatedView(userSecurityProperties);
+ Region<Object, Object> region = regionService.getRegion("/region");
+ region.put(0, "value0");
+ });
+
+ clientVM.invoke(() -> {
+ ClientCache clientCache = ClusterStartupRule.getClientCache();
+ Properties userSecurityProperties = new Properties();
+ userSecurityProperties.put("security-username", "serviceUser1");
+ userSecurityProperties.put("security-password", "serviceUser1");
+ assert clientCache != null;
+ RegionService regionService = clientCache.createAuthenticatedView(userSecurityProperties);
+ Region<Object, Object> region = regionService.getRegion("/region");
+ region.put(1, "value1");
+ regionService.close();
Review comment:
I was unable to reuse the `regionService` but came up with a way accomplish the automatic re-authentication behind the scenes with changes to the `AuthInitialize` implementation we're using for the tests.
--
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
[GitHub] [geode] mhansonp commented on a change in pull request #6755: GEODE-9460: Add testing for mutli-user scenarios
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6755:
URL: https://github.com/apache/geode/pull/6755#discussion_r692552328
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationDUnitTest.java
##########
@@ -98,7 +103,8 @@ public void clientShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed()
clientVM.invoke(() -> {
UpdatableUserAuthInitialize.setUser("user2");
ClientCache clientCache = ClusterStartupRule.getClientCache();
- Region region = clientCache.getRegion("region");
+ assert clientCache != null;
Review comment:
same here.
--
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