Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simple Auth support #345

Merged
merged 25 commits into from
Oct 3, 2019
Merged

Conversation

ktaletsk
Copy link
Contributor

@ktaletsk ktaletsk commented Apr 30, 2019

Adds simple credentials support as requested in #299

User flow

  1. User initiate git operation that may require auth (clone/push/pull)
  2. Extension attempts to clone/push/pull without credentials (assuming user set up their own SSH keys) is done exactly as before
  3. If git returns an error indicating insufficient credentials, extension shows a credential form for git username and password. User credentials are collected and sent to the backend.
    3a. If wrong credentials are provided form is shown over and over until successful authentication / or until user clicks 'CANCEL' button on the credentials form.

If user sets their own credentials, step 3 never happen.
When cloning/pulling from public repo, step 3 never happen

Technical details
On the backend, API handler is overloaded to expect auth information. Git class methods clone, push and pull are updated to allow passing of username/password information to git CLI using pexpect library similar to how subprocess.Popen is used to do that without credentials.

On the frontend, couple of new forms are added. src/components/CredentialsBox.tsx implements the body of credentials dialog. src/components/CommitAuthorBox.tsx implements similar dialog, but for author information on git commit.

Comments
UI is still little rough around the edges, but I hope community can help me fix that.

@ktaletsk ktaletsk marked this pull request as ready for review May 2, 2019 19:42
@saulshanabrook saulshanabrook self-requested a review May 6, 2019 20:51
@saulshanabrook
Copy link
Member

I tried this out locally and it worked for me. I am going to give a longer review of the code as well.

Screen Shot 2019-05-07 at 4 21 01 PM

@brunowego
Copy link

Hi @ktaletsk, thanks for this, I'm looking exactly for this!

@ktaletsk
Copy link
Contributor Author

@saulshanabrook, is there any update on this PR? Do you have any questions regarding proposed changes? I would rebase to the latest 0.6.0, but I need to know if this PR will be considered.

saulshanabrook
saulshanabrook previously approved these changes Jun 12, 2019
Copy link
Member

@saulshanabrook saulshanabrook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay on this, thank you for contributing this feature! It looks good to me.

@ktaletsk
Copy link
Contributor Author

I am going to resolve all the current conflicts with master shortly

@jaipreet-s
Copy link
Member

But I am still unclear, can't we include the auth support without this extract logic around the name and email? And just assume people have already set that up globally?

I don't think this question from Saul was addressed, and I have the same question.

@ktaletsk
Copy link
Contributor Author

Per @saulshanabrook,

I think this looks fine as is. We can always move it to setting the global later.

Should I take commit author attribution out of this Auth PR and submit as a separate PR?

@saulshanabrook
Copy link
Member

Should I take commit author attribution out of this Auth PR and submit as a separate PR?

That would be ideal, but I understand it's more work for you. I am OK taking this as is, but @jaipreet-s would you prefer them to be split?

@ktaletsk
Copy link
Contributor Author

ktaletsk commented Aug 9, 2019

@saulshanabrook @jaipreet-s I removed my implementation of the identity feature as it was added by #366. I also rebased to the latest master. Please, take a look.

jupyterlab_git/git.py Outdated Show resolved Hide resolved
jupyterlab_git/git.py Outdated Show resolved Hide resolved
jupyterlab_git/git.py Outdated Show resolved Hide resolved
jupyterlab_git/handlers.py Outdated Show resolved Hide resolved
jupyterlab_git/handlers.py Outdated Show resolved Hide resolved
src/git.ts Outdated Show resolved Hide resolved
src/git.ts Outdated Show resolved Hide resolved
src/widgets/gitClone.ts Outdated Show resolved Hide resolved
src/widgets/gitPushPull.ts Outdated Show resolved Hide resolved
src/widgets/gitPushPull.ts Outdated Show resolved Hide resolved
@ktaletsk
Copy link
Contributor Author

Thank you @fcollonval for the review and useful suggestions. I addressed the issues in the latest commit.

fcollonval
fcollonval previously approved these changes Aug 14, 2019
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx @ktaletsk
This is a great addition!

@fcollonval
Copy link
Member

@ktaletsk I forgot to point out, you need to add pexpect in setup.py

jupyterlab-git/setup.py

Lines 17 to 20 in 0b4339f

install_requires=[
'notebook',
'nbdime >= 1.1.0'
],

@jaipreet-s @saulshanabrook I think this is now ready to be merged. Could you please have a look?

@jaipreet-s
Copy link
Member

Thanks @ktaletsk for this work! I'll attempt to take a look sometime next week.

Copy link
Member

@jaipreet-s jaipreet-s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! I have some feedback but once that is addressed it would be great to have this feature merged in :-)

jupyterlab_git/git.py Show resolved Hide resolved
jupyterlab_git/handlers.py Outdated Show resolved Hide resolved
src/git.ts Outdated Show resolved Hide resolved
src/widgets/CredentialsBox.tsx Outdated Show resolved Hide resolved
src/widgets/gitClone.ts Outdated Show resolved Hide resolved
src/widgets/gitPushPull.ts Outdated Show resolved Hide resolved
tests/unit/test_pushpull.py Outdated Show resolved Hide resolved
src/widgets/CredentialsBox.tsx Show resolved Hide resolved
src/widgets/CredentialsBox.tsx Outdated Show resolved Hide resolved
src/widgets/gitClone.ts Outdated Show resolved Hide resolved
@ktaletsk
Copy link
Contributor Author

@jaipreet-s @fcollonval I went through the latest review, but I couldn't find a way to make the username and password required fields. Do you have any suggestions how to implement that?

@fcollonval
Copy link
Member

@ktaletsk Could you please rebase your branch on the master to solve the conflicts?

@ktaletsk
Copy link
Contributor Author

ktaletsk commented Oct 2, 2019

@fcollonval rebased! Let's merge it

@fcollonval
Copy link
Member

Thx a lot @ktaletsk especially for your patience to bring it through all the comments.

@fcollonval fcollonval dismissed jaipreet-s’s stale review October 3, 2019 05:03

Changes have been taken into account

@fcollonval fcollonval merged commit 1c93669 into jupyterlab:master Oct 3, 2019
@ktaletsk
Copy link
Contributor Author

ktaletsk commented Oct 3, 2019

Thanks @fcollonval @jaipreet-s @saulshanabrook and everyone who participated in discussions. Feels great to contribute to this extension!

Copy link
Member

@jaipreet-s jaipreet-s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@sudeshgit
Copy link

sudeshgit commented Oct 17, 2019

In which release this change is going to be included? I can see 0.8.1 as latest (https://pypi.org/project/jupyterlab-git/)

@fcollonval
Copy link
Member

fcollonval commented Oct 17, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants