Prevent double-login for Git HTTP and LFS and simplify login (#15303)

* Prevent double-login for Git HTTP and LFS and simplify login

There are a number of inconsistencies with our current methods for
logging in for git and lfs. The first is that there is a double login
process. This is particularly evident in 1.13 where there are no less
than 4 hash checks for basic authentication due to the previous
IsPasswordSet behaviour.

This duplicated code had individual inconsistencies that were not
helpful and caused confusion.

This PR does the following:

* Remove the specific login code from the git and lfs handlers except
for the lfs special bearer token
* Simplify the meaning of DisableBasicAuthentication to allow Token and
Oauth2 sign-in.
* The removal of the specific code from git and lfs means that these
both now have the same login semantics and can - if not
DisableBasicAuthentication - login from external services. Further it
allows Oauth2 token authentication as per our standard mechanisms.
* The change in the recovery handler prevents the service from
re-attempting to login - primarily because this could easily cause a
further panic and it is wasteful.

* add test

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: Andrew Thornton <art27@cantab.net>

integrations/api_repo_lfs_locks_test.go

@@ -44,7 +44,7 @@ func TestAPILFSLocksNotLogin(t *testing.T) {
 	resp := MakeRequest(t, req, http.StatusUnauthorized)
 	var lfsLockError api.LFSLockError
 	DecodeJSON(t, resp, &lfsLockError)
-	assert.Equal(t, "Unauthorized", lfsLockError.Message)
+	assert.Equal(t, "You must have pull access to list locks", lfsLockError.Message)
 }
 
 func TestAPILFSLocksLogged(t *testing.T) {

modules/auth/sso/basic.go

@@ -14,6 +14,7 @@ import (
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/timeutil"
+	"code.gitea.io/gitea/modules/web/middleware"
 )
 
 // Ensure the struct implements the interface.
@@ -40,7 +41,7 @@ func (b *Basic) Free() error {
 // IsEnabled returns true as this plugin is enabled by default and its not possible to disable
 // it from settings.
 func (b *Basic) IsEnabled() bool {
-	return setting.Service.EnableBasicAuth
+	return true
 }
 
 // VerifyAuthData extracts and validates Basic data (username and password/token) from the
@@ -48,17 +49,22 @@ func (b *Basic) IsEnabled() bool {
 // name/token on successful validation.
 // Returns nil if header is empty or validation fails.
 func (b *Basic) VerifyAuthData(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *models.User {
+
+	// Basic authentication should only fire on API, Download or on Git or LFSPaths
+	if middleware.IsInternalPath(req) || !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isGitOrLFSPath(req) {
+		return nil
+	}
+
 	baHead := req.Header.Get("Authorization")
 	if len(baHead) == 0 {
 		return nil
 	}
 
-	auths := strings.Fields(baHead)
+	auths := strings.SplitN(baHead, " ", 2)
 	if len(auths) != 2 || (auths[0] != "Basic" && auths[0] != "basic") {
 		return nil
 	}
 
-	var u *models.User
 	uname, passwd, _ := base.BasicAuthDecode(auths[1])
 
 	// Check if username or password is a token
@@ -76,20 +82,21 @@ func (b *Basic) VerifyAuthData(req *http.Request, w http.ResponseWriter, store D
 	uid := CheckOAuthAccessToken(authToken)
 	if uid != 0 {
 		log.Trace("Basic Authorization: Valid OAuthAccessToken for user[%d]", uid)
-		var err error
-		store.GetData()["IsApiToken"] = true
 
-		u, err = models.GetUserByID(uid)
+		u, err := models.GetUserByID(uid)
 		if err != nil {
 			log.Error("GetUserByID:  %v", err)
 			return nil
 		}
+
+		store.GetData()["IsApiToken"] = true
+		return u
 	}
+
 	token, err := models.GetAccessTokenBySHA(authToken)
 	if err == nil {
 		log.Trace("Basic Authorization: Valid AccessToken for user[%d]", uid)
-
+		u, err := models.GetUserByID(token.UID)
-		u, err = models.GetUserByID(token.UID)
 		if err != nil {
 			log.Error("GetUserByID:  %v", err)
 			return nil
@@ -99,22 +106,24 @@ func (b *Basic) VerifyAuthData(req *http.Request, w http.ResponseWriter, store D
 		if err = models.UpdateAccessToken(token); err != nil {
 			log.Error("UpdateAccessToken:  %v", err)
 		}
+
+		store.GetData()["IsApiToken"] = true
+		return u
 	} else if !models.IsErrAccessTokenNotExist(err) && !models.IsErrAccessTokenEmpty(err) {
 		log.Error("GetAccessTokenBySha: %v", err)
 	}
 
-	if u == nil {
+	if !setting.Service.EnableBasicAuth {
-		log.Trace("Basic Authorization: Attempting SignIn for %s", uname)
+		return nil
+	}
 
-		u, err = models.UserSignIn(uname, passwd)
+	log.Trace("Basic Authorization: Attempting SignIn for %s", uname)
-		if err != nil {
+	u, err := models.UserSignIn(uname, passwd)
-			if !models.IsErrUserNotExist(err) {
+	if err != nil {
-				log.Error("UserSignIn: %v", err)
+		if !models.IsErrUserNotExist(err) {
-			}
+			log.Error("UserSignIn: %v", err)
-			return nil
 		}
-	} else {
+		return nil
-		store.GetData()["IsApiToken"] = true
 	}
 
 	log.Trace("Basic Authorization: Logged in user %-v", u)

modules/auth/sso/sso.go

@@ -9,10 +9,12 @@ import (
 	"fmt"
 	"net/http"
 	"reflect"
+	"regexp"
 	"strings"
 
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/web/middleware"
 )
 
@@ -27,9 +29,9 @@ import (
 // for users that have already signed in.
 var ssoMethods = []SingleSignOn{
 	&OAuth2{},
+	&Basic{},
 	&Session{},
 	&ReverseProxy{},
-	&Basic{},
 }
 
 // The purpose of the following three function variables is to let the linter know that
@@ -102,6 +104,19 @@ func isAttachmentDownload(req *http.Request) bool {
 	return strings.HasPrefix(req.URL.Path, "/attachments/") && req.Method == "GET"
 }
 
+var gitPathRe = regexp.MustCompile(`^/[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+/(?:(?:git-(?:(?:upload)|(?:receive))-pack$)|(?:info/refs$)|(?:HEAD$)|(?:objects/))`)
+var lfsPathRe = regexp.MustCompile(`^/[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+/info/lfs/`)
+
+func isGitOrLFSPath(req *http.Request) bool {
+	if gitPathRe.MatchString(req.URL.Path) {
+		return true
+	}
+	if setting.LFS.StartServer {
+		return lfsPathRe.MatchString(req.URL.Path)
+	}
+	return false
+}
+
 // handleSignIn clears existing session variables and stores new ones for the specified user object
 func handleSignIn(resp http.ResponseWriter, req *http.Request, sess SessionStore, user *models.User) {
 	_ = sess.Delete("openid_verified_uri")

modules/auth/sso/sso_test.go

@@ -0,0 +1,124 @@
+// Copyright 2014 The Gogs Authors. All rights reserved.
+// Copyright 2019 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package sso
+
+import (
+	"net/http"
+	"testing"
+
+	"code.gitea.io/gitea/modules/setting"
+)
+
+func Test_isGitOrLFSPath(t *testing.T) {
+
+	tests := []struct {
+		path string
+
+		want bool
+	}{
+		{
+			"/owner/repo/git-upload-pack",
+			true,
+		},
+		{
+			"/owner/repo/git-receive-pack",
+			true,
+		},
+		{
+			"/owner/repo/info/refs",
+			true,
+		},
+		{
+			"/owner/repo/HEAD",
+			true,
+		},
+		{
+			"/owner/repo/objects/info/alternates",
+			true,
+		},
+		{
+			"/owner/repo/objects/info/http-alternates",
+			true,
+		},
+		{
+			"/owner/repo/objects/info/packs",
+			true,
+		},
+		{
+			"/owner/repo/objects/info/blahahsdhsdkla",
+			true,
+		},
+		{
+			"/owner/repo/objects/01/23456789abcdef0123456789abcdef01234567",
+			true,
+		},
+		{
+			"/owner/repo/objects/pack/pack-123456789012345678921234567893124567894.pack",
+			true,
+		},
+		{
+			"/owner/repo/objects/pack/pack-0123456789abcdef0123456789abcdef0123456.idx",
+			true,
+		},
+		{
+			"/owner/repo/stars",
+			false,
+		},
+		{
+			"/notowner",
+			false,
+		},
+		{
+			"/owner/repo",
+			false,
+		},
+		{
+			"/owner/repo/commit/123456789012345678921234567893124567894",
+			false,
+		},
+	}
+	lfsTests := []string{
+		"/owner/repo/info/lfs/",
+		"/owner/repo/info/lfs/objects/batch",
+		"/owner/repo/info/lfs/objects/oid/filename",
+		"/owner/repo/info/lfs/objects/oid",
+		"/owner/repo/info/lfs/objects",
+		"/owner/repo/info/lfs/verify",
+		"/owner/repo/info/lfs/locks",
+		"/owner/repo/info/lfs/locks/verify",
+		"/owner/repo/info/lfs/locks/123/unlock",
+	}
+
+	origLFSStartServer := setting.LFS.StartServer
+
+	for _, tt := range tests {
+		t.Run(tt.path, func(t *testing.T) {
+			req, _ := http.NewRequest("POST", "http://localhost"+tt.path, nil)
+			setting.LFS.StartServer = false
+			if got := isGitOrLFSPath(req); got != tt.want {
+				t.Errorf("isGitOrLFSPath() = %v, want %v", got, tt.want)
+			}
+			setting.LFS.StartServer = true
+			if got := isGitOrLFSPath(req); got != tt.want {
+				t.Errorf("isGitOrLFSPath() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+	for _, tt := range lfsTests {
+		t.Run(tt, func(t *testing.T) {
+			req, _ := http.NewRequest("POST", tt, nil)
+			setting.LFS.StartServer = false
+			if got := isGitOrLFSPath(req); got != setting.LFS.StartServer {
+				t.Errorf("isGitOrLFSPath(%q) = %v, want %v, %v", tt, got, setting.LFS.StartServer, gitPathRe.MatchString(tt))
+			}
+			setting.LFS.StartServer = true
+			if got := isGitOrLFSPath(req); got != setting.LFS.StartServer {
+				t.Errorf("isGitOrLFSPath(%q) = %v, want %v", tt, got, setting.LFS.StartServer)
+			}
+		})
+	}
+	setting.LFS.StartServer = origLFSStartServer
+}

modules/context/context.go

@@ -683,6 +683,9 @@ func Contexter() func(next http.Handler) http.Handler {
 			} else {
 				ctx.Data["SignedUserID"] = int64(0)
 				ctx.Data["SignedUserName"] = ""
+
+				// ensure the session uid is deleted
+				_ = ctx.Session.Delete("uid")
 			}
 
 			ctx.Resp.Header().Set(`X-Frame-Options`, `SAMEORIGIN`)

routers/private/internal.go

@@ -23,7 +23,7 @@ import (
 func CheckInternalToken(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 		tokens := req.Header.Get("Authorization")
-		fields := strings.Fields(tokens)
+		fields := strings.SplitN(tokens, " ", 2)
 		if len(fields) != 2 || fields[0] != "Bearer" || fields[1] != setting.InternalToken {
 			log.Debug("Forbidden attempt to access internal url: Authorization header: %s", tokens)
 			http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden)

routers/repo/http.go

@@ -22,15 +22,12 @@ import (
 	"time"
 
 	"code.gitea.io/gitea/models"
-	"code.gitea.io/gitea/modules/auth/sso"
-	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/process"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/structs"
-	"code.gitea.io/gitea/modules/timeutil"
 	"code.gitea.io/gitea/modules/util"
 	repo_service "code.gitea.io/gitea/services/repository"
 )
@@ -153,11 +150,8 @@ func httpBase(ctx *context.Context) (h *serviceHandler) {
 	// Only public pull don't need auth.
 	isPublicPull := repoExist && !repo.IsPrivate && isPull
 	var (
-		askAuth      = !isPublicPull || setting.Service.RequireSignInView
+		askAuth = !isPublicPull || setting.Service.RequireSignInView
-		authUser     *models.User
+		environ []string
-		authUsername string
-		authPasswd   string
-		environ      []string
 	)
 
 	// don't allow anonymous pulls if organization is not public
@@ -172,108 +166,33 @@ func httpBase(ctx *context.Context) (h *serviceHandler) {
 
 	// check access
 	if askAuth {
-		authUsername = ctx.Req.Header.Get(setting.ReverseProxyAuthUser)
+		// rely on the results of Contexter
-		if setting.Service.EnableReverseProxyAuth && len(authUsername) > 0 {
+		if !ctx.IsSigned {
-			authUser, err = models.GetUserByName(authUsername)
+			// TODO: support digit auth - which would be Authorization header with digit
-			if err != nil {
+			ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=\".\"")
-				ctx.HandleText(401, "reverse proxy login error, got error while running GetUserByName")
+			ctx.Error(http.StatusUnauthorized)
-				return
+			return
-			}
+		}
-		} else {
-			authHead := ctx.Req.Header.Get("Authorization")
-			if len(authHead) == 0 {
-				ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=\".\"")
-				ctx.Error(http.StatusUnauthorized)
-				return
-			}
 
-			auths := strings.Fields(authHead)
+		if ctx.IsBasicAuth {
-			// currently check basic auth
+			_, err = models.GetTwoFactorByUID(ctx.User.ID)
-			// TODO: support digit auth
-			// FIXME: middlewares/context.go did basic auth check already,
-			// maybe could use that one.
-			if len(auths) != 2 || auths[0] != "Basic" {
-				ctx.HandleText(http.StatusUnauthorized, "no basic auth and digit auth")
-				return
-			}
-			authUsername, authPasswd, err = base.BasicAuthDecode(auths[1])
-			if err != nil {
-				ctx.HandleText(http.StatusUnauthorized, "no basic auth and digit auth")
-				return
-			}
-
-			// Check if username or password is a token
-			isUsernameToken := len(authPasswd) == 0 || authPasswd == "x-oauth-basic"
-			// Assume username is token
-			authToken := authUsername
-			if !isUsernameToken {
-				// Assume password is token
-				authToken = authPasswd
-			}
-			uid := sso.CheckOAuthAccessToken(authToken)
-			if uid != 0 {
-				ctx.Data["IsApiToken"] = true
-
-				authUser, err = models.GetUserByID(uid)
-				if err != nil {
-					ctx.ServerError("GetUserByID", err)
-					return
-				}
-			}
-			// Assume password is a token.
-			token, err := models.GetAccessTokenBySHA(authToken)
 			if err == nil {
-				authUser, err = models.GetUserByID(token.UID)
+				// TODO: This response should be changed to "invalid credentials" for security reasons once the expectation behind it (creating an app token to authenticate) is properly documented
-				if err != nil {
+				ctx.HandleText(http.StatusUnauthorized, "Users with two-factor authentication enabled cannot perform HTTP/HTTPS operations via plain username and password. Please create and use a personal access token on the user settings page")
-					ctx.ServerError("GetUserByID", err)
+				return
-					return
+			} else if !models.IsErrTwoFactorNotEnrolled(err) {
-				}
+				ctx.ServerError("IsErrTwoFactorNotEnrolled", err)
-
+				return
-				token.UpdatedUnix = timeutil.TimeStampNow()
-				if err = models.UpdateAccessToken(token); err != nil {
-					ctx.ServerError("UpdateAccessToken", err)
-				}
-			} else if !models.IsErrAccessTokenNotExist(err) && !models.IsErrAccessTokenEmpty(err) {
-				log.Error("GetAccessTokenBySha: %v", err)
-			}
-
-			if authUser == nil {
-				// Check username and password
-				authUser, err = models.UserSignIn(authUsername, authPasswd)
-				if err != nil {
-					if models.IsErrUserProhibitLogin(err) {
-						ctx.HandleText(http.StatusForbidden, "User is not permitted to login")
-						return
-					} else if !models.IsErrUserNotExist(err) {
-						ctx.ServerError("UserSignIn error: %v", err)
-						return
-					}
-				}
-
-				if authUser == nil {
-					ctx.HandleText(http.StatusUnauthorized, fmt.Sprintf("invalid credentials from %s", ctx.RemoteAddr()))
-					return
-				}
-
-				_, err = models.GetTwoFactorByUID(authUser.ID)
-				if err == nil {
-					// TODO: This response should be changed to "invalid credentials" for security reasons once the expectation behind it (creating an app token to authenticate) is properly documented
-					ctx.HandleText(http.StatusUnauthorized, "Users with two-factor authentication enabled cannot perform HTTP/HTTPS operations via plain username and password. Please create and use a personal access token on the user settings page")
-					return
-				} else if !models.IsErrTwoFactorNotEnrolled(err) {
-					ctx.ServerError("IsErrTwoFactorNotEnrolled", err)
-					return
-				}
 			}
 		}
 
-		if !authUser.IsActive || authUser.ProhibitLogin {
+		if !ctx.User.IsActive || ctx.User.ProhibitLogin {
 			ctx.HandleText(http.StatusForbidden, "Your account is disabled.")
 			return
 		}
 
 		if repoExist {
-			perm, err := models.GetUserRepoPermission(repo, authUser)
+			perm, err := models.GetUserRepoPermission(repo, ctx.User)
 			if err != nil {
 				ctx.ServerError("GetUserRepoPermission", err)
 				return
@@ -293,14 +212,14 @@ func httpBase(ctx *context.Context) (h *serviceHandler) {
 		environ = []string{
 			models.EnvRepoUsername + "=" + username,
 			models.EnvRepoName + "=" + reponame,
-			models.EnvPusherName + "=" + authUser.Name,
+			models.EnvPusherName + "=" + ctx.User.Name,
-			models.EnvPusherID + fmt.Sprintf("=%d", authUser.ID),
+			models.EnvPusherID + fmt.Sprintf("=%d", ctx.User.ID),
 			models.EnvIsDeployKey + "=false",
 			models.EnvAppURL + "=" + setting.AppURL,
 		}
 
-		if !authUser.KeepEmailPrivate {
+		if !ctx.User.KeepEmailPrivate {
-			environ = append(environ, models.EnvPusherEmail+"="+authUser.Email)
+			environ = append(environ, models.EnvPusherEmail+"="+ctx.User.Email)
 		}
 
 		if isWiki {
@@ -336,7 +255,7 @@ func httpBase(ctx *context.Context) (h *serviceHandler) {
 			return
 		}
 
-		repo, err = repo_service.PushCreateRepo(authUser, owner, reponame)
+		repo, err = repo_service.PushCreateRepo(ctx.User, owner, reponame)
 		if err != nil {
 			log.Error("pushCreateRepo: %v", err)
 			ctx.Status(http.StatusNotFound)

routers/routes/base.go

@@ -15,6 +15,7 @@ import (
 	"strings"
 	"time"
 
+	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/auth/sso"
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/httpcache"
@@ -171,8 +172,19 @@ func Recovery() func(next http.Handler) http.Handler {
 						},
 					}
 
-					// Get user from session if logged in.
+					var user *models.User
-					user, _ := sso.SignedInUser(req, w, &store, sessionStore)
+					if apiContext := context.GetAPIContext(req); apiContext != nil {
+						user = apiContext.User
+					}
+					if user == nil {
+						if ctx := context.GetContext(req); ctx != nil {
+							user = ctx.User
+						}
+					}
+					if user == nil {
+						// Get user from session if logged in - do not attempt to sign-in
+						user = sso.SessionUser(sessionStore)
+					}
 					if user != nil {
 						store.Data["IsSigned"] = true
 						store.Data["SignedUser"] = user

services/lfs/locks.go

@@ -31,15 +31,6 @@ func checkIsValidRequest(ctx *context.Context) bool {
 		writeStatus(ctx, http.StatusBadRequest)
 		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, http.StatusUnauthorized)
-			return false
-		}
-		ctx.User = user
-	}
 	return true
 }
 
@@ -73,19 +64,21 @@ func GetListLockHandler(ctx *context.Context) {
 		// Status is written in checkIsValidRequest
 		return
 	}
-	ctx.Resp.Header().Set("Content-Type", lfs_module.MediaType)
 
 	rv, _ := unpack(ctx)
 
 	repository, err := models.GetRepositoryByOwnerAndName(rv.User, rv.Repo)
 	if err != nil {
 		log.Debug("Could not find repository: %s/%s - %s", rv.User, rv.Repo, err)
-		writeStatus(ctx, 404)
+		ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs")
+		ctx.JSON(401, api.LFSLockError{
+			Message: "You must have pull access to list locks",
+		})
 		return
 	}
 	repository.MustOwner()
 
-	authenticated := authenticate(ctx, repository, rv.Authorization, false)
+	authenticated := authenticate(ctx, repository, rv.Authorization, true, false)
 	if !authenticated {
 		ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs")
 		ctx.JSON(http.StatusUnauthorized, api.LFSLockError{
@@ -93,6 +86,7 @@ func GetListLockHandler(ctx *context.Context) {
 		})
 		return
 	}
+	ctx.Resp.Header().Set("Content-Type", lfs_module.MediaType)
 
 	cursor := ctx.QueryInt("cursor")
 	if cursor < 0 {
@@ -160,7 +154,6 @@ func PostLockHandler(ctx *context.Context) {
 		// Status is written in checkIsValidRequest
 		return
 	}
-	ctx.Resp.Header().Set("Content-Type", lfs_module.MediaType)
 
 	userName := ctx.Params("username")
 	repoName := strings.TrimSuffix(ctx.Params("reponame"), ".git")
@@ -169,12 +162,15 @@ func PostLockHandler(ctx *context.Context) {
 	repository, err := models.GetRepositoryByOwnerAndName(userName, repoName)
 	if err != nil {
 		log.Error("Unable to get repository: %s/%s Error: %v", userName, repoName, err)
-		writeStatus(ctx, 404)
+		ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs")
+		ctx.JSON(401, api.LFSLockError{
+			Message: "You must have push access to create locks",
+		})
 		return
 	}
 	repository.MustOwner()
 
-	authenticated := authenticate(ctx, repository, authorization, true)
+	authenticated := authenticate(ctx, repository, authorization, true, true)
 	if !authenticated {
 		ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs")
 		ctx.JSON(http.StatusUnauthorized, api.LFSLockError{
@@ -183,6 +179,8 @@ func PostLockHandler(ctx *context.Context) {
 		return
 	}
 
+	ctx.Resp.Header().Set("Content-Type", lfs_module.MediaType)
+
 	var req api.LFSLockRequest
 	bodyReader := ctx.Req.Body
 	defer bodyReader.Close()
@@ -229,7 +227,6 @@ func VerifyLockHandler(ctx *context.Context) {
 		// Status is written in checkIsValidRequest
 		return
 	}
-	ctx.Resp.Header().Set("Content-Type", lfs_module.MediaType)
 
 	userName := ctx.Params("username")
 	repoName := strings.TrimSuffix(ctx.Params("reponame"), ".git")
@@ -238,12 +235,15 @@ func VerifyLockHandler(ctx *context.Context) {
 	repository, err := models.GetRepositoryByOwnerAndName(userName, repoName)
 	if err != nil {
 		log.Error("Unable to get repository: %s/%s Error: %v", userName, repoName, err)
-		writeStatus(ctx, 404)
+		ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs")
+		ctx.JSON(401, api.LFSLockError{
+			Message: "You must have push access to verify locks",
+		})
 		return
 	}
 	repository.MustOwner()
 
-	authenticated := authenticate(ctx, repository, authorization, true)
+	authenticated := authenticate(ctx, repository, authorization, true, true)
 	if !authenticated {
 		ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs")
 		ctx.JSON(http.StatusUnauthorized, api.LFSLockError{
@@ -252,6 +252,8 @@ func VerifyLockHandler(ctx *context.Context) {
 		return
 	}
 
+	ctx.Resp.Header().Set("Content-Type", lfs_module.MediaType)
+
 	cursor := ctx.QueryInt("cursor")
 	if cursor < 0 {
 		cursor = 0
@@ -296,7 +298,6 @@ func UnLockHandler(ctx *context.Context) {
 		// Status is written in checkIsValidRequest
 		return
 	}
-	ctx.Resp.Header().Set("Content-Type", lfs_module.MediaType)
 
 	userName := ctx.Params("username")
 	repoName := strings.TrimSuffix(ctx.Params("reponame"), ".git")
@@ -305,12 +306,15 @@ func UnLockHandler(ctx *context.Context) {
 	repository, err := models.GetRepositoryByOwnerAndName(userName, repoName)
 	if err != nil {
 		log.Error("Unable to get repository: %s/%s Error: %v", userName, repoName, err)
-		writeStatus(ctx, 404)
+		ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs")
+		ctx.JSON(401, api.LFSLockError{
+			Message: "You must have push access to delete locks",
+		})
 		return
 	}
 	repository.MustOwner()
 
-	authenticated := authenticate(ctx, repository, authorization, true)
+	authenticated := authenticate(ctx, repository, authorization, true, true)
 	if !authenticated {
 		ctx.Resp.Header().Set("WWW-Authenticate", "Basic realm=gitea-lfs")
 		ctx.JSON(http.StatusUnauthorized, api.LFSLockError{
@@ -319,6 +323,8 @@ func UnLockHandler(ctx *context.Context) {
 		return
 	}
 
+	ctx.Resp.Header().Set("Content-Type", lfs_module.MediaType)
+
 	var req api.LFSLockDeleteRequest
 	bodyReader := ctx.Req.Body
 	defer bodyReader.Close()

services/lfs/server.go

@@ -94,7 +94,7 @@ func getAuthenticatedRepoAndMeta(ctx *context.Context, rc *requestContext, p lfs
 		return nil, nil
 	}
 
-	if !authenticate(ctx, repository, rc.Authorization, requireWrite) {
+	if !authenticate(ctx, repository, rc.Authorization, false, requireWrite) {
 		requireAuth(ctx)
 		return nil, nil
 	}
@@ -232,7 +232,7 @@ func PostHandler(ctx *context.Context) {
 		return
 	}
 
-	if !authenticate(ctx, repository, rc.Authorization, true) {
+	if !authenticate(ctx, repository, rc.Authorization, false, true) {
 		requireAuth(ctx)
 		return
 	}
@@ -322,7 +322,7 @@ func BatchHandler(ctx *context.Context) {
 			requireWrite = true
 		}
 
-		if !authenticate(ctx, repository, reqCtx.Authorization, requireWrite) {
+		if !authenticate(ctx, repository, reqCtx.Authorization, false, requireWrite) {
 			requireAuth(ctx)
 			return
 		}
@@ -561,7 +561,7 @@ func logRequest(r *http.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 {
+func authenticate(ctx *context.Context, repository *models.Repository, authorization string, requireSigned, requireWrite bool) bool {
 	accessMode := models.AccessModeRead
 	if requireWrite {
 		accessMode = models.AccessModeWrite
@@ -575,89 +575,72 @@ func authenticate(ctx *context.Context, repository *models.Repository, authoriza
 	}
 
 	canRead := perm.CanAccess(accessMode, models.UnitTypeCode)
-	if canRead {
+	if canRead && (!requireSigned || ctx.IsSigned) {
 		return true
 	}
 
-	user, repo, opStr, err := parseToken(authorization)
+	user, err := parseToken(authorization, repository, accessMode)
 	if err != nil {
 		// Most of these are Warn level - the true internal server errors are logged in parseToken already
 		log.Warn("Authentication failure for provided token with Error: %v", err)
 		return false
 	}
 	ctx.User = user
-	if opStr == "basic" {
+	return true
-		perm, err = models.GetUserRepoPermission(repository, ctx.User)
-		if err != nil {
-			log.Error("Unable to GetUserRepoPermission for user %-v in repo %-v Error: %v", ctx.User, repository)
-			return false
-		}
-		return perm.CanAccess(accessMode, models.UnitTypeCode)
-	}
-	if repository.ID == repo.ID {
-		if requireWrite && opStr != "upload" {
-			return false
-		}
-		return true
-	}
-	return false
 }
 
-func parseToken(authorization string) (*models.User, *models.Repository, string, error) {
+func handleLFSToken(tokenSHA string, target *models.Repository, mode models.AccessMode) (*models.User, error) {
+	if !strings.Contains(tokenSHA, ".") {
+		return nil, nil
+	}
+	token, err := jwt.ParseWithClaims(tokenSHA, &Claims{}, func(t *jwt.Token) (interface{}, error) {
+		if _, ok := t.Method.(*jwt.SigningMethodHMAC); !ok {
+			return nil, fmt.Errorf("unexpected signing method: %v", t.Header["alg"])
+		}
+		return setting.LFS.JWTSecretBytes, nil
+	})
+	if err != nil {
+		return nil, nil
+	}
+
+	claims, claimsOk := token.Claims.(*Claims)
+	if !token.Valid || !claimsOk {
+		return nil, fmt.Errorf("invalid token claim")
+	}
+
+	if claims.RepoID != target.ID {
+		return nil, fmt.Errorf("invalid token claim")
+	}
+
+	if mode == models.AccessModeWrite && claims.Op != "upload" {
+		return nil, fmt.Errorf("invalid token claim")
+	}
+
+	u, err := models.GetUserByID(claims.UserID)
+	if err != nil {
+		log.Error("Unable to GetUserById[%d]: Error: %v", claims.UserID, err)
+		return nil, err
+	}
+	return u, nil
+}
+
+func parseToken(authorization string, target *models.Repository, mode models.AccessMode) (*models.User, error) {
 	if authorization == "" {
-		return nil, nil, "unknown", fmt.Errorf("No token")
+		return nil, fmt.Errorf("no token")
-	}
-	if strings.HasPrefix(authorization, "Bearer ") {
-		token, err := jwt.ParseWithClaims(authorization[7:], &Claims{}, func(t *jwt.Token) (interface{}, error) {
-			if _, ok := t.Method.(*jwt.SigningMethodHMAC); !ok {
-				return nil, fmt.Errorf("unexpected signing method: %v", t.Header["alg"])
-			}
-			return setting.LFS.JWTSecretBytes, nil
-		})
-		if err != nil {
-			// The error here is WARN level because it is caused by bad authorization rather than an internal server error
-			return nil, nil, "unknown", err
-		}
-		claims, claimsOk := token.Claims.(*Claims)
-		if !token.Valid || !claimsOk {
-			return nil, nil, "unknown", fmt.Errorf("Token claim invalid")
-		}
-		r, err := models.GetRepositoryByID(claims.RepoID)
-		if err != nil {
-			log.Error("Unable to GetRepositoryById[%d]: Error: %v", claims.RepoID, err)
-			return nil, nil, claims.Op, err
-		}
-		u, err := models.GetUserByID(claims.UserID)
-		if err != nil {
-			log.Error("Unable to GetUserById[%d]: Error: %v", claims.UserID, err)
-			return nil, r, claims.Op, err
-		}
-		return u, r, claims.Op, nil
 	}
 
-	if strings.HasPrefix(authorization, "Basic ") {
+	parts := strings.SplitN(authorization, " ", 2)
-		c, err := base64.StdEncoding.DecodeString(strings.TrimPrefix(authorization, "Basic "))
+	if len(parts) != 2 {
-		if err != nil {
+		return nil, fmt.Errorf("no token")
-			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 {
-			log.Error("Unable to GetUserByName[%d]: Error: %v", user, err)
-			return nil, nil, "basic", err
-		}
-		if !u.IsPasswordSet() || !u.ValidatePassword(password) {
-			return nil, nil, "basic", fmt.Errorf("Basic auth failed")
-		}
-		return u, nil, "basic", nil
 	}
-
+	tokenSHA := parts[1]
-	return nil, nil, "unknown", fmt.Errorf("Token not found")
+	switch strings.ToLower(parts[0]) {
+	case "bearer":
+		fallthrough
+	case "token":
+		return handleLFSToken(tokenSHA, target, mode)
+	}
+	return nil, fmt.Errorf("token not found")
 }
 
 func requireAuth(ctx *context.Context) {