Skip to content

Commit 9e842c8

Browse files
sapklafriks
sapk
authored and
lafriks
committed
Fix SSH auth lfs locks (#3152)
* Fix SSH auth LFS locks * Activate SSH/lock test * Remove debug * Follow @lunny recommendation for AfterLoad method
1 parent 97fe773 commit 9e842c8

File tree

7 files changed

+162
-143
lines changed

7 files changed

+162
-143
lines changed

‎cmd/serv.go

+6-2
Original file line numberDiff line numberDiff line change
@@ -259,12 +259,16 @@ func runServ(c *cli.Context) error {
259259
url := fmt.Sprintf("%s%s/%s.git/info/lfs", setting.AppURL, username, repo.Name)
260260

261261
now := time.Now()
262-
token := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{
262+
claims := jwt.MapClaims{
263263
"repo": repo.ID,
264264
"op": lfsVerb,
265265
"exp": now.Add(5 * time.Minute).Unix(),
266266
"nbf": now.Unix(),
267-
})
267+
}
268+
if user != nil {
269+
claims["user"] = user.ID
270+
}
271+
token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims)
268272

269273
// Sign and get the complete encoded token as a string using the secret
270274
tokenString, err := token.SignedString(setting.LFS.JWTSecretBytes)

‎integrations/api_repo_lfs_locks_test.go

+4-4
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,10 @@ func TestAPILFSLocksNotLogin(t *testing.T) {
4141

4242
req := NewRequestf(t, "GET", "/%s/%s.git/info/lfs/locks", user.Name, repo.Name)
4343
req.Header.Set("Accept", "application/vnd.git-lfs+json")
44-
resp := MakeRequest(t, req, http.StatusForbidden)
44+
resp := MakeRequest(t, req, http.StatusUnauthorized)
4545
var lfsLockError api.LFSLockError
4646
DecodeJSON(t, resp, &lfsLockError)
47-
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)
47+
assert.Equal(t, "Unauthorized", lfsLockError.Message)
4848
}
4949

5050
func TestAPILFSLocksLogged(t *testing.T) {
@@ -68,8 +68,8 @@ func TestAPILFSLocksLogged(t *testing.T) {
6868
{user: user2, repo: repo1, path: "path/test", httpResult: http.StatusConflict},
6969
{user: user2, repo: repo1, path: "Foo/BaR.zip", httpResult: http.StatusConflict},
7070
{user: user2, repo: repo1, path: "Foo/Test/../subFOlder/../Relative/../BaR.zip", httpResult: http.StatusConflict},
71-
{user: user4, repo: repo1, path: "FoO/BaR.zip", httpResult: http.StatusForbidden},
72-
{user: user4, repo: repo1, path: "path/test-user4", httpResult: http.StatusForbidden},
71+
{user: user4, repo: repo1, path: "FoO/BaR.zip", httpResult: http.StatusUnauthorized},
72+
{user: user4, repo: repo1, path: "path/test-user4", httpResult: http.StatusUnauthorized},
7373
{user: user2, repo: repo1, path: "patH/Test-user4", httpResult: http.StatusCreated, addTime: []int{0}},
7474
{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}},
7575

‎integrations/git_test.go

+1-3
Original file line numberDiff line numberDiff line change
@@ -214,11 +214,9 @@ func TestGit(t *testing.T) {
214214
commitAndPush(t, bigSize, dstPath)
215215
})
216216
})
217-
/* Failed without #3152. TODO activate with fix.
218217
t.Run("Locks", func(t *testing.T) {
219-
lockTest(t, u.String(), dstPath)
218+
lockTest(t, u.String(), dstPath)
220219
})
221-
*/
222220
})
223221
})
224222
})

‎models/error.go

+11-8
Original file line numberDiff line numberDiff line change
@@ -530,21 +530,24 @@ func (err ErrLFSLockNotExist) Error() string {
530530
return fmt.Sprintf("lfs lock does not exist [id: %d, rid: %d, path: %s]", err.ID, err.RepoID, err.Path)
531531
}
532532

533-
// ErrLFSLockUnauthorizedAction represents a "LFSLockUnauthorizedAction" kind of error.
534-
type ErrLFSLockUnauthorizedAction struct {
533+
// ErrLFSUnauthorizedAction represents a "LFSUnauthorizedAction" kind of error.
534+
type ErrLFSUnauthorizedAction struct {
535535
RepoID int64
536536
UserName string
537-
Action string
537+
Mode AccessMode
538538
}
539539

540-
// IsErrLFSLockUnauthorizedAction checks if an error is a ErrLFSLockUnauthorizedAction.
541-
func IsErrLFSLockUnauthorizedAction(err error) bool {
542-
_, ok := err.(ErrLFSLockUnauthorizedAction)
540+
// IsErrLFSUnauthorizedAction checks if an error is a ErrLFSUnauthorizedAction.
541+
func IsErrLFSUnauthorizedAction(err error) bool {
542+
_, ok := err.(ErrLFSUnauthorizedAction)
543543
return ok
544544
}
545545

546-
func (err ErrLFSLockUnauthorizedAction) Error() string {
547-
return fmt.Sprintf("User %s doesn't have rigth to %s for lfs lock [rid: %d]", err.UserName, err.Action, err.RepoID)
546+
func (err ErrLFSUnauthorizedAction) Error() string {
547+
if err.Mode == AccessModeWrite {
548+
return fmt.Sprintf("User %s doesn't have write access for lfs lock [rid: %d]", err.UserName, err.RepoID)
549+
}
550+
return fmt.Sprintf("User %s doesn't have read access for lfs lock [rid: %d]", err.UserName, err.RepoID)
548551
}
549552

550553
// ErrLFSLockAlreadyExist represents a "LFSLockAlreadyExist" kind of error.

‎models/lfs_lock.go

+29-26
Original file line numberDiff line numberDiff line change
@@ -11,28 +11,40 @@ import (
1111
"strings"
1212
"time"
1313

14+
"code.gitea.io/gitea/modules/log"
1415
api "code.gitea.io/sdk/gitea"
16+
"github.com/go-xorm/xorm"
1517
)
1618

1719
// LFSLock represents a git lfs lock of repository.
1820
type LFSLock struct {
19-
ID int64 `xorm:"pk autoincr"`
20-
RepoID int64 `xorm:"INDEX NOT NULL"`
21-
Owner *User `xorm:"-"`
22-
OwnerID int64 `xorm:"INDEX NOT NULL"`
23-
Path string `xorm:"TEXT"`
24-
Created time.Time `xorm:"created"`
21+
ID int64 `xorm:"pk autoincr"`
22+
Repo *Repository `xorm:"-"`
23+
RepoID int64 `xorm:"INDEX NOT NULL"`
24+
Owner *User `xorm:"-"`
25+
OwnerID int64 `xorm:"INDEX NOT NULL"`
26+
Path string `xorm:"TEXT"`
27+
Created time.Time `xorm:"created"`
2528
}
2629

2730
// BeforeInsert is invoked from XORM before inserting an object of this type.
2831
func (l *LFSLock) BeforeInsert() {
2932
l.OwnerID = l.Owner.ID
33+
l.RepoID = l.Repo.ID
3034
l.Path = cleanPath(l.Path)
3135
}
3236

3337
// AfterLoad is invoked from XORM after setting the values of all fields of this object.
34-
func (l *LFSLock) AfterLoad() {
35-
l.Owner, _ = GetUserByID(l.OwnerID)
38+
func (l *LFSLock) AfterLoad(session *xorm.Session) {
39+
var err error
40+
l.Owner, err = getUserByID(session, l.OwnerID)
41+
if err != nil {
42+
log.Error(2, "LFS lock AfterLoad failed OwnerId[%d] not found: %v", l.OwnerID, err)
43+
}
44+
l.Repo, err = getRepositoryByID(session, l.RepoID)
45+
if err != nil {
46+
log.Error(2, "LFS lock AfterLoad failed RepoId[%d] not found: %v", l.RepoID, err)
47+
}
3648
}
3749

3850
func cleanPath(p string) string {
@@ -53,12 +65,12 @@ func (l *LFSLock) APIFormat() *api.LFSLock {
5365

5466
// CreateLFSLock creates a new lock.
5567
func CreateLFSLock(lock *LFSLock) (*LFSLock, error) {
56-
err := CheckLFSAccessForRepo(lock.Owner, lock.RepoID, "create")
68+
err := CheckLFSAccessForRepo(lock.Owner, lock.Repo, AccessModeWrite)
5769
if err != nil {
5870
return nil, err
5971
}
6072

61-
l, err := GetLFSLock(lock.RepoID, lock.Path)
73+
l, err := GetLFSLock(lock.Repo, lock.Path)
6274
if err == nil {
6375
return l, ErrLFSLockAlreadyExist{lock.RepoID, lock.Path}
6476
}
@@ -71,15 +83,15 @@ func CreateLFSLock(lock *LFSLock) (*LFSLock, error) {
7183
}
7284

7385
// GetLFSLock returns release by given path.
74-
func GetLFSLock(repoID int64, path string) (*LFSLock, error) {
86+
func GetLFSLock(repo *Repository, path string) (*LFSLock, error) {
7587
path = cleanPath(path)
76-
rel := &LFSLock{RepoID: repoID}
88+
rel := &LFSLock{RepoID: repo.ID}
7789
has, err := x.Where("lower(path) = ?", strings.ToLower(path)).Get(rel)
7890
if err != nil {
7991
return nil, err
8092
}
8193
if !has {
82-
return nil, ErrLFSLockNotExist{0, repoID, path}
94+
return nil, ErrLFSLockNotExist{0, repo.ID, path}
8395
}
8496
return rel, nil
8597
}
@@ -109,7 +121,7 @@ func DeleteLFSLockByID(id int64, u *User, force bool) (*LFSLock, error) {
109121
return nil, err
110122
}
111123

112-
err = CheckLFSAccessForRepo(u, lock.RepoID, "delete")
124+
err = CheckLFSAccessForRepo(u, lock.Repo, AccessModeWrite)
113125
if err != nil {
114126
return nil, err
115127
}
@@ -123,24 +135,15 @@ func DeleteLFSLockByID(id int64, u *User, force bool) (*LFSLock, error) {
123135
}
124136

125137
//CheckLFSAccessForRepo check needed access mode base on action
126-
func CheckLFSAccessForRepo(u *User, repoID int64, action string) error {
138+
func CheckLFSAccessForRepo(u *User, repo *Repository, mode AccessMode) error {
127139
if u == nil {
128-
return ErrLFSLockUnauthorizedAction{repoID, "undefined", action}
129-
}
130-
mode := AccessModeRead
131-
if action == "create" || action == "delete" || action == "verify" {
132-
mode = AccessModeWrite
133-
}
134-
135-
repo, err := GetRepositoryByID(repoID)
136-
if err != nil {
137-
return err
140+
return ErrLFSUnauthorizedAction{repo.ID, "undefined", mode}
138141
}
139142
has, err := HasAccess(u.ID, repo, mode)
140143
if err != nil {
141144
return err
142145
} else if !has {
143-
return ErrLFSLockUnauthorizedAction{repo.ID, u.DisplayName(), action}
146+
return ErrLFSUnauthorizedAction{repo.ID, u.DisplayName(), mode}
144147
}
145148
return nil
146149
}

‎modules/lfs/locks.go

+42-36
Original file line numberDiff line numberDiff line change
@@ -13,24 +13,35 @@ import (
1313
"code.gitea.io/gitea/modules/context"
1414
"code.gitea.io/gitea/modules/setting"
1515
api "code.gitea.io/sdk/gitea"
16-
17-
"gopkg.in/macaron.v1"
1816
)
1917

20-
func checkRequest(req macaron.Request, post bool) int {
18+
//checkIsValidRequest check if it a valid request in case of bad request it write the response to ctx.
19+
func checkIsValidRequest(ctx *context.Context, post bool) bool {
2120
if !setting.LFS.StartServer {
22-
return 404
21+
writeStatus(ctx, 404)
22+
return false
23+
}
24+
if !MetaMatcher(ctx.Req) {
25+
writeStatus(ctx, 400)
26+
return false
2327
}
24-
if !MetaMatcher(req) {
25-
return 400
28+
if !ctx.IsSigned {
29+
user, _, _, err := parseToken(ctx.Req.Header.Get("Authorization"))
30+
if err != nil {
31+
ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs")
32+
writeStatus(ctx, 401)
33+
return false
34+
}
35+
ctx.User = user
2636
}
2737
if post {
28-
mediaParts := strings.Split(req.Header.Get("Content-Type"), ";")
38+
mediaParts := strings.Split(ctx.Req.Header.Get("Content-Type"), ";")
2939
if mediaParts[0] != metaMediaType {
30-
return 400
40+
writeStatus(ctx, 400)
41+
return false
3142
}
3243
}
33-
return 200
44+
return true
3445
}
3546

3647
func handleLockListOut(ctx *context.Context, lock *models.LFSLock, err error) {
@@ -59,17 +70,16 @@ func handleLockListOut(ctx *context.Context, lock *models.LFSLock, err error) {
5970

6071
// GetListLockHandler list locks
6172
func GetListLockHandler(ctx *context.Context) {
62-
status := checkRequest(ctx.Req, false)
63-
if status != 200 {
64-
writeStatus(ctx, status)
73+
if !checkIsValidRequest(ctx, false) {
6574
return
6675
}
6776
ctx.Resp.Header().Set("Content-Type", metaMediaType)
6877

69-
err := models.CheckLFSAccessForRepo(ctx.User, ctx.Repo.Repository.ID, "list")
78+
err := models.CheckLFSAccessForRepo(ctx.User, ctx.Repo.Repository, models.AccessModeRead)
7079
if err != nil {
71-
if models.IsErrLFSLockUnauthorizedAction(err) {
72-
ctx.JSON(403, api.LFSLockError{
80+
if models.IsErrLFSUnauthorizedAction(err) {
81+
ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs")
82+
ctx.JSON(401, api.LFSLockError{
7383
Message: "You must have pull access to list locks : " + err.Error(),
7484
})
7585
return
@@ -96,7 +106,7 @@ func GetListLockHandler(ctx *context.Context) {
96106

97107
path := ctx.Query("path")
98108
if path != "" { //Case where we request a specific id
99-
lock, err := models.GetLFSLock(ctx.Repo.Repository.ID, path)
109+
lock, err := models.GetLFSLock(ctx.Repo.Repository, path)
100110
handleLockListOut(ctx, lock, err)
101111
return
102112
}
@@ -120,9 +130,7 @@ func GetListLockHandler(ctx *context.Context) {
120130

121131
// PostLockHandler create lock
122132
func PostLockHandler(ctx *context.Context) {
123-
status := checkRequest(ctx.Req, true)
124-
if status != 200 {
125-
writeStatus(ctx, status)
133+
if !checkIsValidRequest(ctx, false) {
126134
return
127135
}
128136
ctx.Resp.Header().Set("Content-Type", metaMediaType)
@@ -136,9 +144,9 @@ func PostLockHandler(ctx *context.Context) {
136144
}
137145

138146
lock, err := models.CreateLFSLock(&models.LFSLock{
139-
RepoID: ctx.Repo.Repository.ID,
140-
Path: req.Path,
141-
Owner: ctx.User,
147+
Repo: ctx.Repo.Repository,
148+
Path: req.Path,
149+
Owner: ctx.User,
142150
})
143151
if err != nil {
144152
if models.IsErrLFSLockAlreadyExist(err) {
@@ -148,8 +156,9 @@ func PostLockHandler(ctx *context.Context) {
148156
})
149157
return
150158
}
151-
if models.IsErrLFSLockUnauthorizedAction(err) {
152-
ctx.JSON(403, api.LFSLockError{
159+
if models.IsErrLFSUnauthorizedAction(err) {
160+
ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs")
161+
ctx.JSON(401, api.LFSLockError{
153162
Message: "You must have push access to create locks : " + err.Error(),
154163
})
155164
return
@@ -164,18 +173,16 @@ func PostLockHandler(ctx *context.Context) {
164173

165174
// VerifyLockHandler list locks for verification
166175
func VerifyLockHandler(ctx *context.Context) {
167-
status := checkRequest(ctx.Req, true)
168-
if status != 200 {
169-
writeStatus(ctx, status)
176+
if !checkIsValidRequest(ctx, false) {
170177
return
171178
}
172-
173179
ctx.Resp.Header().Set("Content-Type", metaMediaType)
174180

175-
err := models.CheckLFSAccessForRepo(ctx.User, ctx.Repo.Repository.ID, "verify")
181+
err := models.CheckLFSAccessForRepo(ctx.User, ctx.Repo.Repository, models.AccessModeWrite)
176182
if err != nil {
177-
if models.IsErrLFSLockUnauthorizedAction(err) {
178-
ctx.JSON(403, api.LFSLockError{
183+
if models.IsErrLFSUnauthorizedAction(err) {
184+
ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs")
185+
ctx.JSON(401, api.LFSLockError{
179186
Message: "You must have push access to verify locks : " + err.Error(),
180187
})
181188
return
@@ -211,9 +218,7 @@ func VerifyLockHandler(ctx *context.Context) {
211218

212219
// UnLockHandler delete locks
213220
func UnLockHandler(ctx *context.Context) {
214-
status := checkRequest(ctx.Req, true)
215-
if status != 200 {
216-
writeStatus(ctx, status)
221+
if !checkIsValidRequest(ctx, false) {
217222
return
218223
}
219224
ctx.Resp.Header().Set("Content-Type", metaMediaType)
@@ -228,8 +233,9 @@ func UnLockHandler(ctx *context.Context) {
228233

229234
lock, err := models.DeleteLFSLockByID(ctx.ParamsInt64("lid"), ctx.User, req.Force)
230235
if err != nil {
231-
if models.IsErrLFSLockUnauthorizedAction(err) {
232-
ctx.JSON(403, api.LFSLockError{
236+
if models.IsErrLFSUnauthorizedAction(err) {
237+
ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs")
238+
ctx.JSON(401, api.LFSLockError{
233239
Message: "You must have push access to delete locks : " + err.Error(),
234240
})
235241
return

0 commit comments

Comments
 (0)