Fix the bug: deploy key with write access can not push (#19010)

Use DeployKeyID to replace the IsDeployKey, then CanWriteCode uses the DeployKeyID to check the write permission.

cmd/hook.go

@@ -185,7 +185,7 @@ Gitea or set your environment appropriately.`, "")
 	reponame := os.Getenv(models.EnvRepoName)
 	userID, _ := strconv.ParseInt(os.Getenv(models.EnvPusherID), 10, 64)
 	prID, _ := strconv.ParseInt(os.Getenv(models.EnvPRID), 10, 64)
-	isDeployKey, _ := strconv.ParseBool(os.Getenv(models.EnvIsDeployKey))
+	deployKeyID, _ := strconv.ParseInt(os.Getenv(models.EnvDeployKeyID), 10, 64)
 
 	hookOptions := private.HookOptions{
 		UserID:                          userID,
@@ -194,7 +194,7 @@ Gitea or set your environment appropriately.`, "")
 		GitQuarantinePath:               os.Getenv(private.GitQuarantinePath),
 		GitPushOptions:                  pushOptions(),
 		PullRequestID:                   prID,
-		IsDeployKey:                     isDeployKey,
+		DeployKeyID:                     deployKeyID,
 	}
 
 	scanner := bufio.NewScanner(os.Stdin)

cmd/serv.go

@@ -243,7 +243,7 @@ func runServ(c *cli.Context) error {
 	os.Setenv(models.EnvPusherID, strconv.FormatInt(results.UserID, 10))
 	os.Setenv(models.EnvRepoID, strconv.FormatInt(results.RepoID, 10))
 	os.Setenv(models.EnvPRID, fmt.Sprintf("%d", 0))
-	os.Setenv(models.EnvIsDeployKey, fmt.Sprintf("%t", results.IsDeployKey))
+	os.Setenv(models.EnvDeployKeyID, fmt.Sprintf("%d", results.DeployKeyID))
 	os.Setenv(models.EnvKeyID, fmt.Sprintf("%d", results.KeyID))
 	os.Setenv(models.EnvAppURL, setting.AppURL)
 

integrations/api_private_serv_test.go

@@ -47,7 +47,7 @@ func TestAPIPrivateServ(t *testing.T) {
 		results, err := private.ServCommand(ctx, 1, "user2", "repo1", perm.AccessModeWrite, "git-upload-pack", "")
 		assert.NoError(t, err)
 		assert.False(t, results.IsWiki)
-		assert.False(t, results.IsDeployKey)
+		assert.Zero(t, results.DeployKeyID)
 		assert.Equal(t, int64(1), results.KeyID)
 		assert.Equal(t, "user2@localhost", results.KeyName)
 		assert.Equal(t, "user2", results.UserName)
@@ -70,7 +70,7 @@ func TestAPIPrivateServ(t *testing.T) {
 		results, err = private.ServCommand(ctx, 1, "user15", "big_test_public_1", perm.AccessModeRead, "git-upload-pack", "")
 		assert.NoError(t, err)
 		assert.False(t, results.IsWiki)
-		assert.False(t, results.IsDeployKey)
+		assert.Zero(t, results.DeployKeyID)
 		assert.Equal(t, int64(1), results.KeyID)
 		assert.Equal(t, "user2@localhost", results.KeyName)
 		assert.Equal(t, "user2", results.UserName)
@@ -92,7 +92,7 @@ func TestAPIPrivateServ(t *testing.T) {
 		results, err = private.ServCommand(ctx, deployKey.KeyID, "user15", "big_test_private_1", perm.AccessModeRead, "git-upload-pack", "")
 		assert.NoError(t, err)
 		assert.False(t, results.IsWiki)
-		assert.True(t, results.IsDeployKey)
+		assert.NotZero(t, results.DeployKeyID)
 		assert.Equal(t, deployKey.KeyID, results.KeyID)
 		assert.Equal(t, "test-deploy", results.KeyName)
 		assert.Equal(t, "user15", results.UserName)
@@ -129,7 +129,7 @@ func TestAPIPrivateServ(t *testing.T) {
 		results, err = private.ServCommand(ctx, deployKey.KeyID, "user15", "big_test_private_2", perm.AccessModeRead, "git-upload-pack", "")
 		assert.NoError(t, err)
 		assert.False(t, results.IsWiki)
-		assert.True(t, results.IsDeployKey)
+		assert.NotZero(t, results.DeployKeyID)
 		assert.Equal(t, deployKey.KeyID, results.KeyID)
 		assert.Equal(t, "test-deploy", results.KeyName)
 		assert.Equal(t, "user15", results.UserName)
@@ -142,7 +142,7 @@ func TestAPIPrivateServ(t *testing.T) {
 		results, err = private.ServCommand(ctx, deployKey.KeyID, "user15", "big_test_private_2", perm.AccessModeWrite, "git-upload-pack", "")
 		assert.NoError(t, err)
 		assert.False(t, results.IsWiki)
-		assert.True(t, results.IsDeployKey)
+		assert.NotZero(t, results.DeployKeyID)
 		assert.Equal(t, deployKey.KeyID, results.KeyID)
 		assert.Equal(t, "test-deploy", results.KeyName)
 		assert.Equal(t, "user15", results.UserName)

models/asymkey/ssh_key_deploy.go

@@ -58,7 +58,7 @@ func (key *DeployKey) GetContent() error {
 	return nil
 }
 
-// IsReadOnly checks if the key can only be used for read operations
+// IsReadOnly checks if the key can only be used for read operations, used by template
 func (key *DeployKey) IsReadOnly() bool {
 	return key.Mode == perm.AccessModeRead
 }
@@ -203,12 +203,6 @@ func UpdateDeployKeyCols(key *DeployKey, cols ...string) error {
 	return err
 }
 
-// UpdateDeployKey updates deploy key information.
-func UpdateDeployKey(key *DeployKey) error {
-	_, err := db.GetEngine(db.DefaultContext).ID(key.ID).AllCols().Update(key)
-	return err
-}
-
 // ListDeployKeysOptions are options for ListDeployKeys
 type ListDeployKeysOptions struct {
 	db.ListOptions

models/helper_environment.go

@@ -23,8 +23,8 @@ const (
 	EnvPusherName   = "GITEA_PUSHER_NAME"
 	EnvPusherEmail  = "GITEA_PUSHER_EMAIL"
 	EnvPusherID     = "GITEA_PUSHER_ID"
-	EnvKeyID        = "GITEA_KEY_ID"
-	EnvIsDeployKey  = "GITEA_IS_DEPLOY_KEY"
+	EnvKeyID        = "GITEA_KEY_ID" // public key ID
+	EnvDeployKeyID  = "GITEA_DEPLOY_KEY_ID"
 	EnvPRID         = "GITEA_PR_ID"
 	EnvIsInternal   = "GITEA_INTERNAL_PUSH"
 	EnvAppURL       = "GITEA_ROOT_URL"

modules/private/hook.go

@@ -56,7 +56,7 @@ type HookOptions struct {
 	GitQuarantinePath               string
 	GitPushOptions                  GitPushOptions
 	PullRequestID                   int64
-	IsDeployKey                     bool
+	DeployKeyID                     int64 // if the pusher is a DeployKey, then UserID is the repo's org user.
 	IsWiki                          bool
 }
 

modules/private/serv.go

@@ -46,9 +46,9 @@ func ServNoCommand(ctx context.Context, keyID int64) (*asymkey_model.PublicKey,
 // ServCommandResults are the results of a call to the private route serv
 type ServCommandResults struct {
 	IsWiki      bool
-	IsDeployKey bool
-	KeyID       int64
-	KeyName     string
+	DeployKeyID int64
+	KeyID       int64  // public key
+	KeyName     string // this field is ambiguous, it can be the name of DeployKey, or the name of the PublicKey
 	UserName    string
 	UserEmail   string
 	UserID      int64

routers/api/v1/repo/key.go

@@ -144,7 +144,7 @@ func GetDeployKey(ctx *context.APIContext) {
 	//   "200":
 	//     "$ref": "#/responses/DeployKey"
 
-	key, err := asymkey_model.GetDeployKeyByID(db.DefaultContext, ctx.ParamsInt64(":id"))
+	key, err := asymkey_model.GetDeployKeyByID(ctx, ctx.ParamsInt64(":id"))
 	if err != nil {
 		if asymkey_model.IsErrDeployKeyNotExist(err) {
 			ctx.NotFound()

routers/private/hook_pre_receive.go

@@ -12,6 +12,8 @@ import (
 	"strings"
 
 	"code.gitea.io/gitea/models"
+	asymkey_model "code.gitea.io/gitea/models/asymkey"
+	perm_model "code.gitea.io/gitea/models/perm"
 	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
 	gitea_context "code.gitea.io/gitea/modules/context"
@@ -24,8 +26,12 @@ import (
 
 type preReceiveContext struct {
 	*gitea_context.PrivateContext
-	user *user_model.User
-	perm models.Permission
+
+	// loadedPusher indicates that where the following information are loaded
+	loadedPusher        bool
+	user                *user_model.User // it's the org user if a DeployKey is used
+	userPerm            models.Permission
+	deployKeyAccessMode perm_model.AccessMode
 
 	canCreatePullRequest        bool
 	checkedCanCreatePullRequest bool
@@ -41,62 +47,52 @@ type preReceiveContext struct {
 	opts *private.HookOptions
 }
 
-// User gets or loads User
-func (ctx *preReceiveContext) User() *user_model.User {
-	if ctx.user == nil {
-		ctx.user, ctx.perm = loadUserAndPermission(ctx.PrivateContext, ctx.opts.UserID)
-	}
-	return ctx.user
-}
-
-// Perm gets or loads Perm
-func (ctx *preReceiveContext) Perm() *models.Permission {
-	if ctx.user == nil {
-		ctx.user, ctx.perm = loadUserAndPermission(ctx.PrivateContext, ctx.opts.UserID)
-	}
-	return &ctx.perm
-}
-
-// CanWriteCode returns true if can write code
+// CanWriteCode returns true if pusher can write code
 func (ctx *preReceiveContext) CanWriteCode() bool {
 	if !ctx.checkedCanWriteCode {
-		ctx.canWriteCode = ctx.Perm().CanWrite(unit.TypeCode)
+		if !ctx.loadPusherAndPermission() {
+			return false
+		}
+		ctx.canWriteCode = ctx.userPerm.CanWrite(unit.TypeCode) || ctx.deployKeyAccessMode >= perm_model.AccessModeWrite
 		ctx.checkedCanWriteCode = true
 	}
 	return ctx.canWriteCode
 }
 
-// AssertCanWriteCode returns true if can write code
+// AssertCanWriteCode returns true if pusher can write code
 func (ctx *preReceiveContext) AssertCanWriteCode() bool {
 	if !ctx.CanWriteCode() {
 		if ctx.Written() {
 			return false
 		}
 		ctx.JSON(http.StatusForbidden, map[string]interface{}{
-			"err": "User permission denied.",
+			"err": "User permission denied for writing.",
 		})
 		return false
 	}
 	return true
 }
 
-// CanCreatePullRequest returns true if can create pull requests
+// CanCreatePullRequest returns true if pusher can create pull requests
 func (ctx *preReceiveContext) CanCreatePullRequest() bool {
 	if !ctx.checkedCanCreatePullRequest {
-		ctx.canCreatePullRequest = ctx.Perm().CanRead(unit.TypePullRequests)
+		if !ctx.loadPusherAndPermission() {
+			return false
+		}
+		ctx.canCreatePullRequest = ctx.userPerm.CanRead(unit.TypePullRequests)
 		ctx.checkedCanCreatePullRequest = true
 	}
 	return ctx.canCreatePullRequest
 }
 
-// AssertCanCreatePullRequest returns true if can create pull requests
+// AssertCreatePullRequest returns true if can create pull requests
 func (ctx *preReceiveContext) AssertCreatePullRequest() bool {
 	if !ctx.CanCreatePullRequest() {
 		if ctx.Written() {
 			return false
 		}
 		ctx.JSON(http.StatusForbidden, map[string]interface{}{
-			"err": "User permission denied.",
+			"err": "User permission denied for creating pull-request.",
 		})
 		return false
 	}
@@ -246,7 +242,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID, refFullN
 
 	// 5. Check if the doer is allowed to push
 	canPush := false
-	if ctx.opts.IsDeployKey {
+	if ctx.opts.DeployKeyID != 0 {
 		canPush = !changedProtectedfiles && protectBranch.CanPush && (!protectBranch.EnableWhitelist || protectBranch.WhitelistDeployKeys)
 	} else {
 		canPush = !changedProtectedfiles && protectBranch.CanUserPush(ctx.opts.UserID)
@@ -303,9 +299,15 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID, refFullN
 			return
 		}
 
+		// although we should have called `loadPusherAndPermission` before, here we call it explicitly again because we need to access ctx.user below
+		if !ctx.loadPusherAndPermission() {
+			// if error occurs, loadPusherAndPermission had written the error response
+			return
+		}
+
 		// Now check if the user is allowed to merge PRs for this repository
 		// Note: we can use ctx.perm and ctx.user directly as they will have been loaded above
-		allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, ctx.perm, ctx.user)
+		allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, ctx.userPerm, ctx.user)
 		if err != nil {
 			log.Error("Error calculating if allowed to merge: %v", err)
 			ctx.JSON(http.StatusInternalServerError, private.Response{
@@ -323,7 +325,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID, refFullN
 		}
 
 		// If we're an admin for the repository we can ignore status checks, reviews and override protected files
-		if ctx.perm.IsAdmin() {
+		if ctx.userPerm.IsAdmin() {
 			return
 		}
 
@@ -450,24 +452,44 @@ func generateGitEnv(opts *private.HookOptions) (env []string) {
 	return env
 }
 
-func loadUserAndPermission(ctx *gitea_context.PrivateContext, id int64) (user *user_model.User, perm models.Permission) {
-	user, err := user_model.GetUserByID(id)
-	if err != nil {
-		log.Error("Unable to get User id %d Error: %v", id, err)
-		ctx.JSON(http.StatusInternalServerError, private.Response{
-			Err: fmt.Sprintf("Unable to get User id %d Error: %v", id, err),
-		})
-		return
+// loadPusherAndPermission returns false if an error occurs, and it writes the error response
+func (ctx *preReceiveContext) loadPusherAndPermission() bool {
+	if ctx.loadedPusher {
+		return true
 	}
 
-	perm, err = models.GetUserRepoPermission(ctx.Repo.Repository, user)
+	user, err := user_model.GetUserByID(ctx.opts.UserID)
+	if err != nil {
+		log.Error("Unable to get User id %d Error: %v", ctx.opts.UserID, err)
+		ctx.JSON(http.StatusInternalServerError, private.Response{
+			Err: fmt.Sprintf("Unable to get User id %d Error: %v", ctx.opts.UserID, err),
+		})
+		return false
+	}
+	ctx.user = user
+
+	userPerm, err := models.GetUserRepoPermission(ctx.Repo.Repository, user)
 	if err != nil {
 		log.Error("Unable to get Repo permission of repo %s/%s of User %s", ctx.Repo.Repository.OwnerName, ctx.Repo.Repository.Name, user.Name, err)
 		ctx.JSON(http.StatusInternalServerError, private.Response{
 			Err: fmt.Sprintf("Unable to get Repo permission of repo %s/%s of User %s: %v", ctx.Repo.Repository.OwnerName, ctx.Repo.Repository.Name, user.Name, err),
 		})
-		return
+		return false
+	}
+	ctx.userPerm = userPerm
+
+	if ctx.opts.DeployKeyID != 0 {
+		deployKey, err := asymkey_model.GetDeployKeyByID(ctx, ctx.opts.DeployKeyID)
+		if err != nil {
+			log.Error("Unable to get DeployKey id %d Error: %v", ctx.opts.DeployKeyID, err)
+			ctx.JSON(http.StatusInternalServerError, private.Response{
+				Err: fmt.Sprintf("Unable to get DeployKey id %d Error: %v", ctx.opts.DeployKeyID, err),
+			})
+			return false
+		}
+		ctx.deployKeyAccessMode = deployKey.Mode
 	}
 
-	return
+	ctx.loadedPusher = true
+	return true
 }

routers/private/serv.go

@@ -229,8 +229,6 @@ func ServCommand(ctx *context.PrivateContext) {
 	var deployKey *asymkey_model.DeployKey
 	var user *user_model.User
 	if key.Type == asymkey_model.KeyTypeDeploy {
-		results.IsDeployKey = true
-
 		var err error
 		deployKey, err = asymkey_model.GetDeployKeyByRepo(key.ID, repo.ID)
 		if err != nil {
@@ -248,6 +246,7 @@ func ServCommand(ctx *context.PrivateContext) {
 			})
 			return
 		}
+		results.DeployKeyID = deployKey.ID
 		results.KeyName = deployKey.Name
 
 		// FIXME: Deploy keys aren't really the owner of the repo pushing changes
@@ -410,9 +409,9 @@ func ServCommand(ctx *context.PrivateContext) {
 			return
 		}
 	}
-	log.Debug("Serv Results:\nIsWiki: %t\nIsDeployKey: %t\nKeyID: %d\tKeyName: %s\nUserName: %s\nUserID: %d\nOwnerName: %s\nRepoName: %s\nRepoID: %d",
+	log.Debug("Serv Results:\nIsWiki: %t\nDeployKeyID: %d\nKeyID: %d\tKeyName: %s\nUserName: %s\nUserID: %d\nOwnerName: %s\nRepoName: %s\nRepoID: %d",
 		results.IsWiki,
-		results.IsDeployKey,
+		results.DeployKeyID,
 		results.KeyID,
 		results.KeyName,
 		results.UserName,

routers/web/repo/http.go

@@ -222,7 +222,6 @@ func httpBase(ctx *context.Context) (h *serviceHandler) {
 			models.EnvRepoName + "=" + reponame,
 			models.EnvPusherName + "=" + ctx.Doer.Name,
 			models.EnvPusherID + fmt.Sprintf("=%d", ctx.Doer.ID),
-			models.EnvIsDeployKey + "=false",
 			models.EnvAppURL + "=" + setting.AppURL,
 		}