search

maplibre / maputnik

An open source visual editor for the 'MapLibre Style Specification'
https://www.maplibre.org/maputnik
MIT License
2.13k stars 401 forks source link

Load/save from/to Github Repo #155

Open stevage opened 7 years ago

stevage commented 7 years ago

This was tracked in #3, which was partially implemented with Gist support, but no repo saving/loading yet.

orangemug commented 7 years ago

I'm going to be working on this over the next week.

orangemug commented 7 years ago

Ok so I'm still working on this, there were a few other things that needed doing first. So a quick update as to where this is at.

The working branch is here https://github.com/orangemug/editor/compare/fix/update-deps...orangemug:feature/github-v2

It's basically hacked together in order to prove out the concept, it seems to work great. Here's what works (try to ignore the global variables 😱)

  1. Login with github

1_open

  1. List all a users repositories
  2. Load a style from a repository (currently tries to load style.json in the base of a repository)

3_show_repos

  1. Commit back to that repository

4_commit

These commits were made through the Maputnik UI using that process https://github.com/orangemug-test/style-test/commits/master

On my local machine this is all running inside docker containers via https://github.com/orangemug/maputnik-services including the GitHub authentication via micro-github (see https://github.com/orangemug/maputnik-services/blob/master/docker-compose.yml#L27-L43).

I've also started on a flags section it's basically chrome://flags for Maputnik. This is so I can release it early to get feedback, although releasing it is a few days off yet (probably towards middle of next week)

Next up, find a place to host micro-github, I'm currently thinking https://zeit.co/pricing but if anyone has any better ideas feel free to share.

As always any feedback is welcome.

stevage commented 7 years ago

Sounds great. Couple of quick comments:

orangemug commented 7 years ago

@stevage thanks for the feedback. Basically my aim initially is to to get the core working so we can iterate. I'll have a think about the above comments and get back to you. Any UI suggestions are also welcome.

gregwolanski commented 7 years ago

It's more complicated than I anticipated. :)

I'll try to take a look at it tomorrow.

gregwolanski commented 7 years ago

OK, today I end on:

npm ERR! Linux 4.9.49-moby
npm ERR! argv "/usr/bin/nodejs" "/usr/bin/npm" "install" "-d" "--dev"
npm ERR! node v6.1.0
npm ERR! npm  v3.8.6
npm ERR! path /maputnik/node_modules/jsonlint
npm ERR! code EXDEV
npm ERR! errno -18
npm ERR! syscall rename

npm ERR! EXDEV: cross-device link not permitted, rename '/maputnik/node_modules/jsonlint' -> '/maputnik/node_modules/.jsonlint.DELETE'
npm ERR!
npm ERR! If you need help, you may report this error at:
npm ERR!     <https://github.com/npm/npm/issues>

npm ERR! Please include the following file with any support request:
npm ERR!     /maputnik/npm-debug.log
ERROR: Service 'editor' failed to build: The command '/bin/sh -c npm install -d --dev' returned a non-zero code: 238

I'll get back to it tomorrow.

orangemug commented 7 years ago

Hey @gregorywolanski I wouldn't bother testing this yet, as it's still a work in progress.

oterral commented 6 years ago

Hi @orangemug we would like to add this save/load via github thing . Do you thing starting with your branch orangemug/editor@fix/update-deps...orangemug:feature/github-v2 is a good starting point ? do you have hints to provide before I start?

orangemug commented 6 years ago

Hey @oterral glad for the help.

So there is a reason why this hasn't been worked on in a while (sorry for that). I've been fixing other parts of the codebase for a new UI to make GitHub integration make sense.

See #252 https://281-84182601-gh.circle-artifacts.com/0/tmp/circle-artifacts.YPaobNs/build/index.html#0.36/0/0 basically we also need somewhere to store git history in context to the UI and also context for what repository is open. I think that UI gives us those abilities.

There are also a few other decisions (features) in that branch

Lets create a plan of action and then we can divide up into smaller tickets, I'm happy to help out.

So things to do

I'm pretty busy for the next couple of days but will try to find the time to respond.

oterral commented 6 years ago

I have 2 weeks full time to work on this.

What we want is:

We could potentially add multiple repositories.

Maybe all the github stuff could be another Icon in the left menu. If we do it correctly, any users could add in the same way different kind of repository , github is not only one.

Click on it, opens a first panel with the login page for github if not logged in or the list of repositories added if already logged in.

Click on repository, opens the 2nd panel wich allows to browse the current repository

Then when you find your style click it to load in the map.

Your thoughts?

stevage commented 6 years ago

Just a couple of comments from the sidelines:

oterral commented 6 years ago

Consider how other version control platforms might be integrated in the future, especially Gitlab. So it should be "version control integration", not "Github integration"

Yes It's important I think. we could also have a new button in the left menu which opens a first panel, with buttons "login with github", "login with gitlab" and others.

Your workflow above doesn't seem to have a way to save a style to a repo that it wasn't loaded from. People will very likely want to do that. (But if you do load from a repo, saving updates to there should be smooth and easy.)

Yes we want that too. But it's more something for the export menu, I guess. I need to think about it.

orangemug commented 6 years ago

So I have some thoughts. Basically my motivations here getting the right solution and not wasting anybodies time (with regards to development).

Maputnik doesn't currently have a server and I'd personally like to keep it that way. In this ticket I'm currently using https://github.com/mxstbr/micro-github via https://github.com/orangemug/maputnik-services which means Maputnik just talks to an auth server.

micro-github however is not very secure as far as I can tell. It sends back the auth token in the querystring, which means it will stay about in the browser history (can somebody confirm this).

I'd also be nervous about adding this into master unless we can get some other security concerns addressed. We basically have the same access as git push once authenticated which means if we have any security vulnerabilities those tokens would be exposed to the attacker. GitHub repos can disable force push but basically hardly anyone does, so the impact of an attack could be pretty big.

It's also worth noting that we could approach this a different way. @lukasmartinelli added filesystem access to the editor via maputnik/desktop we could extend filesystem access to add git support. This would also require less development because we'd instantly be supporting all git services. The downside is that we'd be moving towards a 'download' model for the editor.

Let me know your thoughts?

oterral commented 6 years ago

If I understand correctly, instead of using micro-github we could use an auth0 server or whatever auth server, am I right?

oterral commented 6 years ago

I had a look to maputnink-services with micro-github. It sends backs the access_token in the querystring and it stays in the history, but it's only temporary token. I don't think it's a hudge problem if you 'r using https. We could try to pass it as POST request or in the header instead of the query string.

Otherwise your branch works well. I can load/save without problem in github.

orangemug commented 6 years ago

First off @oterral thank you for looking at this issue. Sorry it's taken me some time to get back to you I've been super busy with the day job.

I am however going to suggest we hold off on this issue for the time being.

The reason being that I think we have some other serious usability issues to resolve. So currently styling for one of the major sources is easy just pick openmaptiles from the data sources and away you go. Styling one of your own sources is somewhat more difficult this is due to the ugliness that is https://. A common use case for using Maputnik seems to be

I have a file on my computer (shapefile/geojson) how do I style it?

Right now that's really a lot more difficult than it should be. The steps are basically upload GeoJSON to a server that supports HTTPS and CORs, which is not an ideal experience. Mostly I run Maputnik locally, because it's always running for development of the code base. Which means I'm not experiencing these issues.

Possible solutions to this problem are as follows

  1. Allow http://
  2. Make a desktop client
  3. Some ugly hacks I have in mind with hidden http:// iframes and window.postMessage

Here's the catch for the branch in this ticket.

  1. If we allow http:// we can't do auth because it would compromise security
  2. If we make a desktop client interacting with the git repositories on disk directly would probably be a better approach because we'd get GitLab/BitBucket/GitHub support in one feature
  3. I'll expand on the ugly hacks I have in mind later

I know some people will be disappointed. I am too, I really want Git support in Maputnik but it seems the right thing to hold off until the we have the other issues resolved.

pathmapper commented 6 years ago

Currently https://github.com/mapbox/geojson.io is being built again (:tada:):

https://github.com/tmcw/geojson.net

Maybe it's worth to take a look how geojson.net is going to implement GitHub integration, there is also micro-github involved:

https://github.com/tmcw/geojson.net/issues/8 https://github.com/tmcw/geojson.net/issues/18

Also geojson.net is using https:// and it is possible to load local files.

oterral commented 6 years ago

I understand all your concerns @orangemug but I don't think using maputnik as desktop software is an option for us (internal security issues. I will continue to investigate.

Thx for the link @pathmapper indeed what geojson.io/net does is exactly what I need. I will have a look.

oterral commented 6 years ago

geojson.io uses https://github.com/prose/gatekeeper instead of micro-github. The big diffference I see is GateKeeper is only used to get the access_token safely. The SPA directly requests the authorize code and uses GateKeeper only to get the access_token which it receives back in the response. No redirect url with access_token in querystring.

oterral commented 6 years ago

Ithnik maputnik should do the same and asks the github token directly.

orangemug commented 6 years ago

@oterral yeah gatekeeper uses the correct approach. Temp token with the real token returned in the response 👍

Also geojson.net is using https:// and it is possible to load local files.

Yeah sorry @pathmapper I probably wasn't clear here. So the issue isn't loading styles but loading sources. Any sources served over http:// from a local computer that could be GeoJSON from a local dataset or some vector tiles server locally. Because Maputnik is served over https:// the cross origin policies in the browser will prevent the fetching of those resources.

So basically if you have a data source at http://localhost:3000/sample.geojson or http://localhost:3000/sample.mvt the browser will throw an error (because it's not https://)

I think it's a pretty common use case to have local data and services from other applications either locally (localhost) or on your local network. Also setting up https:// for a local resource is not easy because self signed certificates can't realistically be used without first getting your browser to accept them (lots of hassle)

orangemug commented 6 years ago

I understand all your concerns @orangemug but I don't think using maputnik as desktop software is an option for us

@oterral if you could drop a comment in #164 as to what you using Maputnik for, that'd be much appreciated.

oterral commented 6 years ago

I have finally a working version with github (browsing all the repos) and gatekeeper base on @orangemug work. See here:

https://github.com/oterral/maputnik-service

To make it work you need to put the app oauth client id in the editor too: https://github.com/oterral/editor/blob/5f91018cce2ba886d072ba2d364ddefe1b30f7e5/src/components/modals/GitHubSection.jsx#L17

orangemug commented 6 years ago

@oterral congrats on getting it working! I'm a bit busy for the next couple of days but will find time to take a proper look at the weekend.

Note: Also I think https://github.com/maputnik/editor/issues/164#issuecomment-398652621 has convinced me we should just add this to Maputnik master branch behind a flag for now if you're still interested.

gregwolanski commented 6 years ago

This issue appears in the answers to question “What annoys you about Maputnik?” in the survey (original spelling):