Skip to content
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.

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

Ensure deploy keys with write access can push (#19010) #19182

Merged
merged 2 commits into from
Mar 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions cmd/hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion cmd/serv.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
10 changes: 5 additions & 5 deletions integrations/api_private_serv_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down
8 changes: 1 addition & 7 deletions models/asymkey/ssh_key_deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions models/helper_environment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion modules/private/hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
6 changes: 3 additions & 3 deletions modules/private/serv.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/repo/key.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
96 changes: 59 additions & 37 deletions routers/private/hook_pre_receive.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand All @@ -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
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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{
Expand All @@ -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
}

Expand Down Expand Up @@ -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)
// loadPusherAndPermission returns false if an error occurs, and it writes the error response
func (ctx *preReceiveContext) loadPusherAndPermission() bool {
if ctx.loadedPusher {
return true
}

user, err := user_model.GetUserByID(ctx.opts.UserID)
if err != nil {
log.Error("Unable to get User id %d Error: %v", id, err)
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", id, err),
Err: fmt.Sprintf("Unable to get User id %d Error: %v", ctx.opts.UserID, err),
})
return
return false
}
ctx.user = user

perm, err = models.GetUserRepoPermission(ctx.Repo.Repository, 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
}
7 changes: 3 additions & 4 deletions routers/private/serv.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand Down
1 change: 0 additions & 1 deletion routers/web/repo/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,6 @@ func httpBase(ctx *context.Context) (h *serviceHandler) {
models.EnvRepoName + "=" + reponame,
models.EnvPusherName + "=" + ctx.User.Name,
models.EnvPusherID + fmt.Sprintf("=%d", ctx.User.ID),
models.EnvIsDeployKey + "=false",
models.EnvAppURL + "=" + setting.AppURL,
}

Expand Down