Remove resource binding out of sasl auth

Follow XEP-170 recommendations: Resource binding *after* compression.

Fixes also a bunch of race conditions related to the wait()/notify()
pattern used in the early stages of a Connection where
createPacketCollectorAndSend(Packet).nextResultOrThrow() can not be
used. Those wait()/notify() patterns are currently
- SASL authentication
- compression
- resource binding

Fixes SMACK-454

bosh/src/main/java/org/jivesoftware/smack/BOSHPacketReader.java

@@ -148,10 +148,10 @@ public class BOSHPacketReader implements BOSHClientResponseListener {
                 } else if (parser.getName().equals("bind")) {
                     // The server requires the client to bind a resource to the
                     // stream
-                    connection.getSASLAuthentication().bindingRequired();
+                    connection.serverRequiresBinding();
                 } else if (parser.getName().equals("session")) {
                     // The server supports sessions
-                    connection.getSASLAuthentication().sessionsSupported();
+                    connection.serverSupportsSession();
                 } else if (parser.getName().equals("register")) {
                     AccountManager.getInstance(connection).setSupportsAccountCreation(true);
                 }

bosh/src/main/java/org/jivesoftware/smack/XMPPBOSHConnection.java

@@ -135,7 +135,8 @@ public class XMPPBOSHConnection extends XMPPConnection {
         this.config = config;
     }
 
-    public void connect() throws SmackException {
+    @Override
+    void connectInternal() throws SmackException {
         if (connected) {
             throw new IllegalStateException("Already connected to a server.");
         }
@@ -146,7 +147,6 @@ public class XMPPBOSHConnection extends XMPPConnection {
                 client.close();
                 client = null;
             }
-            saslAuthentication.init();
             sessionID = null;
             authID = null;
 
@@ -250,18 +250,18 @@ public class XMPPBOSHConnection extends XMPPConnection {
         // Do partial version of nameprep on the username.
         username = username.toLowerCase(Locale.US).trim();
 
-        String response;
         if (saslAuthentication.hasNonAnonymousAuthentication()) {
             // Authenticate using SASL
             if (password != null) {
-                response = saslAuthentication.authenticate(username, password, resource);
+                 saslAuthentication.authenticate(username, password, resource);
             } else {
-                response = saslAuthentication.authenticate(resource, config.getCallbackHandler());
+                saslAuthentication.authenticate(resource, config.getCallbackHandler());
             }
         } else {
             throw new SaslException("No non-anonymous SASL authentication mechanism available");
         }
 
+        String response = bindResourceAndEstablishSession(resource);
         // Set the user.
         if (response != null) {
             this.user = response;
@@ -303,15 +303,15 @@ public class XMPPBOSHConnection extends XMPPConnection {
             throw new AlreadyLoggedInException();
         }
 
-        String response;
         if (saslAuthentication.hasAnonymousAuthentication()) {
-            response = saslAuthentication.authenticateAnonymously();
+            saslAuthentication.authenticateAnonymously();
         }
         else {
             // Authenticate using Non-SASL
             throw new SaslException("No anonymous SASL authentication mechanism available");
         }
 
+        String response = bindResourceAndEstablishSession(null);
         // Set the user value.
         this.user = response;
         // Update the serviceName with the one returned by the server

core/src/main/java/org/jivesoftware/smack/SASLAuthentication.java

@@ -21,11 +21,16 @@ import org.jivesoftware.smack.SmackException.NoResponseException;
 import org.jivesoftware.smack.SmackException.NotConnectedException;
 import org.jivesoftware.smack.SmackException.ResourceBindingNotOfferedException;
 import org.jivesoftware.smack.XMPPException.XMPPErrorException;
-import org.jivesoftware.smack.packet.Bind;
 import org.jivesoftware.smack.packet.Packet;
-import org.jivesoftware.smack.packet.Session;
+import org.jivesoftware.smack.sasl.SASLAnonymous;
-import org.jivesoftware.smack.sasl.*;
+import org.jivesoftware.smack.sasl.SASLCramMD5Mechanism;
+import org.jivesoftware.smack.sasl.SASLDigestMD5Mechanism;
+import org.jivesoftware.smack.sasl.SASLErrorException;
+import org.jivesoftware.smack.sasl.SASLExternalMechanism;
+import org.jivesoftware.smack.sasl.SASLGSSAPIMechanism;
+import org.jivesoftware.smack.sasl.SASLMechanism;
 import org.jivesoftware.smack.sasl.SASLMechanism.SASLFailure;
+import org.jivesoftware.smack.sasl.SASLPlainMechanism;
 
 import javax.security.auth.callback.CallbackHandler;
 import javax.security.sasl.SaslException;
@@ -80,8 +85,6 @@ public class SASLAuthentication {
      */
     private boolean saslNegotiated;
 
-    private boolean resourceBinded;
-    private boolean sessionSupported;
     /**
      * The SASL related error condition if there was one provided by the server.
      */
@@ -211,7 +214,6 @@ public class SASLAuthentication {
      *
      * @param resource the desired resource.
      * @param cbh the CallbackHandler used to get information from the user
-     * @return the full JID provided by the server while binding a resource to the connection.
      * @throws IOException 
      * @throws XMPPErrorException 
      * @throws NoResponseException 
@@ -219,7 +221,7 @@ public class SASLAuthentication {
      * @throws ResourceBindingNotOfferedException
      * @throws NotConnectedException 
      */
-    public String authenticate(String resource, CallbackHandler cbh) throws IOException,
+    public void authenticate(String resource, CallbackHandler cbh) throws IOException,
                     NoResponseException, XMPPErrorException, SASLErrorException, ResourceBindingNotOfferedException, NotConnectedException {
         // Locate the SASLMechanism to use
         String selectedMechanism = null;
@@ -245,22 +247,19 @@ public class SASLAuthentication {
                                 e);
             }
 
+            synchronized (this) {
                 // Trigger SASL authentication with the selected mechanism. We use
                 // connection.getHost() since GSAPI requires the FQDN of the server, which
                 // may not match the XMPP domain.
                 currentMechanism.authenticate(connection.getHost(), cbh);
-
-            // Wait until SASL negotiation finishes
-            synchronized (this) {
-                if (!saslNegotiated && saslFailure == null) {
                 try {
-                        wait(30000);
+                    // Wait until SASL negotiation finishes
+                    wait(connection.getPacketReplyTimeout());
                 }
                 catch (InterruptedException e) {
                     // Ignore
                 }
             }
-            }
 
             if (saslFailure != null) {
                 // SASL authentication failed and the server may have closed the connection
@@ -268,14 +267,9 @@ public class SASLAuthentication {
                 throw new SASLErrorException(selectedMechanism, saslFailure);
             }
 
-            if (saslNegotiated) {
+            if (!saslNegotiated) {
-                // Bind a resource for this connection and
-                return bindResourceAndEstablishSession(resource);
-            }
-            else {
                 throw new NoResponseException();
             }
-
         }
         else {
             throw new SaslException(
@@ -294,14 +288,13 @@ public class SASLAuthentication {
      * @param username the username that is authenticating with the server.
      * @param password the password to send to the server.
      * @param resource the desired resource.
-     * @return the full JID provided by the server while binding a resource to the connection.
      * @throws XMPPErrorException 
      * @throws SASLErrorException 
      * @throws IOException 
      * @throws SaslException 
      * @throws SmackException 
      */
-    public String authenticate(String username, String password, String resource)
+    public void authenticate(String username, String password, String resource)
                     throws XMPPErrorException, SASLErrorException, SaslException, IOException,
                     SmackException {
         // Locate the SASLMechanism to use
@@ -326,26 +319,27 @@ public class SASLAuthentication {
                                 e);
             }
 
+            synchronized (this) {
                 // Trigger SASL authentication with the selected mechanism. We use
                 // connection.getHost() since GSAPI requires the FQDN of the server, which
                 // may not match the XMPP domain.
 
-            // The serviceName is basically the value that XMPP server sends to the client as being
+                // The serviceName is basically the value that XMPP server sends to the client as
-            // the location of the XMPP service we are trying to connect to. This should have the
+                // being
+                // the location of the XMPP service we are trying to connect to. This should have
+                // the
                 // format: host ["/" serv-name ] as per RFC-2831 guidelines
                 String serviceName = connection.getServiceName();
                 currentMechanism.authenticate(username, connection.getHost(), serviceName, password);
 
-            // Wait until SASL negotiation finishes
-            synchronized (this) {
-                if (!saslNegotiated && saslFailure == null) {
                 try {
-                        wait(30000);
+                    // Wait until SASL negotiation finishes
+                    wait(connection.getPacketReplyTimeout());
                 }
                 catch (InterruptedException e) {
                     // Ignore
                 }
-                }
+
             }
 
             if (saslFailure != null) {
@@ -354,11 +348,7 @@ public class SASLAuthentication {
                 throw new SASLErrorException(selectedMechanism, saslFailure);
             }
 
-            if (saslNegotiated) {
+            if (!saslNegotiated) {
-                // Bind a resource for this connection and
-                return bindResourceAndEstablishSession(resource);
-            }
-            else {
                 throw new NoResponseException();
             }
         }
@@ -376,28 +366,26 @@ public class SASLAuthentication {
      * The server will assign a full JID with a randomly generated resource and possibly with
      * no username.
      *
-     * @return the full JID provided by the server while binding a resource to the connection.
      * @throws SASLErrorException 
      * @throws IOException 
      * @throws SaslException 
      * @throws XMPPErrorException if an error occures while authenticating.
      * @throws SmackException if there was no response from the server.
      */
-    public String authenticateAnonymously() throws SASLErrorException, SaslException, IOException, SmackException, XMPPErrorException {
+    public void authenticateAnonymously() throws SASLErrorException, SaslException, IOException,
+                    SmackException, XMPPErrorException {
         currentMechanism = new SASLAnonymous(this);
-            currentMechanism.authenticate(null,null,null,"");
 
         // Wait until SASL negotiation finishes
         synchronized (this) {
-                if (!saslNegotiated && saslFailure == null) {
+            currentMechanism.authenticate(null, null, null, "");
             try {
-                        wait(5000);
+                wait(connection.getPacketReplyTimeout());
             }
             catch (InterruptedException e) {
                 // Ignore
             }
         }
-            }
 
         if (saslFailure != null) {
             // SASL authentication failed and the server may have closed the connection
@@ -405,48 +393,11 @@ public class SASLAuthentication {
             throw new SASLErrorException(currentMechanism.toString(), saslFailure);
         }
 
-            if (saslNegotiated) {
+        if (!saslNegotiated) {
-                // Bind a resource for this connection and
-                return bindResourceAndEstablishSession(null);
-            }
-            else {
             throw new NoResponseException();
         }
     }
 
-    private String bindResourceAndEstablishSession(String resource) throws XMPPErrorException,
-                    ResourceBindingNotOfferedException, NoResponseException, NotConnectedException {
-        // Wait until server sends response containing the <bind> element
-        synchronized (this) {
-            if (!resourceBinded) {
-                try {
-                    wait(30000);
-                }
-                catch (InterruptedException e) {
-                    // Ignore
-                }
-            }
-        }
-
-        if (!resourceBinded) {
-            // Server never offered resource binding, which is REQURIED in XMPP client and server
-            // implementations as per RFC6120 7.2
-            throw new ResourceBindingNotOfferedException();
-        }
-
-        Bind bindResource = new Bind();
-        bindResource.setResource(resource);
-
-        Bind response = (Bind) connection.createPacketCollectorAndSend(bindResource).nextResultOrThrow();
-        String userJID = response.getJid();
-
-        if (sessionSupported && !connection.getConfiguration().isLegacySessionDisabled()) {
-            Session session = new Session();
-            connection.createPacketCollectorAndSend(session).nextResultOrThrow();
-        }
-        return userJID;
-    }
-
     /**
      * Sets the available SASL mechanism reported by the server. The server will report the
      * available SASL mechanism once the TLS negotiation was successful. This information is
@@ -486,11 +437,13 @@ public class SASLAuthentication {
      * Notification message saying that SASL authentication was successful. The next step
      * would be to bind the resource.
      */
-    synchronized void authenticated() {
+    void authenticated() {
         saslNegotiated = true;
         // Wake up the thread that is waiting in the #authenticate method
+        synchronized (this) {
             notify();
         }
+    }
 
     /**
      * Notification message saying that SASL authentication has failed. The server may have
@@ -499,34 +452,18 @@ public class SASLAuthentication {
      * @param saslFailure the SASL failure as reported by the server
      * @see <a href="https://tools.ietf.org/html/rfc6120#section-6.5">RFC6120 6.5</a>
      */
-    synchronized void authenticationFailed(SASLFailure saslFailure) {
+    void authenticationFailed(SASLFailure saslFailure) {
         this.saslFailure = saslFailure;
         // Wake up the thread that is waiting in the #authenticate method
+        synchronized (this) {
             notify();
         }
-
-    /**
-     * Notification message saying that the server requires the client to bind a
-     * resource to the stream.
-     */
-    synchronized void bindingRequired() {
-        resourceBinded = true;
-        // Wake up the thread that is waiting in the #authenticate method
-        notify();
     }
 
     public void send(Packet stanza) throws NotConnectedException {
         connection.sendPacket(stanza);
     }
 
-    /**
-     * Notification message saying that the server supports sessions. When a server supports
-     * sessions the client needs to send a Session packet after successfully binding a resource
-     * for the session.
-     */
-    void sessionsSupported() {
-        sessionSupported = true;
-    }
     
     /**
      * Initializes the internal state in order to be able to be reused. The authentication
@@ -536,7 +473,5 @@ public class SASLAuthentication {
     protected void init() {
         saslNegotiated = false;
         saslFailure = null;
-        resourceBinded = false;
-        sessionSupported = false;
     }
 }

core/src/main/java/org/jivesoftware/smack/XMPPConnection.java

@@ -44,13 +44,17 @@ import javax.security.sasl.SaslException;
 import org.jivesoftware.smack.SmackException.NoResponseException;
 import org.jivesoftware.smack.SmackException.NotConnectedException;
 import org.jivesoftware.smack.SmackException.ConnectionException;
+import org.jivesoftware.smack.SmackException.ResourceBindingNotOfferedException;
+import org.jivesoftware.smack.XMPPException.XMPPErrorException;
 import org.jivesoftware.smack.compression.XMPPInputOutputStream;
 import org.jivesoftware.smack.debugger.SmackDebugger;
 import org.jivesoftware.smack.filter.IQReplyFilter;
 import org.jivesoftware.smack.filter.PacketFilter;
+import org.jivesoftware.smack.packet.Bind;
 import org.jivesoftware.smack.packet.IQ;
 import org.jivesoftware.smack.packet.Packet;
 import org.jivesoftware.smack.packet.Presence;
+import org.jivesoftware.smack.packet.Session;
 
 /**
  * The abstract XMPPConnection class provides an interface for connections to a XMPP server and
@@ -90,6 +94,7 @@ import org.jivesoftware.smack.packet.Presence;
  * @author Matt Tucker
  * @author Guenther Niess
  */
+@SuppressWarnings("javadoc")
 public abstract class XMPPConnection {
     private static final Logger LOGGER = Logger.getLogger(XMPPConnection.class.getName());
     
@@ -210,6 +215,9 @@ public abstract class XMPPConnection {
      */
     private int port;
 
+    private Boolean bindingRequired;
+    private boolean sessionSupported;
+
     /**
      * Create an executor to deliver incoming packets to listeners. We'll use a single thread with an unbounded queue.
      */
@@ -334,17 +342,37 @@ public abstract class XMPPConnection {
     /**
      * Establishes a connection to the XMPP server and performs an automatic login
      * only if the previous connection state was logged (authenticated). It basically
-     * creates and maintains a connection to the server.<p>
+     * creates and maintains a connection to the server.
-     * <p/>
+     * <p>
-     * Listeners will be preserved from a previous connection if the reconnection
+     * Listeners will be preserved from a previous connection.
-     * occurs after an abrupt termination.
      * 
      * @throws XMPPException if an error occurs on the XMPP protocol level.
-     * @throws SmackException if an error occurs somehwere else besides XMPP protocol level.
+     * @throws SmackException if an error occurs somewhere else besides XMPP protocol level.
      * @throws IOException 
      * @throws ConnectionException with detailed information about the failed connection.
      */
-    public abstract void connect() throws SmackException, IOException, XMPPException;
+    public void connect() throws SmackException, IOException, XMPPException {
+        saslAuthentication.init();
+        bindingRequired = false;
+        sessionSupported = false;
+        connectInternal();
+    }
+
+    /**
+     * Abstract method that concrete subclasses of XMPPConnection need to implement to perform their
+     * way of XMPP connection establishment. Implementations must guarantee that this method will
+     * block until the last features stanzas has been parsed and the features have been reported
+     * back to XMPPConnection (e.g. by calling @{link {@link XMPPConnection#serverRequiresBinding()}
+     * and such).
+     * <p>
+     * Also implementations are required to perform an automatic login if the previous connection
+     * state was logged (authenticated).
+     *
+     * @throws SmackException
+     * @throws IOException
+     * @throws XMPPException
+     */
+    abstract void connectInternal() throws SmackException, IOException, XMPPException;
 
     /**
      * Logs in to the server using the strongest authentication mode supported by
@@ -418,6 +446,60 @@ public abstract class XMPPConnection {
      */
     public abstract void loginAnonymously() throws XMPPException, SmackException, SaslException, IOException;
 
+    /**
+     * Notification message saying that the server requires the client to bind a
+     * resource to the stream.
+     */
+    void serverRequiresBinding() {
+        bindingRequired = true;
+        // Wake up the thread that is waiting
+        synchronized(bindingRequired) {
+            bindingRequired.notify();
+        }
+    }
+
+    /**
+     * Notification message saying that the server supports sessions. When a server supports
+     * sessions the client needs to send a Session packet after successfully binding a resource
+     * for the session.
+     */
+    void serverSupportsSession() {
+        sessionSupported = true;
+    }
+
+    String bindResourceAndEstablishSession(String resource) throws XMPPErrorException,
+                    ResourceBindingNotOfferedException, NoResponseException, NotConnectedException {
+        synchronized (bindingRequired) {
+            if (!bindingRequired) {
+                try {
+                    // Wait until server sends response containing the <bind> element
+                    bindingRequired.wait(getPacketReplyTimeout());
+                }
+                catch (InterruptedException e) {
+                    // Ignore
+                }
+            }
+        }
+
+        if (!bindingRequired) {
+            // Server never offered resource binding, which is REQURIED in XMPP client and server
+            // implementations as per RFC6120 7.2
+            throw new ResourceBindingNotOfferedException();
+        }
+
+        Bind bindResource = new Bind();
+        bindResource.setResource(resource);
+
+        Bind response = (Bind) createPacketCollectorAndSend(bindResource).nextResultOrThrow();
+        String userJID = response.getJid();
+
+        if (sessionSupported && !getConfiguration().isLegacySessionDisabled()) {
+            Session session = new Session();
+            createPacketCollectorAndSend(session).nextResultOrThrow();
+        }
+        return userJID;
+    }
+
     /**
      * Sends the specified packet to the server.
      * 

core/src/test/java/org/jivesoftware/smack/DummyConnection.java

@@ -75,7 +75,7 @@ public class DummyConnection extends XMPPConnection {
     }
 
     @Override
-    public void connect() {
+    void connectInternal() {
         connectionID = "dummy-" + new Random(new Date().getTime()).nextInt();
 
         if (reconnect) {

tcp/src/main/java/org/jivesoftware/smack/PacketReader.java

@@ -308,7 +308,7 @@ class PacketReader {
                 }
                 else if (parser.getName().equals("bind")) {
                     // The server requires the client to bind a resource to the stream
-                    connection.getSASLAuthentication().bindingRequired();
+                    connection.serverRequiresBinding();
                 }
                 // Set the entity caps node for the server if one is send
                 // See http://xmpp.org/extensions/xep-0115.html#stream
@@ -325,7 +325,7 @@ class PacketReader {
                 }
                 else if (parser.getName().equals("session")) {
                     // The server supports sessions
-                    connection.getSASLAuthentication().sessionsSupported();
+                    connection.serverSupportsSession();
                 }
                 else if (parser.getName().equals("ver")) {
                     if (parser.getNamespace().equals("urn:xmpp:features:rosterver")) {

tcp/src/main/java/org/jivesoftware/smack/XMPPTCPConnection.java

@@ -108,6 +108,10 @@ public class XMPPTCPConnection extends XMPPConnection {
      */
     private boolean serverAckdCompression = false;
 
+    /**
+     * Lock for the wait()/notify() pattern for the compression negotiation
+     */
+    private final Object compressionLock = new Object();
 
     /**
      * Creates a new connection to the specified XMPP server. A DNS SRV lookup will be
@@ -230,20 +234,26 @@ public class XMPPTCPConnection extends XMPPConnection {
         // Do partial version of nameprep on the username.
         username = username.toLowerCase(Locale.US).trim();
 
-        String response;
         if (saslAuthentication.hasNonAnonymousAuthentication()) {
             // Authenticate using SASL
             if (password != null) {
-                response = saslAuthentication.authenticate(username, password, resource);
+                saslAuthentication.authenticate(username, password, resource);
             }
             else {
-                response = saslAuthentication.authenticate(resource, config.getCallbackHandler());
+                saslAuthentication.authenticate(resource, config.getCallbackHandler());
             }
         } else {
             throw new SaslException("No non-anonymous SASL authentication mechanism available");
         }
 
+        // If compression is enabled then request the server to use stream compression. XEP-170
+        // recommends to perform stream compression before resource binding.
+        if (config.isCompressionEnabled()) {
+            useCompression();
+        }
+
         // Set the user.
+        String response = bindResourceAndEstablishSession(resource);
         if (response != null) {
             this.user = response;
             // Update the serviceName with the one returned by the server
@@ -256,11 +266,6 @@ public class XMPPTCPConnection extends XMPPConnection {
             }
         }
 
-        // If compression is enabled then request the server to use stream compression
-        if (config.isCompressionEnabled()) {
-            useCompression();
-        }
-
         // Indicate that we're now authenticated.
         authenticated = true;
         anonymous = false;
@@ -292,14 +297,14 @@ public class XMPPTCPConnection extends XMPPConnection {
             throw new AlreadyLoggedInException();
         }
 
-        String response;
         if (saslAuthentication.hasAnonymousAuthentication()) {
-            response = saslAuthentication.authenticateAnonymously();
+            saslAuthentication.authenticateAnonymously();
         }
         else {
             throw new SaslException("No anonymous SASL authentication mechanism available");
         }
 
+        String response = bindResourceAndEstablishSession(null);
         // Set the user value.
         this.user = response;
         // Update the serviceName with the one returned by the server
@@ -396,8 +401,6 @@ public class XMPPTCPConnection extends XMPPConnection {
 
         reader = null;
         writer = null;
-
-        saslAuthentication.init();
     }
 
     public synchronized void disconnect(Presence unavailablePresence) {
@@ -786,11 +789,11 @@ public class XMPPTCPConnection extends XMPPConnection {
         }
 
         if ((compressionHandler = maybeGetCompressionHandler()) != null) {
-            synchronized (this) {
+            synchronized (compressionLock) {
                 requestStreamCompression(compressionHandler.getCompressionMethod());
                 // Wait until compression is being used or a timeout happened
                 try {
-                    wait(getPacketReplyTimeout());
+                    compressionLock.wait(getPacketReplyTimeout());
                 }
                 catch (InterruptedException e) {
                     // Ignore.
@@ -835,8 +838,10 @@ public class XMPPTCPConnection extends XMPPConnection {
      * Notifies the XMPP connection that stream compression negotiation is done so that the
      * connection process can proceed.
      */
-    synchronized void streamCompressionNegotiationDone() {
+    void streamCompressionNegotiationDone() {
-        this.notify();
+        synchronized (compressionLock) {
+            compressionLock.notify();
+        }
     }
 
     /**
@@ -851,7 +856,8 @@ public class XMPPTCPConnection extends XMPPConnection {
      * @throws SmackException 
      * @throws IOException 
      */
-    public void connect() throws SmackException, IOException, XMPPException {
+    @Override
+    void connectInternal() throws SmackException, IOException, XMPPException {
         // Establishes the connection, readers and writers
         connectUsingConfiguration(config);
         // TODO is there a case where connectUsing.. does not throw an exception but connected is