From 2b55422cd71b0b325f054646de5cebf39b72b502 Mon Sep 17 00:00:00 2001
From: wxiaoguang <wxiaoguang@gmail.com>
Date: Tue, 22 Mar 2022 17:29:07 +0800
Subject: [PATCH] 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 | 4 +-
cmd/serv.go | 2 +-
integrations/api_private_serv_test.go | 10 +--
models/asymkey/ssh_key_deploy.go | 8 +-
models/helper_environment.go | 4 +-
modules/private/hook.go | 2 +-
modules/private/serv.go | 6 +-
routers/api/v1/repo/key.go | 2 +-
routers/private/hook_pre_receive.go | 102 ++++++++++++++++----------
routers/private/serv.go | 7 +-
routers/web/repo/http.go | 1 -
11 files changed, 81 insertions(+), 67 deletions(-)
diff --git a/cmd/hook.go b/cmd/hook.go
index 1dd59e8192..05fa6e56c1 100644
--- a/cmd/hook.go
+++ b/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)
diff --git a/cmd/serv.go b/cmd/serv.go
index b4ef37f1dc..c834ca298a 100644
--- a/cmd/serv.go
+++ b/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)
diff --git a/integrations/api_private_serv_test.go b/integrations/api_private_serv_test.go
index a58d927cb9..fd3cb25ef2 100644
--- a/integrations/api_private_serv_test.go
+++ b/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)
diff --git a/models/asymkey/ssh_key_deploy.go b/models/asymkey/ssh_key_deploy.go
index fc6324792a..fe2ade43ae 100644
--- a/models/asymkey/ssh_key_deploy.go
+++ b/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
diff --git a/models/helper_environment.go b/models/helper_environment.go
index 57ec3ea1e9..4cad1e5368 100644
--- a/models/helper_environment.go
+++ b/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"
diff --git a/modules/private/hook.go b/modules/private/hook.go
index fd864b1e6b..559019344e 100644
--- a/modules/private/hook.go
+++ b/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
}
diff --git a/modules/private/serv.go b/modules/private/serv.go
index e1204c23a7..2e1367e4c4 100644
--- a/modules/private/serv.go
+++ b/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
diff --git a/routers/api/v1/repo/key.go b/routers/api/v1/repo/key.go
index 35efa64ad3..568f92d7fb 100644
--- a/routers/api/v1/repo/key.go
+++ b/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()
diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go
index 85464deb29..c6ea422287 100644
--- a/routers/private/hook_pre_receive.go
+++ b/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
}
diff --git a/routers/private/serv.go b/routers/private/serv.go
index 65989d868b..b0451df5d8 100644
--- a/routers/private/serv.go
+++ b/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,
diff --git a/routers/web/repo/http.go b/routers/web/repo/http.go
index 989e71a3cf..b706330d6d 100644
--- a/routers/web/repo/http.go
+++ b/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,
}