Merge lp:~ltrager/maas/lp1566108 into lp:~maas-committers/maas/trunk

[Lee Trager]

Better handle filenames which start with '/'

The '//' was appearing because MAAS was concatenating the base API URL(/MAAS/api/2.0/files/) with the filename. This removes the leading '/' before concatenation so it doesn't appear in the resource_uri. I also noticed that you couldn't read or delete files with a leading or trailing slash. The API now uses a regex to search for files by filename which adds the leading or trailing / automatically.

[Gavin Panella (allenap)]

I have concerns; more in the diff comments.

review: Needs Fixing

[Lee Trager (ltrager)]

Rejected in favor of https://code.launchpad.net/~ltrager/maas/delete_on_files/+merge/292455

=== modified file 'src/maasserver/api/files.py'
--- src/maasserver/api/files.py	2016-04-05 21:01:13 +0000
+++ src/maasserver/api/files.py	2016-04-11 20:13:52 +0000
@@ -100,7 +100,7 @@
     dict_representation['content'] = b64encode(
         getattr(stored_file, 'content'))
     dict_representation['resource_uri'] = reverse(
-        'file_handler', args=[stored_file.filename])
+        'file_handler', args=[stored_file.filename]).replace('//', '/')
     # Emit the json for this object manually because, no matter what the
     # piston documentation says, once a type is associated with a list
     # of fields by piston's typemapper mechanism, there is no way to
@@ -126,7 +126,8 @@
         The 'content' of the file is base64-encoded."""
         try:
             stored_file = get_object_or_404(
-                FileStorage, filename=filename, owner=request.user)
+                FileStorage, filename__regex=('^[/]?%s[/]?$' % filename),
+                owner=request.user)
         except Http404:
             # In order to fix bug 1123986 we need to distinguish between
             # a 404 returned when the file is not present and a 404 returned
@@ -144,7 +145,8 @@
     def delete(self, request, filename):
         """Delete a FileStorage object."""
         stored_file = get_object_or_404(
-            FileStorage, filename=filename, owner=request.user)
+            FileStorage, filename__regex=('^[/]?%s[/]?$' % filename),
+            owner=request.user)
         stored_file.delete()
         return rc.DELETED
 
@@ -153,6 +155,12 @@
         filename = "filename"
         if stored_file is not None:
             filename = stored_file.filename
+            # Remove leading and trailing '/' so we're not expecting '//' which
+            # most web clients combine into just '/'
+            if filename[0] == '/':
+                filename = filename[1:]
+            if filename[-1] == '/':
+                filename = filename[:-1]
         return ('file_handler', (filename, ))
 
 

=== modified file 'src/maasserver/api/tests/test_filestorage.py'
--- src/maasserver/api/tests/test_filestorage.py	2016-02-18 01:32:50 +0000
+++ src/maasserver/api/tests/test_filestorage.py	2016-04-11 20:13:52 +0000
@@ -97,6 +97,13 @@
         self.assertEqual(http.client.OK, response.status_code)
         self.assertEqual(storage.content, response.content)
 
+    def test_get_works_with_slashes(self):
+        filename = "/a/filename/with/slashes/in/it/"
+        storage = factory.make_FileStorage(filename=filename)
+        response = self.make_API_GET_request("get", filename)
+        self.assertEqual(http.client.OK, response.status_code)
+        self.assertEqual(storage.content, response.content)
+
     def test_anon_resource_uri_allows_anonymous_access(self):
         storage = factory.make_FileStorage()
         response = self.client.get(storage.anon_resource_uri)
@@ -139,7 +146,7 @@
         self.assertEqual(http.client.CREATED, response.status_code)
 
     def test_add_file_with_slashes_in_name_succeeds(self):
-        filename = "filename/with/slashes/in/it"
+        filename = "/filename/with/slashes/in/it"
         response = self.make_API_POST_request(
             None, filename, factory.make_file_upload())
         self.assertEqual(http.client.CREATED, response.status_code)
@@ -269,7 +276,7 @@
         self.assertNotIn('content', parsed_results[0])
 
     def test_files_resource_uri_supports_slashes_in_filenames(self):
-        filename = "a/filename/with/slashes/in/it/"
+        filename = "/a/filename/with/slashes/in/it/"
         factory.make_FileStorage(
             filename=filename, content=b"test content",
             owner=self.logged_in_user)
@@ -277,14 +284,27 @@
         parsed_results = json_load_bytes(response.content)
         resource_uri = parsed_results[0]['resource_uri']
         expected_uri = reverse('file_handler', args=[filename])
+        expected_uri = expected_uri.replace('//', '/')
+        self.assertEqual(expected_uri, resource_uri)
+
+    def test_file_resource_uri_supports_slashes_in_filenames(self):
+        filename = "/a/filename/with/slashes/in/it/"
+        factory.make_FileStorage(
+            filename=filename, content=b"test content",
+            owner=self.logged_in_user)
+        response = self.make_API_GET_request(filename=filename)
+        parsed_results = json_load_bytes(response.content)
+        resource_uri = parsed_results[0]['resource_uri']
+        expected_uri = reverse('file_handler', args=[filename])
+        expected_uri = expected_uri.replace('//', '/')
         self.assertEqual(expected_uri, resource_uri)
 
     def test_api_supports_slashes_in_filenames_roundtrip_test(self):
         # Do a roundtrip (upload a file then get it) for a file with a
         # name that contains slashes.
-        filename = "filename/with/slashes/in/it"
+        filename = "/a/filename/with/slashes/in/it/"
         self.make_API_POST_request(None, filename, factory.make_file_upload())
-        file_url = reverse('file_handler', args=[filename])
+        file_url = reverse('file_handler', args=[filename]).replace('//', '/')
         # The file url contains the filename without any kind of
         # escaping.
         self.assertIn(filename, file_url)
@@ -375,3 +395,14 @@
         self.assertEqual(http.client.NO_CONTENT, response.status_code)
         files = FileStorage.objects.filter(filename=filename)
         self.assertEqual([], list(files))
+
+    def test_delete_file_deletes_file_with_slashes(self):
+        filename = '/path/to/file'
+        factory.make_FileStorage(
+            filename=filename, content=b"test content",
+            owner=self.logged_in_user)
+        response = self.client.delete(
+            reverse('file_handler', args=[filename]))
+        self.assertEqual(http.client.NO_CONTENT, response.status_code)
+        files = FileStorage.objects.filter(filename=filename)
+        self.assertEqual([], list(files))