PROTON-2781 Cleanups in the SASL layer API docs and some code tidying

Add some details in API docs and improve some error messages and code.

protonj2/src/main/java/org/apache/qpid/protonj2/engine/Engine.java

@@ -232,7 +232,8 @@ default void accept(ProtonBuffer input) throws EngineStateException {
      * default no-op driver must be returned that indicates this.  The SASL driver provides
      * the engine with client and server side SASL handshaking support.  An {@link Engine}
      * implementation can support pluggable SASL drivers or exert tight control over the
-     * driver as it sees fit.
+     * driver as it sees fit. The engine implementation in use will dictate how a SASL
+     * driver is assigned or discovered.
      *
      * @return the SASL driver for the engine.
      */

protonj2/src/main/java/org/apache/qpid/protonj2/engine/EngineSaslDriver.java

@@ -30,10 +30,10 @@
  */
 public interface EngineSaslDriver {
 
-	/**
-	 * The SASL driver state used to determine at what point the current SASL negotiation process
-	 * is currently in.  If the state is 'none' then no SASL negotiations will be performed.
-	 */
+    /**
+     * The SASL driver state used to determine at what point the current SASL negotiation process
+     * is currently in.  If the state is 'none' then no SASL negotiations will be performed.
+     */
     public enum SaslState {
 
         /**
@@ -116,6 +116,11 @@ public enum SaslState {
 
     /**
      * Set the maximum frame size the remote can send before an error is indicated.
+     * <p>
+     * The AMQP specification defines a default of 512 bytes for this value however in
+     * some cases this value is to small for the data needed in some SASL mechanisms.
+     * This allows the user to configure a larger acceptable value, the lowest possible
+     * value remains the default 512 bytes.
      *
      * @param maxFrameSize
      *      The maximum allowed frame size from the remote sender.

protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/SaslContext.java

@@ -76,6 +76,11 @@ enum Role { CLIENT, SERVER }
     Role getRole();
 
     /**
+     * Returns if the SASL context has been marked as completed. A completed context
+     * does not imply the SASL authentication was successful, the caller should check
+     * the state of the {@link #getSaslOutcome()} value to determine if the SASL
+     * authentication process was successful or not.
+     *
      * @return true if SASL authentication has completed
      */
     boolean isDone();

protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/SaslSystemException.java

@@ -27,15 +27,14 @@
 public class SaslSystemException extends SaslException {
 
     private static final long serialVersionUID = 1L;
+
     private final boolean permanent;
 
     /**
      * Creates an exception indicating a system error.
      *
      * @param permanent
-     *        {@code true} if the error is permanent and requires (manual)
-     *        intervention.
-     *
+     * 		<code>true</code> if the error is permanent and requires (manual) intervention
      */
     public SaslSystemException(boolean permanent) {
         this(permanent, null);
@@ -55,10 +54,12 @@ public SaslSystemException(boolean permanent, String detail) {
     }
 
     /**
-     * Checks if the condition that caused this exception is of a permanent
-     * nature.
+     * Checks if the condition that caused this exception is of a permanent nature.
+     * <p>
+     * A permanent error should be treated as terminal for transports that implement
+     * reconnection logic and the implementation should stop attempting connections.
      *
-     * @return {@code true} if the error condition is permanent.
+     * @return <code>true</code> if the error condition is permanent.
      */
     public final boolean isPermanent() {
         return permanent;

protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/client/AbstractScramSHAMechanism.java

@@ -20,7 +20,6 @@
 import java.security.InvalidKeyException;
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
-import java.util.Arrays;
 import java.util.Base64;
 
 import javax.crypto.Mac;
@@ -112,7 +111,7 @@ public void verifyCompletion() throws SaslException {
 
         if (state != State.COMPLETE) {
             throw new SaslException(String.format(
-                "SASL exchange was not fully completed.  Expected state %s but actual state %s", State.COMPLETE, state));
+                "SASL exchange was not fully completed. Expected state %s but actual state %s", State.COMPLETE, state));
         }
     }
 
@@ -186,13 +185,13 @@ private void evaluateOutcome(ProtonBuffer challenge) throws SaslException {
         String[] parts = serverFinalMessage.split(",");
 
         if (!parts[0].startsWith("v=")) {
-            throw new SaslException("Server final message did not contain verifier");
+            throw new SaslException("Server final message did not contain expected verifier");
         }
 
         byte[] serverSignature = Base64.getDecoder().decode(parts[0].substring(2));
 
-        if (!Arrays.equals(this.serverSignature, serverSignature)) {
-            throw new SaslException("Server signature did not match");
+        if (!MessageDigest.isEqual(this.serverSignature, serverSignature)) {
+            throw new SaslException("Server signature did not match expected signature.");
         }
     }
 

protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/client/SaslCredentialsProvider.java

@@ -42,6 +42,10 @@ public interface SaslCredentialsProvider {
     String password();
 
     /**
+     * Returns the local principal for use in SASL authentication which is generally
+     * provided by the IO layer (TLS). This method can return null if there is no
+     * local {@link Principal} in use.
+     *
      * @return the local principal value to use when performing SASL authentication.
      */
     Principal localPrincipal();

protonj2/src/main/java/org/apache/qpid/protonj2/engine/sasl/client/SaslMechanisms.java

@@ -155,7 +155,7 @@ public static SaslMechanisms valueOf(Symbol mechanism) {
             }
         }
 
-        throw new IllegalArgumentException("No Matching SASL Mechanism with name: " + mechanism.toString());
+        throw new IllegalArgumentException("No Matching SASL Mechanism with name: " + mechanism);
     }
 
     /**

protonj2/src/test/java/org/apache/qpid/protonj2/engine/sasl/client/AbstractScramSHAMechanismTestBase.java

@@ -17,7 +17,7 @@
 package org.apache.qpid.protonj2.engine.sasl.client;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.fail;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import javax.security.sasl.SaslException;
 
@@ -75,12 +75,7 @@ public void testServerFirstMessageMalformed() throws Exception {
         ProtonBuffer challenge = ProtonBufferAllocator.defaultAllocator().copy("badserverfirst".getBytes());
         challenge.setWriteOffset(challenge.capacity());
 
-        try {
-            mechanism.getChallengeResponse(getTestCredentials(), challenge);
-            fail("Exception not thrown");
-        } catch (SaslException s) {
-            // PASS
-        }
+        assertThrows(SaslException.class, () -> mechanism.getChallengeResponse(getTestCredentials(), challenge));
     }
 
     /**
@@ -101,12 +96,7 @@ public void testServerFirstMessageMandatoryExtensionRejected() throws Exception
         ProtonBuffer challenge = ProtonBufferAllocator.defaultAllocator().copy("m=notsupported,s=,i=".getBytes());
         challenge.setWriteOffset(challenge.capacity());
 
-        try {
-            mechanism.getChallengeResponse(getTestCredentials(), challenge);
-            fail("Exception not thrown");
-        } catch (SaslException s) {
-            // PASS
-        }
+        assertThrows(SaslException.class, () -> mechanism.getChallengeResponse(getTestCredentials(), challenge));
     }
 
     /**
@@ -127,12 +117,7 @@ public void testServerFirstMessageInvalidNonceRejected() throws Exception {
             "r=invalidnonce,s=W22ZaJ0SNY7soEsUEjb6gQ==,i=4096".getBytes());
         challenge.setWriteOffset(challenge.capacity());
 
-        try {
-            mechanism.getChallengeResponse(getTestCredentials(), challenge);
-            fail("Exception not thrown");
-        } catch (SaslException s) {
-            // PASS
-        }
+        assertThrows(SaslException.class, () -> mechanism.getChallengeResponse(getTestCredentials(), challenge));
     }
 
     /**
@@ -155,12 +140,7 @@ public void testServerSignatureDiffer() throws Exception {
         ProtonBuffer challenge = ProtonBufferAllocator.defaultAllocator().copy("v=badserverfinal".getBytes());
         challenge.setWriteOffset(challenge.capacity());
 
-        try {
-            mechanism.getChallengeResponse(getTestCredentials(), challenge);
-            fail("Exception not thrown");
-        } catch (SaslException e) {
-            // PASS
-        }
+        assertThrows(SaslException.class, () -> mechanism.getChallengeResponse(getTestCredentials(), challenge));
     }
 
     @Test
@@ -173,11 +153,6 @@ public void testIncompleteExchange() throws Exception {
         ProtonBuffer clientFinalMessage = mechanism.getChallengeResponse(getTestCredentials(), serverFirstMessage);
         assertEquals(expectedClientFinalMessage, clientFinalMessage);
 
-        try {
-            mechanism.verifyCompletion();
-            fail("Exception not thrown");
-        } catch (SaslException e) {
-            // PASS
-        }
+        assertThrows(SaslException.class, () -> mechanism.verifyCompletion());
     }
 }