feat: use google maps api to support secure requests #311

darrenklein · 2022-01-15T03:12:23Z

This is still a work-in-progress, but I believe that it generally fixes the issue outlined in #269.

In brief summary -

A Google Maps API key provides a user with two means for geocoding address and location data -

A query can be sent to the endpoint https://maps.googleapis.com/maps/api/geocode/json, which makes use of Google's Geocoding API. However, Google recommends that this endpoint should only be used for geocoding known addresses, rather than dynamic input like a user-facing address geocoder - see https://developers.google.com/maps/documentation/geocoding/overview. Furthermore (and of particular concern to me), this service cannot be protected by a website referrer restriction - so it can't be limited to requests from a particular set of domain names (you can enforce other restrictions, though).
A query can be sent via the Google Maps JS API, which is the preferred way of making dynamic geocoding requests from a web client. An additional big plus here is that this API does allow a key owner to restrict their key to a specific set of website referrers, so an API key can be protected from abuse.

This PR upgrades this repo by shifting Google Maps geocoding support from the first method to the second.

In order to use this service, the owner of the Google Maps API key needs to make sure their key is allowed to make use of both the Maps JavaScript API and Geocoding API (typically, a new API key is unrestricted by default and would be able to access these services and more.

There's probably a lot left to do before this is merge-ready, but here are a few things off the top of my head

TODO -

The GoogleProvider class no longer needs to implement an endpoint method, but I'm not sure how to remove it without TypeScript complaining
Although I can confirm that the data returned by the GoogleProvider's search method looks to me to be in the right format when compared to other providers, I haven't yet been able to successfully test it in the context of an app. When trying to use a local copy of this repo, I get errors from the addMarker and centerMap methods - that's not specifically a symptom of this branch, it happens if I use a local copy of the main repo as well (and with a different provider). I haven't yet been able to figure out why that would be.
Update/add documentation

I would gladly welcome any help and feedback needed to get this PR in ship-shape!

Many thanks to @twillard22 for helping me crack some TS errors!

Edit - just documenting this again here to be thorough. The work I've done in this PR needs to be instantiated by a user via something like

const options = {
  params: {
    key: GOOGLE_MAPS_API_KEY,
    region: "us"
  }
};

GoogleProvider.loadGeocoder(options)
  .then((geoCoder) => {
    provider = new GoogleProvider(geoCoder);
})

// or
// const geoCoder = await GoogleProvider.loadGeocoder(options);
// const provider = new GoogleProvider(geoCoder);
// etc.

package.json

src/providers/googleProvider.ts

darrenklein · 2022-02-13T01:02:53Z

This PR would introduce a breaking change for users who use the Google provider, since it requires that the Google JS API needs to be loaded into the DOM before the provider object can be instantiated. The recommended implementation would be 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 GoogleProvider(geoCoder);

Unfortunately, that means that accepting this PR would be a major version bump. Of course, I'm open to other suggestions of how to handle this scenario.

src/providers/googleProvider.ts

darrenklein · 2022-03-29T01:53:46Z

@smeijer just a friendly "ping" to see what you think about this approach. I don't mean to be a bother - I sincerely appreciate all of your hard work on this repo and want to be respectful of your time - but I do think that this is an important fix, as it will help users of the Google provider to protect their API keys. It's not quite ready for merge, but I think it's close, I could use your perspective on it. I would be happy to arrange a pairing session with you to talk this through, I'm sure we could knock it out together pretty quickly.

darrenklein · 2022-06-08T14:01:05Z

@smeijer thank you so much for continuing to maintain this project! I'm still really interested in getting the work in this PR merged - is it something you think you'll be interested in? I'm sorry that I don't quite have the confidence as a frontend dev to "finish" it myself, it would be great if we had the chance to collaborate on it. I think it's close, I just can't quite test it correctly, and I'd like the guidance of someone who's a more experienced frontend dev.

smeijer · 2022-06-08T20:01:37Z

Hi! Sorry I've let you hanging like that. I've fixed the docz which also serve as development environment, and rebased your branch upon that.

I'll have to continue the work later, as the Google Provider currently doesn't run for me. I think it's just the current state of this pull, but I need to take another look.

When I run npm run start, and navigate to the Google Provider, this is the error I get:

Unhandled Rejection (TypeError): e.geoCoder.geocode is not a function

src/providers/googleProvider.ts:86
  83 | async search(
  84 |   options: SearchArgument,
  85 | ): Promise<SearchResult<google.maps.GeocoderResult>[]> {
> 86 |   const response = await this.geoCoder
  87 |     .geocode(
  88 |       {
  89 |         // TODO (maybe?) - a user can specify a region when loading the Google Maps JS API,

darrenklein · 2022-06-08T21:22:24Z

@smeijer No worries! I am sure that you're very busy, and I sincerely appreciate all of the time and expertise you've dedicated to this project.

In regards to the error you're getting, I believe that this is because of the way the GoogleProvider class gets instantiated by the client - you first need to call the class' loadGeocoder static method to actually load the client script from Google, and then you can instantiate the object - so something like

const options = {
  params: {
    key: GOOGLE_MAPS_API_KEY,
    region: "us"
  }
};

GoogleProvider.loadGeocoder(options)
  .then((geoCoder) => {
    provider = new GoogleProvider(geoCoder);
})

// or
// const geoCoder = await GoogleProvider.loadGeocoder(options);
// const provider = new GoogleProvider(geoCoder);
// etc.

This way, it ensures that the script from Google is loaded before the user could potentially call one of its methods. My apologies for not documenting that yet, I didn't want to go too far down this rabbit hole in case you had a different way that you would want to handle this. Maybe there's a more elegant solution? I realize that the way I've set it up now creates a breaking change for current GoogleProvider users, but again, I think it's worthwhile, so as to make use of "correct" API and to allow users to restrict the use of their keys - I wrote a bit about that in some of the comments in the thread #269

I'm curious to hear your thoughts on this, let me know what I can do to help this along.

IsabellaS09 · 2022-07-13T02:42:07Z

Hey @darrenklein, since this would be a breaking change for current users of Google Provider, you could probably just treat this as an entirely new provider GoogleClientFacingProvider or something similar. I'm hoping to get some more time to look at this PR this week.

darrenklein · 2022-07-14T01:15:31Z

That's an interesting idea - but I'd really like the repo owner's guidance on the best way forward with this.

kstratis · 2022-09-19T16:19:01Z

I'd love to see this one going through @darrenklein !

darrenklein · 2022-09-19T16:51:42Z

@kstratis If what I've got here is roughly the right way to go, then yes, I'd love to see the work get finished and merged as well - but I think we need input from the repo owner to really finish this properly, as this would most definitely be a significant breaking change for current Google provider users (it will not only require code changes, but also possibly changes to Google account settings, all of which will need to be documented) - I've taken this as far as I feel comfortable without his guidance. If this is important to you, please consider pinging the repo owner.

darrenklein · 2022-09-19T16:52:56Z

src/providers/__tests__/googleResponse.json

@@ -70,7 +70,6 @@
        "amusement_park",
        "establishment",
        "point_of_interest",
-        "premise",


I believe that this value is no longer a possible option in the Google response, should double-check though.

kstratis · 2022-09-19T20:23:52Z

This one is a long time coming and -imho- truly important since (without it) Google provider is effectively broken.

@smeijer What's actually left to do here to move this forward?
I can chip in and help but you have to provide a clear way forward 🙏

smeijer · 2022-09-19T20:31:23Z

@darrenklein, @kstratis I've added you both as collaborators on this project, so you'll be able to create branches here, and merge pulls - like this one - into main without me. Please take this forward as you seem fit.

I'll add a github action here later this week, so that any merged pull request also automatically gets published to npm.

smeijer · 2022-09-19T20:37:50Z

Regarding the breaking change, there are ways to make the new provider work in the old way, unless a secure option is provided. But do we want that? Do we wish to keep offering insecure services as default?

The alternative would be to rename the current provider to InsecureGoogleProvider to make it clear that it's not secure? Or LegacyGoogleProvider if you want to make it sound more friendly.

What are your thoughts? I'm totally cool with having a breaking change for the sake of security.

darrenklein · 2022-09-19T20:38:25Z

@smeijer thank you very much for the opportunity - if we can get this finished, I do hope that you'll give it a look over and weigh in with any thoughts you have. @kstratis if you are interested in working on this, I would really appreciate the opportunity to collaborate. I'm not a super-strong JS/TS developer, and it's also been a few months since I really looked at this, so I'm a little rusty on what I was doing here. If you feel up to it, that would really help a lot - I see you're in Greece, and I'm in northeastern US - maybe we could find some time to pair?

kstratis · 2022-09-20T17:24:55Z

Thank you @smeijer !

Regarding the breaking change, there are ways to make the new provider work in the old way, unless a secure option is provided. But do we want that? Do we wish to keep offering insecure services as default?

Could you plz elaborate? What do you mean? What's this option about?

@darrenklein Let's push this over the finish line.
I'm not sure if we'll be able to pair due to vastly different timezones (and immense workload on my end) but we should definitely somehow sync on this one.
My twitter handle is @k_stratis so give me a ping and we can pick it up from there.

In the next few days I'll be looking at the code and try to complete your todo list.
Please mind that near the end this entire PR may need a restructuring of commits.

smeijer · 2022-09-20T18:15:20Z

Feel free to ping me (@meijer_s) there as well, I'm much more responsive on Twitter than on github, due to the overflow in notifications.

Regarding backwards compatibility, I was thinking in the line of renaming the current provider to LegacyGoogleProvider, and making the new provider proxy requests to that one if a legacy: true option is given.

I'm not sure about naming it legacy or insecure, that depends on how much we want to advise against using it I guess.

darrenklein · 2022-09-20T19:15:49Z

Thanks all - unfortunately, I am not a Twitter user... but ok don't worry, we'll figure out a way to communicate.

Do we feel like maintaining backwards compatibility is the right idea? In most circumstances, I'd say yes, but this is a bit of a unique situation. The issue is a little deeper than just the security aspect - it's also that Google advises against using the current endpoint for client-originating requests (if I recall, I don't think they specifically say why that's the case - but they indicate that the current endpoint is for fully-formed server-originating requests, as opposed to dynamic client-based requests). To be clear, I've personally never run into any problems with the current configuration of the Google provider (aside from the security aspect), but I haven't used it a lot, so I don't know, maybe it can be problematic on a high-traffic application.

src/providers/googleProvider.ts

kstratis · 2022-09-21T17:06:59Z

@darrenklein I pushed some commits earlier on and addressed a couple of minor issues and comments.
Now, regarding the todos you mention:

The GoogleProvider class no longer needs to implement an endpoint method, but I'm not sure how to remove it without TypeScript complaining

I already mentioned in my comment earlier that since a method is declared as abstract there aren't really any escape hatches for children classes. However it shouldn't be a problem: I've seen this in other repos and is usually handled by throwing to prevent errors further down the line. I pushed out a commit that does just that. IMHO, we're good with that.

Although I can confirm that the data returned by the GoogleProvider's search method looks to me to be in the right format when compared to other providers, I haven't yet been able to successfully test it in the context of an app. When trying to use a local copy of this repo, I get errors from the addMarker and centerMap methods - that's not specifically a symptom of this branch, it happens if I use a local copy of the main repo as well (and with a different provider). I haven't yet been able to figure out why that would be.

I've managed to test it against my app and works wonderfully. Here's how I did it:

git fetch --all
git checkout google-provider-fix
git reset --hard origin/google-provider-fix
npm install
npm run build

Follow the steps above and once you get a clean working directory do this:

npm pack --pack-destination ~/Downloads

This way you'll effectively be creating a tgz file under your ~/Downloads dir: leaflet-geosearch-3.6.1.tgz

Now switch to your own app, open the package.json and replace the leaflet-geosearch entry with this:

"leaflet-geosearch": "file:/Users/USERNAME/Downloads/leaflet-geosearch-3.6.1.tgz"

run npm install one last time inside the dir of your app and you're good to go. I got no errors at all.

Update/add documentation

I'll update the docs. Could you plz have a look at the code again? Are the tests good to go?

src/providers/googleProvider.ts

smeijer · 2022-10-03T18:00:53Z

I'm currently trying to fix the github action, which keeps complaining about the package-lock, but in the meanwhile. What do you think of the changes I've pushed?

Specifically these three: https://github.com/smeijer/leaflet-geosearch/pull/311/files/d58c3634d632fe8fa002c2a28dcb827cd2d9ac6f..9276070cc5a7de441707ef26bae3e0c9e37d0f73

The GoogleMaps script is now loaded via the constructor, and the search action only waits if the loading isn't done at the moment the user starts typing. That way, the first search is way faster.

I've added the typeof window !== 'undefined' check so the fetching in the constructor won't throw errors on server environments - such as our docs.

kstratis · 2022-10-03T18:16:10Z

src/providers/googleProvider.ts

-          return { results: response };
-        },
-      )
+    const geocoder = this.geocoder || (await this.loader);


Nice! Very clever!

kstratis · 2022-10-03T18:30:55Z

I'm currently trying to fix the github action, which keeps complaining about the package-lock

🚀🚀🚀

What do you think of the changes I've pushed?

Love where you've taken this. To be honest I was afraid that this might blow up but to my surprise it went through! Looks like the instance returns immediately and the async call carries on uninterrupted until called in search! Very smart! 👏

I've added the typeof window !== 'undefined' check so the fetching in the constructor won't throw errors on server environments - such as our docs.

This thing caused me a few nightmares a couple days ago. Super glad to know that you guard against it!

Overall, I absolutely love the direction this thing took! I definitely picked up a few things on the way!
@smeijer Please let me know if there's anything else I can do to help 🙏

@darrenklein Seems like the separation you talked about here .... is happening! 🙌

smeijer · 2022-10-03T19:56:28Z

Looks like the instance returns immediately and the async call carries on uninterrupted until called in search!

That's exactly what's happening. The async call isn't "awaited", so it just runs in the background. By the time the user has focussed the input and pressed a key to search, the promise has already resolved, and the callback in the then was invoked (which sets this.geocoder). So the chance of that one await this.loader (which holds the promise) is ever invoked, is really slim. But just in case someone is that fast, or the network is that slow, we've covered it.

smeijer · 2022-10-03T19:57:19Z

While I was writing that message, all flags turned green. Let's go! 🚀

smeijer · 2022-10-03T20:01:51Z

@all-contributors please add @darrenklein for code, test, doc

allcontributors · 2022-10-03T20:02:00Z

@smeijer

I've put up a pull request to add @darrenklein! 🎉

smeijer · 2022-10-03T20:02:11Z

@all-contributors please add @kstratis for code, test, doc

allcontributors · 2022-10-03T20:02:19Z

@smeijer

I've put up a pull request to add @kstratis! 🎉

smeijer · 2022-10-03T21:15:14Z

🎉 This PR is included in version 3.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

smeijer · 2022-10-03T21:20:52Z

I can't thank you two enough for what you've done here. This change would not have landed if it weren't for your endless time, effort and patience. Thank you so much! Much, much appreciated.

darrenklein · 2022-10-03T23:06:26Z

@smeijer Thank you so much for the opportunity to contribute to this project. I really appreciate all of your hard work over the years, it is an honor to be a part of this! And, of course, thank you very much for your thoughts and contributions on this piece of work.

@kstratis Could not have done this without you! Your expertise and hard work were exactly what was needed here, no way was I ever going to figure all of that out! It's been a pleasure working with you on this, hopefully again some day (both of you)!

darrenklein · 2022-10-03T23:07:51Z

@IsabellaS09 good news!!! ⬆️

darrenklein mentioned this pull request Jan 15, 2022

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

Closed

darrenklein commented Jan 17, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

darrenklein commented Jan 17, 2022

View reviewed changes

src/providers/googleProvider.ts Outdated Show resolved Hide resolved

darrenklein commented Feb 13, 2022

View reviewed changes

src/providers/googleProvider.ts Outdated Show resolved Hide resolved

darrenklein commented Feb 13, 2022

View reviewed changes

src/providers/googleProvider.ts Outdated Show resolved Hide resolved

smeijer force-pushed the google-provider-fix branch from ce19809 to 312a771 Compare June 8, 2022 19:57

darrenklein mentioned this pull request Jul 11, 2022

Encrypted/Proxied Provider #332

Open

darrenklein commented Sep 19, 2022

View reviewed changes

kstratis reviewed Sep 21, 2022

View reviewed changes

src/providers/googleProvider.ts Show resolved Hide resolved

kstratis force-pushed the google-provider-fix branch from d0df903 to 2a6bebf Compare September 21, 2022 16:48

kstratis reviewed Sep 21, 2022

View reviewed changes

src/providers/googleProvider.ts Outdated Show resolved Hide resolved

kstratis force-pushed the google-provider-fix branch 2 times, most recently from 4069c70 to e93114b Compare September 21, 2022 19:31

smeijer added 2 commits October 3, 2022 19:49

ci: bump node version to 16.x

f80c1a2

chore: fix package-lock

4ccbfe1

smeijer force-pushed the google-provider-fix branch from d56fd44 to 4ccbfe1 Compare October 3, 2022 17:55

kstratis reviewed Oct 3, 2022

View reviewed changes

smeijer force-pushed the develop branch 3 times, most recently from 7618154 to 6453ded Compare October 3, 2022 19:26

Merge branch 'develop' into pr/311

dbdad1c

smeijer force-pushed the google-provider-fix branch from cbccd3d to dbdad1c Compare October 3, 2022 19:43

smeijer added 2 commits October 3, 2022 21:49

chore: fix lint errors

a71ec4f

Merge branch 'develop' into pr/311

e28cc28

smeijer changed the title ~~Google provider fix~~ feat: use google maps api to support secure requests Oct 3, 2022

smeijer merged commit 50953a6 into smeijer:develop Oct 3, 2022

allcontributors bot mentioned this pull request Oct 3, 2022

docs: add @darrenklein as a contributor for code, test, and doc #342

Merged

allcontributors bot mentioned this pull request Oct 3, 2022

docs: add @kstratis as a contributor for code, test, and doc #343

Merged

smeijer added the released on @develop label Oct 3, 2022

darrenklein deleted the google-provider-fix branch October 3, 2022 23:07

Marius-Romanus mentioned this pull request Oct 5, 2022

Error google compile #349

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use google maps api to support secure requests #311

feat: use google maps api to support secure requests #311

darrenklein commented Jan 15, 2022 •

edited

Loading

darrenklein commented Feb 13, 2022 •

edited

Loading

darrenklein commented Mar 29, 2022

darrenklein commented Jun 8, 2022

smeijer commented Jun 8, 2022

darrenklein commented Jun 8, 2022

IsabellaS09 commented Jul 13, 2022

darrenklein commented Jul 14, 2022

kstratis commented Sep 19, 2022

darrenklein commented Sep 19, 2022

darrenklein Sep 19, 2022

kstratis commented Sep 19, 2022

smeijer commented Sep 19, 2022

smeijer commented Sep 19, 2022

darrenklein commented Sep 19, 2022

kstratis commented Sep 20, 2022 •

edited

Loading

smeijer commented Sep 20, 2022

darrenklein commented Sep 20, 2022

kstratis commented Sep 21, 2022 •

edited

Loading

smeijer commented Oct 3, 2022 •

edited

Loading

kstratis Oct 3, 2022

kstratis commented Oct 3, 2022

smeijer commented Oct 3, 2022

smeijer commented Oct 3, 2022

smeijer commented Oct 3, 2022

allcontributors bot commented Oct 3, 2022

smeijer commented Oct 3, 2022

allcontributors bot commented Oct 3, 2022

smeijer commented Oct 3, 2022

smeijer commented Oct 3, 2022

darrenklein commented Oct 3, 2022

darrenklein commented Oct 3, 2022

feat: use google maps api to support secure requests #311

feat: use google maps api to support secure requests #311

Conversation

darrenklein commented Jan 15, 2022 • edited Loading

TODO -

darrenklein commented Feb 13, 2022 • edited Loading

darrenklein commented Mar 29, 2022

darrenklein commented Jun 8, 2022

smeijer commented Jun 8, 2022

darrenklein commented Jun 8, 2022

IsabellaS09 commented Jul 13, 2022

darrenklein commented Jul 14, 2022

kstratis commented Sep 19, 2022

darrenklein commented Sep 19, 2022

darrenklein Sep 19, 2022

Choose a reason for hiding this comment

kstratis commented Sep 19, 2022

smeijer commented Sep 19, 2022

smeijer commented Sep 19, 2022

darrenklein commented Sep 19, 2022

kstratis commented Sep 20, 2022 • edited Loading

smeijer commented Sep 20, 2022

darrenklein commented Sep 20, 2022

kstratis commented Sep 21, 2022 • edited Loading

smeijer commented Oct 3, 2022 • edited Loading

kstratis Oct 3, 2022

Choose a reason for hiding this comment

kstratis commented Oct 3, 2022

smeijer commented Oct 3, 2022

smeijer commented Oct 3, 2022

smeijer commented Oct 3, 2022

allcontributors bot commented Oct 3, 2022

smeijer commented Oct 3, 2022

allcontributors bot commented Oct 3, 2022

smeijer commented Oct 3, 2022

smeijer commented Oct 3, 2022

darrenklein commented Oct 3, 2022

darrenklein commented Oct 3, 2022

darrenklein commented Jan 15, 2022 •

edited

Loading

darrenklein commented Feb 13, 2022 •

edited

Loading

kstratis commented Sep 20, 2022 •

edited

Loading

kstratis commented Sep 21, 2022 •

edited

Loading

smeijer commented Oct 3, 2022 •

edited

Loading