diff --git a/models/actions/TestRunner_FindRunnerOptionsToConds/action_runner.yml b/models/actions/TestRunner_FindRunnerOptionsToConds/action_runner.yml new file mode 100644 index 0000000000..fc5190107e --- /dev/null +++ b/models/actions/TestRunner_FindRunnerOptionsToConds/action_runner.yml @@ -0,0 +1,45 @@ +- id: 719931 + uuid: "9e0eb762-cbdf-4725-9c90-4298568a2a77" + name: "runner-1" + version: "dev" + owner_id: 3 # Owned by org3 + repo_id: 0 + description: "A superb runner" + agent_labels: ["debian", "gpu"] + deleted: 0 +- id: 719932 + uuid: "b0a0a168-1b08-4365-95dd-e65040ca384d" + name: "runner-2" + version: "11.3.1" + owner_id: 2 # Owned by user2 + repo_id: 0 + description: "An exclusive runner" + agent_labels: ["docker"] + deleted: 0 +- id: 719933 + uuid: "974f9caf-ee64-4022-a56b-67b28ea06a0d" + name: "runner-3" + version: "12.2.0" + owner_id: 0 + repo_id: 0 + description: "A runner for everyone" + agent_labels: ["docker"] + deleted: 0 +- id: 719934 + uuid: "022650a8-e999-4a3d-afb0-21f75233798b" + name: "runner-4" + version: "12.1.0" + owner_id: 0 + repo_id: 32 # owned by org3 + description: "" + agent_labels: ["debian"] + deleted: 0 +- id: 719935 + uuid: "f3031695-87ea-41cf-8d48-21948e83ee17" + name: "runner-5" + version: "12.7.0" + owner_id: 0 + repo_id: 36 # owned by user2 + description: "" + agent_labels: ["arch"] + deleted: 0 diff --git a/models/actions/TestRunner_GetAvailableRunnerByID/action_runner.yml b/models/actions/TestRunner_GetVisibleRunnerByID/action_runner.yml similarity index 100% rename from models/actions/TestRunner_GetAvailableRunnerByID/action_runner.yml rename to models/actions/TestRunner_GetVisibleRunnerByID/action_runner.yml diff --git a/models/actions/runner.go b/models/actions/runner.go index ea2e811f94..c524ed6060 100644 --- a/models/actions/runner.go +++ b/models/actions/runner.go @@ -196,12 +196,12 @@ func init() { type FindRunnerOptions struct { db.ListOptions - RepoID int64 - OwnerID int64 // it will be ignored if RepoID is set - Sort string - Filter string - IsOnline optional.Option[bool] - WithAvailable bool // not only runners belong to, but also runners can be used + RepoID int64 + OwnerID int64 // it will be ignored if RepoID is set + Sort string + Filter string + IsOnline optional.Option[bool] + WithVisible bool // include all runners that are visible to the repository, owner, or instance } func (opts FindRunnerOptions) ToConds() builder.Cond { @@ -209,17 +209,19 @@ func (opts FindRunnerOptions) ToConds() builder.Cond { if opts.RepoID > 0 { c := builder.NewCond().And(builder.Eq{"repo_id": opts.RepoID}) - if opts.WithAvailable { + if opts.WithVisible { c = c.Or(builder.Eq{"owner_id": builder.Select("owner_id").From("repository").Where(builder.Eq{"id": opts.RepoID})}) c = c.Or(builder.Eq{"repo_id": 0, "owner_id": 0}) } cond = cond.And(c) } else if opts.OwnerID > 0 { // OwnerID is ignored if RepoID is set c := builder.NewCond().And(builder.Eq{"owner_id": opts.OwnerID}) - if opts.WithAvailable { + if opts.WithVisible { c = c.Or(builder.Eq{"repo_id": 0, "owner_id": 0}) } cond = cond.And(c) + } else if !opts.WithVisible { + cond = cond.And(builder.Eq{"repo_id": 0, "owner_id": 0}) } if opts.Filter != "" { @@ -278,9 +280,9 @@ func GetRunnerByID(ctx context.Context, id int64) (*ActionRunner, error) { return &runner, nil } -// GetAvailableRunnerByID is like GetRunnerByID, but it only finds the runner if it is accessible to the given owner or +// GetVisibleRunnerByID is like GetRunnerByID, but it only finds the runner if it is visible to the given owner or // repository. If it is not, util.ErrNotExist will be returned even if the runner exists. -func GetAvailableRunnerByID(ctx context.Context, id, ownerID, repoID int64) (*ActionRunner, error) { +func GetVisibleRunnerByID(ctx context.Context, id, ownerID, repoID int64) (*ActionRunner, error) { query := db.GetEngine(ctx).Where("id=?", id) if repoID > 0 { diff --git a/models/actions/runner_test.go b/models/actions/runner_test.go index d279c776c6..bf5911ff47 100644 --- a/models/actions/runner_test.go +++ b/models/actions/runner_test.go @@ -237,8 +237,8 @@ func TestRunnerEditable(t *testing.T) { } } -func TestRunner_GetAvailableRunnerByID(t *testing.T) { - defer unittest.OverrideFixtures("models/actions/TestRunner_GetAvailableRunnerByID")() +func TestRunner_GetVisibleRunnerByID(t *testing.T) { + defer unittest.OverrideFixtures("models/actions/TestRunner_GetVisibleRunnerByID")() require.NoError(t, unittest.PrepareTestDatabase()) repository32 := unittest.AssertExistsAndLoadBean(t, &repo.Repository{ID: 32, OwnerID: 3}) @@ -365,7 +365,7 @@ func TestRunner_GetAvailableRunnerByID(t *testing.T) { for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { - _, err := GetAvailableRunnerByID(t.Context(), testCase.runner.ID, testCase.ownerID, testCase.repoID) + _, err := GetVisibleRunnerByID(t.Context(), testCase.runner.ID, testCase.ownerID, testCase.repoID) if testCase.expectedError == "" { require.NoError(t, err) } else { @@ -374,3 +374,96 @@ func TestRunner_GetAvailableRunnerByID(t *testing.T) { }) } } + +func TestRunner_FindRunnerOptionsToConds(t *testing.T) { + defer unittest.OverrideFixtures("models/actions/TestRunner_FindRunnerOptionsToConds")() + require.NoError(t, unittest.PrepareTestDatabase()) + + runner1 := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: 719931, OwnerID: 3, RepoID: 0}) // Owned by org3 + runner2 := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: 719932, OwnerID: 2, RepoID: 0}) // Owned by user2 + runner3 := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: 719933, OwnerID: 0, RepoID: 0}) + runner4 := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: 719934, OwnerID: 0, RepoID: 32}) + runner5 := unittest.AssertExistsAndLoadBean(t, &ActionRunner{ID: 719935, OwnerID: 0, RepoID: 36}) + + testCases := []struct { + name string + opts FindRunnerOptions + expectedRunners RunnerList + unexpectedRunners RunnerList + }{ + { + name: "Only runners owned by instance", + opts: FindRunnerOptions{OwnerID: 0, RepoID: 0, WithVisible: false}, + expectedRunners: RunnerList{runner3}, + unexpectedRunners: RunnerList{runner1, runner2, runner4, runner5}, + }, + { + name: "All runners on instance", + opts: FindRunnerOptions{OwnerID: 0, RepoID: 0, WithVisible: true}, + expectedRunners: RunnerList{runner1, runner2, runner3, runner4, runner5}, + unexpectedRunners: RunnerList{}, + }, + { + name: "Only runners owned by organization", + opts: FindRunnerOptions{OwnerID: 3, RepoID: 0, WithVisible: false}, + expectedRunners: RunnerList{runner1}, + unexpectedRunners: RunnerList{runner2, runner3, runner4, runner5}, + }, + { + name: "Runners available to organization", + opts: FindRunnerOptions{OwnerID: 3, RepoID: 0, WithVisible: true}, + expectedRunners: RunnerList{runner1, runner3}, + unexpectedRunners: RunnerList{runner2, runner4, runner5}, + }, + { + name: "Only runners owned by user", + opts: FindRunnerOptions{OwnerID: 2, RepoID: 0, WithVisible: false}, + expectedRunners: RunnerList{runner2}, + unexpectedRunners: RunnerList{runner1, runner3, runner4, runner5}, + }, + { + name: "Runners available to user", + opts: FindRunnerOptions{OwnerID: 2, RepoID: 0, WithVisible: true}, + expectedRunners: RunnerList{runner2, runner3}, + unexpectedRunners: RunnerList{runner1, runner4, runner5}, + }, + { + name: "Only runners owned by organization repository", + opts: FindRunnerOptions{OwnerID: 0, RepoID: 32, WithVisible: false}, + expectedRunners: RunnerList{runner4}, + unexpectedRunners: RunnerList{runner1, runner2, runner3, runner5}, + }, + { + name: "Runners available to organization repository", + opts: FindRunnerOptions{OwnerID: 0, RepoID: 32, WithVisible: true}, + expectedRunners: RunnerList{runner1, runner3, runner4}, + unexpectedRunners: RunnerList{runner2, runner5}, + }, + { + name: "Only runners owned by user repository", + opts: FindRunnerOptions{OwnerID: 0, RepoID: 36, WithVisible: false}, + expectedRunners: RunnerList{runner5}, + unexpectedRunners: RunnerList{runner1, runner2, runner3, runner4}, + }, + { + name: "Runners available to user repository", + opts: FindRunnerOptions{OwnerID: 0, RepoID: 36, WithVisible: true}, + expectedRunners: RunnerList{runner2, runner3, runner5}, + unexpectedRunners: RunnerList{runner1, runner4}, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + runners, err := db.Find[ActionRunner](t.Context(), testCase.opts) + require.NoError(t, err) + + for _, expectedRunner := range testCase.expectedRunners { + assert.Contains(t, runners, expectedRunner) + } + for _, unexpectedRunner := range testCase.unexpectedRunners { + assert.NotContains(t, runners, unexpectedRunner) + } + }) + } +} diff --git a/routers/api/v1/shared/runners.go b/routers/api/v1/shared/runners.go index 5fadbd92e0..a86c0ee904 100644 --- a/routers/api/v1/shared/runners.go +++ b/routers/api/v1/shared/runners.go @@ -106,6 +106,7 @@ func ListRunners(ctx *context.APIContext, ownerID, repoID int64) { OwnerID: ownerID, RepoID: repoID, ListOptions: listOptions, + WithVisible: ownerID == 0 && repoID == 0, }) if err != nil { ctx.Error(http.StatusInternalServerError, "FindCountRunners", map[string]string{}) diff --git a/routers/web/repo/actions/actions.go b/routers/web/repo/actions/actions.go index 63098b62b6..b70f06c2f4 100644 --- a/routers/web/repo/actions/actions.go +++ b/routers/web/repo/actions/actions.go @@ -88,9 +88,9 @@ func List(ctx *context.Context) { // Get all runner labels runners, err := db.Find[actions_model.ActionRunner](ctx, actions_model.FindRunnerOptions{ - RepoID: ctx.Repo.Repository.ID, - IsOnline: optional.Some(true), - WithAvailable: true, + RepoID: ctx.Repo.Repository.ID, + IsOnline: optional.Some(true), + WithVisible: true, }) if err != nil { ctx.ServerError("FindRunners", err) diff --git a/routers/web/repo/setting/runners.go b/routers/web/repo/setting/runners.go index ab3ec32c4a..70f8c99c6e 100644 --- a/routers/web/repo/setting/runners.go +++ b/routers/web/repo/setting/runners.go @@ -137,15 +137,14 @@ func Runners(ctx *context.Context) { Page: page, PageSize: 100, }, - Sort: ctx.Req.URL.Query().Get("sort"), - Filter: ctx.Req.URL.Query().Get("q"), + WithVisible: true, + Sort: ctx.Req.URL.Query().Get("sort"), + Filter: ctx.Req.URL.Query().Get("q"), } if rCtx.IsRepo { opts.RepoID = rCtx.RepoID - opts.WithAvailable = true } else if rCtx.IsOrg || rCtx.IsUser { opts.OwnerID = rCtx.OwnerID - opts.WithAvailable = true } ctx.Data["RunnersListLink"] = rCtx.RedirectLink diff --git a/routers/web/shared/actions/runners.go b/routers/web/shared/actions/runners.go index ab52faf9f5..436b103f35 100644 --- a/routers/web/shared/actions/runners.go +++ b/routers/web/shared/actions/runners.go @@ -77,12 +77,12 @@ func RunnersList(ctx *context.Context, template base.TplName, opts actions_model // RunnerDetails displays detail information about each runner. The page is purely informational and visible to everyone // who is allowed to use a runner. func RunnerDetails(ctx *context.Context, runnerID, ownerID, repoID int64, template base.TplName, page int) { - runner, err := actions_model.GetAvailableRunnerByID(ctx, runnerID, ownerID, repoID) + runner, err := actions_model.GetVisibleRunnerByID(ctx, runnerID, ownerID, repoID) if errors.Is(err, util.ErrNotExist) { - ctx.NotFound("GetAvailableRunnerByID", err) + ctx.NotFound("GetVisibleRunnerByID", err) return } else if err != nil { - ctx.ServerError("GetAvailableRunnerByID", err) + ctx.ServerError("GetVisibleRunnerByID", err) return } if err := runner.LoadAttributes(ctx); err != nil { @@ -166,12 +166,12 @@ func RunnerCreatePost(ctx *context.Context, ownerID, repoID int64, template, suc // RunnerEdit displays a form to modify the given runner. func RunnerEdit(ctx *context.Context, runnerID, ownerID, repoID int64, template base.TplName) { - runner, err := actions_model.GetAvailableRunnerByID(ctx, runnerID, ownerID, repoID) + runner, err := actions_model.GetVisibleRunnerByID(ctx, runnerID, ownerID, repoID) if errors.Is(err, util.ErrNotExist) { - ctx.NotFound("GetAvailableRunnerByID", err) + ctx.NotFound("GetVisibleRunnerByID", err) return } else if err != nil { - ctx.ServerError("GetAvailableRunnerByID", err) + ctx.ServerError("GetVisibleRunnerByID", err) return } if err := runner.LoadAttributes(ctx); err != nil { @@ -194,12 +194,12 @@ func RunnerEdit(ctx *context.Context, runnerID, ownerID, repoID int64, template // RunnerEditPost handles the form submitted by RunnerEdit. func RunnerEditPost(ctx *context.Context, runnerID, ownerID, repoID int64, template, successTemplate base.TplName, redirectTo string) { - runner, err := actions_model.GetAvailableRunnerByID(ctx, runnerID, ownerID, repoID) + runner, err := actions_model.GetVisibleRunnerByID(ctx, runnerID, ownerID, repoID) if errors.Is(err, util.ErrNotExist) { - ctx.NotFound("GetAvailableRunnerByID", err) + ctx.NotFound("GetVisibleRunnerByID", err) return } else if err != nil { - ctx.ServerError("GetAvailableRunnerByID", err) + ctx.ServerError("GetVisibleRunnerByID", err) return } if !runner.Editable(ownerID, repoID) {