Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Here geocoder authentication issues #381

Closed
kenseii opened this issue Dec 16, 2019 · 11 comments
Closed

Here geocoder authentication issues #381

kenseii opened this issue Dec 16, 2019 · 11 comments
Milestone

Comments

@kenseii
Copy link

kenseii commented Dec 16, 2019

Maybe i am mistaken but the Here API wrapper inside geopy is outdated, it seems it cant support the api_key argument which is the current way of authenticating with the here api alongside the app_id.

for example this fail as the current class uses app_id and app_code:

from geopy.geocoders import Here as GeopyHere
client = GeopyHere(api_key="YOUR_API_KEY", app_id="YOUR_APP_ID")
client.geocode("Shimogyo")

Basically all new apps don't receive an app_code so they cant work with geopy's wrapper.
is it planned in a future release or a PR is welcome?
Here docs about the app_code stopping:

@enriqueav
Copy link
Contributor

enriqueav commented Dec 17, 2019

Just to be clear. The API still supports the old type of authentication, so if you already have created an APP with app_id and app_code, it still works.

So it seems for a period of time geopy should support

  1. Old-style app_id and app_code, currently supported
  2. New api_key and app_id
  3. OAuth 2.0 (JSON Web Tokens). Not sure if it's supported for other geocoders

@KostyaEsmukov
Copy link
Member

is it planned in a future release or a PR is welcome?

No. PR is welcome.

  1. OAuth 2.0 (JSON Web Tokens). Not sure if it's supported for other geocoders

Currently there're no geocoders using oauth in geopy, but there was one before: 5fe1884

@deeplook
Copy link
Contributor

deeplook commented Dec 20, 2019

I was just about to raise an issue myself. I can help with this as I've implemented the original plugin as well. I would add only the new APIKEY mechanism available for new HERE accounts, and leave OAuth2 for later or somebody else.

In any case it would be a breaking change to the internal API which now has two positional parameters app_code and app_id. Since these will continue to work for old accounts for an unspecified period of time (see details) there is a need to support both for now (app_code and app_id or only appkey alone). Maybe @KostyaEsmukov can suggest a clean way how this would work best inside geopy?

@KostyaEsmukov
Copy link
Member

Since these will continue to work for old accounts for an unspecified period of time (see details) there is a need to support both for now (app_code and app_id or only appkey alone). Maybe @KostyaEsmukov can suggest a clean way how this would work best inside geopy?

Making app_code and app_id optional (with default value None) and adding a new keyword argument appkey=None should be fine. This way the old authentication method would still work in a backwards-compatible way, and the new one could be used with kwarg as Here(appkey="...").

GoogleV3 implements a similar approach with a subsequent validation of passed credentials in __init__:

if client_id and not secret_key:
raise ConfigurationError('Must provide secret_key with client_id.')
if secret_key and not client_id:
raise ConfigurationError('Must provide client_id with secret_key.')

@deeplook I'm going to cut a new release this week. Would you like me to wait for your PR with the fix so it could get to the upcoming release?

@deeplook
Copy link
Contributor

@KostyaEsmukov If you can wait for a few days I'd appreciate. I've implemented the fix in a different context already. It's just a matter of repeating this for geopy.

@KostyaEsmukov
Copy link
Member

@deeplook No problem at all, thank you for prompt reply!

@KostyaEsmukov KostyaEsmukov added this to the 1.21 milestone Jan 22, 2020
@trevorphillipscoding
Copy link

is there a fix for this yet?

@deeplook
Copy link
Contributor

deeplook commented Feb 2, 2020

@trevorphillips On it...

@deeplook
Copy link
Contributor

deeplook commented Feb 2, 2020

@trevorphillips @KostyaEsmukov See #388.

@KostyaEsmukov
Copy link
Member

Fixed by b81969c...29b42e5

@KostyaEsmukov
Copy link
Member

Released in 1.21.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants