Small refactoring of modules/private #15947

lunny · 2021-05-22T12:29:29Z

KN4CK3R commented

2021-05-22 12:29:29 +00:00

(Migrated from github.com)

as title

lunny reviewed 2021-05-22 12:44:21 +00:00

routers/api/v1/repo/branch.go

lunny commented

2021-05-22 12:44:20 +00:00

If ctx.User == nil should not be removed.

KN4CK3R (Migrated from github.com) reviewed 2021-05-22 13:41:01 +00:00

routers/api/v1/repo/branch.go

KN4CK3R (Migrated from github.com) commented

2021-05-22 13:41:01 +00:00

Why? It's never called with nil. And even if it would be called the check is not logical there. It does not make a branch protected if the user is nil.

DeleteBranchPost redirects to the login page on anonymous acces.
The api DeleteBranch results in "user should have a permission to write to a repo".

Here it's covered by line 1450 and 1459:
77fa7146c6/routers/repo/issue.go (L1450-L1461)
Here it's covered by line 1160:
77fa7146c6/routers/repo/pull.go (L1155-L1193)

Why? It's never called with nil. And even if it would be called the check is not logical there. It does not make a branch protected if the user is nil. `DeleteBranchPost` redirects to the login page on anonymous acces. The api `DeleteBranch` results in "user should have a permission to write to a repo". Here it's covered by line 1450 and 1459: https://github.com/go-gitea/gitea/blob/77fa7146c6c7f9b1efdc06c24ef7d1f278871e13/routers/repo/issue.go#L1450-L1461 Here it's covered by line 1160: https://github.com/go-gitea/gitea/blob/77fa7146c6c7f9b1efdc06c24ef7d1f278871e13/routers/repo/pull.go#L1155-L1193

zeripath approved these changes 2021-05-22 14:41:21 +00:00

routers/api/v1/repo/branch.go

zeripath commented

2021-05-22 14:36:39 +00:00

It's also enforced by reqRepoWriter(models.UnitTypeCode) at:

77fa7146c6/routers/api/v1/api.go (L757)

It's also enforced by `reqRepoWriter(models.UnitTypeCode)` at: https://github.com/go-gitea/gitea/blob/77fa7146c6c7f9b1efdc06c24ef7d1f278871e13/routers/api/v1/api.go#L757

lunny reviewed 2021-06-10 13:16:55 +00:00

routers/private/serv.go

						
				@ -295,10 +281,9 @@ func ServCommand(ctx *context.PrivateContext) {

					if repoExist && (mode > models.AccessModeRead || repo.IsPrivate || setting.Service.RequireSignInView) {

						if key.Type == models.KeyTypeDeploy {

							if deployKey.Mode < mode {

lunny commented

2021-06-10 13:16:55 +00:00

Maybe return 400 or 405

lunny reviewed 2021-06-10 13:17:21 +00:00

routers/private/serv.go

lunny commented

2021-06-10 13:17:21 +00:00

Notfound?

lunny reviewed 2021-06-10 13:17:38 +00:00

routers/private/serv.go

lunny commented

2021-06-10 13:17:38 +00:00

As above.

lunny reviewed 2021-06-11 05:44:58 +00:00

services/repository/branch.go

						
				@ -28,3 +28,3 @@

					isProtected, err := repo.IsProtectedBranch(branchName, doer)

					isProtected, err := repo.IsProtectedBranch(branchName)

					if err != nil {

lunny commented

2021-06-11 05:44:57 +00:00

Why doer removed?

Why `doer` removed?

KN4CK3R (Migrated from github.com) reviewed 2021-06-11 06:30:03 +00:00

services/repository/branch.go

						
				@ -28,3 +28,3 @@

					isProtected, err := repo.IsProtectedBranch(branchName, doer)

					isProtected, err := repo.IsProtectedBranch(branchName)

					if err != nil {

KN4CK3R (Migrated from github.com) commented

2021-06-11 06:30:03 +00:00

see https://github.com/go-gitea/gitea/pull/15947#pullrequestreview-666179378

KN4CK3R (Migrated from github.com) reviewed 2021-06-11 06:58:47 +00:00

routers/private/serv.go

						
				@ -295,10 +281,9 @@ func ServCommand(ctx *context.PrivateContext) {

					if repoExist && (mode > models.AccessModeRead || repo.IsPrivate || setting.Service.RequireSignInView) {

						if key.Type == models.KeyTypeDeploy {

							if deployKey.Mode < mode {

KN4CK3R (Migrated from github.com) commented

2021-06-11 06:58:46 +00:00

In the end all different error codes result in Unauthorized:
daa5a23548/cmd/serv.go (L214-L218)

In the end all different error codes result in `Unauthorized`: https://github.com/go-gitea/gitea/blob/daa5a2354879a6dff93f431bc7e47670438995ac/cmd/serv.go#L214-L218