kirklund commented on a change in pull request #6835:
URL: https://github.com/apache/geode/pull/6835#discussion_r707765614
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/management/internal/security/MultiUserAuthenticationDUnitTest.java
##########
@@ -16,9 +16,14 @@
package org.apache.geode.management.internal.security;
import static org.apache.geode.cache.Region.SEPARATOR;
+import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
Review comment:
I think you meant to use:
```
import static org.assertj.core.api.Assertions.assertThat;
```
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/management/internal/security/MultiUserAuthenticationDUnitTest.java
##########
@@ -100,4 +113,67 @@ 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().atMost(100, TimeUnit.SECONDS).untilAsserted(
Review comment:
These `await()` calls should just use the default timeout. If this test
runs on a slow or overloaded machine, it may fail here.
##########
File path:
geode-core/src/main/java/org/apache/geode/cache/client/internal/AuthenticateUserOp.java
##########
@@ -123,39 +119,61 @@ 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);
+ if (securityProperties == null) {
+ securityProperties = getConnectedSystem().getSecurityProperties();
}
- try {
- secureBytes = ((ConnectionImpl) cnx).encryptBytes(hdos.toByteArray());
- } finally {
- hdos.close();
+ byte[] credentialBytes = getCredentialBytes(cnx, securityProperties);
+ getMessage().addBytesPart(credentialBytes);
+
+ long userId = getUserId(cnx);
+ try (HeapDataOutputStream hdos = new HeapDataOutputStream(16,
KnownVersion.CURRENT)) {
+ hdos.writeLong(cnx.getConnectionID());
+ hdos.writeLong(userId);
+ getMessage().setSecurePart(((ConnectionImpl)
cnx).encryptBytes(hdos.toByteArray()));
}
- getMessage().setSecurePart(secureBytes);
getMessage().send(false);
}
+ protected long getUserId(Connection cnx) {
+ // single user mode
+ if (UserAttributes.userAttributes.get() == null) {
+ return cnx.getServer().getUserId();
+ }
+ // multi user mode
+ Long id =
UserAttributes.userAttributes.get().getServerToId().get(cnx.getServer());
+ if (id == null) {
+ return -1L;
+ }
+ return id;
+ }
+
+ protected InternalDistributedSystem getConnectedSystem() {
+ return InternalDistributedSystem.getConnectedInstance();
+ }
+
+ protected byte[] getCredentialBytes(Connection cnx, Properties
securityProperties)
Review comment:
`connection` instead of `cnx`? I know the old uses abbreviations, but we
really want to escape that trend.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/CacheClientProxy.java
##########
@@ -345,19 +348,15 @@ protected CacheClientProxy(InternalCache cache,
CacheClientNotifier ccn, Socket
initializeClientAuths();
}
- private void initializeClientAuths() {
- if (AcceptorImpl.isPostAuthzCallbackPresent()) {
- clientUserAuths = ServerConnection.getClientUserAuths(proxyID);
- }
+ void initializeClientAuths() {
+ clientUserAuths = ServerConnection.getClientUserAuths(proxyID);
Review comment:
One way to make the use of `CacheClientProxy` more testable is to wrap
the`ServerConnection.getClientUserAuths` calls in a `
java.util.function.Function` or custom inner-interface.
You basically add a new field:
```
private final Function<ClientProxyMembershipID, ClientUserAuths>
getClientUserAuthsFunction;
```
Then update the 2nd `CacheClientProxy` constructor marked with
`@VisibleForTesting` to include one more parameter:
```
MessageDispatcherFactory messageDispatcherFactory,
Function<ClientProxyMembershipID, ClientUserAuths>
getClientUserAuthsFunction) {
```
Change the 1st constructor to pass in a method reference:
```
DEFAULT_MESSAGEDISPATCHERFACTORY,
ServerConnection::getClientUserAuths);
```
And save the the parameter in the 2nd constructor to the field:
```
this.getClientUserAuthsFunction = getClientUserAuthsFunction;
```
And finally use it anywhere the class calls the static
`ServerConnection.getClientUserAuths`:
```
clientUserAuths = getClientUserAuths.apply(proxyID);
```
##########
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</code>'s <code>EventID</code>
Review comment:
Please use the newer syntax `{@code xxx}` instead of `<code>`:
```
* This {@code ClientMessage}'s {@code EventID}
```
##########
File path:
geode-core/src/main/java/org/apache/geode/cache/client/internal/AuthenticateUserOp.java
##########
@@ -123,39 +119,61 @@ 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);
+ if (securityProperties == null) {
+ securityProperties = getConnectedSystem().getSecurityProperties();
}
- try {
- secureBytes = ((ConnectionImpl) cnx).encryptBytes(hdos.toByteArray());
- } finally {
- hdos.close();
+ byte[] credentialBytes = getCredentialBytes(cnx, securityProperties);
+ getMessage().addBytesPart(credentialBytes);
+
+ long userId = getUserId(cnx);
+ try (HeapDataOutputStream hdos = new HeapDataOutputStream(16,
KnownVersion.CURRENT)) {
+ hdos.writeLong(cnx.getConnectionID());
+ hdos.writeLong(userId);
+ getMessage().setSecurePart(((ConnectionImpl)
cnx).encryptBytes(hdos.toByteArray()));
}
- getMessage().setSecurePart(secureBytes);
getMessage().send(false);
}
+ protected long getUserId(Connection cnx) {
Review comment:
Can we name the parameter `connection` instead of `cnx`?
##########
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</code>'s <code>EventID</code>
+ */
+ private final EventID eventId;
+
+ public ClientReAuthenticateMessage() {
+ eventId = new EventID();
+ }
+
+ public ClientReAuthenticateMessage(EventID eventId) {
+ this.eventId = eventId;
+ }
Review comment:
You should really use constructor chaining here:
```
public ClientReAuthenticateMessage() {
this(new EventID());
}
```
--
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]
