Fix SSH auth lfs locks (#3152)

* Fix SSH auth LFS locks

* Activate SSH/lock test

* Remove debug

* Follow @lunny recommendation for AfterLoad method

cmd/serv.go

@@ -259,12 +259,16 @@ func runServ(c *cli.Context) error {
 		url := fmt.Sprintf("%s%s/%s.git/info/lfs", setting.AppURL, username, repo.Name)
 
 		now := time.Now()
-		token := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{
+		claims := jwt.MapClaims{
 			"repo": repo.ID,
 			"op":   lfsVerb,
 			"exp":  now.Add(5 * time.Minute).Unix(),
 			"nbf":  now.Unix(),
-		})
+		}
+		if user != nil {
+			claims["user"] = user.ID
+		}
+		token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims)
 
 		// Sign and get the complete encoded token as a string using the secret
 		tokenString, err := token.SignedString(setting.LFS.JWTSecretBytes)

integrations/api_repo_lfs_locks_test.go

@@ -41,10 +41,10 @@ func TestAPILFSLocksNotLogin(t *testing.T) {
 
 	req := NewRequestf(t, "GET", "/%s/%s.git/info/lfs/locks", user.Name, repo.Name)
 	req.Header.Set("Accept", "application/vnd.git-lfs+json")
-	resp := MakeRequest(t, req, http.StatusForbidden)
+	resp := MakeRequest(t, req, http.StatusUnauthorized)
 	var lfsLockError api.LFSLockError
 	DecodeJSON(t, resp, &lfsLockError)
-	assert.Equal(t, "You must have pull access to list locks : User undefined doesn't have rigth to list for lfs lock [rid: 1]", lfsLockError.Message)
+	assert.Equal(t, "Unauthorized", lfsLockError.Message)
 }
 
 func TestAPILFSLocksLogged(t *testing.T) {
@@ -68,8 +68,8 @@ func TestAPILFSLocksLogged(t *testing.T) {
 		{user: user2, repo: repo1, path: "path/test", httpResult: http.StatusConflict},
 		{user: user2, repo: repo1, path: "Foo/BaR.zip", httpResult: http.StatusConflict},
 		{user: user2, repo: repo1, path: "Foo/Test/../subFOlder/../Relative/../BaR.zip", httpResult: http.StatusConflict},
-		{user: user4, repo: repo1, path: "FoO/BaR.zip", httpResult: http.StatusForbidden},
+		{user: user4, repo: repo1, path: "FoO/BaR.zip", httpResult: http.StatusUnauthorized},
-		{user: user4, repo: repo1, path: "path/test-user4", httpResult: http.StatusForbidden},
+		{user: user4, repo: repo1, path: "path/test-user4", httpResult: http.StatusUnauthorized},
 		{user: user2, repo: repo1, path: "patH/Test-user4", httpResult: http.StatusCreated, addTime: []int{0}},
 		{user: user2, repo: repo1, path: "some/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/path", httpResult: http.StatusCreated, addTime: []int{0}},
 

integrations/git_test.go

@@ -214,11 +214,9 @@ func TestGit(t *testing.T) {
 						commitAndPush(t, bigSize, dstPath)
 					})
 				})
-				/* Failed without #3152. TODO activate with fix.
 				t.Run("Locks", func(t *testing.T) {
-				  lockTest(t, u.String(), dstPath)
+					lockTest(t, u.String(), dstPath)
 				})
-				*/
 			})
 		})
 	})

models/error.go

@@ -530,21 +530,24 @@ func (err ErrLFSLockNotExist) Error() string {
 	return fmt.Sprintf("lfs lock does not exist [id: %d, rid: %d, path: %s]", err.ID, err.RepoID, err.Path)
 }
 
-// ErrLFSLockUnauthorizedAction represents a "LFSLockUnauthorizedAction" kind of error.
+// ErrLFSUnauthorizedAction represents a "LFSUnauthorizedAction" kind of error.
-type ErrLFSLockUnauthorizedAction struct {
+type ErrLFSUnauthorizedAction struct {
 	RepoID   int64
 	UserName string
-	Action   string
+	Mode     AccessMode
 }
 
-// IsErrLFSLockUnauthorizedAction checks if an error is a ErrLFSLockUnauthorizedAction.
+// IsErrLFSUnauthorizedAction checks if an error is a ErrLFSUnauthorizedAction.
-func IsErrLFSLockUnauthorizedAction(err error) bool {
+func IsErrLFSUnauthorizedAction(err error) bool {
-	_, ok := err.(ErrLFSLockUnauthorizedAction)
+	_, ok := err.(ErrLFSUnauthorizedAction)
 	return ok
 }
 
-func (err ErrLFSLockUnauthorizedAction) Error() string {
+func (err ErrLFSUnauthorizedAction) Error() string {
-	return fmt.Sprintf("User %s doesn't have rigth to %s for lfs lock [rid: %d]", err.UserName, err.Action, err.RepoID)
+	if err.Mode == AccessModeWrite {
+		return fmt.Sprintf("User %s doesn't have write access for lfs lock [rid: %d]", err.UserName, err.RepoID)
+	}
+	return fmt.Sprintf("User %s doesn't have read access for lfs lock [rid: %d]", err.UserName, err.RepoID)
 }
 
 // ErrLFSLockAlreadyExist represents a "LFSLockAlreadyExist" kind of error.

models/lfs_lock.go

@@ -11,28 +11,40 @@ import (
 	"strings"
 	"time"
 
+	"code.gitea.io/gitea/modules/log"
 	api "code.gitea.io/sdk/gitea"
+	"github.com/go-xorm/xorm"
 )
 
 // LFSLock represents a git lfs lock of repository.
 type LFSLock struct {
-	ID      int64     `xorm:"pk autoincr"`
+	ID      int64       `xorm:"pk autoincr"`
-	RepoID  int64     `xorm:"INDEX NOT NULL"`
+	Repo    *Repository `xorm:"-"`
-	Owner   *User     `xorm:"-"`
+	RepoID  int64       `xorm:"INDEX NOT NULL"`
-	OwnerID int64     `xorm:"INDEX NOT NULL"`
+	Owner   *User       `xorm:"-"`
-	Path    string    `xorm:"TEXT"`
+	OwnerID int64       `xorm:"INDEX NOT NULL"`
-	Created time.Time `xorm:"created"`
+	Path    string      `xorm:"TEXT"`
+	Created time.Time   `xorm:"created"`
 }
 
 // BeforeInsert is invoked from XORM before inserting an object of this type.
 func (l *LFSLock) BeforeInsert() {
 	l.OwnerID = l.Owner.ID
+	l.RepoID = l.Repo.ID
 	l.Path = cleanPath(l.Path)
 }
 
 // AfterLoad is invoked from XORM after setting the values of all fields of this object.
-func (l *LFSLock) AfterLoad() {
+func (l *LFSLock) AfterLoad(session *xorm.Session) {
-	l.Owner, _ = GetUserByID(l.OwnerID)
+	var err error
+	l.Owner, err = getUserByID(session, l.OwnerID)
+	if err != nil {
+		log.Error(2, "LFS lock AfterLoad failed OwnerId[%d] not found: %v", l.OwnerID, err)
+	}
+	l.Repo, err = getRepositoryByID(session, l.RepoID)
+	if err != nil {
+		log.Error(2, "LFS lock AfterLoad failed RepoId[%d] not found: %v", l.RepoID, err)
+	}
 }
 
 func cleanPath(p string) string {
@@ -53,12 +65,12 @@ func (l *LFSLock) APIFormat() *api.LFSLock {
 
 // CreateLFSLock creates a new lock.
 func CreateLFSLock(lock *LFSLock) (*LFSLock, error) {
-	err := CheckLFSAccessForRepo(lock.Owner, lock.RepoID, "create")
+	err := CheckLFSAccessForRepo(lock.Owner, lock.Repo, AccessModeWrite)
 	if err != nil {
 		return nil, err
 	}
 
-	l, err := GetLFSLock(lock.RepoID, lock.Path)
+	l, err := GetLFSLock(lock.Repo, lock.Path)
 	if err == nil {
 		return l, ErrLFSLockAlreadyExist{lock.RepoID, lock.Path}
 	}
@@ -71,15 +83,15 @@ func CreateLFSLock(lock *LFSLock) (*LFSLock, error) {
 }
 
 // GetLFSLock returns release by given path.
-func GetLFSLock(repoID int64, path string) (*LFSLock, error) {
+func GetLFSLock(repo *Repository, path string) (*LFSLock, error) {
 	path = cleanPath(path)
-	rel := &LFSLock{RepoID: repoID}
+	rel := &LFSLock{RepoID: repo.ID}
 	has, err := x.Where("lower(path) = ?", strings.ToLower(path)).Get(rel)
 	if err != nil {
 		return nil, err
 	}
 	if !has {
-		return nil, ErrLFSLockNotExist{0, repoID, path}
+		return nil, ErrLFSLockNotExist{0, repo.ID, path}
 	}
 	return rel, nil
 }
@@ -109,7 +121,7 @@ func DeleteLFSLockByID(id int64, u *User, force bool) (*LFSLock, error) {
 		return nil, err
 	}
 
-	err = CheckLFSAccessForRepo(u, lock.RepoID, "delete")
+	err = CheckLFSAccessForRepo(u, lock.Repo, AccessModeWrite)
 	if err != nil {
 		return nil, err
 	}
@@ -123,24 +135,15 @@ func DeleteLFSLockByID(id int64, u *User, force bool) (*LFSLock, error) {
 }
 
 //CheckLFSAccessForRepo check needed access mode base on action
-func CheckLFSAccessForRepo(u *User, repoID int64, action string) error {
+func CheckLFSAccessForRepo(u *User, repo *Repository, mode AccessMode) error {
 	if u == nil {
-		return ErrLFSLockUnauthorizedAction{repoID, "undefined", action}
+		return ErrLFSUnauthorizedAction{repo.ID, "undefined", mode}
-	}
-	mode := AccessModeRead
-	if action == "create" || action == "delete" || action == "verify" {
-		mode = AccessModeWrite
-	}
-
-	repo, err := GetRepositoryByID(repoID)
-	if err != nil {
-		return err
 	}
 	has, err := HasAccess(u.ID, repo, mode)
 	if err != nil {
 		return err
 	} else if !has {
-		return ErrLFSLockUnauthorizedAction{repo.ID, u.DisplayName(), action}
+		return ErrLFSUnauthorizedAction{repo.ID, u.DisplayName(), mode}
 	}
 	return nil
 }

modules/lfs/locks.go

@@ -13,24 +13,35 @@ import (
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/sdk/gitea"
-
-	"gopkg.in/macaron.v1"
 )
 
-func checkRequest(req macaron.Request, post bool) int {
+//checkIsValidRequest check if it a valid request in case of bad request it write the response to ctx.
+func checkIsValidRequest(ctx *context.Context, post bool) bool {
 	if !setting.LFS.StartServer {
-		return 404
+		writeStatus(ctx, 404)
+		return false
 	}
-	if !MetaMatcher(req) {
+	if !MetaMatcher(ctx.Req) {
-		return 400
+		writeStatus(ctx, 400)
+		return false
+	}
+	if !ctx.IsSigned {
+		user, _, _, err := parseToken(ctx.Req.Header.Get("Authorization"))
+		if err != nil {
+			ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs")
+			writeStatus(ctx, 401)
+			return false
+		}
+		ctx.User = user
 	}
 	if post {
-		mediaParts := strings.Split(req.Header.Get("Content-Type"), ";")
+		mediaParts := strings.Split(ctx.Req.Header.Get("Content-Type"), ";")
 		if mediaParts[0] != metaMediaType {
-			return 400
+			writeStatus(ctx, 400)
+			return false
 		}
 	}
-	return 200
+	return true
 }
 
 func handleLockListOut(ctx *context.Context, lock *models.LFSLock, err error) {
@@ -59,17 +70,16 @@ func handleLockListOut(ctx *context.Context, lock *models.LFSLock, err error) {
 
 // GetListLockHandler list locks
 func GetListLockHandler(ctx *context.Context) {
-	status := checkRequest(ctx.Req, false)
+	if !checkIsValidRequest(ctx, false) {
-	if status != 200 {
-		writeStatus(ctx, status)
 		return
 	}
 	ctx.Resp.Header().Set("Content-Type", metaMediaType)
 
-	err := models.CheckLFSAccessForRepo(ctx.User, ctx.Repo.Repository.ID, "list")
+	err := models.CheckLFSAccessForRepo(ctx.User, ctx.Repo.Repository, models.AccessModeRead)
 	if err != nil {
-		if models.IsErrLFSLockUnauthorizedAction(err) {
+		if models.IsErrLFSUnauthorizedAction(err) {
-			ctx.JSON(403, api.LFSLockError{
+			ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs")
+			ctx.JSON(401, api.LFSLockError{
 				Message: "You must have pull access to list locks : " + err.Error(),
 			})
 			return
@@ -96,7 +106,7 @@ func GetListLockHandler(ctx *context.Context) {
 
 	path := ctx.Query("path")
 	if path != "" { //Case where we request a specific id
-		lock, err := models.GetLFSLock(ctx.Repo.Repository.ID, path)
+		lock, err := models.GetLFSLock(ctx.Repo.Repository, path)
 		handleLockListOut(ctx, lock, err)
 		return
 	}
@@ -120,9 +130,7 @@ func GetListLockHandler(ctx *context.Context) {
 
 // PostLockHandler create lock
 func PostLockHandler(ctx *context.Context) {
-	status := checkRequest(ctx.Req, true)
+	if !checkIsValidRequest(ctx, false) {
-	if status != 200 {
-		writeStatus(ctx, status)
 		return
 	}
 	ctx.Resp.Header().Set("Content-Type", metaMediaType)
@@ -136,9 +144,9 @@ func PostLockHandler(ctx *context.Context) {
 	}
 
 	lock, err := models.CreateLFSLock(&models.LFSLock{
-		RepoID: ctx.Repo.Repository.ID,
+		Repo:  ctx.Repo.Repository,
-		Path:   req.Path,
+		Path:  req.Path,
-		Owner:  ctx.User,
+		Owner: ctx.User,
 	})
 	if err != nil {
 		if models.IsErrLFSLockAlreadyExist(err) {
@@ -148,8 +156,9 @@ func PostLockHandler(ctx *context.Context) {
 			})
 			return
 		}
-		if models.IsErrLFSLockUnauthorizedAction(err) {
+		if models.IsErrLFSUnauthorizedAction(err) {
-			ctx.JSON(403, api.LFSLockError{
+			ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs")
+			ctx.JSON(401, api.LFSLockError{
 				Message: "You must have push access to create locks : " + err.Error(),
 			})
 			return
@@ -164,18 +173,16 @@ func PostLockHandler(ctx *context.Context) {
 
 // VerifyLockHandler list locks for verification
 func VerifyLockHandler(ctx *context.Context) {
-	status := checkRequest(ctx.Req, true)
+	if !checkIsValidRequest(ctx, false) {
-	if status != 200 {
-		writeStatus(ctx, status)
 		return
 	}
-
 	ctx.Resp.Header().Set("Content-Type", metaMediaType)
 
-	err := models.CheckLFSAccessForRepo(ctx.User, ctx.Repo.Repository.ID, "verify")
+	err := models.CheckLFSAccessForRepo(ctx.User, ctx.Repo.Repository, models.AccessModeWrite)
 	if err != nil {
-		if models.IsErrLFSLockUnauthorizedAction(err) {
+		if models.IsErrLFSUnauthorizedAction(err) {
-			ctx.JSON(403, api.LFSLockError{
+			ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs")
+			ctx.JSON(401, api.LFSLockError{
 				Message: "You must have push access to verify locks : " + err.Error(),
 			})
 			return
@@ -211,9 +218,7 @@ func VerifyLockHandler(ctx *context.Context) {
 
 // UnLockHandler delete locks
 func UnLockHandler(ctx *context.Context) {
-	status := checkRequest(ctx.Req, true)
+	if !checkIsValidRequest(ctx, false) {
-	if status != 200 {
-		writeStatus(ctx, status)
 		return
 	}
 	ctx.Resp.Header().Set("Content-Type", metaMediaType)
@@ -228,8 +233,9 @@ func UnLockHandler(ctx *context.Context) {
 
 	lock, err := models.DeleteLFSLockByID(ctx.ParamsInt64("lid"), ctx.User, req.Force)
 	if err != nil {
-		if models.IsErrLFSLockUnauthorizedAction(err) {
+		if models.IsErrLFSUnauthorizedAction(err) {
-			ctx.JSON(403, api.LFSLockError{
+			ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs")
+			ctx.JSON(401, api.LFSLockError{
 				Message: "You must have push access to delete locks : " + err.Error(),
 			})
 			return

modules/lfs/server.go

@@ -473,7 +473,6 @@ func logRequest(r macaron.Request, status int) {
 // authenticate uses the authorization string to determine whether
 // or not to proceed. This server assumes an HTTP Basic auth format.
 func authenticate(ctx *context.Context, repository *models.Repository, authorization string, requireWrite bool) bool {
-
 	accessMode := models.AccessModeRead
 	if requireWrite {
 		accessMode = models.AccessModeWrite
@@ -482,86 +481,92 @@ func authenticate(ctx *context.Context, repository *models.Repository, authoriza
 	if !repository.IsPrivate && !requireWrite {
 		return true
 	}
-
 	if ctx.IsSigned {
 		accessCheck, _ := models.HasAccess(ctx.User.ID, repository, accessMode)
 		return accessCheck
 	}
 
-	if authorization == "" {
+	user, repo, opStr, err := parseToken(authorization)
+	if err != nil {
 		return false
 	}
-
+	ctx.User = user
-	if authenticateToken(repository, authorization, requireWrite) {
+	if opStr == "basic" {
+		accessCheck, _ := models.HasAccess(ctx.User.ID, repository, accessMode)
+		return accessCheck
+	}
+	if repository.ID == repo.ID {
+		if requireWrite && opStr != "upload" {
+			return false
+		}
 		return true
 	}
-
+	return false
-	if !strings.HasPrefix(authorization, "Basic ") {
-		return false
-	}
-
-	c, err := base64.StdEncoding.DecodeString(strings.TrimPrefix(authorization, "Basic "))
-	if err != nil {
-		return false
-	}
-	cs := string(c)
-	i := strings.IndexByte(cs, ':')
-	if i < 0 {
-		return false
-	}
-	user, password := cs[:i], cs[i+1:]
-
-	userModel, err := models.GetUserByName(user)
-	if err != nil {
-		return false
-	}
-
-	if !userModel.ValidatePassword(password) {
-		return false
-	}
-
-	accessCheck, _ := models.HasAccess(userModel.ID, repository, accessMode)
-	return accessCheck
 }
 
-func authenticateToken(repository *models.Repository, authorization string, requireWrite bool) bool {
+func parseToken(authorization string) (*models.User, *models.Repository, string, error) {
-	if !strings.HasPrefix(authorization, "Bearer ") {
+	if authorization == "" {
-		return false
+		return nil, nil, "unknown", fmt.Errorf("No token")
 	}
-
+	if strings.HasPrefix(authorization, "Bearer ") {
-	token, err := jwt.Parse(authorization[7:], func(t *jwt.Token) (interface{}, error) {
+		token, err := jwt.Parse(authorization[7:], func(t *jwt.Token) (interface{}, error) {
-		if _, ok := t.Method.(*jwt.SigningMethodHMAC); !ok {
+			if _, ok := t.Method.(*jwt.SigningMethodHMAC); !ok {
-			return nil, fmt.Errorf("unexpected signing method: %v", t.Header["alg"])
+				return nil, fmt.Errorf("unexpected signing method: %v", t.Header["alg"])
+			}
+			return setting.LFS.JWTSecretBytes, nil
+		})
+		if err != nil {
+			return nil, nil, "unknown", err
 		}
-		return setting.LFS.JWTSecretBytes, nil
+		claims, claimsOk := token.Claims.(jwt.MapClaims)
-	})
+		if !token.Valid || !claimsOk {
-	if err != nil {
+			return nil, nil, "unknown", fmt.Errorf("Token claim invalid")
-		return false
+		}
-	}
+		opStr, ok := claims["op"].(string)
-	claims, claimsOk := token.Claims.(jwt.MapClaims)
+		if !ok {
-	if !token.Valid || !claimsOk {
+			return nil, nil, "unknown", fmt.Errorf("Token operation invalid")
-		return false
+		}
+		repoID, ok := claims["repo"].(float64)
+		if !ok {
+			return nil, nil, opStr, fmt.Errorf("Token repository id invalid")
+		}
+		r, err := models.GetRepositoryByID(int64(repoID))
+		if err != nil {
+			return nil, nil, opStr, err
+		}
+		userID, ok := claims["user"].(float64)
+		if !ok {
+			return nil, r, opStr, fmt.Errorf("Token user id invalid")
+		}
+		u, err := models.GetUserByID(int64(userID))
+		if err != nil {
+			return nil, r, opStr, err
+		}
+		return u, r, opStr, nil
 	}
 
-	opStr, ok := claims["op"].(string)
+	if strings.HasPrefix(authorization, "Basic ") {
-	if !ok {
+		c, err := base64.StdEncoding.DecodeString(strings.TrimPrefix(authorization, "Basic "))
-		return false
+		if err != nil {
+			return nil, nil, "basic", err
+		}
+		cs := string(c)
+		i := strings.IndexByte(cs, ':')
+		if i < 0 {
+			return nil, nil, "basic", fmt.Errorf("Basic auth invalid")
+		}
+		user, password := cs[:i], cs[i+1:]
+		u, err := models.GetUserByName(user)
+		if err != nil {
+			return nil, nil, "basic", err
+		}
+		if !u.ValidatePassword(password) {
+			return nil, nil, "basic", fmt.Errorf("Basic auth failed")
+		}
+		return u, nil, "basic", nil
 	}
 
-	if requireWrite && opStr != "upload" {
+	return nil, nil, "unknown", fmt.Errorf("Token not found")
-		return false
-	}
-
-	repoID, ok := claims["repo"].(float64)
-	if !ok {
-		return false
-	}
-
-	if repository.ID != int64(repoID) {
-		return false
-	}
-
-	return true
 }
 
 func requireAuth(ctx *context.Context) {