Merge lp:~dooferlad/linaro-license-protection/public_push_support into lp:~linaro-automation/linaro-license-protection/trunk
- public_push_support
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Milo Casagrande |
Approved revision: | 204 |
Merged at revision: | 209 |
Proposed branch: | lp:~dooferlad/linaro-license-protection/public_push_support |
Merge into: | lp:~linaro-automation/linaro-license-protection/trunk |
Diff against target: |
210 lines (+90/-17) 4 files modified
license_protected_downloads/models.py (+1/-0) license_protected_downloads/tests/test_views.py (+53/-0) license_protected_downloads/uploads.py (+18/-4) license_protected_downloads/views.py (+18/-13) |
To merge this branch: | bzr merge lp:~dooferlad/linaro-license-protection/public_push_support |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Milo Casagrande (community) | Approve | ||
Review via email: mp+175319@code.launchpad.net |
Commit message
Description of the change
* 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) wrote : | # |
Milo Casagrande (milo) wrote : | # |
Needs fixing, at least for the try-finally as said above.
James Tunnicliffe (dooferlad) 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_
>> + response = self.client.get("http://
>> + data={"key": settings.
>> + "public": ""})
>> +
>> + self.assertEqua
>> + # Don't care what the key is, as long as it isn't blank
>> + self.assertRege
>> + key = response.content
>> +
>> + # Now write a file so we can upload it
>> + file_content = "test_get_
>> + file_root = "/tmp"
>> +
>> + tmp_file_name = os.path.join(
>> + file_root,
>> + self.make_
>> +
>> + 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_
>> +
>> + # Send the files
>> + with open(tmp_file_name) as f:
>> + response = self.client.post("http://
>> + data={"key": key, "file": f})
>> + self.assertEqua
>> +
>> + with open(tmp_
>> + response = self.client.post("http://
>> + data={"key": key, "file": f})
>> + self.assertEqua
>> +
>> + # Check the upload worked by reading the file back from its uploaded
>> + # location
>> + uploaded_file_path = os.path.
>> + "file_name")
>> +
>> + with open(uploaded_
>> + self.assertEqua
>> +
>> + # Test we can fetch the newly uploaded file
>> + response = self.client.get("http://
>> + self.assertEqua
>> +
>> + # Delete the files generated by the test
>> + shutil.
>
>
> 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_
>>
>>
>> @@ -41,11 +44,14 @@
>> APIKeyStore.
>> return HttpResponseSer
>>
>> + api_key = APIKeyStore.
>> +
>
> Does the call to APIKeyStore.
> The pro...
James Tunnicliffe (dooferlad) 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.
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_
>>> + response = self.client.get("http://
>>> + data={"key": settings.
>>> + "public": ""})
>>> +
>>> + self.assertEqua
>>> + # Don't care what the key is, as long as it isn't blank
>>> + self.assertRege
>>> + key = response.content
>>> +
>>> + # Now write a file so we can upload it
>>> + file_content = "test_get_
>>> + file_root = "/tmp"
>>> +
>>> + tmp_file_name = os.path.join(
>>> + file_root,
>>> + self.make_
>>> +
>>> + 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_
>>> +
>>> + # Send the files
>>> + with open(tmp_file_name) as f:
>>> + response = self.client.post("http://
>>> + data={"key": key, "file": f})
>>> + self.assertEqua
>>> +
>>> + with open(tmp_
>>> + response = self.client.post("http://
>>> + data={"key": key, "file": f})
>>> + self.assertEqua
>>> +
>>> + # Check the upload worked by reading the file back from its uploaded
>>> + # location
>>> + uploaded_file_path = os.path.
>>> + "file_name")
>>> +
>>> + with open(uploaded_
>>> + self.assertEqua
>>> +
>>> + # Test we can fetch the newly uploaded file
>>> + response = self.client.get("http://
>>> + self.assertEqua
>>> +
>>> + # Delete the files generated by the test
>>> + shutil.
>>
>>
>> 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_
>>>
>>>
>>> @@ -41,...
Milo Casagrande (milo) wrote : | # |
On Mon, Jul 22, 2013 at 5:34 PM, James Tunnicliffe
<email address hidden> wrote:
>> Does the call to APIKeyStore.
>> The problem is that the other solution I have in mind means to always
>> call APIKeyStore.
>> 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) wrote : | # |
Spoke with James about this merge proposal: I will merge it and fix test issue discovered.
Preview Diff
1 | === modified file 'license_protected_downloads/models.py' |
2 | --- license_protected_downloads/models.py 2013-06-05 13:16:41 +0000 |
3 | +++ license_protected_downloads/models.py 2013-07-17 15:28:26 +0000 |
4 | @@ -30,3 +30,4 @@ |
5 | |
6 | class APIKeyStore(models.Model): |
7 | key = models.CharField(max_length=80) |
8 | + public = models.BooleanField() |
9 | |
10 | === modified file 'license_protected_downloads/tests/test_views.py' |
11 | --- license_protected_downloads/tests/test_views.py 2013-06-14 12:39:29 +0000 |
12 | +++ license_protected_downloads/tests/test_views.py 2013-07-17 15:28:26 +0000 |
13 | @@ -907,6 +907,59 @@ |
14 | # Delete the files generated by the test |
15 | shutil.rmtree(os.path.join(settings.UPLOAD_PATH, key)) |
16 | |
17 | + def test_get_public_key_post_and_get_file(self): |
18 | + response = self.client.get("http://testserver/api/request_key", |
19 | + data={"key": settings.MASTER_API_KEY, |
20 | + "public": ""}) |
21 | + |
22 | + self.assertEqual(response.status_code, 200) |
23 | + # Don't care what the key is, as long as it isn't blank |
24 | + self.assertRegexpMatches(response.content, "\S+") |
25 | + key = response.content |
26 | + |
27 | + # Now write a file so we can upload it |
28 | + file_content = "test_get_key_post_and_get_file" |
29 | + file_root = "/tmp" |
30 | + |
31 | + tmp_file_name = os.path.join( |
32 | + file_root, |
33 | + self.make_temporary_file(file_content)) |
34 | + |
35 | + buildinfo_content = "\n".join([ |
36 | + "Format-Version: 0.1", |
37 | + "Files-Pattern: *", |
38 | + "Build-Name: test", |
39 | + "License-Type: open"]) |
40 | + tmp_build_info = os.path.join( |
41 | + file_root, |
42 | + self.make_temporary_file(buildinfo_content)) |
43 | + |
44 | + # Send the files |
45 | + with open(tmp_file_name) as f: |
46 | + response = self.client.post("http://testserver/pub/file_name", |
47 | + data={"key": key, "file": f}) |
48 | + self.assertEqual(response.status_code, 200) |
49 | + |
50 | + with open(tmp_build_info) as f: |
51 | + response = self.client.post("http://testserver/pub/BUILD-INFO.txt", |
52 | + data={"key": key, "file": f}) |
53 | + self.assertEqual(response.status_code, 200) |
54 | + |
55 | + # Check the upload worked by reading the file back from its uploaded |
56 | + # location |
57 | + uploaded_file_path = os.path.join(settings.SERVED_PATHS[0], |
58 | + "file_name") |
59 | + |
60 | + with open(uploaded_file_path) as f: |
61 | + self.assertEqual(f.read(), file_content) |
62 | + |
63 | + # Test we can fetch the newly uploaded file |
64 | + response = self.client.get("http://testserver/pub/file_name") |
65 | + self.assertEqual(response.status_code, 200) |
66 | + |
67 | + # Delete the files generated by the test |
68 | + shutil.rmtree(os.path.join(settings.SERVED_PATHS[0], "pub")) |
69 | + |
70 | def test_post_file_no_key(self): |
71 | file_content = "test_post_file_no_key" |
72 | file_root = "/tmp" |
73 | |
74 | === modified file 'license_protected_downloads/uploads.py' |
75 | --- license_protected_downloads/uploads.py 2013-06-11 15:03:01 +0000 |
76 | +++ license_protected_downloads/uploads.py 2013-07-17 15:28:26 +0000 |
77 | @@ -17,13 +17,16 @@ |
78 | file = forms.FileField() |
79 | |
80 | |
81 | -def upload_target_path(path, key): |
82 | +def upload_target_path(path, key, public): |
83 | """Quick path handling function. |
84 | |
85 | Checks that the generated path doesn't end up outside the target directory, |
86 | so you can't set path to start with "/" and upload to anywhere. |
87 | """ |
88 | - base_path = os.path.join(settings.UPLOAD_PATH, key) |
89 | + if public: |
90 | + base_path = os.path.join(settings.SERVED_PATHS[0]) |
91 | + else: |
92 | + base_path = os.path.join(settings.UPLOAD_PATH, key) |
93 | return safe_path_join(base_path, path) |
94 | |
95 | |
96 | @@ -41,11 +44,14 @@ |
97 | APIKeyStore.objects.filter(key=request.POST["key"])): |
98 | return HttpResponseServerError("Invalid key") |
99 | |
100 | + api_key = APIKeyStore.objects.filter(key=request.POST["key"]) |
101 | + |
102 | form = UploadFileForm(request.POST, request.FILES) |
103 | if not form.is_valid() or not path: |
104 | return HttpResponseServerError("Invalid call") |
105 | |
106 | - path = upload_target_path(path, request.POST["key"]) |
107 | + path = upload_target_path( |
108 | + path, request.POST["key"], public=api_key[0].public) |
109 | |
110 | # Create directory if required |
111 | dirname = os.path.dirname(path) |
112 | @@ -69,7 +75,15 @@ |
113 | while APIKeyStore.objects.filter(key=key): |
114 | key = "%030x" % random.randrange(256**15) |
115 | |
116 | - api_key = APIKeyStore(key=key) |
117 | + # Look for a hint of sanity in the value given to public, but don't |
118 | + # care about it too much. |
119 | + yes = ["", "y", "yes", "true", "1"] |
120 | + if "public" in request.GET: |
121 | + public = request.GET["public"].lower() in yes |
122 | + else: |
123 | + public = False |
124 | + |
125 | + api_key = APIKeyStore(key=key, public=public) |
126 | api_key.save() |
127 | return HttpResponse(key) |
128 | |
129 | |
130 | === modified file 'license_protected_downloads/views.py' |
131 | --- license_protected_downloads/views.py 2013-06-19 09:51:06 +0000 |
132 | +++ license_protected_downloads/views.py 2013-07-17 15:28:26 +0000 |
133 | @@ -24,7 +24,7 @@ |
134 | import bzr_version |
135 | from buildinfo import BuildInfo, IncorrectDataFormatException |
136 | from render_text_files import RenderTextFiles |
137 | -from models import License |
138 | +from models import License, APIKeyStore |
139 | # Load group auth "plugin" dynamically |
140 | import importlib |
141 | group_auth_modules = [importlib.import_module(m) for m in settings.GROUP_AUTH_MODULES] |
142 | @@ -136,13 +136,25 @@ |
143 | return listing |
144 | |
145 | |
146 | -def test_path(path, served_paths=None): |
147 | +def test_path(path, request, served_paths=None): |
148 | """Check that path points to something we can serve up. |
149 | |
150 | served_paths can be provided to overwrite settings.SERVED_PATHS. This is |
151 | used for uploaded files, which may not be shared in the server root. |
152 | """ |
153 | |
154 | + # if key is in request.GET["key"] then need to mod path and give |
155 | + # access to a per-key directory. |
156 | + if "key" in request.GET: |
157 | + key_details = APIKeyStore.objects.filter(key=request.GET["key"]) |
158 | + if key_details: |
159 | + path = os.path.join(request.GET["key"], path) |
160 | + |
161 | + # Private uploads are in a separate path (or can be), so set |
162 | + # served_paths as needed. |
163 | + if key_details[0].public == False: |
164 | + served_paths = settings.UPLOAD_PATH |
165 | + |
166 | if served_paths is None: |
167 | served_paths = settings.SERVED_PATHS |
168 | else: |
169 | @@ -458,14 +470,7 @@ |
170 | def file_server_get(request, path): |
171 | |
172 | url = path |
173 | - |
174 | - # if key is in request.GET["key"] then need to mod path and give |
175 | - # access to a per-key directory. |
176 | - if "key" in request.GET: |
177 | - path = os.path.join(request.GET["key"], path) |
178 | - result = test_path(path, settings.UPLOAD_PATH) |
179 | - else: |
180 | - result = test_path(path) |
181 | + result = test_path(path, request) |
182 | |
183 | if not result: |
184 | raise Http404 |
185 | @@ -572,7 +577,7 @@ |
186 | |
187 | def get_textile_files(request): |
188 | |
189 | - result = test_path(request.GET.get("path")) |
190 | + result = test_path(request.GET.get("path"), request) |
191 | if not result: |
192 | raise Http404 |
193 | |
194 | @@ -600,7 +605,7 @@ |
195 | def list_files_api(request, path): |
196 | path = iri_to_uri(path) |
197 | url = path |
198 | - result = test_path(path) |
199 | + result = test_path(path, request) |
200 | if not result: |
201 | raise Http404 |
202 | |
203 | @@ -643,7 +648,7 @@ |
204 | |
205 | def get_license_api(request, path): |
206 | path = iri_to_uri(path) |
207 | - result = test_path(path) |
208 | + result = test_path(path, request) |
209 | if not result: |
210 | raise Http404 |
211 |
Hi James,
overall it looks good, a couple of suggestion follows.
On Wed, Jul 17, 2013 at 5:29 PM, James Tunnicliffe public_ key_post_ and_get_ file(self) : testserver/ api/request_ key", MASTER_ API_KEY, l(response. status_ code, 200) xpMatches( response. content, "\S+") key_post_ and_get_ file" temporary_ file(file_ content) ) temporary_ file(buildinfo_ content) ) testserver/ pub/file_ name", l(response. status_ code, 200) build_info) as f: testserver/ pub/BUILD- INFO.txt", l(response. status_ code, 200) join(settings. SERVED_ PATHS[0] , file_path) as f: l(f.read( ), file_content) testserver/ pub/file_ name") l(response. status_ code, 200) rmtree( os.path. join(settings. SERVED_ PATHS[0] , "pub"))
<email address hidden> wrote:
>
> + def test_get_
> + response = self.client.get("http://
> + data={"key": settings.
> + "public": ""})
> +
> + self.assertEqua
> + # Don't care what the key is, as long as it isn't blank
> + self.assertRege
> + key = response.content
> +
> + # Now write a file so we can upload it
> + file_content = "test_get_
> + file_root = "/tmp"
> +
> + tmp_file_name = os.path.join(
> + file_root,
> + self.make_
> +
> + 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_
> +
> + # Send the files
> + with open(tmp_file_name) as f:
> + response = self.client.post("http://
> + data={"key": key, "file": f})
> + self.assertEqua
> +
> + with open(tmp_
> + response = self.client.post("http://
> + data={"key": key, "file": f})
> + self.assertEqua
> +
> + # Check the upload worked by reading the file back from its uploaded
> + # location
> + uploaded_file_path = os.path.
> + "file_name")
> +
> + with open(uploaded_
> + self.assertEqua
> +
> + # Test we can fetch the newly uploaded file
> + response = self.client.get("http://
> + self.assertEqua
> +
> + # Delete the files generated by the test
> + shutil.
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' objects. filter( key=request. POST["key" ])): verError( "Invalid key") objects. filter( key=request. POST["key" ])
>
>
> @@ -41,11 +44,14 @@
> APIKeyStore.
> return HttpResponseSer
>
> + api_key = APIKeyStore.
> +
Does the call to APIKeyStore. objects. filter get cached in some way? objects. filter even if "key in request.POST" is
The problem is that the other solution I have in mind means to always
call APIKeyStore.
False.
--
Milo Casagrande | Automation Engineer
Linaro.org <www.li...