diff --git a/server/lib/python/cartodb_services/cartodb_services/google/client_factory.py b/server/lib/python/cartodb_services/cartodb_services/google/client_factory.py new file mode 100644 index 0000000..caf3fd8 --- /dev/null +++ b/server/lib/python/cartodb_services/cartodb_services/google/client_factory.py @@ -0,0 +1,38 @@ +#!/usr/local/bin/python +# -*- coding: utf-8 -*- + +import googlemaps +import base64 +from exceptions import InvalidGoogleCredentials + +class GoogleMapsClientFactory(): + clients = {} + + @classmethod + def get(cls, client_id, client_secret): + cache_key = "{}:{}".format(client_id, client_secret) + client = cls.clients.get(cache_key) + if not client: + cls.assert_valid_crendentials(client_secret) + client = googlemaps.Client( + client_id=client_id, + client_secret=client_secret) + cls.clients[cache_key] = client + return client + + @classmethod + def assert_valid_crendentials(cls, client_secret): + if not cls.valid_credentials(client_secret): + raise InvalidGoogleCredentials + + @staticmethod + def valid_credentials(client_secret): + try: + # Only fails if the string dont have a correct padding for b64 + # but this way we could provide a more clear error than + # TypeError: Incorrect padding + b64_secret = client_secret.replace('-', '+').replace('_', '/') + base64.b64decode(b64_secret) + return True + except TypeError: + return False diff --git a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py index a8458a4..bf14b84 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py @@ -1,22 +1,19 @@ #!/usr/local/bin/python # -*- coding: utf-8 -*- -import base64 import googlemaps -from exceptions import MalformedResult, InvalidGoogleCredentials +from exceptions import MalformedResult +from client_factory import GoogleMapsClientFactory class GoogleMapsGeocoder: """A Google Maps Geocoder wrapper for python""" def __init__(self, client_id, client_secret, logger): - if not self._valid_credentials(client_secret): - raise InvalidGoogleCredentials('Invalid google secret key') self.client_id = self._clean_client_id(client_id) self.client_secret = client_secret - self.geocoder = googlemaps.Client( - client_id=self.client_id, client_secret=self.client_secret) + self.geocoder = GoogleMapsClientFactory.get(self.client_id, self.client_secret) self._logger = logger def geocode(self, searchtext, city=None, state=None, @@ -52,13 +49,3 @@ class GoogleMapsGeocoder: def _clean_client_id(self, client_id): # Consistency with how the client_id is saved in metadata return client_id.replace('client=', '') - - def _valid_credentials(self, private_key): - try: - # Only fails if the string dont have a correct padding for b64 - # but this way we could provide a more clear error than - # TypeError: Incorrect padding - base64.b64decode(private_key) - return True - except TypeError: - return False diff --git a/server/lib/python/cartodb_services/test/test_google_client_factory.py b/server/lib/python/cartodb_services/test/test_google_client_factory.py new file mode 100644 index 0000000..f09da5a --- /dev/null +++ b/server/lib/python/cartodb_services/test/test_google_client_factory.py @@ -0,0 +1,61 @@ +#!/usr/local/bin/python +# -*- coding: utf-8 -*- + +import googlemaps +import unittest +import base64 + +from cartodb_services.google.client_factory import GoogleMapsClientFactory +from cartodb_services.google.exceptions import InvalidGoogleCredentials + +class GoogleMapsClientFactoryTestCase(unittest.TestCase): + + def setUp(self): + pass + + def tearDown(self): + # reset client cache + GoogleMapsClientFactory.clients = {} + + def test_consecutive_calls_with_same_params_return_same_client(self): + id = 'any_id' + key = base64.b64encode('any_key') + client1 = GoogleMapsClientFactory.get(id, key) + client2 = GoogleMapsClientFactory.get(id, key) + self.assertEqual(client1, client2) + + def test_consecutive_calls_with_different_key_return_different_clients(self): + """ + This requirement is important for security reasons as well as not to + cache a wrong key accidentally. + """ + id = 'any_id' + key1 = base64.b64encode('any_key') + key2 = base64.b64encode('another_key') + client1 = GoogleMapsClientFactory.get(id, key1) + client2 = GoogleMapsClientFactory.get(id, key2) + self.assertNotEqual(client1, client2) + + def test_consecutive_calls_with_different_ids_return_different_clients(self): + """ + This requirement is important for security reasons as well as not to + cache a wrong key accidentally. + """ + id1 = 'any_id' + id2 = 'another_id' + key = base64.b64encode('any_key') + client1 = GoogleMapsClientFactory.get(id1, key) + client2 = GoogleMapsClientFactory.get(id2, key) + self.assertNotEqual(client1, client2) + + def test_invalid_credentials(self): + with self.assertRaises(InvalidGoogleCredentials): + GoogleMapsClientFactory.get('dummy_client_id', 'lalala') + + def test_credentials_with_dashes_can_be_valid(self): + client = GoogleMapsClientFactory.get('yet_another_dummy_client_id', 'Ola-k-ase---') + self.assertIsInstance(client, googlemaps.Client) + + def test_credentials_with_underscores_can_be_valid(self): + client = GoogleMapsClientFactory.get('yet_another_dummy_client_id', 'Ola_k_ase___') + self.assertIsInstance(client, googlemaps.Client) diff --git a/server/lib/python/cartodb_services/test/test_googlegeocoder.py b/server/lib/python/cartodb_services/test/test_googlegeocoder.py index f700ec3..ecbaa7b 100644 --- a/server/lib/python/cartodb_services/test/test_googlegeocoder.py +++ b/server/lib/python/cartodb_services/test/test_googlegeocoder.py @@ -6,7 +6,7 @@ import requests_mock from mock import Mock from cartodb_services.google import GoogleMapsGeocoder -from cartodb_services.google.exceptions import MalformedResult, InvalidGoogleCredentials +from cartodb_services.google.exceptions import MalformedResult requests_mock.Mocker.TEST_PREFIX = 'test_' @@ -118,9 +118,3 @@ class GoogleGeocoderTestCase(unittest.TestCase): searchtext='Calle Eloy Gonzalo 27', city='Madrid', country='España') - - def test_invalid_credentials(self, req_mock): - with self.assertRaises(InvalidGoogleCredentials): - GoogleMapsGeocoder('dummy_client_id', - 'lalala', - None)