From ed376b42ff1be237322e1c0c1d48c470ab169d8c Mon Sep 17 00:00:00 2001 From: Rick Olson Date: Thu, 27 Aug 2015 11:57:16 -0600 Subject: [PATCH 01/15] only check git-credentials for lfs api requests --- lfs/client.go | 123 ++++++++++++++++++++----------------------- lfs/download_test.go | 4 +- 2 files changed, 60 insertions(+), 67 deletions(-) diff --git a/lfs/client.go b/lfs/client.go index ceb9f1d1..b686b883 100644 --- a/lfs/client.go +++ b/lfs/client.go @@ -56,18 +56,18 @@ type objectResource struct { Error *objectError `json:"error,omitempty"` } -func (o *objectResource) NewRequest(relation, method string) (*http.Request, Creds, error) { +func (o *objectResource) NewRequest(relation, method string) (*http.Request, error) { rel, ok := o.Rel(relation) if !ok { - return nil, nil, objectRelationDoesNotExist + return nil, objectRelationDoesNotExist } - req, creds, err := newClientRequest(method, rel.Href, rel.Header) + req, err := newClientRequest(method, rel.Href, rel.Header) if err != nil { - return nil, nil, err + return nil, err } - return req, creds, nil + return req, nil } func (o *objectResource) Rel(name string) (*linkRelation, bool) { @@ -106,23 +106,22 @@ func (e *ClientError) Error() string { } func Download(oid string) (io.ReadCloser, int64, *WrappedError) { - req, creds, err := newApiRequest("GET", oid) + req, err := newApiRequest("GET", oid) if err != nil { return nil, 0, Error(err) } - res, obj, wErr := doApiRequest(req, creds) + res, obj, wErr := doApiRequest(req) if wErr != nil { return nil, 0, wErr } LogTransfer("lfs.api.download", res) - - req, creds, err = obj.NewRequest("download", "GET") + req, err = obj.NewRequest("download", "GET") if err != nil { return nil, 0, Error(err) } - res, wErr = doHttpRequest(req, creds) + res, wErr = doHttpRequest(req) if wErr != nil { return nil, 0, wErr } @@ -136,18 +135,18 @@ type byteCloser struct { } func DownloadCheck(oid string) (*objectResource, *WrappedError) { - req, creds, err := newApiRequest("GET", oid) + req, err := newApiRequest("GET", oid) if err != nil { return nil, Error(err) } - res, obj, wErr := doApiRequest(req, creds) + res, obj, wErr := doApiRequest(req) if wErr != nil { return nil, wErr } LogTransfer("lfs.api.download", res) - _, _, err = obj.NewRequest("download", "GET") + _, err = obj.NewRequest("download", "GET") if err != nil { return nil, Error(err) } @@ -156,12 +155,12 @@ func DownloadCheck(oid string) (*objectResource, *WrappedError) { } func DownloadObject(obj *objectResource) (io.ReadCloser, int64, *WrappedError) { - req, creds, err := obj.NewRequest("download", "GET") + req, err := obj.NewRequest("download", "GET") if err != nil { return nil, 0, Error(err) } - res, wErr := doHttpRequest(req, creds) + res, wErr := doHttpRequest(req) if wErr != nil { return nil, 0, wErr } @@ -186,7 +185,7 @@ func Batch(objects []*objectResource, operation string) ([]*objectResource, *Wra return nil, Error(err) } - req, creds, err := newBatchApiRequest() + req, err := newBatchApiRequest() if err != nil { return nil, Error(err) } @@ -197,7 +196,7 @@ func Batch(objects []*objectResource, operation string) ([]*objectResource, *Wra req.Body = &byteCloser{bytes.NewReader(by)} tracerx.Printf("api: batch %d files", len(objects)) - res, objs, wErr := doApiBatchRequest(req, creds) + res, objs, wErr := doApiBatchRequest(req) if wErr != nil { if res == nil { return nil, wErr @@ -242,7 +241,7 @@ func UploadCheck(oidPath string) (*objectResource, *WrappedError) { return nil, Error(err) } - req, creds, err := newApiRequest("POST", oid) + req, err := newApiRequest("POST", oid) if err != nil { return nil, Error(err) } @@ -253,7 +252,7 @@ func UploadCheck(oidPath string) (*objectResource, *WrappedError) { req.Body = &byteCloser{bytes.NewReader(by)} tracerx.Printf("api: uploading (%s)", oid) - res, obj, wErr := doApiRequest(req, creds) + res, obj, wErr := doApiRequest(req) if wErr != nil { return nil, wErr } @@ -291,7 +290,7 @@ func UploadObject(o *objectResource, cb CopyCallback) *WrappedError { Reader: file, } - req, creds, err := o.NewRequest("upload", "PUT") + req, err := o.NewRequest("upload", "PUT") if err != nil { return Error(err) } @@ -308,7 +307,7 @@ func UploadObject(o *objectResource, cb CopyCallback) *WrappedError { req.Body = ioutil.NopCloser(reader) - res, wErr := doHttpRequest(req, creds) + res, wErr := doHttpRequest(req) if wErr != nil { return wErr } @@ -321,7 +320,7 @@ func UploadObject(o *objectResource, cb CopyCallback) *WrappedError { io.Copy(ioutil.Discard, res.Body) res.Body.Close() - req, creds, err = o.NewRequest("verify", "POST") + req, err = o.NewRequest("verify", "POST") if err == objectRelationDoesNotExist { return nil } else if err != nil { @@ -337,7 +336,7 @@ func UploadObject(o *objectResource, cb CopyCallback) *WrappedError { req.Header.Set("Content-Length", strconv.Itoa(len(by))) req.ContentLength = int64(len(by)) req.Body = ioutil.NopCloser(bytes.NewReader(by)) - res, wErr = doHttpRequest(req, creds) + res, wErr = doHttpRequest(req) if wErr != nil { return wErr } @@ -349,7 +348,7 @@ func UploadObject(o *objectResource, cb CopyCallback) *WrappedError { return wErr } -func doHttpRequest(req *http.Request, creds Creds) (*http.Response, *WrappedError) { +func doHttpRequest(req *http.Request) (*http.Response, *WrappedError) { res, err := Config.HttpClient().Do(req) if res == nil { res = &http.Response{ @@ -365,7 +364,6 @@ func doHttpRequest(req *http.Request, creds Creds) (*http.Response, *WrappedErro if err != nil { wErr = Errorf(err, "Error for %s %s", res.Request.Method, res.Request.URL) } else { - saveCredentials(creds, res) wErr = handleResponse(res) } @@ -380,10 +378,21 @@ func doHttpRequest(req *http.Request, creds Creds) (*http.Response, *WrappedErro return res, wErr } -func doApiRequestWithRedirects(req *http.Request, creds Creds, via []*http.Request) (*http.Response, *WrappedError) { - res, wErr := doHttpRequest(req, creds) +func doApiRequestWithRedirects(req *http.Request, via []*http.Request, useCreds bool) (*http.Response, *WrappedError) { + var creds Creds + if useCreds { + c, err := getCreds(req) + if err != nil { + return nil, Error(err) + } + creds = c + } + + res, wErr := doHttpRequest(req) if wErr != nil { return res, wErr + } else { + saveCredentials(creds, res) } if res.StatusCode == 307 { @@ -394,7 +403,7 @@ func doApiRequestWithRedirects(req *http.Request, creds Creds, via []*http.Reque redirectTo = locurl.String() } - redirectedReq, redirectedCreds, err := newClientRequest(req.Method, redirectTo, nil) + redirectedReq, err := newClientRequest(req.Method, redirectTo, nil) if err != nil { return res, Errorf(err, err.Error()) } @@ -422,15 +431,15 @@ func doApiRequestWithRedirects(req *http.Request, creds Creds, via []*http.Reque return res, Errorf(err, err.Error()) } - return doApiRequestWithRedirects(redirectedReq, redirectedCreds, via) + return doApiRequestWithRedirects(redirectedReq, via, useCreds) } return res, nil } -func doApiRequest(req *http.Request, creds Creds) (*http.Response, *objectResource, *WrappedError) { +func doApiRequest(req *http.Request) (*http.Response, *objectResource, *WrappedError) { via := make([]*http.Request, 0, 4) - res, wErr := doApiRequestWithRedirects(req, creds, via) + res, wErr := doApiRequestWithRedirects(req, via, true) if wErr != nil { return res, nil, wErr } @@ -450,9 +459,9 @@ func doApiRequest(req *http.Request, creds Creds) (*http.Response, *objectResour // the repo will be marked as having private access and the request will be // re-run. When the repo is marked as having private access, credentials will // be retrieved. -func doApiBatchRequest(req *http.Request, creds Creds) (*http.Response, []*objectResource, *WrappedError) { +func doApiBatchRequest(req *http.Request) (*http.Response, []*objectResource, *WrappedError) { via := make([]*http.Request, 0, 4) - res, wErr := doApiRequestWithRedirects(req, creds, via) + res, wErr := doApiRequestWithRedirects(req, via, Config.PrivateAccess()) if wErr != nil { return res, nil, wErr @@ -535,7 +544,7 @@ func saveCredentials(creds Creds, res *http.Response) { } } -func newApiRequest(method, oid string) (*http.Request, Creds, error) { +func newApiRequest(method, oid string) (*http.Request, error) { endpoint := Config.Endpoint() objectOid := oid operation := "download" @@ -559,22 +568,22 @@ func newApiRequest(method, oid string) (*http.Request, Creds, error) { u, err := ObjectUrl(endpoint, objectOid) if err != nil { - return nil, nil, err + return nil, err } - req, creds, err := newClientRequest(method, u.String(), res.Header) + req, err := newClientRequest(method, u.String(), res.Header) if err != nil { - return nil, nil, err + return nil, err } req.Header.Set("Accept", mediaType) - return req, creds, nil + return req, nil } -func newClientRequest(method, rawurl string, header map[string]string) (*http.Request, Creds, error) { +func newClientRequest(method, rawurl string, header map[string]string) (*http.Request, error) { req, err := http.NewRequest(method, rawurl, nil) if err != nil { - return nil, nil, err + return nil, err } for key, value := range header { @@ -582,15 +591,11 @@ func newClientRequest(method, rawurl string, header map[string]string) (*http.Re } req.Header.Set("User-Agent", UserAgent) - creds, err := getCreds(req) - if err != nil { - return nil, nil, err - } - return req, creds, nil + return req, nil } -func newBatchApiRequest() (*http.Request, Creds, error) { +func newBatchApiRequest() (*http.Request, error) { endpoint := Config.Endpoint() res, err := sshAuthenticate(endpoint, "download", "") @@ -606,12 +611,12 @@ func newBatchApiRequest() (*http.Request, Creds, error) { u, err := ObjectUrl(endpoint, "batch") if err != nil { - return nil, nil, err + return nil, err } - req, creds, err := newBatchClientRequest("POST", u.String()) + req, err := newBatchClientRequest("POST", u.String()) if err != nil { - return nil, nil, err + return nil, err } req.Header.Set("Accept", mediaType) @@ -621,30 +626,18 @@ func newBatchApiRequest() (*http.Request, Creds, error) { } } - return req, creds, nil + return req, nil } -func newBatchClientRequest(method, rawurl string) (*http.Request, Creds, error) { +func newBatchClientRequest(method, rawurl string) (*http.Request, error) { req, err := http.NewRequest(method, rawurl, nil) if err != nil { - return nil, nil, err + return nil, err } req.Header.Set("User-Agent", UserAgent) - // Get the creds if we're private - if Config.PrivateAccess() { - // The PrivateAccess() check can be pushed down and this block simplified - // once everything goes through the batch endpoint. - creds, err := getCreds(req) - if err != nil { - return nil, nil, err - } - - return req, creds, nil - } - - return req, nil, nil + return req, nil } func getCreds(req *http.Request) (Creds, error) { diff --git a/lfs/download_test.go b/lfs/download_test.go index bcb7e5db..4f163e98 100644 --- a/lfs/download_test.go +++ b/lfs/download_test.go @@ -72,7 +72,7 @@ func TestSuccessfulDownload(t *testing.T) { t.Error("Invalid Accept") } - if r.Header.Get("Authorization") != expectedAuth(t, server) { + if r.Header.Get("Authorization") != "" { t.Error("Invalid Authorization") } @@ -204,7 +204,7 @@ func TestSuccessfulDownloadWithRedirects(t *testing.T) { t.Error("Invalid Accept") } - if r.Header.Get("Authorization") != expectedAuth(t, server) { + if r.Header.Get("Authorization") != "" { t.Error("Invalid Authorization") } From 55b1fdf8c5f0c05726c82e101db013d08fc15fe8 Mon Sep 17 00:00:00 2001 From: Rick Olson Date: Fri, 28 Aug 2015 11:02:17 -0600 Subject: [PATCH 02/15] rename getCreds() to getCredsForAPI() to describe it's LFS-API-specific behavior --- lfs/client.go | 63 +++++++---------------------------------- lfs/credentials.go | 54 +++++++++++++++++++++++++++++++++++ lfs/credentials_test.go | 12 ++++---- 3 files changed, 71 insertions(+), 58 deletions(-) diff --git a/lfs/client.go b/lfs/client.go index b686b883..ff630e89 100644 --- a/lfs/client.go +++ b/lfs/client.go @@ -381,7 +381,7 @@ func doHttpRequest(req *http.Request) (*http.Response, *WrappedError) { func doApiRequestWithRedirects(req *http.Request, via []*http.Request, useCreds bool) (*http.Response, *WrappedError) { var creds Creds if useCreds { - c, err := getCreds(req) + c, err := getCredsForAPI(req) if err != nil { return nil, Error(err) } @@ -537,10 +537,13 @@ func saveCredentials(creds Creds, res *http.Response) { return } - if res.StatusCode < 300 { - execCreds(creds, "approve") - } else if res.StatusCode == 401 { + switch res.StatusCode { + case 401, 403: execCreds(creds, "reject") + default: + if res.StatusCode < 300 { + execCreds(creds, "approve") + } } } @@ -640,54 +643,6 @@ func newBatchClientRequest(method, rawurl string) (*http.Request, error) { return req, nil } -func getCreds(req *http.Request) (Creds, error) { - if len(req.Header.Get("Authorization")) > 0 { - return nil, nil - } - - apiUrl, err := Config.ObjectUrl("") - if err != nil { - return nil, err - } - - if req.URL.Scheme != apiUrl.Scheme || - req.URL.Host != apiUrl.Host { - return nil, nil - } - - if setRequestAuthFromUrl(req, apiUrl) { - return nil, nil - } - - credsUrl := apiUrl - if len(Config.CurrentRemote) > 0 { - if u, ok := Config.GitConfig("remote." + Config.CurrentRemote + ".url"); ok { - gitRemoteUrl, err := url.Parse(u) - if err != nil { - return nil, err - } - - if gitRemoteUrl.Scheme == apiUrl.Scheme && - gitRemoteUrl.Host == apiUrl.Host { - - if setRequestAuthFromUrl(req, gitRemoteUrl) { - return nil, nil - } - - credsUrl = gitRemoteUrl - } - } - } - - creds, err := credentials(credsUrl) - if err != nil { - return nil, err - } - - setRequestAuth(req, creds["username"], creds["password"]) - return creds, nil -} - func setRequestAuthFromUrl(req *http.Request, u *url.URL) bool { if u.User != nil { if pass, ok := u.User.Password(); ok { @@ -701,6 +656,10 @@ func setRequestAuthFromUrl(req *http.Request, u *url.URL) bool { } func setRequestAuth(req *http.Request, user, pass string) { + if len(user) == 0 && len(pass) == 0 { + return + } + token := fmt.Sprintf("%s:%s", user, pass) auth := "Basic " + base64.URLEncoding.EncodeToString([]byte(token)) req.Header.Set("Authorization", auth) diff --git a/lfs/credentials.go b/lfs/credentials.go index 7f595a6c..96655dcf 100644 --- a/lfs/credentials.go +++ b/lfs/credentials.go @@ -3,11 +3,65 @@ package lfs import ( "bytes" "fmt" + "net/http" "net/url" "os/exec" "strings" ) +// getCredsForAPI gets the credentials for LFS API requests: +// 1. Check the LFS URL for authentication. Ex: http://user:pass@example.com +// 2. Check the Git remote URL for authentication IF it's the same scheme and +// host of the LFS URL. +// 3. Ask 'git credential' to fill in the password from one of the above URLs. +func getCredsForAPI(req *http.Request) (Creds, error) { + if len(req.Header.Get("Authorization")) > 0 { + return nil, nil + } + + apiUrl, err := Config.ObjectUrl("") + if err != nil { + return nil, err + } + + if req.URL.Scheme != apiUrl.Scheme || + req.URL.Host != apiUrl.Host { + return nil, nil + } + + if setRequestAuthFromUrl(req, apiUrl) { + return nil, nil + } + + credsUrl := apiUrl + if len(Config.CurrentRemote) > 0 { + if u, ok := Config.GitConfig("remote." + Config.CurrentRemote + ".url"); ok { + gitRemoteUrl, err := url.Parse(u) + if err != nil { + return nil, err + } + + if gitRemoteUrl.Scheme == apiUrl.Scheme && + gitRemoteUrl.Host == apiUrl.Host { + + if setRequestAuthFromUrl(req, gitRemoteUrl) { + return nil, nil + } + + credsUrl = gitRemoteUrl + } + } + } + + creds, err := credentials(credsUrl) + if err != nil { + return nil, err + } + + setRequestAuth(req, creds["username"], creds["password"]) + return creds, nil +} + type credentialFetcher interface { Credentials() Creds } diff --git a/lfs/credentials_test.go b/lfs/credentials_test.go index c2ec7617..70764a84 100644 --- a/lfs/credentials_test.go +++ b/lfs/credentials_test.go @@ -13,7 +13,7 @@ func TestGetCredentials(t *testing.T) { t.Fatal(err) } - creds, err := getCreds(req) + creds, err := getCredsForAPI(req) if err != nil { t.Fatal(err) } @@ -41,7 +41,7 @@ func TestGetCredentialsWithExistingAuthorization(t *testing.T) { req.Header.Set("Authorization", "Test monkey") - creds, err := getCreds(req) + creds, err := getCredsForAPI(req) if err != nil { t.Fatal(err) } @@ -62,7 +62,7 @@ func TestGetCredentialsWithSchemeMismatch(t *testing.T) { t.Fatal(err) } - creds, err := getCreds(req) + creds, err := getCredsForAPI(req) if err != nil { t.Fatal(err) } @@ -83,7 +83,7 @@ func TestGetCredentialsWithHostMismatch(t *testing.T) { t.Fatal(err) } - creds, err := getCreds(req) + creds, err := getCredsForAPI(req) if err != nil { t.Fatal(err) } @@ -104,7 +104,7 @@ func TestGetCredentialsWithPortMismatch(t *testing.T) { t.Fatal(err) } - creds, err := getCreds(req) + creds, err := getCredsForAPI(req) if err != nil { t.Fatal(err) } @@ -125,7 +125,7 @@ func TestGetCredentialsWithRfc1738UsernameAndPassword(t *testing.T) { t.Fatal(err) } - creds, err := getCreds(req) + creds, err := getCredsForAPI(req) if err != nil { t.Fatal(err) } From 74b6e2d992a97a96590f352ef4ab9a272b442872 Mon Sep 17 00:00:00 2001 From: Rick Olson Date: Fri, 28 Aug 2015 11:09:00 -0600 Subject: [PATCH 03/15] re-arrange some methods this puts the more important methods towards the top. It also stops exporting internal credential functions --- lfs/credentials.go | 99 ++++++++++++++++++++++------------------------ 1 file changed, 48 insertions(+), 51 deletions(-) diff --git a/lfs/credentials.go b/lfs/credentials.go index 96655dcf..bc650fa7 100644 --- a/lfs/credentials.go +++ b/lfs/credentials.go @@ -62,14 +62,6 @@ func getCredsForAPI(req *http.Request) (Creds, error) { return creds, nil } -type credentialFetcher interface { - Credentials() Creds -} - -type credentialFunc func(Creds, string) (credentialFetcher, error) - -var execCreds credentialFunc - func credentials(u *url.URL) (Creds, error) { path := strings.TrimPrefix(u.Path, "/") creds := Creds{"protocol": u.Scheme, "host": u.Host, "path": path} @@ -80,48 +72,6 @@ func credentials(u *url.URL) (Creds, error) { return cmd.Credentials(), nil } -type CredentialCmd struct { - output *bytes.Buffer - SubCommand string - *exec.Cmd -} - -func NewCommand(input Creds, subCommand string) *CredentialCmd { - buf1 := new(bytes.Buffer) - cmd := exec.Command("git", "credential", subCommand) - - cmd.Stdin = input.Buffer() - cmd.Stdout = buf1 - /* - There is a reason we don't hook up stderr here: - Git's credential cache daemon helper does not close its stderr, so if this - process is the process that fires up the daemon, it will wait forever - (until the daemon exits, really) trying to read from stderr. - - See https://github.com/github/git-lfs/issues/117 for more details. - */ - - return &CredentialCmd{buf1, subCommand, cmd} -} - -func (c *CredentialCmd) StdoutString() string { - return c.output.String() -} - -func (c *CredentialCmd) Credentials() Creds { - creds := make(Creds) - - for _, line := range strings.Split(c.StdoutString(), "\n") { - pieces := strings.SplitN(line, "=", 2) - if len(pieces) < 2 { - continue - } - creds[pieces[0]] = pieces[1] - } - - return creds -} - type Creds map[string]string func (c Creds) Buffer() *bytes.Buffer { @@ -137,9 +87,56 @@ func (c Creds) Buffer() *bytes.Buffer { return buf } +type credentialCmd struct { + output *bytes.Buffer + SubCommand string + *exec.Cmd +} + +func newCredentialCommand(input Creds, subCommand string) *credentialCmd { + buf1 := new(bytes.Buffer) + cmd := exec.Command("git", "credential", subCommand) + + cmd.Stdin = input.Buffer() + cmd.Stdout = buf1 + /* + There is a reason we don't hook up stderr here: + Git's credential cache daemon helper does not close its stderr, so if this + process is the process that fires up the daemon, it will wait forever + (until the daemon exits, really) trying to read from stderr. + + See https://github.com/github/git-lfs/issues/117 for more details. + */ + + return &credentialCmd{buf1, subCommand, cmd} +} + +func (c *credentialCmd) Credentials() Creds { + creds := make(Creds) + output := c.output.String() + + for _, line := range strings.Split(output, "\n") { + pieces := strings.SplitN(line, "=", 2) + if len(pieces) < 2 { + continue + } + creds[pieces[0]] = pieces[1] + } + + return creds +} + +type credentialFetcher interface { + Credentials() Creds +} + +type credentialFunc func(Creds, string) (credentialFetcher, error) + +var execCreds credentialFunc + func init() { execCreds = func(input Creds, subCommand string) (credentialFetcher, error) { - cmd := NewCommand(input, subCommand) + cmd := newCredentialCommand(input, subCommand) err := cmd.Start() if err == nil { err = cmd.Wait() From f71969d965aaab8d2d0664818fb2bf1350c33af1 Mon Sep 17 00:00:00 2001 From: Rick Olson Date: Fri, 28 Aug 2015 11:46:31 -0600 Subject: [PATCH 04/15] ask git-credential for API creds even if the host is different --- lfs/credentials.go | 47 ++++++++++++++++++++++------ lfs/credentials_test.go | 69 +++++++++++++++++++++++++++++------------ 2 files changed, 88 insertions(+), 28 deletions(-) diff --git a/lfs/credentials.go b/lfs/credentials.go index bc650fa7..1b5c0fd0 100644 --- a/lfs/credentials.go +++ b/lfs/credentials.go @@ -9,7 +9,25 @@ import ( "strings" ) -// getCredsForAPI gets the credentials for LFS API requests: +// getCreds gets the credentials for the given request's URL, and sets its +// Authorization header with them using Basic Authentication. This is like +// getCredsForAPI(), but skips checking the LFS url or git remote. +func getCreds(req *http.Request) (Creds, error) { + if len(req.Header.Get("Authorization")) > 0 { + return nil, nil + } + + creds, err := credentials(req.URL) + if err != nil { + return nil, err + } + + setRequestAuth(req, creds["username"], creds["password"]) + return creds, nil +} + +// getCredsForAPI gets the credentials for LFS API requests and sets the given +// request's Authorization header with them using Basic Authentication. // 1. Check the LFS URL for authentication. Ex: http://user:pass@example.com // 2. Check the Git remote URL for authentication IF it's the same scheme and // host of the LFS URL. @@ -19,14 +37,31 @@ func getCredsForAPI(req *http.Request) (Creds, error) { return nil, nil } + credsUrl, err := getCredURLForAPI(req) + if err != nil || credsUrl == nil { + return nil, err + } + + creds, err := credentials(credsUrl) + if err != nil { + return nil, err + } + + setRequestAuth(req, creds["username"], creds["password"]) + return creds, nil +} + +func getCredURLForAPI(req *http.Request) (*url.URL, error) { apiUrl, err := Config.ObjectUrl("") if err != nil { return nil, err } + // if the LFS request doesn't match the current LFS url, don't bother + // attempting to set the Authorization header from the LFS or Git remote URLs. if req.URL.Scheme != apiUrl.Scheme || req.URL.Host != apiUrl.Host { - return nil, nil + return req.URL, nil } if setRequestAuthFromUrl(req, apiUrl) { @@ -53,13 +88,7 @@ func getCredsForAPI(req *http.Request) (Creds, error) { } } - creds, err := credentials(credsUrl) - if err != nil { - return nil, err - } - - setRequestAuth(req, creds["username"], creds["password"]) - return creds, nil + return credsUrl, nil } func credentials(u *url.URL) (Creds, error) { diff --git a/lfs/credentials_test.go b/lfs/credentials_test.go index 70764a84..5f9b5048 100644 --- a/lfs/credentials_test.go +++ b/lfs/credentials_test.go @@ -6,7 +6,7 @@ import ( "testing" ) -func TestGetCredentials(t *testing.T) { +func TestGetCredentialsForAPI(t *testing.T) { Config.SetConfig("lfs.url", "https://lfs-server.com") req, err := http.NewRequest("GET", "https://lfs-server.com/foo", nil) if err != nil { @@ -32,7 +32,7 @@ func TestGetCredentials(t *testing.T) { } } -func TestGetCredentialsWithExistingAuthorization(t *testing.T) { +func TestGetCredentialsForAPIWithExistingAuthorization(t *testing.T) { Config.SetConfig("lfs.url", "https://lfs-server.com") req, err := http.NewRequest("GET", "http://lfs-server.com/foo", nil) if err != nil { @@ -55,7 +55,7 @@ func TestGetCredentialsWithExistingAuthorization(t *testing.T) { } } -func TestGetCredentialsWithSchemeMismatch(t *testing.T) { +func TestGetCredentialsForAPIWithSchemeMismatch(t *testing.T) { Config.SetConfig("lfs.url", "https://lfs-server.com") req, err := http.NewRequest("GET", "http://lfs-server.com/foo", nil) if err != nil { @@ -67,16 +67,29 @@ func TestGetCredentialsWithSchemeMismatch(t *testing.T) { t.Fatal(err) } - if creds != nil { - t.Errorf("Unexpected creds: %v", creds) + if creds == nil { + t.Fatalf("no credentials returned") } - if actual := req.Header.Get("Authorization"); actual != "" { - t.Errorf("Unexpected Authorization header: %s", actual) + if v := creds["protocol"]; v != "http" { + t.Errorf("Invalid protocol: %q", v) + } + + if v := creds["host"]; v != "lfs-server.com" { + t.Errorf("Invalid host: %q", v) + } + + if v := creds["path"]; v != "foo" { + t.Errorf("Invalid path: %q", v) + } + + expected := "Basic " + base64.URLEncoding.EncodeToString([]byte("lfs-server.com:monkey")) + if value := req.Header.Get("Authorization"); value != expected { + t.Errorf("Bad Authorization. Expected '%s', got '%s'", expected, value) } } -func TestGetCredentialsWithHostMismatch(t *testing.T) { +func TestGetCredentialsForAPIWithHostMismatch(t *testing.T) { Config.SetConfig("lfs.url", "https://lfs-server.com") req, err := http.NewRequest("GET", "https://lfs-server2.com/foo", nil) if err != nil { @@ -88,18 +101,27 @@ func TestGetCredentialsWithHostMismatch(t *testing.T) { t.Fatal(err) } - if creds != nil { - t.Errorf("Unexpected creds: %v", creds) + if v := creds["protocol"]; v != "https" { + t.Errorf("Invalid protocol: %q", v) } - if actual := req.Header.Get("Authorization"); actual != "" { - t.Errorf("Unexpected Authorization header: %s", actual) + if v := creds["host"]; v != "lfs-server2.com" { + t.Errorf("Invalid host: %q", v) + } + + if v := creds["path"]; v != "foo" { + t.Errorf("Invalid path: %q", v) + } + + expected := "Basic " + base64.URLEncoding.EncodeToString([]byte("lfs-server2.com:monkey")) + if value := req.Header.Get("Authorization"); value != expected { + t.Errorf("Bad Authorization. Expected '%s', got '%s'", expected, value) } } -func TestGetCredentialsWithPortMismatch(t *testing.T) { +func TestGetCredentialsForAPIWithPortMismatch(t *testing.T) { Config.SetConfig("lfs.url", "https://lfs-server.com") - req, err := http.NewRequest("GET", "https://lfs-server:8080.com/foo", nil) + req, err := http.NewRequest("GET", "https://lfs-server.com:8080/foo", nil) if err != nil { t.Fatal(err) } @@ -109,16 +131,25 @@ func TestGetCredentialsWithPortMismatch(t *testing.T) { t.Fatal(err) } - if creds != nil { - t.Errorf("Unexpected creds: %v", creds) + if v := creds["protocol"]; v != "https" { + t.Errorf("Invalid protocol: %q", v) } - if actual := req.Header.Get("Authorization"); actual != "" { - t.Errorf("Unexpected Authorization header: %s", actual) + if v := creds["host"]; v != "lfs-server.com:8080" { + t.Errorf("Invalid host: %q", v) + } + + if v := creds["path"]; v != "foo" { + t.Errorf("Invalid path: %q", v) + } + + expected := "Basic " + base64.URLEncoding.EncodeToString([]byte("lfs-server.com:8080:monkey")) + if value := req.Header.Get("Authorization"); value != expected { + t.Errorf("Bad Authorization. Expected '%s', got '%s'", expected, value) } } -func TestGetCredentialsWithRfc1738UsernameAndPassword(t *testing.T) { +func TestGetCredentialsForAPIWithRfc1738UsernameAndPassword(t *testing.T) { Config.SetConfig("lfs.url", "https://testuser:testpass@lfs-server.com") req, err := http.NewRequest("GET", "https://lfs-server.com/foo", nil) if err != nil { From d42ad123bd1c721932c313e471312b040bb8cb40 Mon Sep 17 00:00:00 2001 From: Rick Olson Date: Fri, 28 Aug 2015 13:14:04 -0600 Subject: [PATCH 05/15] rename credentials() => fillCredentials() --- lfs/credentials.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lfs/credentials.go b/lfs/credentials.go index 1b5c0fd0..16fcad8a 100644 --- a/lfs/credentials.go +++ b/lfs/credentials.go @@ -17,7 +17,7 @@ func getCreds(req *http.Request) (Creds, error) { return nil, nil } - creds, err := credentials(req.URL) + creds, err := fillCredentials(req.URL) if err != nil { return nil, err } @@ -42,7 +42,7 @@ func getCredsForAPI(req *http.Request) (Creds, error) { return nil, err } - creds, err := credentials(credsUrl) + creds, err := fillCredentials(credsUrl) if err != nil { return nil, err } @@ -91,7 +91,7 @@ func getCredURLForAPI(req *http.Request) (*url.URL, error) { return credsUrl, nil } -func credentials(u *url.URL) (Creds, error) { +func fillCredentials(u *url.URL) (Creds, error) { path := strings.TrimPrefix(u.Path, "/") creds := Creds{"protocol": u.Scheme, "host": u.Host, "path": path} cmd, err := execCreds(creds, "fill") From 9829d261369c90283e8fa766e322f9b1cca7efd4 Mon Sep 17 00:00:00 2001 From: Rick Olson Date: Fri, 28 Aug 2015 13:38:56 -0600 Subject: [PATCH 06/15] simplify creds --- lfs/client.go | 15 ---- lfs/credentials.go | 93 ++++++++++++------------ lfs/credentials_test.go | 20 +++--- test/cmd/git-credential-lfstest.go | 18 +++-- test/test-credentials.sh | 109 ++++++++++++++++------------- 5 files changed, 120 insertions(+), 135 deletions(-) diff --git a/lfs/client.go b/lfs/client.go index ff630e89..b631eb1d 100644 --- a/lfs/client.go +++ b/lfs/client.go @@ -532,21 +532,6 @@ func defaultError(res *http.Response) *WrappedError { return Error(fmt.Errorf(msgFmt, res.Request.URL)) } -func saveCredentials(creds Creds, res *http.Response) { - if creds == nil { - return - } - - switch res.StatusCode { - case 401, 403: - execCreds(creds, "reject") - default: - if res.StatusCode < 300 { - execCreds(creds, "approve") - } - } -} - func newApiRequest(method, oid string) (*http.Request, error) { endpoint := Config.Endpoint() objectOid := oid diff --git a/lfs/credentials.go b/lfs/credentials.go index 16fcad8a..93fc2f19 100644 --- a/lfs/credentials.go +++ b/lfs/credentials.go @@ -94,11 +94,22 @@ func getCredURLForAPI(req *http.Request) (*url.URL, error) { func fillCredentials(u *url.URL) (Creds, error) { path := strings.TrimPrefix(u.Path, "/") creds := Creds{"protocol": u.Scheme, "host": u.Host, "path": path} - cmd, err := execCreds(creds, "fill") - if err != nil { - return nil, err + return execCreds(creds, "fill") +} + +func saveCredentials(creds Creds, res *http.Response) { + if creds == nil { + return + } + + switch res.StatusCode { + case 401, 403: + execCreds(creds, "reject") + default: + if res.StatusCode < 300 { + execCreds(creds, "approve") + } } - return cmd.Credentials(), nil } type Creds map[string]string @@ -116,18 +127,13 @@ func (c Creds) Buffer() *bytes.Buffer { return buf } -type credentialCmd struct { - output *bytes.Buffer - SubCommand string - *exec.Cmd -} +type credentialFunc func(Creds, string) (Creds, error) -func newCredentialCommand(input Creds, subCommand string) *credentialCmd { - buf1 := new(bytes.Buffer) +func execCredsCommand(input Creds, subCommand string) (Creds, error) { + output := new(bytes.Buffer) cmd := exec.Command("git", "credential", subCommand) - cmd.Stdin = input.Buffer() - cmd.Stdout = buf1 + cmd.Stdout = output /* There is a reason we don't hook up stderr here: Git's credential cache daemon helper does not close its stderr, so if this @@ -137,14 +143,30 @@ func newCredentialCommand(input Creds, subCommand string) *credentialCmd { See https://github.com/github/git-lfs/issues/117 for more details. */ - return &credentialCmd{buf1, subCommand, cmd} -} + err := cmd.Start() + if err == nil { + err = cmd.Wait() + } + + if _, ok := err.(*exec.ExitError); ok { + if !Config.GetenvBool("GIT_TERMINAL_PROMPT", true) { + return nil, fmt.Errorf("Change the GIT_TERMINAL_PROMPT env var to be prompted to enter your credentials for %s://%s.", + input["protocol"], input["host"]) + } + + // 'git credential' exits with 128 if the helper doesn't fill the username + // and password values. + if subCommand == "fill" && err.Error() == "exit status 128" { + return input, nil + } + } + + if err != nil { + return nil, fmt.Errorf("'git credential %s' error: %s\n", subCommand, err.Error()) + } -func (c *credentialCmd) Credentials() Creds { creds := make(Creds) - output := c.output.String() - - for _, line := range strings.Split(output, "\n") { + for _, line := range strings.Split(output.String(), "\n") { pieces := strings.SplitN(line, "=", 2) if len(pieces) < 2 { continue @@ -152,36 +174,7 @@ func (c *credentialCmd) Credentials() Creds { creds[pieces[0]] = pieces[1] } - return creds + return creds, nil } -type credentialFetcher interface { - Credentials() Creds -} - -type credentialFunc func(Creds, string) (credentialFetcher, error) - -var execCreds credentialFunc - -func init() { - execCreds = func(input Creds, subCommand string) (credentialFetcher, error) { - cmd := newCredentialCommand(input, subCommand) - err := cmd.Start() - if err == nil { - err = cmd.Wait() - } - - if exitErr, ok := err.(*exec.ExitError); ok { - if exitErr.ProcessState.Success() == false && !Config.GetenvBool("GIT_TERMINAL_PROMPT", true) { - return nil, fmt.Errorf("Change the GIT_TERMINAL_PROMPT env var to be prompted to enter your credentials for %s://%s.", - input["protocol"], input["host"]) - } - } - - if err != nil { - return cmd, fmt.Errorf("'git credential %s' error: %s\n", cmd.SubCommand, err.Error()) - } - - return cmd, nil - } -} +var execCreds credentialFunc = execCredsCommand diff --git a/lfs/credentials_test.go b/lfs/credentials_test.go index 5f9b5048..737cfd09 100644 --- a/lfs/credentials_test.go +++ b/lfs/credentials_test.go @@ -172,17 +172,13 @@ func TestGetCredentialsForAPIWithRfc1738UsernameAndPassword(t *testing.T) { } func init() { - execCreds = func(input Creds, subCommand string) (credentialFetcher, error) { - return &testCredentialFetcher{input}, nil + execCreds = func(input Creds, subCommand string) (Creds, error) { + output := make(Creds) + for key, value := range input { + output[key] = value + } + output["username"] = input["host"] + output["password"] = "monkey" + return output, nil } } - -type testCredentialFetcher struct { - Creds Creds -} - -func (c *testCredentialFetcher) Credentials() Creds { - c.Creds["username"] = c.Creds["host"] - c.Creds["password"] = "monkey" - return c.Creds -} diff --git a/test/cmd/git-credential-lfstest.go b/test/cmd/git-credential-lfstest.go index 4bcddd03..2301123a 100644 --- a/test/cmd/git-credential-lfstest.go +++ b/test/cmd/git-credential-lfstest.go @@ -6,7 +6,6 @@ import ( "io/ioutil" "os" "path/filepath" - "regexp" "strings" ) @@ -17,10 +16,7 @@ var ( "erase": noop, } - delim = '\n' - - hostRE = regexp.MustCompile(`\A127.0.0.1:\d+\z`) - + delim = '\n' credsDir = "" ) @@ -74,12 +70,14 @@ func fill() { os.Exit(1) } - if _, ok := creds["username"]; !ok { - creds["username"] = user - } + if user != "skip" { + if _, ok := creds["username"]; !ok { + creds["username"] = user + } - if _, ok := creds["password"]; !ok { - creds["password"] = pass + if _, ok := creds["password"]; !ok { + creds["password"] = pass + } } for key, value := range creds { diff --git a/test/test-credentials.sh b/test/test-credentials.sh index c562148a..6b71bf61 100755 --- a/test/test-credentials.sh +++ b/test/test-credentials.sh @@ -2,54 +2,7 @@ . "test/testlib.sh" -begin_test "git credential" -( - set -e - - printf "git:server" > "$CREDSDIR/credential-test.com" - printf "git:path" > "$CREDSDIR/credential-test.com--some-path" - - mkdir empty - cd empty - git init - - echo "protocol=http -host=credential-test.com" | GIT_TERMINAL_PROMPT=0 git credential fill | tee cred.log - - expected="protocol=http -host=credential-test.com -username=git -password=server" - [ "$expected" = "$(cat cred.log)" ] - - echo "protocol=http -host=credential-test.com -path=some/path" | GIT_TERMINAL_PROMPT=0 git credential fill | tee cred.log - - expected="protocol=http -host=credential-test.com -username=git -password=server" - - [ "$expected" = "$(cat cred.log)" ] - - git config credential.useHttpPath true - - echo "protocol=http -host=credential-test.com -path=some/path" | GIT_TERMINAL_PROMPT=0 git credential fill | tee cred.log - - expected="protocol=http -host=credential-test.com -path=some/path -username=git -password=path" - - [ "$expected" = "$(cat cred.log)" ] -) -end_test - -begin_test "credentials without useHttpPath, with wrong password" +begin_test "credentials without useHttpPath, with wrong path password" ( set -e @@ -136,3 +89,63 @@ begin_test "credentials with useHttpPath, with correct password" grep "(1 of 1 files)" push.log ) end_test + +begin_test "git credential" +( + set -e + + printf "git:server" > "$CREDSDIR/credential-test.com" + printf "git:path" > "$CREDSDIR/credential-test.com--some-path" + + mkdir empty + cd empty + git init + + echo "protocol=http +host=credential-test.com" | GIT_TERMINAL_PROMPT=0 git credential fill > cred.log + cat cred.log + + expected="protocol=http +host=credential-test.com +username=git +password=server" + [ "$expected" = "$(cat cred.log)" ] + + echo "protocol=http +host=credential-test.com +path=some/path" | GIT_TERMINAL_PROMPT=0 git credential fill > cred.log + cat cred.log + + expected="protocol=http +host=credential-test.com +username=git +password=server" + + [ "$expected" = "$(cat cred.log)" ] + + git config credential.useHttpPath true + + echo "protocol=http +host=credential-test.com +path=some/path" | GIT_TERMINAL_PROMPT=0 git credential fill > cred.log + cat cred.log + + expected="protocol=http +host=credential-test.com +path=some/path +username=git +password=path" + + [ "$expected" = "$(cat cred.log)" ] + + printf "skip:" > "$CREDSDIR/credential-test.com--some-path" + set +e + echo "protocol=http +host=credential-test.com +path=some/path" | GIT_TERMINAL_PROMPT=0 git credential fill + if [ $? -eq 0 ] + then + exit 1 + fi +) +end_test From 79a82a5cc301e64a566f0f75fdf276f59804768f Mon Sep 17 00:00:00 2001 From: Rick Olson Date: Fri, 28 Aug 2015 13:59:07 -0600 Subject: [PATCH 07/15] bake credential handling into doHttpRequest() and handleResponse() --- lfs/client.go | 107 +++++++++++++++++++++------------------ lfs/client_error_test.go | 6 +-- 2 files changed, 61 insertions(+), 52 deletions(-) diff --git a/lfs/client.go b/lfs/client.go index b631eb1d..8fcf5905 100644 --- a/lfs/client.go +++ b/lfs/client.go @@ -121,7 +121,7 @@ func Download(oid string) (io.ReadCloser, int64, *WrappedError) { return nil, 0, Error(err) } - res, wErr = doHttpRequest(req) + res, wErr = doStorageRequest(req) if wErr != nil { return nil, 0, wErr } @@ -160,7 +160,7 @@ func DownloadObject(obj *objectResource) (io.ReadCloser, int64, *WrappedError) { return nil, 0, Error(err) } - res, wErr := doHttpRequest(req) + res, wErr := doStorageRequest(req) if wErr != nil { return nil, 0, wErr } @@ -307,7 +307,7 @@ func UploadObject(o *objectResource, cb CopyCallback) *WrappedError { req.Body = ioutil.NopCloser(reader) - res, wErr := doHttpRequest(req) + res, wErr := doStorageRequest(req) if wErr != nil { return wErr } @@ -336,7 +336,7 @@ func UploadObject(o *objectResource, cb CopyCallback) *WrappedError { req.Header.Set("Content-Length", strconv.Itoa(len(by))) req.ContentLength = int64(len(by)) req.Body = ioutil.NopCloser(bytes.NewReader(by)) - res, wErr = doHttpRequest(req) + res, wErr = doHttpRequest(req, nil) if wErr != nil { return wErr } @@ -348,7 +348,56 @@ func UploadObject(o *objectResource, cb CopyCallback) *WrappedError { return wErr } -func doHttpRequest(req *http.Request) (*http.Response, *WrappedError) { +// doApiRequest to the legacy API. +func doApiRequest(req *http.Request) (*http.Response, *objectResource, *WrappedError) { + via := make([]*http.Request, 0, 4) + res, wErr := doApiRequestWithRedirects(req, via, true) + if wErr != nil { + return res, nil, wErr + } + + obj := &objectResource{} + wErr = decodeApiResponse(res, obj) + + if wErr != nil { + setErrorResponseContext(wErr, res) + return nil, nil, wErr + } + + return res, obj, nil +} + +// doApiBatchRequest runs the request to the batch API. If the API returns a 401, +// the repo will be marked as having private access and the request will be +// re-run. When the repo is marked as having private access, credentials will +// be retrieved. +func doApiBatchRequest(req *http.Request) (*http.Response, []*objectResource, *WrappedError) { + via := make([]*http.Request, 0, 4) + res, wErr := doApiRequestWithRedirects(req, via, Config.PrivateAccess()) + + if wErr != nil { + return res, nil, wErr + } + + var objs map[string][]*objectResource + wErr = decodeApiResponse(res, &objs) + + if wErr != nil { + setErrorResponseContext(wErr, res) + } + + return res, objs["objects"], wErr +} + +// doStorageREquest runs the request to the storage API from a link provided by +// the "actions" or "_links" properties an LFS API response. +func doStorageRequest(req *http.Request) (*http.Response, *WrappedError) { + return doHttpRequest(req, nil) +} + +// doHttpRequest runs the given HTTP request. LFS or Storage API requests should +// use doApiBatchRequest() or doStorageRequest() instead. +func doHttpRequest(req *http.Request, creds Creds) (*http.Response, *WrappedError) { res, err := Config.HttpClient().Do(req) if res == nil { res = &http.Response{ @@ -364,7 +413,7 @@ func doHttpRequest(req *http.Request) (*http.Response, *WrappedError) { if err != nil { wErr = Errorf(err, "Error for %s %s", res.Request.Method, res.Request.URL) } else { - wErr = handleResponse(res) + wErr = handleResponse(res, creds) } if wErr != nil { @@ -388,11 +437,9 @@ func doApiRequestWithRedirects(req *http.Request, via []*http.Request, useCreds creds = c } - res, wErr := doHttpRequest(req) + res, wErr := doHttpRequest(req, creds) if wErr != nil { return res, wErr - } else { - saveCredentials(creds, res) } if res.StatusCode == 307 { @@ -437,47 +484,9 @@ func doApiRequestWithRedirects(req *http.Request, via []*http.Request, useCreds return res, nil } -func doApiRequest(req *http.Request) (*http.Response, *objectResource, *WrappedError) { - via := make([]*http.Request, 0, 4) - res, wErr := doApiRequestWithRedirects(req, via, true) - if wErr != nil { - return res, nil, wErr - } +func handleResponse(res *http.Response, creds Creds) *WrappedError { + saveCredentials(creds, res) - obj := &objectResource{} - wErr = decodeApiResponse(res, obj) - - if wErr != nil { - setErrorResponseContext(wErr, res) - return nil, nil, wErr - } - - return res, obj, nil -} - -// doApiBatchRequest runs the request to the batch API. If the API returns a 401, -// the repo will be marked as having private access and the request will be -// re-run. When the repo is marked as having private access, credentials will -// be retrieved. -func doApiBatchRequest(req *http.Request) (*http.Response, []*objectResource, *WrappedError) { - via := make([]*http.Request, 0, 4) - res, wErr := doApiRequestWithRedirects(req, via, Config.PrivateAccess()) - - if wErr != nil { - return res, nil, wErr - } - - var objs map[string][]*objectResource - wErr = decodeApiResponse(res, &objs) - - if wErr != nil { - setErrorResponseContext(wErr, res) - } - - return res, objs["objects"], wErr -} - -func handleResponse(res *http.Response) *WrappedError { if res.StatusCode < 400 { return nil } diff --git a/lfs/client_error_test.go b/lfs/client_error_test.go index dc966683..45cb40c7 100644 --- a/lfs/client_error_test.go +++ b/lfs/client_error_test.go @@ -13,7 +13,7 @@ import ( func TestSuccessStatus(t *testing.T) { for _, status := range []int{200, 201, 202} { res := &http.Response{StatusCode: status} - if err := handleResponse(res); err != nil { + if err := handleResponse(res, nil); err != nil { t.Errorf("Unexpected error for HTTP %d: %s", status, err.Error()) } } @@ -59,7 +59,7 @@ func TestErrorStatusWithCustomMessage(t *testing.T) { } res.Header.Set("Content-Type", "application/vnd.git-lfs+json; charset=utf-8") - wErr := handleResponse(res) + wErr := handleResponse(res, nil) if wErr == nil { t.Errorf("No error from HTTP %d", status) continue @@ -121,7 +121,7 @@ func TestErrorStatusWithDefaultMessage(t *testing.T) { // purposely wrong content type so it falls back to default res.Header.Set("Content-Type", "application/vnd.git-lfs+json2") - wErr := handleResponse(res) + wErr := handleResponse(res, nil) if wErr == nil { t.Errorf("No error from HTTP %d", status) continue From 5801efaa7b6546e69b0e9a5ddef4b84f34b3f18a Mon Sep 17 00:00:00 2001 From: Rick Olson Date: Fri, 28 Aug 2015 14:03:40 -0600 Subject: [PATCH 08/15] check git-credential on storage api requests --- lfs/client.go | 7 ++++++- lfs/download_test.go | 16 ---------------- 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/lfs/client.go b/lfs/client.go index 8fcf5905..4e62f0c4 100644 --- a/lfs/client.go +++ b/lfs/client.go @@ -392,7 +392,12 @@ func doApiBatchRequest(req *http.Request) (*http.Response, []*objectResource, *W // doStorageREquest runs the request to the storage API from a link provided by // the "actions" or "_links" properties an LFS API response. func doStorageRequest(req *http.Request) (*http.Response, *WrappedError) { - return doHttpRequest(req, nil) + creds, err := getCreds(req) + if err != nil { + return nil, Error(err) + } + + return doHttpRequest(req, creds) } // doHttpRequest runs the given HTTP request. LFS or Storage API requests should diff --git a/lfs/download_test.go b/lfs/download_test.go index 4f163e98..6da54c12 100644 --- a/lfs/download_test.go +++ b/lfs/download_test.go @@ -72,10 +72,6 @@ func TestSuccessfulDownload(t *testing.T) { t.Error("Invalid Accept") } - if r.Header.Get("Authorization") != "" { - t.Error("Invalid Authorization") - } - if r.Header.Get("A") != "1" { t.Error("invalid A") } @@ -204,10 +200,6 @@ func TestSuccessfulDownloadWithRedirects(t *testing.T) { t.Error("Invalid Accept") } - if r.Header.Get("Authorization") != "" { - t.Error("Invalid Authorization") - } - if r.Header.Get("A") != "1" { t.Error("invalid A") } @@ -412,10 +404,6 @@ func TestSuccessfulDownloadFromSeparateHost(t *testing.T) { t.Error("Invalid Accept") } - if r.Header.Get("Authorization") != "" { - t.Error("Invalid Authorization") - } - if r.Header.Get("A") != "1" { t.Error("invalid A") } @@ -546,10 +534,6 @@ func TestSuccessfulDownloadFromSeparateRedirectedHost(t *testing.T) { t.Error("Invalid Accept") } - if r.Header.Get("Authorization") != "" { - t.Error("Invalid Authorization") - } - if r.Header.Get("A") != "1" { t.Error("invalid A") } From e406d70c60d60db0041e292dc301cc475acc84a2 Mon Sep 17 00:00:00 2001 From: Rick Olson Date: Fri, 28 Aug 2015 14:07:26 -0600 Subject: [PATCH 09/15] more info --- lfs/credentials.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lfs/credentials.go b/lfs/credentials.go index 93fc2f19..8aada1b4 100644 --- a/lfs/credentials.go +++ b/lfs/credentials.go @@ -32,6 +32,10 @@ func getCreds(req *http.Request) (Creds, error) { // 2. Check the Git remote URL for authentication IF it's the same scheme and // host of the LFS URL. // 3. Ask 'git credential' to fill in the password from one of the above URLs. +// +// This prefers the Git remote URL for checking credentials so that users only +// have to enter their passwords once for Git and Git LFS. It uses the same +// URL path that Git does, in case 'useHttpPath' is enabled in the Git config. func getCredsForAPI(req *http.Request) (Creds, error) { if len(req.Header.Get("Authorization")) > 0 { return nil, nil From a3a03075d8e6b539f49975240afdd4c838e1c22f Mon Sep 17 00:00:00 2001 From: Rick Olson Date: Fri, 28 Aug 2015 14:10:55 -0600 Subject: [PATCH 10/15] test the output if git-credential doesnt fail --- test/test-credentials.sh | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/test-credentials.sh b/test/test-credentials.sh index 6b71bf61..7974a95a 100755 --- a/test/test-credentials.sh +++ b/test/test-credentials.sh @@ -142,10 +142,14 @@ password=path" set +e echo "protocol=http host=credential-test.com -path=some/path" | GIT_TERMINAL_PROMPT=0 git credential fill +path=some/path" | GIT_TERMINAL_PROMPT=0 git credential fill > cred.log if [ $? -eq 0 ] then - exit 1 + expected="protocol=http +host=credential-test.com +path=some/path" + + [ "$expected" = "$(cat cred.log)" ] fi ) end_test From 5b7724470227e1e464b37bf4c6f68f9dfdf43341 Mon Sep 17 00:00:00 2001 From: Rick Olson Date: Fri, 28 Aug 2015 14:13:42 -0600 Subject: [PATCH 11/15] backing out of this test --- test/test-credentials.sh | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/test/test-credentials.sh b/test/test-credentials.sh index 7974a95a..d705c6a2 100755 --- a/test/test-credentials.sh +++ b/test/test-credentials.sh @@ -137,19 +137,5 @@ username=git password=path" [ "$expected" = "$(cat cred.log)" ] - - printf "skip:" > "$CREDSDIR/credential-test.com--some-path" - set +e - echo "protocol=http -host=credential-test.com -path=some/path" | GIT_TERMINAL_PROMPT=0 git credential fill > cred.log - if [ $? -eq 0 ] - then - expected="protocol=http -host=credential-test.com -path=some/path" - - [ "$expected" = "$(cat cred.log)" ] - fi ) end_test From 446ffb25f430ae369cf6577055324a9ef90c5c08 Mon Sep 17 00:00:00 2001 From: Rick Olson Date: Fri, 28 Aug 2015 14:29:49 -0600 Subject: [PATCH 12/15] run "verify" API requests with credentials too --- lfs/client.go | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/lfs/client.go b/lfs/client.go index 4e62f0c4..bc6f27df 100644 --- a/lfs/client.go +++ b/lfs/client.go @@ -111,7 +111,7 @@ func Download(oid string) (io.ReadCloser, int64, *WrappedError) { return nil, 0, Error(err) } - res, obj, wErr := doApiRequest(req) + res, obj, wErr := doLegacyApiRequest(req) if wErr != nil { return nil, 0, wErr } @@ -140,7 +140,7 @@ func DownloadCheck(oid string) (*objectResource, *WrappedError) { return nil, Error(err) } - res, obj, wErr := doApiRequest(req) + res, obj, wErr := doLegacyApiRequest(req) if wErr != nil { return nil, wErr } @@ -252,7 +252,7 @@ func UploadCheck(oidPath string) (*objectResource, *WrappedError) { req.Body = &byteCloser{bytes.NewReader(by)} tracerx.Printf("api: uploading (%s)", oid) - res, obj, wErr := doApiRequest(req) + res, obj, wErr := doLegacyApiRequest(req) if wErr != nil { return nil, wErr } @@ -336,7 +336,7 @@ func UploadObject(o *objectResource, cb CopyCallback) *WrappedError { req.Header.Set("Content-Length", strconv.Itoa(len(by))) req.ContentLength = int64(len(by)) req.Body = ioutil.NopCloser(bytes.NewReader(by)) - res, wErr = doHttpRequest(req, nil) + res, wErr = doAPIRequest(req) if wErr != nil { return wErr } @@ -348,8 +348,8 @@ func UploadObject(o *objectResource, cb CopyCallback) *WrappedError { return wErr } -// doApiRequest to the legacy API. -func doApiRequest(req *http.Request) (*http.Response, *objectResource, *WrappedError) { +// doLegacyApiRequest runs the request to the LFS legacy API. +func doLegacyApiRequest(req *http.Request) (*http.Response, *objectResource, *WrappedError) { via := make([]*http.Request, 0, 4) res, wErr := doApiRequestWithRedirects(req, via, true) if wErr != nil { @@ -367,13 +367,12 @@ func doApiRequest(req *http.Request) (*http.Response, *objectResource, *WrappedE return res, obj, nil } -// doApiBatchRequest runs the request to the batch API. If the API returns a 401, -// the repo will be marked as having private access and the request will be +// doApiBatchRequest runs the request to the LFS batch API. If the API returns a +// 401, the repo will be marked as having private access and the request will be // re-run. When the repo is marked as having private access, credentials will // be retrieved. func doApiBatchRequest(req *http.Request) (*http.Response, []*objectResource, *WrappedError) { - via := make([]*http.Request, 0, 4) - res, wErr := doApiRequestWithRedirects(req, via, Config.PrivateAccess()) + res, wErr := doAPIRequest(req) if wErr != nil { return res, nil, wErr @@ -400,6 +399,19 @@ func doStorageRequest(req *http.Request) (*http.Response, *WrappedError) { return doHttpRequest(req, creds) } +// doAPIRequest runs the request to the LFS API, without parsing the response +// body. If the API returns a 401, the repo will be marked as having private +// access and the request will be re-run. When the repo is marked as having +// private access, credentials will be retrieved. +func doAPIRequest(req *http.Request) (*http.Response, *WrappedError) { + via := make([]*http.Request, 0, 4) + useCreds := true + if req.Method == "GET" || req.Method == "HEAD" { + useCreds = Config.PrivateAccess() + } + return doApiRequestWithRedirects(req, via, useCreds) +} + // doHttpRequest runs the given HTTP request. LFS or Storage API requests should // use doApiBatchRequest() or doStorageRequest() instead. func doHttpRequest(req *http.Request, creds Creds) (*http.Response, *WrappedError) { From 278d4f2831f11bab651116c5728a8e7273413698 Mon Sep 17 00:00:00 2001 From: Rick Olson Date: Fri, 28 Aug 2015 15:19:52 -0600 Subject: [PATCH 13/15] add Config.Reset() --- lfs/config.go | 12 +++++------- lfs/config_test.go | 26 ++++++++++++++++++++++++++ lfs/credentials_test.go | 7 ++++++- lfs/download_test.go | 7 +++++++ lfs/upload_test.go | 7 +++++++ 5 files changed, 51 insertions(+), 8 deletions(-) diff --git a/lfs/config.go b/lfs/config.go index 007a9e7e..9d549388 100644 --- a/lfs/config.go +++ b/lfs/config.go @@ -29,6 +29,7 @@ type Configuration struct { loading sync.Mutex // guards initialization of gitConfig and remotes gitConfig map[string]string + origConfig map[string]string remotes []string extensions map[string]Extension fetchIncludePaths []string @@ -190,21 +191,16 @@ func (c *Configuration) GitConfig(key string) (string, bool) { return value, ok } -func (c *Configuration) SetConfig(key, value string) { - c.loadGitConfig() - c.gitConfig[key] = value -} - func (c *Configuration) ObjectUrl(oid string) (*url.URL, error) { return ObjectUrl(c.Endpoint(), oid) } -func (c *Configuration) loadGitConfig() { +func (c *Configuration) loadGitConfig() bool { c.loading.Lock() defer c.loading.Unlock() if c.gitConfig != nil { - return + return false } uniqRemotes := make(map[string]bool) @@ -282,4 +278,6 @@ func (c *Configuration) loadGitConfig() { } c.remotes = append(c.remotes, remote) } + + return true } diff --git a/lfs/config_test.go b/lfs/config_test.go index c74d9e91..0df9495e 100644 --- a/lfs/config_test.go +++ b/lfs/config_test.go @@ -184,6 +184,7 @@ func TestBareHTTPEndpointAddsLfsSuffix(t *testing.T) { } func TestObjectUrl(t *testing.T) { + defer Config.ResetConfig() tests := map[string]string{ "http://example.com": "http://example.com/objects/oid", "http://example.com/": "http://example.com/objects/oid", @@ -205,6 +206,8 @@ func TestObjectUrl(t *testing.T) { } func TestObjectsUrl(t *testing.T) { + defer Config.ResetConfig() + tests := map[string]string{ "http://example.com": "http://example.com/objects", "http://example.com/": "http://example.com/objects", @@ -391,3 +394,26 @@ func TestLoadInvalidExtension(t *testing.T) { assert.Equal(t, "", ext.Smudge) assert.Equal(t, 0, ext.Priority) } + +// only used for tests +func (c *Configuration) SetConfig(key, value string) { + if c.loadGitConfig() { + c.loading.Lock() + c.origConfig = make(map[string]string) + for k, v := range c.gitConfig { + c.origConfig[k] = v + } + c.loading.Unlock() + } + + c.gitConfig[key] = value +} + +func (c *Configuration) ResetConfig() { + c.loading.Lock() + c.gitConfig = make(map[string]string) + for k, v := range c.origConfig { + c.gitConfig[k] = v + } + c.loading.Unlock() +} diff --git a/lfs/credentials_test.go b/lfs/credentials_test.go index 737cfd09..0d768979 100644 --- a/lfs/credentials_test.go +++ b/lfs/credentials_test.go @@ -7,6 +7,7 @@ import ( ) func TestGetCredentialsForAPI(t *testing.T) { + defer Config.ResetConfig() Config.SetConfig("lfs.url", "https://lfs-server.com") req, err := http.NewRequest("GET", "https://lfs-server.com/foo", nil) if err != nil { @@ -33,6 +34,7 @@ func TestGetCredentialsForAPI(t *testing.T) { } func TestGetCredentialsForAPIWithExistingAuthorization(t *testing.T) { + defer Config.ResetConfig() Config.SetConfig("lfs.url", "https://lfs-server.com") req, err := http.NewRequest("GET", "http://lfs-server.com/foo", nil) if err != nil { @@ -56,6 +58,7 @@ func TestGetCredentialsForAPIWithExistingAuthorization(t *testing.T) { } func TestGetCredentialsForAPIWithSchemeMismatch(t *testing.T) { + defer Config.ResetConfig() Config.SetConfig("lfs.url", "https://lfs-server.com") req, err := http.NewRequest("GET", "http://lfs-server.com/foo", nil) if err != nil { @@ -90,6 +93,7 @@ func TestGetCredentialsForAPIWithSchemeMismatch(t *testing.T) { } func TestGetCredentialsForAPIWithHostMismatch(t *testing.T) { + defer Config.ResetConfig() Config.SetConfig("lfs.url", "https://lfs-server.com") req, err := http.NewRequest("GET", "https://lfs-server2.com/foo", nil) if err != nil { @@ -120,6 +124,7 @@ func TestGetCredentialsForAPIWithHostMismatch(t *testing.T) { } func TestGetCredentialsForAPIWithPortMismatch(t *testing.T) { + defer Config.ResetConfig() Config.SetConfig("lfs.url", "https://lfs-server.com") req, err := http.NewRequest("GET", "https://lfs-server.com:8080/foo", nil) if err != nil { @@ -150,6 +155,7 @@ func TestGetCredentialsForAPIWithPortMismatch(t *testing.T) { } func TestGetCredentialsForAPIWithRfc1738UsernameAndPassword(t *testing.T) { + defer Config.ResetConfig() Config.SetConfig("lfs.url", "https://testuser:testpass@lfs-server.com") req, err := http.NewRequest("GET", "https://lfs-server.com/foo", nil) if err != nil { @@ -170,7 +176,6 @@ func TestGetCredentialsForAPIWithRfc1738UsernameAndPassword(t *testing.T) { t.Errorf("Bad Authorization. Expected '%s', got '%s'", expected, value) } } - func init() { execCreds = func(input Creds, subCommand string) (Creds, error) { output := make(Creds) diff --git a/lfs/download_test.go b/lfs/download_test.go index 6da54c12..1ba9fb49 100644 --- a/lfs/download_test.go +++ b/lfs/download_test.go @@ -83,6 +83,7 @@ func TestSuccessfulDownload(t *testing.T) { w.Write([]byte("test")) }) + defer Config.ResetConfig() Config.SetConfig("lfs.url", server.URL+"/media") reader, size, wErr := Download("oid") if wErr != nil { @@ -211,6 +212,7 @@ func TestSuccessfulDownloadWithRedirects(t *testing.T) { w.Write([]byte("test")) }) + defer Config.ResetConfig() Config.SetConfig("lfs.url", server.URL+"/redirect") for _, redirect := range redirectCodes { @@ -316,6 +318,7 @@ func TestSuccessfulDownloadWithAuthorization(t *testing.T) { w.Write([]byte("test")) }) + defer Config.ResetConfig() Config.SetConfig("lfs.url", server.URL+"/media") reader, size, wErr := Download("oid") if wErr != nil { @@ -415,6 +418,7 @@ func TestSuccessfulDownloadFromSeparateHost(t *testing.T) { w.Write([]byte("test")) }) + defer Config.ResetConfig() Config.SetConfig("lfs.url", server.URL+"/media") reader, size, wErr := Download("oid") if wErr != nil { @@ -545,6 +549,7 @@ func TestSuccessfulDownloadFromSeparateRedirectedHost(t *testing.T) { w.Write([]byte("test")) }) + defer Config.ResetConfig() Config.SetConfig("lfs.url", server.URL+"/media") for _, redirect := range redirectCodes { @@ -581,6 +586,7 @@ func TestDownloadAPIError(t *testing.T) { w.WriteHeader(404) }) + defer Config.ResetConfig() Config.SetConfig("lfs.url", server.URL+"/media") _, _, wErr := Download("oid") if wErr == nil { @@ -648,6 +654,7 @@ func TestDownloadStorageError(t *testing.T) { w.WriteHeader(500) }) + defer Config.ResetConfig() Config.SetConfig("lfs.url", server.URL+"/media") _, _, wErr := Download("oid") if wErr == nil { diff --git a/lfs/upload_test.go b/lfs/upload_test.go index 5a360822..545feae0 100644 --- a/lfs/upload_test.go +++ b/lfs/upload_test.go @@ -103,6 +103,7 @@ func TestExistingUpload(t *testing.T) { w.WriteHeader(200) }) + defer Config.ResetConfig() Config.SetConfig("lfs.url", server.URL+"/media") oidPath, _ := LocalMediaPath("988881adc9fc3655077dc2d4d757d480b5ea0e11") @@ -226,6 +227,7 @@ func TestUploadWithRedirect(t *testing.T) { w.Write(by) }) + defer Config.ResetConfig() Config.SetConfig("lfs.url", server.URL+"/redirect") oidPath, _ := LocalMediaPath("988881adc9fc3655077dc2d4d757d480b5ea0e11") @@ -399,6 +401,7 @@ func TestSuccessfulUploadWithVerify(t *testing.T) { w.WriteHeader(200) }) + defer Config.ResetConfig() Config.SetConfig("lfs.url", server.URL+"/media") oidPath, _ := LocalMediaPath("988881adc9fc3655077dc2d4d757d480b5ea0e11") @@ -559,6 +562,7 @@ func TestSuccessfulUploadWithoutVerify(t *testing.T) { w.WriteHeader(200) }) + defer Config.ResetConfig() Config.SetConfig("lfs.url", server.URL+"/media") oidPath, _ := LocalMediaPath("988881adc9fc3655077dc2d4d757d480b5ea0e11") @@ -603,6 +607,7 @@ func TestUploadApiError(t *testing.T) { w.WriteHeader(404) }) + defer Config.ResetConfig() Config.SetConfig("lfs.url", server.URL+"/media") oidPath, _ := LocalMediaPath("988881adc9fc3655077dc2d4d757d480b5ea0e11") @@ -710,6 +715,7 @@ func TestUploadStorageError(t *testing.T) { w.WriteHeader(404) }) + defer Config.ResetConfig() Config.SetConfig("lfs.url", server.URL+"/media") oidPath, _ := LocalMediaPath("988881adc9fc3655077dc2d4d757d480b5ea0e11") @@ -858,6 +864,7 @@ func TestUploadVerifyError(t *testing.T) { w.WriteHeader(404) }) + defer Config.ResetConfig() Config.SetConfig("lfs.url", server.URL+"/media") oidPath, _ := LocalMediaPath("988881adc9fc3655077dc2d4d757d480b5ea0e11") From c3996ef52715d296fbf4bcbec2b8972f8550cfff Mon Sep 17 00:00:00 2001 From: Rick Olson Date: Fri, 28 Aug 2015 16:27:40 -0600 Subject: [PATCH 14/15] setup a reusable test matrix --- lfs/credentials.go | 2 +- lfs/credentials_test.go | 348 ++++++++++++++++++++++------------------ 2 files changed, 192 insertions(+), 158 deletions(-) diff --git a/lfs/credentials.go b/lfs/credentials.go index 8aada1b4..78d3de2e 100644 --- a/lfs/credentials.go +++ b/lfs/credentials.go @@ -47,7 +47,7 @@ func getCredsForAPI(req *http.Request) (Creds, error) { } creds, err := fillCredentials(credsUrl) - if err != nil { + if err != nil || creds == nil { return nil, err } diff --git a/lfs/credentials_test.go b/lfs/credentials_test.go index 0d768979..e5df1a23 100644 --- a/lfs/credentials_test.go +++ b/lfs/credentials_test.go @@ -2,180 +2,214 @@ package lfs import ( "encoding/base64" + "fmt" "net/http" "testing" ) -func TestGetCredentialsForAPI(t *testing.T) { - defer Config.ResetConfig() - Config.SetConfig("lfs.url", "https://lfs-server.com") - req, err := http.NewRequest("GET", "https://lfs-server.com/foo", nil) - if err != nil { - t.Fatal(err) +func TestGetCredentialsForApi(t *testing.T) { + checkGetCredentials(t, getCredsForAPI, []*getCredentialCheck{ + { + Desc: "simple", + Config: map[string]string{"lfs.url": "https://git-server.com"}, + Method: "GET", + Href: "https://git-server.com/foo", + Protocol: "https", + Host: "git-server.com", + Username: "git-server.com", + Password: "monkey", + }, + { + Desc: "auth header", + Config: map[string]string{"lfs.url": "https://git-server.com"}, + Header: map[string]string{"Authorization": "Test monkey"}, + Method: "GET", + Href: "https://git-server.com/foo", + Authorization: "Test monkey", + }, + { + Desc: "scheme mismatch", + Config: map[string]string{"lfs.url": "https://git-server.com"}, + Method: "GET", + Href: "http://git-server.com/foo", + Protocol: "http", + Host: "git-server.com", + Path: "foo", + Username: "git-server.com", + Password: "monkey", + }, + { + Desc: "host mismatch", + Config: map[string]string{"lfs.url": "https://git-server.com"}, + Method: "GET", + Href: "https://git-server2.com/foo", + Protocol: "https", + Host: "git-server2.com", + Path: "foo", + Username: "git-server2.com", + Password: "monkey", + }, + { + Desc: "port mismatch", + Config: map[string]string{"lfs.url": "https://git-server.com"}, + Method: "GET", + Href: "https://git-server.com:8080/foo", + Protocol: "https", + Host: "git-server.com:8080", + Path: "foo", + Username: "git-server.com:8080", + Password: "monkey", + }, + { + Desc: "api url auth", + Config: map[string]string{"lfs.url": "https://testuser:testpass@git-server.com"}, + Method: "GET", + Href: "https://git-server.com/foo", + Authorization: "Basic " + base64.URLEncoding.EncodeToString([]byte("testuser:testpass")), + }, + { + Desc: "git url auth", + CurrentRemote: "origin", + Config: map[string]string{ + "lfs.url": "https://git-server.com", + "remote.origin.url": "https://gituser:gitpass@git-server.com", + }, + Method: "GET", + Href: "https://git-server.com/foo", + Authorization: "Basic " + base64.URLEncoding.EncodeToString([]byte("gituser:gitpass")), + }, + }) +} + +func TestGetCredentials(t *testing.T) { + checks := []*getCredentialCheck{ + { + Desc: "git server", + Method: "GET", + Href: "https://git-server.com/foo", + Protocol: "https", + Host: "git-server.com", + Username: "git-server.com", + Password: "monkey", + }, + { + Desc: "separate lfs server", + Method: "GET", + Href: "https://lfs-server.com/foo", + Protocol: "https", + Host: "lfs-server.com", + Username: "lfs-server.com", + Password: "monkey", + }, } - creds, err := getCredsForAPI(req) - if err != nil { - t.Fatal(err) + // these properties should not change the outcome + for _, check := range checks { + check.CurrentRemote = "origin" + check.Config = map[string]string{ + "lfs.url": "https://testuser:testuser@git-server.com", + "remote.origin.url": "https://gituser:gitpass@git-server.com", + } } - if value := creds["username"]; value != "lfs-server.com" { - t.Errorf("bad username: %s", value) - } + checkGetCredentials(t, getCreds, checks) +} - if value := creds["password"]; value != "monkey" { - t.Errorf("bad password: %s", value) - } +func checkGetCredentials(t *testing.T, getCredsFunc func(*http.Request) (Creds, error), checks []*getCredentialCheck) { + existingRemote := Config.CurrentRemote + for _, check := range checks { + t.Logf("Checking %q", check.Desc) + Config.CurrentRemote = check.CurrentRemote - expected := "Basic " + base64.URLEncoding.EncodeToString([]byte("lfs-server.com:monkey")) - if value := req.Header.Get("Authorization"); value != expected { - t.Errorf("Bad Authorization. Expected '%s', got '%s'", expected, value) + for key, value := range check.Config { + Config.SetConfig(key, value) + } + + req, err := http.NewRequest(check.Method, check.Href, nil) + if err != nil { + t.Errorf("[%s] %s", check.Desc, err) + continue + } + + for key, value := range check.Header { + req.Header.Set(key, value) + } + + creds, err := getCredsFunc(req) + if err != nil { + t.Errorf("[%s] %s", check.Desc, err) + continue + } + + if check.ExpectCreds() { + if creds == nil { + t.Errorf("[%s], no credentials returned", check.Desc) + continue + } + + if value := creds["protocol"]; len(check.Protocol) > 0 && value != check.Protocol { + t.Errorf("[%s] bad protocol: %q, expected: %q", check.Desc, value, check.Protocol) + } + + if value := creds["host"]; len(check.Host) > 0 && value != check.Host { + t.Errorf("[%s] bad host: %q, expected: %q", check.Desc, value, check.Host) + } + + if value := creds["username"]; len(check.Username) > 0 && value != check.Username { + t.Errorf("[%s] bad username: %q, expected: %q", check.Desc, value, check.Username) + } + + if value := creds["password"]; len(check.Password) > 0 && value != check.Password { + t.Errorf("[%s] bad password: %q, expected: %q", check.Desc, value, check.Password) + } + + if value := creds["path"]; len(check.Path) > 0 && value != check.Path { + t.Errorf("[%s] bad path: %q, expected: %q", check.Desc, value, check.Path) + } + } else { + if creds != nil { + t.Errorf("[%s], unexpected credentials: %v // %v", check.Desc, creds, check) + continue + } + } + + if len(check.Authorization) > 0 { + if actual := req.Header.Get("Authorization"); actual != check.Authorization { + t.Errorf("[%s] Unexpected Authorization header: %s", check.Desc, actual) + } + } else { + rawtoken := fmt.Sprintf("%s:%s", check.Username, check.Password) + expected := "Basic " + base64.URLEncoding.EncodeToString([]byte(rawtoken)) + if value := req.Header.Get("Authorization"); value != expected { + t.Errorf("[%s] Bad Authorization. Expected '%s', got '%s'", check.Desc, expected, value) + } + } + + Config.ResetConfig() + Config.CurrentRemote = existingRemote } } -func TestGetCredentialsForAPIWithExistingAuthorization(t *testing.T) { - defer Config.ResetConfig() - Config.SetConfig("lfs.url", "https://lfs-server.com") - req, err := http.NewRequest("GET", "http://lfs-server.com/foo", nil) - if err != nil { - t.Fatal(err) - } - - req.Header.Set("Authorization", "Test monkey") - - creds, err := getCredsForAPI(req) - if err != nil { - t.Fatal(err) - } - - if creds != nil { - t.Errorf("Unexpected creds: %v", creds) - } - - if actual := req.Header.Get("Authorization"); actual != "Test monkey" { - t.Errorf("Unexpected Authorization header: %s", actual) - } +type getCredentialCheck struct { + Desc string + Config map[string]string + Header map[string]string + Method string + Href string + Protocol string + Host string + Username string + Password string + Path string + Authorization string + CurrentRemote string } -func TestGetCredentialsForAPIWithSchemeMismatch(t *testing.T) { - defer Config.ResetConfig() - Config.SetConfig("lfs.url", "https://lfs-server.com") - req, err := http.NewRequest("GET", "http://lfs-server.com/foo", nil) - if err != nil { - t.Fatal(err) - } - - creds, err := getCredsForAPI(req) - if err != nil { - t.Fatal(err) - } - - if creds == nil { - t.Fatalf("no credentials returned") - } - - if v := creds["protocol"]; v != "http" { - t.Errorf("Invalid protocol: %q", v) - } - - if v := creds["host"]; v != "lfs-server.com" { - t.Errorf("Invalid host: %q", v) - } - - if v := creds["path"]; v != "foo" { - t.Errorf("Invalid path: %q", v) - } - - expected := "Basic " + base64.URLEncoding.EncodeToString([]byte("lfs-server.com:monkey")) - if value := req.Header.Get("Authorization"); value != expected { - t.Errorf("Bad Authorization. Expected '%s', got '%s'", expected, value) - } +func (c *getCredentialCheck) ExpectCreds() bool { + return len(c.Protocol) > 0 || len(c.Host) > 0 || len(c.Username) > 0 || + len(c.Password) > 0 || len(c.Path) > 0 } -func TestGetCredentialsForAPIWithHostMismatch(t *testing.T) { - defer Config.ResetConfig() - Config.SetConfig("lfs.url", "https://lfs-server.com") - req, err := http.NewRequest("GET", "https://lfs-server2.com/foo", nil) - if err != nil { - t.Fatal(err) - } - - creds, err := getCredsForAPI(req) - if err != nil { - t.Fatal(err) - } - - if v := creds["protocol"]; v != "https" { - t.Errorf("Invalid protocol: %q", v) - } - - if v := creds["host"]; v != "lfs-server2.com" { - t.Errorf("Invalid host: %q", v) - } - - if v := creds["path"]; v != "foo" { - t.Errorf("Invalid path: %q", v) - } - - expected := "Basic " + base64.URLEncoding.EncodeToString([]byte("lfs-server2.com:monkey")) - if value := req.Header.Get("Authorization"); value != expected { - t.Errorf("Bad Authorization. Expected '%s', got '%s'", expected, value) - } -} - -func TestGetCredentialsForAPIWithPortMismatch(t *testing.T) { - defer Config.ResetConfig() - Config.SetConfig("lfs.url", "https://lfs-server.com") - req, err := http.NewRequest("GET", "https://lfs-server.com:8080/foo", nil) - if err != nil { - t.Fatal(err) - } - - creds, err := getCredsForAPI(req) - if err != nil { - t.Fatal(err) - } - - if v := creds["protocol"]; v != "https" { - t.Errorf("Invalid protocol: %q", v) - } - - if v := creds["host"]; v != "lfs-server.com:8080" { - t.Errorf("Invalid host: %q", v) - } - - if v := creds["path"]; v != "foo" { - t.Errorf("Invalid path: %q", v) - } - - expected := "Basic " + base64.URLEncoding.EncodeToString([]byte("lfs-server.com:8080:monkey")) - if value := req.Header.Get("Authorization"); value != expected { - t.Errorf("Bad Authorization. Expected '%s', got '%s'", expected, value) - } -} - -func TestGetCredentialsForAPIWithRfc1738UsernameAndPassword(t *testing.T) { - defer Config.ResetConfig() - Config.SetConfig("lfs.url", "https://testuser:testpass@lfs-server.com") - req, err := http.NewRequest("GET", "https://lfs-server.com/foo", nil) - if err != nil { - t.Fatal(err) - } - - creds, err := getCredsForAPI(req) - if err != nil { - t.Fatal(err) - } - - if creds != nil { - t.Errorf("unexpected creds: %v", creds) - } - - expected := "Basic " + base64.URLEncoding.EncodeToString([]byte("testuser:testpass")) - if value := req.Header.Get("Authorization"); value != expected { - t.Errorf("Bad Authorization. Expected '%s', got '%s'", expected, value) - } -} func init() { execCreds = func(input Creds, subCommand string) (Creds, error) { output := make(Creds) From 71813f6de113c0004d98e2647fa7529d0e39698d Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Tue, 1 Sep 2015 12:49:00 -0600 Subject: [PATCH 15/15] push the error wrapping down to the lower level credential functions --- lfs/client.go | 4 ++-- lfs/credentials.go | 19 +++++++++++++------ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/lfs/client.go b/lfs/client.go index f877907b..9e7ccac4 100644 --- a/lfs/client.go +++ b/lfs/client.go @@ -396,7 +396,7 @@ func doApiBatchRequest(req *http.Request) (*http.Response, []*objectResource, er func doStorageRequest(req *http.Request) (*http.Response, error) { creds, err := getCreds(req) if err != nil { - return nil, Error(err) + return nil, err } return doHttpRequest(req, creds) @@ -450,7 +450,7 @@ func doApiRequestWithRedirects(req *http.Request, via []*http.Request, useCreds if useCreds { c, err := getCredsForAPI(req) if err != nil { - return nil, Error(err) + return nil, err } creds = c } diff --git a/lfs/credentials.go b/lfs/credentials.go index 78d3de2e..382906d7 100644 --- a/lfs/credentials.go +++ b/lfs/credentials.go @@ -19,7 +19,7 @@ func getCreds(req *http.Request) (Creds, error) { creds, err := fillCredentials(req.URL) if err != nil { - return nil, err + return nil, Error(err) } setRequestAuth(req, creds["username"], creds["password"]) @@ -42,16 +42,23 @@ func getCredsForAPI(req *http.Request) (Creds, error) { } credsUrl, err := getCredURLForAPI(req) - if err != nil || credsUrl == nil { - return nil, err + if err != nil { + return nil, Error(err) + } + + if credsUrl == nil { + return nil, nil } creds, err := fillCredentials(credsUrl) - if err != nil || creds == nil { - return nil, err + if err != nil { + return nil, Error(err) + } + + if creds != nil { + setRequestAuth(req, creds["username"], creds["password"]) } - setRequestAuth(req, creds["username"], creds["password"]) return creds, nil }