tabish121 commented on code in PR #5822:
URL: https://github.com/apache/activemq-artemis/pull/5822#discussion_r2193561268
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/connect/AMQPBrokerConnection.java:
##########
@@ -1204,42 +1208,95 @@ public static boolean isApplicable(final
NettyConnection connection) {
}
}
+ private static class XOAuth2SASLMechanism implements ClientSASL {
+
+ private final String userName;
+ private final String token;
+
+ public XOAuth2SASLMechanism(String userName, String token) {
+ this.userName = userName;
+ this.token = token;
+ }
+
+ @Override
+ public String getName() {
+ return XOAUTH2;
+ }
+
+ @Override
+ public byte[] getInitialResponse() {
+ String response = String.format("user=%s\u0001auth=Bearer
%s\u0001\u0001", userName, token);
+ return response.getBytes(StandardCharsets.UTF_8);
+ }
+
+ @Override
+ public byte[] getResponse(byte[] challenge) {
+ return EMPTY;
+ }
+
+ public static boolean isApplicable(final String username, final String
password) {
+ return StringUtils.isNoneEmpty(username, password);
+ }
+ }
+
private static final class SaslFactory implements ClientSASLFactory {
private final NettyConnection connection;
private final AMQPBrokerConnectConfiguration brokerConnectConfiguration;
+ private final String[] availableSaslMechanisms;
- SaslFactory(NettyConnection connection, AMQPBrokerConnectConfiguration
brokerConnectConfiguration) {
+ SaslFactory(NettyConnection connection, AMQPBrokerConnectConfiguration
brokerConnectConfiguration, String[] availableSaslMechanisms) {
this.connection = connection;
this.brokerConnectConfiguration = brokerConnectConfiguration;
+ this.availableSaslMechanisms = availableSaslMechanisms;
}
@Override
- public ClientSASL chooseMechanism(String[] offeredMechanims) {
- List<String> availableMechanisms = offeredMechanims == null ?
Collections.emptyList() : Arrays.asList(offeredMechanims);
+ public ClientSASL chooseMechanism(String[] offeredMechanisms) {
+ Set<String> acceptedMechanisms =
getAcceptedMechanisms(offeredMechanisms, availableSaslMechanisms);
- if (availableMechanisms.contains(EXTERNAL) &&
ExternalSASLMechanism.isApplicable(connection)) {
+ if (acceptedMechanisms.contains(EXTERNAL) &&
ExternalSASLMechanism.isApplicable(connection)) {
return new ExternalSASLMechanism();
}
+
if (SCRAMClientSASL.isApplicable(brokerConnectConfiguration.getUser(),
brokerConnectConfiguration.getPassword())) {
for (SCRAM scram : SCRAM.values()) {
- if (availableMechanisms.contains(scram.getName())) {
+ if (acceptedMechanisms.contains(scram.getName())) {
return new SCRAMClientSASL(scram,
brokerConnectConfiguration.getUser(),
brokerConnectConfiguration.getPassword());
}
}
}
- if (availableMechanisms.contains(PLAIN) &&
PlainSASLMechanism.isApplicable(brokerConnectConfiguration.getUser(),
brokerConnectConfiguration.getPassword())) {
+
+ if (acceptedMechanisms.contains(PLAIN) &&
PlainSASLMechanism.isApplicable(brokerConnectConfiguration.getUser(),
brokerConnectConfiguration.getPassword())) {
return new
PlainSASLMechanism(brokerConnectConfiguration.getUser(),
brokerConnectConfiguration.getPassword());
}
- if (availableMechanisms.contains(ANONYMOUS)) {
+ if (acceptedMechanisms.contains(XOAUTH2) &&
XOAuth2SASLMechanism.isApplicable(brokerConnectConfiguration.getUser(),
brokerConnectConfiguration.getPassword())) {
+ return new
XOAuth2SASLMechanism(brokerConnectConfiguration.getUser(),
brokerConnectConfiguration.getPassword());
+ }
+
+ if (acceptedMechanisms.contains(ANONYMOUS)) {
return new AnonymousSASLMechanism();
}
return null;
}
+
+ private Set<String> getAcceptedMechanisms(String[]
offeredSaslMechanisms, String[] availableSaslMechanisms) {
+ Set<String> offeredSaslMechanismsSet = offeredSaslMechanisms == null
? Set.of() : new HashSet<>(Arrays.asList(offeredSaslMechanisms));
+ Set<String> availableSaslMechanismsSet = availableSaslMechanisms ==
null ? Set.of() : new HashSet<>(Arrays.asList(availableSaslMechanisms));
Review Comment:
This could be refactored to not allocate the enabled set of mechanisms
unless the array is non-null and not empty as it would be most of the time. It
also wouldn't be necessary to create yet another Set for the result if instead
of `Set.of()` for the offered set you used Collections.emptySet() which will
just no-op on retainAll as it is treated as empty.
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/connect/AMQPBrokerConnection.java:
##########
@@ -1204,42 +1208,95 @@ public static boolean isApplicable(final
NettyConnection connection) {
}
}
+ private static class XOAuth2SASLMechanism implements ClientSASL {
+
+ private final String userName;
+ private final String token;
+
+ public XOAuth2SASLMechanism(String userName, String token) {
+ this.userName = userName;
+ this.token = token;
+ }
+
+ @Override
+ public String getName() {
+ return XOAUTH2;
+ }
+
+ @Override
+ public byte[] getInitialResponse() {
+ String response = String.format("user=%s\u0001auth=Bearer
%s\u0001\u0001", userName, token);
+ return response.getBytes(StandardCharsets.UTF_8);
+ }
+
+ @Override
+ public byte[] getResponse(byte[] challenge) {
+ return EMPTY;
+ }
+
+ public static boolean isApplicable(final String username, final String
password) {
+ return StringUtils.isNoneEmpty(username, password);
+ }
+ }
+
private static final class SaslFactory implements ClientSASLFactory {
private final NettyConnection connection;
private final AMQPBrokerConnectConfiguration brokerConnectConfiguration;
+ private final String[] availableSaslMechanisms;
- SaslFactory(NettyConnection connection, AMQPBrokerConnectConfiguration
brokerConnectConfiguration) {
+ SaslFactory(NettyConnection connection, AMQPBrokerConnectConfiguration
brokerConnectConfiguration, String[] availableSaslMechanisms) {
this.connection = connection;
this.brokerConnectConfiguration = brokerConnectConfiguration;
+ this.availableSaslMechanisms = availableSaslMechanisms;
}
@Override
- public ClientSASL chooseMechanism(String[] offeredMechanims) {
- List<String> availableMechanisms = offeredMechanims == null ?
Collections.emptyList() : Arrays.asList(offeredMechanims);
+ public ClientSASL chooseMechanism(String[] offeredMechanisms) {
+ Set<String> acceptedMechanisms =
getAcceptedMechanisms(offeredMechanisms, availableSaslMechanisms);
- if (availableMechanisms.contains(EXTERNAL) &&
ExternalSASLMechanism.isApplicable(connection)) {
+ if (acceptedMechanisms.contains(EXTERNAL) &&
ExternalSASLMechanism.isApplicable(connection)) {
return new ExternalSASLMechanism();
}
+
if (SCRAMClientSASL.isApplicable(brokerConnectConfiguration.getUser(),
brokerConnectConfiguration.getPassword())) {
for (SCRAM scram : SCRAM.values()) {
- if (availableMechanisms.contains(scram.getName())) {
+ if (acceptedMechanisms.contains(scram.getName())) {
return new SCRAMClientSASL(scram,
brokerConnectConfiguration.getUser(),
brokerConnectConfiguration.getPassword());
}
}
}
- if (availableMechanisms.contains(PLAIN) &&
PlainSASLMechanism.isApplicable(brokerConnectConfiguration.getUser(),
brokerConnectConfiguration.getPassword())) {
+
+ if (acceptedMechanisms.contains(PLAIN) &&
PlainSASLMechanism.isApplicable(brokerConnectConfiguration.getUser(),
brokerConnectConfiguration.getPassword())) {
return new
PlainSASLMechanism(brokerConnectConfiguration.getUser(),
brokerConnectConfiguration.getPassword());
}
- if (availableMechanisms.contains(ANONYMOUS)) {
+ if (acceptedMechanisms.contains(XOAUTH2) &&
XOAuth2SASLMechanism.isApplicable(brokerConnectConfiguration.getUser(),
brokerConnectConfiguration.getPassword())) {
+ return new
XOAuth2SASLMechanism(brokerConnectConfiguration.getUser(),
brokerConnectConfiguration.getPassword());
+ }
+
+ if (acceptedMechanisms.contains(ANONYMOUS)) {
return new AnonymousSASLMechanism();
}
return null;
}
+
+ private Set<String> getAcceptedMechanisms(String[]
offeredSaslMechanisms, String[] availableSaslMechanisms) {
Review Comment:
As Dom pointed out above using enabledMechanisms probably makes it more
clear what's being filtered for in the offered set.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/connect/AMQPConnectSaslTest.java:
##########
@@ -115,6 +117,81 @@ public void testConnectsWithPlain() throws Exception {
}
}
+ @Test
+ @Timeout(20)
+ public void testConnectsWithXOauth2() throws Exception {
+ try (ProtonTestServer peer = new ProtonTestServer()) {
+ peer.expectSaslXOauth2Connect(USER, PASSWD);
+ peer.expectOpen().respond();
+ peer.expectBegin().respond();
+ peer.start();
+
+ final URI remoteURI = peer.getServerURI();
+ logger.debug("Connect test started, peer listening on: {}",
remoteURI);
+
+ AMQPBrokerConnectConfiguration amqpConnection =
+ new AMQPBrokerConnectConfiguration(getTestName(),
"tcp://localhost:" + remoteURI.getPort() + "?saslMechanisms=" + XOAUTH2);
+ amqpConnection.setReconnectAttempts(0);// No reconnects
+ amqpConnection.setUser(USER);
+ amqpConnection.setPassword(PASSWD);
+
+ server.getConfiguration().addAMQPConnection(amqpConnection);
+ server.start();
+
+ peer.waitForScriptToComplete(5, TimeUnit.SECONDS);
+ }
+ }
+
+ @Test
+ @Timeout(20)
+ public void testConnectsWithXOauth2WithoutUserAndPassword() throws
Exception {
Review Comment:
suggest name change to
`testXOAuth2NotSelectedWhenUserAndPasswordNotProvided` to better describe the
test
--
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: gitbox-unsubscr...@activemq.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@activemq.apache.org
For additional commands, e-mail: gitbox-h...@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
