darrenklein commented 2 years ago

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.

darrenklein commented 2 years ago

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.

darrenklein commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 https://github.com/smeijer/leaflet-geosearch/issues/269

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

IsabellaS09 commented 1 year ago

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 commented 1 year ago

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

kstratis commented 1 year ago

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

darrenklein commented 1 year ago

@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.

kstratis commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

kstratis commented 1 year ago

@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?

darrenklein commented 1 year ago

@kstratis thank you very much for those dev environment instructions, I'm going to try those out a little later this evening. Really the main problem that I was having with this was that I couldn't seem to figure out how to get the different version of the dependencies to work correctly in my local environment. Actually, in the case of the app I was working on where I originally recognized this issue, this package is actually nestled a few deep - it's a Vue.js project, so this was like two or three deps down. I'll probably scratch that and try to just spin up a simple Express app or something to test this out with.

What do you think about the need to instantiate this provider differently than the others? Did this/these approaches work correctly?

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.

Or can you see a way that we might wrap this a little more neatly? I guess the challenge here is that it's an async operation, since the client needs to load a script from google and know to wait until that's loaded before proceeding...

kstratis commented 1 year ago

@darrenklein

@kstratis thank you very much for those dev environment instructions, I'm going to try those out a little later this evening.

Pleasure 🙂! Hope it helps!

What do you think about the need to instantiate this provider differently than the others? Did this/these approaches work correctly?

Yes. I've thought about it for a bit and I think that any attempt to hide the asynchronicity within the constructor will most likely backfire. Most people suggested passing the promise result to constructors directly. Otherwise everything has to become async and that can get messy very quickly. On that note I don't think it's a problem though. It's the Google provider afterall, it works a bit differently and if we document it correctly we should be fine.

What I'm concerned about is what you talked about earlier on about compatibility: I tend to agree with you on that. I think that we shouldn't offer backwards compatibility on this one because it's just plain wrong. As you correctly stated it is not recommended by Google since we are not supposed to use a backend-oriented API on the front plus the immense security risks. I mean, we could technically support it somehow but IMHO it's just plain wrong. With that said I've got no problem helping implement either. We just have take a decision. wdyt?

Tomorrow I'm gonna write the docs and try to clean this up a bit.

darrenklein commented 1 year ago

@kstratis Sorry for any delays on my part, been a bit busy (as I know you are as well!). Thanks to your excellent instructions, I was able to get this up and running in a simple local Leaflet project (took me a little while to remember how to set up an Express project that could handle this). I'm just going to experiment with it a bit, then see what I can do to help sketch out what we want to cover the docs.

Thank you so much for all of your excellent work here, you've really helped me out a lot - really glad we're able to collaborate on this and get it over the finish line!

kstratis commented 1 year ago

Apologies for the delay.

I took the liberty of restructuring this PR both to get rid of some bugs and improve its git history. Here's what I did:

Moved all library updates in a single separate commit.
Squashed @darrenklein's existing work (along with my fixes) in a single commit.
Rebased everything on top of latest origin/develop.
Fixed the docz examples so that they work with the new google geocoder approach.
Re-Introduced the legacy (existing) google provider and renamed it LegacyGoogleProvider so that people already using it may continue to do. Also updated the docz to advice against using it.
Added a note that the new google provider needs both the Geocoding API and the Maps JavaScript API enabled.

To test this out and be on the same page:

Delete any existing node_modules folder
-Force pull- the branch
Make sure you are using the latest v7 of npm (I used v7.24.2)
run npm ci to install node_modules without overwriting the package-lock.json

For some reason npm in any version other than v7 always overwrites package-lock.json (that includes v6 and v8). This is a separate issue though and needs to be handled in its own space.

Regardless, I think that this PR is now review ready. @darrenklein than you for the impeccable work and @smeijer for the invaluable support.

Please have a look and let's get this merged 🚀

smeijer commented 1 year ago

Thanks to both of you! This is great.

I'm just wondering. Till now, src/SearchControl.ts has always been free of provider specific code. It'd be nice if it's possible to keep it that way and move fetchProviderInstance into the Google provider.

Ignore me if it's too much work, or not even possible. I'm okay with merging this if/when you are. 🙂

Again, the time you've both put into this is much, much, appreciated. Thanks so much.

kstratis commented 1 year ago

Hi @smeijer, again, thanks for everything.

I thought a bit about your latest feedback and here's my thinking: src/SearchControl.ts is effectively user code: User code demonstrating how to use each provider from within the library's docz. When @darrenklein introduced the google async loader he pretty much asked the same question: Is there any way "to hide" the asynchronous loading behind a new constructor just like we do with the rest of the providers? I then looked it up quite a bit but IMHO embedding such kind of loading asynchronicity within a class can be the source of many bugs. Some folks at StackOverflow suggested that in cases like that it's best to resolve what you need to resolve (geocoder in our case) and just pass the object directly to your class constructor. So I suggested that this new loading model would be fine as long as it works as expected and is clearly documented.

Back to that task at hand, src/SearchControl is exactly that: Another piece of user code which should be treated in a similar way. Users have been instructed that GoogleProvider needs to load the geocoder object first and then instantiate its class with it. Which is exactly what we need to do too: I think that this -documented- difference in loading totally justifies placing fetchProviderInstance within src/SearchControl. After all, users usually only need a single provider and would rarely ever need to write code similar to fetchProviderInstance to handle multiple providers in a single spot.

IMHO the alternative would be to move fetchProviderInstance within AbstractProvider and create some sort of factory method which would conceal how each provider is loaded. ~The downside to this however would be that all caller code would stop working.~

wdyt?

kstratis commented 1 year ago

All this time we were trying to load the async geocoder object from within GoogleProvider's constructor. This caused a number of problems which we thought we could solve using one of the following approaches:

Either force the user to handle asynchronicity in his own code (which is what this PR did up to now).
Somehow hide it behind some sort of top-level-await (see providers.ts) like below:

async function loadGoogleProvider() {
  return await ProviderLoader.fetchInstance('GoogleProvider', {
    apiKey: process.env.GATSBY_GOOGLE_API_KEY as string,
  }).catch((e)=> console.error(e));
}

// Then load it just like the rest of the providers:
export default {
 ...
 Google: await loadGoogleProvider(),
 ...
}

This caused a number of problems on its own since:

It simply didn't work: Cannot use keyword 'await' outside an async function. Probably due to some babel/gatsby old version since it should be working on node > 14.3
Even if it did work, I think we would effectively just "shift" the "waiting" problem to node: All providers would have to wait googleProvider instance to load first and then export their own instances each and every time.

So, here's what I came up with: Instead of placing the asynchronicity in the class constructor, "hide it" in the -already asynchronous- search. This way we gain the following:

No user facing API changes
googleProvider becomes just another regular provider and can be handled in a similar way both in thes docs and the tests.
No changes in SearchResult.ts as requested.

On top of that I made sure that the geocoder object is immediately cached after initially fetched to avoid unnecessary network calls.

Doing so I've rewritten most of the history of this PR, so please have a fresh look and let me know what you think.

darrenklein commented 1 year ago

Wow, @kstratis, thank you for all of the amazing work you've done here - this is exactly the sort of thing I was hoping for when I asked for help on this PR. I think that wrapping the call to load the client JS script inside of the already async search function is a pretty intriguing solution - if I understand, this will wait to load the script until the first time a user uses the search functionality, is that correct? Hm, I think I like it! There is a part of me that wants to say that we should be explicit about the different steps to the procedure and give a developer the ability to work with both points of asynchronous behavior (loading the script and searching for an address)... but after reflecting on it for a bit, I'm not certain I feel like there's much value to be gained from that... so yeah, conceptually, I really like where you've taken this project I started.

Ultimately, I don't think my opinion here should be the ultimate deciding factor, I believe you and @smeijer should be the authorities on that. But I've really learned a lot from seeing what you've done here, and that's much appreciated!

kstratis commented 1 year ago

Thank you @darrenklein!

if I understand, this will wait to load the script until the first time a user uses the search functionality, is that correct?

Exactly. It will fetch it once the user starts typing and then cache it for all subsequent searches. All completely transparent.

I think this is our best shot at pushing the functionality we need all while keeping the changes at a minimum. Fingers crossed 🤞! @smeijer wdyt?

smeijer commented 1 year ago

We can load the script in the constructor of the provider, and only await the first search if the script did not yet finish loading (which in practice never happens). That way, we have the best of both.

I can give it a shot a bit later.

Edit, aaah, missed one message. I see you already moved some stuff around. Let me check in a bit. 🙂

smeijer commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

smeijer commented 1 year ago

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

allcontributors[bot] commented 1 year ago

@smeijer

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

smeijer commented 1 year ago

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

allcontributors[bot] commented 1 year ago

@smeijer

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

smeijer commented 1 year ago

:tada: This PR is included in version 3.7.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

smeijer commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@IsabellaS09 good news!!! ⬆️

smeijer / leaflet-geosearch

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

TODO -