Skip to content

Fix SSH auth lfs locks #3152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jan 27, 2018
Merged

Fix SSH auth lfs locks #3152

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ac9e714
Fix SSH auth LFS locks
sapk Dec 11, 2017
bad1991
Activate SSH/lock test
sapk Jan 16, 2018
ef09252
Remove debug
sapk Jan 16, 2018
faf2196
Merge branch 'master' into fix-ssh-auth-lfs-locks
lafriks Jan 20, 2018
5064568
Follow @lunny recommendation for AfterLoad method
sapk Jan 25, 2018
c8fce15
Merge branch 'master' into fix-ssh-auth-lfs-locks
lafriks Jan 27, 2018
2036540
Merge branch 'master' into fix-ssh-auth-lfs-locks
lafriks Jan 27, 2018
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions cmd/serv.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
8 changes: 4 additions & 4 deletions integrations/api_repo_lfs_locks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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: "path/test-user4", httpResult: http.StatusForbidden},
{user: user4, repo: repo1, path: "FoO/BaR.zip", httpResult: http.StatusUnauthorized},
{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}},

Expand Down
4 changes: 1 addition & 3 deletions integrations/git_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
*/
})
})
})
Expand Down
19 changes: 11 additions & 8 deletions models/error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
type ErrLFSLockUnauthorizedAction struct {
// ErrLFSUnauthorizedAction represents a "LFSUnauthorizedAction" kind of error.
type ErrLFSUnauthorizedAction struct {
RepoID int64
UserName string
Action string
Mode AccessMode
}

// IsErrLFSLockUnauthorizedAction checks if an error is a ErrLFSLockUnauthorizedAction.
func IsErrLFSLockUnauthorizedAction(err error) bool {
_, ok := err.(ErrLFSLockUnauthorizedAction)
// IsErrLFSUnauthorizedAction checks if an error is a ErrLFSUnauthorizedAction.
func IsErrLFSUnauthorizedAction(err error) bool {
_, ok := err.(ErrLFSUnauthorizedAction)
return ok
}

func (err ErrLFSLockUnauthorizedAction) Error() string {
return fmt.Sprintf("User %s doesn't have rigth to %s for lfs lock [rid: %d]", err.UserName, err.Action, err.RepoID)
func (err ErrLFSUnauthorizedAction) Error() string {
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.
Expand Down
52 changes: 27 additions & 25 deletions models/lfs_lock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,28 +11,39 @@ import (
"strings"
"time"

"code.gitea.io/gitea/modules/log"
api "code.gitea.io/sdk/gitea"
)

// LFSLock represents a git lfs lock of repository.
type LFSLock struct {
ID int64 `xorm:"pk autoincr"`
RepoID int64 `xorm:"INDEX NOT NULL"`
Owner *User `xorm:"-"`
OwnerID int64 `xorm:"INDEX NOT NULL"`
Path string `xorm:"TEXT"`
Created time.Time `xorm:"created"`
ID int64 `xorm:"pk autoincr"`
Repo *Repository `xorm:"-"`
RepoID int64 `xorm:"INDEX NOT NULL"`
Owner *User `xorm:"-"`
OwnerID int64 `xorm:"INDEX NOT NULL"`
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() {
l.Owner, _ = GetUserByID(l.OwnerID)
var err error
l.Owner, err = GetUserByID(l.OwnerID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will performing database queries inside AfterLoad lead to deadlock problems if the query that triggered AfterLoad is part of a transaction? See #1813
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ethantkoenig queries should not be affected by locking and will load last committed data even if update/insert is in progress
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. In #1813 the offending SQL was a write which blocked on an ongoing transaction, but if you're sure that reading won't block on ongoing transactions then this is okay.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use

func (l *LFSLock) AfterLoad(sess *xorm.Session) { }

to avoid that.
if err != nil {
log.Error(2, "LFS lock AfterLoad failed OwnerId[%d] not found: %v", l.OwnerID, err)
}
l.Repo, err = GetRepositoryByID(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 {
Expand All @@ -53,12 +64,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}
}
Expand All @@ -71,15 +82,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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? Is it really worth passing the repository if the only thing we care about it the id?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I remember it is to use HasAccess that need a repo reference.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I could make a func accessLevel and hasAccess that use repoID more generally for better perf ? (I think in a other PR ?)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I must have missed the call to CheckLFSAccessForRepo. Yes, perhaps in another PR
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact after reviewing code of accessLevel, it use repo.IsPrivate that need a repo reference so.
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
}
Expand Down Expand Up @@ -109,7 +120,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
}
Expand All @@ -123,24 +134,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}
}
mode := AccessModeRead
if action == "create" || action == "delete" || action == "verify" {
mode = AccessModeWrite
}

repo, err := GetRepositoryByID(repoID)
if err != nil {
return err
return ErrLFSUnauthorizedAction{repo.ID, "undefined", mode}
}
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
}
78 changes: 42 additions & 36 deletions modules/lfs/locks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(ctx.Req) {
writeStatus(ctx, 400)
return false
}
if !MetaMatcher(req) {
return 400
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) {
Expand Down Expand Up @@ -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 status != 200 {
writeStatus(ctx, status)
if !checkIsValidRequest(ctx, false) {
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) {
ctx.JSON(403, api.LFSLockError{
if models.IsErrLFSUnauthorizedAction(err) {
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
Expand All @@ -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
}
Expand All @@ -120,9 +130,7 @@ func GetListLockHandler(ctx *context.Context) {

// PostLockHandler create lock
func PostLockHandler(ctx *context.Context) {
status := checkRequest(ctx.Req, true)
if status != 200 {
writeStatus(ctx, status)
if !checkIsValidRequest(ctx, false) {
return
}
ctx.Resp.Header().Set("Content-Type", metaMediaType)
Expand All @@ -136,9 +144,9 @@ func PostLockHandler(ctx *context.Context) {
}

lock, err := models.CreateLFSLock(&models.LFSLock{
RepoID: ctx.Repo.Repository.ID,
Path: req.Path,
Owner: ctx.User,
Repo: ctx.Repo.Repository,
Path: req.Path,
Owner: ctx.User,
})
if err != nil {
if models.IsErrLFSLockAlreadyExist(err) {
Expand All @@ -148,8 +156,9 @@ func PostLockHandler(ctx *context.Context) {
})
return
}
if models.IsErrLFSLockUnauthorizedAction(err) {
ctx.JSON(403, api.LFSLockError{
if models.IsErrLFSUnauthorizedAction(err) {
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
Expand All @@ -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 status != 200 {
writeStatus(ctx, status)
if !checkIsValidRequest(ctx, false) {
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) {
ctx.JSON(403, api.LFSLockError{
if models.IsErrLFSUnauthorizedAction(err) {
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
Expand Down Expand Up @@ -211,9 +218,7 @@ func VerifyLockHandler(ctx *context.Context) {

// UnLockHandler delete locks
func UnLockHandler(ctx *context.Context) {
status := checkRequest(ctx.Req, true)
if status != 200 {
writeStatus(ctx, status)
if !checkIsValidRequest(ctx, false) {
return
}
ctx.Resp.Header().Set("Content-Type", metaMediaType)
Expand All @@ -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) {
ctx.JSON(403, api.LFSLockError{
if models.IsErrLFSUnauthorizedAction(err) {
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
Expand Down
Loading