Skip to content
This repository has been archived by the owner on Sep 11, 2020. It is now read-only.

plumbing/transport/http: add callback based basic auth #419

Closed
wants to merge 1 commit into from
Closed

plumbing/transport/http: add callback based basic auth #419

wants to merge 1 commit into from

Conversation

roboll
Copy link

@roboll roboll commented Jun 12, 2017

Hey - great work.

I'm using go-git in a Github app which uses dynamic credentials, so having http basic auth backed by a callback function is really useful. Let me know what you think of this.

@codecov
Copy link

codecov bot commented Jun 12, 2017

Codecov Report

Merging #419 into master will decrease coverage by 0.88%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #419      +/-   ##
==========================================
- Coverage   78.03%   77.14%   -0.89%     
==========================================
  Files         127      124       -3     
  Lines        9186     9024     -162     
==========================================
- Hits         7168     6962     -206     
- Misses       1237     1304      +67     
+ Partials      781      758      -23
Impacted Files Coverage Δ
plumbing/transport/http/common.go 73.33% <60%> (-2.67%) ⬇️
plumbing/transport/ssh/common.go 0% <0%> (-50.91%) ⬇️
plumbing/transport/ssh/auth_method.go 33.33% <0%> (-24.77%) ⬇️
plumbing/transport/git/common.go 70.73% <0%> (-10.23%) ⬇️
worktree_commit.go 70.49% <0%> (-3.43%) ⬇️
worktree_status.go 69.66% <0%> (-1.71%) ⬇️
storage/filesystem/internal/dotgit/writers.go 75.73% <0%> (-1.48%) ⬇️
worktree.go 64.52% <0%> (-1.42%) ⬇️
remote.go 70.81% <0%> (-0.84%) ⬇️
plumbing/revlist/revlist.go 80.59% <0%> (-0.57%) ⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d02297...00b856a. Read the comment docs.

@mcuadros mcuadros requested a review from smola June 14, 2017 07:56
return &BasicAuthCallback{username, callback}
}

func (a *BasicAuthCallback) setAuth(r *http.Request) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this function is completely untested, can you cover it?

@smola
Copy link
Collaborator

smola commented Jun 14, 2017

So far the preferred way to implement this kind of logic is to set the basic auth from the beginning, or using ErrAuthRequired to ask for the password only when required. See example: #350 (comment)

@smola
Copy link
Collaborator

smola commented Jun 14, 2017

There is also a pending PR adding an example about intended usage: #383

@roboll
Copy link
Author

roboll commented Jun 14, 2017

hey @smola - I did see the other PR and example usage. I think this code solves an additional use case, which is constantly changing credentials. For example, I'm using this library with credentials that are rotated every hour as part of a Github App.

I could conceivably handle it externally, but this felt like a common enough use case to try to include here. If you'd rather not, I understand.

@mcuadros If you all think you may accept this, then yes, I'll definitely follow up with some test coverage for that function 👍.

@roboll roboll closed this Oct 3, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants