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

Proposed by Lee Trager on 2016-04-11
Status: Rejected
Rejected by: Lee Trager on 2016-04-21
Proposed branch: lp:~ltrager/maas/lp1566108
Merge into: lp:maas/trunk
Diff against target: 126 lines (+46/-7)
2 files modified
src/maasserver/api/files.py (+11/-3)
src/maasserver/api/tests/test_filestorage.py (+35/-4)
To merge this branch: bzr merge lp:~ltrager/maas/lp1566108
Reviewer Review Type Date Requested Status
Gavin Panella (community) 2016-04-11 Needs Fixing on 2016-04-12
Review via email: mp+291552@code.launchpad.net

Commit message

Better handle filenames which start with '/'

Description of the change

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.

To post a comment you must log in.
Gavin Panella (allenap) wrote :

I have concerns; more in the diff comments.

review: Needs Fixing

Unmerged revisions

4903. By Lee Trager on 2016-04-11

Better handle filenames which start with '/'

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/api/files.py'
2--- src/maasserver/api/files.py 2016-04-05 21:01:13 +0000
3+++ src/maasserver/api/files.py 2016-04-11 20:13:52 +0000
4@@ -100,7 +100,7 @@
5 dict_representation['content'] = b64encode(
6 getattr(stored_file, 'content'))
7 dict_representation['resource_uri'] = reverse(
8- 'file_handler', args=[stored_file.filename])
9+ 'file_handler', args=[stored_file.filename]).replace('//', '/')
10 # Emit the json for this object manually because, no matter what the
11 # piston documentation says, once a type is associated with a list
12 # of fields by piston's typemapper mechanism, there is no way to
13@@ -126,7 +126,8 @@
14 The 'content' of the file is base64-encoded."""
15 try:
16 stored_file = get_object_or_404(
17- FileStorage, filename=filename, owner=request.user)
18+ FileStorage, filename__regex=('^[/]?%s[/]?$' % filename),
19+ owner=request.user)
20 except Http404:
21 # In order to fix bug 1123986 we need to distinguish between
22 # a 404 returned when the file is not present and a 404 returned
23@@ -144,7 +145,8 @@
24 def delete(self, request, filename):
25 """Delete a FileStorage object."""
26 stored_file = get_object_or_404(
27- FileStorage, filename=filename, owner=request.user)
28+ FileStorage, filename__regex=('^[/]?%s[/]?$' % filename),
29+ owner=request.user)
30 stored_file.delete()
31 return rc.DELETED
32
33@@ -153,6 +155,12 @@
34 filename = "filename"
35 if stored_file is not None:
36 filename = stored_file.filename
37+ # Remove leading and trailing '/' so we're not expecting '//' which
38+ # most web clients combine into just '/'
39+ if filename[0] == '/':
40+ filename = filename[1:]
41+ if filename[-1] == '/':
42+ filename = filename[:-1]
43 return ('file_handler', (filename, ))
44
45
46
47=== modified file 'src/maasserver/api/tests/test_filestorage.py'
48--- src/maasserver/api/tests/test_filestorage.py 2016-02-18 01:32:50 +0000
49+++ src/maasserver/api/tests/test_filestorage.py 2016-04-11 20:13:52 +0000
50@@ -97,6 +97,13 @@
51 self.assertEqual(http.client.OK, response.status_code)
52 self.assertEqual(storage.content, response.content)
53
54+ def test_get_works_with_slashes(self):
55+ filename = "/a/filename/with/slashes/in/it/"
56+ storage = factory.make_FileStorage(filename=filename)
57+ response = self.make_API_GET_request("get", filename)
58+ self.assertEqual(http.client.OK, response.status_code)
59+ self.assertEqual(storage.content, response.content)
60+
61 def test_anon_resource_uri_allows_anonymous_access(self):
62 storage = factory.make_FileStorage()
63 response = self.client.get(storage.anon_resource_uri)
64@@ -139,7 +146,7 @@
65 self.assertEqual(http.client.CREATED, response.status_code)
66
67 def test_add_file_with_slashes_in_name_succeeds(self):
68- filename = "filename/with/slashes/in/it"
69+ filename = "/filename/with/slashes/in/it"
70 response = self.make_API_POST_request(
71 None, filename, factory.make_file_upload())
72 self.assertEqual(http.client.CREATED, response.status_code)
73@@ -269,7 +276,7 @@
74 self.assertNotIn('content', parsed_results[0])
75
76 def test_files_resource_uri_supports_slashes_in_filenames(self):
77- filename = "a/filename/with/slashes/in/it/"
78+ filename = "/a/filename/with/slashes/in/it/"
79 factory.make_FileStorage(
80 filename=filename, content=b"test content",
81 owner=self.logged_in_user)
82@@ -277,14 +284,27 @@
83 parsed_results = json_load_bytes(response.content)
84 resource_uri = parsed_results[0]['resource_uri']
85 expected_uri = reverse('file_handler', args=[filename])
86+ expected_uri = expected_uri.replace('//', '/')
87+ self.assertEqual(expected_uri, resource_uri)
88+
89+ def test_file_resource_uri_supports_slashes_in_filenames(self):
90+ filename = "/a/filename/with/slashes/in/it/"
91+ factory.make_FileStorage(
92+ filename=filename, content=b"test content",
93+ owner=self.logged_in_user)
94+ response = self.make_API_GET_request(filename=filename)
95+ parsed_results = json_load_bytes(response.content)
96+ resource_uri = parsed_results[0]['resource_uri']
97+ expected_uri = reverse('file_handler', args=[filename])
98+ expected_uri = expected_uri.replace('//', '/')
99 self.assertEqual(expected_uri, resource_uri)
100
101 def test_api_supports_slashes_in_filenames_roundtrip_test(self):
102 # Do a roundtrip (upload a file then get it) for a file with a
103 # name that contains slashes.
104- filename = "filename/with/slashes/in/it"
105+ filename = "/a/filename/with/slashes/in/it/"
106 self.make_API_POST_request(None, filename, factory.make_file_upload())
107- file_url = reverse('file_handler', args=[filename])
108+ file_url = reverse('file_handler', args=[filename]).replace('//', '/')
109 # The file url contains the filename without any kind of
110 # escaping.
111 self.assertIn(filename, file_url)
112@@ -375,3 +395,14 @@
113 self.assertEqual(http.client.NO_CONTENT, response.status_code)
114 files = FileStorage.objects.filter(filename=filename)
115 self.assertEqual([], list(files))
116+
117+ def test_delete_file_deletes_file_with_slashes(self):
118+ filename = '/path/to/file'
119+ factory.make_FileStorage(
120+ filename=filename, content=b"test content",
121+ owner=self.logged_in_user)
122+ response = self.client.delete(
123+ reverse('file_handler', args=[filename]))
124+ self.assertEqual(http.client.NO_CONTENT, response.status_code)
125+ files = FileStorage.objects.filter(filename=filename)
126+ self.assertEqual([], list(files))