tabish121 commented on code in PR #5721:
URL: https://github.com/apache/activemq-artemis/pull/5721#discussion_r2114350476
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/connect/AMQPBrokerConnection.java:
##########
@@ -1218,28 +1251,53 @@ private static final class SaslFactory implements
ClientSASLFactory {
public ClientSASL chooseMechanism(String[] offeredMechanims) {
List<String> availableMechanisms = offeredMechanims == null ?
Collections.emptyList() : Arrays.asList(offeredMechanims);
- if (availableMechanisms.contains(EXTERNAL) &&
ExternalSASLMechanism.isApplicable(connection)) {
+ List<String> acceptedMechanisms =
getAcceptedMechanisms(availableMechanisms);
+
+ 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 List<String> getAcceptedMechanisms(List<String>
availableMechanisms) {
+ TransportConfiguration connectorConfig =
connection.getConnectorConfig();
+ if(connectorConfig == null){
Review Comment:
I believe the formatting in this method breaks checkstyle rules which needs
to be fixed beyond fixing the bit about not putting the sasl bits into the
netty transport options
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/connect/AMQPConnectSaslTest.java:
##########
@@ -115,6 +115,31 @@ public void testConnectsWithPlain() throws Exception {
}
}
+ @Test
Review Comment:
Testing is insufficient as there are routes through the is applicable code
that involved checking if user and passwords are set or not which should be
tested.
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/TransportConstants.java:
##########
@@ -536,6 +538,7 @@ private static int parseDefaultVariable(String
variableName, int defaultValue) {
allowableConnectorKeys.add(TransportConstants.TRUST_MANAGER_FACTORY_PLUGIN_PROP_NAME);
allowableConnectorKeys.add(TransportConstants.HANDSHAKE_TIMEOUT);
allowableConnectorKeys.add(TransportConstants.CRL_PATH_PROP_NAME);
+ allowableConnectorKeys.add(TransportConstants.SASL_MECHANISMS);
Review Comment:
Same issue as before, we should not be adding SASL mechanisms to netty
options as AMQP is the protocol that's implementing the SASL bits, others do
not so the options should be handled as other AMQP specific options are.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
