diff --git a/CHANGES.txt b/CHANGES.txt index 1cdd1a24..c846609b 100644 --- a/CHANGES.txt +++ b/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: diff --git a/roundup/cgi/client.py b/roundup/cgi/client.py index a638de6a..c1b6a179 100644 --- a/roundup/cgi/client.py +++ b/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 diff --git a/test/test_cgi.py b/test/test_cgi.py index 430c53de..e99bc59a 100644 --- a/test/test_cgi.py +++ b/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,16 +2527,16 @@ 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 @@ -2465,8 +2544,8 @@ def my_serve_file(a, b, c, d): 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,