Make tempurl command check for valid object path

If the supplied path is not of the form /v1/a/c/o then
swift tempurl subcommand will now return an error message.

Also removes redundant check for seconds parameter being an int
from shell.py because the same check is made when calling
utils.generate_temp_url.

Drive-by fix for missing param definition for generate_temp_url.

Change-Id: I41f4389948b01fadaa5fc4939ea12e0ed2167345
Related-Change: I0fb2ce125fe12d660e4deb778265016bdd5ff31b

4 files changed, 60 insertions, 12 deletions

diff --git a/swiftclient/shell.py b/swiftclient/shell.py
index e1a5a8d..4e3ed0a 100755
--- a/swiftclient/shell.py
+++ b/swiftclient/shell.py
@@ -1239,17 +1239,17 @@ def st_tempurl(parser, args, thread_manager):
 
     parsed = urlparse(path)
 
-    try:
-        seconds = int(seconds)
-    except ValueError:
-        thread_manager.error('Seconds must be an integer')
-        return
     if method.upper() not in ['GET', 'PUT', 'HEAD', 'POST', 'DELETE']:
         thread_manager.print_msg('WARNING: Non default HTTP method %s for '
                                  'tempurl specified, possibly an error' %
                                  method.upper())
-    path = generate_temp_url(parsed.path, seconds, key, method,
-                             absolute=options['absolute_expiry'])
+    try:
+        path = generate_temp_url(parsed.path, seconds, key, method,
+                                 absolute=options['absolute_expiry'])
+    except ValueError as err:
+        thread_manager.error(err)
+        return
+
     if parsed.scheme and parsed.netloc:
         url = "%s://%s%s" % (parsed.scheme, parsed.netloc, path)
     else:
diff --git a/swiftclient/utils.py b/swiftclient/utils.py
index d394283..2e727ad 100644
--- a/swiftclient/utils.py
+++ b/swiftclient/utils.py
@@ -68,14 +68,20 @@ def generate_temp_url(path, seconds, key, method, absolute=False):
 
     :param path: The full path to the Swift object. Example:
         /v1/AUTH_account/c/o.
-    :param seconds: The amount of time in seconds the temporary URL will
-        be valid for.
+    :param seconds: If absolute is False then this specifies the amount of time
+        in seconds for which the temporary URL will be valid. If absolute is
+        True then this specifies an absolute time at which the temporary URL
+        will expire.
     :param key: The secret temporary URL key set on the Swift
         cluster. To set a key, run 'swift post -m
         "Temp-URL-Key: <substitute tempurl key here>"'
     :param method: A HTTP method, typically either GET or PUT, to allow
         for this temporary URL.
-    :raises: ValueError if seconds is not a positive integer
+    :param absolute: if True then the seconds parameter is interpreted as an
+        absolute Unix time, otherwise seconds is interpreted as a relative time
+        offset from current time.
+    :raises: ValueError if seconds is not a positive integer or path is not to
+        an object.
     :raises: TypeError if seconds is not an integer
     :return: the path portion of a temporary URL
     """
@@ -94,6 +100,10 @@ def generate_temp_url(path, seconds, key, method, absolute=False):
     else:
         path_for_body = path
 
+    parts = path_for_body.split('/', 4)
+    if len(parts) != 5 or parts[0] or not all(parts[1:]):
+        raise ValueError('path must be full path to an object e.g. /v1/a/c/o')
+
     standard_methods = ['GET', 'PUT', 'HEAD', 'POST', 'DELETE']
     if method.upper() not in standard_methods:
         logger = logging.getLogger("swiftclient")
diff --git a/tests/unit/test_shell.py b/tests/unit/test_shell.py
index 40881c2..3f077dc 100644
--- a/tests/unit/test_shell.py
+++ b/tests/unit/test_shell.py
@@ -1402,7 +1402,7 @@ class TestShell(unittest.TestCase):
                 "secret_key"]
         swiftclient.shell.main(argv)
         temp_url.assert_called_with(
-            '/v1/AUTH_account/c/o', 60, 'secret_key', 'GET', absolute=False)
+            '/v1/AUTH_account/c/o', "60", 'secret_key', 'GET', absolute=False)
 
     @mock.patch('swiftclient.shell.generate_temp_url', return_value='')
     def test_absolute_expiry_temp_url(self, temp_url):
@@ -1410,7 +1410,7 @@ class TestShell(unittest.TestCase):
                 "secret_key", "--absolute"]
         swiftclient.shell.main(argv)
         temp_url.assert_called_with(
-            '/v1/AUTH_account/c/o', 60, 'secret_key', 'GET', absolute=True)
+            '/v1/AUTH_account/c/o', "60", 'secret_key', 'GET', absolute=True)
 
     def test_temp_url_output(self):
         argv = ["", "tempurl", "GET", "60", "/v1/a/c/o",
@@ -1428,6 +1428,18 @@ class TestShell(unittest.TestCase):
         expected = "http://saio:8080%s" % expected
         self.assertEqual(expected, output.out)
 
+    def test_temp_url_error_output(self):
+        expected = 'path must be full path to an object e.g. /v1/a/c/o\n'
+        for bad_path in ('/v1/a/c', 'v1/a/c/o', '/v1/a/c/', '/v1/a//o',
+                         'http://saio/v1/a/c', 'http://v1/a/c/o'):
+            argv = ["", "tempurl", "GET", "60", bad_path,
+                    "secret_key", "--absolute"]
+            with CaptureOutput(suppress_systemexit=True) as output:
+                swiftclient.shell.main(argv)
+            self.assertEqual(expected, output.err,
+                             'Expected %r but got %r for path %r' %
+                             (expected, output.err, bad_path))
+
     @mock.patch('swiftclient.service.Connection')
     def test_capabilities(self, connection):
         argv = ["", "capabilities"]
diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py
index f0de79c..e820aff 100644
--- a/tests/unit/test_utils.py
+++ b/tests/unit/test_utils.py
@@ -180,6 +180,32 @@ class TestTempURL(unittest.TestCase):
         self.assertEqual(exc_manager.exception.args[0],
                          'seconds must be a positive integer')
 
+    def test_generate_temp_url_bad_path(self):
+        with self.assertRaises(ValueError) as exc_manager:
+            u.generate_temp_url('/v1/a/c', 60, self.key, self.method)
+        self.assertEqual(exc_manager.exception.args[0],
+                         'path must be full path to an object e.g. /v1/a/c/o')
+
+        with self.assertRaises(ValueError) as exc_manager:
+            u.generate_temp_url('v1/a/c/o', 60, self.key, self.method)
+        self.assertEqual(exc_manager.exception.args[0],
+                         'path must be full path to an object e.g. /v1/a/c/o')
+
+        with self.assertRaises(ValueError) as exc_manager:
+            u.generate_temp_url('blah/v1/a/c/o', 60, self.key, self.method)
+        self.assertEqual(exc_manager.exception.args[0],
+                         'path must be full path to an object e.g. /v1/a/c/o')
+
+        with self.assertRaises(ValueError) as exc_manager:
+            u.generate_temp_url('/v1//c/o', 60, self.key, self.method)
+        self.assertEqual(exc_manager.exception.args[0],
+                         'path must be full path to an object e.g. /v1/a/c/o')
+
+        with self.assertRaises(ValueError) as exc_manager:
+            u.generate_temp_url('/v1/a/c/', 60, self.key, self.method)
+        self.assertEqual(exc_manager.exception.args[0],
+                         'path must be full path to an object e.g. /v1/a/c/o')
+
 
 class TestTempURLUnicodePathAndKey(TestTempURL):
     url = u'/v1/\u00e4/c/\u00f3'