From 61efb66abaaa79c00638c2420508d04ccb33385f Mon Sep 17 00:00:00 2001 From: Mario de Frutos Date: Fri, 26 May 2017 11:51:32 +0200 Subject: [PATCH 1/3] Add an extra check for google credentials If the user has a wrong base64 padded secret key the googlemaps python library is returning "TypeError: Incorrect padding" which is very hard to understand. So now we check if the secret key is a valid base64 string --- .../cartodb_services/google/exceptions.py | 2 ++ .../cartodb_services/google/geocoder.py | 17 +++++++++++++++-- .../cartodb_services/metrics/config.py | 4 ++-- server/lib/python/cartodb_services/setup.py | 2 +- .../test/test_googlegeocoder.py | 13 +++++++++---- 5 files changed, 29 insertions(+), 9 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/google/exceptions.py b/server/lib/python/cartodb_services/cartodb_services/google/exceptions.py index f4a3015..4a0e15e 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/exceptions.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/exceptions.py @@ -2,6 +2,8 @@ # -*- coding: utf-8 -*- import json +class InvalidGoogleCredentials(Exception): + pass class BadGeocodingParams(Exception): def __init__(self, value): 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 2a836f3..a8458a4 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py @@ -1,15 +1,18 @@ #!/usr/local/bin/python # -*- coding: utf-8 -*- +import base64 import googlemaps -from exceptions import MalformedResult +from exceptions import MalformedResult, InvalidGoogleCredentials 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( @@ -36,7 +39,7 @@ class GoogleMapsGeocoder: return [longitude, latitude] def _build_optional_parameters(self, city=None, state=None, - country=None): + country=None): optional_params = {} if city: optional_params['locality'] = city @@ -49,3 +52,13 @@ 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/cartodb_services/metrics/config.py b/server/lib/python/cartodb_services/cartodb_services/metrics/config.py index ee849c3..6ad260f 100644 --- a/server/lib/python/cartodb_services/cartodb_services/metrics/config.py +++ b/server/lib/python/cartodb_services/cartodb_services/metrics/config.py @@ -359,10 +359,10 @@ class GeocoderConfig(ServiceConfig): if self._geocoder_provider == self.NOKIA_GEOCODER: if not set(self.NOKIA_GEOCODER_REDIS_MANDATORY_KEYS).issubset(set(filtered_config.keys())) or \ not self.heremaps_app_id or not self.heremaps_app_code: - raise ConfigException("""Some mandatory parameter/s for Nokia geocoder are missing. Check it please""") + raise ConfigException("""Heremaps app id or app code is not set up""") elif self._geocoder_provider == self.GOOGLE_GEOCODER: if self.GOOGLE_GEOCODER_API_KEY not in filtered_config.keys(): - raise ConfigException("""Google geocoder need the mandatory parameter 'google_maps_private_key'""") + raise ConfigException("""Google geocoder private key is not set up""") elif self._geocoder_provider == self.MAPZEN_GEOCODER: if not self.mapzen_api_key: raise ConfigException("""Mapzen config is not set up""") diff --git a/server/lib/python/cartodb_services/setup.py b/server/lib/python/cartodb_services/setup.py index 681e064..1517adb 100644 --- a/server/lib/python/cartodb_services/setup.py +++ b/server/lib/python/cartodb_services/setup.py @@ -10,7 +10,7 @@ from setuptools import setup, find_packages setup( name='cartodb_services', - version='0.15.0', + version='0.15.1', description='CartoDB Services API Python Library', diff --git a/server/lib/python/cartodb_services/test/test_googlegeocoder.py b/server/lib/python/cartodb_services/test/test_googlegeocoder.py index 5593560..f700ec3 100644 --- a/server/lib/python/cartodb_services/test/test_googlegeocoder.py +++ b/server/lib/python/cartodb_services/test/test_googlegeocoder.py @@ -6,9 +6,7 @@ import requests_mock from mock import Mock from cartodb_services.google import GoogleMapsGeocoder -from cartodb_services.google.exceptions import BadGeocodingParams -from cartodb_services.google.exceptions import NoGeocodingParams -from cartodb_services.google.exceptions import MalformedResult +from cartodb_services.google.exceptions import MalformedResult, InvalidGoogleCredentials requests_mock.Mocker.TEST_PREFIX = 'test_' @@ -92,7 +90,8 @@ class GoogleGeocoderTestCase(unittest.TestCase): def setUp(self): logger = Mock() self.geocoder = GoogleMapsGeocoder('dummy_client_id', - 'MgxyOFxjZXIyOGO52jJlMzEzY1Oqy4hsO49E', logger) + 'MgxyOFxjZXIyOGO52jJlMzEzY1Oqy4hsO49E', + logger) def test_geocode_address_with_valid_params(self, req_mock): req_mock.register_uri('GET', self.GOOGLE_MAPS_GEOCODER_URL, @@ -119,3 +118,9 @@ 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) From ac854a94af7732d94e357086b2db58f1403f5e19 Mon Sep 17 00:00:00 2001 From: Mario de Frutos Date: Fri, 26 May 2017 11:52:48 +0200 Subject: [PATCH 2/3] Fixed fallback to internal geocoder in nameplaces geocoding functions - Fixed fallback becuase we have to use spiexceptions in this case because we retrieve the configuration usig plpy.execute fuctions that wrap any exception into a spiexception. - In case we don't want to use Mapzen, we could leave the api_key empty becuase we arise a ConfigException if the Mapzen api_key is empty so we are able to make fallback. Right now we can't remove the mapzen configuration because it'll fail when the InternalGeocoderConfig try to load the ServiceDBConfig This is a dirty hack, we should improve how the DB config is loaded. See --- server/extension/sql/50_namedplaces.sql | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/server/extension/sql/50_namedplaces.sql b/server/extension/sql/50_namedplaces.sql index 338591d..ca9196e 100644 --- a/server/extension/sql/50_namedplaces.sql +++ b/server/extension/sql/50_namedplaces.sql @@ -1,10 +1,11 @@ ---- cdb_geocode_namedplace_point(city_name text) CREATE OR REPLACE FUNCTION cdb_dataservices_server.cdb_geocode_namedplace_point(username text, orgname text, city_name text) RETURNS Geometry AS $$ + import spiexceptions try: mapzen_plan = plpy.prepare("SELECT cdb_dataservices_server._cdb_mapzen_geocode_namedplace($1, $2, $3) as point;", ["text", "text", "text"]) return plpy.execute(mapzen_plan, [username, orgname, city_name])[0]['point'] - except BaseException as e: + except spiexceptions.ExternalRoutineException as e: internal_plan = plpy.prepare("SELECT cdb_dataservices_server._cdb_internal_geocode_namedplace($1, $2, $3) as point;", ["text", "text", "text"]) return plpy.execute(internal_plan, [username, orgname, city_name])[0]['point'] $$ LANGUAGE plpythonu; @@ -12,10 +13,11 @@ $$ LANGUAGE plpythonu; ---- cdb_geocode_namedplace_point(city_name text, country_name text) CREATE OR REPLACE FUNCTION cdb_dataservices_server.cdb_geocode_namedplace_point(username text, orgname text, city_name text, country_name text) RETURNS Geometry AS $$ + import spiexceptions try: mapzen_plan = plpy.prepare("SELECT cdb_dataservices_server._cdb_mapzen_geocode_namedplace($1, $2, $3, NULL, $4) as point;", ["text", "text", "text", "text"]) return plpy.execute(mapzen_plan, [username, orgname, city_name, country_name])[0]['point'] - except BaseException as e: + except spiexceptions.ExternalRoutineException as e: internal_plan = plpy.prepare("SELECT cdb_dataservices_server._cdb_internal_geocode_namedplace($1, $2, $3, NULL, $4) as point;", ["text", "text", "text", "text"]) return plpy.execute(internal_plan, [username, orgname, city_name, country_name])[0]['point'] $$ LANGUAGE plpythonu; @@ -23,10 +25,11 @@ $$ LANGUAGE plpythonu; ---- cdb_geocode_namedplace_point(city_name text, admin1_name text, country_name text) CREATE OR REPLACE FUNCTION cdb_dataservices_server.cdb_geocode_namedplace_point(username text, orgname text, city_name text, admin1_name text, country_name text) RETURNS Geometry AS $$ + import spiexceptions try: mapzen_plan = plpy.prepare("SELECT cdb_dataservices_server._cdb_mapzen_geocode_namedplace($1, $2, $3, $4, $5) as point;", ["text", "text", "text", "text", "text"]) return plpy.execute(mapzen_plan, [username, orgname, city_name, admin1_name, country_name])[0]['point'] - except BaseException as e: + except spiexceptions.ExternalRoutineException as e: internal_plan = plpy.prepare("SELECT cdb_dataservices_server._cdb_internal_geocode_namedplace($1, $2, $3, $4, $5) as point;", ["text", "text", "text", "text", "text"]) return plpy.execute(internal_plan, [username, orgname, city_name, admin1_name, country_name])[0]['point'] $$ LANGUAGE plpythonu; From ad6ed9a9bcc15e37f5be0b90f2d737d4ae8cf58c Mon Sep 17 00:00:00 2001 From: Mario de Frutos Date: Fri, 26 May 2017 12:30:52 +0200 Subject: [PATCH 3/3] Bump version for DS --- server/extension/cdb_dataservices_server--0.24.1--0.24.2.sql | 5 +++++ server/extension/cdb_dataservices_server--0.24.2--0.24.1.sql | 5 +++++ server/extension/cdb_dataservices_server.control | 2 +- .../cdb_dataservices_server--0.24.0--0.24.1.sql | 0 .../cdb_dataservices_server--0.24.1--0.24.0.sql | 0 .../{ => old_versions}/cdb_dataservices_server--0.24.1.sql | 0 6 files changed, 11 insertions(+), 1 deletion(-) create mode 100644 server/extension/cdb_dataservices_server--0.24.1--0.24.2.sql create mode 100644 server/extension/cdb_dataservices_server--0.24.2--0.24.1.sql rename server/extension/{ => old_versions}/cdb_dataservices_server--0.24.0--0.24.1.sql (100%) rename server/extension/{ => old_versions}/cdb_dataservices_server--0.24.1--0.24.0.sql (100%) rename server/extension/{ => old_versions}/cdb_dataservices_server--0.24.1.sql (100%) diff --git a/server/extension/cdb_dataservices_server--0.24.1--0.24.2.sql b/server/extension/cdb_dataservices_server--0.24.1--0.24.2.sql new file mode 100644 index 0000000..47ffa98 --- /dev/null +++ b/server/extension/cdb_dataservices_server--0.24.1--0.24.2.sql @@ -0,0 +1,5 @@ +--DO NOT MODIFY THIS FILE, IT IS GENERATED AUTOMATICALLY FROM SOURCES +-- Complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "ALTER EXTENSION cdb_dataservices_server UPDATE TO '0.24.2'" to load this file. \quit + +-- HERE goes your code to upgrade/downgrade diff --git a/server/extension/cdb_dataservices_server--0.24.2--0.24.1.sql b/server/extension/cdb_dataservices_server--0.24.2--0.24.1.sql new file mode 100644 index 0000000..8e9d84c --- /dev/null +++ b/server/extension/cdb_dataservices_server--0.24.2--0.24.1.sql @@ -0,0 +1,5 @@ +--DO NOT MODIFY THIS FILE, IT IS GENERATED AUTOMATICALLY FROM SOURCES +-- Complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "ALTER EXTENSION cdb_dataservices_server UPDATE TO '0.24.1'" to load this file. \quit + +-- HERE goes your code to upgrade/downgrade diff --git a/server/extension/cdb_dataservices_server.control b/server/extension/cdb_dataservices_server.control index 487ed63..1512a6d 100644 --- a/server/extension/cdb_dataservices_server.control +++ b/server/extension/cdb_dataservices_server.control @@ -1,5 +1,5 @@ comment = 'CartoDB dataservices server extension' -default_version = '0.24.1' +default_version = '0.24.2' requires = 'plpythonu, plproxy, postgis, cdb_geocoder' superuser = true schema = cdb_dataservices_server diff --git a/server/extension/cdb_dataservices_server--0.24.0--0.24.1.sql b/server/extension/old_versions/cdb_dataservices_server--0.24.0--0.24.1.sql similarity index 100% rename from server/extension/cdb_dataservices_server--0.24.0--0.24.1.sql rename to server/extension/old_versions/cdb_dataservices_server--0.24.0--0.24.1.sql diff --git a/server/extension/cdb_dataservices_server--0.24.1--0.24.0.sql b/server/extension/old_versions/cdb_dataservices_server--0.24.1--0.24.0.sql similarity index 100% rename from server/extension/cdb_dataservices_server--0.24.1--0.24.0.sql rename to server/extension/old_versions/cdb_dataservices_server--0.24.1--0.24.0.sql diff --git a/server/extension/cdb_dataservices_server--0.24.1.sql b/server/extension/old_versions/cdb_dataservices_server--0.24.1.sql similarity index 100% rename from server/extension/cdb_dataservices_server--0.24.1.sql rename to server/extension/old_versions/cdb_dataservices_server--0.24.1.sql