Merge lp:~pfalcon/linaro-license-protection/openid-exception-for-internal-hosts into lp:~linaro-automation/linaro-license-protection/trunk

[Paul Sokolovsky]

This does some simplification refactoring on logic in file serving method and then introduces short-circuit action early in the processing for internal trusted hosts, to make sure this short-circuit applies for any restriction checks.

Location of this short-circuit check is something which really worth a review here. file_server() method does quote a lot of checks actually, and again, it should be early enough to not hit the case we fix here (when internal hosts are locked out from access to file). So, I just started with putting it pretty early (after file path validation) and then tried to reason if it should be moved later. There's for example check for presence of BUILD-INFO.txt and bail out if it's not there. Well, I don't see any benefits of putting INTERNAL_HOSTS check after it - then only outcome we can expect from this is that if someone put botched BUILD-INFO.txt in a build, then that build won't show LAVA results in android-build, hardly a useful feature. Later in the code is handling of dir listing generation - again, this should not be needed for the kind of API access we need from INTERNAL_HOSTS.

So, summing up, IMHO location of INTERNAL_HOSTS is pretty good, but I'm open to suggestions from reviewers.

[lp:~pfalcon/linaro-license-protection/openid-exception-for-internal-hosts]

167. By Paul Sokolovsky on 2013-02-21

Add test data for test_exception_internal_host_for_lic_and_openid().

168. By Paul Sokolovsky on 2013-02-21

Handle API access check for INTERNAL_HOSTS early in file_server().

[Данило Шеган (danilo)]

I am generally fine with your changes. I dislike shortening names like "lic" instead of "license". I would also prefer if you created the data for the tests inside tests themselves, but considering ones you are modifying don't have that, I won't make a fuss about it.

With minor comments above which are not binding, approved.

review: Approve

[Paul Sokolovsky (pfalcon)]

I also prefer full names, but test_exception_internal_host_for_licence_and_openid is really too long a function name ;-)

=== modified file 'license_protected_downloads/tests/test_views.py'
--- license_protected_downloads/tests/test_views.py	2013-01-30 10:46:11 +0000
+++ license_protected_downloads/tests/test_views.py	2013-02-21 12:12:22 +0000
@@ -359,7 +359,7 @@
              r'lp:linaro-license-protection</a> r' +
              str(bzr_version.get_my_bzr_revno())))
 
-    def test_exception_ip_remote_addr(self):
+    def test_exception_internal_host_for_lic(self):
         internal_host = INTERNAL_HOSTS[0]
         target_file = 'build-info/origen-blob.txt'
         url = urlparse.urljoin("http://testserver/", target_file)
@@ -371,6 +371,30 @@
         file_path = os.path.join(TESTSERVER_ROOT, target_file)
         self.assertEqual(response['X-Sendfile'], file_path)
 
+    def test_exception_internal_host_for_openid(self):
+        internal_host = INTERNAL_HOSTS[0]
+        target_file = 'build-info/openid.txt'
+        url = urlparse.urljoin("http://testserver/", target_file)
+        response = self.client.get(url, follow=True,
+            REMOTE_ADDR=internal_host)
+
+        # If we have access to the file, we will get an X-Sendfile response
+        self.assertEqual(response.status_code, 200)
+        file_path = os.path.join(TESTSERVER_ROOT, target_file)
+        self.assertEqual(response['X-Sendfile'], file_path)
+
+    def test_exception_internal_host_for_lic_and_openid(self):
+        internal_host = INTERNAL_HOSTS[0]
+        target_file = 'build-info/origen-blob-openid.txt'
+        url = urlparse.urljoin("http://testserver/", target_file)
+        response = self.client.get(url, follow=True,
+            REMOTE_ADDR=internal_host)
+
+        # If we have access to the file, we will get an X-Sendfile response
+        self.assertEqual(response.status_code, 200)
+        file_path = os.path.join(TESTSERVER_ROOT, target_file)
+        self.assertEqual(response['X-Sendfile'], file_path)
+
     def test_no_exception_ip(self):
         internal_host = '10.1.2.3'
         target_file = 'build-info/origen-blob.txt'
 
=== added file 'license_protected_downloads/tests/testserver_root/build-info/origen-blob-openid.txt'
--- license_protected_downloads/tests/testserver_root/build-info/origen-blob-openid.txt	1970-01-01 00:00:00 +0000
+++ license_protected_downloads/tests/testserver_root/build-info/origen-blob-openid.txt	2013-02-21 12:12:22 +0000
@@ -0,0 +1,1 @@
+This is protected with click-through Samsung license and OpenID group.
 \ No newline at end of file
 
=== modified file 'license_protected_downloads/views.py'
--- license_protected_downloads/views.py	2012-11-08 09:42:36 +0000
+++ license_protected_downloads/views.py	2013-02-21 12:12:22 +0000
@@ -362,6 +362,23 @@
     return found
 
 
+def send_file(path):
+    "Return HttpResponse which will send path to user's browser."
+    file_name = os.path.basename(path)
+    mimetypes.init()
+    mime = mimetypes.guess_type(path)[0]
+    if mime is None:
+        mime = "application/force-download"
+    response = HttpResponse(mimetype=mime)
+    response['Content-Disposition'] = ('attachment; filename=%s' %
+                                       smart_str(file_name))
+    response['X-Sendfile'] = smart_str(path)
+    #response['Content-Length'] = os.path.getsize(path)
+    # TODO: Is it possible to add a redirect to response so we can take
+    # the user back to the original directory this file is in?
+    return response
+
+
 def file_server(request, path):
     """Serve up a file / directory listing or license page as required"""
     url = path
@@ -372,6 +389,10 @@
     type = result[0]
     path = result[1]
 
+    if get_client_ip(request) in config.INTERNAL_HOSTS:
+        # For internal trusted hosts, short-circuit any further checks
+        return send_file(path)
+
     if BuildInfo.build_info_exists(path):
         try:
             build_info = BuildInfo(path)
@@ -415,47 +436,37 @@
                                    RenderTextFiles.find_and_render(path)
                                    })
 
-    file_name = os.path.basename(path)
-
     # If the file listing doesn't contain the file requested for download,
     # return a 404. This prevents the download of BUILD-INFO.txt and other
     # hidden files.
     if not file_listed(path, url):
         raise Http404
 
-    response = None
     if get_client_ip(request) in config.INTERNAL_HOSTS or\
        is_whitelisted(os.path.join('/', url)):
         digests = 'OPEN'
     else:
         digests = is_protected(path)
+
+    response = None
     if not digests:
         # File has no license text but is protected
         response = HttpResponseForbidden(
             "You do not have permission to access this file.")
-
-    # Return a file...
+    elif digests == "OPEN":
+        response = None
     else:
-        if digests == "OPEN":
-            response = None
-        else:
-            for digest in digests:
-                if not license_accepted(request, digest):
-                    response = redirect(
-                        '/license?lic=' + digest + "&url=" + url)
-
-        if not response:
-            mimetypes.init()
-            mime = mimetypes.guess_type(path)[0]
-            if mime is None:
-                mime = "application/force-download"
-            response = HttpResponse(mimetype=mime)
-            response['Content-Disposition'] = ('attachment; filename=%s' %
-                                               smart_str(file_name))
-            response['X-Sendfile'] = smart_str(path)
-            #response['Content-Length'] = os.path.getsize(path)
-            # TODO: Is it possible to add a redirect to response so we can take
-            # the user back to the original directory this file is in?
+        for digest in digests:
+            if not license_accepted(request, digest):
+                # Make sure that user accepted each license one by one
+                response = redirect(
+                    '/license?lic=' + digest + "&url=" + url)
+                break
+
+    # If we didn't have any other response, it's ok to send file now
+    if not response:
+        response = send_file(path)
+
     return response