lizmap-web-client icon indicating copy to clipboard operation
lizmap-web-client copied to clipboard

[Feature] Add QMS Google Maps Tiles

Open mind84 opened this issue 10 months ago • 4 comments

Since release 9.0, OpenLayers has introduced support form Google Maps Tiles.

This PR relies basically on the same logic of this one https://github.com/3liz/lizmap-web-client/pull/4150

If a valid Google Api Key is provided in the Lizmap plugin, then the application uses the OL dedicated class to get tiles from the Google endpoint and the API key is used to issue requests.

If no key is provided, then these tiles are treated as a generic WMTS external layer and the application get the tiles directly from the default URI set by QMS QGIS plugin. In this case it is not guaranteed that the tiles will work.

Maps supported:

  • Satellite
  • Roads

TEST NOTES

For now testing is manually. In the test repository there is a google_basemap.qgs project. You can configure the Lizmap plugin by adding your own Google Api Key and then check the functionality.

Funded by Faunalia

mind84 avatar Apr 10 '24 07:04 mind84

Can you rename your commit, like a similar title as your PR ? it will make the git history better.

Gustry avatar Apr 12 '24 09:04 Gustry

So it works if you use QMS AND you set a API key in the plugin right ? Sorry, I see you description, we will have to warn users about some corner cases. Honestly, for me using Google without an API key is discouraged as it's against Google TOS.

Yes, as BingMaps. If you don't specify any KEY in Lizmap plugin, then the basemap will be loaded as generic external layer.

Do you think this behavior should be prevented and we should raise an error in this case? IMHO it's up to the user to make this choice

Maybe it's done already ?

Yes, it is already done

Screenshot_20240415_120154

Google Maps TOS is quite restrictive and some aspect are rather unclear IMHO. For example, take this paragraph extracted from https://cloud.google.com/maps-platform/terms:

(e) No Use With Non-Google Maps. To avoid quality issues and/or brand confusion, Customer will not use the Google Maps Core Services with or near a non-Google Map in a Customer Application. For example, Customer will not (i) display or use Places content on a non-Google map, (ii) display Street View imagery and non-Google maps on the same screen, or (iii) link a Google Map to non-Google Maps content or a non-Google map.

I don't fully understand this point: there are a lot of cases (over 90% I guess?) where "Customer WILL USE the Google Maps Core Services with or near a non-Google Map" and they also paying to use these API. If this is not allowed, I'm wondering about the utility of integrate a Google Maps in any Costumer Application. My idea is that Google wants to avoid being associated to any improper use of their maps and to do so it has inserted a general clause and then possibly enforced it in case there were disputes of some kind. I don't think that Lizmap fits into this case.

mind84 avatar Apr 15 '24 10:04 mind84

Do you think this behavior should be prevented and we should raise an error in this case? IMHO it's up to the user to make this choice

Maybe a new employee in an organization is not aware about publication rule and push a project with a Google layer and/or the Lizmap system administrator (provider) want to be clear about TOS used on his own server.

Would it be possible to have a config in the ini file about this ?

In lizmap/var/config/profiles.ini.php.dist

[tosCheckExternalTiles:default]
;google=yes
;bing=no

Gustry avatar Apr 25 '24 13:04 Gustry

Do you think this behavior should be prevented and we should raise an error in this case? IMHO it's up to the user to make this choice

Maybe a new employee in an organization is not aware about publication rule and push a project with a Google layer and/or the Lizmap system administrator (provider) want to be clear about TOS used on his own server.

Would it be possible to have a config in the ini file about this ?

In lizmap/var/config/profiles.ini.php.dist

[tosCheckExternalTiles:default]
;google=yes
;bing=no

Ok for adding a configuration for this, but instead of storing it in the file profiles.ini.php, better use the dedicated Lizmap lizmapConfig.ini.php which is editable with the admin interface.

We should mark this option as sensitive, hidden when the flag hideSensitiveServicesProperties equals 1 (or True) https://github.com/3liz/lizmap-web-client/blob/master/tests/docker-conf/phpfpm/lizmapConfig.ini.php#L3

Editing can be done with https://github.com/3liz/lizmap-web-client/blob/master/lizmap/modules/admin/controllers/config.classic.php

mdouchin avatar Apr 25 '24 14:04 mdouchin

@mind84 can you add the changes requested by @Gustry and @mdouchin ?

rldhont avatar May 13 '24 13:05 rldhont

@mind84 can you add the changes requested by @Gustry and @mdouchin ?

Hi @rldhont!

I think I'll need some clarifications about these changes, probably next week I could start working on this.

Thanks

mind84 avatar May 14 '24 07:05 mind84

Thanks @mind84

Later, we will forward this new flag in the JSON metadata to make the plugin aware about these layers.

Gustry avatar May 14 '24 08:05 Gustry

Hi @Gustry !

For now I have added the additional configuration in the Lizmap admin panel in the existing Interface section. I chose this section because it seemed suitable for the purpose, but also because I don't want to add further sections.

Any suggestion about labes or options placement would be appreciated :)

@mdouchin they are also marked as sensitive, so when the corresponding flag is enabled, they do not appear in the config interface.

image

So, this will probably be a very silly question, but I want to be sure about the logic after this point. If, say, Google is disabled, then it should not appear as baselayer and should be removed from baselayers list? In this case I think the best way to do it is to remove it from the getProjectConfig service response.

Thanks!

mind84 avatar May 21 '24 11:05 mind84

Hi @Gustry @mdouchin !

Do you have any feedback on this?

If, say, Google is disabled, then it should not appear as baselayer and should be removed from baselayers list? In this case I think the best way to do it is to remove it from the getProjectConfig service response.

Thanks!

mind84 avatar May 30 '24 07:05 mind84

Sorry, I missed your question. I would say yes, to "remove the layer" on the server side, so the frontend won't be aware.

Later, we will be able to forward these values in the JSON metadata from the server : lizmap/modules/lizmap/lib/Server/Server.php

like

"authorized_external_providers" : ["google", "bing"],`

I can plan a dev on the Lizmap plugin side.

Gustry avatar May 30 '24 07:05 Gustry

If, say, Google is disabled, then it should not appear as baselayer and should be removed from baselayers list? In this case I think the best way to do it is to remove it from the getProjectConfig service response.

I think we should indeed remove the layer from the baselayer list. I a not sure though that removing it only from getProjectConfig would be enough, @rldhont & @nboisteault would know best.

Side question : what about if the layer is not in the baselayers group, but anywhere else ? Not sure this PR should tackle it, but we should think about it too.

mdouchin avatar May 31 '24 07:05 mdouchin

Hi @mdouchin, @rldhont, @nboisteault,

I'm working on it, but several questions arose during the process.

I think we should indeed remove the layer from the baselayer list. I a not sure though that removing it only from getProjectConfig would be enough

If I remove the layer from the config then Lizmap throws an error in console, eg: The WMS layer name Google_Satellite is unknown! so I think that the bet way to remove these layers from the baselayers is to remove (or rather, avoid to add) the externalAccess->url property in the config and add a property like 'disabled', and then do some checking via js:

"externalAccess": {
                "crs": "EPSG:3857",
                "format": "",
                "type": "xyz",
                "url": "",<----------------
                "zmax": "20",
                "zmin": "0",
                "disabled": true <-----------------------------------
            }

This way, also the url is hidden from the client (not a big achievement but it's better IHMO)

what about if the layer is not in the baselayers group, but anywhere else ?

Generally speaking, TOS has nothing to do with the Api Key Provide an API Key should be mandatory in the first place. So the new control on the admin is meant to allow external google and bing WMTS, indipendently from the API key.

This is my proposal (eg for Google, extended to Bing end so on):

Map Tiles Enabled Base Layers Lizmap Layers
enableGoogleMapsTiles = ON API Key IS SETTED
Load WMTS trough OpenLayers API
API Key IS NOT SETTED
avoid to display the layer in the baselayer and console.warn() that the api key is needed (avoid also to call the OL API if the the key is not provided)
There is no control on the API Key in this scenario, the layer is loaded by QGIS server so I think should be displayed according to the enableGoogleMapsTiles = on
enableGoogleMapsTiles = OFF No external google WMTS should be displayed, despite API Key

Please let me know if I completely misinterpreted the request.

Thanks

mind84 avatar May 31 '24 09:05 mind84

EDIT:

If I remove the layer from the config then Lizmap throws an error in console

This also happens when a layer is only visible to certain groups:

image

In console: image

To keep the same logic, probably the option to completely remove the layer from config is still valid

mind84 avatar May 31 '24 11:05 mind84

We should have a simple way to disable any group or layer to be displayed in the layer tree & the map. cc @rldhont @nboisteault We should keep it safe and simple.

mdouchin avatar May 31 '24 12:05 mdouchin

I don't think the logic was correct.

enableGoogleMapsTiles = OFF → No external google WMTS should be displayed, despite API Key

  • In the config INI file, does the server has a strict check of TOS for Google or Bing ? :

    • Yes, so any layer loaded with QGIS Desktop for Google or Bing must a have valid API key. If no API key provided, the layer is discarded from the project.
    • No, current behavior of Lizmap, nothing is checked :
      • If API key is set, well, all good, it works.
      • If not API key, maybe it works, maybe not. It depends of the provider, it seems #4192 Bing is not working. But I know Google works sometimes ... 🤷
    • Not set in Ini file, we will choose a default value, set to yes for instance.

But maybe, the first PR is to do the PHP config file lizmapConfig.ini.php and the forward in the JSON. Do you want me to have a look ?

Gustry avatar May 31 '24 12:05 Gustry

@mind84 We have discussed. We are fine with the current state of the PR, Javascript only. Can you wait a little ?

Gustry avatar May 31 '24 12:05 Gustry

We are fine with the current state of the PR, Javascript only. Can you wait a little ?

Of course, if you want I have also the updated PR on my local machine with the admin panel part, the metadata service updated (for lizmap plugin) and the controls on the the layers (remove layers when enableGoogleMapsTiles = OFF, for instance)

If could share it without push it, if you just want to take a look.

Let me know!

Thanks!

mind84 avatar May 31 '24 14:05 mind84

Hi @mind84

You can look at my PR https://github.com/3liz/qgis-lizmap-server-plugin/pull/80

This will discard any Google or Bing Layers from the GetCapibilities, so it will make your PR easier. That's why you can keep this PR with JavaScript only, because everything will be done upfront, in QGIS server. Sorry you started the form in Lizmap administration panel ! It looks more logical for me as well to have a UI to set up this configuration, that's I talked about this solution. But thinking about it, it's way easier that layers are discarded straight from QGIS server response.

Gustry avatar Jun 03 '24 09:06 Gustry

@Gustry , Ok thanks, looks reasonable to me too.

Side question:

You can look at my PR https://github.com/3liz/qgis-lizmap-server-plugin/pull/80

The overall logic is pretty simple, but I'm not very familiar with the Lizmap plugin server code. How can I enable/disable the ApiKey control?

Thanks

mind84 avatar Jun 03 '24 09:06 mind84

The overall logic is pretty simple, but I'm not very familiar with the Lizmap plugin server code. How can I enable/disable the ApiKey control?

I just pushed again on the PR. Can you have a look to the README.md explanation, and let me know if it's clear ? I would prefer to explain through the doc instead of here ;-)

We can merge if you want, so you would have a zip on https://packages.3liz.org/pub/lizmap_server-qgis-plugin/unstable/ or on custom QGIS repository https://packages.3liz.org/pub/server-plugins-repository/unstable/

Gustry avatar Jun 03 '24 09:06 Gustry

I just pushed again on the PR. Can you have a look to the README.md explanation, and let me know if it's clear ? I would prefer to explain through the doc instead of here ;-)

Ok! For me it's clear. Sys admin can set environmental variables (e.g. configured in docker compose or whatever) to enable/disable the Api Key check.

Side note on TOS: Providing the API key in the request does not guarantee compliance with the TOS. I only point this out because from the README it seems that, by switching the API Key control from False to True, the system becomes TOS compliance too.

We can merge if you want

No rush for me at all, can wait for an official stable release

Thanks!

mind84 avatar Jun 03 '24 11:06 mind84

Ok! For me it's clear. Sys admin can set environmental variables (e.g. configured in docker compose or whatever) to enable/disable the Api Key check.

Yes it's the purpose. I was wondering if just a single variable would be OK, because I don't think there is a use case to enable just one and the other one. But let's go like this.

I only point this out because from the README it seems that, by switching the API Key control from False to True, the system becomes TOS compliance too.

Yes true, I will see how I will rephrase on that. I will merge, so the ZIP will be available. Maybe after in the documentation docs.lizmap.com

Gustry avatar Jun 03 '24 13:06 Gustry

Backports ?

rldhont avatar Jun 12 '24 19:06 rldhont

Backports ?

For me, yes

Thanks!

mind84 avatar Jun 13 '24 06:06 mind84

Cool :) @mind84 Could you try with the latest version of the plugin and was it successful ?

Gustry avatar Jun 13 '24 09:06 Gustry

Could you try with the latest version of the plugin and was it successful ?

yep, I'll keep you updated

mind84 avatar Jun 13 '24 09:06 mind84

@Gustry @rldhont,

Server side seems all good to me. I've added both env variables on the docker-compose: image

and system responds as documented.

Problem is, when the tos_check removes all layers from the getCapabilities response, the baseLayers group ends up being empty, and then the client crashes:

image

Not a big deal I think, I can fix it but it will probably take some time.

I'll keep you updated

Thanks

mind84 avatar Jun 13 '24 10:06 mind84

Not a big deal I think, I can fix it but it will probably take some time.

Nice thanks. Indeed, at least skipping "nicely" the loop if it's empty would be better.

Because in your case, you fall back on https://github.com/3liz/lizmap-web-client/issues/4029 which was "fixed" with a fix on the plugin side https://github.com/3liz/lizmap-plugin/issues/540 but I didn't expect the baselayers group to become empty when layers are discarded on runtime.

Thanks for this patch anyway !

Gustry avatar Jun 13 '24 12:06 Gustry

What do you think about adding a very basic test loading google_basemap.qgs and just testing no error is raised

Yes I could add a basic test, of course it will not be capable to test the whole functionality but its better than nothing

Thanks

mind84 avatar Jun 13 '24 12:06 mind84

Yes I could add a basic test, of course it will not be capable to test the whole functionality but its better than nothing

No sure, we do not want to test the OpenLayers Google class :)

Just that the map load correctly, and there isn't any JS error when loading the map. Maybe similar to what you have done for Bing https://github.com/3liz/lizmap-web-client/blob/master/tests/end2end/playwright/bing-basemap.spec.js

Gustry avatar Jun 13 '24 12:06 Gustry