Merge lp:~rvb/maas/bug-1123986-api-fix into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Adapt the file storage API to the 'owner' field on FileStorage objects.

This branch adapts the file storage API to the new 'owner' field.
Each user is now virtually isolated in his own namespace when uploading/reading file from the file storage API.
Since we have to cope with existing installations there are a few caveats:
- when a user gets a file from /files/<filename>/, if no owned-file can be found, the API will try to return a non-owned file with the same name
- the anonymous API will always return the *last* uploaded file with the given name.

[Gavin Panella (allenap)]

Looks good, but I have a couple of questions.

[1]

-def get_file(handler, request):
+def get_file_by_name(handler, request):
     """Get a named file from the file storage.

     :param filename: The exact name of the file you want to get.
     :type filename: string
     :return: The file is returned in the response content.
     """
-    filename = request.GET.get("filename", None)
-    if not filename:
-        raise MAASAPIBadRequest("Filename not supplied")
+    filename = get_mandatory_param(request.GET, 'filename')
     try:
-        db_file = FileStorage.objects.get(filename=filename)
+        db_file = FileStorage.objects.filter(filename=filename).latest('id')

Shouldn't this restrict itself to files without an owner?

[2]

     def delete(self, request, filename):
         """Delete a FileStorage object."""
-        stored_file = get_object_or_404(FileStorage, filename=filename)
+        stored_file = get_object_or_404(
+            FileStorage, filename=filename, owner=request.user)
         stored_file.delete()

Should this fall back to deleting files without owner when an
owned file is not found?

review: Needs Information

[Raphaël Badin (rvb)]

> Looks good, but I have a couple of questions.
>
>
> [1]
>
> -def get_file(handler, request):
> +def get_file_by_name(handler, request):
>     """Get a named file from the file storage.
>
>     :param filename: The exact name of the file you want to get.
>     :type filename: string
>     :return: The file is returned in the response content.
>     """
> -    filename = request.GET.get("filename", None)
> -    if not filename:
> -        raise MAASAPIBadRequest("Filename not supplied")
> +    filename = get_mandatory_param(request.GET, 'filename')
>     try:
> -        db_file = FileStorage.objects.get(filename=filename)
> +        db_file = FileStorage.objects.filter(filename=filename).latest('id')
>
> Shouldn't this restrict itself to files without an owner?

No, imagine you upgrade your juju client in an existing env with services deployed: next time the juju client will upload a file (the file will be storage with an owner), the node might want to read it with the URL it has stored.

> [2]
>
>     def delete(self, request, filename):
>         """Delete a FileStorage object."""
> -        stored_file = get_object_or_404(FileStorage, filename=filename)
> +        stored_file = get_object_or_404(
> +            FileStorage, filename=filename, owner=request.user)
>         stored_file.delete()
>
> Should this fall back to deleting files without owner when an
> owned file is not found?

The python provider does not use this. So the only provider which will use this is the Go provider, which will always set an owner.

[Raphaël Badin (rvb)]

> [2]
>
>     def delete(self, request, filename):
>         """Delete a FileStorage object."""
> -        stored_file = get_object_or_404(FileStorage, filename=filename)
> +        stored_file = get_object_or_404(
> +            FileStorage, filename=filename, owner=request.user)
>         stored_file.delete()
>
> Should this fall back to deleting files without owner when an
> owned file is not found?

The response I made above is still valid but I think we've got a choice to make here:
a) either we focus on supporting the old provider: in this case, as I said, the version of delete is all right because the provider does not use it. But this means we also can remove the compatibility layer I added in the handler of "GET /api/1.0/files/<filename>/" because that handler too is not used by the provider (it was added very recently actually).
b) or we try to have a general behavior that is coherent when upgrading (independently from what the provider uses). In this case, as you pointed out, we need to change delete() to delete files without an owner.

I'm tempted to go for a) because:
- the filestorage stuff is really just here to be used by Juju
- adding a compatibility layer adds yet another layer of complexity to the code

[Gavin Panella (allenap)]

> > Shouldn't this restrict itself to files without an owner?
>
> No, imagine you upgrade your juju client in an existing env with services
> deployed: next time the juju client will upload a file (the file will be
> storage with an owner), the node might want to read it with the URL it has
> stored.

But might this code get the file that belongs to another user? I'm
thinking it should look for a file either owned by the user *or*
unowned. I suspect I'm not seeing something in the implementation,
which could indicate that it could be structured better, or that I'm
stupid :)

> > Should this fall back to deleting files without owner when an
> > owned file is not found?
>
> The python provider does not use this.  So the only provider which will use
> this is the Go provider, which will always set an owner.

The API ought to have a way to delete unowned files, even if neither
Juju provider needs it.

[Raphaël Badin (rvb)]

> But might this code get the file that belongs to another user?

Yes, it will definitely do that if you're still using an old version of juju … and that's exactly the situation we have now already (basically the goal is to: make the situation not worse if you use an old juju, fix the situation if you use a new juju… I'm afraid that's the best we can do)

[Raphaël Badin (rvb)]

> The API ought to have a way to delete unowned files, even if neither
> Juju provider needs it.

Just like we have a consensus on this, done.

[Raphaël Badin (rvb)]

This is now all fixed. I get rid of the whole 'values' non-sense in the follow-up branch, don't worry.

[Gavin Panella (allenap)]

review: Approve

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2013-02-14 14:25:39 +0000
+++ src/maasserver/api.py	2013-02-22 09:28:21 +0000
@@ -777,18 +777,16 @@
         return ('node_mac_handler', [node_system_id, mac_address])
 
 
-def get_file(handler, request):
+def get_file_by_name(handler, request):
     """Get a named file from the file storage.
 
     :param filename: The exact name of the file you want to get.
     :type filename: string
     :return: The file is returned in the response content.
     """
-    filename = request.GET.get("filename", None)
-    if not filename:
-        raise MAASAPIBadRequest("Filename not supplied")
+    filename = get_mandatory_param(request.GET, 'filename')
     try:
-        db_file = FileStorage.objects.get(filename=filename)
+        db_file = FileStorage.objects.filter(filename=filename).latest('id')
     except FileStorage.DoesNotExist:
         raise MAASAPINotFound("File not found")
     return HttpResponse(db_file.content, status=httplib.OK)
@@ -808,7 +806,7 @@
     """
     create = read = update = delete = None
 
-    get = operation(idempotent=True, exported_as='get')(get_file)
+    get = operation(idempotent=True, exported_as='get')(get_file_by_name)
 
     @classmethod
     def resource_uri(cls, *args, **kwargs):
@@ -823,10 +821,35 @@
 DISPLAYED_FILES_FIELDS_OBJECT = DISPLAYED_FILES_FIELDS + ('content', )
 
 
+def get_owned_file_or_404(filename, user, values=False):
+    """Return the file named 'filename' owned by 'user'.
+
+    If there is no such file, try getting the corresponding unowned file
+    to be compatible with old versions of MAAS where all the files where
+    unowned.
+    """
+    stored_files = FileStorage.objects.filter(
+        filename=filename, owner=user)
+    # filename and owner are 'unique together', we can have either 0 or 1
+    # objects in 'stored_files'.
+    if len(stored_files) == 0:
+        # In order to support upgrading from installations where the notion
+        # of a file owner was not yet implemented, return non-owned files
+        # with this filename at a last resort option.
+        stored_files = FileStorage.objects.filter(
+            filename=filename, owner=None)
+        if len(stored_files) == 0:
+            raise Http404
+    if values:
+        return stored_files.values()[0]
+    else:
+        return stored_files[0]
+
+
 class FileHandler(OperationsHandler):
     """Manage a FileStorage object.
 
-    The file is identified by its filename.
+    The file is identified by its filename and owner.
     """
     model = FileStorage
     fields = DISPLAYED_FILES_FIELDS
@@ -836,14 +859,8 @@
         """GET a FileStorage object as a json object.
 
         The 'content' of the file is base64-encoded."""
-        # 'content' is a BinaryField, its 'value' is a base64-encoded version
-        # of its value.
-        stored_files = FileStorage.objects.filter(filename=filename).values()
-        # filename is a 'unique' field, we can have either 0 or 1 objects in
-        # 'stored_files'.
-        if len(stored_files) == 0:
-            raise Http404
-        stored_file = stored_files[0]
+        stored_file = get_owned_file_or_404(
+            filename=filename, user=request.user, values=True)
         # Emit the json for this object manually because, no matter what the
         # piston documentation says, once an type is associated with a list
         # of fields by piston's typemapper mechanism, there is no way to
@@ -858,7 +875,8 @@
     @operation(idempotent=False)
     def delete(self, request, filename):
         """Delete a FileStorage object."""
-        stored_file = get_object_or_404(FileStorage, filename=filename)
+        stored_file = get_owned_file_or_404(
+            filename=filename, user=request.user)
         stored_file.delete()
         return rc.DELETED
 
@@ -875,7 +893,7 @@
     create = read = update = delete = None
     anonymous = AnonFilesHandler
 
-    get = operation(idempotent=True, exported_as='get')(get_file)
+    get = operation(idempotent=True, exported_as='get')(get_file_by_name)
 
     @operation(idempotent=False)
     def add(self, request):
@@ -899,7 +917,7 @@
         # As per the comment in FileStorage, this ought to deal in
         # chunks instead of reading the file into memory, but large
         # files are not expected.
-        FileStorage.objects.save_file(filename, uploaded_file)
+        FileStorage.objects.save_file(filename, uploaded_file, request.user)
         return HttpResponse('', status=httplib.CREATED)
 
     @operation(idempotent=True)
@@ -913,7 +931,7 @@
         :type prefix: string
         """
         prefix = request.GET.get("prefix", None)
-        files = FileStorage.objects.all()
+        files = FileStorage.objects.filter(owner=request.user)
         if prefix is not None:
             files = files.filter(filename__startswith=prefix)
         files = files.order_by('filename')

=== modified file 'src/maasserver/models/filestorage.py'
--- src/maasserver/models/filestorage.py	2013-02-22 09:28:21 +0000
+++ src/maasserver/models/filestorage.py	2013-02-22 09:28:21 +0000
@@ -47,17 +47,17 @@
     old file will continue without iterruption.
     """
 
-    def save_file(self, filename, file_object):
+    def save_file(self, filename, file_object, owner):
         """Save the file to the database.
 
-        If a file of that name already existed, it will be replaced by the
-        new contents.
+        If a file of that name/owner already existed, it will be replaced by
+        the new contents.
         """
         # This probably ought to read in chunks but large files are
         # not expected.
         content = Bin(file_object.read())
         storage, created = self.get_or_create(
-            filename=filename, defaults={'content': content})
+            filename=filename, owner=owner, defaults={'content': content})
         if not created:
             storage.content = content
             storage.save()

=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2012-12-19 12:31:11 +0000
+++ src/maasserver/testing/factory.py	2013-02-22 09:28:21 +0000
@@ -330,9 +330,9 @@
         admin.save()
         return admin
 
-    def make_file_storage(self, filename=None, content=None):
+    def make_file_storage(self, filename=None, content=None, owner=None):
         fake_file = self.make_file_upload(filename, content)
-        return FileStorage.objects.save_file(fake_file.name, fake_file)
+        return FileStorage.objects.save_file(fake_file.name, fake_file, owner)
 
     def make_oauth_header(self, **kwargs):
         """Fake an OAuth authorization header.

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2013-02-14 12:42:30 +0000
+++ src/maasserver/tests/test_api.py	2013-02-22 09:28:21 +0000
@@ -46,7 +46,10 @@
     reverse,
     get_script_prefix,
     )
-from django.http import QueryDict
+from django.http import (
+    Http404,
+    QueryDict,
+    )
 from django.test.client import RequestFactory
 from django.utils.http import urlquote_plus
 from fixtures import (
@@ -62,6 +65,7 @@
     DISPLAYED_NODEGROUP_FIELDS,
     extract_constraints,
     find_nodegroup_for_pxeconfig_request,
+    get_owned_file_or_404,
     store_node_power_parameters,
     )
 from maasserver.enum import (
@@ -2621,37 +2625,82 @@
         return self.client.get(self.get_uri('files/'), params)
 
 
+class CompatibilityUtilitiesTest(TestCase):
+
+    def test_get_owned_file_or_404_returns_owned_file(self):
+        user = factory.make_user()
+        filename = factory.make_name('filename')
+        factory.make_file_storage(filename=filename, owner=None)
+        filestorage = factory.make_file_storage(filename=filename, owner=user)
+
+        self.assertEqual(filestorage, get_owned_file_or_404(filename, user))
+
+    def test_get_owned_file_or_404_fallsback_to_nonowned_file(self):
+        user = factory.make_user()
+        filename = factory.make_name('filename')
+        factory.make_file_storage(
+            filename=factory.make_name('filename'), owner=user)
+        filestorage = factory.make_file_storage(filename=filename, owner=None)
+
+        self.assertEqual(filestorage, get_owned_file_or_404(filename, user))
+
+    def test_get_owned_file_or_404_raises_404_if_no_file(self):
+        user = factory.make_user()
+        filename = factory.make_name('filename')
+        factory.make_file_storage(
+            filename=factory.make_name('filename'), owner=user)
+        factory.make_file_storage(
+            filename=filename, owner=factory.make_user())
+
+        self.assertRaises(Http404, get_owned_file_or_404, filename, user)
+
+    def test_get_owned_file_or_404_returns_values(self):
+        user = factory.make_user()
+        filename = factory.make_name('filename')
+        factory.make_file_storage(filename=filename, owner=None)
+        factory.make_file_storage(filename=filename, owner=user)
+
+        values = FileStorage.objects.filter(
+            filename=filename, owner=user).values()[0]
+        self.assertEqual(
+            values, get_owned_file_or_404(filename, user, values=True))
+
+
 class AnonymousFileStorageAPITest(FileStorageAPITestMixin, AnonAPITestCase):
 
     def test_get_works_anonymously(self):
-        factory.make_file_storage(
-            filename="foofilers", content=b"give me rope")
-        response = self.make_API_GET_request("get", "foofilers")
-
-        self.assertEqual(httplib.OK, response.status_code)
-        self.assertEqual(b"give me rope", response.content)
+        storage = factory.make_file_storage()
+        response = self.make_API_GET_request("get", storage.filename)
+
+        self.assertEqual(httplib.OK, response.status_code)
+        self.assertEqual(storage.content, response.content)
+
+    def test_get_fetches_the_most_recent_file(self):
+        filename = factory.make_name('file')
+        factory.make_file_storage(filename=filename, owner=factory.make_user())
+        storage = factory.make_file_storage(
+            filename=filename, owner=factory.make_user())
+        response = self.make_API_GET_request("get", filename)
+
+        self.assertEqual(httplib.OK, response.status_code)
+        self.assertEqual(storage.content, response.content)
 
     def test_anon_cannot_list_files(self):
-        factory.make_file_storage(
-             filename="filename", content=b"test content")
+        factory.make_file_storage()
         response = self.make_API_GET_request("list")
         # The 'list' operation is not available to anon users.
         self.assertEqual(httplib.BAD_REQUEST, response.status_code)
 
     def test_anon_cannot_get_file(self):
-        filename = factory.make_name("file")
-        factory.make_file_storage(
-            filename=filename, content=b"test file content")
+        storage = factory.make_file_storage()
         response = self.client.get(
-            reverse('file_handler', args=[filename]))
+            reverse('file_handler', args=[storage.filename]))
         self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
 
     def test_anon_cannot_delete_file(self):
-        filename = factory.make_name('file')
-        factory.make_file_storage(
-            filename=filename, content=b"test content")
+        storage = factory.make_file_storage()
         response = self.client.delete(
-            reverse('file_handler', args=[filename]))
+            reverse('file_handler', args=[storage.filename]))
         self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
 
 
@@ -2721,7 +2770,7 @@
 
         self.assertEqual(httplib.BAD_REQUEST, response.status_code)
         self.assertIn('text/plain', response['Content-Type'])
-        self.assertEqual("Filename not supplied", response.content)
+        self.assertEqual("No provided filename!", response.content)
 
     def test_get_file_fails_with_missing_file(self):
         response = self.make_API_GET_request("get", filename="missingfilename")
@@ -2734,19 +2783,28 @@
         filenames = ["myfiles/a", "myfiles/z", "myfiles/b"]
         for filename in filenames:
             factory.make_file_storage(
-                filename=filename, content=b"test content")
+                filename=filename, content=b"test content",
+                owner=self.logged_in_user)
         response = self.make_API_GET_request("list")
         self.assertEqual(httplib.OK, response.status_code)
         parsed_results = json.loads(response.content)
         filenames = [result['filename'] for result in parsed_results]
         self.assertEqual(sorted(filenames), filenames)
 
+    def test_list_files_filters_by_owner(self):
+        factory.make_file_storage(owner=factory.make_user())
+        response = self.make_API_GET_request("list")
+        self.assertEqual(httplib.OK, response.status_code)
+        parsed_results = json.loads(response.content)
+        self.assertEqual([], parsed_results)
+
     def test_list_files_lists_files_with_prefix(self):
         filenames_with_prefix = ["prefix-file1", "prefix-file2"]
         filenames = filenames_with_prefix + ["otherfile", "otherfile2"]
         for filename in filenames:
             factory.make_file_storage(
-                filename=filename, content=b"test content")
+                filename=filename, content=b"test content",
+                owner=self.logged_in_user)
         response = self.client.get(
             self.get_uri('files/'), {"op": "list", "prefix": "prefix-"})
         self.assertEqual(httplib.OK, response.status_code)
@@ -2756,7 +2814,8 @@
 
     def test_list_files_does_not_include_file_content(self):
         factory.make_file_storage(
-             filename="filename", content=b"test content")
+             filename="filename", content=b"test content",
+             owner=self.logged_in_user)
         response = self.make_API_GET_request("list")
         parsed_results = json.loads(response.content)
         self.assertNotIn('content', parsed_results[0].keys())
@@ -2764,7 +2823,8 @@
     def test_files_resource_uri_are_url_escaped(self):
         filename = "&*!c/%//filename/"
         factory.make_file_storage(
-             filename=filename, content=b"test content")
+             filename=filename, content=b"test content",
+             owner=self.logged_in_user)
         response = self.make_API_GET_request("list")
         parsed_results = json.loads(response.content)
         resource_uri = parsed_results[0]['resource_uri']
@@ -2775,21 +2835,80 @@
     def test_get_file_returns_file_object_with_content_base64_encoded(self):
         filename = factory.make_name("file")
         content = sample_binary_data
-        factory.make_file_storage(filename=filename, content=content)
-        response = self.client.get(
-            reverse('file_handler', args=[filename]))
-        parsed_result = json.loads(response.content)
-        self.assertEqual(
-            (filename, content),
-            (
-                parsed_result['filename'],
-                b64decode(parsed_result['content'])
-            ))
+        factory.make_file_storage(
+            filename=filename, content=content, owner=self.logged_in_user)
+        response = self.client.get(
+            reverse('file_handler', args=[filename]))
+        parsed_result = json.loads(response.content)
+        self.assertEqual(
+            (filename, content),
+            (
+                parsed_result['filename'],
+                b64decode(parsed_result['content'])
+            ))
+
+    def test_get_file_returns_owned_file(self):
+        # If both an owned file and a non-owned file are present (with the
+        # same name), the owned file is returned.
+        filename = factory.make_name("file")
+        factory.make_file_storage(filename=filename, owner=None)
+        content = sample_binary_data
+        factory.make_file_storage(
+            filename=filename, content=content, owner=self.logged_in_user)
+        response = self.client.get(
+            reverse('file_handler', args=[filename]))
+        parsed_result = json.loads(response.content)
+        self.assertEqual(
+            (filename, content),
+            (
+                parsed_result['filename'],
+                b64decode(parsed_result['content'])
+            ))
+
+    def test_get_file_returns_file_object_with_anon_owner(self):
+        # To be compatible with the MAAS provider present in Juju as of
+        # revision 616 (lp:juju), reading a file returns the non-owned files
+        # if no owned file can be found.
+        filename = factory.make_name("file")
+        content = sample_binary_data
+        factory.make_file_storage(
+            filename=filename, content=content, owner=None)
+        response = self.client.get(
+            reverse('file_handler', args=[filename]))
+        parsed_result = json.loads(response.content)
+        self.assertEqual(
+            (filename, content),
+            (
+                parsed_result['filename'],
+                b64decode(parsed_result['content'])
+            ))
+
+    def test_delete_filters_by_owner(self):
+        storage = factory.make_file_storage(owner=factory.make_user())
+        response = self.client.delete(
+            reverse('file_handler', args=[storage.filename]))
+        self.assertEqual(httplib.NOT_FOUND, response.status_code)
+        files = FileStorage.objects.filter(filename=storage.filename)
+        self.assertEqual([storage], list(files))
 
     def test_delete_file_deletes_file(self):
         filename = factory.make_name('file')
         factory.make_file_storage(
-            filename=filename, content=b"test content")
+            filename=filename, content=b"test content",
+            owner=self.logged_in_user)
+        response = self.client.delete(
+            reverse('file_handler', args=[filename]))
+        self.assertEqual(httplib.NO_CONTENT, response.status_code)
+        files = FileStorage.objects.filter(filename=filename)
+        self.assertEqual([], list(files))
+
+    def test_delete_file_deletes_file_with_anon_owner(self):
+        # To be compatible with the MAAS provider present in Juju as of
+        # revision 616 (lp:juju), deleting a file deletes the non-owned file
+        # if no owned file can be found.
+        filename = factory.make_name('file')
+        factory.make_file_storage(
+            filename=filename, content=b"test content", owner=None)
         response = self.client.delete(
             reverse('file_handler', args=[filename]))
         self.assertEqual(httplib.NO_CONTENT, response.status_code)

=== modified file 'src/maasserver/tests/test_filestorage.py'
--- src/maasserver/tests/test_filestorage.py	2013-02-22 09:28:21 +0000
+++ src/maasserver/tests/test_filestorage.py	2013-02-22 09:28:21 +0000
@@ -40,10 +40,12 @@
     def test_save_file_creates_storage(self):
         filename = factory.getRandomString()
         content = self.make_data()
-        storage = FileStorage.objects.save_file(filename, BytesIO(content))
+        user = factory.make_user()
+        storage = FileStorage.objects.save_file(
+            filename, BytesIO(content), user)
         self.assertEqual(
-            (filename, content),
-            (storage.filename, storage.content))
+            (filename, content, user),
+            (storage.filename, storage.content, storage.owner))
 
     def test_storage_can_be_retrieved(self):
         filename = factory.getRandomString()