Repository: ansible/tower-cli
Pull request #542 (2018-06-08 15:05:49)

Fixing tower-cli execution on windows
os.path.sep.join was causing windows clients to use \ in a URL. This was causing calls to Tower to fail. Updated to use urljoin.

AlanCoding: This test fail looks like a meaningful find:

```python
>>> from urlparse import urljoin
>>> urljoin('bogus://33.33.33.33/', '/api/v2/')
'/api/v2/'
>>> urljoin('bogus://33.33.33.33/', '/api/')
'/api/'
```

on python 2.7.11 here.

I think we probably need to dig into the docs for how `urljoin` is expected to work.
AlanCoding: I guess you can just change the test to use http

```
>>> urljoin('http://33.33.33.33/', 'api')
'http://33.33.33.33/api'
>>> urljoin('https://33.33.33.33/', 'api')
'https://33.33.33.33/api'
>>> urljoin('https://33.33.33.33/', '/api/')
'https://33.33.33.33/api/'
```

@ryanpetrello 
john-westcott-iv: @AlanCoding Thanks for double checking me. After looking at the source for urljoin, if the URL scheme is not "standard" the behavior we have seen is expected. 

There are two tests in api.py test:
> Establish that the prefix property returns the appropriate URL prefix given a host with no specified protocol.
and
> Establish that the prefix property returns the appropriate URL prefix and don't clobber over an explicit protocol.

Both tests were added in 2014 by @lukesneeringer . Does anyone know of a reason that TowerCLI should be allowed to use alternative schemes? 
AlanCoding: no, if you wanted to go down this road we would prefer to raise some custom exception if we got a non-standard scheme. Probably a generic exception for "your URL is wrong".
john-westcott-iv: @AlanCoding Please see new pull. I am now throwing a ConnectionException if the scheme is not http or https. The tests now validate that http and https urls come back correct and that bogus:// throws the ConnectionException.


Patch:
diff --git a/tower_cli/api.py b/tower_cli/api.py
index 3e47378..fb287f1 100644
--- a/tower_cli/api.py
+++ b/tower_cli/api.py
@@ -20,6 +20,12 @@ import functools
 import json
 import stat
 import warnings
+
+try:
+    from urlparse import urljoin, urlparse
+except Exception as e:
+    from urllib.parse import urljoin, urlparse
+
 from datetime import datetime as dt
 
 from requests.exceptions import ConnectionError, SSLError
@@ -187,9 +193,16 @@ class Client(Session):
                 'Can not verify ssl with non-https protocol. Change the '
                 'verify_ssl configuration setting to continue.'
             )
-        prefix = os.path.sep.join([host.rstrip('/'), 'api', ''])
+        # Validate that we have either an http or https based URL
+        url_pieces = urlparse(host)
+        if url_pieces[0] not in ['http', 'https']:
+            raise exc.ConnectionError('URL must be http(s), {} is not valid'.format(url_pieces[0]))
+
+        prefix = urljoin(host, '/api/')
         if include_version:
-            prefix = os.path.sep.join([prefix.rstrip('/'), CUR_API_VERSION, ''])
+            # We add the / to the end of {} so that our URL has the ending slash.
+            prefix = urljoin(prefix, "{}/".format(CUR_API_VERSION))
+
         return prefix
 
     @functools.wraps(Session.request)


Test patch:
diff --git a/tests/test_api.py b/tests/test_api.py
index 98cc629..45818a0 100644
--- a/tests/test_api.py
+++ b/tests/test_api.py
@@ -46,12 +46,24 @@ class ClientTests(unittest.TestCase):
         with settings.runtime_values(host='33.33.33.33'):
             self.assertEqual(client.get_prefix(), 'https://33.33.33.33/api/%s/' % CUR_API_VERSION)
 
+    def test_prefix_https_protocol(self):
+        """Establish that the http protocol works
+        """
+        with settings.runtime_values(host='https://33.33.33.33'):
+            self.assertEqual(client.get_prefix(), 'https://33.33.33.33/api/%s/' % CUR_API_VERSION)
+
+    def test_prefix_http_protocol(self):
+        """Establish that the http protocol works
+        """
+        with settings.runtime_values(host='http://33.33.33.33', verify_ssl=False):
+            self.assertEqual(client.get_prefix(), 'http://33.33.33.33/api/%s/' % CUR_API_VERSION)
+
     def test_prefix_explicit_protocol(self):
-        """Establish that the prefix property returns the appropriate
-        URL prefix and don't clobber over an explicit protocol.
+        """Establish that the prefix property can not start with non-http protocol.
         """
         with settings.runtime_values(host='bogus://33.33.33.33/'):
-            self.assertEqual(client.get_prefix(), 'bogus://33.33.33.33/api/%s/' % CUR_API_VERSION)
+            with self.assertRaises(exc.ConnectionError):
+                client.get_prefix()
 
     def test_request_ok(self):
         """Establish that a request that returns a valid JSON response