Merge lp:~julian-edwards/maas/anon-get-file into lp:~maas-committers/maas/trunk

[Julian Edwards]

Allow anonymous filestorage retreival since Juju needs it.

[Gavin Panella (allenap)]

Landing this, will comment afterwards ;)

review: Approve

[Gavin Panella (allenap)]

Looks good. It has already landed, but I have promised to follow-up
and repair according to my review comments. Hence these comments are
as much instructions for me.

[1]

+def get_file(request):
+ filename = request.GET.get("filename", None)

Could do with a docstring, though I guess it's fairly
self-explanatory.

[2]

+@api_operations
+class AnonFilesHandler(AnonymousBaseHandler):
+ """Anonymous file operations."""
+ allowed_methods = ('GET',)

This really needs explanation as to why it's a sensible thing to
do. From talking with Francis and Kapil, it seems that (a) file names
will be uuid-like or contain some unguessable nonce, and (b) this is
required so that all the agents within the environment can see the
file without needing provider credentials.

[3]

+ def setUp(self):
...
+ os.mkdir(self.tmpdir)

This is repeated twice, and ought to be extracted.

[Julian Edwards (julian-edwards)]

Thanks Gavin.

> This really needs explanation as to why it's a sensible thing to
> do. From talking with Francis and Kapil, it seems that (a) file names
> will be uuid-like or contain some unguessable nonce, and (b) this is
> required so that all the agents within the environment can see the
> file without needing provider credentials.

Yes, I had already talked about this with the Juju guys and there are
basically 2 choices here:

1. Anonymous access to get files (this is what Orchestra does).
2. We put an OAuth token in the URL itself (this is what EC2 does).

The former is massively easier at this moment! We can do #2 when there's more
time and it can be a v1.1 API.

I hate rushing crap through to meet deadlines :(

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-03-21 05:56:36 +0000
+++ src/maasserver/api.py	2012-03-22 10:08:20 +0000
@@ -479,10 +479,38 @@
         return ('node_mac_handler', [node_system_id, mac_address])
 
 
+def get_file(request):
+    filename = request.GET.get("filename", None)
+    if not filename:
+        raise MAASAPIBadRequest("Filename not supplied")
+    try:
+        db_file = FileStorage.objects.get(filename=filename)
+    except FileStorage.DoesNotExist:
+        raise MAASAPINotFound("File not found")
+    return HttpResponse(db_file.data.read(), status=httplib.OK)
+
+
+@api_operations
+class AnonFilesHandler(AnonymousBaseHandler):
+    """Anonymous file operations."""
+    allowed_methods = ('GET',)
+
+    @api_exported('get', 'GET')
+    def get(self, 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.
+        """
+        return get_file(request)
+
+
 @api_operations
 class FilesHandler(BaseHandler):
     """File management operations."""
     allowed_methods = ('GET', 'POST',)
+    anonymous = AnonFilesHandler
 
     @api_exported('get', 'GET')
     def get(self, request):
@@ -492,14 +520,7 @@
         :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")
-        try:
-            db_file = FileStorage.objects.get(filename=filename)
-        except FileStorage.DoesNotExist:
-            raise MAASAPINotFound("File not found")
-        return HttpResponse(db_file.data.read(), status=httplib.OK)
+        return get_file(request)
 
     @api_exported('add', 'POST')
     def add(self, request):

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-03-21 05:56:36 +0000
+++ src/maasserver/tests/test_api.py	2012-03-22 10:08:20 +0000
@@ -836,16 +836,7 @@
         self.assertEqual(httplib.BAD_REQUEST, response.status_code)
 
 
-class FileStorageAPITest(APITestCase):
-
-    def setUp(self):
-        super(FileStorageAPITest, self).setUp()
-        media_root = settings.MEDIA_ROOT
-        self.assertFalse(os.path.exists(media_root), "See media/README")
-        self.addCleanup(shutil.rmtree, media_root, ignore_errors=True)
-        os.mkdir(media_root)
-        self.tmpdir = os.path.join(media_root, "testing")
-        os.mkdir(self.tmpdir)
+class FileStorageTestMixin:
 
     def make_file(self, name="foo", contents="test file contents"):
         """Make a temp file named `name` with contents `contents`.
@@ -877,6 +868,38 @@
         params = self._create_API_params(op, filename, fileObj)
         return self.client.get(self.get_uri('files/'), params)
 
+
+class AnonymousFileStorageAPITest(APIv10TestMixin, FileStorageTestMixin,
+                                  TestCase):
+
+    def setUp(self):
+        super(AnonymousFileStorageAPITest, self).setUp()
+        media_root = settings.MEDIA_ROOT
+        self.assertFalse(os.path.exists(media_root), "See media/README")
+        self.addCleanup(shutil.rmtree, media_root, ignore_errors=True)
+        os.mkdir(media_root)
+        self.tmpdir = os.path.join(media_root, "testing")
+        os.mkdir(self.tmpdir)
+
+    def test_get_works_anonymously(self):
+        factory.make_file_storage(filename="foofilers", data=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)
+
+
+class FileStorageAPITest(APITestCase, FileStorageTestMixin):
+
+    def setUp(self):
+        super(FileStorageAPITest, self).setUp()
+        media_root = settings.MEDIA_ROOT
+        self.assertFalse(os.path.exists(media_root), "See media/README")
+        self.addCleanup(shutil.rmtree, media_root, ignore_errors=True)
+        os.mkdir(media_root)
+        self.tmpdir = os.path.join(media_root, "testing")
+        os.mkdir(self.tmpdir)
+
     def test_add_file_succeeds(self):
         filepath = self.make_file()