Use the backend to perform routing API requests #167
Conversation
aszlig
force-pushed
the
proxy-routing
branch
2 times, most recently
from
e648267 to
39d41a0
Compare
aszlig
force-pushed
the
proxy-routing
branch
from
39d41a0 to
9124dfa
Compare
This is useful for Nextcloud Maps, so that people are able to host their own routing backend. Unfortunately, running OSRM with a very large dataset requires a LOT of memory, so I'm not enabling this by default if the app is enabled. The implementation patches Nextcloud Maps to use Unix domain sockets to access the OSRM backend, so that we can properly sandbox the service and don't allow any outgoing connections. Right now, I've included a few datasets, namely "planet", "europe" and "germany" for now, because they're the most useful ones at least in my deployments. The long-term goal however is to implement updating and gathering of these datasets into our updater. One thing however that is always enabled whenever the "maps" app is enabled, is the proxy-routing patch, which makes sure that routing is never sent via the user's browser but proxied through Nextcloud Server. The patch is basically a rebase of the one I submitted upstream at nextcloud/maps#167 against Maps version 0.1.2. Signed-off-by: aszlig <[email protected]>
|
@nextcloud/maps anyone with more technical knowledge than me is welcome to do a review of this. :) @aszlig thanks a lot for your contribution and sorry for the wait! |
|
@jancborchardt: No need to be sorry for anything, got PRs that took years to integrate :-) Besides, I'm also a newbie if it comes to the Nextcloud API. |
|
@thomas-mc-work @Galbar since @tacruc @eneiluj are a bit busy atm, and you opened pull requests for Maps too (thanks! :) it would be awesome if you could review this too. Thanks! 🎉 |
There was a problem hiding this comment.
Wow thanks a lot for this amazing PR. I agree API keys shouldn't be exposed to users (specially as it's an admin setting) so it makes total sense to only use them on the server side.
I like the fact that requests are all made by the server to mask client IPs but:
- what if a lot of users perform a lot of routing requests? I mean performance-wise
- even if there are a small amount of routing requests, each request is going to be a bit slower/longer because it's now done in two steps, browser => backend, backend => routing server.
Maybe those two remarks don't make much sense because routing requests are very small and the performance loss will almost never be felt.
I made another comment in the code.
Well, to conclude 😉, let me try this out and play with it, let's wait for @tacruc feedback and from someone with deeper knowledge of how such things are generally done in Nextcloud (@jancborchardt any idea?).
I'll be glad to approve this in (much) less than a year 😁.
I haven't made any large-scale benchmarks but I did deploy this on a range of Nextcloud instances already, which only have a small amount of users (~50-70). For larger installations, we can still add caching if this really turns out to be a performance bottleneck, because as you mentioned, those requests are usually very infrequent and the requests themselves are pretty small (the replies could be a bit longer, but only if you try to walk to Australia from Germany for example).
If you look at an app like
🎉 |
|
Another app ( |
I would say @rullzer, and since @aszlig mentioned the Calendar app too, @georgehrke. :) |
|
Please don't take example in the ProxyController in the calendar app. 🙈 Just some quick remarks about this PR (not necessarily complete)
|
I didn't use it as an example for writing the code here, but was looking for other examples doing it in a similar way after @eneiluj's comment.
Good point, will implement this.
Because I didn't yet dig into Leaflet enough to pass the token along. Right now the attack scenario for this is that an attacker could exhaust API limits for routing services, eg. by directing logged-in user's browser to mass-visit something like |
|
Hey, there will be a conflict between this PR and #187 because I get the Mapbox API key from option values to create Mapbox tile layers in the UI. I don't see how we can easily use the backend to perform tile request... And most importantly I don't know if it would be a good idea to have all the tile-related traffic through the server, performance-wise. So I guess the conflict is quite big: #187 "forces" us to expose the Mapbox API key in the UI code. Sorry about that (but vector tiles are awesome):smile:. @aszlig What do you think? |
Ideally, all the usage of jQuery If you use that package, it will automatically include the request token necessary to pass the CSRF check in every request. |
|
@eneiluj: I think proxying (and maybe caching?) Mapbox tiles is something we could do as part of another PR, because after all this PR is about routing. Unfortunately, I did not manage to work on this during the weekend, but I'll try resume (and rebase) on this later this week. |
aszlig
force-pushed
the
proxy-routing
branch
from
0f508fb to
c68ad9c
Compare
|
Since I didn't get to work on that since quite a while, I at least rebased this branch against current master. What's still missing is rate limiting and the removal of |
Upstream changes:
* Don't display routing API warning message if routing engine is set
* Fix postWrite hook interfering with creation of new user
* Fix display of tracks not working
* Improve initial media scan
* Fix context menu routing actions
* Disable contact tooltip when popup is open
* Feplace lock with lock check in filehooks
* Warn user when importing favorite file with tracks/routes
* Fix old migration script to avoid crash when installing on MySQL
instances
* Fix Add Device Point error when optional value is empty
* Fix popup shift when avatar appears late
* Fix sizing of contacts with profile image
* Add Mapbox vector tiles support
* Add 'export current route' button in routing area
* Show OSRM-demo router only if there is no other router
* Add unicode characters as icons for routing engines
* Replace mapbox satellite by satellite-streets
* Make map default layers system generic
* Fix map left click after interacting with layers button
* Fix locatecontrol staying enabled after error leading to error
message on each page load
* Fix left and right click favorite popup conflict
* Align contact tooltip and popup with marker
* Unify contact tooltip and popup margins
* Add compatibility for Nextcloud 18 and 19
* Fix bug in names making dark and topographic inaccessible
* Open Image Clusters with the viewer app
* Disable Mapbox-gl-js if webgl is not working
* Complete refactor of safeAddStuf -> addStuf in photosControler
* Move the scan of each individual picture into a background job to
make it more crash resistent
* Fix Contact photos being stretched and squeezed when non-square
* Use standalone Viewer app when possible
* If no standalone Viewer is available, choose between a link to
gallery or photos app
* Fix time filter for photos
* Switch to Webpack for generating JS files
* Improve UX for Routing Setup
* Improve design of Contacts tooltip and popup
* Add proper feature policy
* Clean up maps icon
* Add documentation foc OCC
* Add API documentation
* Update translations
On our side, I've rebased the proxy-routing patch[1] against the new
upstream release and also added another patch, which updates the
generated JS and map file in accordance to that patch.
This is considered to be a workaround, because ideally we'd like to
build the maps app (and possibly *every* app) from source, but right now
doing so would introduce a huge churn of node2nix output to the
repository plus we'd also make sure we fetch the app from the GitHub
repository in *addition* to the release tarball if we want to avoid to
have another dump of composer2nix.
Since I intend to someday[TM] have that patch included upstream, I think
it's better for now to use a diff of the generated file instead.
Additionally, I rebased the unix-sockets.patch, which now also got a bit
smaller, since we new use Guzzle instead of our own implementation based
on cURL.
[1]: nextcloud/maps#167
Signed-off-by: aszlig <[email protected]>
We now no longer expose the service URLs and API keys via the /getOptionsValues route and only provide whether these services are configured in the backend or not. This is in preparation for the coming up proxying handlers, which are going to send API requests via the backend rather than direct the user's browser to do so. Signed-off-by: aszlig <[email protected]>
This adds three handlers for OSRM, GraphHopper and Mapbox, which essentially proxy the requests made to the routing APIs and inject the required API keys if needed. The reason for doing this is to offer protection for both the user and administrator, because first of all, external services can then no longer track usage down to individual users but essentially get requests from one source only. Second, the administrator no longer has his/her API keys exposed to all of the users. Note that this change only adds the routes and handlers for proxying and it's not yet integrated into the frontend. Unfortunately, when passing along and injecting API keys into the query string we need to parse query string arguments by ourselves because GraphHopper uses the "point" query string argument multiple times and PHP by default treats them as unique keys. Signed-off-by: aszlig <[email protected]>
This switches the main JavaScript file to use our shiny new proxying handlers instead of directly requesting the API via the user's browser. Since the API keys are solely handled by the backend, we now just pass null for Mapbox and GraphHopper instead of the API key, because it gets replaced before the proxy handler submits the request. In addition to the optionsValues we had so far, we now also have a new availableFeatures object, which only contains the settings necessary to know whether a service is available or not rather than its details, like eg. API keys and URLs. Signed-off-by: aszlig <[email protected]>
Requests to routing are no longer directly issued against the individual services but proxied via Nextcloud, so we no longer need to allow these domains in the content security policy. Signed-off-by: aszlig <[email protected]>
When looking for other uses of HTTP client in the Nextcloud server, I found a few usages of cURL and thought this would actually be the best practice. However, it turns out that there is actually an HTTP client implementation based on Guzzle, so we can completely throw away our custom ProxyResponse. Additionally, I also checked the response format of the routing APIs we use and all of them are using JSON, so instead of using something like DataDisplayResponse (which adds a Content-Disposition header) or yet again implement our custom Response, I just changed this to use JSONResponse instead. What I kept however is the setting of the User-Agent header, because some of the API providers tend to have a policy that require you to properly set it, for example for the OSRM demo server their policy[1] states: > General rules > > ... > > * Valid User-Agent identifying application. Faking another app's > User-Agent WILL get you blocked. [1]: https://github.com/Project-OSRM/osrm-backend/wiki/Api-usage-policy Signed-off-by: aszlig <[email protected]>
This was a suggestion from @eneiluj: > Did you call it "features" because you think ahead and predict it can > be used for other types of features? I wonder if it could be named > more explicitly like "routingProviders" and leave other types of > features in another key to make it clearer. I mean "routing" is a > feature but "mapbox" is not... I originally used "features", because it could be used to expose more than just routing service information. In hindsight however it's a better idea to call it "routingProviders" (as suggested), so it's more clear when you read the code. Should we add something like map tile services, we can still add another variable, eg. "tileProviders". Signed-off-by: aszlig <[email protected]>
Since there are quite a lot of changes to be listed, here are the
highlights from the upstream blog post[1]:
The three biggest features we introduce are:
* Our new dashboard provides a great starting point for the day with
over a dozen widgets ranging from Twitter and Github to Moodle and
Zammad already available
* Search was unified, bringing search results of Nextcloud apps as
well as external services like Gitlab, Jira and Discourse in one
place
* Talk introduced bridging to other platforms including MS Teams,
Slack, IRC, Matrix and a dozen others
There is more:
* Notifications and Activities were brought together, making sure
you won't miss anything important
* We added a 'status' setting so you can communicate to other users
what you are up to
* Talk also brings dashboard and search integration, emoji picker,
upload view, camera and microphone settings, mute and more
* Calendar integrates in dashboard and search, introduced a list
view and design improvements
* Mail introduces threaded view, mailbox management and more
* Deck integrates with dashboard and search, introduces Calendar
integration, modal view for card editing and series of smaller
improvements
Some other app improvements we want to highlight include:
* Flow adds push notification and webhooks so other web apps can
easily integrate with Nextcloud
* Text introduced direct linking to files in Nextcloud
* Files lets you add a description to public link shares
New external apps in Nextcloud 20:
collectives -> default: false
Collectives is a Nextcloud App for activist and community projects
to organize together. Come and gather in collectives to build shared
knowledge.
flow_notifications
Enable users to configure notifications with customized conditions
in their Flow configuration.
integration_{discourse,github,gitlab,google,jira,mastodon,
moodle,reddit,twitter,zammad}
Various integration apps for external services.
talk_matterbridge
Brings the Matterbridge binary to your server to connect Nextcloud
Talk with other chat services.
In addition to those, we now have three more shipped apps that are
enabled by default: dashboard, user_status and weather_status.
The maintenance:install occ subcommand now requires to use the
--database-pass option and not doing so results in an error like this:
What is the password to access the database with user <nextcloud>?
Unable to hide the response.
Since we exclusively use peer authentication, we don't need to supply a
password, so setting the value of the option to an empty string causes
the error to go away.
Additionally, we now get the following exception message during
installation:
Could not boot files_trashbinCould not resolve trashManager! Class
trashManager does not exist
The error is non-fatal and doesn't seem to affect the finaly system, but
it's ugly nonetheless. Fortunately, there is already an upstream
issue[2], so we can expect this to be addressed in the next release.
One of the apps we are patching is gpxpod, which also got an update
specifically for Nextcloud 20. The update comes with a completely
changed directory structure that is more in line with the developer
manual, which is a good thing.
However, we do need to take this into account, when patching in
executable paths. Fortunately the code itself is unchanged though.
I also disabled the tencentcloudcosconfig app, since we currently get a
hash mismatch when fetching the app for Nextcloud 19 and I currently
have zero use for integrating this Tencent thingy.
Another app I disabled was the gpgmailer app, which needs GnuPG in order
to work. In the future we might want to properly patch apps like this
one to explicitly use a specific dependency.
When running the Nextcloud Cron service, the "news" app also fails with
an exception about "Call to a member function execute() on int". For our
VM test, this currently is not yet fatal, but in the long term we should
change this. Nevertheless, the issue also has been reported[3] upstream.
Nextcloud Talk now also has a few different CSS selectors, which I fixed
in our test driver.
Our proxy patch[4] for Nextcloud Maps also needed to be rebased against
version 0.1.8, which is only released for Nextcloud 20 in the app store.
[1]: https://nextcloud.com/blog/nextcloud-hub-20-debuts-dashboard-unifies-search-and-notifications-integrates-with-other-technologies/
[2]: nextcloud/server#22590
[3]: nextcloud/news#819
[4]: nextcloud/maps#167
Signed-off-by: aszlig <[email protected]>
As suggested[1] by @georgehrke: Please use rate-limiting (Read up about the @UserRateThrottle annotation, e.g. @UserRateThrottle(limit=5, period=100)) Unfortunately, determining sensible values here isn't so easy, since we don't want to annoy legitimate users while throttling those trying to flood the various services. For example consider clicking and dragging a point in the route: Every time you change the point a request is sent and at least I sometimes do this in rapid succession when misclicking. Throttingling after 5 requests in a 100 second time period is something I hit pretty frequently. So I arbitrarily settled on a maximum of 20 requests in a 60 second time window, which I still hit sometimes in experiments but since normally people won't request so many different routes, I think this value should be sufficient. If that's not the case we can still adjust it later anyway. I didn't add rate limiting to the OSRM routes, since those are usually self-hosted where we can't exceed some API data plan limit or even cause expensive bills. [1]: nextcloud#167 (comment) Signed-off-by: aszlig <[email protected]>
|
Rebased against master and added throttling for GraphHopper and Mapbox. |
This essentially adds the latest commit of my pull request[1] upstream and throttles requests to GraphHopper and Mapbox to avoid hitting API limits on these services. Right now, the limit is 20 requests in 60 seconds, which is choosen somewhat arbitrarily at the moment and the precise reason why I'm including it here as well is to check whether users of my Nextcloud instances are hitting these limits in production. [1]: nextcloud/maps#167 Signed-off-by: aszlig <[email protected]>
|
What going on with this PR request? |
|
A port to the new UI based on Vue #510 is needed. Reopen if planned. |
6 participants
With this, the routing API requests are no longer performed via the user's browser but are sent to the backend where the API calls are proxied accordingly.
I've done this for two reasons:
I've tested this with OSRM, Mapbox and GraphHopper.