GH-3397: Implemented the HTTP-based authentication for Git in Electron. #3426

kittaakos · 2018-11-07T15:39:19Z

Closes: #3397

Signed-off-by: Akos Kitta kittaakos@typefox.io

Resources for CQ:

kittaakos · 2018-11-07T15:50:21Z

For reviewers: it is in an early preview state.
What it does:

in the electron Theia, if the credential.helper is not (yet) set, it prompts for the username and password.

What it does not yet support:

save the password and reuse it.
I am sure, there are issues with the reconnection this point.

If you want to try out:

unset your credential.helper for Git

git config --global --unset credential.helper
git config --system --unset credential.helper

start the electron example.
open a workspace using a private Git repository.
do a pull from the Git widget.

kittaakos · 2018-11-08T14:10:14Z

@marcdumais-work, this PR contains a few resources that are based on VS Code. Can you please assist me what to do to start the CQ process? Thank you!

packages/git/src/electron-node/askpass/askpass.ts

marcdumais-work · 2018-11-08T20:13:33Z

Hi @kittaakos,

I am learning how to do this ATM, so let's try together. If you would add me in cc to the CQ/IP record, once created, I can more easily follow.

ref: https://www.eclipse.org/legal/EclipseLegalProcessPoster.pdf
I think we are in this case (Page 2 -> Everything else -> license is not EPL -> license is approved (MIT).

extra info:

navigate to our project page and log-in
On the right side of the page, under "Intellectual Property", chose "Create a Contribution Questionnaire"
type of contribution: "Contribution of code to be maintained by an Eclipse Foundation project"
I am a bit less sure at this point. But I would suggest:

name: same as PR?
descr: short description of PR, including mentioning the borrowing of files from the vscode project and which ones, that the original license for those files is MIT.
reason: New Feature but not initial contribution
contribution record: URL of the PR?
Authors: yourself for the bulk, plus Microsoft for copied files previously specified in the description?

I think we then need to attach the copied code (from original vscode project) to the IP record, that I think is automatically created when submitting the CQ. A zip file where only the copied file, in their original context, are preserved. e.g. keep the directory structure and copied files. Maybe also e.g. the original LICENSE file.

kittaakos · 2018-11-09T08:37:01Z

Thank you, @marcdumais-work.
CQ: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=18131

marcdumais-work · 2018-11-09T11:55:15Z

Hi @kittaakos

CQ: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=18131

Nice, thanks.

A couple of comments:

I think the type should be "Type_A"
From what I understood, we should submit the borrowed files in the form of a zip archive. The expected content was not 100% clear to me, but what seemed to work was to take the original repo (vscode in this case) and only keep, in their original directories, the borrowed source files and files that might include related license info, for context. e.g.: see latest attachment: on https://dev.eclipse.org/ipzilla/show_bug.cgi?id=17011
when ready to go forward, you have to click the "Issues addressed, return CQ to IP team" button on the CQ.

I added a quick comment on the CQ about the project type and to point towards our previous vscode CQ.

kittaakos · 2018-11-09T12:27:36Z

@marcdumais-work, thanks for the feedback.

I think the type should be "Type_A"

👍 Is it required? I must have missed it.

borrowed files

The files are not just borrowed but modified too. I do not know if this matters.

The expected content was not 100% clear to me

Me neither. Do I need to collect all files (from this PR) and attach it as a ZIP archive? Let's wait for the feedback for the CQ. What do you think?
For instance, here in JDT LS, the attachment was the entire patch (git diff) of the PR.

take the original repo (vscode in this case) and only keep, in their original directories, the borrowed source files and files that might include related license info

I do not understand it.

marcdumais-work · 2018-11-09T13:08:19Z

For instance, here in JDT LS, the attachment was the entire patch (git diff) of the PR.

Interesting. However I think in that case the reason for needing a CQ was that the patch size was over the 1000 lines threshold and from a non-committer / non-committer-company. The copied code came from an Eclipse Foundation project, so it was already known to be license-compatible. So maybe not the same case exactly.

Let's wait for the feedback for the CQ

+1. We can learn and then adjust for later CQs

packages/git/src/node/env/git-env-provider.ts

packages/git/src/node/git-backend-module.ts

AlexTugarev · 2018-11-09T14:46:31Z

packages/git/src/node/dugite-git.ts

    }

    async clone(remoteUrl: string, options: Git.Options.Clone): Promise<Repository> {
        const { localUri, branch } = options;
-        const exec = await this.execProvider.exec();
-        await clone(remoteUrl, this.getFsPath(localUri), { branch }, { exec });
+        const [exec, env] = await Promise.all([this.execProvider.exec(), this.gitEnv.promise]);


minor: we could also have a single initialized: Deferred to await on, and in the init we could set both properties.
but this is fine as well.

packages/git/src/electron-node/askpass/askpass.ts

AlexTugarev

Works nicely! Thanks!
🎉

kittaakos · 2018-11-12T08:17:27Z

On Windows, I still have the following issue:

root ERROR Cannot find a service for the path: /services/plugin-ext
Server worker failed. [ID: 1 | PID: 4260] { Error: write ENOTSUP
    at exports._errnoException (util.js:1024:11)
    at ChildProcess.target._send (internal/child_process.js:698:20)
    at ChildProcess.target.send (internal/child_process.js:582:19)
    at sendHelper (internal/cluster/utils.js:25:15)
    at send (internal/cluster/master.js:339:10)
    at handle.add (internal/cluster/master.js:311:5)
    at SharedHandle.add (internal/cluster/shared_handle.js:29:3)
    at queryServer (internal/cluster/master.js:300:10)
    at Worker.onmessage (internal/cluster/master.js:244:5)
    at ChildProcess.onInternalMessage (internal/cluster/utils.js:42:8) code: 'ENOTSUP', errno: 'ENOTSUP', syscall: 'write' }

Runninng chmod -R 777 ./packages/git/src/electron-node/askpass/ does not solve it.

kittaakos · 2018-11-13T07:15:35Z

I have managed to get this running (again) on OS X with and without the clusters, and on Windows without the clusters. There is a caveat, it seems, named pipe servers cannot be started from a cluster worker on Windows.

Please note that on Windows, it is not yet possible to set up a named pipe server in a worker.

I need to find a solution for that.

kittaakos · 2018-11-13T11:09:22Z

It works with the patch. I have replaced the named pipes with a pure HTTP server. I need to squash the commits.

marcdumais-work · 2018-11-16T21:54:58Z

Which files should be attached to the CQ? original from vscode or our modified version?

The files are not just borrowed but modified too. I do not know if this matters.

From email discussions with Sharon, I now understand better. For this case, it's the original vscode files that need to be attached to the CQ.

Given that you are a committer on the project, the main concern for the Foundation is to confirm license compatibility of the files we are effectively forking. Even if it's know a project has a compatible license, a given file can legitimately contain license info that's different from the project it's in. Should we miss a case like that for a file we copy, it could have significant consequences. Hence the CQ for this case in interested in the upstream version of the files.

I do not know if this matters.

For the CQ, no. It does of course matters that we properly maintain files that we have forked. What that means may vary depending on each file's original license. But it seems generally assumed that committers and the project can take care of that aspect, without automatic preemptive oversight.

@kittaakos I have sent you an email that covers a bit more. Please ask if there's anything.

kittaakos · 2018-11-19T08:14:39Z

I have sent you an email that covers a bit more. Please ask if there's anything.

Thank you for the help, @marcdumais-work! I will update the PR.

kittaakos · 2018-11-19T08:43:25Z

@marcdumais-work, I have updated the PR to use the original MIT copyright from vscode. I also attached the borrowed resources to the CQ.

marcdumais-work · 2018-11-19T13:06:09Z

I also attached the borrowed resources to the CQ.

Thanks @kittaakos . Reminder: please click the Issues addresses, return CQ to IPTeam button, on the CQ.

kittaakos · 2018-11-19T13:07:18Z

please click the Issues addresses, return CQ to IPTeam button, on the CQ.

Thanks! 👍

kittaakos · 2018-12-03T16:02:34Z

@marcdumais-work, did I get this correctly, we have the green lights with the CQ, right? Thanks!

marcdumais-work · 2018-12-03T17:06:16Z

The CQ registered for this PR is now "license-certified": http://dev.eclipse.org/ipzilla/show_bug.cgi?id=18131. Check-in can proceed when otherwise ready - thanks for your patience @kittaakos

If a Git operation (fetch, pull, merge, ...) requires authentication, the Theia backend will ask the frontend for the username and password. User credentials are neither stored nor reused. In electron, if a Git operation fails due to authentication, we report it back to the user. This PR does not change the behavior of the browser-based application. Closes: #3397 Signed-off-by: Akos Kitta <kittaakos@typefox.io>

kittaakos · 2018-12-04T11:54:48Z

I have verified it once more on Windows, I am merging this PR.

kittaakos requested review from akosyakov, AlexTugarev and svenefftinge November 7, 2018 15:46

kittaakos force-pushed the GH-3397 branch 3 times, most recently from 8d74813 to 9eb02aa Compare November 8, 2018 14:02

kittaakos changed the title ~~[WIP] GH-3397: Implemented the HTTP-based authentication for Git in Electron.~~ GH-3397: Implemented the HTTP-based authentication for Git in Electron. Nov 8, 2018

kittaakos requested review from marcdumais-work and removed request for AlexTugarev and akosyakov November 8, 2018 14:13

marcdumais-work reviewed Nov 8, 2018

View reviewed changes

packages/git/src/electron-node/askpass/askpass.ts Outdated Show resolved Hide resolved

kittaakos force-pushed the GH-3397 branch from 9eb02aa to c8f9b04 Compare November 9, 2018 08:08

kittaakos added the CQ Required (deprecated) issues requiring a CQ (contributor questionnaire) label Nov 9, 2018

akosyakov reviewed Nov 9, 2018

View reviewed changes

packages/git/src/node/env/git-env-provider.ts Outdated Show resolved Hide resolved

akosyakov reviewed Nov 9, 2018

View reviewed changes

packages/git/src/node/git-backend-module.ts Outdated Show resolved Hide resolved

AlexTugarev reviewed Nov 9, 2018

View reviewed changes

packages/git/src/electron-node/askpass/askpass.ts Outdated Show resolved Hide resolved

kittaakos force-pushed the GH-3397 branch from c8f9b04 to 47ba0d3 Compare November 9, 2018 15:10

AlexTugarev approved these changes Nov 9, 2018

View reviewed changes

kittaakos force-pushed the GH-3397 branch from 47ba0d3 to 4df451e Compare November 9, 2018 15:34

kittaakos force-pushed the GH-3397 branch from 4df451e to 97217c7 Compare November 12, 2018 13:31

kittaakos force-pushed the GH-3397 branch from 97217c7 to 4fd952c Compare November 13, 2018 07:26

kittaakos force-pushed the GH-3397 branch from 5c31c51 to 112fd02 Compare November 13, 2018 11:55

kittaakos force-pushed the GH-3397 branch from 112fd02 to b168306 Compare November 18, 2018 15:45

kittaakos force-pushed the GH-3397 branch 2 times, most recently from 112fd02 to 67fabac Compare November 19, 2018 08:33

marcdumais-work mentioned this pull request Dec 3, 2018

add java-debug extension based on vscode-java-debug #3613

Merged

11 tasks

kittaakos force-pushed the GH-3397 branch from 67fabac to 0c747fb Compare December 3, 2018 21:02

kittaakos force-pushed the GH-3397 branch from 0c747fb to 84cb36d Compare December 4, 2018 07:43

kittaakos merged commit d40e3dd into master Dec 4, 2018

kittaakos deleted the GH-3397 branch December 4, 2018 11:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-3397: Implemented the HTTP-based authentication for Git in Electron. #3426

GH-3397: Implemented the HTTP-based authentication for Git in Electron. #3426

kittaakos commented Nov 7, 2018 •

edited

Loading

kittaakos commented Nov 7, 2018

kittaakos commented Nov 8, 2018

marcdumais-work commented Nov 8, 2018

kittaakos commented Nov 9, 2018

marcdumais-work commented Nov 9, 2018

kittaakos commented Nov 9, 2018

marcdumais-work commented Nov 9, 2018

AlexTugarev Nov 9, 2018

AlexTugarev left a comment

kittaakos commented Nov 12, 2018

kittaakos commented Nov 13, 2018

kittaakos commented Nov 13, 2018

marcdumais-work commented Nov 16, 2018 •

edited

Loading

kittaakos commented Nov 19, 2018

kittaakos commented Nov 19, 2018

marcdumais-work commented Nov 19, 2018

kittaakos commented Nov 19, 2018

kittaakos commented Dec 3, 2018

marcdumais-work commented Dec 3, 2018

kittaakos commented Dec 4, 2018

GH-3397: Implemented the HTTP-based authentication for Git in Electron. #3426

GH-3397: Implemented the HTTP-based authentication for Git in Electron. #3426

Conversation

kittaakos commented Nov 7, 2018 • edited Loading

kittaakos commented Nov 7, 2018

kittaakos commented Nov 8, 2018

marcdumais-work commented Nov 8, 2018

kittaakos commented Nov 9, 2018

marcdumais-work commented Nov 9, 2018

kittaakos commented Nov 9, 2018

marcdumais-work commented Nov 9, 2018

AlexTugarev Nov 9, 2018

Choose a reason for hiding this comment

AlexTugarev left a comment

Choose a reason for hiding this comment

kittaakos commented Nov 12, 2018

kittaakos commented Nov 13, 2018

kittaakos commented Nov 13, 2018

marcdumais-work commented Nov 16, 2018 • edited Loading

kittaakos commented Nov 19, 2018

kittaakos commented Nov 19, 2018

marcdumais-work commented Nov 19, 2018

kittaakos commented Nov 19, 2018

kittaakos commented Dec 3, 2018

marcdumais-work commented Dec 3, 2018

kittaakos commented Dec 4, 2018

kittaakos commented Nov 7, 2018 •

edited

Loading

marcdumais-work commented Nov 16, 2018 •

edited

Loading