Skip to content

Commit

Permalink
Small refactoring of modules/private (#15947)
Browse files Browse the repository at this point in the history 
* Use correct variable name.

* doer is never nil here.

* Use status code constants.

* Replaced generic map with concrete struct.

* Fixed windows lint.

* Removed unused method.

* Changed error codes.

Co-authored-by: techknowlogick <[email protected]>
  • Loading branch information
@KN4CK3R @techknowlogick
KN4CK3R and techknowlogick authored Jun 23, 2021
1 parent 5930d09 commit 383ffcf
Show file tree
Hide file tree
Showing 13 changed files with 185 additions and 231 deletions.
2 changes: 1 addition & 1 deletion cmd/hook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ Gitea or set your environment appropriately.`, "")
GitObjectDirectory: os.Getenv(private.GitObjectDirectory),
GitQuarantinePath: os.Getenv(private.GitQuarantinePath),
GitPushOptions: pushOptions(),
ProtectedBranchID: prID,
PullRequestID: prID,
IsDeployKey: isDeployKey,
}

Expand Down
27 changes: 1 addition & 26 deletions models/branches.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -362,11 +362,7 @@ func (repo *Repository) GetBranchProtection(branchName string) (*ProtectedBranch
}

// IsProtectedBranch checks if branch is protected
func (repo *Repository) IsProtectedBranch(branchName string, doer *User) (bool, error) {
if doer == nil {
return true, nil
}

func (repo *Repository) IsProtectedBranch(branchName string) (bool, error) {
protectedBranch := &ProtectedBranch{
RepoID: repo.ID,
BranchName: branchName,
Expand All @@ -379,27 +375,6 @@ func (repo *Repository) IsProtectedBranch(branchName string, doer *User) (bool,
return has, nil
}

// IsProtectedBranchForPush checks if branch is protected for push
func (repo *Repository) IsProtectedBranchForPush(branchName string, doer *User) (bool, error) {
if doer == nil {
return true, nil
}

protectedBranch := &ProtectedBranch{
RepoID: repo.ID,
BranchName: branchName,
}

has, err := x.Get(protectedBranch)
if err != nil {
return true, err
} else if has {
return !protectedBranch.CanUserPush(doer.ID), nil
}

return false, nil
}

// updateApprovalWhitelist checks whether the user whitelist changed and returns a whitelist with
// the users from newWhitelist which have explicit read or write access to the repo.
func updateApprovalWhitelist(repo *Repository, currentWhitelist, newWhitelist []int64) (whitelist []int64, err error) {
Expand Down
2 changes: 1 addition & 1 deletion modules/private/hook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ type HookOptions struct {
GitAlternativeObjectDirectories string
GitQuarantinePath string
GitPushOptions GitPushOptions
ProtectedBranchID int64
PullRequestID int64
IsDeployKey bool
}

Expand Down
1 change: 0 additions & 1 deletion modules/private/serv.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ type ServCommandResults struct {
// ErrServCommand is an error returned from ServCommmand.
type ErrServCommand struct {
Results ServCommandResults
Type string
Err string
StatusCode int
}
Expand Down
104 changes: 52 additions & 52 deletions routers/private/hook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,17 +124,17 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
repo, err := models.GetRepositoryByOwnerAndName(ownerName, repoName)
if err != nil {
log.Error("Unable to get repository: %s/%s Error: %v", ownerName, repoName, err)
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"err": err.Error(),
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: err.Error(),
})
return
}
repo.OwnerName = ownerName
gitRepo, err := git.OpenRepository(repo.RepoPath())
if err != nil {
log.Error("Unable to get git repository for: %s/%s Error: %v", ownerName, repoName, err)
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"err": err.Error(),
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: err.Error(),
})
return
}
Expand Down Expand Up @@ -164,17 +164,17 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
branchName := strings.TrimPrefix(refFullName, git.BranchPrefix)
if branchName == repo.DefaultBranch && newCommitID == git.EmptySHA {
log.Warn("Forbidden: Branch: %s is the default branch in %-v and cannot be deleted", branchName, repo)
ctx.JSON(http.StatusForbidden, map[string]interface{}{
"err": fmt.Sprintf("branch %s is the default branch and cannot be deleted", branchName),
ctx.JSON(http.StatusForbidden, private.Response{
Err: fmt.Sprintf("branch %s is the default branch and cannot be deleted", branchName),
})
return
}

protectBranch, err := models.GetProtectedBranchBy(repo.ID, branchName)
if err != nil {
log.Error("Unable to get protected branch: %s in %-v Error: %v", branchName, repo, err)
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"err": err.Error(),
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: err.Error(),
})
return
}
Expand All @@ -191,8 +191,8 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
// 1. Detect and prevent deletion of the branch
if newCommitID == git.EmptySHA {
log.Warn("Forbidden: Branch: %s in %-v is protected from deletion", branchName, repo)
ctx.JSON(http.StatusForbidden, map[string]interface{}{
"err": fmt.Sprintf("branch %s is protected from deletion", branchName),
ctx.JSON(http.StatusForbidden, private.Response{
Err: fmt.Sprintf("branch %s is protected from deletion", branchName),
})
return
}
Expand All @@ -202,14 +202,14 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
output, err := git.NewCommand("rev-list", "--max-count=1", oldCommitID, "^"+newCommitID).RunInDirWithEnv(repo.RepoPath(), env)
if err != nil {
log.Error("Unable to detect force push between: %s and %s in %-v Error: %v", oldCommitID, newCommitID, repo, err)
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"err": fmt.Sprintf("Fail to detect force push: %v", err),
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: fmt.Sprintf("Fail to detect force push: %v", err),
})
return
} else if len(output) > 0 {
log.Warn("Forbidden: Branch: %s in %-v is protected from force push", branchName, repo)
ctx.JSON(http.StatusForbidden, map[string]interface{}{
"err": fmt.Sprintf("branch %s is protected from force push", branchName),
ctx.JSON(http.StatusForbidden, private.Response{
Err: fmt.Sprintf("branch %s is protected from force push", branchName),
})
return

Expand All @@ -222,15 +222,15 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
if err != nil {
if !isErrUnverifiedCommit(err) {
log.Error("Unable to check commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err)
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"err": fmt.Sprintf("Unable to check commits from %s to %s: %v", oldCommitID, newCommitID, err),
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: fmt.Sprintf("Unable to check commits from %s to %s: %v", oldCommitID, newCommitID, err),
})
return
}
unverifiedCommit := err.(*errUnverifiedCommit).sha
log.Warn("Forbidden: Branch: %s in %-v is protected from unverified commit %s", branchName, repo, unverifiedCommit)
ctx.JSON(http.StatusForbidden, map[string]interface{}{
"err": fmt.Sprintf("branch %s is protected from unverified commit %s", branchName, unverifiedCommit),
ctx.JSON(http.StatusForbidden, private.Response{
Err: fmt.Sprintf("branch %s is protected from unverified commit %s", branchName, unverifiedCommit),
})
return
}
Expand All @@ -248,8 +248,8 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
if err != nil {
if !models.IsErrFilePathProtected(err) {
log.Error("Unable to check file protection for commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err)
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"err": fmt.Sprintf("Unable to check file protection for commits from %s to %s: %v", oldCommitID, newCommitID, err),
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: fmt.Sprintf("Unable to check file protection for commits from %s to %s: %v", oldCommitID, newCommitID, err),
})
return
}
Expand All @@ -270,49 +270,49 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
// 6. If we're not allowed to push directly
if !canPush {
// Is this is a merge from the UI/API?
if opts.ProtectedBranchID == 0 {
if opts.PullRequestID == 0 {
// 6a. If we're not merging from the UI/API then there are two ways we got here:
//
// We are changing a protected file and we're not allowed to do that
if changedProtectedfiles {
log.Warn("Forbidden: Branch: %s in %-v is protected from changing file %s", branchName, repo, protectedFilePath)
ctx.JSON(http.StatusForbidden, map[string]interface{}{
"err": fmt.Sprintf("branch %s is protected from changing file %s", branchName, protectedFilePath),
ctx.JSON(http.StatusForbidden, private.Response{
Err: fmt.Sprintf("branch %s is protected from changing file %s", branchName, protectedFilePath),
})
return
}

// Or we're simply not able to push to this protected branch
log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v", opts.UserID, branchName, repo)
ctx.JSON(http.StatusForbidden, map[string]interface{}{
"err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName),
ctx.JSON(http.StatusForbidden, private.Response{
Err: fmt.Sprintf("Not allowed to push to protected branch %s", branchName),
})
return
}
// 6b. Merge (from UI or API)

// Get the PR, user and permissions for the user in the repository
pr, err := models.GetPullRequestByID(opts.ProtectedBranchID)
pr, err := models.GetPullRequestByID(opts.PullRequestID)
if err != nil {
log.Error("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err)
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"err": fmt.Sprintf("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err),
log.Error("Unable to get PullRequest %d Error: %v", opts.PullRequestID, err)
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: fmt.Sprintf("Unable to get PullRequest %d Error: %v", opts.PullRequestID, err),
})
return
}
user, err := models.GetUserByID(opts.UserID)
if err != nil {
log.Error("Unable to get User id %d Error: %v", opts.UserID, err)
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"err": fmt.Sprintf("Unable to get User id %d Error: %v", opts.UserID, err),
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: fmt.Sprintf("Unable to get User id %d Error: %v", opts.UserID, err),
})
return
}
perm, err := models.GetUserRepoPermission(repo, user)
if err != nil {
log.Error("Unable to get Repo permission of repo %s/%s of User %s", repo.OwnerName, repo.Name, user.Name, err)
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"err": fmt.Sprintf("Unable to get Repo permission of repo %s/%s of User %s: %v", repo.OwnerName, repo.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", repo.OwnerName, repo.Name, user.Name, err),
})
return
}
Expand All @@ -321,16 +321,16 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, perm, user)
if err != nil {
log.Error("Error calculating if allowed to merge: %v", err)
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"err": fmt.Sprintf("Error calculating if allowed to merge: %v", err),
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: fmt.Sprintf("Error calculating if allowed to merge: %v", err),
})
return
}

if !allowedMerge {
log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v and is not allowed to merge pr #%d", opts.UserID, branchName, repo, pr.Index)
ctx.JSON(http.StatusForbidden, map[string]interface{}{
"err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName),
ctx.JSON(http.StatusForbidden, private.Response{
Err: fmt.Sprintf("Not allowed to push to protected branch %s", branchName),
})
return
}
Expand All @@ -343,8 +343,8 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
// Now if we're not an admin - we can't overwrite protected files so fail now
if changedProtectedfiles {
log.Warn("Forbidden: Branch: %s in %-v is protected from changing file %s", branchName, repo, protectedFilePath)
ctx.JSON(http.StatusForbidden, map[string]interface{}{
"err": fmt.Sprintf("branch %s is protected from changing file %s", branchName, protectedFilePath),
ctx.JSON(http.StatusForbidden, private.Response{
Err: fmt.Sprintf("branch %s is protected from changing file %s", branchName, protectedFilePath),
})
return
}
Expand All @@ -353,14 +353,14 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
if err := pull_service.CheckPRReadyToMerge(pr, true); err != nil {
if models.IsErrNotAllowedToMerge(err) {
log.Warn("Forbidden: User %d is not allowed push to protected branch %s in %-v and pr #%d is not ready to be merged: %s", opts.UserID, branchName, repo, pr.Index, err.Error())
ctx.JSON(http.StatusForbidden, map[string]interface{}{
"err": fmt.Sprintf("Not allowed to push to protected branch %s and pr #%d is not ready to be merged: %s", branchName, opts.ProtectedBranchID, err.Error()),
ctx.JSON(http.StatusForbidden, private.Response{
Err: fmt.Sprintf("Not allowed to push to protected branch %s and pr #%d is not ready to be merged: %s", branchName, opts.PullRequestID, err.Error()),
})
return
}
log.Error("Unable to check if mergable: protected branch %s in %-v and pr #%d. Error: %v", opts.UserID, branchName, repo, pr.Index, err)
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"err": fmt.Sprintf("Unable to get status of pull request %d. Error: %v", opts.ProtectedBranchID, err),
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: fmt.Sprintf("Unable to get status of pull request %d. Error: %v", opts.PullRequestID, err),
})
return
}
Expand Down Expand Up @@ -549,8 +549,8 @@ func SetDefaultBranch(ctx *gitea_context.PrivateContext) {
repo, err := models.GetRepositoryByOwnerAndName(ownerName, repoName)
if err != nil {
log.Error("Failed to get repository: %s/%s Error: %v", ownerName, repoName, err)
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"Err": fmt.Sprintf("Failed to get repository: %s/%s Error: %v", ownerName, repoName, err),
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: fmt.Sprintf("Failed to get repository: %s/%s Error: %v", ownerName, repoName, err),
})
return
}
Expand All @@ -561,27 +561,27 @@ func SetDefaultBranch(ctx *gitea_context.PrivateContext) {
repo.DefaultBranch = branch
gitRepo, err := git.OpenRepository(repo.RepoPath())
if err != nil {
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"Err": fmt.Sprintf("Failed to get git repository: %s/%s Error: %v", ownerName, repoName, err),
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: fmt.Sprintf("Failed to get git repository: %s/%s Error: %v", ownerName, repoName, err),
})
return
}
if err := gitRepo.SetDefaultBranch(repo.DefaultBranch); err != nil {
if !git.IsErrUnsupportedVersion(err) {
gitRepo.Close()
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"Err": fmt.Sprintf("Unable to set default branch on repository: %s/%s Error: %v", ownerName, repoName, err),
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: fmt.Sprintf("Unable to set default branch on repository: %s/%s Error: %v", ownerName, repoName, err),
})
return
}
}
gitRepo.Close()

if err := repo.UpdateDefaultBranch(); err != nil {
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"Err": fmt.Sprintf("Unable to set default branch on repository: %s/%s Error: %v", ownerName, repoName, err),
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: fmt.Sprintf("Unable to set default branch on repository: %s/%s Error: %v", ownerName, repoName, err),
})
return
}
ctx.PlainText(200, []byte("success"))
ctx.PlainText(http.StatusOK, []byte("success"))
}
19 changes: 10 additions & 9 deletions routers/private/key.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

"code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/context"
"code.gitea.io/gitea/modules/private"
"code.gitea.io/gitea/modules/timeutil"
)

Expand All @@ -18,27 +19,27 @@ func UpdatePublicKeyInRepo(ctx *context.PrivateContext) {
keyID := ctx.ParamsInt64(":id")
repoID := ctx.ParamsInt64(":repoid")
if err := models.UpdatePublicKeyUpdated(keyID); err != nil {
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"err": err.Error(),
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: err.Error(),
})
return
}

deployKey, err := models.GetDeployKeyByRepo(keyID, repoID)
if err != nil {
if models.IsErrDeployKeyNotExist(err) {
ctx.PlainText(200, []byte("success"))
ctx.PlainText(http.StatusOK, []byte("success"))
return
}
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"err": err.Error(),
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: err.Error(),
})
return
}
deployKey.UpdatedUnix = timeutil.TimeStampNow()
if err = models.UpdateDeployKeyCols(deployKey, "updated_unix"); err != nil {
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"err": err.Error(),
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: err.Error(),
})
return
}
Expand All @@ -53,8 +54,8 @@ func AuthorizedPublicKeyByContent(ctx *context.PrivateContext) {

publicKey, err := models.SearchPublicKeyByContent(content)
if err != nil {
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"err": err.Error(),
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: err.Error(),
})
return
}
Expand Down
Loading

0 comments on commit 383ffcf

Please sign in to comment.