Implement changes requested in feedback. Add download object test.

lfs.go

@@ -13,7 +13,6 @@ import (
 	"net/http"
 	"os"
 	"path/filepath"
-	"strconv"
 )
 
 var (
@@ -34,7 +33,7 @@ func lfsAuthorizeHandler(handleFunc serviceHandleFunc) serviceHandleFunc {
 			return
 		}
 
-		if r.LfsSize == "" {
+		if r.LfsSize == 0 {
 			fail500(w, "lfsAuthorizeHandler", errors.New("Lfs object size not specified."))
 			return
 		}
@@ -48,62 +47,47 @@ func lfsAuthorizeHandler(handleFunc serviceHandleFunc) serviceHandleFunc {
 		}
 
 		handleFunc(w, r)
-	}, "")
+	}, "/authorize")
 }
 
-func handleStoreLfsObject(handleFunc serviceHandleFunc) serviceHandleFunc {
-	return func(w http.ResponseWriter, r *gitRequest) {
-		oid := r.LfsOid
-		size := r.LfsSize
+func handleStoreLfsObject(w http.ResponseWriter, r *gitRequest) {
+	var body io.ReadCloser
 
-		var body io.ReadCloser
+	body = r.Body
+	defer body.Close()
 
-		body = r.Body
-		defer body.Close()
-
-		tmpPath := r.StoreLFSPath
-		file, err := ioutil.TempFile(tmpPath, "")
-		if err != nil {
-			fail500(w, "Couldn't open tmp file for writing.", err)
-			return
-		}
-		defer os.Remove(tmpPath)
-		defer file.Close()
-
-		hash := sha256.New()
-		hw := io.MultiWriter(hash, file)
-
-		written, err := io.Copy(hw, body)
-		if err != nil {
-			fail500(w, "Failed to save received LFS object.", err)
-			return
-		}
-		file.Close()
+	tmpPath := r.StoreLFSPath
+	file, err := ioutil.TempFile(tmpPath, r.LfsOid)
+	if err != nil {
+		fail500(w, "Couldn't open tmp file for writing.", err)
+		return
+	}
+	defer os.Remove(tmpPath)
+	defer file.Close()
 
-		sizeInt, err := strconv.ParseInt(size, 10, 64)
-		if err != nil {
-			fail500(w, "Couldn't read size: ", err)
-			return
-		}
+	hash := sha256.New()
+	hw := io.MultiWriter(hash, file)
 
-		if written != sizeInt {
-			fail500(w, "Inconsistent size: ", errSizeMismatch)
-			return
-		}
+	written, err := io.Copy(hw, body)
+	if err != nil {
+		fail500(w, "Failed to save received LFS object.", err)
+		return
+	}
+	file.Close()
 
-		shaStr := hex.EncodeToString(hash.Sum(nil))
-		if shaStr != oid {
-			fail500(w, "Inconsistent size: ", errSizeMismatch)
-			return
-		}
-		r.Header.Set("X-GitLab-Lfs-Tmp", filepath.Base(file.Name()))
+	if written != r.LfsSize {
+		fail500(w, "Inconsistent size: ", errSizeMismatch)
+		return
+	}
 
-		handleFunc(w, r)
+	shaStr := hex.EncodeToString(hash.Sum(nil))
+	if shaStr != r.LfsOid {
+		fail500(w, "Inconsistent size: ", errSizeMismatch)
+		return
 	}
-}
+	r.Header.Set("X-GitLab-Lfs-Tmp", filepath.Base(file.Name()))
 
-func lfsCallback(w http.ResponseWriter, r *gitRequest) {
-	authReq, err := r.u.newUpstreamRequest(r.Request, nil, "/authorize")
+	authReq, err := r.u.newUpstreamRequest(r.Request, nil, "")
 	if err != nil {
 		fail500(w, "newUpstreamRequestlfsCallback", err)
 		return
@@ -115,5 +99,6 @@ func lfsCallback(w http.ResponseWriter, r *gitRequest) {
 		return
 	}
 	defer authResponse.Body.Close()
+
 	return
 }

main_test.go

@@ -241,69 +241,18 @@ func TestDownloadCacheCreate(t *testing.T) {
 
 func TestAllowedXSendfileDownload(t *testing.T) {
 	contentFilename := "my-content"
-	contentPath := path.Join(cacheDir, contentFilename)
+	url := fmt.Sprintf("http://%s/foo/uploads/bar", servAddr)
 	prepareDownloadDir(t)
 
-	// Prepare test server and backend
-	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if xSendfileType := r.Header.Get("X-Sendfile-Type"); xSendfileType != "X-Sendfile" {
-			t.Fatalf(`X-Sendfile-Type want "X-Sendfile" got %q`, xSendfileType)
-		}
-		w.Header().Set("X-Sendfile", contentPath)
-		w.Header().Set("Content-Disposition", fmt.Sprintf(`attachment; filename="%s"`, contentFilename))
-		w.WriteHeader(200)
-	}))
-	defer ts.Close()
-	defer cleanUpProcessGroup(startServerOrFail(t, ts))
-
-	if err := os.MkdirAll(cacheDir, 0755); err != nil {
-		t.Fatal(err)
-	}
-	contentBytes := []byte{'c', 'o', 'n', 't', 'e', 'n', 't'}
-	if err := ioutil.WriteFile(contentPath, contentBytes, 0644); err != nil {
-		t.Fatal(err)
-	}
-
-	downloadCmd := exec.Command("curl", "-J", "-O", fmt.Sprintf("http://%s/foo/uploads/bar", servAddr))
-	downloadCmd.Dir = scratchDir
-	runOrFail(t, downloadCmd)
-
-	actual, err := ioutil.ReadFile(path.Join(scratchDir, contentFilename))
-	if err != nil {
-		t.Fatal(err)
-	}
-	if bytes.Compare(actual, contentBytes) != 0 {
-		t.Fatal("Unexpected file contents in download")
-	}
+	allowedXSendfileDownload(t, contentFilename, url)
 }
 
 func TestDeniedXSendfileDownload(t *testing.T) {
 	contentFilename := "my-content"
+	url := fmt.Sprintf("http://%s/foo/uploads/bar", servAddr)
 	prepareDownloadDir(t)
 
-	// Prepare test server and backend
-	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if xSendfileType := r.Header.Get("X-Sendfile-Type"); xSendfileType != "X-Sendfile" {
-			t.Fatalf(`X-Sendfile-Type want "X-Sendfile" got %q`, xSendfileType)
-		}
-		w.Header().Set("Content-Disposition", fmt.Sprintf(`attachment; filename="%s"`, contentFilename))
-		w.WriteHeader(200)
-		fmt.Fprint(w, "Denied")
-	}))
-	defer ts.Close()
-	defer cleanUpProcessGroup(startServerOrFail(t, ts))
-
-	downloadCmd := exec.Command("curl", "-J", "-O", fmt.Sprintf("http://%s/foo/uploads/bar", servAddr))
-	downloadCmd.Dir = scratchDir
-	runOrFail(t, downloadCmd)
-
-	actual, err := ioutil.ReadFile(path.Join(scratchDir, contentFilename))
-	if err != nil {
-		t.Fatal(err)
-	}
-	if bytes.Compare(actual, []byte{'D', 'e', 'n', 'i', 'e', 'd'}) != 0 {
-		t.Fatal("Unexpected file contents in download")
-	}
+	deniedXSendfileDownload(t, contentFilename, url)
 }
 
 func prepareDownloadDir(t *testing.T) {
@@ -401,3 +350,85 @@ func repoPath(t *testing.T) string {
 	}
 	return path.Join(cwd, testRepoRoot, testRepo)
 }
+
+func TestDeniedLfsDownload(t *testing.T) {
+	contentFilename := "b68143e6463773b1b6c6fd009a76c32aeec041faff32ba2ed42fd7f708a17f80"
+	url := fmt.Sprintf("http://%s/gitlab-lfs/objects/%s", servAddr, contentFilename)
+
+	prepareDownloadDir(t)
+	deniedXSendfileDownload(t, contentFilename, url)
+}
+
+func TestAllowedLfsDownload(t *testing.T) {
+	contentFilename := "b68143e6463773b1b6c6fd009a76c32aeec041faff32ba2ed42fd7f708a17f80"
+	url := fmt.Sprintf("http://%s/gitlab-lfs/objects/%s", servAddr, contentFilename)
+
+	prepareDownloadDir(t)
+	allowedXSendfileDownload(t, contentFilename, url)
+}
+
+func allowedXSendfileDownload(t *testing.T, contentFilename string, url string) {
+	contentPath := path.Join(cacheDir, contentFilename)
+	prepareDownloadDir(t)
+
+	// Prepare test server and backend
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if xSendfileType := r.Header.Get("X-Sendfile-Type"); xSendfileType != "X-Sendfile" {
+			t.Fatalf(`X-Sendfile-Type want "X-Sendfile" got %q`, xSendfileType)
+		}
+		w.Header().Set("X-Sendfile", contentPath)
+		w.Header().Set("Content-Disposition", fmt.Sprintf(`attachment; filename="%s"`, contentFilename))
+		w.Header().Set("Content-Type", fmt.Sprintf(`application/octet-stream`))
+		w.WriteHeader(200)
+	}))
+	defer ts.Close()
+	defer cleanUpProcessGroup(startServerOrFail(t, ts))
+
+	if err := os.MkdirAll(cacheDir, 0755); err != nil {
+		t.Fatal(err)
+	}
+	contentBytes := []byte{'c', 'o', 'n', 't', 'e', 'n', 't'}
+	if err := ioutil.WriteFile(contentPath, contentBytes, 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	downloadCmd := exec.Command("curl", "-J", "-O", url)
+	downloadCmd.Dir = scratchDir
+	runOrFail(t, downloadCmd)
+
+	actual, err := ioutil.ReadFile(path.Join(scratchDir, contentFilename))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if bytes.Compare(actual, contentBytes) != 0 {
+		t.Fatal("Unexpected file contents in download")
+	}
+}
+
+func deniedXSendfileDownload(t *testing.T, contentFilename string, url string) {
+	prepareDownloadDir(t)
+
+	// Prepare test server and backend
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if xSendfileType := r.Header.Get("X-Sendfile-Type"); xSendfileType != "X-Sendfile" {
+			t.Fatalf(`X-Sendfile-Type want "X-Sendfile" got %q`, xSendfileType)
+		}
+		w.Header().Set("Content-Disposition", fmt.Sprintf(`attachment; filename="%s"`, contentFilename))
+		w.WriteHeader(200)
+		fmt.Fprint(w, "Denied")
+	}))
+	defer ts.Close()
+	defer cleanUpProcessGroup(startServerOrFail(t, ts))
+
+	downloadCmd := exec.Command("curl", "-J", "-O", url)
+	downloadCmd.Dir = scratchDir
+	runOrFail(t, downloadCmd)
+
+	actual, err := ioutil.ReadFile(path.Join(scratchDir, contentFilename))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if bytes.Compare(actual, []byte{'D', 'e', 'n', 'i', 'e', 'd'}) != 0 {
+		t.Fatal("Unexpected file contents in download")
+	}
+}

upstream.go

@@ -50,7 +50,7 @@ type authorizationResponse struct {
 	// LFS object id
 	LfsOid string
 	// LFS object size
-	LfsSize string
+	LfsSize int64
 }
 
 // A gitReqest is an *http.Request decorated with attributes returned by the
@@ -72,7 +72,7 @@ var gitServices = [...]gitService{
 	gitService{"GET", regexp.MustCompile(`/repository/archive.tar.gz\z`), repoPreAuthorizeHandler(handleGetArchive)},
 	gitService{"GET", regexp.MustCompile(`/repository/archive.tar.bz2\z`), repoPreAuthorizeHandler(handleGetArchive)},
 	gitService{"GET", regexp.MustCompile(`/uploads/`), handleSendFile},
-	gitService{"PUT", regexp.MustCompile(`/gitlab-lfs/objects/([0-9a-f]{64})/([0-9]+)\z`), lfsAuthorizeHandler(handleStoreLfsObject(lfsCallback))},
+	gitService{"PUT", regexp.MustCompile(`/gitlab-lfs/objects/([0-9a-f]{64})/([0-9]+)\z`), lfsAuthorizeHandler(handleStoreLfsObject)},
 	gitService{"GET", regexp.MustCompile(`/gitlab-lfs/objects/([0-9a-f]{64})\z`), handleSendFile},
 }