[feat] check if there are newest configuration files

here we port the if-modified-since conditional mechanism, so that we
only write the config if it is newer than whan we have.

we also add a line with the status code to the logs, so that it's easier
to debug.

note that the 'configs.json' file is never returning 304.

- Resolves: #8773

7 files changed, 124 insertions, 71 deletions

diff --git a/Makefile b/Makefile
index 3c33a094..7ff10d1e 100644
--- a/Makefile
+++ b/Makefile
@@ -45,6 +45,7 @@ test:
 test_e2e: install_helpers
 	tests/e2e/e2e-test-mail.sh
 	tests/e2e/e2e-test-vpn.sh
+	tests/e2e/conditional_downloads.py
 
 test_functional_setup:
 	pip install -U behave selenium
diff --git a/src/leap/bitmask/bonafide/__init__.py b/src/leap/bitmask/bonafide/__init__.py
index 9dfb19db..a007f8c5 100644
--- a/src/leap/bitmask/bonafide/__init__.py
+++ b/src/leap/bitmask/bonafide/__init__.py
@@ -3,7 +3,6 @@
 # exersise the codebase.
 
 import config
-import bootstrap
 import session
 import provider
 import service
diff --git a/src/leap/bitmask/bonafide/_http.py b/src/leap/bitmask/bonafide/_http.py
index 8f05b421..94d0bcb9 100644
--- a/src/leap/bitmask/bonafide/_http.py
+++ b/src/leap/bitmask/bonafide/_http.py
@@ -1,6 +1,6 @@
 # -*- coding: utf-8 -*-
 # _http.py
-# Copyright (C) 2015 LEAP
+# Copyright (C) 2015-2017 LEAP
 #
 # This program is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -18,10 +18,14 @@
 """
 twisted.web utils for bonafide.
 """
-import base64
+
 import cookielib
+import os
 import urllib
 
+from leap.common.files import get_mtime
+
+from twisted.logger import Logger
 from twisted.internet import defer, protocol, reactor
 from twisted.internet.ssl import Certificate
 from twisted.python.filepath import FilePath
@@ -32,6 +36,9 @@ from twisted.web.iweb import IBodyProducer
 from zope.interface import implements
 
 
+log = Logger()
+
+
 def cookieAgentFactory(verify_path, connectTimeout=30):
     customPolicy = BrowserLikePolicyForHTTPS(
         Certificate.loadPEM(FilePath(verify_path).getContent()))
@@ -40,19 +47,36 @@ def cookieAgentFactory(verify_path, connectTimeout=30):
     return CookieAgent(agent, cookiejar)
 
 
-def httpRequest(agent, url, values={}, headers={}, method='POST', token=None):
+class Unchanged(Exception):
+    pass
+
+
+# TODO this should be ported to use treq client.
+
+def httpRequest(agent, url, values={}, headers={}, method='POST', token=None,
+                saveto=None):
     data = ''
     if values:
         data = urllib.urlencode(values)
         headers['Content-Type'] = ['application/x-www-form-urlencoded']
+    if saveto is not None:
+        # TODO - I think we need a force parameter, because we might have a
+        # malformed file. Or maybe just remove the file if sanity check does
+        # not pass.
+        mtime = get_mtime(saveto)
+        if mtime is not None:
+            headers['if-modified-since'] = [mtime]
 
     if token:
         headers['Authorization'] = ['Token token="%s"' % (bytes(token))]
 
     def handle_response(response):
-        # print "RESPONSE CODE", response.code
+        log.debug("RESPONSE %s %s %s" % (method, response.code, url))
         if response.code == 204:
             d = defer.succeed('')
+        if response.code == 304:
+            log.debug('304 (Not modified): %s' % url)
+            raise Unchanged()
         else:
             class SimpleReceiver(protocol.Protocol):
                 def __init__(s, d):
@@ -70,12 +94,28 @@ def httpRequest(agent, url, values={}, headers={}, method='POST', token=None):
             response.deliverBody(SimpleReceiver(d))
         return d
 
+    def passthru(failure):
+        failure.trap(Unchanged)
+
     d = agent.request(method, url, Headers(headers),
                       StringProducer(data) if data else None)
     d.addCallback(handle_response)
+    if saveto:
+        d.addCallback(lambda body: _write_to_file(body, saveto))
+        d.addErrback(passthru)
     return d
 
 
+def _write_to_file(content, path):
+    folder = os.path.split(path)[0]
+    if not os.path.isdir(folder):
+        os.makedirs(folder)
+    with open(path, 'w') as f:
+        f.write(content)
+    # touch it to update its utime
+    os.utime(path, None)
+
+
 class StringProducer(object):
 
     implements(IBodyProducer)
diff --git a/src/leap/bitmask/bonafide/bootstrap.py b/src/leap/bitmask/bonafide/bootstrap.py
deleted file mode 100644
index e69de29b..00000000
--- a/src/leap/bitmask/bonafide/bootstrap.py
+++ /dev/null
diff --git a/src/leap/bitmask/bonafide/config.py b/src/leap/bitmask/bonafide/config.py
index 6857a3f8..d0468a49 100644
--- a/src/leap/bitmask/bonafide/config.py
+++ b/src/leap/bitmask/bonafide/config.py
@@ -34,13 +34,15 @@ from urlparse import urlparse
 from twisted.internet import defer, reactor
 from twisted.logger import Logger
 from twisted.web.client import downloadPage
+from twisted.web.client import readBody
 
+from leap.bitmask.bonafide._http import httpRequest
 from leap.bitmask.bonafide.provider import Discovery
 from leap.bitmask.bonafide.errors import NotConfiguredError, NetworkError
 
 from leap.common.check import leap_assert
 from leap.common.config import get_path_prefix as common_get_path_prefix
-from leap.common.files import mkdir_p
+from leap.common.files import mkdir_p, get_mtime
 from leap.common.http import HTTPClient
 
 
@@ -56,6 +58,7 @@ def get_path_prefix(standalone=False):
 
 
 _preffix = get_path_prefix()
+log = Logger()
 
 
 def get_provider_path(domain, config='provider.json'):
@@ -78,19 +81,6 @@ def get_ca_cert_path(domain):
     return os.path.join('providers', domain, 'keys', 'ca', 'cacert.pem')
 
 
-def get_modification_ts(path):
-    """
-    Gets modification time of a file.
-
-    :param path: the path to get ts from
-    :type path: str
-    :returns: modification time
-    :rtype: datetime object
-    """
-    ts = os.path.getmtime(path)
-    return datetime.datetime.fromtimestamp(ts)
-
-
 def update_modification_ts(path):
     """
     Sets modification time of a file to current time.
@@ -101,7 +91,7 @@ def update_modification_ts(path):
     :rtype: datetime object
     """
     os.utime(path, None)
-    return get_modification_ts(path)
+    return get_mtime(path)
 
 
 def is_file(path):
@@ -189,18 +179,17 @@ class Provider(object):
         if not is_configured:
             if autoconf:
                 self.log.debug(
-                    'provider %s not configured: downloading files...' %
-                    domain)
+                    'BOOTSTRAP: provider %s not initialized, '
+                    'downloading files...' % domain)
                 self.bootstrap()
             else:
                 raise NotConfiguredError("Provider %s is not configured"
                                          % (domain,))
         else:
-            self.log.debug('Provider already initialized')
-            self.first_bootstrap[self._domain] = defer.succeed(
-                'already_initialized')
-            self.ongoing_bootstrap[self._domain] = defer.succeed(
-                'already_initialized')
+            self.log.debug(
+                'BOOTSTRAP: Provider is already initialized, checking if '
+                'there are newest config files...')
+            self.bootstrap(replace_if_newer=True)
 
     @property
     def domain(self):
@@ -241,7 +230,7 @@ class Provider(object):
                              " not found in provider " + self._domain)
         return config
 
-    def bootstrap(self):
+    def bootstrap(self, replace_if_newer=False):
         domain = self._domain
         self.log.debug('Bootstrapping provider %s' % domain)
         ongoing = self.ongoing_bootstrap.get(domain)
@@ -263,7 +252,7 @@ class Provider(object):
             self.first_bootstrap[domain].errback(failure)
             return failure
 
-        d = self.maybe_download_provider_info()
+        d = self.maybe_download_provider_info(replace=replace_if_newer)
         d.addCallback(self.maybe_download_ca_cert)
         d.addCallback(self.validate_ca_cert)
         d.addCallbacks(first_bootstrap_done, first_bootstrap_error)
@@ -291,6 +280,7 @@ class Provider(object):
         # TODO handle pre-seeded providers?
         # or let client handle that? We could move them to bonafide.
         provider_json = self._get_provider_json_path()
+
         if is_file(provider_json) and not replace:
             return defer.succeed('provider_info_already_exists')
 
@@ -304,8 +294,9 @@ class Provider(object):
             shutil.rmtree(folders)
             raise NetworkError(failure.getErrorMessage())
 
-        d = self._http.request(uri, method=met)
-        d.addCallback(_write_to_file, provider_json)
+        d = httpRequest(
+            self._http._agent,
+            uri, method=met, saveto=provider_json)
         d.addCallback(lambda _: self._load_provider_json())
         d.addErrback(errback)
         return d
@@ -378,12 +369,6 @@ class Provider(object):
         # UNAUTHENTICATED if we try to get the services
         # See: # https://leap.se/code/issues/7906
 
-        def check_error(content):
-            c = json.loads(content)
-            if 'error' in c:
-                raise Exception(c['error'])
-            return content
-
         def further_bootstrap_needs_auth(ignored):
             self.log.warn('Cannot download services config yet, need auth')
             pending_deferred = defer.Deferred()
@@ -391,10 +376,8 @@ class Provider(object):
             return defer.succeed('ok for now')
 
         uri, met, path = self._get_configs_download_params()
-
-        d = self._http.request(uri, method=met)
-        d.addCallback(check_error)
-        d.addCallback(_write_to_file, path)
+        d = httpRequest(
+            self._http._agent, uri, method=met, saveto=path)
         d.addCallback(lambda _: self._load_provider_json())
         d.addCallback(
             lambda _: self._get_config_for_all_services(session=None))
@@ -414,17 +397,13 @@ class Provider(object):
                 d.addCallback(lambda _: stuck.callback('continue!'))
                 return d
 
-        if not self.has_fetched_services_config():
-            self._load_provider_json()
-            uri, met, path = self._get_configs_download_params()
-            d = session.fetch_provider_configs(uri, path, met)
-            d.addCallback(verify_provider_configs)
-            d.addCallback(complete_bootstrapping)
-            return d
-        else:
-            d = defer.succeed('already downloaded')
-            d.addCallback(complete_bootstrapping)
-            return d
+        self._load_provider_json()
+        uri, met, path = self._get_configs_download_params()
+
+        d = session.fetch_provider_configs(uri, path, met)
+        d.addCallback(verify_provider_configs)
+        d.addCallback(complete_bootstrapping)
+        return d
 
     def _get_configs_download_params(self):
         uri = self._disco.get_configs_uri()
@@ -519,10 +498,11 @@ class Provider(object):
                     uri = base + str(services_dict[subservice])
                     path = self._get_service_config_path(subservice)
                     if session:
-                        d = session.fetch_provider_configs(uri, path)
+                        d = session.fetch_provider_configs(
+                            uri, path, method='GET')
                     else:
                         d = self._fetch_provider_configs_unauthenticated(
-                            uri, path)
+                            uri, path, method='GET')
                     pending.append(d)
         return defer.gatherResults(pending)
 
@@ -534,9 +514,8 @@ class Provider(object):
 
     def _fetch_provider_configs_unauthenticated(self, uri, path):
         self.log.info('Downloading config for %s...' % uri)
-        d = self._http.request(uri)
-        d.addCallback(_write_to_file, path)
-        return d
+        return httpRequest(
+            self._http._agent, uri, saveto=path)
 
 
 class Record(object):
@@ -547,16 +526,14 @@ class Record(object):
         return self.__dict__
 
 
-def _write_to_file(content, path):
-    with open(path, 'w') as f:
-        f.write(content)
-
-
 if __name__ == '__main__':
+    from twisted.internet.task import react
+
+    def main(reactor, *args):
+        provider = Provider('cdev.bitmask.net', autoconf=True)
+        return provider.callWhenReady(print_done)
 
     def print_done():
         print '>>> bootstrapping done!!!'
 
-    provider = Provider('cdev.bitmask.net')
-    provider.callWhenReady(print_done)
-    reactor.run()
+    react(main, [])
diff --git a/src/leap/bitmask/bonafide/session.py b/src/leap/bitmask/bonafide/session.py
index 711e9ee2..988cbb99 100644
--- a/src/leap/bitmask/bonafide/session.py
+++ b/src/leap/bitmask/bonafide/session.py
@@ -165,12 +165,9 @@ class Session(object):
         # right path.
         uri = self._api.get_smtp_cert_uri()
         met = self._api.get_smtp_cert_method()
-        print met, "to", uri
         return self._request(self._agent, uri, method=met)
 
     def _request(self, *args, **kw):
-        # TODO: we are not pinning the TLS cert of the API
-        # maybe we can use leap.common.http
         kw['token'] = self._token
         return httpRequest(*args, **kw)
 
@@ -201,9 +198,8 @@ class Session(object):
 
     @defer.inlineCallbacks
     def fetch_provider_configs(self, uri, path, method='GET'):
-        config = yield self._request(self._agent, uri, method=method)
-        with open(path, 'w') as cf:
-            cf.write(config)
+        config = yield self._request(
+            self._agent, uri, method=method, saveto=path)
         defer.returnValue('ok')
 
 
diff --git a/tests/e2e/conditional_downloads.py b/tests/e2e/conditional_downloads.py
new file mode 100755
index 00000000..6b9a8409
--- /dev/null
+++ b/tests/e2e/conditional_downloads.py
@@ -0,0 +1,40 @@
+#!/usr/bin/env python
+"""
+Exercises the bonafide http module against a real server.
+Checks for the conditional header behavior, ensuring that we do not write a
+config file if we receive a 304 Not Modified response.
+"""
+import uuid
+import os
+import tempfile
+import shutil
+
+from twisted.internet import defer
+from twisted.internet.task import react
+
+from leap.bitmask.bonafide._http import httpRequest
+from leap.common import http
+
+URI = 'https://demo.bitmask.net/1/configs/eip-service.json'
+tmp = tempfile.mkdtemp()
+
+
+@defer.inlineCallbacks
+def main(reactor, *args):
+    client = http.HTTPClient()
+    fname = os.path.join(tmp, str(uuid.uuid4()))
+    yield httpRequest(client._agent, URI, method='GET', saveto=fname)
+    filesize = os.path.getsize(fname)
+    assert filesize > 1
+    # touch file to 5 minutes in the past
+    past = int(os.path.getmtime(fname)) - 300
+    os.utime(fname, (past, past))
+    assert os.path.getmtime(fname) == past
+    yield httpRequest(client._agent, URI, method='GET', saveto=fname)
+    # it was not modified
+    assert os.path.getmtime(fname) == past
+    print 'OK'
+    shutil.rmtree(tmp)
+
+
+react(main, [])