DonalEvans commented on a change in pull request #7063:
URL: https://github.com/apache/geode/pull/7063#discussion_r741408553
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientUserAuths.java
##########
@@ -16,29 +16,44 @@
import static
org.apache.geode.cache.client.internal.AuthenticateUserOp.NOT_A_USER_ID;
+import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
+import java.util.List;
import java.util.Map;
import java.util.Random;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
+import java.util.stream.Collectors;
import org.apache.logging.log4j.Logger;
import org.apache.shiro.subject.Subject;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.TestOnly;
import org.apache.geode.annotations.VisibleForTesting;
import org.apache.geode.internal.security.AuthorizeRequest;
import org.apache.geode.internal.security.AuthorizeRequestPP;
import org.apache.geode.logging.internal.log4j.api.LogService;
+/**
+ * This is per ServerConnection or per CacheClientProxy, corresponding to only
one client
+ * connection.
+ * Credentials should usually be just one, only multiple in multi-user case.
+ */
public class ClientUserAuths {
private static final Logger logger = LogService.getLogger();
private final ConcurrentMap<Long, UserAuthAttributes> uniqueIdVsUserAuth =
new ConcurrentHashMap<>();
private final ConcurrentMap<String, UserAuthAttributes> cqNameVsUserAuth =
new ConcurrentHashMap<>();
- private final ConcurrentMap<Long, Subject> uniqueIdVsSubject = new
ConcurrentHashMap<>();
+ // use a list to store all the subjects that's created for this uniqueId
+ // In the expirable credential case, there will be multiple
+ // subjects created associated with one uniqueId. We always save the current
subject to the top of
+ // the list. The rest are "to-be-retired".
+ private final ConcurrentMap<Long, List<Subject>> uniqueIdVsSubject =
Review comment:
Could this be renamed to "uniqueIdVsSubject" so that it's clearer that
each ID may map to more than one Subject?
##########
File path:
geode-core/src/integrationTest/java/org/apache/geode/management/internal/security/SecurityWithExpirationIntegrationTest.java
##########
@@ -57,6 +58,14 @@ public void testAuthorizationWhenUserExpired() {
.isInstanceOf(AuthenticationExpiredException.class);
}
+ @Test
+ public void logoutMultipleTimeOnTheSameSubjectShouldNotThrowExcpetion() {
Review comment:
Typo here, should be "ThrowException"
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientUserAuths.java
##########
@@ -90,28 +120,29 @@ public UserAuthAttributes getUserAuthAttributes(final Long
userId) {
}
@VisibleForTesting
+ @TestOnly
protected Collection<Subject> getSubjects() {
Review comment:
Could this be "getAllSubjects" to make it clear that it's getting all
subjects for all unique IDs?
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/CacheClientProxyTest.java
##########
@@ -109,23 +122,59 @@ public void deliverMessageWhenSubjectIsNotNull() {
public void deliverMessageWhenSubjectIsNull() {
when(proxyStatsFactory.create(any(), any(), any()))
.thenReturn(mock(CacheClientProxyStats.class));
- proxy = spy(new CacheClientProxy(cache, notifier, socket, id, true, (byte)
1, version, 1L, true,
- securityService, null, clock, statsFactory, proxyStatsFactory,
dispatcherFactory));
- assertThat(proxy.getSubject()).isNull();
+ proxyWithMultiUser =
+ new CacheClientProxy(cache, notifier, socket, id, true, (byte) 1,
version, 1L, true,
+ securityService, null, clock, statsFactory, proxyStatsFactory,
dispatcherFactory,
+ clientUserAuths);
+ assertThat(proxyWithMultiUser.getSubject()).isNull();
Conflatable message = mock(ClientUpdateMessage.class);
when(securityService.needPostProcess()).thenReturn(true);
when(proxyStatsFactory.create(any(), any(), any()))
.thenReturn(mock(CacheClientProxyStats.class));
- proxy.deliverMessage(message);
+ proxyWithMultiUser.deliverMessage(message);
verify(securityService, never()).bindSubject(subject);
verify(securityService, never()).postProcess(any(), any(), any(),
anyBoolean());
}
@Test
public void replacingSubjectShouldNotLogout() {
- proxy = new CacheClientProxy(cache, notifier, socket, id, true, (byte) 1,
version, 1L, true,
- securityService, subject, clock, statsFactory, proxyStatsFactory,
dispatcherFactory);
- proxy.setSubject(mock(Subject.class));
+ proxyWithSingleUser.setSubject(mock(Subject.class));
+ verify(subject, never()).logout();
+ }
+
+ @Test
+ public void close_keepProxy_ShouldNotLogoutUser() {
+ when(id.isDurable()).thenReturn(true);
+ boolean keepProxy = proxyWithSingleUser.close(true, false);
+ assertThat(keepProxy).isTrue();
+ verify(subject, never()).logout();
+ verify(clientUserAuths, never()).cleanup(anyBoolean());
+
+ keepProxy = proxyWithMultiUser.close(true, false);
+ assertThat(keepProxy).isTrue();
+ verify(subject, never()).logout();
+ verify(clientUserAuths, never()).cleanup(anyBoolean());
+ }
+
+ @Test
+ public void close_singleUser() {
Review comment:
Could this test name be a little more descriptive? Adding what the
expected behaviour is would make it much clearer what the test is actually
testing. This also applies to the `close_multiUser()` test.
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/CacheClientProxyTest.java
##########
@@ -75,32 +79,41 @@ public void before() throws Exception {
statsFactory = mock(StatisticsFactory.class);
proxyStatsFactory = mock(CacheClientProxyStatsFactory.class);
dispatcherFactory = mock(MessageDispatcherFactory.class);
+ clientUserAuths = mock(ClientUserAuths.class);
+
+ proxyWithSingleUser =
+ new CacheClientProxy(cache, notifier, socket, id, true, (byte) 1,
version, 1L, true,
+ securityService, subject, clock, statsFactory, proxyStatsFactory,
dispatcherFactory,
+ clientUserAuths);
+
+ proxyWithMultiUser =
+ new CacheClientProxy(cache, notifier, socket, id, true, (byte) 1,
version, 1L, true,
+ securityService, null, clock, statsFactory, proxyStatsFactory,
dispatcherFactory,
+ clientUserAuths);
}
@Test
public void noExceptionWhenGettingSubjectForCQWhenSubjectIsNotNull() {
- proxy = spy(new CacheClientProxy(cache, notifier, socket, id, true, (byte)
1, version, 1L, true,
- securityService, subject, clock, statsFactory, proxyStatsFactory,
dispatcherFactory));
- proxy.getSubject("cq");
+ proxyWithSingleUser.getSubject("cq");
}
@Test
public void noExceptionWhenGettingSubjectForCQWhenSubjectIsNull() {
- proxy = spy(new CacheClientProxy(cache, notifier, socket, id, true, (byte)
1, version, 1L, true,
- securityService, null, clock, statsFactory, proxyStatsFactory,
dispatcherFactory));
- proxy.getSubject("cq");
+ proxyWithMultiUser.getSubject("cq");
}
@Test
public void deliverMessageWhenSubjectIsNotNull() {
when(proxyStatsFactory.create(any(), any(), any()))
.thenReturn(mock(CacheClientProxyStats.class));
Review comment:
If this mocking is moved to the `@Before` method then you can avoid
having to create a new `CacheClientProxy` on the following lines. This also
applies to the `deliverMessageWhenSubjectIsNull()` test.
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/CacheClientProxyTest.java
##########
@@ -109,23 +122,59 @@ public void deliverMessageWhenSubjectIsNotNull() {
public void deliverMessageWhenSubjectIsNull() {
when(proxyStatsFactory.create(any(), any(), any()))
.thenReturn(mock(CacheClientProxyStats.class));
- proxy = spy(new CacheClientProxy(cache, notifier, socket, id, true, (byte)
1, version, 1L, true,
- securityService, null, clock, statsFactory, proxyStatsFactory,
dispatcherFactory));
- assertThat(proxy.getSubject()).isNull();
+ proxyWithMultiUser =
+ new CacheClientProxy(cache, notifier, socket, id, true, (byte) 1,
version, 1L, true,
+ securityService, null, clock, statsFactory, proxyStatsFactory,
dispatcherFactory,
+ clientUserAuths);
+ assertThat(proxyWithMultiUser.getSubject()).isNull();
Conflatable message = mock(ClientUpdateMessage.class);
when(securityService.needPostProcess()).thenReturn(true);
when(proxyStatsFactory.create(any(), any(), any()))
.thenReturn(mock(CacheClientProxyStats.class));
- proxy.deliverMessage(message);
+ proxyWithMultiUser.deliverMessage(message);
verify(securityService, never()).bindSubject(subject);
verify(securityService, never()).postProcess(any(), any(), any(),
anyBoolean());
}
@Test
public void replacingSubjectShouldNotLogout() {
- proxy = new CacheClientProxy(cache, notifier, socket, id, true, (byte) 1,
version, 1L, true,
- securityService, subject, clock, statsFactory, proxyStatsFactory,
dispatcherFactory);
- proxy.setSubject(mock(Subject.class));
+ proxyWithSingleUser.setSubject(mock(Subject.class));
+ verify(subject, never()).logout();
+ }
+
+ @Test
+ public void close_keepProxy_ShouldNotLogoutUser() {
+ when(id.isDurable()).thenReturn(true);
+ boolean keepProxy = proxyWithSingleUser.close(true, false);
+ assertThat(keepProxy).isTrue();
+ verify(subject, never()).logout();
+ verify(clientUserAuths, never()).cleanup(anyBoolean());
+
+ keepProxy = proxyWithMultiUser.close(true, false);
+ assertThat(keepProxy).isTrue();
+ verify(subject, never()).logout();
+ verify(clientUserAuths, never()).cleanup(anyBoolean());
+ }
+
+ @Test
+ public void close_singleUser() {
+ when(id.isDurable()).thenReturn(false);
+ CacheClientProxy spy = spy(proxyWithSingleUser);
+ doNothing().when(spy).closeTransientFields();
+ boolean keepProxy = spy.close(true, false);
+ assertThat(keepProxy).isFalse();
+ verify(subject, times(1)).logout();
Review comment:
When `times()` is 1, it can be omitted, as the default behaviour of
`verify()` with no second argument is to verify that the method was invoked
once.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientUserAuths.java
##########
@@ -52,17 +62,32 @@ public Long putUserAuth(UserAuthAttributes userAuthAttr) {
return newId;
}
- public Long putSubject(Subject subject, long existingUniqueId) {
- final Long newId;
+
+ public long putSubject(@NotNull Subject subject, long existingUniqueId) {
+ final long newId;
if (existingUniqueId == 0 || existingUniqueId == NOT_A_USER_ID) {
newId = getNextID();
} else {
newId = existingUniqueId;
}
- Subject oldSubject = uniqueIdVsSubject.put(newId, subject);
- removeSubject(oldSubject);
- logger.debug("Subject of {} added.", newId);
+ // we are saving all the subjects that's related to this uniqueId
+ // we cannot immediately log out the old subject of this userId because
+ // it might already be bound to another thread and doing operations. If
+ // we log out that subject immediately, that thread "authorize" would get
null principal.
+ synchronized (this) {
+ CopyOnWriteArrayList<Subject> subjects;
+ if (!uniqueIdVsSubject.containsKey(newId)) {
+ logger.debug("Subject of {} added.", newId);
+ subjects = new CopyOnWriteArrayList<>();
+ uniqueIdVsSubject.put(newId, subjects);
+ } else {
+ logger.debug("Subject of {} replaced.", newId);
+ subjects = uniqueIdVsSubject.get(newId);
+ }
+ // always add the latest subject to the top of the list;
+ subjects.add(0, subject);
Review comment:
Why does the latest subject have to be inserted as the first element in
the list? Could we instead add that subject to the end of the list, and then in
`getSubject(final Long userId)` retrieve the last element in the list? This
would be a simple solution to the performance issue that Jianxia described.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientUserAuths.java
##########
@@ -90,28 +120,29 @@ public UserAuthAttributes getUserAuthAttributes(final Long
userId) {
}
@VisibleForTesting
+ @TestOnly
protected Collection<Subject> getSubjects() {
- return Collections.unmodifiableCollection(uniqueIdVsSubject.values());
+ List<Subject> all = uniqueIdVsSubject.values().stream()
+ .flatMap(List::stream)
+ .collect(Collectors.toList());
+ return Collections.unmodifiableCollection(all);
}
- public Subject getSubject(final Long userId) {
- return uniqueIdVsSubject.get(userId);
+ public synchronized Subject getSubject(final Long userId) {
+ List<Subject> subjects = uniqueIdVsSubject.get(userId);
+ if (subjects == null || subjects.isEmpty()) {
+ return null;
+ }
+ return subjects.get(0);
}
- public void removeSubject(final Long userId) {
+ public synchronized void removeSubject(final Long userId) {
logger.debug("Subject of {} removed.", userId);
Review comment:
Given that this call may result in multiple subjects being removed,
would it be worth modifying the debug logging to give a little more
information, such as how many subjects were removed (if any)?
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ClientUserAuthsTest.java
##########
@@ -45,37 +47,78 @@ public void before() throws Exception {
}
@Test
- public void putSubjectWithNegativeOneWillProduceNewId() throws Exception {
+ public void putSubjectWithNegativeOneWillProduceNewId() {
Long uniqueId = auth.putSubject(subject1, -1);
- verify(auth).removeSubject((Subject) null);
+ verify(auth, never()).removeSubject(anyLong());
assertThat(uniqueId).isEqualTo(123L);
}
@Test
- public void putSubjectWithZeroWillProduceNewId() throws Exception {
+ public void putSubjectWithZeroWillProduceNewId() {
Long uniqueId = auth.putSubject(subject1, 0);
- verify(auth).removeSubject((Subject) null);
+ verify(auth, never()).removeSubject(anyLong());
assertThat(uniqueId).isEqualTo(123L);
+ verify(subject1, never()).logout();
}
@Test
- public void putSubjectWithExistingId() throws Exception {
+ public void putSubjectWithExistingId() {
Long uniqueId = auth.putSubject(subject1, 456L);
- verify(auth).removeSubject((Subject) null);
+ verify(auth, never()).removeSubject(anyLong());
assertThat(uniqueId).isEqualTo(456L);
+ verify(subject1, never()).logout();
}
@Test
- public void replacedSubjectShouldLogout() throws Exception {
+ public void replacedSubjectShouldNotLogout() {
Long id1 = auth.putSubject(subject1, -1);
Long id2 = auth.putSubject(subject2, id1);
assertThat(id1).isEqualTo(id2);
- verify(auth).removeSubject(eq(subject1));
+ verify(auth, never()).removeSubject(anyLong());
+ verify(subject1, never()).logout();
+
+ }
+
+ @Test
+ public void getSubjectReturnsTheLatestOne() {
+ Long id1 = auth.putSubject(subject1, -1);
+ assertThat(auth.getSubject(id1)).isSameAs(subject1);
+ auth.putSubject(subject2, id1);
+ assertThat(auth.getSubject(id1)).isSameAs(subject2);
+ }
+
+ @Test
+ public void getSubjectOfNonExistentId() {
Review comment:
Could this test name include the expected behaviour? Something like
"getSubjectWithNonExistentIdReturnsNull" maybe? This also applies to
`removeSubjectOfNonExistentId()`, `getSubjectWithCq()` and `removeSubject()`.
--
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]
