jcubic / git

GIT Web Terminal
https://git-terminal.js.org/
MIT License
89 stars 47 forks source link

WIP: credentialManager implementation #6

Closed billiegoose closed 6 years ago

billiegoose commented 6 years ago

For the proof-of-concept, I switched 'clone' and 'push' back to using git rather than git_wrapper. But so far it appears to work! You can even start from a clean state and do "git clone [a private github repo]" and it will prompt you for username/password! After that, it remembers the credentials, and it remembers the username in localStorage. Right now it just remembers one set of credentials, which I think is fine.

I did discover a bug with the way the new corsProxy parameter works though. It didn't handle your proxy server correctly. 😢 But I've fixed it in https://github.com/isomorphic-git/isomorphic-git/pull/396/commits/0863da385238d0d6c80284a302574f9577f54eb7 😄

billiegoose commented 6 years ago

image

jcubic commented 6 years ago

Wow, plugin work for two instances of git one in worker and one in main file? in my code I use read because that's less problematic since it use strings and Promises and I've send read request from worker to main tread to call read and send back the result string.

jcubic commented 6 years ago

Also rejected and approved should pause the terminal so you don't see the prompt when you wait for the command to finish.

is fill always executed to push, pull and clone? because If so I can pause in approved and rejected and don't resume in fill, pause/resume is important when you execute commands from url hash. (you can save commands in hash and they will be executed when open the link in other browser or refresh) so you can send someone link that will clone the repo change directory and one the file from that repo. If you have pause before clone and resume in credentials plugin it will not work it will cd to directory that don't exists.

Will check if this is the case with your code.

jcubic commented 6 years ago

I can't push I've got 401 with invalid username or password, I'm using two factor authentication, is that matter?

billiegoose commented 6 years ago

fill is called during a push, pull, or clone, after the first 401 HTTP response and before the second attempt is made.

since my approved/rejected aren't interactive then they dont' need to pause the terminal, but if you wanted to ask "save this password?" and "delete this password?" then you could pause it just like I did for fill.

Wow, plugin work for two instances of git one in worker and one in main file?

I mean... probably you'd want to go fully one way or the other. Seems wasteful (of memory) to have isomorphic-git in both environments.

Once I come up with a plugin to replace the eventemitter parameter, it should be much easier to proxy git between the UI thread and the worker. And since the credentialManager plugin follows the same API guideline (every function is async and takes a single pure JSON argument) it should be equally easy to proxy the credentialManager between the worker and the UI thread. (Now I say "easy." but that's a relative term...)

billiegoose commented 6 years ago

It's a WIP. Once I merge https://github.com/isomorphic-git/isomorphic-git/pull/396 it should work. The screenshot was using a custom build of isomorphic-git.

jcubic commented 6 years ago

I mean I get two 401 request one before push and one after I enter password with error in terminal.

and I wanted to pause before I execute the command that require credentials so the terminal wait untill push finish, there is need to be one pause before git.push but it need to be executed in the plugin so it's after user type password.

billiegoose commented 6 years ago

fill, approved, and rejected are all async. isomorphic-git will await for them to finish before resuming.

billiegoose commented 6 years ago

OK, I published a new release of isomorphic-git that includes the credentialManager, so this PR "works" now: https://wmhilton-contrib.github.io/git

I think we probably won't actually merge this PR, but I'll leave it up until we get yours working.

billiegoose commented 6 years ago

I'm going to close this PR in favor of #7