From d029ad7041829dda0f003f507875e3798d1bc13c Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 6 Oct 2017 10:49:40 +0200 Subject: [PATCH 01/10] GoogleMapsClientFactory to persist clients #401 Add a GoogleMapsClientFactory and remove the check for valid key, which is no longer needed, as it is done in the google library. --- .../cartodb_services/google/client_factory.py | 17 +++++++++++++++++ .../cartodb_services/google/geocoder.py | 16 ++-------------- 2 files changed, 19 insertions(+), 14 deletions(-) create mode 100644 server/lib/python/cartodb_services/cartodb_services/google/client_factory.py 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..3e2ada6 --- /dev/null +++ b/server/lib/python/cartodb_services/cartodb_services/google/client_factory.py @@ -0,0 +1,17 @@ +#!/usr/local/bin/python +# -*- coding: utf-8 -*- + +import googlemaps + +class GoogleMapsClientFactory(): + clients = {} + + @classmethod + def get(cls, client_id, client_secret): + client = cls.clients.get(client_id) + if not client: + client = googlemaps.Client( + client_id=client_id, + client_secret=client_secret) + cls.clients[client_id] = client + return client 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..b900921 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py @@ -5,18 +5,16 @@ import base64 import googlemaps from exceptions import MalformedResult, InvalidGoogleCredentials +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 +50,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 From 854fbb803c1cce795c42b439de3f733df6ac1ffc Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 6 Oct 2017 13:41:50 +0200 Subject: [PATCH 02/10] Use a different client_id for the test --- server/lib/python/cartodb_services/test/test_googlegeocoder.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/lib/python/cartodb_services/test/test_googlegeocoder.py b/server/lib/python/cartodb_services/test/test_googlegeocoder.py index f700ec3..d2ab93c 100644 --- a/server/lib/python/cartodb_services/test/test_googlegeocoder.py +++ b/server/lib/python/cartodb_services/test/test_googlegeocoder.py @@ -121,6 +121,6 @@ class GoogleGeocoderTestCase(unittest.TestCase): def test_invalid_credentials(self, req_mock): with self.assertRaises(InvalidGoogleCredentials): - GoogleMapsGeocoder('dummy_client_id', + GoogleMapsGeocoder('another_dummy_client_id', 'lalala', None) From f79ac9297db22f7dd28d232d7b60131687b82d2e Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 6 Oct 2017 13:44:01 +0200 Subject: [PATCH 03/10] Add again the check for valid credentials As our check is more strict that the one provided in googlemaps library. --- .../cartodb_services/google/client_factory.py | 19 +++++++++++++++++++ .../cartodb_services/google/geocoder.py | 3 +-- 2 files changed, 20 insertions(+), 2 deletions(-) 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 index 3e2ada6..61e5d4e 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/client_factory.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/client_factory.py @@ -2,6 +2,8 @@ # -*- coding: utf-8 -*- import googlemaps +import base64 +from exceptions import InvalidGoogleCredentials class GoogleMapsClientFactory(): clients = {} @@ -10,8 +12,25 @@ class GoogleMapsClientFactory(): def get(cls, client_id, client_secret): client = cls.clients.get(client_id) if not client: + cls.assert_valid_crendentials(client_secret) client = googlemaps.Client( client_id=client_id, client_secret=client_secret) cls.clients[client_id] = 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 + base64.b64decode(client_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 b900921..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,10 +1,9 @@ #!/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 From 7b988e3f78315bf7dd4a99b927d35ad7eb0ff650 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 6 Oct 2017 14:37:26 +0200 Subject: [PATCH 04/10] Add tests for #382 The examples are not arbitrary: base64 by default ignores characters that are not in its alphabet. When decoding, the dashes and underscores are not b64-valid and therefore ignored, resulting in a sequence with the wrong padding. --- .../cartodb_services/test/test_googlegeocoder.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/server/lib/python/cartodb_services/test/test_googlegeocoder.py b/server/lib/python/cartodb_services/test/test_googlegeocoder.py index d2ab93c..c910d23 100644 --- a/server/lib/python/cartodb_services/test/test_googlegeocoder.py +++ b/server/lib/python/cartodb_services/test/test_googlegeocoder.py @@ -124,3 +124,13 @@ class GoogleGeocoderTestCase(unittest.TestCase): GoogleMapsGeocoder('another_dummy_client_id', 'lalala', None) + + def test_credentials_with_dashes_can_be_valid(self, req_mock): + GoogleMapsGeocoder('yet_another_dummy_client_id', + 'Ola-k-ase---', + None) + + def test_credentials_with_underscores_can_be_valid(self, req_mock): + GoogleMapsGeocoder('yet_another_dummy_client_id', + 'Ola_k_ase___', + None) From 8e33cf9723b46e10498f0a9c3cf3022c6a43666d Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 6 Oct 2017 14:40:53 +0200 Subject: [PATCH 05/10] Fix google services key validity check #382 --- .../cartodb_services/cartodb_services/google/client_factory.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 index 61e5d4e..c60c1c3 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/client_factory.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/client_factory.py @@ -30,7 +30,8 @@ class GoogleMapsClientFactory(): # 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(client_secret) + b64_secret = client_secret.replace('-', '+').replace('_', '/') + base64.b64decode(b64_secret) return True except TypeError: return False From b8e1120169f656c59ba404e4ac499d95c4419270 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 6 Oct 2017 15:03:08 +0200 Subject: [PATCH 06/10] Revert "Use a different client_id for the test" This reverts commit 854fbb803c1cce795c42b439de3f733df6ac1ffc. --- server/lib/python/cartodb_services/test/test_googlegeocoder.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/lib/python/cartodb_services/test/test_googlegeocoder.py b/server/lib/python/cartodb_services/test/test_googlegeocoder.py index c910d23..f2b7a5e 100644 --- a/server/lib/python/cartodb_services/test/test_googlegeocoder.py +++ b/server/lib/python/cartodb_services/test/test_googlegeocoder.py @@ -121,7 +121,7 @@ class GoogleGeocoderTestCase(unittest.TestCase): def test_invalid_credentials(self, req_mock): with self.assertRaises(InvalidGoogleCredentials): - GoogleMapsGeocoder('another_dummy_client_id', + GoogleMapsGeocoder('dummy_client_id', 'lalala', None) From 27fde5a910c5274e6f56bbb0b46dbb375822296d Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 6 Oct 2017 15:04:18 +0200 Subject: [PATCH 07/10] Use "{id}:{secret}" as caching key --- .../cartodb_services/google/client_factory.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 index c60c1c3..caf3fd8 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/client_factory.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/client_factory.py @@ -10,13 +10,14 @@ class GoogleMapsClientFactory(): @classmethod def get(cls, client_id, client_secret): - client = cls.clients.get(client_id) + 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[client_id] = client + cls.clients[cache_key] = client return client @classmethod From 5005572f89eb0f2bac761360b2f9306de7aed1a3 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 6 Oct 2017 15:18:51 +0200 Subject: [PATCH 08/10] Add tests for GoogleMapsClientFactory --- .../test/test_google_client_factory.py | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 server/lib/python/cartodb_services/test/test_google_client_factory.py 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..b5b3121 --- /dev/null +++ b/server/lib/python/cartodb_services/test/test_google_client_factory.py @@ -0,0 +1,47 @@ +#!/usr/local/bin/python +# -*- coding: utf-8 -*- + +import unittest +import base64 + +from cartodb_services.google.client_factory import GoogleMapsClientFactory + +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) From 6c3260b6ee8cffd2ba9a46cae41bc493dafcb3be Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 6 Oct 2017 15:23:44 +0200 Subject: [PATCH 09/10] Move credentials tests to GoogleMapsClientFactoryTestCase --- .../test/test_google_client_factory.py | 11 +++++++++++ .../test/test_googlegeocoder.py | 18 +----------------- 2 files changed, 12 insertions(+), 17 deletions(-) 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 index b5b3121..f245925 100644 --- a/server/lib/python/cartodb_services/test/test_google_client_factory.py +++ b/server/lib/python/cartodb_services/test/test_google_client_factory.py @@ -5,6 +5,7 @@ import unittest import base64 from cartodb_services.google.client_factory import GoogleMapsClientFactory +from cartodb_services.google.exceptions import InvalidGoogleCredentials class GoogleMapsClientFactoryTestCase(unittest.TestCase): @@ -45,3 +46,13 @@ class GoogleMapsClientFactoryTestCase(unittest.TestCase): 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): + GoogleMapsClientFactory.get('yet_another_dummy_client_id', 'Ola-k-ase---') + + def test_credentials_with_underscores_can_be_valid(self): + GoogleMapsClientFactory.get('yet_another_dummy_client_id', 'Ola_k_ase___') diff --git a/server/lib/python/cartodb_services/test/test_googlegeocoder.py b/server/lib/python/cartodb_services/test/test_googlegeocoder.py index f2b7a5e..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,19 +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) - - def test_credentials_with_dashes_can_be_valid(self, req_mock): - GoogleMapsGeocoder('yet_another_dummy_client_id', - 'Ola-k-ase---', - None) - - def test_credentials_with_underscores_can_be_valid(self, req_mock): - GoogleMapsGeocoder('yet_another_dummy_client_id', - 'Ola_k_ase___', - None) From 3ed41b065a6f5dff105d2e20cc93e9300548675b Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 6 Oct 2017 15:27:51 +0200 Subject: [PATCH 10/10] Marginally improve tests for #382 --- .../cartodb_services/test/test_google_client_factory.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) 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 index f245925..f09da5a 100644 --- a/server/lib/python/cartodb_services/test/test_google_client_factory.py +++ b/server/lib/python/cartodb_services/test/test_google_client_factory.py @@ -1,6 +1,7 @@ #!/usr/local/bin/python # -*- coding: utf-8 -*- +import googlemaps import unittest import base64 @@ -52,7 +53,9 @@ class GoogleMapsClientFactoryTestCase(unittest.TestCase): GoogleMapsClientFactory.get('dummy_client_id', 'lalala') def test_credentials_with_dashes_can_be_valid(self): - GoogleMapsClientFactory.get('yet_another_dummy_client_id', 'Ola-k-ase---') + 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): - GoogleMapsClientFactory.get('yet_another_dummy_client_id', 'Ola_k_ase___') + client = GoogleMapsClientFactory.get('yet_another_dummy_client_id', 'Ola_k_ase___') + self.assertIsInstance(client, googlemaps.Client)