Skip to content

Commit 4fb1580

Browse files
wxiaoguangwxiaoguang
committed
add panic checks, add comments
1 parent 424ee22 commit 4fb1580

File tree

5 files changed

+32
-18
lines changed

5 files changed

+32
-18
lines changed

‎models/actions/runner.go‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"code.gitea.io/gitea/models/shared/types"
1515
user_model "code.gitea.io/gitea/models/user"
1616
"code.gitea.io/gitea/modules/optional"
17+
"code.gitea.io/gitea/modules/setting"
1718
"code.gitea.io/gitea/modules/timeutil"
1819
"code.gitea.io/gitea/modules/translation"
1920
"code.gitea.io/gitea/modules/util"
@@ -123,12 +124,15 @@ func (r *ActionRunner) IsOnline() bool {
123124
return false
124125
}
125126

126-
// Editable checks if the runner is editable by the user
127-
// ownerID == 0 and repoID == 0 means any runner including global runners
128-
// ownerID == 0 and repoID != 0 means any runner for the given repo
129-
// ownerID != 0 and repoID == 0 means any runner for the given user/org
130-
// ownerID != 0 and repoID != 0 means any runner for the given user/org or repo
131-
func (r *ActionRunner) Editable(ownerID, repoID int64) bool {
127+
// EditableInContext checks if the runner is editable by the "context" owner/repo
128+
// ownerID == 0 and repoID == 0 means "admin" context, any runner including global runners could be edited
129+
// ownerID == 0 and repoID != 0 means "repo" context, any runner belonging to the given repo could be edited
130+
// ownerID != 0 and repoID == 0 means "org" context, any runner belonging to the given user/org could be edited
131+
// ownerID != 0 and repoID != 0 means "owner" OR "repo" context, legacy behavior, but we should forbid using it
132+
func (r *ActionRunner) EditableInContext(ownerID, repoID int64) bool {
133+
if ownerID != 0 && repoID != 0 {
134+
setting.PanicInDevOrTesting("ownerID and repoID should not be both set")
135+
}
132136
if ownerID == 0 && repoID == 0 {
133137
return true
134138
}

‎routers/api/v1/shared/runners.go‎

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

1010
actions_model "code.gitea.io/gitea/models/actions"
1111
"code.gitea.io/gitea/models/db"
12+
"code.gitea.io/gitea/modules/setting"
1213
api "code.gitea.io/gitea/modules/structs"
1314
"code.gitea.io/gitea/modules/util"
1415
"code.gitea.io/gitea/routers/api/v1/utils"
@@ -35,13 +36,16 @@ func GetRegistrationToken(ctx *context.APIContext, ownerID, repoID int64) {
3536
ctx.JSON(http.StatusOK, RegistrationToken{Token: token.Token})
3637
}
3738

38-
// List Runners for api route validated ownerID and repoID
39+
// ListRunners lists runners for api route validated ownerID and repoID
3940
// ownerID == 0 and repoID == 0 means all runners including global runners, does not appear in sql where clause
4041
// ownerID == 0 and repoID != 0 means all runners for the given repo
4142
// ownerID != 0 and repoID == 0 means all runners for the given user/org
4243
// ownerID != 0 and repoID != 0 undefined behavior
4344
// Access rights are checked at the API route level
4445
func ListRunners(ctx *context.APIContext, ownerID, repoID int64) {
46+
if ownerID != 0 && repoID != 0 {
47+
setting.PanicInDevOrTesting("ownerID and repoID should not be both set")
48+
}
4549
runners, total, err := db.FindAndCount[actions_model.ActionRunner](ctx, &actions_model.FindRunnerOptions{
4650
OwnerID: ownerID,
4751
RepoID: repoID,
@@ -63,38 +67,44 @@ func ListRunners(ctx *context.APIContext, ownerID, repoID int64) {
6367
ctx.JSON(http.StatusOK, &res)
6468
}
6569

66-
// Get Runners for api route validated ownerID and repoID
70+
// GetRunner get the runner for api route validated ownerID and repoID
6771
// ownerID == 0 and repoID == 0 means any runner including global runners
6872
// ownerID == 0 and repoID != 0 means any runner for the given repo
6973
// ownerID != 0 and repoID == 0 means any runner for the given user/org
7074
// ownerID != 0 and repoID != 0 undefined behavior
7175
// Access rights are checked at the API route level
7276
func GetRunner(ctx *context.APIContext, ownerID, repoID, runnerID int64) {
77+
if ownerID != 0 && repoID != 0 {
78+
setting.PanicInDevOrTesting("ownerID and repoID should not be both set")
79+
}
7380
runner, err := actions_model.GetRunnerByID(ctx, runnerID)
7481
if err != nil {
7582
ctx.APIErrorNotFound(err)
7683
return
7784
}
78-
if !runner.Editable(ownerID, repoID) {
85+
if !runner.EditableInContext(ownerID, repoID) {
7986
ctx.APIErrorNotFound("No permission to get this runner")
8087
return
8188
}
8289
ctx.JSON(http.StatusOK, convert.ToActionRunner(ctx, runner))
8390
}
8491

85-
// Delete Runner for api route validated ownerID and repoID
92+
// DeleteRunner deletes the runner for api route validated ownerID and repoID
8693
// ownerID == 0 and repoID == 0 means any runner including global runners
8794
// ownerID == 0 and repoID != 0 means any runner for the given repo
8895
// ownerID != 0 and repoID == 0 means any runner for the given user/org
8996
// ownerID != 0 and repoID != 0 undefined behavior
9097
// Access rights are checked at the API route level
9198
func DeleteRunner(ctx *context.APIContext, ownerID, repoID, runnerID int64) {
99+
if ownerID != 0 && repoID != 0 {
100+
setting.PanicInDevOrTesting("ownerID and repoID should not be both set")
101+
}
92102
runner, err := actions_model.GetRunnerByID(ctx, runnerID)
93103
if err != nil {
94104
ctx.APIErrorInternal(err)
95105
return
96106
}
97-
if !runner.Editable(ownerID, repoID) {
107+
if !runner.EditableInContext(ownerID, repoID) {
98108
ctx.APIErrorNotFound("No permission to delete this runner")
99109
return
100110
}

‎routers/web/shared/actions/runners.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,7 @@ func RunnersEdit(ctx *context.Context) {
197197
ctx.ServerError("LoadAttributes", err)
198198
return
199199
}
200-
if !runner.Editable(ownerID, repoID) {
200+
if !runner.EditableInContext(ownerID, repoID) {
201201
err = errors.New("no permission to edit this runner")
202202
ctx.NotFound(err)
203203
return
@@ -250,7 +250,7 @@ func RunnersEditPost(ctx *context.Context) {
250250
ctx.ServerError("RunnerDetailsEditPost.GetRunnerByID", err)
251251
return
252252
}
253-
if !runner.Editable(ownerID, repoID) {
253+
if !runner.EditableInContext(ownerID, repoID) {
254254
ctx.NotFound(util.NewPermissionDeniedErrorf("no permission to edit this runner"))
255255
return
256256
}
@@ -304,7 +304,7 @@ func RunnerDeletePost(ctx *context.Context) {
304304
return
305305
}
306306

307-
if !runner.Editable(rCtx.OwnerID, rCtx.RepoID) {
307+
if !runner.EditableInContext(rCtx.OwnerID, rCtx.RepoID) {
308308
ctx.NotFound(util.NewPermissionDeniedErrorf("no permission to delete this runner"))
309309
return
310310
}

‎tests/integration/api_org_runner_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ func TestAPIRunnerGetAdminRunnerNotFoundOrgApi(t *testing.T) {
108108
userUsername := "user2"
109109
token := getUserToken(t, userUsername, auth_model.AccessTokenScopeReadOrganization)
110110
// Verify get a runner by id of different entity is not found
111-
// runner.Editable(ownerID, repoID) false
111+
// runner.EditableInContext(ownerID, repoID) false
112112
req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/orgs/org3/actions/runners/%d", 34349)).AddTokenAuth(token)
113113
MakeRequest(t, req, http.StatusNotFound)
114114
}
@@ -118,7 +118,7 @@ func TestAPIRunnerDeleteAdminRunnerNotFoundOrgApi(t *testing.T) {
118118
userUsername := "user2"
119119
token := getUserToken(t, userUsername, auth_model.AccessTokenScopeWriteOrganization)
120120
// Verify delete a runner by id of different entity is not found
121-
// runner.Editable(ownerID, repoID) false
121+
// runner.EditableInContext(ownerID, repoID) false
122122
req := NewRequest(t, "DELETE", fmt.Sprintf("/api/v1/orgs/org3/actions/runners/%d", 34349)).AddTokenAuth(token)
123123
MakeRequest(t, req, http.StatusNotFound)
124124
}

‎tests/integration/api_repo_runner_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ func TestAPIRunnerGetAdminRunnerNotFoundRepoApi(t *testing.T) {
108108
userUsername := "user2"
109109
token := getUserToken(t, userUsername, auth_model.AccessTokenScopeReadRepository)
110110
// Verify get a runner by id of different entity is not found
111-
// runner.Editable(ownerID, repoID) false
111+
// runner.EditableInContext(ownerID, repoID) false
112112
req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/user2/repo1/actions/runners/%d", 34349)).AddTokenAuth(token)
113113
MakeRequest(t, req, http.StatusNotFound)
114114
}
@@ -118,7 +118,7 @@ func TestAPIRunnerDeleteAdminRunnerNotFoundRepoApi(t *testing.T) {
118118
userUsername := "user2"
119119
token := getUserToken(t, userUsername, auth_model.AccessTokenScopeWriteRepository)
120120
// Verify delete a runner by id of different entity is not found
121-
// runner.Editable(ownerID, repoID) false
121+
// runner.EditableInContext(ownerID, repoID) false
122122
req := NewRequest(t, "DELETE", fmt.Sprintf("/api/v1/repos/user2/repo1/actions/runners/%d", 34349)).AddTokenAuth(token)
123123
MakeRequest(t, req, http.StatusNotFound)
124124
}

0 commit comments

Comments
 (0)