remove ErrLFSUnauthorizedAction, add comment, fix test

Author: wxiaoguang

models/git/lfs.go

@@ -8,7 +8,6 @@ import (
 	"fmt"
 
 	"code.gitea.io/gitea/models/db"
-	"code.gitea.io/gitea/models/perm"
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
@@ -42,30 +41,6 @@ func (err ErrLFSLockNotExist) Unwrap() error {
 	return util.ErrNotExist
 }
 
-// ErrLFSUnauthorizedAction represents a "LFSUnauthorizedAction" kind of error.
-type ErrLFSUnauthorizedAction struct {
-	RepoID   int64
-	UserName string
-	Mode     perm.AccessMode
-}
-
-// IsErrLFSUnauthorizedAction checks if an error is a ErrLFSUnauthorizedAction.
-func IsErrLFSUnauthorizedAction(err error) bool {
-	_, ok := err.(ErrLFSUnauthorizedAction)
-	return ok
-}
-
-func (err ErrLFSUnauthorizedAction) Error() string {
-	if err.Mode == perm.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)
-}
-
-func (err ErrLFSUnauthorizedAction) Unwrap() error {
-	return util.ErrPermissionDenied
-}
-
 // ErrLFSLockAlreadyExist represents a "LFSLockAlreadyExist" kind of error.
 type ErrLFSLockAlreadyExist struct {
 	RepoID int64
@@ -93,12 +68,6 @@ type ErrLFSFileLocked struct {
 	UserName string
 }
 
-// IsErrLFSFileLocked checks if an error is a ErrLFSFileLocked.
-func IsErrLFSFileLocked(err error) bool {
-	_, ok := err.(ErrLFSFileLocked)
-	return ok
-}
-
 func (err ErrLFSFileLocked) Error() string {
 	return fmt.Sprintf("File is lfs locked [repo: %d, locked by: %s, path: %s]", err.RepoID, err.UserName, err.Path)
 }

services/lfs/locks.go

@@ -187,13 +187,6 @@ func PostLockHandler(ctx *context.Context) {
 			})
 			return
 		}
-		if git_model.IsErrLFSUnauthorizedAction(err) {
-			ctx.Resp.Header().Set("WWW-Authenticate", `Basic realm="gitea-lfs"`)
-			ctx.JSON(http.StatusUnauthorized, api.LFSLockError{
-				Message: "You must have push access to create locks : " + err.Error(),
-			})
-			return
-		}
 		log.Error("Unable to CreateLFSLock in repository %-v at %s for user %-v: Error: %v", repository, req.Path, ctx.Doer, err)
 		ctx.JSON(http.StatusInternalServerError, api.LFSLockError{
 			Message: "internal server error : Internal Server Error",
@@ -317,13 +310,6 @@ func UnLockHandler(ctx *context.Context) {
 
 	lock, err := git_model.DeleteLFSLockByID(ctx, ctx.PathParamInt64("lid"), repository, ctx.Doer, req.Force)
 	if err != nil {
-		if git_model.IsErrLFSUnauthorizedAction(err) {
-			ctx.Resp.Header().Set("WWW-Authenticate", `Basic realm="gitea-lfs"`)
-			ctx.JSON(http.StatusUnauthorized, api.LFSLockError{
-				Message: "You must have push access to delete locks : " + err.Error(),
-			})
-			return
-		}
 		log.Error("Unable to DeleteLFSLockByID[%d] by user %-v with force %t: Error: %v", ctx.PathParamInt64("lid"), ctx.Doer, req.Force, err)
 		ctx.JSON(http.StatusInternalServerError, api.LFSLockError{
 			Message: "unable to delete lock : Internal Server Error",

services/lfs/server.go

@@ -563,11 +563,14 @@ func authenticate(ctx *context.Context, repository *repo_model.Repository, autho
 		return false
 	}
 
-	canRead := perm.CanAccess(accessMode, unit.TypeCode)
-	if canRead && (!requireSigned || ctx.IsSigned) {
+	canAccess := perm.CanAccess(accessMode, unit.TypeCode)
+	// if it doesn't require sign-in and anonymous user has access, return true
+	// if the user is already signed in (for example: by session auth method), and the doer can access, return true
+	if canAccess && (!requireSigned || ctx.IsSigned) {
 		return true
 	}
 
+	// now, either sign-in is required or the ctx.Doer cannot access, check the LFS token
 	user, err := parseToken(ctx, authorization, repository, accessMode)
 	if err != nil {
 		// Most of these are Warn level - the true internal server errors are logged in parseToken already

tests/integration/api_repo_file_get_test.go

@@ -9,7 +9,6 @@ import (
 	"testing"
 
 	auth_model "code.gitea.io/gitea/models/auth"
-	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/tests"
 
 	"github.com/stretchr/testify/assert"
@@ -25,8 +24,9 @@ func TestAPIGetRawFileOrLFS(t *testing.T) {
 
 	// Test with LFS
 	onGiteaRun(t, func(t *testing.T, u *url.URL) {
-		httpContext := NewAPITestContext(t, "user2", "repo-lfs-test", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
-		t.Run("repo-lfs-test", doAPICreateRepository(httpContext, false, func(t *testing.T, repository api.Repository) {
+		createLFSTestRepository(t, "repo-lfs-test")
+		httpContext := NewAPITestContext(t, "user2", "repo-lfs-test", auth_model.AccessTokenScopeWriteRepository)
+		t.Run("repo-lfs-test", func(t *testing.T) {
 			u.Path = httpContext.GitPath()
 			dstPath := t.TempDir()
 
@@ -44,6 +44,6 @@ func TestAPIGetRawFileOrLFS(t *testing.T) {
 			reqLFS := NewRequest(t, "GET", "/api/v1/repos/user2/repo-lfs-test/media/"+lfs).AddTokenAuth(httpContext.Token)
 			respLFS := MakeRequestNilResponseRecorder(t, reqLFS, http.StatusOK)
 			assert.Equal(t, testFileSizeSmall, respLFS.Length)
-		}))
+		})
 	})
 }

tests/integration/api_repo_lfs_test.go

@@ -23,6 +23,7 @@ import (
 	"code.gitea.io/gitea/tests"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestAPILFSNotStarted(t *testing.T) {
@@ -59,12 +60,12 @@ func TestAPILFSMediaType(t *testing.T) {
 	MakeRequest(t, req, http.StatusUnsupportedMediaType)
 }
 
-func createLFSTestRepository(t *testing.T, name string) *repo_model.Repository {
-	ctx := NewAPITestContext(t, "user2", "lfs-"+name+"-repo", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
+func createLFSTestRepository(t *testing.T, repoName string) *repo_model.Repository {
+	ctx := NewAPITestContext(t, "user2", repoName, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
 	t.Run("CreateRepo", doAPICreateRepository(ctx, false))
 
-	repo, err := repo_model.GetRepositoryByOwnerAndName(t.Context(), "user2", "lfs-"+name+"-repo")
-	assert.NoError(t, err)
+	repo, err := repo_model.GetRepositoryByOwnerAndName(t.Context(), "user2", repoName)
+	require.NoError(t, err)
 
 	return repo
 }
@@ -74,7 +75,7 @@ func TestAPILFSBatch(t *testing.T) {
 
 	setting.LFS.StartServer = true
 
-	repo := createLFSTestRepository(t, "batch")
+	repo := createLFSTestRepository(t, "lfs-batch-repo")
 
 	content := []byte("dummy1")
 	oid := storeObjectInRepo(t, repo.ID, &content)
@@ -253,7 +254,7 @@ func TestAPILFSBatch(t *testing.T) {
 			assert.NoError(t, err)
 			assert.True(t, exist)
 
-			repo2 := createLFSTestRepository(t, "batch2")
+			repo2 := createLFSTestRepository(t, "lfs-batch2-repo")
 			content := []byte("dummy0")
 			storeObjectInRepo(t, repo2.ID, &content)
 
@@ -329,7 +330,7 @@ func TestAPILFSUpload(t *testing.T) {
 
 	setting.LFS.StartServer = true
 
-	repo := createLFSTestRepository(t, "upload")
+	repo := createLFSTestRepository(t, "lfs-upload-repo")
 
 	content := []byte("dummy3")
 	oid := storeObjectInRepo(t, repo.ID, &content)
@@ -433,7 +434,7 @@ func TestAPILFSVerify(t *testing.T) {
 
 	setting.LFS.StartServer = true
 
-	repo := createLFSTestRepository(t, "verify")
+	repo := createLFSTestRepository(t, "lfs-verify-repo")
 
 	content := []byte("dummy3")
 	oid := storeObjectInRepo(t, repo.ID, &content)