Merge lp:~gesha/linaro-license-protection/include-readme into lp:~linaro-automation/linaro-license-protection/trunk

[Georgy Redkozubov]

This branch adds simple handling for <linaro:include file="README" /> tags to include content from files in HEADER.html.
Only files in the same dir as HEADER.html are processed. If file is found then corresponding tag is replaced with content of file, in all other cases tag is replaced with empty string.

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

Hi Gesha,

Thanks for doing this quickly. I am not a big fan of on-disk test data,
but considering that's the pattern in lp:linaro-license-protection, it's
ok for this branch (we should refactor it some time in the future).

Thanks for writing the tests as well. Comments inline.

  review needs-fixing

У сре, 19. 09 2012. у 08:34 +0000, Georgy Redkozubov пише:

> разлике међу датотекама прилог (review-diff.txt)
> === modified file 'license_protected_downloads/tests/test_views.py'
> --- license_protected_downloads/tests/test_views.py 2012-08-30 09:44:29 +0000
> +++ license_protected_downloads/tests/test_views.py 2012-09-19 08:33:25 +0000
> @@ -486,6 +489,78 @@
> file_path = os.path.join(TESTSERVER_ROOT, target_file)
> self.assertEqual(response['X-Sendfile'], file_path)
>
> + def make_temporary_file(self, data, root=None):
> + """Creates a temporary file and fills it with data.
> +
> + Returns the file name of the new temporary file.
> + """
> + tmp_file_handle, tmp_filename = tempfile.mkstemp(dir=root)
> + tmp_file = os.fdopen(tmp_file_handle, "w")
> + tmp_file.write(data)
> + tmp_file.close()
> + return os.path.basename(tmp_filename)
> +
> + def test_repl(self):

It would be good to split this up into several functions, each one
testing a single corner case/condition (like you do now, but just not in
a single method).

> + target_file = "readme"
> + old_cwd = os.getcwd()
> + file_path = os.path.join(TESTSERVER_ROOT, target_file)
> + os.chdir(file_path)
> + ret = re.sub(r'<linaro:include file="(?P<file_name>.*)"[ ]*/>',
> + repl, 'Test <linaro:include file="README" /> html')

If you just make the regular expression a compiled one and put it
directly in views.py as eg. LINARO_INCLUDE_FILE_RE, you can import that,
and then you'd at least be testing the right RE all the time (i.e. in
this way, we can change the regular expression in the code, code would
stop working, but tests would still pass).

> + self.assertEqual(ret, r"Test Included from README html")

...

> + def test_process_include_tags(self):
> + target_file = "readme"
> + url = urlparse.urljoin("http://testserver/", target_file)
> + response = self.client.get(url, follow=True)
> +
> + self.assertContains(response, r"Included from README")

This one is perfect for an integration test, just enough.

> === modified file 'license_protected_downloads/views.py'
...
> +def repl(m):
> + """
> + Return content of file in current directory otherwise empty string.
> + """

Name and docstring are not very informative. How about

 def process_include_and_read_file()

(That immediately tells you how method actually does two separate
things, and should probably be split into two)

> + content = ''
> + fname = m.group('file_name')
> + dirname = os.path.dirname(fname)
> + if (not dirname or dirname=='.') and os.path.isfile(fname) and not os.path.islink(fname):
> + with open(fname, "r") as infile:
> + content = infile.read()
> +
> + return content

Is it not simpler to check:

  full...

review: Needs Fixing

[lp:~gesha/linaro-license-protection/include-readme]

129. By Georgy Redkozubov on 2012-09-19

Split test into separate functions. Updates based on review comments.

130. By Georgy Redkozubov on 2012-09-19

Refactored file check

131. By Georgy Redkozubov on 2012-09-19

Split out parent dir finding. Added tests.

[Georgy Redkozubov (gesha)]

Danilo,
>
> It would be good to split this up into several functions, each one
> testing a single corner case/condition (like you do now, but just not in
> a single method).
>

done

>
> If you just make the regular expression a compiled one and put it
> directly in views.py as eg. LINARO_INCLUDE_FILE_RE, you can import that,
> and then you'd at least be testing the right RE all the time (i.e. in
> this way, we can change the regular expression in the code, code would
> stop working, but tests would still pass).
>

I thought about it :)

> > +def repl(m):
> > + """
> > + Return content of file in current directory otherwise empty string.
> > + """
>
> Name and docstring are not very informative. How about
>
> def process_include_and_read_file()
>
> (That immediately tells you how method actually does two separate
> things, and should probably be split into two)

This function is only for re.sub() it returns a replacement string and takes only one parameter - match object.
Anyway I've updated name and docstring.

>
> > + content = ''
> > + fname = m.group('file_name')
> > + dirname = os.path.dirname(fname)
> > + if (not dirname or dirname=='.') and os.path.isfile(fname) and not
> os.path.islink(fname):
> > + with open(fname, "r") as infile:
> > + content = infile.read()
> > +
> > + return content
>
> Is it not simpler to check:
>
> full_filename = os.path.join(current_dir, fname)
> normalized_path = os.path.normpath(os.path.realname(full_filename))
> if current_dir == os.path.dirname(normalized_path):
> ....
>
> I'd also split this into a separate method "is_same_parent_dir(parent,
> filename)".
>

Implemented

>
> I generally hate this: we should not depend on cwd in our code, but
> instead pass appropriate parameters (path) around. I know it makes it
> harder for you since you can't simply pass the "repl" as the function to
> run, but it shouldn't be much harder :)
>

I agree, but in our case I don't see any other proper way (may be just now) :)

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

Thanks, tests look much neater now. Thanks for the code changes as well, r=me.

As for your comment about getcwd stuff, there was a simple way around it:

  class LinaroIncludeReplacer(object):
      base_path = None

      def read_file_with_include_data(self, matchobj):
          # Assume self.base_path is the allowed path, instead of getcwd().
          ...

  def _process_include_tags(contents, base_path):
      replacer = LinaroIncludeReplacer()
      replacer.base_path = base_path

      contents = LINARO_INCLUDE_FILE_RE.sub(replacer.read_file_with_include_data, contents)
      ...

Anyway, no need to go to these lengths now (there are shorter but less readable tricks involving lambdas as well :), it's looking good, so let's get it landed and email Fathi/Alex when it's up on the server.

PS. You don't have to check for os.path.islink() anymore: the normalization/realpath code resolves all the links.

review: Approve

=== modified file 'license_protected_downloads/tests/test_views.py'
--- license_protected_downloads/tests/test_views.py	2012-08-30 09:44:29 +0000
+++ license_protected_downloads/tests/test_views.py	2012-09-19 14:45:33 +0000
@@ -6,11 +6,14 @@
 import os
 import unittest
 import urlparse
+import tempfile
 
 from license_protected_downloads import bzr_version
 from license_protected_downloads.buildinfo import BuildInfo
 from license_protected_downloads.views import _insert_license_into_db
 from license_protected_downloads.views import _sizeof_fmt
+from license_protected_downloads.views import _process_include_tags
+from license_protected_downloads.views import is_same_parent_dir
 from license_protected_downloads.config import INTERNAL_HOSTS
 
 
@@ -486,6 +489,152 @@
         file_path = os.path.join(TESTSERVER_ROOT, target_file)
         self.assertEqual(response['X-Sendfile'], file_path)
 
+    def make_temporary_file(self, data, root=None):
+        """Creates a temporary file and fills it with data.
+
+        Returns the file name of the new temporary file.
+        """
+        tmp_file_handle, tmp_filename = tempfile.mkstemp(dir=root)
+        tmp_file = os.fdopen(tmp_file_handle, "w")
+        tmp_file.write(data)
+        tmp_file.close()
+        return os.path.basename(tmp_filename)
+
+    def test_replace_self_closing_tag(self):
+        target_file = "readme"
+        old_cwd = os.getcwd()
+        file_path = os.path.join(TESTSERVER_ROOT, target_file)
+        os.chdir(file_path)
+        ret = _process_include_tags(
+                    'Test <linaro:include file="README" /> html')
+        self.assertEqual(ret, r"Test Included from README html")
+        os.chdir(old_cwd)
+
+    def test_replace_self_closing_tag1(self):
+        target_file = "readme"
+        old_cwd = os.getcwd()
+        file_path = os.path.join(TESTSERVER_ROOT, target_file)
+        os.chdir(file_path)
+        ret = _process_include_tags(
+                    'Test <linaro:include file="README"/> html')
+        self.assertEqual(ret, r"Test Included from README html")
+        os.chdir(old_cwd)
+
+    def test_replace_with_closing_tag(self):
+        target_file = "readme"
+        old_cwd = os.getcwd()
+        file_path = os.path.join(TESTSERVER_ROOT, target_file)
+        os.chdir(file_path)
+        ret = _process_include_tags(
+                    'Test <linaro:include file="README">README is missing</linaro:include> html')
+        self.assertEqual(ret, r"Test Included from README html")
+        os.chdir(old_cwd)
+
+    def test_replace_non_existent_file(self):
+        target_file = "readme"
+        old_cwd = os.getcwd()
+        file_path = os.path.join(TESTSERVER_ROOT, target_file)
+        os.chdir(file_path)
+        ret = _process_include_tags(
+                    'Test <linaro:include file="NON_EXISTENT_FILE" /> html')
+        self.assertEqual(ret, r"Test  html")
+        os.chdir(old_cwd)
+
+    def test_replace_empty_file_property(self):
+        target_file = "readme"
+        old_cwd = os.getcwd()
+        file_path = os.path.join(TESTSERVER_ROOT, target_file)
+        os.chdir(file_path)
+        ret = _process_include_tags(
+                    'Test <linaro:include file="" /> html')
+        self.assertEqual(ret, r"Test  html")
+        os.chdir(old_cwd)
+
+    def test_replace_parent_dir(self):
+        target_file = "readme"
+        old_cwd = os.getcwd()
+        file_path = os.path.join(TESTSERVER_ROOT, target_file)
+        os.chdir(file_path)
+        ret = _process_include_tags(
+                    'Test <linaro:include file="../README" /> html')
+        self.assertEqual(ret, r"Test  html")
+        os.chdir(old_cwd)
+
+    def test_replace_subdir(self):
+        target_file = "readme"
+        old_cwd = os.getcwd()
+        file_path = os.path.join(TESTSERVER_ROOT, target_file)
+        os.chdir(file_path)
+        ret = _process_include_tags(
+                    'Test <linaro:include file="subdir/README" /> html')
+        self.assertEqual(ret, r"Test  html")
+        os.chdir(old_cwd)
+
+    def test_replace_subdir_parent_dir(self):
+        target_file = "readme"
+        old_cwd = os.getcwd()
+        file_path = os.path.join(TESTSERVER_ROOT, target_file)
+        os.chdir(file_path)
+        ret = _process_include_tags(
+                    'Test <linaro:include file="subdir/../README" /> html')
+        self.assertEqual(ret, r"Test Included from README html")
+        os.chdir(old_cwd)
+
+    def test_replace_full_path(self):
+        target_file = "readme"
+        old_cwd = os.getcwd()
+        file_path = os.path.join(TESTSERVER_ROOT, target_file)
+        os.chdir(file_path)
+        tmp = self.make_temporary_file("Included from /tmp", root="/tmp")
+        ret = _process_include_tags(
+                    'Test <linaro:include file="/tmp/%s" /> html' % tmp)
+        self.assertEqual(ret, r"Test  html")
+        os.chdir(old_cwd)
+
+    def test_replace_self_dir(self):
+        target_file = "readme"
+        old_cwd = os.getcwd()
+        file_path = os.path.join(TESTSERVER_ROOT, target_file)
+        os.chdir(file_path)
+        ret = _process_include_tags(
+                    'Test <linaro:include file="./README" /> html')
+        self.assertEqual(ret, r"Test Included from README html")
+        os.chdir(old_cwd)
+
+    def test_replace_self_parent_dir(self):
+        target_file = "readme"
+        old_cwd = os.getcwd()
+        file_path = os.path.join(TESTSERVER_ROOT, target_file)
+        os.chdir(file_path)
+        ret = _process_include_tags(
+                    'Test <linaro:include file="./../README" /> html')
+        self.assertEqual(ret, r"Test  html")
+        os.chdir(old_cwd)
+
+    def test_replace_symlink(self):
+        target_file = "readme"
+        old_cwd = os.getcwd()
+        file_path = os.path.join(TESTSERVER_ROOT, target_file)
+        os.chdir(file_path)
+        ret = _process_include_tags(
+                    'Test <linaro:include file="READMELINK" /> html')
+        self.assertEqual(ret, r"Test  html")
+        os.chdir(old_cwd)
+
+    def test_process_include_tags(self):
+        target_file = "readme"
+        url = urlparse.urljoin("http://testserver/", target_file)
+        response = self.client.get(url, follow=True)
+
+        self.assertContains(response, r"Included from README")
+
+    def test_is_same_parent_dir_true(self):
+        fname = os.path.join(TESTSERVER_ROOT, "subdir/../file")
+        self.assertTrue(is_same_parent_dir(TESTSERVER_ROOT, fname))
+
+    def test_is_same_parent_dir_false(self):
+        fname = os.path.join(TESTSERVER_ROOT, "../file")
+        self.assertFalse(is_same_parent_dir(TESTSERVER_ROOT, fname))
 
 if __name__ == '__main__':
     unittest.main()

=== added file 'license_protected_downloads/tests/testserver_root/README'
--- license_protected_downloads/tests/testserver_root/README	1970-01-01 00:00:00 +0000
+++ license_protected_downloads/tests/testserver_root/README	2012-09-19 14:45:33 +0000
@@ -0,0 +1,1 @@
+Included from ../README
\ No newline at end of file

=== added directory 'license_protected_downloads/tests/testserver_root/readme'
=== added file 'license_protected_downloads/tests/testserver_root/readme/HACKING'
--- license_protected_downloads/tests/testserver_root/readme/HACKING	1970-01-01 00:00:00 +0000
+++ license_protected_downloads/tests/testserver_root/readme/HACKING	2012-09-19 14:45:33 +0000
@@ -0,0 +1,1 @@
+Included from HACKING
\ No newline at end of file

=== added file 'license_protected_downloads/tests/testserver_root/readme/HEADER.html'
--- license_protected_downloads/tests/testserver_root/readme/HEADER.html	1970-01-01 00:00:00 +0000
+++ license_protected_downloads/tests/testserver_root/readme/HEADER.html	2012-09-19 14:45:33 +0000
@@ -0,0 +1,37 @@
+<html>
+<head>
+</head>
+<body>
+    <div id="content"><meta http-equiv="content-type" content="text/html; charset=utf-8" />
+    <h2>Sample includes</h2>
+    <br />
+    <h2><pre>&lt;linaro:include file="README" /&gt;</pre></h2>
+    <pre style="font-size:.85em; padding:0 0 0 10;">
+    <linaro:include file="README" />
+    </pre>
+    <h2><pre>&lt;linaro:include file="INSTALL"/&gt;</pre></h2>
+    <pre style="font-size:.85em; padding:0 0 0 10;">
+    <linaro:include file="INSTALL"/>
+    </pre>
+    <h2><pre>&lt;linaro:include file="HACKING"&gt;HACKING is missing&lt;/linaro:include&gt;</pre></h2>
+    <pre style="font-size:.85em; padding:0 0 0 10;">
+    <linaro:include file="HACKING">HACKING is missing</linaro:include>
+    </pre>
+    <h2><pre>&lt;linaro:include file="subdir/README"/&gt;</pre></h2>
+    <pre style="font-size:.85em; padding:0 0 0 10;">
+    <linaro:include file="subdir/README"/>
+    </pre>
+    <h2><pre>&lt;linaro:include file="subdir/../README"/&gt;</pre></h2>
+    <pre style="font-size:.85em; padding:0 0 0 10;">
+    <linaro:include file="../README"/>
+    </pre>
+    <h2><pre>&lt;linaro:include file="/tmp/README"/&gt;</pre></h2>
+    <pre style="font-size:.85em; padding:0 0 0 10;">
+    <linaro:include file="/tmp/README"/>
+    </pre>
+    <h2><pre>&lt;linaro:include file="READMELINK"/&gt;</pre></h2>
+    <pre style="font-size:.85em; padding:0 0 0 10;">
+    <linaro:include file="READMELINK"/>
+    </pre>
+</body>
+</html>
\ No newline at end of file

=== added file 'license_protected_downloads/tests/testserver_root/readme/INSTALL'
--- license_protected_downloads/tests/testserver_root/readme/INSTALL	1970-01-01 00:00:00 +0000
+++ license_protected_downloads/tests/testserver_root/readme/INSTALL	2012-09-19 14:45:33 +0000
@@ -0,0 +1,1 @@
+Included from INSTALL
\ No newline at end of file

=== added file 'license_protected_downloads/tests/testserver_root/readme/README'
--- license_protected_downloads/tests/testserver_root/readme/README	1970-01-01 00:00:00 +0000
+++ license_protected_downloads/tests/testserver_root/readme/README	2012-09-19 14:45:33 +0000
@@ -0,0 +1,1 @@
+Included from README
\ No newline at end of file

=== added symlink 'license_protected_downloads/tests/testserver_root/readme/READMELINK'
=== target is u'../README'
=== added directory 'license_protected_downloads/tests/testserver_root/readme/subdir'
=== added file 'license_protected_downloads/tests/testserver_root/readme/subdir/README'
--- license_protected_downloads/tests/testserver_root/readme/subdir/README	1970-01-01 00:00:00 +0000
+++ license_protected_downloads/tests/testserver_root/readme/subdir/README	2012-09-19 14:45:33 +0000
@@ -0,0 +1,1 @@
+Included from subdir/README
\ No newline at end of file

=== modified file 'license_protected_downloads/views.py'
--- license_protected_downloads/views.py	2012-08-30 09:44:29 +0000
+++ license_protected_downloads/views.py	2012-09-19 14:45:33 +0000
@@ -25,6 +25,10 @@
 import config
 
 
+LINARO_INCLUDE_FILE_RE = re.compile(r'<linaro:include file="(?P<file_name>.*)"[ ]*/>')
+LINARO_INCLUDE_FILE_RE1 = re.compile(r'<linaro:include file="(?P<file_name>.*)">(.*)</linaro:include>')
+
+
 def _hidden_file(file_name):
     hidden_files = ["BUILD-INFO.txt", "EULA.txt", r"^\.", "HEADER.html"]
     for pattern in hidden_files:
@@ -149,13 +153,60 @@
     if os.path.isfile(header_html):
         with open(header_html, "r") as infile:
             body = infile.read()
+        body = _process_include_tags(body)
         soup = BeautifulSoup(body)
         for chunk in soup.findAll(id="content"):
             header_content += chunk.prettify().decode("utf-8")
         header_content = '\n'.join(header_content.split('\n')[1:-1])
+
     return header_content
 
 
+def is_same_parent_dir(parent, filename):
+    """
+        Checks if filename's parent dir is parent.
+    """
+    full_filename = os.path.join(parent, filename)
+    normalized_path = os.path.normpath(os.path.realpath(full_filename))
+    if parent == os.path.dirname(normalized_path):
+        return True
+
+    return False
+
+
+def read_file_with_include_data(matchobj):
+    """
+        Function to get data for re.sub() in _process_include_tags() from file
+        which name is in named match group 'file_name'.
+        Returns content of file in current directory otherwise empty string.
+    """
+    content = ''
+    current_dir = os.getcwd()
+    fname = matchobj.group('file_name')
+    if is_same_parent_dir(current_dir, fname):
+        if os.path.isfile(fname) and not os.path.islink(fname):
+            with open(fname, "r") as infile:
+                content = infile.read()
+
+    return content
+
+
+def _process_include_tags(content):
+    """
+        Replaces <linaro:include file="README" /> or
+        <linaro:include file="README">text to show</linaro:include> tags
+        with content of README file or empty string if file not found or
+        not allowed.
+    """
+    content = re.sub(LINARO_INCLUDE_FILE_RE,
+                     read_file_with_include_data,
+                     content)
+    content = re.sub(LINARO_INCLUDE_FILE_RE1,
+                     read_file_with_include_data,
+                     content)
+    return content
+
+
 def is_protected(path):
     build_info = None
     max_index = 1
@@ -336,7 +387,10 @@
         else:
             up_dir = None
 
+        old_cwd = os.getcwd()
+        os.chdir(path)
         header_content = _get_header_html_content(path)
+        os.chdir(old_cwd)
         download = None
         if 'dl' in request.GET:
             download = request.GET['dl']

=== added file 'sampleroot/README'
--- sampleroot/README	1970-01-01 00:00:00 +0000
+++ sampleroot/README	2012-09-19 14:45:33 +0000
@@ -0,0 +1,1 @@
+Included from ../README
\ No newline at end of file

=== added directory 'sampleroot/readme'
=== added file 'sampleroot/readme/HACKING'
--- sampleroot/readme/HACKING	1970-01-01 00:00:00 +0000
+++ sampleroot/readme/HACKING	2012-09-19 14:45:33 +0000
@@ -0,0 +1,1 @@
+Included from HACKING
\ No newline at end of file

=== added file 'sampleroot/readme/HEADER.html'
--- sampleroot/readme/HEADER.html	1970-01-01 00:00:00 +0000
+++ sampleroot/readme/HEADER.html	2012-09-19 14:45:33 +0000
@@ -0,0 +1,37 @@
+<html>
+<head>
+</head>
+<body>
+    <div id="content"><meta http-equiv="content-type" content="text/html; charset=utf-8" />
+    <h2>Sample includes</h2>
+    <br />
+    <h2><pre>&lt;linaro:include file="README" /&gt;</pre></h2>
+    <pre style="font-size:.85em; padding:0 0 0 10;">
+    <linaro:include file="README" />
+    </pre>
+    <h2><pre>&lt;linaro:include file="INSTALL"/&gt;</pre></h2>
+    <pre style="font-size:.85em; padding:0 0 0 10;">
+    <linaro:include file="INSTALL"/>
+    </pre>
+    <h2><pre>&lt;linaro:include file="HACKING"&gt;HACKING is missing&lt;/linaro:include&gt;</pre></h2>
+    <pre style="font-size:.85em; padding:0 0 0 10;">
+    <linaro:include file="HACKING">HACKING is missing</linaro:include>
+    </pre>
+    <h2><pre>&lt;linaro:include file="subdir/README"/&gt;</pre></h2>
+    <pre style="font-size:.85em; padding:0 0 0 10;">
+    <linaro:include file="subdir/README"/>
+    </pre>
+    <h2><pre>&lt;linaro:include file="subdir/../README"/&gt;</pre></h2>
+    <pre style="font-size:.85em; padding:0 0 0 10;">
+    <linaro:include file="../README"/>
+    </pre>
+    <h2><pre>&lt;linaro:include file="/tmp/README"/&gt;</pre></h2>
+    <pre style="font-size:.85em; padding:0 0 0 10;">
+    <linaro:include file="/tmp/README"/>
+    </pre>
+    <h2><pre>&lt;linaro:include file="READMELINK"/&gt;</pre></h2>
+    <pre style="font-size:.85em; padding:0 0 0 10;">
+    <linaro:include file="READMELINK"/>
+    </pre>
+</body>
+</html>
\ No newline at end of file

=== added file 'sampleroot/readme/INSTALL'
--- sampleroot/readme/INSTALL	1970-01-01 00:00:00 +0000
+++ sampleroot/readme/INSTALL	2012-09-19 14:45:33 +0000
@@ -0,0 +1,1 @@
+Included from INSTALL
\ No newline at end of file

=== added file 'sampleroot/readme/README'
--- sampleroot/readme/README	1970-01-01 00:00:00 +0000
+++ sampleroot/readme/README	2012-09-19 14:45:33 +0000
@@ -0,0 +1,1 @@
+Included from README
\ No newline at end of file

=== added symlink 'sampleroot/readme/READMELINK'
=== target is u'../README'
=== added directory 'sampleroot/readme/subdir'
=== added file 'sampleroot/readme/subdir/README'
--- sampleroot/readme/subdir/README	1970-01-01 00:00:00 +0000
+++ sampleroot/readme/subdir/README	2012-09-19 14:45:33 +0000
@@ -0,0 +1,1 @@
+Included from subdir/README
\ No newline at end of file