[Fixes #12253] Improvements to the proxy view #12254
Conversation
giohappy
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12254 +/- ##
==========================================
+ Coverage 64.07% 64.12% +0.04%
==========================================
Files 871 872 +1
Lines 52247 52306 +59
Branches 6487 6480 -7
==========================================
+ Hits 33477 33540 +63
- Misses 17274 17275 +1
+ Partials 1496 1491 -5 |
geonode/proxy/views.py
Outdated
| if url.hostname not in proxy_urls_registry.get_proxy_allowed_hosts(): | ||
| if not any(needle.lower() in url.query.lower() for needle in PROXY_ALLOWED_PARAMS_NEEDLES) and not any( | ||
| needle.lower() in url.path.lower() for needle in PROXY_ALLOWED_PATH_NEEDLES |
There was a problem hiding this comment.
@giohappy Does it make sense to allow hosts to be proxied, just because some paths or query parameters are allowed? Any unknown and perhaps bad URL (maybe from a user input) which contains an arbitrary needle (path or query does not matter here) would be allowed by the proxy. As an administrator, I would like to configure allowed hosts explicitly for which I know they should be allowed.
I see PROXY_ALLOWED_PARAMS_NEEDLES and PROXY_ALLOWED_PATH_NEEDLES are configurable, but these match for all hosts which are actually /not/ allowed.
IMO, servers which expose a Web API (e.g. OGC WMS) which is not CORS enabled (I guess, this is what this is all about) is not configured correctly. Not sure, if the operator wants all traffic to be routed through the GeoNode proxy. That would seem to me more as a quick and dirty workaround rather than a good productive configuration.
What do you think? Did I miss something?
There was a problem hiding this comment.
hi @ridoo I understand your concerns, and I agree that this solution is far from ideal.
This has been implemented to meet the logic that MS implements now. The client allows adding external layers to a map, and it implements the reverse logic compared to GeoNode: if the URLs are not listed inside a useCORS array (that GeoNode sets empty) they are automatically proxied. Basically it does what we're doing here.
If we do not support this, the catalog functionality that will be available since GeoNode 4.3.0 (thanks to a more extended integration with MS) won't work.
This is a functionality that many users wanted from MS, and this is why we are making this (temporary) move.
The final solution will be to make MS support the reverse logic, which is the one implemented by GeoNode until now: it should go straight to the origin server, unless stated differently (PROXY_ALLOWED_HOSTS).
BTW, the two settings (which will be set by the client) let administrators change this behavior.
There was a problem hiding this comment.
@ridoo The only alternative for now is to make the two settings an opt-in. However also the MS catalog plugin should be an opt-in in that case, otherwise, we will have a tool that will never work.
There was a problem hiding this comment.
Hey @giohappy, so this is more a (temporary) compatibility thing. Thanks for the background.
* Some improvements to the proxy view * Implemented proxy allowed hosts registry and signal handler for services * Moved signals inside proxy view * Moved proxy registry initialization to app setup stage * Initialize proxy registry only once Services have been configured * flake fixes * fixed typo * Avoid cycling the services inside the view * Restore reading from PROXY_ALLOWED_HOSTS * Simplify error message * do not cache hostnames passing needels validation * Reinit proxy registry when service is deleted * Fixed hotname passing needle test not being validated * avoid calling get_proxy_allowed_hosts again * A few more optimizations (cherry picked from commit 5e24974)
* Some improvements to the proxy view * Implemented proxy allowed hosts registry and signal handler for services * Moved signals inside proxy view * Moved proxy registry initialization to app setup stage * Initialize proxy registry only once Services have been configured * flake fixes * fixed typo * Avoid cycling the services inside the view * Restore reading from PROXY_ALLOWED_HOSTS * Simplify error message * do not cache hostnames passing needels validation * Reinit proxy registry when service is deleted * Fixed hotname passing needle test not being validated * avoid calling get_proxy_allowed_hosts again * A few more optimizations (cherry picked from commit 5e24974) Co-authored-by: Giovanni Allegri <giohappy@gmail.com>
Checklist
For all pull requests:
The following are required only for core and extension modules (they are welcomed, but not required, for contrib modules):
Submitting the PR does not require you to check all items, but by the time it gets merged, they should be either satisfied or inapplicable.