Repository: mtalexan/VersionOne.SDK.Python

Issue:
V1Meta does not handle authenication error properly with HTTPNTLMAuthHandler
Copying from https://github.com/versionone/VersionOne.SDK.Python/issues/25

> The solution provided in issue 3 had a minor bug:
> https://github.com/versionone/VersionOne.SDK.Python/issues/3
> 
> Under client.py:
> ```python
> 33                response = self.parent.error(
> 34                        'http', req, response, response.code, response.msg,
> 35                        response.info)
>  ```
> It passed in response.info method when it should be issue response.info()
> 
> Once this was fixed and no longer raised an exception, it expose an additional bug where it would recursively call the http_error_401 due to self.parent.error recalling this method. So to fix this, I found I needed to apply the following solution to raise the appropriate HTTP Error for 404.
> 
> ```python
> class CustomHTTPNtlmAuthHandler(HTTPNtlmAuthHandler):
>     """ A version of HTTPNtlmAuthHandler that handles errors (better).
>         The default version doesn't use `self.parent.open` in it's
>         error handler, and completely bypasses the normal `OpenerDirector`
>         call chain, most importantly `HTTPErrorProcessor.http_response`,
>         which normally raises an error for 'bad' http status codes..
>     """
>     def http_error_401(self, req, fp, code, msg, hdrs):
>         response = HTTPNtlmAuthHandler.http_error_401(self, req, fp, code, msg, hdrs)
>         if not (200 <= response.code < 300):
> -            response = self.parent.error(
> -                    'http', req, response, response.code, response.msg,
> -                    response.info)
> +            if response.code == 401:
> +                    raise HTTPError(req.get_full_url(), response.code, response.msg, response.info(), fp)
> +            else:
> +                    response = self.parent.error(
> +                           'http', req, response, response.code, response.msg,
> +                            response.info())
>         return response
> ```

Also
> An additional issue here. 'ntlm' not in auth_header_value.lower(), then response is None, and .code is an attribute error.

Discussion:

Patch:
diff --git a/v1pysdk/cache_decorator.py b/v1pysdk/cache_decorator.py
index 87fcac9..acac901 100644
--- a/v1pysdk/cache_decorator.py
+++ b/v1pysdk/cache_decorator.py
@@ -1,25 +1,25 @@
+def key_by_args_and_func_kw(old_f, args, kw, cache_data):
+    """Function to produce a key hash for the cache_data hash based on the function and the arguments
+       provided."""
+    return repr((old_f, args, kw))
 
-def key_by_args_kw(old_f, args, kw, cache_data):
-  'Function to build a cache key for the cached_by_keyfunc decorator. '
-  'This one just caches based on the function call arguments. i.e. Memoize '
-  return repr((args, kw))
 
-
-def cached_by_keyfunc(keyfunc):
-  """Calls keyfunc with (old_f, args, kw, datadict) to get cache key """
-  def decorator(old_f):
-    data = {}
+def memoized(old_f):
+    """Decorator that memoizes calls to old_f into a single instance-specific _memoized_data hash"""
     def new_f(self, *args, **kw):
-      new_key = keyfunc(old_f, args, kw, data)
-      if new_key in data:
-        return data[new_key]
-      new_value = old_f(self, *args, **kw)
-      data[new_key] = new_value
-      return new_value
+        """Function to wrap the decorated method"""
+        if not self._memoized_data:
+            self._memoized_data = {}
+        #generate a hash using the provided hashing function from the closure
+        new_key = key_by_args_and_func_kw(old_f, args, kw, self._memoized_data)
+        # check if it's already in the memoized data
+        if new_key in  self._memoized_data:
+            # return what's already there
+            return self._memoized_data[new_key]
+        # not found, call the real function and put the results in the memoized data
+        new_value = old_f(self, *args, **kw)
+        self._memoized_data[new_key] = new_value
+        return new_value
     return new_f
-  return decorator
-
-
-memoized = cached_by_keyfunc(key_by_args_kw)
-
 
+#memoized = cached_by_keyfunc()
diff --git a/v1pysdk/client.py b/v1pysdk/client.py
index b1e1859..799f66b 100644
--- a/v1pysdk/client.py
+++ b/v1pysdk/client.py
@@ -24,7 +24,7 @@ except ImportError:
     from elementtree import ElementTree
     from elementtree.ElementTree import Element
 
-AUTH_HANDLERS = [HTTPBasicAuthHandler]
+NTLM_FOUND=False
 
 try:
     if (sys.version_info < (3,0)):
@@ -34,23 +34,29 @@ try:
 except ImportError:
     logging.warn("Windows integrated authentication module (ntlm) not found.")
 else:
+    NTLM_FOUND=True
     class CustomHTTPNtlmAuthHandler(HTTPNtlmAuthHandler):
-        """ A version of HTTPNtlmAuthHandler that handles errors (better).
-
-            The default version doesn't use `self.parent.open` in it's
-            error handler, and completely bypasses the normal `OpenerDirector`
-            call chain, most importantly `HTTPErrorProcessor.http_response`,
-            which normally raises an error for 'bad' http status codes..
-        """
-        def http_error_401(self, req, fp, code, msg, hdrs):
-            response = HTTPNtlmAuthHandler.http_error_401(self, req, fp, code, msg, hdrs)
-            if not (200 <= response.code < 300):
-                response = self.parent.error(
-                        'http', req, response, response.code, response.msg,
-                        response.info)
-            return response
-
-    AUTH_HANDLERS.append(CustomHTTPNtlmAuthHandler)
+	    # The following code was a recommended change to the existing.  However unittesting proves that it
+	    # and the existing obscure authentication failures.  There seems to be no actual reason to be doing
+	    # this unless there was a particular issue with the HTTPNtlmAuthHandler in some specific version of
+	    # urllib2 for Python2 that no longer exists.  Using the default handler provably works correctly
+	    pass
+        #""" A version of HTTPNtlmAuthHandler that handles errors (better).
+        #    The default version doesn't use `self.parent.open` in it's
+        #    error handler, and completely bypasses the normal `OpenerDirector`
+        #    call chain, most importantly `HTTPErrorProcessor.http_response`,
+        #    which normally raises an error for 'bad' http status codes..
+        #"""
+        #def http_error_401(self, req, fp, code, msg, hdrs):
+        #    response = HTTPNtlmAuthHandler.http_error_401(self, req, fp, code, msg, hdrs)
+        #    if not (200 <= response.code < 300):
+        #        if response.code == 401:
+        #            raise HTTPError(req.get_full_url(), response.code, response.msg, response.info(), fp)
+        #        else:
+        #            response = self.parent.error(
+        #                'http', req, response, response.code, response.msg,
+        #                response.info())
+        #    return response
 
 
 class V1Error(Exception):
@@ -76,7 +82,9 @@ class V1Server(object):
       self.instance = instance.strip('/')
       self.scheme = scheme
       self.instance_url = self.build_url('')
-
+    self.AUTH_HANDLERS = [HTTPBasicAuthHandler]
+    if NTLM_FOUND:
+        self.AUTH_HANDLERS.append(CustomHTTPNtlmAuthHandler)
     modulelogname='v1pysdk.client'
     logname = "%s.%s" % (logparent, modulelogname) if logparent else None
     self.logger = logging.getLogger(logname)
@@ -89,8 +97,8 @@ class V1Server(object):
   def _install_opener(self):
     base_url = self.build_url('')
     password_manager = theUrlLib.HTTPPasswordMgrWithDefaultRealm()
-    password_manager.add_password(None, base_url, self.username, self.password)
-    handlers = [HandlerClass(password_manager) for HandlerClass in AUTH_HANDLERS]
+    password_manager.add_password(realm=None, uri=base_url, user=self.username, passwd=self.password)
+    handlers = [HandlerClass(password_manager) for HandlerClass in self.AUTH_HANDLERS]
     self.opener = theUrlLib.build_opener(*handlers)
     if self.use_password_as_token:
         self.opener.addheaders.append(('Authorization', 'Bearer ' + self.password))
diff --git a/v1pysdk/v1meta.py b/v1pysdk/v1meta.py
index 0e6c696..1666815 100644
--- a/v1pysdk/v1meta.py
+++ b/v1pysdk/v1meta.py
@@ -12,37 +12,34 @@ from .special_class_methods import special_classes
 from .none_deref import NoneDeref
 from .string_utils import split_attribute
 
-class V1Meta(object):        
+class V1Meta(object):
   def __init__(self, *args, **kw):
     self.server = V1Server(*args, **kw)
     self.global_cache = {}
     self.dirtylist = []
-    self.memoize_cache = {}
-    
+    self._memoized_data = {}
+
   def __getattr__(self, attr):
     "Dynamically build asset type classes when someone tries to get attrs "
     "that we don't have."
     return self.asset_class(attr)
-    
+
   def __enter__(self):
     return self
-  
+
   def __exit__(self, *args, **kw):
+    self.clear_memoized_cache()
     self.commit()
-    
-  # The @memoized decorator cannot be used here.  If it is, Python will memoize the input arguments against
-  # the return values for all instances of this class that ever exist within the Python shell instantiation.
-  # This doesn't account for the fact that different instances of V1Meta may have different underlying data
-  # returned from the query and the return from this function should therefore be different given the same
-  # arguments for different instances of V1Meta.
-  def asset_class(self, asset_type_name):
-    # per-instance memoization
-    if asset_type_name in self.memoize_cache:
-        return self.memoize_cache[asset_type_name]
 
+  def clear_memoized_cache(self):
+      """Clears the memoization cache produced by the @memoized decorator"""
+      self._memoized_data={}
+
+  @memoized # from .cache_decorator
+  def asset_class(self, asset_type_name):
     xmldata = self.server.get_meta_xml(asset_type_name)
     class_members = {
-        '_v1_v1meta': self, 
+        '_v1_v1meta': self,
         '_v1_asset_type_name': asset_type_name,
         }
     for operation in xmldata.findall('Operation'):
@@ -79,35 +76,32 @@ class V1Meta(object):
             return self._v1_setattr(attr, value)
           def deleter(self, attr=attr):
             raise NotImplementedError
-            
+
       class_members[attr] = property(getter, setter, deleter)
-      
+
     bases = [BaseAsset,]
     # mix in any special methods
     if asset_type_name in special_classes:
       mixin = special_classes[asset_type_name]
       bases.append(mixin)
-      
-    new_asset_class = type(asset_type_name, tuple(bases), class_members)
 
-    # save to our memoization cache
-    self.memoize_cache[asset_type_name] = new_asset_class
+    new_asset_class = type(asset_type_name, tuple(bases), class_members)
     return new_asset_class
-    
+
   def add_to_dirty_list(self, asset_instance):
     # at some point we're going to flush the items in the dirty list.  Since there
     # are a few triggers to do this, it's best to clear our memoization cache for
     # query responses as soon as we have something that can get flushed rather than
     # waiting for it to actually be flushed
-    self.memoize_cache = {}
+    self.clear_memoized_cache()
     self.dirtylist.append(asset_instance)
-    
+
   def commit(self):
       errors = []
       # we're flushing changes, make sure our memoization cache is cleared so the updates
       # are re-queried
       if self.dirtylist:
-          self.memoize_cache = {}
+          self.clear_memoized_cache()
       for asset in self.dirtylist:
           try:
               asset._v1_commit()
@@ -115,7 +109,7 @@ class V1Meta(object):
               errors.append(e)
           self.dirtylist = []
       return errors
-    
+
   def generate_update_doc(self, newdata):
     update_doc = Element('Asset')
     for attrname, newvalue in newdata.items():
@@ -145,13 +139,13 @@ class V1Meta(object):
         node.text = str(newvalue)
       update_doc.append(node)
     return update_doc
-    
+
   def create_asset(self, asset_type_name, newdata):
     update_doc = self.generate_update_doc(newdata)
     new_asset_xml = self.server.create_asset(asset_type_name,  update_doc)
     asset_type, asset_oid, asset_moment = new_asset_xml.get('id').split(':')
     return self.asset_class(asset_type)(asset_oid)
-    
+
   def update_asset(self, asset_type_name, asset_oid, newdata):
     update_doc = self.generate_update_doc(newdata)
     return self.server.update_asset(asset_type_name, asset_oid, update_doc)


Test patch:
diff --git a/tests/connect_tests.py b/tests/connect_tests.py
index 3b642c1..01ad28c 100755
--- a/tests/connect_tests.py
+++ b/tests/connect_tests.py
@@ -2,6 +2,11 @@ from testtools import TestCase
 from testtools.assertions import assert_that
 from testtools.matchers import Equals
 from testtools.content import text_content
+import sys
+if sys.version_info >= (3,0):
+    from urllib.error import HTTPError
+else:
+    from urllib2 import HTTPError
 
 # try the old version, then fallback to the new one
 try:
@@ -143,3 +148,102 @@ class TestV1Connection(TestCase):
             items.first() #run the query
         except Exception as e:
             assert_that(False, Equals(True), message="Error running query from connection: " + str(e))
+
+
+    def test_connect_fails_when_invalid(self):
+        v1bad = None
+        username = self.getUniqueString() #garbage
+        password = self.getUniqueString() #garbage
+        self.addDetail('address', text_content(PublicTestServerConnection.address))
+        self.addDetail('instance', text_content(PublicTestServerConnection.instance))
+        self.addDetail('bad-username', text_content(username))
+        self.addDetail('bad-password', text_content(password))
+
+        try:
+            v1bad = V1Meta(
+                instance_url = PublicTestServerConnection.instance_url,
+                username = username,
+                password = password,
+                use_password_as_token=False,
+                )
+            # we have to try to use it to get it to connect and fail
+            items = v1bad.Story.select('Name').page(size=1)
+            items.first() #run the query
+        except HTTPError as e:
+            assert_that(e.code, Equals(401), message="Connection failed for reasons other than authorization")
+        else:
+            assert_that(False, Equals(True), message="Connection succeeded with bad credentials")
+
+    def test_reconnect_succeeds_after_invalid(self):
+        v1bad = None
+        username = self.getUniqueString() #garbage
+        password = self.getUniqueString() #garbage
+        self.addDetail('bad-username', text_content(username))
+        self.addDetail('bad-password', text_content(password))
+
+        try:
+            v1bad = V1Meta(
+                instance_url = PublicTestServerConnection.instance_url,
+                username = username,
+                password = password,
+                use_password_as_token=False,
+                )
+            items = v1bad.Story.select('Name').page(size=1)
+            items.first() #run the query
+        except HTTPError as e:
+            assert_that(e.code, Equals(401), message="Connection failed for reasons other than authorization")
+        else:
+            assert_that(False, Equals(True), message="First connection succeeded with bad credentials, cannot continue test")
+
+        v1good = None
+        self.addDetail('address', text_content(PublicTestServerConnection.address))
+        self.addDetail('instance', text_content(PublicTestServerConnection.instance))
+
+        # Connect correctly first
+        try:
+            v1good = V1Meta(
+                instance_url = PublicTestServerConnection.instance_url,
+                password = PublicTestServerConnection.token,
+                use_password_as_token=True,
+                )
+            items = v1good.Story.select('Name').page(size=1)
+            items.first() #run the query
+        except Exception as e:
+            assert_that(False, Equals(True), message="Error running query from good connection: " + str(e))
+
+    def test_reconnect_fails_when_invalid(self):
+        v1good = None
+        self.addDetail('address', text_content(PublicTestServerConnection.address))
+        self.addDetail('instance', text_content(PublicTestServerConnection.instance))
+
+        # Connect correctly first
+        try:
+            v1good = V1Meta(
+                instance_url = PublicTestServerConnection.instance_url,
+                password = PublicTestServerConnection.token,
+                use_password_as_token=True,
+                )
+            items = v1good.Story.select('Name').page(size=1)
+            items.first() #run the query
+        except Exception as e:
+            assert_that(False, Equals(True), message="Error running query from good connection, cannot perform test: " + str(e))
+
+        v1bad = None
+        username = self.getUniqueString() #garbage
+        password = self.getUniqueString() #garbage
+        self.addDetail('bad-username', text_content(username))
+        self.addDetail('bad-password', text_content(password))
+
+        try:
+            v1bad = V1Meta(
+                instance_url = PublicTestServerConnection.instance_url,
+                username = username,
+                password = password,
+                use_password_as_token=False,
+                )
+            items = v1bad.Story.select('Name').page(size=1)
+            items.first() #run the query
+        except HTTPError as e:
+            assert_that(e.code, Equals(401), message="Connection failed for reasons other than authorization")
+        else:
+            assert_that(False, Equals(True), message="Second connection succeeded with bad credentials")
diff --git a/tests/query_tests.py b/tests/query_tests.py
index 60c6529..2dd336d 100644
--- a/tests/query_tests.py
+++ b/tests/query_tests.py
@@ -5,8 +5,9 @@ from testtools.matchers import GreaterThan, LessThan, Contains, StartsWith, Ends
 
 import v1pysdk
 from .common_test_server import PublicTestServerConnection
+from .common_test_setup import TestV1CommonSetup
 
-class TestV1Query(TestCase):
+class TestV1Query(TestV1CommonSetup):
     def test_select_story_as_generic_asset(self):
         """Queries up to 5 story assets"""
         with PublicTestServerConnection.getV1Meta() as v1:
@@ -157,63 +158,38 @@ class TestV1Query(TestCase):
 
             self.assertFalse(exceptionReached)
 
-# This should work, but doesn't for some reason.  It doesn't find any results from the find command
-#    def test_find_query(self):
-#        """Queries for the first story, then does a server-side find on a portion of the name to see if 
-#           any results are returned.
-#        """
-#        searchName=""
-#        exceptionReached=False
-#        with PublicTestServerConnection.getV1Meta() as v1:
-#            item = None
-#            size = 0
-#            name = ""
-#            items = None
-#            try:
-#                items = v1.Story.select('Name').page(size=40)
-#                item = items.first() #triggers actual query to happen
-#                size = len(items)
-#            except:
-#                self.skipTest("Unable to query for a Story object -- Story query test should have failed")
-#                return
-#            else:
-#                if size <= 0:
-#                    self.skipTest("Test Server contains no Stories")
-#                    return
-#
-#            for i in items:
-#                if len(i.Name) >= 2:
-#                    name = i.Name
-#                    self.addDetail('item-name', text_content(name))
-#                    break
-#
-#            if len(name) < 2:
-#                self.skipTest("Test Server contains too many single character named stories")
-#                return
-#
-#            # make a search term that's just one character shorter
-#            searchName = name[:-1]
-#            self.addDetail('search-name', text_content(searchName))
-#
-#        with PublicTestServerConnection.getV1Meta() as v1:
-#            findItems = None
-#            findItem = None
-#            size = 0
-#            firstName = ""
-#            try:
-#                findItems = v1.Story.select('Name').find(text=searchName, field='Name')
-#                findItem = findItems.first() #actually run the query
-#                size = len(findItems)
-#                firstName = findItem.Name
-#            except Exception as e:
-#                raise e
-#                #exceptions here are almost always because the query failed to work right
-#                exceptionReached=True
-#            else:
-#                # at the very least we should have found the one we based the search string on
-#                self.assertThat(size, GreaterThan(0))
-#                # results need to contain the string we searched for
-#                self.assertThat(firstName, Contains(searchName))
-#
-#            self.assertFalse(exceptionReached)
-#
+    def test_find_query(self):
+        """Creates a story, then does a find to see if it can be located by a partial name from a separate
+           connection instance.
+        """
+        searchName=""
+        exceptionReached=False
+        with PublicTestServerConnection.getV1Meta() as v1create:
+            createdStory = self._create_story(v1create)
+
+            # make a search term that's just one character shorter
+            searchName = createdStory.Name[:-1]
+            self.addDetail('search-name', text_content(searchName))
+
+        with PublicTestServerConnection.getV1Meta() as v1find:
+            findItems = None
+            findItem = None
+            size = 0
+            firstName = ""
+            try:
+                findItems = v1find.Story.select('Name').find(text=searchName, field='Name')
+                findItem = findItems.first() #actually run the query
+                size = len(findItems)
+                firstName = findItem.Name
+            except Exception as e:
+                raise e
+                #exceptions here are almost always because the query failed to work right
+                exceptionReached=True
+            else:
+                # at the very least we should have found the one we based the search string on
+                self.assertThat(size, GreaterThan(0))
+                # results need to contain the string we searched for
+                self.assertThat(firstName, Contains(searchName))
+
+            self.assertFalse(exceptionReached)
+