DonalEvans commented on a change in pull request #6835:
URL: https://github.com/apache/geode/pull/6835#discussion_r708699303
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/management/internal/security/MultiUserAuthenticationDUnitTest.java
##########
@@ -100,4 +112,65 @@ public void multiAuthenticatedView() throws Exception {
cache.close();
}
}
+
+ @Test
+ public void multiUserCQ() throws Exception {
+ int locatorPort = locator.getPort();
+ ClientCache cache = client.withCacheSetup(f ->
f.setPoolSubscriptionEnabled(true)
+ .setPoolMultiuserAuthentication(true)
+ .addPoolLocator("localhost", locatorPort))
+ .createCache();
+
+ // both are able to read data
+ RegionService regionService1 = client.createAuthenticatedView("data",
"data");
+ RegionService regionService2 = client.createAuthenticatedView("dataRead",
"dataRead");
+
+ EventsCqListner listener1 =
createAndExecuteCQ(regionService1.getQueryService(), "cq1",
+ "select * from /region r where r.length<=2");
+ EventsCqListner listener2 =
createAndExecuteCQ(regionService2.getQueryService(), "cq2",
+ "select * from /region r where r.length>=2");
+
+ // put 4 data in the region
Review comment:
This comment is incorrect; only three entries are put into the region.
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/management/internal/security/MultiUserAuthenticationDUnitTest.java
##########
@@ -100,4 +112,65 @@ public void multiAuthenticatedView() throws Exception {
cache.close();
}
}
+
+ @Test
+ public void multiUserCQ() throws Exception {
+ int locatorPort = locator.getPort();
+ ClientCache cache = client.withCacheSetup(f ->
f.setPoolSubscriptionEnabled(true)
Review comment:
This variable is never used, so this line could just be
`client.withCacheSetup(f -> f.setPoolSubscriptionEnabled(true)`
##########
File path:
geode-junit/src/main/java/org/apache/geode/test/version/VersionManager.java
##########
@@ -143,6 +143,14 @@ public String getInstall(String version) {
return unmodifiableList(testVersions);
}
+ public List<String> getVersionsLaterThanAndEqualTo(String version) {
+ checkForLoadFailure();
+ List<String> result = new ArrayList<>(testVersions);
+ result.removeIf(s -> TestVersion.compare(s, version) < 0);
+ result.add(CURRENT_VERSION);
Review comment:
This method may return unexpected results if the `version` passed to it
is greater than `CURRENT_VERSION`, since, from the method name, we would not
expect any versions to be returned in that situation.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/PutUserCredentials.java
##########
@@ -39,45 +39,44 @@ public void cmdExecute(final Message clientMessage, final
ServerConnection serve
throws IOException, ClassNotFoundException, InterruptedException {
boolean isSecureMode = clientMessage.isSecureMode();
- // if (!isSecureMode)
- // client has not send secuirty header, need to send exception and log
this in security (file)
-
- if (isSecureMode) {
-
- int numberOfParts = clientMessage.getNumberOfParts();
+ if (!isSecureMode) {
+ // client has not send secuirty header, need to send exception and log
this in security (file)
Review comment:
Typo here, should be "security". Also, the comment makes it seem like an
exception should be sent and something should be logged here.
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/CacheClientProxyTest.java
##########
@@ -0,0 +1,123 @@
+/*
+ *
+ * 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.internal.cache.tier.sockets;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.net.InetAddress;
+import java.net.Socket;
+
+import org.apache.shiro.subject.Subject;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.geode.StatisticsFactory;
+import org.apache.geode.internal.cache.Conflatable;
+import org.apache.geode.internal.cache.InternalCache;
+import
org.apache.geode.internal.cache.tier.sockets.CacheClientProxy.CacheClientProxyStatsFactory;
+import
org.apache.geode.internal.cache.tier.sockets.CacheClientProxy.MessageDispatcherFactory;
+import org.apache.geode.internal.security.SecurityService;
+import org.apache.geode.internal.serialization.KnownVersion;
+import org.apache.geode.internal.statistics.StatisticsClock;
+
+public class CacheClientProxyTest {
+ private CacheClientProxy proxy;
+ private CacheClientNotifier notifier;
+ private Socket socket;
+ private ClientProxyMembershipID id;
+ private KnownVersion version;
+ private SecurityService securityService;
+ private Subject subject;
+ private StatisticsClock clock;
+ private InternalCache cache;
+ private StatisticsFactory statsFactory;
+ private CacheClientProxyStatsFactory proxyStatsFactory;
+ private MessageDispatcherFactory dispatcherFactory;
+ private InetAddress inetAddress;
+ private CacheServerStats stats;
+
+ @Before
+ public void before() throws Exception {
+ notifier = mock(CacheClientNotifier.class);
+ stats = mock(CacheServerStats.class);
+ socket = mock(Socket.class);
+ inetAddress = mock(InetAddress.class);
+ when(socket.getInetAddress()).thenReturn(inetAddress);
+ when(notifier.getAcceptorStats()).thenReturn(stats);
+ id = mock(ClientProxyMembershipID.class);
+ version = KnownVersion.TEST_VERSION;
+ securityService = mock(SecurityService.class);
+ subject = mock(Subject.class);
+ clock = mock(StatisticsClock.class);
+ cache = mock(InternalCache.class);
+ statsFactory = mock(StatisticsFactory.class);
+ proxyStatsFactory = mock(CacheClientProxyStatsFactory.class);
+ dispatcherFactory = mock(MessageDispatcherFactory.class);
+ }
+
+ @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");
+ }
+
+ @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");
+ }
+
+ @Test
+ public void deliverMessageWhenSubjectIsNotNull() throws Exception {
Review comment:
`Exception` is not thrown from this method, nor from the one below.
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/management/internal/security/MultiUserAuthenticationDUnitTest.java
##########
@@ -100,4 +112,65 @@ public void multiAuthenticatedView() throws Exception {
cache.close();
}
}
+
+ @Test
+ public void multiUserCQ() throws Exception {
+ int locatorPort = locator.getPort();
+ ClientCache cache = client.withCacheSetup(f ->
f.setPoolSubscriptionEnabled(true)
+ .setPoolMultiuserAuthentication(true)
+ .addPoolLocator("localhost", locatorPort))
+ .createCache();
+
+ // both are able to read data
+ RegionService regionService1 = client.createAuthenticatedView("data",
"data");
+ RegionService regionService2 = client.createAuthenticatedView("dataRead",
"dataRead");
+
+ EventsCqListner listener1 =
createAndExecuteCQ(regionService1.getQueryService(), "cq1",
+ "select * from /region r where r.length<=2");
+ EventsCqListner listener2 =
createAndExecuteCQ(regionService2.getQueryService(), "cq2",
+ "select * from /region r where r.length>=2");
+
+ // put 4 data in the region
+ gfsh.executeAndAssertThat("put --region=region --key=1 --value=1");
+ gfsh.executeAndAssertThat("put --region=region --key=11 --value=11");
+ gfsh.executeAndAssertThat("put --region=region --key=111 --value=111");
+
+ await().untilAsserted(
+ () -> assertThat(listener1.getKeys())
+ .containsExactly("1", "11"));
+
+ // user1Service listener will eventually get these events
Review comment:
This comment is unclear in what it's referring to, as there's nothing
called "user1Service" in this test. Could it be reworded?
##########
File path:
geode-core/src/main/java/org/apache/geode/cache/client/internal/AuthenticateUserOp.java
##########
@@ -87,31 +101,13 @@ private AuthenticateUserOp() {
}
static class AuthenticateUserOpImpl extends AbstractOp {
-
+ private static final Logger logger = LogService.getLogger();
Review comment:
This logger is never used in this class and can probably be removed.
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/management/internal/security/MultiUserAuthenticationDUnitTest.java
##########
@@ -100,4 +112,65 @@ public void multiAuthenticatedView() throws Exception {
cache.close();
}
}
+
+ @Test
+ public void multiUserCQ() throws Exception {
Review comment:
Would it make sense to have additional test cases where both users don't
have the same/equivalent permissions?
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/CacheClientUpdater.java
##########
@@ -1763,6 +1767,18 @@ private void processMessages() {
}
}
+ private void handleAuthenticate(Message clientMessage) {
+ // if client is in multi-user mode, the CacheClientUpdator (at this point)
Review comment:
Typo here, should be "CacheClientUpdater"
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/OldClientSupportService.java
##########
@@ -18,13 +18,16 @@
import java.io.DataInput;
import java.io.DataOutput;
+import org.apache.geode.annotations.Immutable;
import org.apache.geode.internal.cache.CacheService;
import org.apache.geode.internal.serialization.KnownVersion;
/**
* Support for old GemFire clients
*/
public interface OldClientSupportService extends CacheService {
+ @Immutable
+ KnownVersion RE_AUTHENTICATION_START_VERSION = KnownVersion.GEODE_1_15_0;
Review comment:
Would it make more sense to keep this constant in the
`ClientReAuthenticateMessage` class or some other class specifically related to
re-authentication?
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientReAuthenticateMessage.java
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.internal.cache.tier.sockets;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+
+import org.apache.geode.internal.cache.EventID;
+import org.apache.geode.internal.cache.tier.MessageType;
+import org.apache.geode.internal.serialization.DeserializationContext;
+import org.apache.geode.internal.serialization.KnownVersion;
+import org.apache.geode.internal.serialization.SerializationContext;
+
+public class ClientReAuthenticateMessage implements ClientMessage {
+
+ /**
+ * This {@code ClientMessage}'s {@code EventID}
+ */
+ private final EventID eventId;
+
+ public ClientReAuthenticateMessage() {
+ this(new EventID());
+ }
+
+ public ClientReAuthenticateMessage(EventID eventId) {
+ this.eventId = eventId;
+ }
+
+ @Override
+ public Message getMessage(CacheClientProxy proxy, boolean notify) throws
IOException {
Review comment:
`IOException` is never thrown from this method.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/PutUserCredentials.java
##########
@@ -39,45 +39,44 @@ public void cmdExecute(final Message clientMessage, final
ServerConnection serve
throws IOException, ClassNotFoundException, InterruptedException {
boolean isSecureMode = clientMessage.isSecureMode();
- // if (!isSecureMode)
- // client has not send secuirty header, need to send exception and log
this in security (file)
-
- if (isSecureMode) {
-
- int numberOfParts = clientMessage.getNumberOfParts();
+ if (!isSecureMode) {
+ // client has not send secuirty header, need to send exception and log
this in security (file)
+ return;
+ }
- if (numberOfParts == 1) {
- // need to get credentials
- try {
- serverConnection.setAsTrue(REQUIRES_RESPONSE);
- byte[] uniqueId = serverConnection.setCredentials(clientMessage);
- writeResponse(uniqueId, null, clientMessage, false,
serverConnection);
- } catch (GemFireSecurityException gfse) {
- if (serverConnection.getSecurityLogWriter().warningEnabled()) {
- serverConnection.getSecurityLogWriter().warning(String.format("%s",
- serverConnection.getName() + ": Security exception: " +
gfse.toString()
- + (gfse.getCause() != null ? ", caused by: " +
gfse.getCause().toString()
- : "")));
- }
- writeException(clientMessage, gfse, false, serverConnection);
- } catch (Exception ex) {
- if (serverConnection.getLogWriter().warningEnabled()) {
- serverConnection.getLogWriter().warning(
- String.format("An exception was thrown for client [%s]. %s",
- serverConnection.getProxyID(), ""),
- ex);
- }
- writeException(clientMessage, ex, false, serverConnection);
- } finally {
- serverConnection.setAsTrue(RESPONDED);
- }
+ int numberOfParts = clientMessage.getNumberOfParts();
+ if (numberOfParts != 1) {
+ // need to throw exception
Review comment:
This comment makes it seem that an exception should be thrown.
##########
File path:
geode-core/src/main/java/org/apache/geode/cache/client/internal/AbstractOp.java
##########
@@ -166,6 +166,8 @@ protected void processSecureBytes(Connection cnx, Message
message) throws Except
}
/**
+ * return true if this operation needs to be authenticated first
Review comment:
Could this be moved to a `@return` statement in the javadoc to make it
more visible?
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/CacheClientUpdater.java
##########
@@ -1763,6 +1767,18 @@ private void processMessages() {
}
}
+ private void handleAuthenticate(Message clientMessage) {
Review comment:
The `clientMessage` is never used in this method, so can probably be
removed from the signature.
##########
File path:
geode-core/src/main/java/org/apache/geode/cache/client/internal/AuthenticateUserOp.java
##########
@@ -123,67 +119,89 @@ private AuthenticateUserOp() {
super(MessageType.USER_CREDENTIAL_MESSAGE, 1);
securityProperties = securityProps;
needsServerLocation = needsServer;
-
getMessage().setMessageHasSecurePartFlag();
}
@Override
- protected void sendMessage(Connection cnx) throws Exception {
- HeapDataOutputStream hdos = new
HeapDataOutputStream(KnownVersion.CURRENT);
- byte[] secureBytes;
- hdos.writeLong(cnx.getConnectionID());
- if (securityProperties != null) {
- DistributedMember server = new
InternalDistributedMember(cnx.getSocket().getInetAddress(),
- cnx.getSocket().getPort(), false);
- DistributedSystem sys =
InternalDistributedSystem.getConnectedInstance();
- String authInitMethod =
sys.getProperties().getProperty(SECURITY_CLIENT_AUTH_INIT);
-
- Properties credentials = Handshake.getCredentials(authInitMethod,
securityProperties,
- server, false, sys.getLogWriter(), sys.getSecurityLogWriter());
- byte[] credentialBytes;
- try (HeapDataOutputStream heapdos = new
HeapDataOutputStream(KnownVersion.CURRENT)) {
- DataSerializer.writeProperties(credentials, heapdos);
- credentialBytes = ((ConnectionImpl)
cnx).encryptBytes(heapdos.toByteArray());
- }
- getMessage().addBytesPart(credentialBytes);
+ protected void sendMessage(Connection connection) throws Exception {
+ if (securityProperties == null) {
+ securityProperties = getConnectedSystem().getSecurityProperties();
}
- try {
- secureBytes = ((ConnectionImpl) cnx).encryptBytes(hdos.toByteArray());
- } finally {
- hdos.close();
+ byte[] credentialBytes = getCredentialBytes(connection,
securityProperties);
+ getMessage().addBytesPart(credentialBytes);
+
+ try (HeapDataOutputStream hdos = new HeapDataOutputStream(16,
KnownVersion.CURRENT)) {
+ hdos.writeLong(connection.getConnectionID());
+ hdos.writeLong(getUserId(connection));
+ getMessage().setSecurePart(((ConnectionImpl)
connection).encryptBytes(hdos.toByteArray()));
}
- getMessage().setSecurePart(secureBytes);
getMessage().send(false);
}
+ protected long getUserId(Connection connection) {
+ // single user mode
+ if (UserAttributes.userAttributes.get() == null) {
+ return connection.getServer().getUserId();
+ }
+ // multi user mode
+ Long id =
UserAttributes.userAttributes.get().getServerToId().get(connection.getServer());
+ if (id == null) {
+ return -1L;
+ }
+ return id;
+ }
+
+ protected InternalDistributedSystem getConnectedSystem() {
+ return InternalDistributedSystem.getConnectedInstance();
+ }
+
+ protected byte[] getCredentialBytes(Connection connection, Properties
securityProperties)
+ throws Exception {
+ InternalDistributedSystem system = getConnectedSystem();
Review comment:
Could this variable be renamed to prevent confusion between the internal
distributed system and `java.lang.System`, which also has a `getProperties()`
method?
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java
##########
@@ -894,6 +876,50 @@ void doNormalMessage() {
}
}
+ ThreadState bindSubject(Command command) {
+ if (!securityService.isIntegratedSecurity()) {
+ return null;
+ }
+
+ if (communicationMode.isWAN()) {
+ return null;
+ }
+
+ if (command instanceof PutUserCredentials) {
+ return null;
+ }
+
+ if (isInternalMessage(requestMessage,
allowInternalMessagesWithoutCredentials)) {
+ return null;
+ }
+
+ long uniqueId = getUniqueId();
+ String messageType =
MessageType.getString(requestMessage.getMessageType());
+ if (uniqueId == 0 || uniqueId == -1) {
+ logger.debug("No unique ID yet. {}, {}", messageType, getName());
Review comment:
Should this (and the other `logger.debug()` call below) be wrapped with
`if (LogService.isDebugEnabled())`?
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcher.java
##########
@@ -609,7 +707,18 @@ protected boolean dispatchMessage(ClientMessage
clientMessage) throws IOExceptio
return isDispatched;
}
- private void sendMessage(Message message) throws IOException {
+ @NotNull
+ private ResourcePermission getResourcePermission(ClientUpdateMessage
message) {
+ String regionName = message.getRegionName();
+ Object key = message.getKeyOfInterest();
+ ResourcePermission permission = new
ResourcePermission(ResourcePermission.Resource.DATA,
+ ResourcePermission.Operation.READ,
+ regionName, key == null ? null : key.toString());
+ return permission;
Review comment:
This can be simplified to just
```
return new ResourcePermission(ResourcePermission.Resource.DATA,
ResourcePermission.Operation.READ, regionName, key == null ? null :
key.toString());
```
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/management/internal/security/MultiUserAuthenticationDUnitTest.java
##########
@@ -100,4 +112,65 @@ public void multiAuthenticatedView() throws Exception {
cache.close();
}
}
+
+ @Test
+ public void multiUserCQ() throws Exception {
+ int locatorPort = locator.getPort();
+ ClientCache cache = client.withCacheSetup(f ->
f.setPoolSubscriptionEnabled(true)
+ .setPoolMultiuserAuthentication(true)
+ .addPoolLocator("localhost", locatorPort))
+ .createCache();
+
+ // both are able to read data
+ RegionService regionService1 = client.createAuthenticatedView("data",
"data");
+ RegionService regionService2 = client.createAuthenticatedView("dataRead",
"dataRead");
+
+ EventsCqListner listener1 =
createAndExecuteCQ(regionService1.getQueryService(), "cq1",
+ "select * from /region r where r.length<=2");
+ EventsCqListner listener2 =
createAndExecuteCQ(regionService2.getQueryService(), "cq2",
+ "select * from /region r where r.length>=2");
+
+ // put 4 data in the region
+ gfsh.executeAndAssertThat("put --region=region --key=1 --value=1");
+ gfsh.executeAndAssertThat("put --region=region --key=11 --value=11");
+ gfsh.executeAndAssertThat("put --region=region --key=111 --value=111");
+
+ await().untilAsserted(
+ () -> assertThat(listener1.getKeys())
+ .containsExactly("1", "11"));
+
+ // user1Service listener will eventually get these events
+ await().untilAsserted(
+ () -> assertThat(listener2.getKeys())
+ .containsExactly("11", "111"));
+
+ }
+
+ private static EventsCqListner createAndExecuteCQ(QueryService queryService,
String cqName,
+ String query)
+ throws CqExistsException, CqException, RegionNotFoundException {
+ CqAttributesFactory cqaf = new CqAttributesFactory();
+ EventsCqListner listenter = new EventsCqListner();
Review comment:
Typo in this variable name, it should be `listener`.
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/CacheClientProxyTest.java
##########
@@ -0,0 +1,123 @@
+/*
+ *
+ * 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.internal.cache.tier.sockets;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.net.InetAddress;
+import java.net.Socket;
+
+import org.apache.shiro.subject.Subject;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.geode.StatisticsFactory;
+import org.apache.geode.internal.cache.Conflatable;
+import org.apache.geode.internal.cache.InternalCache;
+import
org.apache.geode.internal.cache.tier.sockets.CacheClientProxy.CacheClientProxyStatsFactory;
+import
org.apache.geode.internal.cache.tier.sockets.CacheClientProxy.MessageDispatcherFactory;
+import org.apache.geode.internal.security.SecurityService;
+import org.apache.geode.internal.serialization.KnownVersion;
+import org.apache.geode.internal.statistics.StatisticsClock;
+
+public class CacheClientProxyTest {
+ private CacheClientProxy proxy;
+ private CacheClientNotifier notifier;
+ private Socket socket;
+ private ClientProxyMembershipID id;
+ private KnownVersion version;
+ private SecurityService securityService;
+ private Subject subject;
+ private StatisticsClock clock;
+ private InternalCache cache;
+ private StatisticsFactory statsFactory;
+ private CacheClientProxyStatsFactory proxyStatsFactory;
+ private MessageDispatcherFactory dispatcherFactory;
+ private InetAddress inetAddress;
+ private CacheServerStats stats;
Review comment:
These fields can be converted to local variables in `before()`.
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcherTest.java
##########
@@ -0,0 +1,167 @@
+/*
+ *
+ * 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.internal.cache.tier.sockets;
+
+import static
org.apache.geode.internal.lang.SystemPropertyHelper.RE_AUTHENTICATE_WAIT_TIME;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import org.apache.shiro.subject.Subject;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.geode.CancelCriterion;
+import org.apache.geode.internal.cache.EventID;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.cache.ha.HARegionQueue;
+import org.apache.geode.internal.cache.ha.HARegionQueueStats;
+import
org.apache.geode.internal.cache.tier.sockets.ClientUpdateMessageImpl.CqNameToOp;
+import org.apache.geode.internal.security.SecurityService;
+import org.apache.geode.internal.serialization.KnownVersion;
+import org.apache.geode.security.AuthenticationExpiredException;
+import org.apache.geode.security.ResourcePermission;
+
+public class MessageDispatcherTest {
+ private MessageDispatcher dispatcher;
+ private CacheClientProxy proxy;
+ private ClientUpdateMessage message;
+ private InternalCache cache;
+ private SecurityService securityService;
+ private Subject subject;
+ private CacheClientNotifier notifier;
+ private ClientProxyMembershipID proxyId;
+ private HARegionQueue messageQueue;
+ private HARegionQueueStats queueStats;
+ private CancelCriterion cancelCriterion;
+ private CacheClientProxyStats proxyStats;
Review comment:
`cache`, `notifier`, `proxyId`, `queueStats`, `cancelCriterion` and
`proxyStats` can all be converted to local variables in the `before()` method.
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcherTest.java
##########
@@ -0,0 +1,167 @@
+/*
+ *
+ * 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.internal.cache.tier.sockets;
+
+import static
org.apache.geode.internal.lang.SystemPropertyHelper.RE_AUTHENTICATE_WAIT_TIME;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import org.apache.shiro.subject.Subject;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.geode.CancelCriterion;
+import org.apache.geode.internal.cache.EventID;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.cache.ha.HARegionQueue;
+import org.apache.geode.internal.cache.ha.HARegionQueueStats;
+import
org.apache.geode.internal.cache.tier.sockets.ClientUpdateMessageImpl.CqNameToOp;
+import org.apache.geode.internal.security.SecurityService;
+import org.apache.geode.internal.serialization.KnownVersion;
+import org.apache.geode.security.AuthenticationExpiredException;
+import org.apache.geode.security.ResourcePermission;
+
+public class MessageDispatcherTest {
+ private MessageDispatcher dispatcher;
+ private CacheClientProxy proxy;
+ private ClientUpdateMessage message;
+ private InternalCache cache;
+ private SecurityService securityService;
+ private Subject subject;
+ private CacheClientNotifier notifier;
+ private ClientProxyMembershipID proxyId;
+ private HARegionQueue messageQueue;
+ private HARegionQueueStats queueStats;
+ private CancelCriterion cancelCriterion;
+ private CacheClientProxyStats proxyStats;
+ private EventID eventID;
+
+ @Before
+ public void before() throws Exception {
+ proxy = mock(CacheClientProxy.class);
+ message = mock(ClientUpdateMessageImpl.class);
+ cache = mock(InternalCache.class);
+ subject = mock(Subject.class);
+ securityService = mock(SecurityService.class);
+ notifier = mock(CacheClientNotifier.class);
+ proxyId = mock(ClientProxyMembershipID.class);
+ messageQueue = mock(HARegionQueue.class);
+ queueStats = mock(HARegionQueueStats.class);
+ cancelCriterion = mock(CancelCriterion.class);
+ proxyStats = mock(CacheClientProxyStats.class);
+ eventID = mock(EventID.class);
+ when(messageQueue.getStatistics()).thenReturn(queueStats);
+ when(cache.getSecurityService()).thenReturn(securityService);
+ when(proxy.getCache()).thenReturn(cache);
+ when(cache.getCancelCriterion()).thenReturn(cancelCriterion);
+ when(proxy.getCacheClientNotifier()).thenReturn(notifier);
+ when(proxy.getProxyID()).thenReturn(proxyId);;
Review comment:
Extra semicolon here.
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionTest.java
##########
@@ -124,4 +146,97 @@ public void nonSecureShouldThrow() {
.isExactlyInstanceOf(AuthenticationRequiredException.class)
.hasMessage("No security credentials are provided");
}
+
+ @Test
+ public void bindSubjectDoesNothingIfNotIntegratedService() throws Exception {
Review comment:
`Exception` is never thrown from any of the tests added in this class.
##########
File path:
geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationDUnitTest.java
##########
@@ -70,15 +84,70 @@
.withSecurityManager(ExpirableSecurityManager.class)
.withRegion(RegionShortcut.REPLICATE, "region");
+
+ private ClientVM clientVM;
+
@Test
- public void
clientShouldReAuthenticateWhenCredentialExpiredAndOperationSucceed()
- throws Exception {
+ public void clientWithNoUserRefreshWillNotSucceed() throws Exception {
int serverPort = server.getPort();
ClientVM clientVM = lsRule.startClientVM(0, clientVersion,
c -> c.withProperty(SECURITY_CLIENT_AUTH_INIT,
UpdatableUserAuthInitialize.class.getName())
.withPoolSubscription(true)
.withServerConnection(serverPort));
+ clientVM.invoke(() -> {
+ UpdatableUserAuthInitialize.setUser("user1");
+ ClientCache clientCache = ClusterStartupRule.getClientCache();
+ ClientRegionFactory<Object, Object> clientRegionFactory =
+ clientCache.createClientRegionFactory(ClientRegionShortcut.PROXY);
+ Region<Object, Object> region = clientRegionFactory.create("region");
+ region.put(0, "value0");
+ });
+
+ // expire the current user
+ ExpirableSecurityManager securityManager = getSecurityManager();
+ securityManager.addExpiredUser("user1");
+
+ // if client, even after getting AuthExpiredExpiration, still sends in
+ // old credentials, the operation will fail (we only try re-authenticate
once)
+ // this test makes sure no lingering old credentials will allow the
operations to succeed.
+ clientVM.invoke(() -> {
+ ClientCache clientCache = ClusterStartupRule.getClientCache();
+ Region<Object, Object> region = clientCache.getRegion("region");
+ doPutAndExpectFailure(region, 100);
+ });
+
+ Region<Object, Object> region = server.getCache().getRegion("/region");
+ assertThat(region.size()).isEqualTo(1);
+ Map<String, List<String>> authorizedOps =
securityManager.getAuthorizedOps();
+ Map<String, List<String>> unAuthorizedOps =
securityManager.getUnAuthorizedOps();
+ assertThat(authorizedOps.keySet().size()).isEqualTo(1);
+
assertThat(authorizedOps.get("user1")).asList().containsExactly("DATA:WRITE:region:0");
+ assertThat(unAuthorizedOps.keySet().size()).isEqualTo(1);
+ }
+
+ private static void doPutAndExpectFailure(Region<Object, Object> region, int
times) {
+ for (int i = 1; i < times; i++) {
+ try {
+ region.put(1, "value1");
+ fail("Exception expected");
+ } catch (Exception e) {
+ assertThat(e).isInstanceOf(ServerOperationException.class);
+
assertThat(e.getCause()).isInstanceOfAny(AuthenticationFailedException.class,
+ AuthenticationRequiredException.class);
+ }
Review comment:
This can be simplified to:
```
assertThatThrownBy(() -> region.put(1, "value1"))
.isInstanceOf(ServerOperationException.class)
.getCause().isInstanceOfAny(AuthenticationFailedException.class,
AuthenticationRequiredException.class);
```
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionTest.java
##########
@@ -57,25 +67,37 @@
private AcceptorImpl acceptor;
private Message requestMessage;
private ServerSideHandshake handshake;
+ private SecurityService securityService;
private ServerConnection serverConnection;
+ private Command command;
+ private InetAddress inetAddress;
+ private Socket socket;
+ private InternalCacheForClientAccess cache;
+ private CachedRegionHelper cachedRegionHelper;
+ private InternalDistributedSystem internalDistributedSystem;
+ private DistributionManager distributionManager;
+ private ThreadsMonitoring threadsMonitoring;
Review comment:
`inetAddress`, `internalDistributedSystem`, `distributionManager` and
`threadsMonitoring` can all be converted to local variables in `setUp()`.
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/ClientUserAuthsTest.java
##########
@@ -0,0 +1,84 @@
+/*
+ *
+ * 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.internal.cache.tier.sockets;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import org.apache.shiro.subject.Subject;
+import org.junit.Before;
+import org.junit.Test;
+
+public class ClientUserAuthsTest {
+ private ClientUserAuths auth;
+ private Subject subject1;
+ private Subject subject2;
+
+ @Before
+ public void before() throws Exception {
+ subject1 = mock(Subject.class);
+ subject2 = mock(Subject.class);
+ when(subject1.getPrincipal()).thenReturn("user1");
+ when(subject2.getPrincipal()).thenReturn("user2");
+ auth = spy(new ClientUserAuths(0));
+ doReturn(123L).when(auth).getNextID();
+ }
+
+ @Test
+ public void putSubjectWithNegativeOneWillProduceNewId() throws Exception {
Review comment:
`Exception` is never thrown from any of the test methods in this class.
##########
File path:
geode-core/src/upgradeTest/java/org/apache/geode/security/AuthExpirationDUnitTest.java
##########
@@ -56,7 +70,7 @@
@Parameterized.Parameters(name = "{0}")
public static Collection<String> data() {
// only test the current version and the latest released version
Review comment:
This comment is inaccurate, as once we're on version 1.16.0, this test
will use versions 1.14.0, 1.15.0 and 1.16.0. A more accurate comment would be
"only test versions greater than or equal to 1.14.0"
--
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]
