Skip to content

Test if LFS object is accessible #16865

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 3 commits into from
Aug 31, 2021
+60 −19
Merged

Test if LFS object is accessible #16865

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
365e3c0
Test if object is accessible.
KN4CK3R Aug 29, 2021
6a6ce8a
Added more logging.
KN4CK3R Aug 29, 2021
7a25a41
Merge branch 'main' into fix-lfs-upload
6543 Aug 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions integrations/api_repo_lfs_test.go
View file
Original file line number Diff line number Diff line change
@@ -253,6 +253,10 @@ func TestAPILFSBatch(t *testing.T) {
assert.NoError(t, err)
assert.True(t, exist)

repo2 := createLFSTestRepository(t, "batch2")
content := []byte("dummy0")
storeObjectInRepo(t, repo2.ID, &content)

meta, err := repo.GetLFSMetaObjectByOid(p.Oid)
assert.Nil(t, meta)
assert.Equal(t, models.ErrLFSObjectNotExist, err)
@@ -358,13 +362,19 @@ func TestAPILFSUpload(t *testing.T) {
assert.Nil(t, meta)
assert.Equal(t, models.ErrLFSObjectNotExist, err)

req := newRequest(t, p, "")
t.Run("InvalidAccess", func(t *testing.T) {
req := newRequest(t, p, "invalid")
session.MakeRequest(t, req, http.StatusUnprocessableEntity)
})

session.MakeRequest(t, req, http.StatusOK)
t.Run("ValidAccess", func(t *testing.T) {
req := newRequest(t, p, "dummy5")

meta, err = repo.GetLFSMetaObjectByOid(p.Oid)
assert.NoError(t, err)
assert.NotNil(t, meta)
session.MakeRequest(t, req, http.StatusOK)
meta, err = repo.GetLFSMetaObjectByOid(p.Oid)
assert.NoError(t, err)
assert.NotNil(t, meta)
})
})

t.Run("MetaAlreadyExists", func(t *testing.T) {
59 changes: 45 additions & 14 deletions services/lfs/server.go
View file
Original file line number Diff line number Diff line change
@@ -5,7 +5,9 @@
package lfs

import (
"crypto/sha256"
"encoding/base64"
"encoding/hex"
"errors"
"fmt"
"io"
@@ -214,14 +216,22 @@ func BatchHandler(ctx *context.Context) {
}
}

if exists {
if meta == nil {
if exists && meta == nil {
accessible, err := models.LFSObjectAccessible(ctx.User, p.Oid)
if err != nil {
log.Error("Unable to check if LFS MetaObject [%s] is accessible. Error: %v", p.Oid, err)
writeStatus(ctx, http.StatusInternalServerError)
return
}
if accessible {
_, err := models.NewLFSMetaObject(&models.LFSMetaObject{Pointer: p, RepositoryID: repository.ID})
if err != nil {
log.Error("Unable to create LFS MetaObject [%s] for %s/%s. Error: %v", p.Oid, rc.User, rc.Repo, err)
writeStatus(ctx, http.StatusInternalServerError)
return
}
} else {
exists = false
}
}

@@ -271,29 +281,50 @@ func UploadHandler(ctx *context.Context) {
return
}

meta, err := models.NewLFSMetaObject(&models.LFSMetaObject{Pointer: p, RepositoryID: repository.ID})
if err != nil {
log.Error("Unable to create LFS MetaObject [%s] for %s/%s. Error: %v", p.Oid, rc.User, rc.Repo, err)
writeStatus(ctx, http.StatusInternalServerError)
return
}

contentStore := lfs_module.NewContentStore()

exists, err := contentStore.Exists(p)
if err != nil {
log.Error("Unable to check if LFS OID[%s] exist. Error: %v", p.Oid, err)
writeStatus(ctx, http.StatusInternalServerError)
return
}
if meta.Existing || exists {
ctx.Resp.WriteHeader(http.StatusOK)
return

uploadOrVerify := func() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to just unfold this.

Although it would require some slight duplication of error messages we'd be able to work out from the error where the problem occurred much more easily.

if exists {
accessible, err := models.LFSObjectAccessible(ctx.User, p.Oid)
if err != nil {
log.Error("Unable to check if LFS MetaObject [%s] is accessible. Error: %v", p.Oid, err)
return err
}
if !accessible {
// The file exists but the user has no access to it.
// The upload gets verified by hashing and size comparison to prove access to it.
hash := sha256.New()
written, err := io.Copy(hash, ctx.Req.Body)
if err != nil {
log.Error("Error creating hash. Error: %v", err)
return err
}

if written != p.Size {
return lfs_module.ErrSizeMismatch
}
if hex.EncodeToString(hash.Sum(nil)) != p.Oid {
return lfs_module.ErrHashMismatch
}
}
} else if err := contentStore.Put(p, ctx.Req.Body); err != nil {
log.Error("Error putting LFS MetaObject [%s] into content store. Error: %v", p.Oid, err)
return err
}
_, err := models.NewLFSMetaObject(&models.LFSMetaObject{Pointer: p, RepositoryID: repository.ID})
return err
}

defer ctx.Req.Body.Close()
if err := contentStore.Put(meta.Pointer, ctx.Req.Body); err != nil {
if err := uploadOrVerify(); err != nil {
if errors.Is(err, lfs_module.ErrSizeMismatch) || errors.Is(err, lfs_module.ErrHashMismatch) {
log.Error("Upload does not match LFS MetaObject [%s]. Error: %v", p.Oid, err)
writeStatusMessage(ctx, http.StatusUnprocessableEntity, err.Error())
} else {
writeStatus(ctx, http.StatusInternalServerError)