Merge lp:~dooferlad/linaro-license-protection/public_push_support into lp:~linaro-automation/linaro-license-protection/trunk

[James Tunnicliffe]

* Tidy up a few bits from the old publishing code (so the old API should function against upload paths - untested)
* Should work for public publishing - when getting a key, have &public=true on the end of the URL.
* Haven't updated the docs, just found that explaining a proposed change took more effort than writing it!

[Milo Casagrande (milo)]

Hi James,

overall it looks good, a couple of suggestion follows.

On Wed, Jul 17, 2013 at 5:29 PM, James Tunnicliffe
<email address hidden> wrote:
>
> + def test_get_public_key_post_and_get_file(self):
> + response = self.client.get("http://testserver/api/request_key",
> + data={"key": settings.MASTER_API_KEY,
> + "public": ""})
> +
> + self.assertEqual(response.status_code, 200)
> + # Don't care what the key is, as long as it isn't blank
> + self.assertRegexpMatches(response.content, "\S+")
> + key = response.content
> +
> + # Now write a file so we can upload it
> + file_content = "test_get_key_post_and_get_file"
> + file_root = "/tmp"
> +
> + tmp_file_name = os.path.join(
> + file_root,
> + self.make_temporary_file(file_content))
> +
> + buildinfo_content = "\n".join([
> + "Format-Version: 0.1",
> + "Files-Pattern: *",
> + "Build-Name: test",
> + "License-Type: open"])
> + tmp_build_info = os.path.join(
> + file_root,
> + self.make_temporary_file(buildinfo_content))
> +
> + # Send the files
> + with open(tmp_file_name) as f:
> + response = self.client.post("http://testserver/pub/file_name",
> + data={"key": key, "file": f})
> + self.assertEqual(response.status_code, 200)
> +
> + with open(tmp_build_info) as f:
> + response = self.client.post("http://testserver/pub/BUILD-INFO.txt",
> + data={"key": key, "file": f})
> + self.assertEqual(response.status_code, 200)
> +
> + # Check the upload worked by reading the file back from its uploaded
> + # location
> + uploaded_file_path = os.path.join(settings.SERVED_PATHS[0],
> + "file_name")
> +
> + with open(uploaded_file_path) as f:
> + self.assertEqual(f.read(), file_content)
> +
> + # Test we can fetch the newly uploaded file
> + response = self.client.get("http://testserver/pub/file_name")
> + self.assertEqual(response.status_code, 200)
> +
> + # Delete the files generated by the test
> + shutil.rmtree(os.path.join(settings.SERVED_PATHS[0], "pub"))

Wouldn't it be better to have all the logic of this test method inside
a try-finally?
I'm concerned with an error that can happen, for whatever reason,
somewhere before the call to shutil.rmtree, leaving cruft on the file
system.

> === modified file 'license_protected_downloads/uploads.py'
>
>
> @@ -41,11 +44,14 @@
> APIKeyStore.objects.filter(key=request.POST["key"])):
> return HttpResponseServerError("Invalid key")
>
> + api_key = APIKeyStore.objects.filter(key=request.POST["key"])
> +

Does the call to APIKeyStore.objects.filter get cached in some way?
The problem is that the other solution I have in mind means to always
call APIKeyStore.objects.filter even if "key in request.POST" is
False.

--
Milo Casagrande | Automation Engineer
Linaro.org <www.li...

[Milo Casagrande (milo)]

Needs fixing, at least for the try-finally as said above.

review: Needs Fixing

[James Tunnicliffe (dooferlad)]

On 22 July 2013 16:04, Milo Casagrande <email address hidden> wrote:
> Hi James,
>
> overall it looks good, a couple of suggestion follows.
>
> On Wed, Jul 17, 2013 at 5:29 PM, James Tunnicliffe
> <email address hidden> wrote:
>>
>> + def test_get_public_key_post_and_get_file(self):
>> + response = self.client.get("http://testserver/api/request_key",
>> + data={"key": settings.MASTER_API_KEY,
>> + "public": ""})
>> +
>> + self.assertEqual(response.status_code, 200)
>> + # Don't care what the key is, as long as it isn't blank
>> + self.assertRegexpMatches(response.content, "\S+")
>> + key = response.content
>> +
>> + # Now write a file so we can upload it
>> + file_content = "test_get_key_post_and_get_file"
>> + file_root = "/tmp"
>> +
>> + tmp_file_name = os.path.join(
>> + file_root,
>> + self.make_temporary_file(file_content))
>> +
>> + buildinfo_content = "\n".join([
>> + "Format-Version: 0.1",
>> + "Files-Pattern: *",
>> + "Build-Name: test",
>> + "License-Type: open"])
>> + tmp_build_info = os.path.join(
>> + file_root,
>> + self.make_temporary_file(buildinfo_content))
>> +
>> + # Send the files
>> + with open(tmp_file_name) as f:
>> + response = self.client.post("http://testserver/pub/file_name",
>> + data={"key": key, "file": f})
>> + self.assertEqual(response.status_code, 200)
>> +
>> + with open(tmp_build_info) as f:
>> + response = self.client.post("http://testserver/pub/BUILD-INFO.txt",
>> + data={"key": key, "file": f})
>> + self.assertEqual(response.status_code, 200)
>> +
>> + # Check the upload worked by reading the file back from its uploaded
>> + # location
>> + uploaded_file_path = os.path.join(settings.SERVED_PATHS[0],
>> + "file_name")
>> +
>> + with open(uploaded_file_path) as f:
>> + self.assertEqual(f.read(), file_content)
>> +
>> + # Test we can fetch the newly uploaded file
>> + response = self.client.get("http://testserver/pub/file_name")
>> + self.assertEqual(response.status_code, 200)
>> +
>> + # Delete the files generated by the test
>> + shutil.rmtree(os.path.join(settings.SERVED_PATHS[0], "pub"))
>
>
> Wouldn't it be better to have all the logic of this test method inside
> a try-finally?
> I'm concerned with an error that can happen, for whatever reason,
> somewhere before the call to shutil.rmtree, leaving cruft on the file
> system.

Good point. Will clean that up.

>> === modified file 'license_protected_downloads/uploads.py'
>>
>>
>> @@ -41,11 +44,14 @@
>> APIKeyStore.objects.filter(key=request.POST["key"])):
>> return HttpResponseServerError("Invalid key")
>>
>> + api_key = APIKeyStore.objects.filter(key=request.POST["key"])
>> +
>
> Does the call to APIKeyStore.objects.filter get cached in some way?
> The pro...

[James Tunnicliffe (dooferlad)]

Actually I should put the tests in a new class with a tearDown that
removes extra directories - it is called even if there is an
exception.

On 22 July 2013 16:31, James Tunnicliffe <email address hidden> wrote:
> On 22 July 2013 16:04, Milo Casagrande <email address hidden> wrote:
>> Hi James,
>>
>> overall it looks good, a couple of suggestion follows.
>>
>> On Wed, Jul 17, 2013 at 5:29 PM, James Tunnicliffe
>> <email address hidden> wrote:
>>>
>>> + def test_get_public_key_post_and_get_file(self):
>>> + response = self.client.get("http://testserver/api/request_key",
>>> + data={"key": settings.MASTER_API_KEY,
>>> + "public": ""})
>>> +
>>> + self.assertEqual(response.status_code, 200)
>>> + # Don't care what the key is, as long as it isn't blank
>>> + self.assertRegexpMatches(response.content, "\S+")
>>> + key = response.content
>>> +
>>> + # Now write a file so we can upload it
>>> + file_content = "test_get_key_post_and_get_file"
>>> + file_root = "/tmp"
>>> +
>>> + tmp_file_name = os.path.join(
>>> + file_root,
>>> + self.make_temporary_file(file_content))
>>> +
>>> + buildinfo_content = "\n".join([
>>> + "Format-Version: 0.1",
>>> + "Files-Pattern: *",
>>> + "Build-Name: test",
>>> + "License-Type: open"])
>>> + tmp_build_info = os.path.join(
>>> + file_root,
>>> + self.make_temporary_file(buildinfo_content))
>>> +
>>> + # Send the files
>>> + with open(tmp_file_name) as f:
>>> + response = self.client.post("http://testserver/pub/file_name",
>>> + data={"key": key, "file": f})
>>> + self.assertEqual(response.status_code, 200)
>>> +
>>> + with open(tmp_build_info) as f:
>>> + response = self.client.post("http://testserver/pub/BUILD-INFO.txt",
>>> + data={"key": key, "file": f})
>>> + self.assertEqual(response.status_code, 200)
>>> +
>>> + # Check the upload worked by reading the file back from its uploaded
>>> + # location
>>> + uploaded_file_path = os.path.join(settings.SERVED_PATHS[0],
>>> + "file_name")
>>> +
>>> + with open(uploaded_file_path) as f:
>>> + self.assertEqual(f.read(), file_content)
>>> +
>>> + # Test we can fetch the newly uploaded file
>>> + response = self.client.get("http://testserver/pub/file_name")
>>> + self.assertEqual(response.status_code, 200)
>>> +
>>> + # Delete the files generated by the test
>>> + shutil.rmtree(os.path.join(settings.SERVED_PATHS[0], "pub"))
>>
>>
>> Wouldn't it be better to have all the logic of this test method inside
>> a try-finally?
>> I'm concerned with an error that can happen, for whatever reason,
>> somewhere before the call to shutil.rmtree, leaving cruft on the file
>> system.
>
> Good point. Will clean that up.
>
>>> === modified file 'license_protected_downloads/uploads.py'
>>>
>>>
>>> @@ -41,...

[Milo Casagrande (milo)]

On Mon, Jul 22, 2013 at 5:34 PM, James Tunnicliffe
<email address hidden> wrote:
>> Does the call to APIKeyStore.objects.filter get cached in some way?
>> The problem is that the other solution I have in mind means to always
>> call APIKeyStore.objects.filter even if "key in request.POST" is
>> False.
>
> You couldn't make the call - if request.POST["key"] didn't exist, it
> would raise an exception. If you had no filter for the key value, the
> filter would match everything, which would be a bad thing.

Righto, it is the same key.

On Mon, Jul 22, 2013 at 5:37 PM, James Tunnicliffe
<email address hidden> wrote:
> Actually I should put the tests in a new class with a tearDown that
> removes extra directories - it is called even if there is an
> exception.

Even better!

--
Milo Casagrande | Automation Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

[Milo Casagrande (milo)]

Spoke with James about this merge proposal: I will merge it and fix test issue discovered.

review: Approve

=== modified file 'license_protected_downloads/models.py'
--- license_protected_downloads/models.py	2013-06-05 13:16:41 +0000
+++ license_protected_downloads/models.py	2013-07-17 15:28:26 +0000
@@ -30,3 +30,4 @@
 
 class APIKeyStore(models.Model):
     key = models.CharField(max_length=80)
+    public = models.BooleanField()
 
=== modified file 'license_protected_downloads/tests/test_views.py'
--- license_protected_downloads/tests/test_views.py	2013-06-14 12:39:29 +0000
+++ license_protected_downloads/tests/test_views.py	2013-07-17 15:28:26 +0000
@@ -907,6 +907,59 @@
         # Delete the files generated by the test
         shutil.rmtree(os.path.join(settings.UPLOAD_PATH, key))
 
+    def test_get_public_key_post_and_get_file(self):
+        response = self.client.get("http://testserver/api/request_key",
+                                   data={"key": settings.MASTER_API_KEY,
+                                         "public": ""})
+
+        self.assertEqual(response.status_code, 200)
+        # Don't care what the key is, as long as it isn't blank
+        self.assertRegexpMatches(response.content, "\S+")
+        key = response.content
+
+        # Now write a file so we can upload it
+        file_content = "test_get_key_post_and_get_file"
+        file_root = "/tmp"
+
+        tmp_file_name = os.path.join(
+            file_root,
+            self.make_temporary_file(file_content))
+
+        buildinfo_content = "\n".join([
+            "Format-Version: 0.1",
+            "Files-Pattern: *",
+            "Build-Name: test",
+            "License-Type: open"])
+        tmp_build_info = os.path.join(
+            file_root,
+            self.make_temporary_file(buildinfo_content))
+
+        # Send the files
+        with open(tmp_file_name) as f:
+            response = self.client.post("http://testserver/pub/file_name",
+                             data={"key": key, "file": f})
+            self.assertEqual(response.status_code, 200)
+
+        with open(tmp_build_info) as f:
+            response = self.client.post("http://testserver/pub/BUILD-INFO.txt",
+                             data={"key": key, "file": f})
+            self.assertEqual(response.status_code, 200)
+
+        # Check the upload worked by reading the file back from its uploaded
+        # location
+        uploaded_file_path = os.path.join(settings.SERVED_PATHS[0],
+                                          "file_name")
+
+        with open(uploaded_file_path) as f:
+            self.assertEqual(f.read(), file_content)
+
+        # Test we can fetch the newly uploaded file
+        response = self.client.get("http://testserver/pub/file_name")
+        self.assertEqual(response.status_code, 200)
+
+        # Delete the files generated by the test
+        shutil.rmtree(os.path.join(settings.SERVED_PATHS[0], "pub"))
+
     def test_post_file_no_key(self):
         file_content = "test_post_file_no_key"
         file_root = "/tmp"
 
=== modified file 'license_protected_downloads/uploads.py'
--- license_protected_downloads/uploads.py	2013-06-11 15:03:01 +0000
+++ license_protected_downloads/uploads.py	2013-07-17 15:28:26 +0000
@@ -17,13 +17,16 @@
     file = forms.FileField()
 
 
-def upload_target_path(path, key):
+def upload_target_path(path, key, public):
     """Quick path handling function.
 
     Checks that the generated path doesn't end up outside the target directory,
     so you can't set path to start with "/" and upload to anywhere.
     """
-    base_path = os.path.join(settings.UPLOAD_PATH, key)
+    if public:
+        base_path = os.path.join(settings.SERVED_PATHS[0])
+    else:
+        base_path = os.path.join(settings.UPLOAD_PATH, key)
     return safe_path_join(base_path, path)
 
 
@@ -41,11 +44,14 @@
         APIKeyStore.objects.filter(key=request.POST["key"])):
         return HttpResponseServerError("Invalid key")
 
+    api_key = APIKeyStore.objects.filter(key=request.POST["key"])
+
     form = UploadFileForm(request.POST, request.FILES)
     if not form.is_valid() or not path:
         return HttpResponseServerError("Invalid call")
 
-    path = upload_target_path(path, request.POST["key"])
+    path = upload_target_path(
+        path, request.POST["key"], public=api_key[0].public)
 
     # Create directory if required
     dirname = os.path.dirname(path)
@@ -69,7 +75,15 @@
         while APIKeyStore.objects.filter(key=key):
             key = "%030x" % random.randrange(256**15)
 
-        api_key = APIKeyStore(key=key)
+        # Look for a hint of sanity in the value given to public, but don't
+        # care about it too much.
+        yes = ["", "y", "yes", "true", "1"]
+        if "public" in request.GET:
+            public = request.GET["public"].lower() in yes
+        else:
+            public = False
+
+        api_key = APIKeyStore(key=key, public=public)
         api_key.save()
         return HttpResponse(key)
 
 
=== modified file 'license_protected_downloads/views.py'
--- license_protected_downloads/views.py	2013-06-19 09:51:06 +0000
+++ license_protected_downloads/views.py	2013-07-17 15:28:26 +0000
@@ -24,7 +24,7 @@
 import bzr_version
 from buildinfo import BuildInfo, IncorrectDataFormatException
 from render_text_files import RenderTextFiles
-from models import License
+from models import License, APIKeyStore
 # Load group auth "plugin" dynamically
 import importlib
 group_auth_modules = [importlib.import_module(m) for m in settings.GROUP_AUTH_MODULES]
@@ -136,13 +136,25 @@
     return listing
 
 
-def test_path(path, served_paths=None):
+def test_path(path, request, served_paths=None):
     """Check that path points to something we can serve up.
 
     served_paths can be provided to overwrite settings.SERVED_PATHS. This is
     used for uploaded files, which may not be shared in the server root.
     """
 
+    # if key is in request.GET["key"] then need to mod path and give
+    # access to a per-key directory.
+    if "key" in request.GET:
+        key_details = APIKeyStore.objects.filter(key=request.GET["key"])
+        if key_details:
+            path = os.path.join(request.GET["key"], path)
+
+            # Private uploads are in a separate path (or can be), so set
+            # served_paths as needed.
+            if key_details[0].public == False:
+                served_paths = settings.UPLOAD_PATH
+
     if served_paths is None:
         served_paths = settings.SERVED_PATHS
     else:
@@ -458,14 +470,7 @@
 def file_server_get(request, path):
 
     url = path
-
-    # if key is in request.GET["key"] then need to mod path and give
-    # access to a per-key directory.
-    if "key" in request.GET:
-        path = os.path.join(request.GET["key"], path)
-        result = test_path(path, settings.UPLOAD_PATH)
-    else:
-        result = test_path(path)
+    result = test_path(path, request)
 
     if not result:
         raise Http404
@@ -572,7 +577,7 @@
 
 def get_textile_files(request):
 
-    result = test_path(request.GET.get("path"))
+    result = test_path(request.GET.get("path"), request)
     if not result:
         raise Http404
 
@@ -600,7 +605,7 @@
 def list_files_api(request, path):
     path = iri_to_uri(path)
     url = path
-    result = test_path(path)
+    result = test_path(path, request)
     if not result:
         raise Http404
 
@@ -643,7 +648,7 @@
 
 def get_license_api(request, path):
     path = iri_to_uri(path)
-    result = test_path(path)
+    result = test_path(path, request)
     if not result:
         raise Http404