Skip to content

Conversation

@pmrowla
Copy link
Contributor

@pmrowla pmrowla commented Mar 24, 2023

  • Adds higher level git.credentials API equivalent to git-credentials
  • Currently only used in dulwich backend for HTTP clients
  • Credentials that are successfully filled by helper backend will be approved/stored upon a successful request

Closes #215

@pmrowla pmrowla self-assigned this Mar 24, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 24, 2023

Codecov Report

Patch coverage: 46.49% and project coverage change: -0.54 ⚠️

Comparison is base (64a8926) 82.41% compared to head (a93a244) 81.88%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #216      +/-   ##
==========================================
- Coverage   82.41%   81.88%   -0.54%     
==========================================
  Files          25       25              
  Lines        3572     3643      +71     
  Branches      626      641      +15     
==========================================
+ Hits         2944     2983      +39     
- Misses        531      559      +28     
- Partials       97      101       +4     
Impacted Files Coverage Δ
src/scmrepo/git/credentials.py 38.00% <44.44%> (+10.94%) ⬆️
src/scmrepo/git/backend/dulwich/client.py 68.18% <60.00%> (+5.68%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pmrowla pmrowla merged commit 5a33e65 into treeverse:main Mar 24, 2023
@pmrowla pmrowla deleted the git-credentials branch March 24, 2023 09:49
@dberenbaum
Copy link
Contributor

Thanks @pmrowla! Does it have any impact on prompting/interactive auth? Previously, I don't think we supported it at all, but AFAIK git credential fill may prompt users if there are no credentials found. Does it mean that dulwich HTTP git clients will prompt for missing credentials?

@pmrowla
Copy link
Contributor Author

pmrowla commented Mar 24, 2023

Thanks @pmrowla! Does it have any impact on prompting/interactive auth? Previously, I don't think we supported it at all, but AFAIK git credential fill may prompt users if there are no credentials found. Does it mean that dulwich HTTP git clients will prompt for missing credentials?

If the credential manager requires interactive input then yes. With microsoft's standard GCM it opens a browser window for http logins. This was already the scmrepo behavior prior to this patch though (since the original credential helper support was added).

The difference was that we did not actually save the credentials afterwards, so users would be prompted every time we did a git operation that required auth, whereas now they are prompted on the first call and then we save the credentials in GCM, the same way that CLI git handles it.

@dberenbaum
Copy link
Contributor

Sorry @pmrowla, I think I was conflating the credential helpers with the default git behavior for requesting credentials. That is really only useful initially, but this is one place where CLI Git doesn't fail and DVC does. What do you think about supporting something like this?

@pmrowla
Copy link
Contributor Author

pmrowla commented Mar 24, 2023

We could support interactive prompting but it's probably not a quick fix since dulwich doesn't have native auth callback support yet. The intended workflow is that git should try an unauthenticated request (without using any credential helper or prompting) and then if it fails it should retry with helpers and/or prompting.

The current behavior in scmrepo is that we try credential helpers first if they exist. Technically this means we load GCM credentials even for a request that would not require it, like cloning a public repo, but in practice this isn't really an issue for the credential helper case. For the interactive CLI prompt it does make a difference, since with the current behavior we would have to prompt the user before making every request (since whatever the user enters via the interactive prompt is not saved).

We can implement this in scmrepo and then patch it into dulwich later, but it probably won't be a quick/small change.

@dberenbaum
Copy link
Contributor

What do you think about falling back to Git CLI when an auth error is raised?

@pmrowla
Copy link
Contributor Author

pmrowla commented Mar 27, 2023

@dberenbaum I would prefer that we just put the time into implementing it instead of using the CLI fallback

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.

credential-store: support store for new credentials

3 participants