[go: nahoru, domu]

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

Using a restricted Google Maps API key yields no results-- what referrer is the geocoding request coming from? #269

Closed
janemumei opened this issue Mar 26, 2021 · 20 comments

Comments

@janemumei
Copy link

I have a Google Maps API key that's restricted to my domain, so that nobody can "steal" my key. Street View works, and other enabled APIs work, but geocoding using geosearch doesn't work. Switching to an unrestricted API key makes it work.

@Boschman
Copy link

I've got exactly the same problem.

As far as I understand there are two different Google Geocoding API versions: a server-side version (which should not be used in a client-side project, because you can't protect your API key) and a client-side version, which is part of the Maps JavaScript API.

Does anybody know what version is used for leaflet-geosearch / GoogleProvider?

Also see this Stackoverflow answer.

@Boschman
Copy link
Boschman commented Apr 1, 2021

I've done some research and it seems that GoogleProvider is using the server-side version of the API.

Important warning: don't use leaflet-geosearch/GoogleProvider in your client-side projects! You're not able to protect your API key, so people could easily steal the key from your javascript code, and start using it in their own project.

You will be billed for all the API requests they generate!

As far as I understand the only solution is to rewrite GoogleProvider. It should use the Geocoding Service of the Maps JavaScript API. Unfortunately I don't have the time to do this...

Or am I missing something and is there another solution?

@darrenklein
Copy link
Collaborator

@jonoyuan I believe that you can restrict by IP addresses for Google's geocoding service, so that would be an option if you know that your client has a fixed IP (or fixed within a known range). Having said that, I must emphasize that, as @Boschman pointed out, Google discourages the use of the geocoding API for client-side geocoding:

"This service is generally designed for geocoding static (known in advance) addresses for placement of application content on a map; this service is not designed to respond in real time to user input. For dynamic geocoding (for example, within a user interface element), consult the documentation for the Maps JavaScript API client geocoder and/or the Google Play services Location APIs."

https://developers.google.com/maps/documentation/geocoding/overview

So yeah, the right thing to do would be to rework the Google provider in this project to use the JS API instead.

(I had no idea about any of this until I started trying to use the Google provider and ran into the same issues that you did when trying to restrict my key)

@smeijer
Copy link
Owner
smeijer commented Sep 7, 2021

I do not have time to rewrite the Google Provider on the short term, and removing it from the source would break peoples applications.

Does anyone here have time to look into this and have it use a different API?

@yznts
Copy link
Collaborator
yznts commented Sep 7, 2021

Looks like something critical. I'll try to take a look it next weekend, if no one takes it. But I can't guarantee anything, since I don't have much time myself.

@darrenklein
Copy link
Collaborator

@smeijer Thank you so much for all of your hard work on this project!

Seems like we're all in the same boat - the boat in which there is precious little time... I did start to experiment with this yesterday, but the JS/TS development environment is not my strong suit, so I can't say I was able to make much progress. Someone with more experience building TS libraries would probably be in a better position to wrap this up quickly and well - so anyone reading this should not assume that I'm confident in my ability to successfully resolve this.

In the meanwhile, here's what I've been able to figure out so far -

To make use of the Google Maps JS API geocoder, it's a little more involved than just using a different endpoint. Of course, there is a different request URL - for example

https://maps.googleapis.com/maps/api/js/GeocodeService.Search?4sSydney%2C%20NSW&7sUS&9sen-US&callback=_xdc_._oy2rtl&key=API_KEY&channel=1&token=97414

but Google will block calls to this endpoint from the browser, returning the error "Google Maps JavaScript API library must be loaded" (however, calls to this endpoint from a client like Postman are successful). Perhaps there's a clever way around that, but it seems that it will be necessary to include the JS API package - a little info on that https://developers.google.com/maps/documentation/javascript/overview.

And an otherwise handy resource with a JS geocoding example - https://developers.google.com/maps/documentation/javascript/examples/geocoding-simple

@smeijer
Copy link
Owner
smeijer commented Sep 7, 2021

Google Maps JavaScript API library must be loaded

Yeah, I remember that I ran into that when I created the current Google Provider. If possible, I'd like to prevent any Google scripts from being loaded. If that's for some reason not possible, then we should at least make it lazy-loaded (load when rendering the Provider, and make sure it isn't included in /dist bundles).

Let's share any findings here so that over time we collect enough info for someone to fix it in the time they have. It would be a waste if multiple people end up having half-finished branches, that are never submitted. Better make it a team effort ❤️

@yuriizinets thanks! No pressure 🙂

@darrenklein
Copy link
Collaborator

I agree - it'd be great to not have to depend on loading anything else from Google.

Team effort 👍 !!!

@yznts
Copy link
Collaborator
yznts commented Sep 7, 2021

I don't think that bypassing Google requirements is a great idea. I had some experience with bypassing google's translate service limitations. Any solution, that uses unofficial API, is a temporary solution. Even google's official libs/APIs are changing frequently. Supporting unofficial way will require a lot of time and will lead to unobvious situations.

Google generates callback name and token for request from input. With this 2 parameters google knows when you're trying to use API directly. Theoretically it's possible to reverse google's JS to find out how callback name and token are generating, but it's really hard. Digging into obfuscated JS is a real pain, I tried :)

So, I see 2 options:

  • Dynamically load google's script on provider init
  • Give user full control on that, take google's geocoder object as parameter for provider init

@smeijer, so, the choice is yours :)

@smeijer
Copy link
Owner
smeijer commented Sep 8, 2021

Thanks for the explanation.

Giving the user full control, means it will be a breaking change. Thereby I think that the best route is to dynamically load it.

@darrenklein
Copy link
Collaborator

@smeijer I've finally gotten a few free moments to start looking at this, though with JS/TS not being my strong suit, I'm a bit stuck, was wondering if you could help out.

After researching a bit, I did notice that there's an NPM module that could help out here - https://github.com/googlemaps/js-api-loader - that'll load the necessary script into the DOM. I added that to the "dependencies" in the package.json, and added the types from "@types/google.maps": "^3.47.2" to the "devDependencies". Then I just tried lifting the code directly from the example and adding it to the GoogleProvider constructor in googleProvider.ts like so -

export default class GoogleProvider extends AbstractProvider<
  RequestResult,
  RawResult
> {
  searchUrl = 'https://maps.googleapis.com/maps/api/geocode/json';

  loader = new Loader({
    apiKey: "",
    version: "weekly",
    libraries: ["places"]
  });

  loader.load();

  endpoint({ query }: EndpointArgument) {
    const params = typeof query === 'string' ? { address: query } : query;
    return this.getUrl(this.searchUrl, params);
  }

  parse(result: ParseArgument<RequestResult>): SearchResult<RawResult>[] {
    return result.data.results.map((r) => ({
      x: r.geometry.location.lng,
      y: r.geometry.location.lat,
      label: r.formatted_address,
      bounds: [
        [r.geometry.viewport.southwest.lat, r.geometry.viewport.southwest.lng], // s, w
        [r.geometry.viewport.northeast.lat, r.geometry.viewport.northeast.lng], // n, e
      ],
      raw: r,
    }));
  }
}

However, when I try to build this via npm run build, I get the following error -

(rpt2 plugin) Error: src/providers/googleProvider.ts:49:9 - error TS1005: ';' expected.

49   loader.load();
           ~

Error: src/providers/googleProvider.ts:49:9 - error TS1005: ';' expected.

49   loader.load();
           ~

If I remove loader.load(); the code compiles just fine and I can see this.loader is added to lib/googleProvider.js... but this line seems to be causing a fuss, and I'm not sure how to fix it, or how this should otherwise be handled... but basically, on instantiating this class, I'd want to call the loader's load() method, which should add the script to the DOM.

Any guidance would be much appreciated (and please do let me know if you think I'm barking up the wrong tree with this approach).

@smeijer
Copy link
Owner
smeijer commented Jan 9, 2022

Thanks for taking the time to figure this out!

Regarding the typescript warning, you can't call a function (loader.load()) there as if you're defining a class property.

You probably wish to implement a (class) constructor method, and call loader.load in the constructor.

@darrenklein
Copy link
Collaborator

Ah excellent, thank you, an explicit constructor in the class seemed to do the trick. I didn't realize it would be necessary, since it seemed to me like a constructor method was being added during the build process to wrap the searchUrl (for example), but yep, this had the desired end result. Thanks very much for your guidance there.

@darrenklein
Copy link
Collaborator

@smeijer I think I might be close to a solution, but I'm going to need some help to get it over the finish line. I've been working on a fork, https://github.com/darrenklein/leaflet-geosearch/tree/google-provider-fix - it's not working (nor compiling) in its current form, but I did kinda get it to work in a very limited way - but then I just pseudo-coded it as best I could to try to illustrate the flow that I think will work. Here's a bit of a summary as to what I've figured out so far, and what's up on my branch...

In order to properly use Google's geocoding service when making dynamic geocoding requests from a web client, the page needs to import the Google Maps JS API. I was able to do this by adding the @googlemaps/js-api-loader NPM module, which is a pretty lightweight module that pulls that script in. That script is important, because it has a Geocoder class that can handle making API calls to the https://maps.googleapis.com/maps/api/js/GeocodeService.Search endpoint.

I set up googleProvider.ts to call the js-api-loader's load() method in its constructor - that retrieves the Maps JS script and appends it to the DOM - and in the callback method for the loader, we get access to the google object, where all the good stuff lives. At this point, the code I have in the repo becomes pseudo-code, more or less. However, I did a test where I created a new instance of the Geocoder right in the constructor and geocoded an address that I hard-coded - and it worked! But that's obviously not a sustainable solution, just a proof-of-concept.

The approach that I think that we want to take is to create a new instance of the Google Maps' Geocoder in the constructor and then add that to the class as this.geoCoder, or something along those lines. Then we could have the GoogleProvider implement its own search method that overrides the AbstractProvider's, as the handling of the API call is somewhat different - we simply call the Geocoder's geocode method, rather than needing to use a fetch call, and pass the result to the parse method (which I think will work as-is, with just a few minor tweaks related to some key naming differences). Here's a slightly abridged excerpt of that from my fork -

export default class GoogleProvider extends AbstractProvider<
  RequestResult,
  RawResult
> {
  loader: Loader;
  // Not sure what geoCoder's type should be
  geoCoder: any;

  constructor(options: ProviderOptions = {}) {
    super({ ...options });

    // The toString() is necessary to address a type mismatch -
    // key can be string | number | boolean, but Google Maps
    // expects it to be a string.
    this.loader = new Loader({
      apiKey: options.params ? options.params['key'].toString() : '',
      version: 'weekly',
    });

    this.loader.load()
    .then((google) => {
      this.geoCoder = new google.maps.Geocoder();
    })
  }

  parse(result: ParseArgument<RequestResult>): SearchResult<RawResult>[] {
    return result.data.results.map((r) => ({
      x: r.geometry.location.lng,
      y: r.geometry.location.lat,
      label: r.formatted_address,
      bounds: [
        [r.geometry.viewport.Ab.g, r.geometry.Ra.g], // s, w
        [r.geometry.viewport.Ab.h, r.geometry.Ra.h], // n, e
      ],
      raw: r,
    }));

    // "pseudo-code" - this isn't working, but I think it might if it
    // were able to override the AbstractProvider's search method.
    search(input) {
      this.geoCoder.geocode({
        address: input,
        // we'll probably want to allow the user to optionally specify a region;
        // Google will fallback to US as a default if none is provided.
        region: 'US',
      }, (results, status) => {
        // results appears to be an array of objects, not clear if they need
        // any additional pre-processing before passing off to parse()
        return this.parse({ data, results });
      })
    }
  }
}

So yeah this isn't working code, but I think it's the idea.

The final piece to the puzzle is proper configuration of the Google Maps API key - the key needs to be configured to allow both the Maps JavaScript API as well as the Geocoding API -

Screen Shot 2022-01-10 at 11 56 26 PM

Because the Geocoding API is enabled, you can still hit the original geocoding endpoint https://maps.googleapis.com/maps/api/geocode/json as well - however, the whole reason I ended up down this rabbit hole in the first place was because I wanted to be able to add a referrer restriction to my key, and the Geocoding API doesn't support that behavior. But good news! Once the key is enabled for the JavaScript API, you can safely add the referrer restriction - attempts to use the original geocoding endpoint will fail, because referrer restrictions aren't supported and the API just outright rejects the request, but an unauthorized client gets the message "Google Maps JavaScript API error: RefererNotAllowedMapError" - very cool. I tested authorizing my localhost port, and requests from the browser worked (in my shoddy test), but requests from Postman against the old API failed. Internally, your key needs to be able to access the Geocoding API, but adding the referrer restriction makes it unavailable to the outside world and limits the use of the JS API, exactly what we want.

Anyway, sorry to dump a lot on you. Any guidance or feedback you can offer would be much appreciated!

@darrenklein
Copy link
Collaborator
darrenklein commented Jan 15, 2022

@smeijer I believe that I have a working fix for this - #311, but it's going to need some eyes on it. I mention this in the PR as well, but in my local dev environment, I can't quite seem to get this repo to work quite right - any time I geocode an address when using a local copy of this repo (regardless of the provider I'm using), I get errors in the calls to centerMap and addMarker, though the data those functions are dealing with all looks right to me.

I'll try to dig into that some more, but what I can say so far is that this setup seems to work, and it looks like the data that we get back is being handled correctly. For example, the final result that we get from OSM looks like

{
  bounds: Array(2),
  0: (2) [41.1600725, -85.1745374],
  1: (2) [41.1600875, -85.1733126],
  length: 2,
  [[Prototype]]: Array(0),
  label: "Allen Street, Wallen, Fort Wayne, Allen County, Indiana, 46818, United States",
  raw: {place_id: 102903001, licence: 'Data © OpenStreetMap contributors, ODbL 1.0. https://osm.org/copyright', osm_type: 'way', osm_id: 17250310, boundingbox: Array(4), …},
  x: -85.1733126,
  y: 41.1600725
}

and the result we get from the Google provider is

{
  bounds: Array(2),
  0: (2) [31.1129391697085, -97.8923810302915],
  1: (2) [31.1156371302915, -97.8896830697085],
  length: 2,
  [[Prototype]]: Array(0),
  label: "516 Allen St, Copperas Cove, TX 76522, USA",
  raw: {address_components: Array(8), formatted_address: '516 Allen St, Copperas Cove, TX 76522, USA', geometry: {…}, place_id: 'ChIJ2xotHEuxWoYRIt98h1TbtNU', types: Array(1)},
  x: -97.8910185,
  y: 31.1142757
}

But definitely still work to do. Anyone available to give this a look over and advise?

@smeijer
Copy link
Owner
smeijer commented Jan 15, 2022

@darrenklein sorry for my absence. I've had a busy week. I'll do my best to catch up over the weekend.

Thanks for all the hard work you've put into this. Much appreciated!

@darrenklein
Copy link
Collaborator

@smeijer I've made some solid further progress with this, I'm just getting a little muddled with some of the types, and figuring out how best to handle testing. Would you be interested in maybe pairing with me sometime and seeing if we can knock this out together? I don't think it'd take too long.

@darrenklein
Copy link
Collaborator
darrenklein commented Feb 6, 2022

I think my PR might be just about where it needs to be - I did manage to figure out how to do some additional mocking for tests and got them to pass.

Although I was hoping that this PR (#311) wouldn't introduce breaking changes, I'm afraid that it might have to, if it's to be done "right"... the challenge there is that, when the Google provider is instantiated, we need to make an async call to Google to load the Google Maps JS API into the DOM. Initially, I was making that call in the constructor, but I came to understand that wasn't a good strategy, as it could result in calls to methods that are dependent on that script before the script had loaded. Instead, I added a static method that can retrieve the script, and then the result of that call can be passed to the object instantiation call - from the user's perspective, it would look something like

import { GoogleProvider } from "leaflet-geosearch";

const options = {
  params: {
    key: 'GOOGLE_MAPS_API_KEY',
    region: 'nl'
  }
}

const geoCoder = await GoogleProvider.loadGeocoder(options);
const provider = new Provider(geoCoder);

Maybe there's a way to arrange this where we don't need the extra step? I'm not sure... but I think we want to be sure the script is loaded before trying to instantiate an object that would reference it.

Well anyway, this is getting closer... I'd like to hear how you feel about this. I realize a breaking change is not ideal (especially one that means that the Google provider has a different end-user implementation than the other providers), but I do think it's an important step to take.

@kstratis
Copy link
Collaborator

Hello and thank you for your hard work!
It is much appreciated!

Are there any updates on this one or perhaps a roadmap on when we should (vaguely) expect it to be merged?

@smeijer
Copy link
Owner
smeijer commented Oct 3, 2022

This has been released via #311, thanks for all the hard work and collaboration here. Much appreciated ❤️

@smeijer smeijer closed this as completed Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants