Authentication failures could cause incorrect response

Oops. To best ensure we performed post-authentication activities I hacked it
directly into the BaseController, but these didn't take into account that
authentication could fail. The post-auth requests then failed because tor
closes the socket.

Caught thanks to daftaupe on...

  https://trac.torproject.org/projects/tor/ticket/22679

While troubleshooting this I ran into another bug that might be an issue with
tor. When authentication failed we called 'QUIT'. Tor will sometimes respond
with 'closing connection' and other times just hang. Reached out to Nick to see
if this is something we'd care to address, but on Stem's side now simply
omitting a QUIT call if unauthenticated.

4 files changed, 25 insertions, 12 deletions

diff --git a/docs/change_log.rst b/docs/change_log.rst
index 06341b47..96690ac2 100644
--- a/docs/change_log.rst
+++ b/docs/change_log.rst
@@ -46,6 +46,7 @@ The following are only available within Stem's `git repository
  * **Controller**
 
   * :func:`~stem.process.launch_tor` raised a ValueError if invoked when outside the main thread
+  * Failure to authenticate could raise an improper response or hang (:trac:`22679`)
   * Renamed :class:`~stem.response.events.ConnectionBandwidthEvent` type attribute to conn_type to avoid conflict with parent class (:trac:`21774`)
   * Added the GUARD_WAIT :data:`~stem.CircStatus` (:spec:`6446210`)
   * Unable to use cookie auth when path includes wide characters (chinese, japanese, etc)
diff --git a/stem/connection.py b/stem/connection.py
index a3f27a07..db3f218e 100644
--- a/stem/connection.py
+++ b/stem/connection.py
@@ -584,6 +584,9 @@ def authenticate(controller, password = None, chroot_path = None, protocolinfo_r
         else:
           authenticate_cookie(controller, cookie_path, False)
 
+      if isinstance(controller, stem.control.BaseController):
+        controller._post_authentication()
+
       return  # success!
     except OpenAuthRejected as exc:
       auth_exceptions.append(exc)
diff --git a/stem/control.py b/stem/control.py
index 65e34016..8c01d947 100644
--- a/stem/control.py
+++ b/stem/control.py
@@ -633,13 +633,6 @@ class BaseController(object):
         if isinstance(response, stem.ControllerError):
           raise response
         else:
-          # I really, really don't like putting hooks into this method, but
-          # this is the most reliable method I can think of for taking actions
-          # immediately after successfully authenticating to a connection.
-
-          if message.upper().startswith('AUTHENTICATE'):
-            self._post_authentication()
-
           return response
       except stem.SocketClosed:
         # If the recv() thread caused the SocketClosed then we could still be
@@ -693,10 +686,7 @@ class BaseController(object):
       and **False** otherwise
     """
 
-    if self.is_alive():
-      return self._is_authenticated
-
-    return False
+    return self._is_authenticated if self.is_alive() else False
 
   def connect(self):
     """
@@ -1051,7 +1041,7 @@ class Controller(BaseController):
 
   def close(self):
     # making a best-effort attempt to quit before detaching the socket
-    if self.is_alive():
+    if self.is_authenticated():
       try:
         self.msg('QUIT')
       except:
diff --git a/test/integ/connection/authentication.py b/test/integ/connection/authentication.py
index 3a2fcb00..a8c0c821 100644
--- a/test/integ/connection/authentication.py
+++ b/test/integ/connection/authentication.py
@@ -268,6 +268,25 @@ class TestAuthenticate(unittest.TestCase):
         self.assertRaises(exc_type, self._check_auth, auth_type, auth_value)
 
   @test.require.controller
+  def test_wrong_password_with_controller(self):
+    """
+    We ran into a race condition where providing the wrong password to the
+    Controller caused inconsistent responses. Checking for that...
+
+    https://trac.torproject.org/projects/tor/ticket/22679
+    """
+
+    runner = test.runner.get_runner()
+
+    if test.runner.Torrc.PASSWORD not in runner.get_options():
+      self.skipTest('(requires password auth)')
+      return
+
+    for i in range(10):
+      with runner.get_tor_controller(False) as controller:
+        self.assertRaises(stem.connection.IncorrectPassword, controller.authenticate, 'wrong_password')
+
+  @test.require.controller
   def test_authenticate_cookie(self):
     """
     Tests the authenticate_cookie function.