Skip to content

GEODE-9460: Add testing for mutli-user scenarios #6755

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jinmeiliao merged 6 commits into from
Aug 20, 2021
Merged

GEODE-9460: Add testing for mutli-user scenarios #6755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f9eca96
Add testing for mutli-user scenarios
jmelchio Aug 10, 2021
3a016aa
Changes based on PR feedback
jmelchio Aug 11, 2021
bea63ec
Update code based on PR review feedback on invoke blocks
jmelchio Aug 12, 2021
a6d7ed3
Fix spotless error in build
jmelchio Aug 16, 2021
ddcb8a8
Simplify tests
jmelchio Aug 19, 2021
ef2d1c7
Fix test failures
jmelchio Aug 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 66 additions & 6 deletions geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationDUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,14 @@
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.test.version.VersionManager.CURRENT_VERSION;
import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Properties;

import org.junit.After;
import org.junit.Rule;
Expand All @@ -31,6 +33,7 @@
import org.junit.runners.Parameterized;

import org.apache.geode.cache.Region;
import org.apache.geode.cache.RegionService;
import org.apache.geode.cache.RegionShortcut;
import org.apache.geode.cache.client.ClientCache;
import org.apache.geode.cache.client.ClientRegionFactory;
Expand All @@ -45,6 +48,8 @@
@RunWith(Parameterized.class)
@Parameterized.UseParametersRunnerFactory(CategoryWithParameterizedRunnerFactory.class)
public class AuthExpirationDUnitTest {
static RegionService regionService0;
static RegionService regionService1;

@Parameterized.Parameter
public String clientVersion;
Expand All @@ -58,13 +63,12 @@ public static Collection<String> data() {
@Rule
public ClusterStartupRule lsRule = new ClusterStartupRule();


@Rule
public RestoreSystemProperties restore = new RestoreSystemProperties();

@Rule
public ServerStarterRule server = new ServerStarterRule()
.withProperty(SECURITY_MANAGER, ExpirableSecurityManager.class.getName())
.withSecurityManager(ExpirableSecurityManager.class)
.withRegion(RegionShortcut.REPLICATE, "region");

@After
Expand All @@ -85,9 +89,10 @@ public void clientShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed()
clientVM.invoke(() -> {
ClientCache clientCache = ClusterStartupRule.getClientCache();
UpdatableUserAuthInitialize.setUser("user1");
ClientRegionFactory clientRegionFactory =
assert clientCache != null;
Copy link
Contributor

@kirklund kirklund Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use actual Assertions in tests instead of Java asserts.

Copy link
Contributor Author

@jmelchio jmelchio Aug 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@mhansonp mhansonp Aug 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ClientRegionFactory<Object, Object> clientRegionFactory =
clientCache.createClientRegionFactory(ClientRegionShortcut.PROXY);
Region region = clientRegionFactory.create("region");
Region<Object, Object> region = clientRegionFactory.create("region");
region.put(0, "value0");
});

Expand All @@ -98,7 +103,8 @@ public void clientShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed()
clientVM.invoke(() -> {
UpdatableUserAuthInitialize.setUser("user2");
ClientCache clientCache = ClusterStartupRule.getClientCache();
Region region = clientCache.getRegion("region");
assert clientCache != null;
Copy link
Contributor

@mhansonp mhansonp Aug 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here and elsewhere...

Region<Object, Object> region = clientCache.getRegion("region");
region.put(1, "value1");
});

Expand All @@ -109,4 +115,58 @@ 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");
ClientCache clientCache = ClusterStartupRule.getClientCache();
assert clientCache != null;
clientCache.createClientRegionFactory(ClientRegionShortcut.PROXY).create("region");
Properties userSecurityProperties = new Properties();
userSecurityProperties.put(SECURITY_CLIENT_AUTH_INIT,
UpdatableUserAuthInitialize.class.getName());
regionService0 = clientCache.createAuthenticatedView(userSecurityProperties);
Region<Object, Object> region = regionService0.getRegion("/region");
region.put(0, "value0");

Copy link
Contributor

@mhansonp mhansonp Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are splitting this blocks for the same VM?

Copy link
Contributor Author

@jmelchio jmelchio Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mhansonp mhansonp Aug 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One note that is most likely irrelevant in this case is that splitting it takes more time for context switching.

UpdatableUserAuthInitialize.setUser("serviceUser1");
userSecurityProperties.put(SECURITY_CLIENT_AUTH_INIT,
UpdatableUserAuthInitialize.class.getName());
regionService1 = clientCache.createAuthenticatedView(userSecurityProperties);
region = regionService1.getRegion("/region");
region.put(1, "value1");
});

ExpirableSecurityManager.addExpiredUser("serviceUser1");

clientVM.invoke(() -> {
Region<Object, Object> region = regionService1.getRegion("/region");
UpdatableUserAuthInitialize.setUser("serviceUser2");
region.put(2, "value2");

region = regionService0.getRegion("/region");
region.put(3, "value3");
regionService0.close();
regionService1.close();
});

Region<Object, Object> region = server.getCache().getRegion("/region");
assertThat(ExpirableSecurityManager.getExpiredUsers().size()).isEqualTo(1);
assertThat(ExpirableSecurityManager.getExpiredUsers().contains("serviceUser1")).isTrue();
Map<Object, List<ResourcePermission>> authorizedOps =
ExpirableSecurityManager.getAuthorizedOps();
assertThat(authorizedOps.size()).isEqualTo(3);
assertThat(authorizedOps.get("serviceUser0").size()).isEqualTo(2);
assertThat(authorizedOps.get("serviceUser1").size()).isEqualTo(1);
assertThat(authorizedOps.get("serviceUser2").size()).isEqualTo(1);
assertThat(region.size()).isEqualTo(4);
}

}
19 changes: 18 additions & 1 deletion geode-junit/src/main/java/org/apache/geode/security/ExpirableSecurityManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@

package org.apache.geode.security;

import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;

Expand All @@ -32,12 +35,21 @@ public class ExpirableSecurityManager extends SimpleSecurityManager {
// use static field for ease of testing since there is only one instance of this in each VM
// we only need ConcurrentHashSet here, but map is only construct available in the library
private static final Set<String> EXPIRED_USERS = ConcurrentHashMap.newKeySet();
private static final Map<Object, List<ResourcePermission>> AUTHORIZED_OPS =
new ConcurrentHashMap<>();

@Override
public boolean authorize(Object principal, ResourcePermission permission) {
if (EXPIRED_USERS.contains(principal)) {
if (EXPIRED_USERS.contains((String) principal)) {
throw new AuthenticationExpiredException("User authentication expired.");
}
List<ResourcePermission> permissions = AUTHORIZED_OPS.get(principal);
if (permissions == null) {
permissions = new ArrayList<>();
}
permissions.add(permission);
AUTHORIZED_OPS.put(principal, permissions);

// always authorized
return true;
}
Expand All @@ -50,7 +62,12 @@ public static Set<String> getExpiredUsers() {
return EXPIRED_USERS;
}

public static Map<Object, List<ResourcePermission>> getAuthorizedOps() {
return AUTHORIZED_OPS;
}

public static void reset() {
EXPIRED_USERS.clear();
AUTHORIZED_OPS.clear();
}
}
1 change: 1 addition & 0 deletions geode-junit/src/main/java/org/apache/geode/security/UpdatableUserAuthInitialize.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public Properties getCredentials(Properties securityProps, DistributedMember ser
Properties credentials = new Properties();
credentials.put("security-username", user.get());
credentials.put("security-password", user.get());

return credentials;
}

Expand Down