fix(web): issue2551356. Add etag header for not-modified (304) request.

When a 304 is returned to a conditional request for a static file,
print an ETag for the response.

ETag was always sent with a 200 response.

This also adds initial support for if-none-match conditional requests
for static files.

Changes:

Refactors the if-modified-since code out to a method.

It moves a file stat call from serve_static_file to _serve_file
so that an etag can be generated by both serve_static_file and
serve_file which call _serve_file.

Tests added. This does not test the codepath where serve_file pulls
content from the database rather than from a local file on disk.

Test mocking _serve_file changed to account for 5th argument to serve_file

BREAKING CHANGE:

function signature for client.py-Client::_serve_file() now has 5 not 4
parameters (added etag param). Since this is a "hidden" method I am
not too worried about it.

Author: rouilj

CHANGES.txt

@@ -36,6 +36,9 @@ Fixed:
 - issue2551289 - Invalid REST Accept header with post/put performs
   change before returning 406. Error before making any changes to the
   db if we can't respond with requested format. (John Rouillard)
+- issue2551356 - Add etag header when If-Modified-Since GET request
+  returns not-modified (304). Breaking change to function signature
+  for client.py-Client::_serve_file(). (John Rouillard)
 
 Features:
 

roundup/cgi/client.py

@@ -1949,7 +1949,7 @@ def serve_file(self, designator, dre=dre):
 
         lmt = klass.get(nodeid, 'activity').timestamp()
 
-        self._serve_file(lmt, mime_type, content, filename)
+        self._serve_file(lmt, None, mime_type, content, filename)
 
     def serve_static_file(self, file):
         """ Serve up the file named from the templates dir
@@ -1989,9 +1989,6 @@ def serve_static_file(self, file):
         if filename is None:  # we didn't find a filename
             raise NotFound(file)
 
-        # last-modified time
-        lmt = os.stat(filename)[stat.ST_MTIME]
-
         # detemine meta-type
         file = str(file)
         mime_type = mimetypes.guess_type(file)[0]
@@ -2009,28 +2006,59 @@ def serve_static_file(self, file):
             self.additional_headers['Cache-Control'] = \
                             self.Cache_Control[mime_type]
 
-        self._serve_file(lmt, mime_type, '', filename)
+        self._serve_file(None, None, mime_type, '', filename)
 
-    def _serve_file(self, lmt, mime_type, content=None, filename=None):
-        """ guts of serve_file() and serve_static_file()
-        """
+    def _serve_file(self, lmt, etag, mime_type, content=None, filename=None):
+        """guts of serve_file() and serve_static_file()
 
-        # spit out headers
-        self.additional_headers['Last-Modified'] = email.utils.formatdate(lmt,
-	 usegmt=True)
+            if lmt or etag are None, derive them from file filename.
 
-        ims = None
-        # see if there's an if-modified-since...
-        #    used if this is run behind a non-caching http proxy
-        if hasattr(self.request, 'headers'):
-            ims = self.request.headers.get('if-modified-since')
-        elif 'HTTP_IF_MODIFIED_SINCE' in self.env:
-            # cgi will put the header in the env var
-            ims = self.env['HTTP_IF_MODIFIED_SINCE']
-        if ims:
-            ims = email.utils.parsedate(ims)[:6]
-            lmtt = time.gmtime(lmt)[:6]
-            if lmtt <= ims:
+            Handles if-modified-since and if-none-match etag
+            conditional gets.
+
+            It produces an raw etag header without encoding suffix.
+            But it adds Accept-Encoding to the vary header.
+
+        """
+        if filename:
+            stat_info = os.stat(filename)
+
+            if lmt is None:
+                # last-modified time
+                lmt = stat_info[stat.ST_MTIME]
+            if etag is None:
+                # FIXME: maybe etag should depend on encoding.
+                # it is an apache compatible etag without encoding.
+                etag = '"%x-%x-%x"' % (stat_info[stat.ST_INO],
+                                       stat_info[stat.ST_SIZE],
+                                       stat_info[stat.ST_MTIME])
+
+            # spit out headers for conditional request
+            self.setHeader("ETag", etag)
+            self.additional_headers['Last-Modified'] = \
+                email.utils.formatdate(lmt, usegmt=True)
+
+            inm = None
+            # ETag is a more strict check than modified date. Use etag
+            # check if available. Skip testing modified data.
+            if hasattr(self.request, 'headers'):
+                inm = self.request.headers.get('if-none-match')
+            elif 'HTTP_IF_NONE_MATCH' in self.env:
+                # maybe the cgi will put the header in the env var
+                inm = self.env['HTTP_ETAG']
+            if inm and etag == inm:
+                # because we can compress, always set Accept-Encoding
+                # value. Otherwise caches can serve up the wrong info
+                # if their cached copy has no compression.
+                self.setVary("Accept-Encoding")
+                '''
+                to solve issue2551356 I may need to determine
+                the content encoding.
+                if (self.determine_content_encoding()):
+                '''
+                raise NotModified
+
+            if self.if_not_modified_since(lmt):
                 # because we can compress, always set Accept-Encoding
                 # value. Otherwise caches can serve up the wrong info
                 # if their cached copy has no compression.
@@ -2051,6 +2079,27 @@ def _serve_file(self, lmt, mime_type, content=None, filename=None):
             self.additional_headers['Content-Length'] = str(len(content))
             self.write(content)
 
+    def if_not_modified_since(self, lmt):
+        ims = None
+        # see if there's an if-modified-since...
+        if hasattr(self.request, 'headers'):
+            ims = self.request.headers.get('if-modified-since')
+        elif 'HTTP_IF_MODIFIED_SINCE' in self.env:
+            # cgi will put the header in the env var
+            ims = self.env['HTTP_IF_MODIFIED_SINCE']
+
+        if ims:
+            datestamp = email.utils.parsedate(ims)
+            if datestamp is not None:
+                ims = datestamp[:6]
+            else:
+                # set to beginning of time so whole file will be sent
+                ims = (0, 0, 0, 0, 0, 0)
+            lmtt = time.gmtime(lmt)[:6]
+            return lmtt <= ims
+
+        return False
+
     def send_error_to_admin(self, subject, html, txt):
         """Send traceback information to admin via email.
            We send both, the formatted html (with more information) and

test/test_cgi.py

@@ -18,7 +18,7 @@
 
 from roundup.anypy.cgi_ import cgi
 from roundup.cgi import client, actions, exceptions
-from roundup.cgi.exceptions import FormError, NotFound, Redirect
+from roundup.cgi.exceptions import FormError, NotFound, Redirect, NotModified
 from roundup.exceptions import UsageError, Reject
 from roundup.cgi.templating import HTMLItem, HTMLRequest, NoTemplate
 from roundup.cgi.templating import HTMLProperty, _HTMLItem, anti_csrf_nonce
@@ -1942,6 +1942,7 @@ def _make_client(self, form, classname='user', nodeid='1',
         if nodeid is not None:
             cl.nodeid = nodeid
         cl.db = self.db
+        cl.request = MockNull()
         cl.db.Otk =  cl.db.getOTKManager()
         #cl.db.Otk = MockNull()
         #cl.db.Otk.data = {}
@@ -2382,6 +2383,83 @@ def testRegisterActionUnusedUserCheck(self):
         if os.path.exists(SENDMAILDEBUG):
             os.remove(SENDMAILDEBUG)
 
+    def testserve_static_files_cache_headers(self):
+        """Note for headers the real headers class is case
+           insensitive.
+        """
+        # make a client instance
+        cl = self._make_client({})
+        # Make local copy in cl to not modify value in class
+        cl.Cache_Control = copy.copy (cl.Cache_Control)
+
+        # TEMPLATES dir is searched by default. So this file exists.
+        # Check the returned values.
+        cl.serve_static_file("style.css")
+
+        # gather the conditional request headers from the 200 response
+        inm =  cl.additional_headers['ETag']
+        ims =  cl.additional_headers['Last-Modified']
+
+
+        # loop over all header value possibilities that will
+        # result in not modified.
+        for headers in [
+                {'if-none-match' : inm},
+                {'if-modified-since' : ims},
+                {'if-none-match' : inm, 'if-modified-since' : ims },
+                {'if-none-match' : inm, 'if-modified-since' : "fake" },
+                {'if-none-match' : "fake", 'if-modified-since' : ims },
+        ]:
+            print(headers)
+
+            # Request same file with if-modified-since header
+            # expect NotModified with same ETag and Last-Modified headers.
+            cl.request.headers = headers
+            cl.response_code = None
+            cl.additional_headers = {}
+
+            with self.assertRaises(NotModified) as cm:
+                cl.serve_static_file("style.css")
+
+            self.assertEqual(cm.exception.args, ())
+
+            self.assertEqual(cl.response_code, None)
+            self.assertEqual(cl.additional_headers['ETag'], inm)
+            self.assertEqual(cl.additional_headers['Last-Modified'], ims)
+
+
+        ## run two cases that should not return NotModified
+        for headers in [
+                {},
+                {'if-none-match' : "fake", 'if-modified-since' : "fake" },
+        ]:
+            cl.request.headers = headers
+            cl.response_code = None
+            cl.additional_headers = {}
+
+            cl.serve_static_file("style.css")
+
+            self.assertEqual(cl.response_code, None)
+            self.assertEqual(cl.additional_headers['ETag'], inm)
+            self.assertEqual(cl.additional_headers['Last-Modified'], ims)
+
+        ## test pure cgi case
+        # headers attribute does not exist
+        cl.request = None
+        cl.response_code = None
+        cl.additional_headers = {}
+
+        cl.env["HTTP_IF_MODIFIED_SINCE"] = ims
+
+        with self.assertRaises(NotModified) as cm:
+            cl.serve_static_file("style.css")
+
+        self.assertEqual(cm.exception.args, ())
+
+        self.assertEqual(cl.response_code, None)
+        self.assertEqual(cl.additional_headers['ETag'], inm)
+        self.assertEqual(cl.additional_headers['Last-Modified'], ims)
+
     def testserve_static_files(self):
         # make a client instance
         cl = self._make_client({})
@@ -2390,8 +2468,8 @@ def testserve_static_files(self):
 
         # hijack _serve_file so I can see what is found
         output = []
-        def my_serve_file(a, b, c, d):
-            output.append((a,b,c,d))
+        def my_serve_file(a, b, c, d, e):
+            output.append((a,b,c,d,e))
         cl._serve_file = my_serve_file
 
         # check case where file is not found.
@@ -2401,8 +2479,9 @@ def my_serve_file(a, b, c, d):
         # TEMPLATES dir is searched by default. So this file exists.
         # Check the returned values.
         cl.serve_static_file("issue.index.html")
-        self.assertEqual(output[0][1], "text/html")
-        self.assertEqual(output[0][3],
+        print(output)
+        self.assertEqual(output[0][2], "text/html")
+        self.assertEqual(output[0][4],
                          normpath('_test_cgi_form/html/issue.index.html'))
         del output[0] # reset output buffer
 
@@ -2415,8 +2494,8 @@ def my_serve_file(a, b, c, d):
         # explicitly allow html directory
         cl.instance.config['STATIC_FILES'] = 'html -'
         cl.serve_static_file("issue.index.html")
-        self.assertEqual(output[0][1], "text/html")
-        self.assertEqual(output[0][3],
+        self.assertEqual(output[0][2], "text/html")
+        self.assertEqual(output[0][4],
                          normpath('_test_cgi_form/html/issue.index.html'))
         del output[0] # reset output buffer
 
@@ -2425,15 +2504,15 @@ def my_serve_file(a, b, c, d):
 
         # find file in first directory
         cl.serve_static_file("messagesummary.py")
-        self.assertEqual(output[0][1], "text/x-python")
-        self.assertEqual(output[0][3],
+        self.assertEqual(output[0][2], "text/x-python")
+        self.assertEqual(output[0][4],
                          normpath( "_test_cgi_form/detectors/messagesummary.py"))
         del output[0] # reset output buffer
 
         # find file in second directory
         cl.serve_static_file("README.txt")
-        self.assertEqual(output[0][1], "text/plain")
-        self.assertEqual(output[0][3],
+        self.assertEqual(output[0][2], "text/plain")
+        self.assertEqual(output[0][4],
                          normpath("_test_cgi_form/extensions/README.txt"))
         del output[0] # reset output buffer
 
@@ -2448,25 +2527,25 @@ def my_serve_file(a, b, c, d):
         f = open('_test_cgi_form/detectors/README.txt', 'a').close()
         # find file now in first directory
         cl.serve_static_file("README.txt")
-        self.assertEqual(output[0][1], "text/plain")
-        self.assertEqual(output[0][3],
+        self.assertEqual(output[0][2], "text/plain")
+        self.assertEqual(output[0][4],
                          normpath("_test_cgi_form/detectors/README.txt"))
         del output[0] # reset output buffer
 
         cl.instance.config['STATIC_FILES'] = ' detectors extensions '
         # make sure lack of trailing - allows searching TEMPLATES
         cl.serve_static_file("issue.index.html")
-        self.assertEqual(output[0][1], "text/html")
-        self.assertEqual(output[0][3],
+        self.assertEqual(output[0][2], "text/html")
+        self.assertEqual(output[0][4],
                          normpath("_test_cgi_form/html/issue.index.html"))
         del output[0] # reset output buffer
 
         # Make STATIC_FILES a single element.
         cl.instance.config['STATIC_FILES'] = 'detectors'
         # find file now in first directory
         cl.serve_static_file("messagesummary.py")
-        self.assertEqual(output[0][1], "text/x-python")
-        self.assertEqual(output[0][3],
+        self.assertEqual(output[0][2], "text/x-python")
+        self.assertEqual(output[0][4],
                          normpath("_test_cgi_form/detectors/messagesummary.py"))
         del output[0] # reset output buffer
 
@@ -2475,8 +2554,8 @@ def my_serve_file(a, b, c, d):
         f = open('_test_cgi_form/detectors/css/README.css', 'a').close()
         # use subdir in filename
         cl.serve_static_file("css/README.css")
-        self.assertEqual(output[0][1], "text/css")
-        self.assertEqual(output[0][3],
+        self.assertEqual(output[0][2], "text/css")
+        self.assertEqual(output[0][4],
                          normpath("_test_cgi_form/detectors/css/README.css"))
         del output[0] # reset output buffer
 
@@ -2486,18 +2565,19 @@ def my_serve_file(a, b, c, d):
         os.mkdir('_test_cgi_form/html/css')
         f = open('_test_cgi_form/html/css/README1.css', 'a').close()
         cl.serve_static_file("README1.css")
-        self.assertEqual(output[0][1], "text/css")
-        self.assertEqual(output[0][3],
+        self.assertEqual(output[0][2], "text/css")
+        self.assertEqual(output[0][4],
                          normpath("_test_cgi_form/html/css/README1.css"))
         self.assertTrue( "Cache-Control" in cl.additional_headers )
         self.assertEqual( cl.additional_headers,
                           {'Cache-Control': 'public, max-age=3600'} )
+        print(cl.additional_headers)
         del output[0] # reset output buffer
 
         cl.Cache_Control['README1.css'] = 'public, max-age=60'
         cl.serve_static_file("README1.css")
-        self.assertEqual(output[0][1], "text/css")
-        self.assertEqual(output[0][3],
+        self.assertEqual(output[0][2], "text/css")
+        self.assertEqual(output[0][4],
                          normpath("_test_cgi_form/html/css/README1.css"))
         self.assertTrue( "Cache-Control" in cl.additional_headers )
         self.assertEqual( cl.additional_headers,