Rewrite of the LFS server (#15523)

* Restructured code. Moved static checks out of loop.

* Restructured batch api. Add support for individual errors.

* Let router decide if LFS is enabled.

* Renamed methods.

* Return correct status from verify handler.

* Unified media type check in router.

* Changed error code according to spec.

* Moved checks into router.

* Removed invalid v1 api methods.

* Unified methods.

* Display better error messages.

* Added size parameter. Create meta object on upload.

* Use object error on invalid size.

* Skip upload if object exists.

* Moved methods.

* Suppress fields in response.

* Changed error on accept.

* Added tests.

* Use ErrorResponse object.

* Test against message property.

* Add support for the old invalid lfs client.

* Fixed the check because MinIO wraps the error.

* Use individual repositories.

Co-authored-by: 6543 <6543@obermui.de>
Co-authored-by: Lauris BH <lauris@nix.lv>

integrations/api_repo_lfs_locks_test.go

@@ -11,6 +11,7 @@ import (
 	"time"
 
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/modules/lfs"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 
@@ -40,7 +41,7 @@ func TestAPILFSLocksNotLogin(t *testing.T) {
 	repo := models.AssertExistsAndLoadBean(t, &models.Repository{ID: 1}).(*models.Repository)
 
 	req := NewRequestf(t, "GET", "/%s/%s.git/info/lfs/locks", user.Name, repo.Name)
-	req.Header.Set("Accept", "application/vnd.git-lfs+json")
+	req.Header.Set("Accept", lfs.MediaType)
 	resp := MakeRequest(t, req, http.StatusUnauthorized)
 	var lfsLockError api.LFSLockError
 	DecodeJSON(t, resp, &lfsLockError)
@@ -102,8 +103,8 @@ func TestAPILFSLocksLogged(t *testing.T) {
 	for _, test := range tests {
 		session := loginUser(t, test.user.Name)
 		req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/%s.git/info/lfs/locks", test.repo.FullName()), map[string]string{"path": test.path})
-		req.Header.Set("Accept", "application/vnd.git-lfs+json")
-		req.Header.Set("Content-Type", "application/vnd.git-lfs+json")
+		req.Header.Set("Accept", lfs.MediaType)
+		req.Header.Set("Content-Type", lfs.MediaType)
 		resp := session.MakeRequest(t, req, test.httpResult)
 		if len(test.addTime) > 0 {
 			var lfsLock api.LFSLockResponse
@@ -119,7 +120,7 @@ func TestAPILFSLocksLogged(t *testing.T) {
 	for _, test := range resultsTests {
 		session := loginUser(t, test.user.Name)
 		req := NewRequestf(t, "GET", "/%s.git/info/lfs/locks", test.repo.FullName())
-		req.Header.Set("Accept", "application/vnd.git-lfs+json")
+		req.Header.Set("Accept", lfs.MediaType)
 		resp := session.MakeRequest(t, req, http.StatusOK)
 		var lfsLocks api.LFSLockList
 		DecodeJSON(t, resp, &lfsLocks)
@@ -131,8 +132,8 @@ func TestAPILFSLocksLogged(t *testing.T) {
 		}
 
 		req = NewRequestWithJSON(t, "POST", fmt.Sprintf("/%s.git/info/lfs/locks/verify", test.repo.FullName()), map[string]string{})
-		req.Header.Set("Accept", "application/vnd.git-lfs+json")
-		req.Header.Set("Content-Type", "application/vnd.git-lfs+json")
+		req.Header.Set("Accept", lfs.MediaType)
+		req.Header.Set("Content-Type", lfs.MediaType)
 		resp = session.MakeRequest(t, req, http.StatusOK)
 		var lfsLocksVerify api.LFSLockListVerify
 		DecodeJSON(t, resp, &lfsLocksVerify)
@@ -155,8 +156,8 @@ func TestAPILFSLocksLogged(t *testing.T) {
 	for _, test := range deleteTests {
 		session := loginUser(t, test.user.Name)
 		req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/%s.git/info/lfs/locks/%s/unlock", test.repo.FullName(), test.lockID), map[string]string{})
-		req.Header.Set("Accept", "application/vnd.git-lfs+json")
-		req.Header.Set("Content-Type", "application/vnd.git-lfs+json")
+		req.Header.Set("Accept", lfs.MediaType)
+		req.Header.Set("Content-Type", lfs.MediaType)
 		resp := session.MakeRequest(t, req, http.StatusOK)
 		var lfsLockRep api.LFSLockResponse
 		DecodeJSON(t, resp, &lfsLockRep)
@@ -168,7 +169,7 @@ func TestAPILFSLocksLogged(t *testing.T) {
 	for _, test := range resultsTests {
 		session := loginUser(t, test.user.Name)
 		req := NewRequestf(t, "GET", "/%s.git/info/lfs/locks", test.repo.FullName())
-		req.Header.Set("Accept", "application/vnd.git-lfs+json")
+		req.Header.Set("Accept", lfs.MediaType)
 		resp := session.MakeRequest(t, req, http.StatusOK)
 		var lfsLocks api.LFSLockList
 		DecodeJSON(t, resp, &lfsLocks)

integrations/lfs_getobject_test.go

@@ -17,25 +17,16 @@ import (
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/routers/routes"
 
+	jsoniter "github.com/json-iterator/go"
 	gzipp "github.com/klauspost/compress/gzip"
 	"github.com/stretchr/testify/assert"
 )
 
-var lfsID = int64(20000)
-
 func storeObjectInRepo(t *testing.T, repositoryID int64, content *[]byte) string {
 	pointer, err := lfs.GeneratePointer(bytes.NewReader(*content))
 	assert.NoError(t, err)
-	var lfsMetaObject *models.LFSMetaObject
 
-	if setting.Database.UsePostgreSQL {
-		lfsMetaObject = &models.LFSMetaObject{ID: lfsID, Pointer: pointer, RepositoryID: repositoryID}
-	} else {
-		lfsMetaObject = &models.LFSMetaObject{Pointer: pointer, RepositoryID: repositoryID}
-	}
-
-	lfsID++
-	lfsMetaObject, err = models.NewLFSMetaObject(lfsMetaObject)
+	_, err = models.NewLFSMetaObject(&models.LFSMetaObject{Pointer: pointer, RepositoryID: repositoryID})
 	assert.NoError(t, err)
 	contentStore := lfs.NewContentStore()
 	exist, err := contentStore.Exists(pointer)
@@ -210,7 +201,14 @@ func TestGetLFSRange(t *testing.T) {
 				"Range": []string{tt.in},
 			}
 			resp := storeAndGetLfs(t, &content, &h, tt.status)
-			assert.Equal(t, tt.out, resp.Body.String())
+			if tt.status == http.StatusPartialContent || tt.status == http.StatusOK {
+				assert.Equal(t, tt.out, resp.Body.String())
+			} else {
+				var er lfs.ErrorResponse
+				err := jsoniter.Unmarshal(resp.Body.Bytes(), &er)
+				assert.NoError(t, err)
+				assert.Equal(t, tt.out, er.Message)
+			}
 		})
 	}
 }

modules/lfs/shared.go

@@ -45,7 +45,7 @@ type BatchResponse struct {
 // ObjectResponse is object metadata as seen by clients of the LFS server.
 type ObjectResponse struct {
 	Pointer
-	Actions map[string]*Link `json:"actions"`
+	Actions map[string]*Link `json:"actions,omitempty"`
 	Error   *ObjectError     `json:"error,omitempty"`
 }
 
@@ -53,7 +53,7 @@ type ObjectResponse struct {
 type Link struct {
 	Href      string            `json:"href"`
 	Header    map[string]string `json:"header,omitempty"`
-	ExpiresAt time.Time         `json:"expires_at,omitempty"`
+	ExpiresAt *time.Time        `json:"expires_at,omitempty"`
 }
 
 // ObjectError defines the JSON structure returned to the client in case of an error
@@ -67,3 +67,10 @@ type PointerBlob struct {
 	Hash string
 	Pointer
 }
+
+// ErrorResponse describes the error to the client.
+type ErrorResponse struct {
+	Message          string
+	DocumentationURL string `json:"documentation_url,omitempty"`
+	RequestID        string `json:"request_id,omitempty"`
+}

routers/routes/web.go

@@ -286,6 +286,13 @@ func RegisterRoutes(m *web.Route) {
 		}
 	}
 
+	lfsServerEnabled := func(ctx *context.Context) {
+		if !setting.LFS.StartServer {
+			ctx.Error(http.StatusNotFound)
+			return
+		}
+	}
+
 	// FIXME: not all routes need go through same middleware.
 	// Especially some AJAX requests, we can reduce middleware number to improve performance.
 	// Routers.
@@ -1042,21 +1049,21 @@ func RegisterRoutes(m *web.Route) {
 
 		m.Group("/{reponame}", func() {
 			m.Group("/info/lfs", func() {
-				m.Post("/objects/batch", lfs.BatchHandler)
-				m.Get("/objects/{oid}/{filename}", lfs.ObjectOidHandler)
-				m.Any("/objects/{oid}", lfs.ObjectOidHandler)
-				m.Post("/objects", lfs.PostHandler)
-				m.Post("/verify", lfs.VerifyHandler)
+				m.Post("/objects/batch", lfs.CheckAcceptMediaType, lfs.BatchHandler)
+				m.Put("/objects/{oid}/{size}", lfs.UploadHandler)
+				m.Get("/objects/{oid}/{filename}", lfs.DownloadHandler)
+				m.Get("/objects/{oid}", lfs.DownloadHandler)
+				m.Post("/verify", lfs.CheckAcceptMediaType, lfs.VerifyHandler)
 				m.Group("/locks", func() {
 					m.Get("/", lfs.GetListLockHandler)
 					m.Post("/", lfs.PostLockHandler)
 					m.Post("/verify", lfs.VerifyLockHandler)
 					m.Post("/{lid}/unlock", lfs.UnLockHandler)
-				})
+				}, lfs.CheckAcceptMediaType)
 				m.Any("/*", func(ctx *context.Context) {
 					ctx.NotFound("", nil)
 				})
-			}, ignSignInAndCsrf)
+			}, ignSignInAndCsrf, lfsServerEnabled)
 
 			m.Group("", func() {
 				m.Post("/git-upload-pack", repo.ServiceUploadPack)

services/lfs/locks.go

@@ -19,21 +19,6 @@ import (
 	jsoniter "github.com/json-iterator/go"
 )
 
-//checkIsValidRequest check if it a valid request in case of bad request it write the response to ctx.
-func checkIsValidRequest(ctx *context.Context) bool {
-	if !setting.LFS.StartServer {
-		log.Debug("Attempt to access LFS server but LFS server is disabled")
-		writeStatus(ctx, http.StatusNotFound)
-		return false
-	}
-	if !MetaMatcher(ctx.Req) {
-		log.Info("Attempt access LOCKs without accepting the correct media type: %s", lfs_module.MediaType)
-		writeStatus(ctx, http.StatusBadRequest)
-		return false
-	}
-	return true
-}
-
 func handleLockListOut(ctx *context.Context, repo *models.Repository, lock *models.LFSLock, err error) {
 	if err != nil {
 		if models.IsErrLFSLockNotExist(err) {
@@ -60,12 +45,7 @@ func handleLockListOut(ctx *context.Context, repo *models.Repository, lock *mode
 
 // GetListLockHandler list locks
 func GetListLockHandler(ctx *context.Context) {
-	if !checkIsValidRequest(ctx) {
-		// Status is written in checkIsValidRequest
-		return
-	}
-
-	rv, _ := unpack(ctx)
+	rv := getRequestContext(ctx)
 
 	repository, err := models.GetRepositoryByOwnerAndName(rv.User, rv.Repo)
 	if err != nil {
@@ -150,11 +130,6 @@ func GetListLockHandler(ctx *context.Context) {
 
 // PostLockHandler create lock
 func PostLockHandler(ctx *context.Context) {
-	if !checkIsValidRequest(ctx) {
-		// Status is written in checkIsValidRequest
-		return
-	}
-
 	userName := ctx.Params("username")
 	repoName := strings.TrimSuffix(ctx.Params("reponame"), ".git")
 	authorization := ctx.Req.Header.Get("Authorization")
@@ -223,11 +198,6 @@ func PostLockHandler(ctx *context.Context) {
 
 // VerifyLockHandler list locks for verification
 func VerifyLockHandler(ctx *context.Context) {
-	if !checkIsValidRequest(ctx) {
-		// Status is written in checkIsValidRequest
-		return
-	}
-
 	userName := ctx.Params("username")
 	repoName := strings.TrimSuffix(ctx.Params("reponame"), ".git")
 	authorization := ctx.Req.Header.Get("Authorization")
@@ -294,11 +264,6 @@ func VerifyLockHandler(ctx *context.Context) {
 
 // UnLockHandler delete locks
 func UnLockHandler(ctx *context.Context) {
-	if !checkIsValidRequest(ctx) {
-		// Status is written in checkIsValidRequest
-		return
-	}
-
 	userName := ctx.Params("username")
 	repoName := strings.TrimSuffix(ctx.Params("reponame"), ".git")
 	authorization := ctx.Req.Header.Get("Authorization")