Merge pull request #2695 from git-lfs/cred-helper-improvements

Cred helper improvements

config/environment.go

@@ -65,36 +65,63 @@ func (e *environment) GetAll(key string) []string {
 	return e.Fetcher.GetAll(key)
 }
 
-func (e *environment) Bool(key string, def bool) (val bool) {
+func (e *environment) Bool(key string, def bool) bool {
 	s, _ := e.Fetcher.Get(key)
-	if len(s) == 0 {
-		return def
-	}
-
-	switch strings.ToLower(s) {
-	case "true", "1", "on", "yes", "t":
-		return true
-	case "false", "0", "off", "no", "f":
-		return false
-	default:
-		return false
-	}
+	return Bool(s, def)
 }
 
-func (e *environment) Int(key string, def int) (val int) {
+func (e *environment) Int(key string, def int) int {
 	s, _ := e.Fetcher.Get(key)
-	if len(s) == 0 {
+	return Int(s, def)
+}
+
+func (e *environment) All() map[string][]string {
+	return e.Fetcher.All()
+}
+
+// Int returns the int value associated with the given value, or the value
+// "def", if the value is blank.
+//
+// To convert from a the string value attached to a given key,
+// `strconv.Atoi(val)` is called. If `Atoi` returned a non-nil error,
+// then the value "def" will be returned instead.
+//
+// Otherwise, if the value was converted `string -> int` successfully,
+// then it will be returned wholesale.
+func Int(value string, def int) int {
+	if len(value) == 0 {
 		return def
 	}
 
-	i, err := strconv.Atoi(s)
+	i, err := strconv.Atoi(value)
 	if err != nil {
 		return def
 	}
 
 	return i
 }
 
-func (e *environment) All() map[string][]string {
-	return e.Fetcher.All()
+// Bool returns the boolean state associated with the given value, or the
+// value "def", if the value is blank.
+//
+// The "boolean state associated with a given key" is defined as the
+// case-insensitive string comparison with the following:
+//
+// 1) true if...
+//   "true", "1", "on", "yes", or "t"
+// 2) false if...
+//   "false", "0", "off", "no", "f", or otherwise.
+func Bool(value string, def bool) bool {
+	if len(value) == 0 {
+		return def
+	}
+
+	switch strings.ToLower(value) {
+	case "true", "1", "on", "yes", "t":
+		return true
+	case "false", "0", "off", "no", "f":
+		return false
+	default:
+		return false
+	}
 }

config/url_config.go

@@ -50,6 +50,11 @@ func (c *URLConfig) GetAll(prefix, rawurl, key string) []string {
 	return c.git.GetAll(strings.Join([]string{prefix, key}, "."))
 }
 
+func (c *URLConfig) Bool(prefix, rawurl, key string, def bool) bool {
+	s, _ := c.Get(prefix, rawurl, key)
+	return Bool(s, def)
+}
+
 func (c *URLConfig) getAll(prefix, rawurl, key string) []string {
 	hosts, paths := c.hostsAndPaths(rawurl)
 

git/filter_process_scanner.go

@@ -172,8 +172,6 @@ func (o *FilterProcessScanner) Err() error { return o.err }
 // will read the body of the request. Since the body is _not_ offset, one
 // request should be read in its entirety before consuming the next request.
 func (o *FilterProcessScanner) readRequest() (*Request, error) {
-	tracerx.Printf("Read filter-process request.")
-
 	requestList, err := o.pl.readPacketList()
 	if err != nil {
 		return nil, err

lfsapi/auth.go

@@ -20,23 +20,11 @@ var (
 	defaultEndpointFinder   = NewEndpointFinder(nil)
 )
 
+// DoWithAuth sends an HTTP request to get an HTTP response. It attempts to add
+// authentication from netrc or git's credential helpers if necessary,
+// supporting basic and ntlm authentication.
 func (c *Client) DoWithAuth(remote string, req *http.Request) (*http.Response, error) {
-	credHelper := c.Credentials
-	if credHelper == nil {
-		credHelper = defaultCredentialHelper
-	}
-
-	netrcFinder := c.Netrc
-	if netrcFinder == nil {
-		netrcFinder = defaultNetrcFinder
-	}
-
-	ef := c.Endpoints
-	if ef == nil {
-		ef = defaultEndpointFinder
-	}
-
-	apiEndpoint, access, creds, credsURL, err := getCreds(credHelper, netrcFinder, ef, remote, req)
+	apiEndpoint, access, credHelper, credsURL, creds, err := c.getCreds(remote, req)
 	if err != nil {
 		return nil, err
 	}
@@ -94,32 +82,48 @@ func (c *Client) doWithCreds(req *http.Request, credHelper CredentialHelper, cre
 // 3. The Git Remote URL, which should be something like "https://git.com/repo.git"
 //    This URL is used for the Git Credential Helper. This way existing https
 //    Git remote credentials can be re-used for LFS.
-func getCreds(credHelper CredentialHelper, netrcFinder NetrcFinder, ef EndpointFinder, remote string, req *http.Request) (Endpoint, Access, Creds, *url.URL, error) {
+func (c *Client) getCreds(remote string, req *http.Request) (Endpoint, Access, CredentialHelper, *url.URL, Creds, error) {
+	ef := c.Endpoints
+	if ef == nil {
+		ef = defaultEndpointFinder
+	}
+
+	netrcFinder := c.Netrc
+	if netrcFinder == nil {
+		netrcFinder = defaultNetrcFinder
+	}
+
 	operation := getReqOperation(req)
 	apiEndpoint := ef.Endpoint(operation, remote)
 	access := ef.AccessFor(apiEndpoint.Url)
 
 	if access != NTLMAccess {
 		if requestHasAuth(req) || setAuthFromNetrc(netrcFinder, req) || access == NoneAccess {
-			return apiEndpoint, access, nil, nil, nil
+			return apiEndpoint, access, nullCreds, nil, nil, nil
 		}
 
 		credsURL, err := getCredURLForAPI(ef, operation, remote, apiEndpoint, req)
 		if err != nil {
-			return apiEndpoint, access, nil, nil, errors.Wrap(err, "creds")
+			return apiEndpoint, access, nullCreds, nil, nil, errors.Wrap(err, "creds")
 		}
 
 		if credsURL == nil {
-			return apiEndpoint, access, nil, nil, nil
+			return apiEndpoint, access, nullCreds, nil, nil, nil
 		}
 
-		creds, err := fillGitCreds(credHelper, ef, req, credsURL)
-		return apiEndpoint, access, creds, credsURL, err
+		credHelper, creds, err := c.getGitCreds(ef, req, credsURL)
+		if err == nil {
+			tracerx.Printf("Filled credentials for %s", credsURL)
+			setRequestAuth(req, creds["username"], creds["password"])
+		}
+		return apiEndpoint, access, credHelper, credsURL, creds, err
 	}
 
+	// NTLM ONLY
+
 	credsURL, err := url.Parse(apiEndpoint.Url)
 	if err != nil {
-		return apiEndpoint, access, nil, nil, errors.Wrap(err, "creds")
+		return apiEndpoint, access, nullCreds, nil, nil, errors.Wrap(err, "creds")
 	}
 
 	if netrcMachine := getAuthFromNetrc(netrcFinder, req); netrcMachine != nil {
@@ -131,20 +135,16 @@ func getCreds(credHelper CredentialHelper, netrcFinder NetrcFinder, ef EndpointF
 			"source":   "netrc",
 		}
 
-		return apiEndpoint, access, creds, credsURL, nil
+		return apiEndpoint, access, nullCreds, credsURL, creds, nil
 	}
 
-	creds, err := getGitCreds(credHelper, ef, req, credsURL)
-	return apiEndpoint, access, creds, credsURL, err
+	// NTLM uses creds to create the session
+	credHelper, creds, err := c.getGitCreds(ef, req, credsURL)
+	return apiEndpoint, access, credHelper, credsURL, creds, err
 }
 
-func getGitCreds(credHelper CredentialHelper, ef EndpointFinder, req *http.Request, u *url.URL) (Creds, error) {
-	path := strings.TrimPrefix(u.Path, "/")
-	input := Creds{"protocol": u.Scheme, "host": u.Host, "path": path}
-	if u.User != nil && u.User.Username() != "" {
-		input["username"] = u.User.Username()
-	}
-
+func (c *Client) getGitCreds(ef EndpointFinder, req *http.Request, u *url.URL) (CredentialHelper, Creds, error) {
+	credHelper, input := c.getCredentialHelper(u)
 	creds, err := credHelper.Fill(input)
 	if creds == nil || len(creds) < 1 {
 		errmsg := fmt.Sprintf("Git credentials for %s not found", u)
@@ -156,17 +156,7 @@ func getGitCreds(credHelper CredentialHelper, ef EndpointFinder, req *http.Reque
 		err = errors.New(errmsg)
 	}
 
-	return creds, err
-}
-
-func fillGitCreds(credHelper CredentialHelper, ef EndpointFinder, req *http.Request, u *url.URL) (Creds, error) {
-	creds, err := getGitCreds(credHelper, ef, req, u)
-	if err == nil {
-		tracerx.Printf("Filled credentials for %s", u)
-		setRequestAuth(req, creds["username"], creds["password"])
-	}
-
-	return creds, err
+	return credHelper, creds, err
 }
 
 func getAuthFromNetrc(netrcFinder NetrcFinder, req *http.Request) *netrc.Machine {

lfsapi/auth_test.go

@@ -11,6 +11,7 @@ import (
 	"testing"
 
 	"github.com/git-lfs/git-lfs/errors"
+	"github.com/git-lfs/git-lfs/git"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -93,7 +94,6 @@ func TestDoWithAuthApprove(t *testing.T) {
 	assert.True(t, creds.IsApproved(Creds(map[string]string{
 		"username": "user",
 		"password": "pass",
-		"path":     "repo/lfs",
 		"protocol": "http",
 		"host":     srv.Listener.Addr().String(),
 	})))
@@ -264,6 +264,51 @@ func TestGetCreds(t *testing.T) {
 				"lfs.url": "https://git-server.com/repo/lfs",
 				"lfs.https://git-server.com/repo/lfs.access": "basic",
 			},
+			Expected: getCredsExpected{
+				Access:        BasicAccess,
+				Endpoint:      "https://git-server.com/repo/lfs",
+				Authorization: basicAuth("git-server.com", "monkey"),
+				CredsURL:      "https://git-server.com/repo/lfs",
+				Creds: map[string]string{
+					"protocol": "https",
+					"host":     "git-server.com",
+					"username": "git-server.com",
+					"password": "monkey",
+				},
+			},
+		},
+		"basic access with usehttppath": getCredsTest{
+			Remote: "origin",
+			Method: "GET",
+			Href:   "https://git-server.com/repo/lfs/locks",
+			Config: map[string]string{
+				"lfs.url": "https://git-server.com/repo/lfs",
+				"lfs.https://git-server.com/repo/lfs.access": "basic",
+				"credential.usehttppath":                     "true",
+			},
+			Expected: getCredsExpected{
+				Access:        BasicAccess,
+				Endpoint:      "https://git-server.com/repo/lfs",
+				Authorization: basicAuth("git-server.com", "monkey"),
+				CredsURL:      "https://git-server.com/repo/lfs",
+				Creds: map[string]string{
+					"protocol": "https",
+					"host":     "git-server.com",
+					"username": "git-server.com",
+					"password": "monkey",
+					"path":     "repo/lfs",
+				},
+			},
+		},
+		"basic access with url-specific usehttppath": getCredsTest{
+			Remote: "origin",
+			Method: "GET",
+			Href:   "https://git-server.com/repo/lfs/locks",
+			Config: map[string]string{
+				"lfs.url": "https://git-server.com/repo/lfs",
+				"lfs.https://git-server.com/repo/lfs.access":    "basic",
+				"credential.https://git-server.com.usehttppath": "true",
+			},
 			Expected: getCredsExpected{
 				Access:        BasicAccess,
 				Endpoint:      "https://git-server.com/repo/lfs",
@@ -295,7 +340,6 @@ func TestGetCreds(t *testing.T) {
 					"host":     "git-server.com",
 					"username": "git-server.com",
 					"password": "monkey",
-					"path":     "repo/lfs",
 				},
 			},
 		},
@@ -369,7 +413,6 @@ func TestGetCreds(t *testing.T) {
 					"host":     "git-server.com",
 					"username": "user",
 					"password": "monkey",
-					"path":     "repo/lfs",
 				},
 			},
 		},
@@ -392,7 +435,6 @@ func TestGetCreds(t *testing.T) {
 					"host":     "git-server.com",
 					"username": "git-server.com",
 					"password": "monkey",
-					"path":     "repo",
 				},
 			},
 		},
@@ -443,7 +485,6 @@ func TestGetCreds(t *testing.T) {
 					"host":     "git-server.com",
 					"username": "git-server.com",
 					"password": "monkey",
-					"path":     "repo/lfs/locks",
 				},
 			},
 		},
@@ -465,7 +506,6 @@ func TestGetCreds(t *testing.T) {
 					"host":     "lfs-server.com",
 					"username": "lfs-server.com",
 					"password": "monkey",
-					"path":     "repo/lfs/locks",
 				},
 			},
 		},
@@ -487,7 +527,6 @@ func TestGetCreds(t *testing.T) {
 					"host":     "git-server.com:8080",
 					"username": "git-server.com:8080",
 					"password": "monkey",
-					"path":     "repo/lfs/locks",
 				},
 			},
 		},
@@ -509,16 +548,13 @@ func TestGetCreds(t *testing.T) {
 				Creds: map[string]string{
 					"host":     "git-server.com",
 					"password": "monkey",
-					"path":     "repo/lfs",
 					"protocol": "https",
 					"username": "git-server.com",
 				},
 			},
 		},
 	}
 
-	credHelper := &fakeCredentialFiller{}
-	netrcFinder := &fakeNetrc{}
 	for desc, test := range tests {
 		t.Log(desc)
 		req, err := http.NewRequest(test.Method, test.Href, nil)
@@ -531,8 +567,12 @@ func TestGetCreds(t *testing.T) {
 			req.Header.Set(key, value)
 		}
 
-		ef := NewEndpointFinder(NewContext(nil, nil, test.Config))
-		endpoint, access, creds, credsURL, err := getCreds(credHelper, netrcFinder, ef, test.Remote, req)
+		ctx := NewContext(git.NewConfig("", ""), nil, test.Config)
+		client, _ := NewClient(ctx)
+		client.Credentials = &fakeCredentialFiller{}
+		client.Netrc = &fakeNetrc{}
+		client.Endpoints = NewEndpointFinder(ctx)
+		endpoint, access, _, credsURL, creds, err := client.getCreds(test.Remote, req)
 		if !assert.Nil(t, err) {
 			continue
 		}

lfsapi/client.go

@@ -79,6 +79,8 @@ func joinURL(prefix, suffix string) string {
 	return prefix + slash + suffix
 }
 
+// Do sends an HTTP request to get an HTTP response. It wraps net/http, adding
+// extra headers, redirection handling, and error reporting.
 func (c *Client) Do(req *http.Request) (*http.Response, error) {
 	req.Header = c.extraHeadersFor(req)
 	req.Header.Set("User-Agent", UserAgent)