Skip to content

Commit 838c390

Browse files
KN4CK3Rtechknowlogick
authored andcommitted
Small refactoring of modules/private (go-gitea#15947)
* 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]>
1 parent cb51afa commit 838c390

File tree

13 files changed

+185
-231
lines changed

13 files changed

+185
-231
lines changed

cmd/hook.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ Gitea or set your environment appropriately.`, "")
179179
GitObjectDirectory: os.Getenv(private.GitObjectDirectory),
180180
GitQuarantinePath: os.Getenv(private.GitQuarantinePath),
181181
GitPushOptions: pushOptions(),
182-
ProtectedBranchID: prID,
182+
PullRequestID: prID,
183183
IsDeployKey: isDeployKey,
184184
}
185185

models/branches.go

Lines changed: 1 addition & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -362,11 +362,7 @@ func (repo *Repository) GetBranchProtection(branchName string) (*ProtectedBranch
362362
}
363363

364364
// IsProtectedBranch checks if branch is protected
365-
func (repo *Repository) IsProtectedBranch(branchName string, doer *User) (bool, error) {
366-
if doer == nil {
367-
return true, nil
368-
}
369-
365+
func (repo *Repository) IsProtectedBranch(branchName string) (bool, error) {
370366
protectedBranch := &ProtectedBranch{
371367
RepoID: repo.ID,
372368
BranchName: branchName,
@@ -379,27 +375,6 @@ func (repo *Repository) IsProtectedBranch(branchName string, doer *User) (bool,
379375
return has, nil
380376
}
381377

382-
// IsProtectedBranchForPush checks if branch is protected for push
383-
func (repo *Repository) IsProtectedBranchForPush(branchName string, doer *User) (bool, error) {
384-
if doer == nil {
385-
return true, nil
386-
}
387-
388-
protectedBranch := &ProtectedBranch{
389-
RepoID: repo.ID,
390-
BranchName: branchName,
391-
}
392-
393-
has, err := x.Get(protectedBranch)
394-
if err != nil {
395-
return true, err
396-
} else if has {
397-
return !protectedBranch.CanUserPush(doer.ID), nil
398-
}
399-
400-
return false, nil
401-
}
402-
403378
// updateApprovalWhitelist checks whether the user whitelist changed and returns a whitelist with
404379
// the users from newWhitelist which have explicit read or write access to the repo.
405380
func updateApprovalWhitelist(repo *Repository, currentWhitelist, newWhitelist []int64) (whitelist []int64, err error) {

modules/private/hook.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ type HookOptions struct {
5454
GitAlternativeObjectDirectories string
5555
GitQuarantinePath string
5656
GitPushOptions GitPushOptions
57-
ProtectedBranchID int64
57+
PullRequestID int64
5858
IsDeployKey bool
5959
}
6060

modules/private/serv.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,6 @@ type ServCommandResults struct {
5858
// ErrServCommand is an error returned from ServCommmand.
5959
type ErrServCommand struct {
6060
Results ServCommandResults
61-
Type string
6261
Err string
6362
StatusCode int
6463
}

routers/private/hook.go

Lines changed: 52 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -124,17 +124,17 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
124124
repo, err := models.GetRepositoryByOwnerAndName(ownerName, repoName)
125125
if err != nil {
126126
log.Error("Unable to get repository: %s/%s Error: %v", ownerName, repoName, err)
127-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
128-
"err": err.Error(),
127+
ctx.JSON(http.StatusInternalServerError, private.Response{
128+
Err: err.Error(),
129129
})
130130
return
131131
}
132132
repo.OwnerName = ownerName
133133
gitRepo, err := git.OpenRepository(repo.RepoPath())
134134
if err != nil {
135135
log.Error("Unable to get git repository for: %s/%s Error: %v", ownerName, repoName, err)
136-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
137-
"err": err.Error(),
136+
ctx.JSON(http.StatusInternalServerError, private.Response{
137+
Err: err.Error(),
138138
})
139139
return
140140
}
@@ -164,17 +164,17 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
164164
branchName := strings.TrimPrefix(refFullName, git.BranchPrefix)
165165
if branchName == repo.DefaultBranch && newCommitID == git.EmptySHA {
166166
log.Warn("Forbidden: Branch: %s is the default branch in %-v and cannot be deleted", branchName, repo)
167-
ctx.JSON(http.StatusForbidden, map[string]interface{}{
168-
"err": fmt.Sprintf("branch %s is the default branch and cannot be deleted", branchName),
167+
ctx.JSON(http.StatusForbidden, private.Response{
168+
Err: fmt.Sprintf("branch %s is the default branch and cannot be deleted", branchName),
169169
})
170170
return
171171
}
172172

173173
protectBranch, err := models.GetProtectedBranchBy(repo.ID, branchName)
174174
if err != nil {
175175
log.Error("Unable to get protected branch: %s in %-v Error: %v", branchName, repo, err)
176-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
177-
"err": err.Error(),
176+
ctx.JSON(http.StatusInternalServerError, private.Response{
177+
Err: err.Error(),
178178
})
179179
return
180180
}
@@ -191,8 +191,8 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
191191
// 1. Detect and prevent deletion of the branch
192192
if newCommitID == git.EmptySHA {
193193
log.Warn("Forbidden: Branch: %s in %-v is protected from deletion", branchName, repo)
194-
ctx.JSON(http.StatusForbidden, map[string]interface{}{
195-
"err": fmt.Sprintf("branch %s is protected from deletion", branchName),
194+
ctx.JSON(http.StatusForbidden, private.Response{
195+
Err: fmt.Sprintf("branch %s is protected from deletion", branchName),
196196
})
197197
return
198198
}
@@ -202,14 +202,14 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
202202
output, err := git.NewCommand("rev-list", "--max-count=1", oldCommitID, "^"+newCommitID).RunInDirWithEnv(repo.RepoPath(), env)
203203
if err != nil {
204204
log.Error("Unable to detect force push between: %s and %s in %-v Error: %v", oldCommitID, newCommitID, repo, err)
205-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
206-
"err": fmt.Sprintf("Fail to detect force push: %v", err),
205+
ctx.JSON(http.StatusInternalServerError, private.Response{
206+
Err: fmt.Sprintf("Fail to detect force push: %v", err),
207207
})
208208
return
209209
} else if len(output) > 0 {
210210
log.Warn("Forbidden: Branch: %s in %-v is protected from force push", branchName, repo)
211-
ctx.JSON(http.StatusForbidden, map[string]interface{}{
212-
"err": fmt.Sprintf("branch %s is protected from force push", branchName),
211+
ctx.JSON(http.StatusForbidden, private.Response{
212+
Err: fmt.Sprintf("branch %s is protected from force push", branchName),
213213
})
214214
return
215215

@@ -222,15 +222,15 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
222222
if err != nil {
223223
if !isErrUnverifiedCommit(err) {
224224
log.Error("Unable to check commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err)
225-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
226-
"err": fmt.Sprintf("Unable to check commits from %s to %s: %v", oldCommitID, newCommitID, err),
225+
ctx.JSON(http.StatusInternalServerError, private.Response{
226+
Err: fmt.Sprintf("Unable to check commits from %s to %s: %v", oldCommitID, newCommitID, err),
227227
})
228228
return
229229
}
230230
unverifiedCommit := err.(*errUnverifiedCommit).sha
231231
log.Warn("Forbidden: Branch: %s in %-v is protected from unverified commit %s", branchName, repo, unverifiedCommit)
232-
ctx.JSON(http.StatusForbidden, map[string]interface{}{
233-
"err": fmt.Sprintf("branch %s is protected from unverified commit %s", branchName, unverifiedCommit),
232+
ctx.JSON(http.StatusForbidden, private.Response{
233+
Err: fmt.Sprintf("branch %s is protected from unverified commit %s", branchName, unverifiedCommit),
234234
})
235235
return
236236
}
@@ -248,8 +248,8 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
248248
if err != nil {
249249
if !models.IsErrFilePathProtected(err) {
250250
log.Error("Unable to check file protection for commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err)
251-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
252-
"err": fmt.Sprintf("Unable to check file protection for commits from %s to %s: %v", oldCommitID, newCommitID, err),
251+
ctx.JSON(http.StatusInternalServerError, private.Response{
252+
Err: fmt.Sprintf("Unable to check file protection for commits from %s to %s: %v", oldCommitID, newCommitID, err),
253253
})
254254
return
255255
}
@@ -270,49 +270,49 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
270270
// 6. If we're not allowed to push directly
271271
if !canPush {
272272
// Is this is a merge from the UI/API?
273-
if opts.ProtectedBranchID == 0 {
273+
if opts.PullRequestID == 0 {
274274
// 6a. If we're not merging from the UI/API then there are two ways we got here:
275275
//
276276
// We are changing a protected file and we're not allowed to do that
277277
if changedProtectedfiles {
278278
log.Warn("Forbidden: Branch: %s in %-v is protected from changing file %s", branchName, repo, protectedFilePath)
279-
ctx.JSON(http.StatusForbidden, map[string]interface{}{
280-
"err": fmt.Sprintf("branch %s is protected from changing file %s", branchName, protectedFilePath),
279+
ctx.JSON(http.StatusForbidden, private.Response{
280+
Err: fmt.Sprintf("branch %s is protected from changing file %s", branchName, protectedFilePath),
281281
})
282282
return
283283
}
284284

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

294294
// Get the PR, user and permissions for the user in the repository
295-
pr, err := models.GetPullRequestByID(opts.ProtectedBranchID)
295+
pr, err := models.GetPullRequestByID(opts.PullRequestID)
296296
if err != nil {
297-
log.Error("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err)
298-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
299-
"err": fmt.Sprintf("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err),
297+
log.Error("Unable to get PullRequest %d Error: %v", opts.PullRequestID, err)
298+
ctx.JSON(http.StatusInternalServerError, private.Response{
299+
Err: fmt.Sprintf("Unable to get PullRequest %d Error: %v", opts.PullRequestID, err),
300300
})
301301
return
302302
}
303303
user, err := models.GetUserByID(opts.UserID)
304304
if err != nil {
305305
log.Error("Unable to get User id %d Error: %v", opts.UserID, err)
306-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
307-
"err": fmt.Sprintf("Unable to get User id %d Error: %v", opts.UserID, err),
306+
ctx.JSON(http.StatusInternalServerError, private.Response{
307+
Err: fmt.Sprintf("Unable to get User id %d Error: %v", opts.UserID, err),
308308
})
309309
return
310310
}
311311
perm, err := models.GetUserRepoPermission(repo, user)
312312
if err != nil {
313313
log.Error("Unable to get Repo permission of repo %s/%s of User %s", repo.OwnerName, repo.Name, user.Name, err)
314-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
315-
"err": fmt.Sprintf("Unable to get Repo permission of repo %s/%s of User %s: %v", repo.OwnerName, repo.Name, user.Name, err),
314+
ctx.JSON(http.StatusInternalServerError, private.Response{
315+
Err: fmt.Sprintf("Unable to get Repo permission of repo %s/%s of User %s: %v", repo.OwnerName, repo.Name, user.Name, err),
316316
})
317317
return
318318
}
@@ -321,16 +321,16 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
321321
allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, perm, user)
322322
if err != nil {
323323
log.Error("Error calculating if allowed to merge: %v", err)
324-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
325-
"err": fmt.Sprintf("Error calculating if allowed to merge: %v", err),
324+
ctx.JSON(http.StatusInternalServerError, private.Response{
325+
Err: fmt.Sprintf("Error calculating if allowed to merge: %v", err),
326326
})
327327
return
328328
}
329329

330330
if !allowedMerge {
331331
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)
332-
ctx.JSON(http.StatusForbidden, map[string]interface{}{
333-
"err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName),
332+
ctx.JSON(http.StatusForbidden, private.Response{
333+
Err: fmt.Sprintf("Not allowed to push to protected branch %s", branchName),
334334
})
335335
return
336336
}
@@ -343,8 +343,8 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
343343
// Now if we're not an admin - we can't overwrite protected files so fail now
344344
if changedProtectedfiles {
345345
log.Warn("Forbidden: Branch: %s in %-v is protected from changing file %s", branchName, repo, protectedFilePath)
346-
ctx.JSON(http.StatusForbidden, map[string]interface{}{
347-
"err": fmt.Sprintf("branch %s is protected from changing file %s", branchName, protectedFilePath),
346+
ctx.JSON(http.StatusForbidden, private.Response{
347+
Err: fmt.Sprintf("branch %s is protected from changing file %s", branchName, protectedFilePath),
348348
})
349349
return
350350
}
@@ -353,14 +353,14 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) {
353353
if err := pull_service.CheckPRReadyToMerge(pr, true); err != nil {
354354
if models.IsErrNotAllowedToMerge(err) {
355355
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())
356-
ctx.JSON(http.StatusForbidden, map[string]interface{}{
357-
"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()),
356+
ctx.JSON(http.StatusForbidden, private.Response{
357+
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()),
358358
})
359359
return
360360
}
361361
log.Error("Unable to check if mergable: protected branch %s in %-v and pr #%d. Error: %v", opts.UserID, branchName, repo, pr.Index, err)
362-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
363-
"err": fmt.Sprintf("Unable to get status of pull request %d. Error: %v", opts.ProtectedBranchID, err),
362+
ctx.JSON(http.StatusInternalServerError, private.Response{
363+
Err: fmt.Sprintf("Unable to get status of pull request %d. Error: %v", opts.PullRequestID, err),
364364
})
365365
return
366366
}
@@ -549,8 +549,8 @@ func SetDefaultBranch(ctx *gitea_context.PrivateContext) {
549549
repo, err := models.GetRepositoryByOwnerAndName(ownerName, repoName)
550550
if err != nil {
551551
log.Error("Failed to get repository: %s/%s Error: %v", ownerName, repoName, err)
552-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
553-
"Err": fmt.Sprintf("Failed to get repository: %s/%s Error: %v", ownerName, repoName, err),
552+
ctx.JSON(http.StatusInternalServerError, private.Response{
553+
Err: fmt.Sprintf("Failed to get repository: %s/%s Error: %v", ownerName, repoName, err),
554554
})
555555
return
556556
}
@@ -561,27 +561,27 @@ func SetDefaultBranch(ctx *gitea_context.PrivateContext) {
561561
repo.DefaultBranch = branch
562562
gitRepo, err := git.OpenRepository(repo.RepoPath())
563563
if err != nil {
564-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
565-
"Err": fmt.Sprintf("Failed to get git repository: %s/%s Error: %v", ownerName, repoName, err),
564+
ctx.JSON(http.StatusInternalServerError, private.Response{
565+
Err: fmt.Sprintf("Failed to get git repository: %s/%s Error: %v", ownerName, repoName, err),
566566
})
567567
return
568568
}
569569
if err := gitRepo.SetDefaultBranch(repo.DefaultBranch); err != nil {
570570
if !git.IsErrUnsupportedVersion(err) {
571571
gitRepo.Close()
572-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
573-
"Err": fmt.Sprintf("Unable to set default branch on repository: %s/%s Error: %v", ownerName, repoName, err),
572+
ctx.JSON(http.StatusInternalServerError, private.Response{
573+
Err: fmt.Sprintf("Unable to set default branch on repository: %s/%s Error: %v", ownerName, repoName, err),
574574
})
575575
return
576576
}
577577
}
578578
gitRepo.Close()
579579

580580
if err := repo.UpdateDefaultBranch(); err != nil {
581-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
582-
"Err": fmt.Sprintf("Unable to set default branch on repository: %s/%s Error: %v", ownerName, repoName, err),
581+
ctx.JSON(http.StatusInternalServerError, private.Response{
582+
Err: fmt.Sprintf("Unable to set default branch on repository: %s/%s Error: %v", ownerName, repoName, err),
583583
})
584584
return
585585
}
586-
ctx.PlainText(200, []byte("success"))
586+
ctx.PlainText(http.StatusOK, []byte("success"))
587587
}

routers/private/key.go

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010

1111
"code.gitea.io/gitea/models"
1212
"code.gitea.io/gitea/modules/context"
13+
"code.gitea.io/gitea/modules/private"
1314
"code.gitea.io/gitea/modules/timeutil"
1415
)
1516

@@ -18,27 +19,27 @@ func UpdatePublicKeyInRepo(ctx *context.PrivateContext) {
1819
keyID := ctx.ParamsInt64(":id")
1920
repoID := ctx.ParamsInt64(":repoid")
2021
if err := models.UpdatePublicKeyUpdated(keyID); err != nil {
21-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
22-
"err": err.Error(),
22+
ctx.JSON(http.StatusInternalServerError, private.Response{
23+
Err: err.Error(),
2324
})
2425
return
2526
}
2627

2728
deployKey, err := models.GetDeployKeyByRepo(keyID, repoID)
2829
if err != nil {
2930
if models.IsErrDeployKeyNotExist(err) {
30-
ctx.PlainText(200, []byte("success"))
31+
ctx.PlainText(http.StatusOK, []byte("success"))
3132
return
3233
}
33-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
34-
"err": err.Error(),
34+
ctx.JSON(http.StatusInternalServerError, private.Response{
35+
Err: err.Error(),
3536
})
3637
return
3738
}
3839
deployKey.UpdatedUnix = timeutil.TimeStampNow()
3940
if err = models.UpdateDeployKeyCols(deployKey, "updated_unix"); err != nil {
40-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
41-
"err": err.Error(),
41+
ctx.JSON(http.StatusInternalServerError, private.Response{
42+
Err: err.Error(),
4243
})
4344
return
4445
}
@@ -53,8 +54,8 @@ func AuthorizedPublicKeyByContent(ctx *context.PrivateContext) {
5354

5455
publicKey, err := models.SearchPublicKeyByContent(content)
5556
if err != nil {
56-
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
57-
"err": err.Error(),
57+
ctx.JSON(http.StatusInternalServerError, private.Response{
58+
Err: err.Error(),
5859
})
5960
return
6061
}

0 commit comments

Comments
 (0)