From 59f68b94698eaa319667497e197add45525653bc Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Wed, 29 Oct 2025 18:03:17 +0100 Subject: [PATCH 01/16] chore(typo): s/Copyright 20124/Copyright 2024/ --- tests/integration/actions_commit_status_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/actions_commit_status_test.go b/tests/integration/actions_commit_status_test.go index 05de90e5ab..6965832ee1 100644 --- a/tests/integration/actions_commit_status_test.go +++ b/tests/integration/actions_commit_status_test.go @@ -1,4 +1,4 @@ -// Copyright 20124 The Forgejo Authors. All rights reserved. +// Copyright 2024 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package integration From 7fccc2676bd2821fa1ce59125e24144f30e9ddda Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Fri, 24 Oct 2025 16:58:02 +0200 Subject: [PATCH 02/16] chore(refactor): add fixture helper testActionsNotifierPullRequest All pull request related notifier tests use a similar pattern to create runs and jobs. Move them to a helper to keep it DRY and cut the size of the number of lines in the test file by 20%. --- services/actions/notifier_helper_test.go | 131 +++++++---------------- 1 file changed, 39 insertions(+), 92 deletions(-) diff --git a/services/actions/notifier_helper_test.go b/services/actions/notifier_helper_test.go index 4a99340445..5080585c7c 100644 --- a/services/actions/notifier_helper_test.go +++ b/services/actions/notifier_helper_test.go @@ -24,7 +24,7 @@ import ( "github.com/stretchr/testify/require" ) -func Test_SkipPullRequestEvent(t *testing.T) { +func TestActionsNotifier_SkipPullRequestEvent(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) repoID := int64(1) @@ -59,7 +59,7 @@ func Test_SkipPullRequestEvent(t *testing.T) { assert.True(t, SkipPullRequestEvent(db.DefaultContext, webhook_module.HookEventPullRequestSync, repoID, commitSHA)) } -func Test_IssueCommentOnForkPullRequestEvent(t *testing.T) { +func TestActionsNotifier_IssueCommentOnForkPullRequestEvent(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) @@ -103,39 +103,47 @@ func Test_IssueCommentOnForkPullRequestEvent(t *testing.T) { assert.False(t, runs[0].IsForkPullRequest) } -func Test_OpenForkPullRequestEvent(t *testing.T) { - require.NoError(t, unittest.PrepareTestDatabase()) +func testActionsNotifierPullRequest(t *testing.T, repo *repo_model.Repository, pr *issues_model.PullRequest, dw *actions_module.DetectedWorkflow, event webhook_module.HookEventType) { + t.Helper() - repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) - pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 3}) require.NoError(t, pr.LoadIssue(db.DefaultContext)) - require.True(t, pr.IsFromFork()) + testActionsNotifierPullRequestWithDoer(t, repo, pr, doer, dw, event) +} + +func testActionsNotifierPullRequestWithDoer(t *testing.T, repo *repo_model.Repository, pr *issues_model.PullRequest, doer *user_model.User, dw *actions_module.DetectedWorkflow, event webhook_module.HookEventType) { + t.Helper() commit := &git.Commit{ ID: git.MustIDFromString("0000000000000000000000000000000000000000"), CommitMessage: "test", } - detectedWorkflows := []*actions_module.DetectedWorkflow{ - { - TriggerEvent: &jobparser.Event{ - Name: "pull_request", - }, - }, + dw.EntryName = "test.yml" + dw.TriggerEvent = &jobparser.Event{ + Name: "pull_request", } + detectedWorkflows := []*actions_module.DetectedWorkflow{dw} input := ¬ifyInput{ Repo: repo, Doer: doer, - Event: webhook_module.HookEventPullRequest, + Event: event, PullRequest: pr, Payload: &api.PullRequestPayload{}, } - unittest.AssertSuccessfulDelete(t, &actions_model.ActionRun{RepoID: repo.ID}) - - err := handleWorkflows(db.DefaultContext, detectedWorkflows, commit, input, "") + err := handleWorkflows(db.DefaultContext, detectedWorkflows, commit, input, "refs/head/main") require.NoError(t, err) +} + +func TestActionsNotifier_OpenForkPullRequestEvent(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 3}) + require.True(t, pr.IsFromFork()) + + testActionsNotifierPullRequest(t, repo, pr, &actions_module.DetectedWorkflow{}, webhook_module.HookEventPullRequest) runs, err := db.Find[actions_model.ActionRun](db.DefaultContext, actions_model.FindRunOptions{ RepoID: repo.ID, @@ -147,36 +155,16 @@ func Test_OpenForkPullRequestEvent(t *testing.T) { assert.True(t, runs[0].IsForkPullRequest) } -func TestActionsNotifierConcurrencyGroup(t *testing.T) { +func TestActionsNotifier_ConcurrencyGroup(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) - doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 3}) - commit := &git.Commit{ - ID: git.MustIDFromString("0000000000000000000000000000000000000000"), - CommitMessage: "test", + dw := &actions_module.DetectedWorkflow{ + Content: []byte("{ on: pull_request, jobs: { j1: {} }}"), } - detectedWorkflows := []*actions_module.DetectedWorkflow{ - { - EntryName: "test.yml", - TriggerEvent: &jobparser.Event{ - Name: "pull_request", - }, - Content: []byte("{ on: pull_request, jobs: { j1: {} }}"), - }, - } - input := ¬ifyInput{ - Repo: repo, - Doer: doer, - Event: webhook_module.HookEventPullRequestSync, - PullRequest: pr, - Payload: &api.PullRequestPayload{}, - } - - err := handleWorkflows(db.DefaultContext, detectedWorkflows, commit, input, "refs/head/main") - require.NoError(t, err) + testActionsNotifierPullRequest(t, repo, pr, dw, webhook_module.HookEventPullRequestSync) runs, err := db.Find[actions_model.ActionRun](db.DefaultContext, actions_model.FindRunOptions{ RepoID: repo.ID, @@ -192,8 +180,7 @@ func TestActionsNotifierConcurrencyGroup(t *testing.T) { // Also... check if CancelPreviousWithConcurrencyGroup is invoked from handleWorkflows by firing off a second // workflow and checking that the first one gets cancelled: - err = handleWorkflows(db.DefaultContext, detectedWorkflows, commit, input, "refs/head/main") - require.NoError(t, err) + testActionsNotifierPullRequest(t, repo, pr, dw, webhook_module.HookEventPullRequestSync) runs, err = db.Find[actions_model.ActionRun](db.DefaultContext, actions_model.FindRunOptions{ RepoID: repo.ID, @@ -210,36 +197,16 @@ func TestActionsNotifierConcurrencyGroup(t *testing.T) { assert.Equal(t, actions_model.StatusCancelled, firstRun.Status) } -func TestActionsPreExecutionErrorInvalidJobs(t *testing.T) { +func TestActionsNotifier_PreExecutionErrorInvalidJobs(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) - doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 3}) - commit := &git.Commit{ - ID: git.MustIDFromString("0000000000000000000000000000000000000000"), - CommitMessage: "test", + dw := &actions_module.DetectedWorkflow{ + Content: []byte("{ on: pull_request, jobs: 'hello, I am the jobs!' }"), } - detectedWorkflows := []*actions_module.DetectedWorkflow{ - { - EntryName: "test.yml", - TriggerEvent: &jobparser.Event{ - Name: "pull_request", - }, - Content: []byte("{ on: pull_request, jobs: 'hello, I am the jobs!' }"), - }, - } - input := ¬ifyInput{ - Repo: repo, - Doer: doer, - Event: webhook_module.HookEventPullRequestSync, - PullRequest: pr, - Payload: &api.PullRequestPayload{}, - } - - err := handleWorkflows(db.DefaultContext, detectedWorkflows, commit, input, "refs/head/main") - require.NoError(t, err) + testActionsNotifierPullRequest(t, repo, pr, dw, webhook_module.HookEventPullRequestSync) runs, err := db.Find[actions_model.ActionRun](db.DefaultContext, actions_model.FindRunOptions{ RepoID: repo.ID, @@ -252,37 +219,17 @@ func TestActionsPreExecutionErrorInvalidJobs(t *testing.T) { assert.Contains(t, createdRun.PreExecutionError, "actions.workflow.job_parsing_error%!(EXTRA *fmt.wrapError=") } -func TestActionsPreExecutionEventDetectionError(t *testing.T) { +func TestActionsNotifier_PreExecutionEventDetectionError(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) - doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 3}) - commit := &git.Commit{ - ID: git.MustIDFromString("0000000000000000000000000000000000000000"), - CommitMessage: "test", + dw := &actions_module.DetectedWorkflow{ + Content: []byte("{ on: nothing, jobs: { j1: {} }}"), + EventDetectionError: errors.New("nothing is not a valid event"), } - detectedWorkflows := []*actions_module.DetectedWorkflow{ - { - EntryName: "test.yml", - TriggerEvent: &jobparser.Event{ - Name: "pull_request", - }, - Content: []byte("{ on: nothing, jobs: { j1: {} }}"), - EventDetectionError: errors.New("nothing is not a valid event"), - }, - } - input := ¬ifyInput{ - Repo: repo, - Doer: doer, - Event: webhook_module.HookEventPullRequestSync, - PullRequest: pr, - Payload: &api.PullRequestPayload{}, - } - - err := handleWorkflows(db.DefaultContext, detectedWorkflows, commit, input, "refs/head/main") - require.NoError(t, err) + testActionsNotifierPullRequest(t, repo, pr, dw, webhook_module.HookEventPullRequestSync) runs, err := db.Find[actions_model.ActionRun](db.DefaultContext, actions_model.FindRunOptions{ RepoID: repo.ID, From 0989a2495e5c3fc12ad61ae4c24ca8d3ea835392 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Tue, 28 Oct 2025 20:14:35 +0100 Subject: [PATCH 03/16] chore(refactor): move actions_service.jobParser to actions_module.jobParser --- {services => modules}/actions/job_parser.go | 2 +- {services => modules}/actions/job_parser_test.go | 2 +- services/actions/notifier_helper.go | 2 +- services/actions/schedule_tasks.go | 3 ++- services/actions/workflows.go | 2 +- 5 files changed, 6 insertions(+), 5 deletions(-) rename {services => modules}/actions/job_parser.go (93%) rename {services => modules}/actions/job_parser_test.go (98%) diff --git a/services/actions/job_parser.go b/modules/actions/job_parser.go similarity index 93% rename from services/actions/job_parser.go rename to modules/actions/job_parser.go index 8c880977d1..6360902b9a 100644 --- a/services/actions/job_parser.go +++ b/modules/actions/job_parser.go @@ -9,7 +9,7 @@ import ( "code.forgejo.org/forgejo/runner/v11/act/jobparser" ) -func jobParser(workflow []byte, options ...jobparser.ParseOption) ([]*jobparser.SingleWorkflow, error) { +func JobParser(workflow []byte, options ...jobparser.ParseOption) ([]*jobparser.SingleWorkflow, error) { singleWorkflows, err := jobparser.Parse(workflow, false, options...) if err != nil { return nil, err diff --git a/services/actions/job_parser_test.go b/modules/actions/job_parser_test.go similarity index 98% rename from services/actions/job_parser_test.go rename to modules/actions/job_parser_test.go index 9c1361d74e..82bce3da13 100644 --- a/services/actions/job_parser_test.go +++ b/modules/actions/job_parser_test.go @@ -200,7 +200,7 @@ jobs: }, } { t.Run(testCase.name, func(t *testing.T) { - sw, err := jobParser([]byte(testCase.workflow)) + sw, err := JobParser([]byte(testCase.workflow)) require.NoError(t, err) for i, sw := range sw { actual, err := sw.Marshal() diff --git a/services/actions/notifier_helper.go b/services/actions/notifier_helper.go index 2fc89e221d..e093643222 100644 --- a/services/actions/notifier_helper.go +++ b/services/actions/notifier_helper.go @@ -396,7 +396,7 @@ func handleWorkflows( Name: dwf.EntryName, }} } else { - jobs, err = jobParser(dwf.Content, jobparser.WithVars(vars)) + jobs, err = actions_module.JobParser(dwf.Content, jobparser.WithVars(vars)) if err != nil { log.Info("jobparser.Parse: invalid workflow, setting job status to failed: %v", err) tr := translation.NewLocale(input.Doer.Language) diff --git a/services/actions/schedule_tasks.go b/services/actions/schedule_tasks.go index e7dad2d948..dfaeaae59b 100644 --- a/services/actions/schedule_tasks.go +++ b/services/actions/schedule_tasks.go @@ -14,6 +14,7 @@ import ( "forgejo.org/models/db" repo_model "forgejo.org/models/repo" "forgejo.org/models/unit" + actions_module "forgejo.org/modules/actions" "forgejo.org/modules/log" "forgejo.org/modules/timeutil" webhook_module "forgejo.org/modules/webhook" @@ -168,7 +169,7 @@ func CreateScheduleTask(ctx context.Context, cron *actions_model.ActionSchedule) } // Parse the workflow specification from the cron schedule - workflows, err := jobParser(cron.Content, jobparser.WithVars(vars)) + workflows, err := actions_module.JobParser(cron.Content, jobparser.WithVars(vars)) if err != nil { return err } diff --git a/services/actions/workflows.go b/services/actions/workflows.go index 4c0a097452..1e4d92fa2f 100644 --- a/services/actions/workflows.go +++ b/services/actions/workflows.go @@ -155,7 +155,7 @@ func (entry *Workflow) Dispatch(ctx context.Context, inputGetter InputValueGette } } - jobs, err := jobParser(content, jobparser.WithVars(vars), jobparser.WithInputs(inputsAny)) + jobs, err := actions.JobParser(content, jobparser.WithVars(vars), jobparser.WithInputs(inputsAny)) if err != nil { return nil, nil, err } From 86e08f4e1bad11a8e0474ef07b94c5ce25610915 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Tue, 4 Nov 2025 11:37:16 +0100 Subject: [PATCH 04/16] chore(refactor): split actions notify function in three There is no functional change, code reorganization or variable names changes. Two distinct code blocks from the notify function are moved to the functions: - getGitRepoAndCommit - detectWorkflows The intent is to help with unit testing each of them individually. --- services/actions/notifier_helper.go | 70 ++++++++++++++++++----------- 1 file changed, 45 insertions(+), 25 deletions(-) diff --git a/services/actions/notifier_helper.go b/services/actions/notifier_helper.go index e093643222..6a9f7534b8 100644 --- a/services/actions/notifier_helper.go +++ b/services/actions/notifier_helper.go @@ -157,12 +157,43 @@ func notify(ctx context.Context, input *notifyInput) error { return nil } - gitRepo, err := gitrepo.OpenRepository(context.Background(), input.Repo) + gitRepo, commit, ref, err := getGitRepoAndCommit(ctx, input) if err != nil { - return fmt.Errorf("git.OpenRepository: %w", err) + return err + } else if gitRepo == nil && commit == nil { + return nil } defer gitRepo.Close() + if skipWorkflows(input, commit) { + return nil + } + + if SkipPullRequestEvent(ctx, input.Event, input.Repo.ID, commit.ID.String()) { + log.Trace("repo %s with commit %s skip event %v", input.Repo.RepoPath(), commit.ID, input.Event) + return nil + } + + detectedWorkflows, schedules, err := detectWorkflows(ctx, input, gitRepo, commit, shouldDetectSchedules) + if err != nil { + return err + } + + if shouldDetectSchedules { + if err := handleSchedules(ctx, schedules, commit, input, ref.String()); err != nil { + return err + } + } + + return handleWorkflows(ctx, detectedWorkflows, commit, input, ref.String()) +} + +func getGitRepoAndCommit(_ context.Context, input *notifyInput) (*git.Repository, *git.Commit, git.RefName, error) { + gitRepo, err := gitrepo.OpenRepository(context.Background(), input.Repo) + if err != nil { + return nil, nil, "", fmt.Errorf("git.OpenRepository: %w", err) + } + ref := input.Ref if ref.BranchName() != input.Repo.DefaultBranch && actions_module.IsDefaultBranchWorkflow(input.Event) { if ref != "" { @@ -178,24 +209,20 @@ func notify(ctx context.Context, input *notifyInput) error { commitID, err := gitRepo.GetRefCommitID(ref.String()) if err != nil { - return fmt.Errorf("gitRepo.GetRefCommitID: %w", err) + gitRepo.Close() + return nil, nil, "", fmt.Errorf("gitRepo.GetRefCommitID: %w", err) } // Get the commit object for the ref commit, err := gitRepo.GetCommit(commitID) if err != nil { - return fmt.Errorf("gitRepo.GetCommit: %w", err) - } - - if skipWorkflows(input, commit) { - return nil - } - - if SkipPullRequestEvent(ctx, input.Event, input.Repo.ID, commit.ID.String()) { - log.Trace("repo %s with commit %s skip event %v", input.Repo.RepoPath(), commit.ID, input.Event) - return nil + gitRepo.Close() + return nil, nil, "", fmt.Errorf("gitRepo.GetCommit: %w", err) } + return gitRepo, commit, ref, nil +} +func detectWorkflows(ctx context.Context, input *notifyInput, gitRepo *git.Repository, commit *git.Commit, shouldDetectSchedules bool) ([]*actions_module.DetectedWorkflow, []*actions_module.DetectedWorkflow, error) { var detectedWorkflows []*actions_module.DetectedWorkflow actionsConfig := input.Repo.MustGetUnit(ctx, unit_model.TypeActions).ActionsConfig() workflows, schedules, err := actions_module.DetectWorkflows(gitRepo, commit, @@ -204,7 +231,7 @@ func notify(ctx context.Context, input *notifyInput) error { shouldDetectSchedules, ) if err != nil { - return fmt.Errorf("DetectWorkflows: %w", err) + return nil, nil, fmt.Errorf("DetectWorkflows: %w", err) } log.Trace("repo %s with commit %s event %s find %d workflows and %d schedules", @@ -234,14 +261,14 @@ func notify(ctx context.Context, input *notifyInput) error { if prp, ok := input.Payload.(*api.PullRequestPayload); ok && errors.Is(err, util.ErrNotExist) { // the baseBranch was deleted and the PR closed: the action can be skipped if prp.Action == api.HookIssueClosed { - return nil + return nil, nil, nil } } - return fmt.Errorf("gitRepo.GetCommit: %w", err) + return nil, nil, fmt.Errorf("gitRepo.GetCommit: %w", err) } baseWorkflows, _, err := actions_module.DetectWorkflows(gitRepo, baseCommit, input.Event, input.Payload, false) if err != nil { - return fmt.Errorf("DetectWorkflows: %w", err) + return nil, nil, fmt.Errorf("DetectWorkflows: %w", err) } if len(baseWorkflows) == 0 { log.Trace("repo %s with commit %s couldn't find pull_request_target workflows", input.Repo.RepoPath(), baseCommit.ID) @@ -253,14 +280,7 @@ func notify(ctx context.Context, input *notifyInput) error { } } } - - if shouldDetectSchedules { - if err := handleSchedules(ctx, schedules, commit, input, ref.String()); err != nil { - return err - } - } - - return handleWorkflows(ctx, detectedWorkflows, commit, input, ref.String()) + return detectedWorkflows, schedules, nil } func SkipPullRequestEvent(ctx context.Context, event webhook_module.HookEventType, repoID int64, commitSHA string) bool { From bf7c63a2aeb1e665bea8001f25ba0581dd0d3663 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 30 Oct 2025 15:40:58 +0100 Subject: [PATCH 05/16] feat: add ActionUser model & fields to ActionRun ActionUser is to keep track of pull requests posters that are permanently trusted. It has a used field to track when it was last used so records can be expired instead of accumulating forever. ActionRun has new fields to make it possible to look them up given either the pull request ID or the poster ID. --- models/actions/run.go | 94 ++++++++--- models/actions/run_test.go | 119 ++++++++++++++ models/actions/user.go | 97 +++++++++++ models/actions/user_test.go | 107 +++++++++++++ models/fixtures/action_user.yml | 6 + .../v14a_actions-approval-and-trust.go | 104 ++++++++++++ .../v14a_actions-approval-and-trust_test.go | 103 ++++++++++++ .../action_run.yml | 151 ++++++++++++++++++ .../repository.yml | 5 + .../user.yml | 11 ++ services/repository/delete.go | 1 + services/user/delete.go | 1 + 12 files changed, 778 insertions(+), 21 deletions(-) create mode 100644 models/actions/user.go create mode 100644 models/actions/user_test.go create mode 100644 models/fixtures/action_user.yml create mode 100644 models/forgejo_migrations/v14a_actions-approval-and-trust.go create mode 100644 models/forgejo_migrations/v14a_actions-approval-and-trust_test.go create mode 100644 models/gitea_migrations/fixtures/Test_v14ActionsApprovalAndTrustPopulateTableActionUser/action_run.yml create mode 100644 models/gitea_migrations/fixtures/Test_v14ActionsApprovalAndTrustPopulateTableActionUser/repository.yml create mode 100644 models/gitea_migrations/fixtures/Test_v14ActionsApprovalAndTrustPopulateTableActionUser/user.yml diff --git a/models/actions/run.go b/models/actions/run.go index fe516d75d4..79a6683df9 100644 --- a/models/actions/run.go +++ b/models/actions/run.go @@ -41,27 +41,24 @@ const ( // ActionRun represents a run of a workflow file type ActionRun struct { - ID int64 - Title string - RepoID int64 `xorm:"index unique(repo_index) index(concurrency)"` - Repo *repo_model.Repository `xorm:"-"` - OwnerID int64 `xorm:"index"` - WorkflowID string `xorm:"index"` // the name of workflow file - Index int64 `xorm:"index unique(repo_index)"` // a unique number for each run of a repository - TriggerUserID int64 `xorm:"index"` - TriggerUser *user_model.User `xorm:"-"` - ScheduleID int64 - Ref string `xorm:"index"` // the commit/tag/… that caused the run - IsRefDeleted bool `xorm:"-"` - CommitSHA string - IsForkPullRequest bool // If this is triggered by a PR from a forked repository or an untrusted user, we need to check if it is approved and limit permissions when running the workflow. - NeedApproval bool // may need approval if it's a fork pull request - ApprovedBy int64 `xorm:"index"` // who approved - Event webhook_module.HookEventType // the webhook event that causes the workflow to run - EventPayload string `xorm:"LONGTEXT"` - TriggerEvent string // the trigger event defined in the `on` configuration of the triggered workflow - Status Status `xorm:"index"` - Version int `xorm:"version default 0"` // Status could be updated concomitantly, so an optimistic lock is needed + ID int64 + Title string + RepoID int64 `xorm:"index unique(repo_index) index(concurrency)"` + Repo *repo_model.Repository `xorm:"-"` + OwnerID int64 `xorm:"index"` + WorkflowID string `xorm:"index"` // the name of workflow file + Index int64 `xorm:"index unique(repo_index)"` // a unique number for each run of a repository + TriggerUserID int64 `xorm:"index"` + TriggerUser *user_model.User `xorm:"-"` + ScheduleID int64 + Ref string `xorm:"index"` // the commit/tag/… that caused the run + IsRefDeleted bool `xorm:"-"` + CommitSHA string + Event webhook_module.HookEventType // the webhook event that causes the workflow to run + EventPayload string `xorm:"LONGTEXT"` + TriggerEvent string // the trigger event defined in the `on` configuration of the triggered workflow + Status Status `xorm:"index"` + Version int `xorm:"version default 0"` // Status could be updated concomitantly, so an optimistic lock is needed // Started and Stopped is used for recording last run time, if rerun happened, they will be reset to 0 Started timeutil.TimeStamp Stopped timeutil.TimeStamp @@ -71,6 +68,13 @@ type ActionRun struct { Updated timeutil.TimeStamp `xorm:"updated"` NotifyEmail bool + // pull request trust + IsForkPullRequest bool + PullRequestPosterID int64 + PullRequestID int64 `xorm:"index"` + NeedApproval bool + ApprovedBy int64 `xorm:"index"` + ConcurrencyGroup string `xorm:"'concurrency_group' index(concurrency)"` ConcurrencyType ConcurrencyMode @@ -228,6 +232,54 @@ func clearRepoRunCountCache(repo *repo_model.Repository) { cache.Remove(actionsCountOpenCacheKey(repo.ID)) } +func condRunsThatNeedApproval(repoID, pullRequestID int64) builder.Cond { + // performance relies indexes on repo_id and pull_request_id + return builder.Eq{"repo_id": repoID, "pull_request_id": pullRequestID, "need_approval": true} +} + +func GetRunsThatNeedApprovalByRepoIDAndPullRequestID(ctx context.Context, repoID, pullRequestID int64) ([]*ActionRun, error) { + var runs []*ActionRun + if err := db.GetEngine(ctx).Where(condRunsThatNeedApproval(repoID, pullRequestID)).Find(&runs); err != nil { + return nil, err + } + return runs, nil +} + +func HasRunThatNeedApproval(ctx context.Context, repoID, pullRequestID int64) (bool, error) { + return db.GetEngine(ctx).Where(condRunsThatNeedApproval(repoID, pullRequestID)).Exist(&ActionRun{}) +} + +type ApprovalType bool + +const ( + NeedApproval = ApprovalType(true) + DoesNotNeedApproval = ApprovalType(false) + UndefinedApproval = ApprovalType(false) +) + +func UpdateRunApprovalByID(ctx context.Context, id int64, approval ApprovalType, approvedBy int64) error { + _, err := db.GetEngine(ctx).Exec("UPDATE action_run SET need_approval=?, approved_by=? WHERE id=?", bool(approval), approvedBy, id) + return err +} + +func GetRunsNotDoneByRepoIDAndPullRequestPosterID(ctx context.Context, repoID, pullRequestPosterID int64) ([]*ActionRun, error) { + var runs []*ActionRun + // performance relies on indexes on repo_id and status + if err := db.GetEngine(ctx).Where("repo_id=? AND pull_request_poster_id=?", repoID, pullRequestPosterID).And(builder.In("status", []Status{StatusUnknown, StatusWaiting, StatusRunning, StatusBlocked})).Find(&runs); err != nil { + return nil, err + } + return runs, nil +} + +func GetRunsNotDoneByRepoIDAndPullRequestID(ctx context.Context, repoID, pullRequestID int64) ([]*ActionRun, error) { + var runs []*ActionRun + // performance relies on indexes on repo_id and status + if err := db.GetEngine(ctx).Where("repo_id=? AND pull_request_id=?", repoID, pullRequestID).And(builder.In("status", []Status{StatusUnknown, StatusWaiting, StatusRunning, StatusBlocked})).Find(&runs); err != nil { + return nil, err + } + return runs, nil +} + // InsertRun inserts a run // The title will be cut off at 255 characters if it's longer than 255 characters. // We don't have to send the ActionRunNowDone notification here because there are no runs that start in a not done status. diff --git a/models/actions/run_test.go b/models/actions/run_test.go index a6973bb14d..fdb5ec320d 100644 --- a/models/actions/run_test.go +++ b/models/actions/run_test.go @@ -4,6 +4,7 @@ package actions import ( + "fmt" "testing" "forgejo.org/models/db" @@ -81,3 +82,121 @@ func TestRepoNumOpenActions(t *testing.T) { assert.Equal(t, 0, RepoNumOpenActions(t.Context(), repo.ID)) }) } + +func TestActionRun_GetRunsNotDoneByRepoIDAndPullRequestPosterID(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + repoID := int64(10) + pullRequestID := int64(3) + pullRequestPosterID := int64(30) + + runDone := &ActionRun{ + RepoID: repoID, + PullRequestID: pullRequestID, + PullRequestPosterID: pullRequestPosterID, + Status: StatusSuccess, + } + require.NoError(t, InsertRun(t.Context(), runDone, nil)) + + unrelatedUser := int64(5) + runNotByPoster := &ActionRun{ + RepoID: repoID, + PullRequestID: pullRequestID, + PullRequestPosterID: unrelatedUser, + Status: StatusRunning, + } + require.NoError(t, InsertRun(t.Context(), runNotByPoster, nil)) + + unrelatedRepository := int64(6) + runNotInTheSameRepository := &ActionRun{ + RepoID: unrelatedRepository, + PullRequestID: pullRequestID, + PullRequestPosterID: pullRequestPosterID, + Status: StatusSuccess, + } + require.NoError(t, InsertRun(t.Context(), runNotInTheSameRepository, nil)) + + for _, status := range []Status{StatusUnknown, StatusWaiting, StatusRunning} { + t.Run(fmt.Sprintf("%s", status), func(t *testing.T) { + runNotDone := &ActionRun{ + RepoID: repoID, + PullRequestID: pullRequestID, + Status: status, + PullRequestPosterID: pullRequestPosterID, + } + require.NoError(t, InsertRun(t.Context(), runNotDone, nil)) + runs, err := GetRunsNotDoneByRepoIDAndPullRequestPosterID(t.Context(), repoID, pullRequestPosterID) + require.NoError(t, err) + require.Len(t, runs, 1) + run := runs[0] + assert.Equal(t, runNotDone.ID, run.ID) + assert.Equal(t, runNotDone.Status, run.Status) + unittest.AssertSuccessfulDelete(t, run) + }) + } +} + +func TestActionRun_NeedApproval(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + pullRequestPosterID := int64(4) + repoID := int64(10) + pullRequestID := int64(2) + runDoesNotNeedApproval := &ActionRun{ + RepoID: repoID, + PullRequestID: pullRequestID, + PullRequestPosterID: pullRequestPosterID, + } + require.NoError(t, InsertRun(t.Context(), runDoesNotNeedApproval, nil)) + unrelatedRepository := int64(6) + runNotInTheSameRepository := &ActionRun{ + RepoID: unrelatedRepository, + PullRequestID: pullRequestID, + PullRequestPosterID: pullRequestPosterID, + NeedApproval: true, + } + require.NoError(t, InsertRun(t.Context(), runNotInTheSameRepository, nil)) + unrelatedPullRequest := int64(3) + runNotInTheSamePullRequest := &ActionRun{ + RepoID: repoID, + PullRequestID: unrelatedPullRequest, + PullRequestPosterID: pullRequestPosterID, + NeedApproval: true, + } + require.NoError(t, InsertRun(t.Context(), runNotInTheSamePullRequest, nil)) + + t.Run("HasRunThatNeedApproval is false", func(t *testing.T) { + has, err := HasRunThatNeedApproval(t.Context(), repoID, pullRequestID) + require.NoError(t, err) + assert.False(t, has) + }) + + runNeedApproval := &ActionRun{ + RepoID: repoID, + PullRequestID: pullRequestID, + PullRequestPosterID: pullRequestPosterID, + NeedApproval: true, + } + require.NoError(t, InsertRun(t.Context(), runNeedApproval, nil)) + + t.Run("HasRunThatNeedApproval is true", func(t *testing.T) { + has, err := HasRunThatNeedApproval(t.Context(), repoID, pullRequestID) + require.NoError(t, err) + assert.True(t, has) + }) + + assertApprovalEqual := func(t *testing.T, expected, actual *ActionRun) { + t.Helper() + assert.Equal(t, expected.RepoID, actual.RepoID) + assert.Equal(t, expected.PullRequestID, actual.PullRequestID) + assert.Equal(t, expected.PullRequestPosterID, actual.PullRequestPosterID) + assert.Equal(t, expected.NeedApproval, actual.NeedApproval) + } + + t.Run("GetRunsThatNeedApproval", func(t *testing.T) { + runs, err := GetRunsThatNeedApprovalByRepoIDAndPullRequestID(t.Context(), repoID, pullRequestID) + require.NoError(t, err) + require.Len(t, runs, 1) + assertApprovalEqual(t, runNeedApproval, runs[0]) + }) +} diff --git a/models/actions/user.go b/models/actions/user.go new file mode 100644 index 0000000000..1be8606e2e --- /dev/null +++ b/models/actions/user.go @@ -0,0 +1,97 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package actions + +import ( + "context" + "fmt" + "time" + + "forgejo.org/models/db" + "forgejo.org/modules/timeutil" + + "xorm.io/builder" +) + +type ActionUser struct { + ID int64 `xorm:"pk autoincr"` + UserID int64 `xorm:"INDEX UNIQUE(action_user_index) REFERENCES(user, id)"` + RepoID int64 `xorm:"INDEX UNIQUE(action_user_index) REFERENCES(repository, id)"` + + TrustedWithPullRequests bool + + LastAccess timeutil.TimeStamp `xorm:"INDEX"` +} + +func init() { + db.RegisterModel(new(ActionUser)) +} + +type ErrUserNotExist struct { + UserID int64 + RepoID int64 +} + +func IsErrUserNotExist(err error) bool { + _, ok := err.(ErrUserNotExist) + return ok +} + +func (err ErrUserNotExist) Error() string { + return fmt.Sprintf("ActionUser does not exist [user_id: %d, repo_id: %d]", err.UserID, err.RepoID) +} + +func InsertActionUser(ctx context.Context, user *ActionUser) error { + user.LastAccess = timeutil.TimeStampNow() + return db.Insert(ctx, user) +} + +func DeleteActionUserByUserIDAndRepoID(ctx context.Context, userID, repoID int64) error { + _, err := db.GetEngine(ctx).Table(&ActionUser{}).Where("user_id=? AND repo_id=?", userID, repoID).Delete() + return err +} + +var updateFrequency = 24 * time.Hour + +func MaybeUpdateAccess(ctx context.Context, user *ActionUser) error { + // Keep track of the last time the record was accessed to identify which one + // are never accessed so they can be removed eventually. But only every updateFrequency + // to not stress the underlying database. + if timeutil.TimeStampNow() > user.LastAccess.AddDuration(updateFrequency) { + user.LastAccess = timeutil.TimeStampNow() + if _, err := db.GetEngine(ctx).ID(user.ID).Cols("last_access").Update(user); err != nil { + return err + } + } + + return nil +} + +func GetActionUserByUserIDAndRepoID(ctx context.Context, userID, repoID int64) (*ActionUser, error) { + user := new(ActionUser) + has, err := db.GetEngine(ctx).Where("user_id=? AND repo_id=?", userID, repoID).Get(user) + if err != nil { + return nil, err + } else if !has { + return nil, ErrUserNotExist{userID, repoID} + } + return user, nil +} + +func GetActionUserByUserIDAndRepoIDAndUpdateAccess(ctx context.Context, userID, repoID int64) (*ActionUser, error) { + user, err := GetActionUserByUserIDAndRepoID(ctx, userID, repoID) + if err != nil { + return nil, err + } + return user, MaybeUpdateAccess(ctx, user) +} + +var expire = 3 * 30 * 24 * time.Hour + +func RevokeInactiveActionUser(ctx context.Context) error { + olderThan := timeutil.TimeStampNow().AddDuration(-expire) + + _, err := db.GetEngine(ctx).Where(builder.Lt{"last_access": olderThan}).Delete(&ActionUser{}) + return err +} diff --git a/models/actions/user_test.go b/models/actions/user_test.go new file mode 100644 index 0000000000..648609a151 --- /dev/null +++ b/models/actions/user_test.go @@ -0,0 +1,107 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package actions + +import ( + "testing" + "time" + + repo_model "forgejo.org/models/repo" + "forgejo.org/models/unittest" + user_model "forgejo.org/models/user" + "forgejo.org/modules/test" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestActionUser_CreateDelete(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + require.ErrorContains(t, InsertActionUser(t.Context(), &ActionUser{ + UserID: user.ID, + }), "FOREIGN KEY") + + require.ErrorContains(t, InsertActionUser(t.Context(), &ActionUser{ + RepoID: repo.ID, + }), "FOREIGN KEY") + + actionUser := &ActionUser{ + UserID: user.ID, + RepoID: repo.ID, + } + require.NoError(t, InsertActionUser(t.Context(), actionUser)) + assert.NotZero(t, actionUser.ID) + assert.NotZero(t, actionUser.LastAccess) + + otherUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}) + actionUserNotSameUser := &ActionUser{ + UserID: otherUser.ID, + RepoID: repo.ID, + } + require.NoError(t, InsertActionUser(t.Context(), actionUserNotSameUser)) + + otherRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) + actionUserNotSameRepo := &ActionUser{ + UserID: user.ID, + RepoID: otherRepo.ID, + } + require.NoError(t, InsertActionUser(t.Context(), actionUserNotSameRepo)) + + unittest.AssertExistsAndLoadBean(t, &ActionUser{ID: actionUser.ID}) + require.NoError(t, DeleteActionUserByUserIDAndRepoID(t.Context(), user.ID, repo.ID)) + unittest.AssertNotExistsBean(t, &ActionUser{ID: actionUser.ID}) +} + +func TestActionUser_RevokeInactiveActionUser(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + actionUser := &ActionUser{ + UserID: user.ID, + RepoID: repo.ID, + } + require.NoError(t, InsertActionUser(t.Context(), actionUser)) + + t.Run("not revoked because it was just created", func(t *testing.T) { + unittest.AssertExistsAndLoadBean(t, &ActionUser{ID: actionUser.ID}) + require.NoError(t, RevokeInactiveActionUser(t.Context())) + unittest.AssertExistsAndLoadBean(t, &ActionUser{ID: actionUser.ID}) + }) + + // needs to be at least 1 second because unix timestamp resolution is 1 second + defer test.MockVariableValue(&expire, 1*time.Second)() + + t.Run("used not updated too frequently", func(t *testing.T) { + time.Sleep(2 * time.Second) + usedActionUser, err := GetActionUserByUserIDAndRepoIDAndUpdateAccess(t.Context(), user.ID, repo.ID) + require.NoError(t, err) + require.Equal(t, actionUser.ID, usedActionUser.ID) + assert.Equal(t, usedActionUser.LastAccess, actionUser.LastAccess) + }) + + defer test.MockVariableValue(&updateFrequency, 0)() + + t.Run("not revoked because it was recently used", func(t *testing.T) { + time.Sleep(2 * time.Second) + usedActionUser, err := GetActionUserByUserIDAndRepoIDAndUpdateAccess(t.Context(), user.ID, repo.ID) + require.NoError(t, err) + require.Equal(t, actionUser.ID, usedActionUser.ID) + assert.Greater(t, usedActionUser.LastAccess, actionUser.LastAccess) + require.NoError(t, RevokeInactiveActionUser(t.Context())) + unittest.AssertExistsAndLoadBean(t, &ActionUser{ID: actionUser.ID}) + }) + + t.Run("revoked because it was not recently used", func(t *testing.T) { + time.Sleep(2 * time.Second) + unittest.AssertExistsAndLoadBean(t, &ActionUser{ID: actionUser.ID}) + require.NoError(t, RevokeInactiveActionUser(t.Context())) + unittest.AssertNotExistsBean(t, &ActionUser{ID: actionUser.ID}) + }) +} diff --git a/models/fixtures/action_user.yml b/models/fixtures/action_user.yml new file mode 100644 index 0000000000..63b63a0e58 --- /dev/null +++ b/models/fixtures/action_user.yml @@ -0,0 +1,6 @@ +- + id: 1 + repo_id: 1 + user_id: 2 + trusted_with_pull_requests: true + last_access: 1683636528 diff --git a/models/forgejo_migrations/v14a_actions-approval-and-trust.go b/models/forgejo_migrations/v14a_actions-approval-and-trust.go new file mode 100644 index 0000000000..657d512548 --- /dev/null +++ b/models/forgejo_migrations/v14a_actions-approval-and-trust.go @@ -0,0 +1,104 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package forgejo_migrations + +import ( + "context" + + actions_model "forgejo.org/models/actions" + "forgejo.org/models/db" + "forgejo.org/modules/log" + "forgejo.org/modules/timeutil" + + "xorm.io/xorm" +) + +func init() { + registerMigration(&Migration{ + Description: "add actions approval and trust table and fields", + Upgrade: v14ActionsApprovalAndTrust, + }) +} + +func v14ActionsApprovalAndTrust(x *xorm.Engine) error { + if err := v14ActionsApprovalAndTrustCreateTableActionUser(x); err != nil { + return err + } + if err := v14ActionsApprovalAndTrustAddActionsRunFields(x); err != nil { + return err + } + return v14ActionsApprovalAndTrustPopulateTableActionUser(x) +} + +func v14ActionsApprovalAndTrustCreateTableActionUser(x *xorm.Engine) error { + type ActionUser struct { + ID int64 `xorm:"pk autoincr"` + UserID int64 `xorm:"INDEX UNIQUE(action_user_index) REFERENCES(user, id)"` + RepoID int64 `xorm:"INDEX UNIQUE(action_user_index) REFERENCES(repository, id)"` + + TrustedWithPullRequests bool + + LastAccess timeutil.TimeStamp `xorm:"INDEX"` + } + return x.Sync(new(ActionUser)) +} + +func v14ActionsApprovalAndTrustAddActionsRunFields(x *xorm.Engine) error { + type ActionRun struct { + PullRequestPosterID int64 + PullRequestID int64 `xorm:"index"` + } + _, err := x.SyncWithOptions(xorm.SyncOptions{IgnoreDropIndices: true}, new(ActionRun)) + return err +} + +type v14ActionsApprovalAndTrustTrusted struct { + RepoID int64 + UserID int64 +} + +func v14ActionsApprovalAndTrustPopulateTableActionUser(x *xorm.Engine) error { + // + // Users approved once were trusted before and are trusted now. + // + // The admin will see they can revoke that trust when the user + // submits a new pull request. + // + // If the user does not submit any pull request, this trust will + // eventually be automatically revoked. + // + // The number of trusted users is assumed to be small enough to not require + // pagination, even on large instances. + // + log.Info("v14a_actions-approval-and-trust: search") + var trustedList []*v14ActionsApprovalAndTrustTrusted + if err := x.Table("`action_run`"). + Select("DISTINCT `action_run`.`repo_id`, `action_run`.`trigger_user_id` AS `user_id`"). + Join("INNER", "`repository`", "`repository`.`id` = `action_run`.`repo_id`"). + Join("INNER", "`user`", "`user`.`id` = `action_run`.`trigger_user_id`"). + Where("`action_run`.`approved_by` > 0 AND `action_run`.`trigger_user_id` > 0"). + OrderBy("`action_run`.`repo_id`, `action_run`.`trigger_user_id`"). + Find(&trustedList); err != nil { + return err + } + + log.Info("v14a_actions-approval-and-trust: start adding %d users trusted with workflow runs", len(trustedList)) + if err := db.WithTx(db.DefaultContext, func(ctx context.Context) error { + for _, trusted := range trustedList { + log.Debug("v14a_actions-approval-and-trust: repository %d trusts user %d", trusted.RepoID, trusted.UserID) + if err := actions_model.InsertActionUser(ctx, &actions_model.ActionUser{ + RepoID: trusted.RepoID, + UserID: trusted.UserID, + TrustedWithPullRequests: true, + }); err != nil { + return err + } + } + return nil + }); err != nil { + return err + } + log.Info("v14a_actions-approval-and-trust: done adding %d users", len(trustedList)) + return nil +} diff --git a/models/forgejo_migrations/v14a_actions-approval-and-trust_test.go b/models/forgejo_migrations/v14a_actions-approval-and-trust_test.go new file mode 100644 index 0000000000..c639a0d2e9 --- /dev/null +++ b/models/forgejo_migrations/v14a_actions-approval-and-trust_test.go @@ -0,0 +1,103 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package forgejo_migrations + +import ( + "testing" + "time" + + actions_model "forgejo.org/models/actions" + "forgejo.org/models/db" + migration_tests "forgejo.org/models/gitea_migrations/test" + repo_model "forgejo.org/models/repo" + user_model "forgejo.org/models/user" + "forgejo.org/modules/timeutil" + webhook_module "forgejo.org/modules/webhook" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func Test_v14ActionsApprovalAndTrustPopulateTableActionUser(t *testing.T) { + type ActionUser struct { + ID int64 `xorm:"pk autoincr"` + UserID int64 `xorm:"INDEX UNIQUE(action_user_index) REFERENCES(user, id)"` + RepoID int64 `xorm:"INDEX UNIQUE(action_user_index) REFERENCES(repository, id)"` + + TrustedWithPullRequests bool + + LastAccess timeutil.TimeStamp `xorm:"INDEX"` + } + type ActionRun struct { + ID int64 + Title string + RepoID int64 `xorm:"index unique(repo_index) index(concurrency)"` + Repo *repo_model.Repository `xorm:"-"` + OwnerID int64 `xorm:"index"` + WorkflowID string `xorm:"index"` // the name of workflow file + Index int64 `xorm:"index unique(repo_index)"` // a unique number for each run of a repository + TriggerUserID int64 `xorm:"index"` + TriggerUser *user_model.User `xorm:"-"` + ScheduleID int64 + Ref string `xorm:"index"` // the commit/tag/… that caused the run + IsRefDeleted bool `xorm:"-"` + CommitSHA string + Event webhook_module.HookEventType // the webhook event that causes the workflow to run + EventPayload string `xorm:"LONGTEXT"` + TriggerEvent string // the trigger event defined in the `on` configuration of the triggered workflow + Status actions_model.Status `xorm:"index"` + Version int `xorm:"version default 0"` // Status could be updated concomitantly, so an optimistic lock is needed + // Started and Stopped is used for recording last run time, if rerun happened, they will be reset to 0 + Started timeutil.TimeStamp + Stopped timeutil.TimeStamp + // PreviousDuration is used for recording previous duration + PreviousDuration time.Duration + Created timeutil.TimeStamp `xorm:"created"` + Updated timeutil.TimeStamp `xorm:"updated"` + NotifyEmail bool + + // pull request trust + IsForkPullRequest bool + PullRequestPosterID int64 + PullRequestID int64 `xorm:"index"` + NeedApproval bool + ApprovedBy int64 `xorm:"index"` + + ConcurrencyGroup string `xorm:"'concurrency_group' index(concurrency)"` + ConcurrencyType actions_model.ConcurrencyMode + + PreExecutionError string `xorm:"LONGTEXT"` // used to report errors that blocked execution of a workflow + } + type Repository struct { + ID int64 `xorm:"pk autoincr"` + } + type User struct { + ID int64 `xorm:"pk autoincr"` + } + x, deferable := migration_tests.PrepareTestEnv(t, 0, new(User), new(Repository), new(ActionUser), new(ActionRun)) + defer deferable() + if x == nil || t.Failed() { + return + } + + require.NoError(t, v14ActionsApprovalAndTrustPopulateTableActionUser(x)) + + var users []*actions_model.ActionUser + require.NoError(t, db.GetEngine(t.Context()).Select("`repo_id`, `user_id`").OrderBy("`id`").Find(&users)) + // See models/gitea_migrations/fixtures/Test_v14ActionsApprovalAndTrustPopulateTableActionUser/action_run.yml + assert.Equal(t, []*actions_model.ActionUser{ + { + UserID: 3, + RepoID: 15, + }, + { + UserID: 3, + RepoID: 63, + }, + { + UserID: 4, + RepoID: 63, + }, + }, users) +} diff --git a/models/gitea_migrations/fixtures/Test_v14ActionsApprovalAndTrustPopulateTableActionUser/action_run.yml b/models/gitea_migrations/fixtures/Test_v14ActionsApprovalAndTrustPopulateTableActionUser/action_run.yml new file mode 100644 index 0000000000..78eb18a955 --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_v14ActionsApprovalAndTrustPopulateTableActionUser/action_run.yml @@ -0,0 +1,151 @@ +- + id: 1000 + title: "63, 2 not trusted" + repo_id: 63 + trigger_user_id: 2 + approved_by: 0 + owner_id: 2 + workflow_id: "running.yaml" + index: 1000 + ref: "refs/heads/main" + commit_sha: "97f29ee599c373c729132a5c46a046978311e0ee" + trigger_event: "workflow_dispatch" + is_fork_pull_request: 0 + status: 1 # success + started: 1683636528 + created: 1683636108 + updated: 1683636626 + need_approval: 0 + +- + id: 1001 + title: "63, 3 trusted" + repo_id: 63 + trigger_user_id: 3 + approved_by: 1 + owner_id: 2 + workflow_id: "running.yaml" + index: 1001 + ref: "refs/heads/main" + commit_sha: "97f29ee599c373c729132a5c46a046978311e0ee" + trigger_event: "workflow_dispatch" + is_fork_pull_request: 0 + status: 1 # success + started: 1683636528 + created: 1683636108 + updated: 1683636626 + need_approval: 0 + +- + id: 1002 + title: "63, 3 trusted (duplicate, ignored)" + repo_id: 63 + trigger_user_id: 3 + approved_by: 1 + owner_id: 2 + workflow_id: "running.yaml" + index: 1002 + ref: "refs/heads/main" + commit_sha: "97f29ee599c373c729132a5c46a046978311e0ee" + trigger_event: "workflow_dispatch" + is_fork_pull_request: 0 + status: 1 # success + started: 1683636528 + created: 1683636108 + updated: 1683636626 + need_approval: 0 + +- + id: 1003 + title: "5000, 3 trusted (non existent repo_id, ignored)" + repo_id: 5000 + trigger_user_id: 2 + approved_by: 1 + owner_id: 2 + workflow_id: "running.yaml" + index: 1003 + ref: "refs/heads/main" + commit_sha: "97f29ee599c373c729132a5c46a046978311e0ee" + trigger_event: "workflow_dispatch" + is_fork_pull_request: 0 + status: 1 # success + started: 1683636528 + created: 1683636108 + updated: 1683636626 + need_approval: 0 + +- + id: 1004 + title: "63, 3000 trusted (non existent trigger_user, ignored)" + repo_id: 63 + trigger_user_id: 3000 + approved_by: 1 + owner_id: 2 + workflow_id: "running.yaml" + index: 1004 + ref: "refs/heads/main" + commit_sha: "97f29ee599c373c729132a5c46a046978311e0ee" + trigger_event: "workflow_dispatch" + is_fork_pull_request: 0 + status: 1 # success + started: 1683636528 + created: 1683636108 + updated: 1683636626 + need_approval: 0 + +- + id: 1005 + title: "63, 4 trusted" + repo_id: 63 + trigger_user_id: 4 + approved_by: 1 + owner_id: 2 + workflow_id: "running.yaml" + index: 1005 + ref: "refs/heads/main" + commit_sha: "97f29ee599c373c729132a5c46a046978311e0ee" + trigger_event: "workflow_dispatch" + is_fork_pull_request: 0 + status: 1 # success + started: 1683636528 + created: 1683636108 + updated: 1683636626 + need_approval: 0 + +- + id: 1006 + title: "15, 3 trusted" + repo_id: 15 + trigger_user_id: 3 + approved_by: 1 + owner_id: 2 + workflow_id: "running.yaml" + index: 1006 + ref: "refs/heads/main" + commit_sha: "97f29ee599c373c729132a5c46a046978311e0ee" + trigger_event: "workflow_dispatch" + is_fork_pull_request: 0 + status: 1 # success + started: 1683636528 + created: 1683636108 + updated: 1683636626 + need_approval: 0 + +- + id: 1007 + title: "15, 0 trigger user id zero is ignored" + repo_id: 15 + trigger_user_id: 0 + approved_by: 1 + owner_id: 2 + workflow_id: "running.yaml" + index: 1007 + ref: "refs/heads/main" + commit_sha: "97f29ee599c373c729132a5c46a046978311e0ee" + trigger_event: "workflow_dispatch" + is_fork_pull_request: 0 + status: 1 # success + started: 1683636528 + created: 1683636108 + updated: 1683636626 + need_approval: 0 diff --git a/models/gitea_migrations/fixtures/Test_v14ActionsApprovalAndTrustPopulateTableActionUser/repository.yml b/models/gitea_migrations/fixtures/Test_v14ActionsApprovalAndTrustPopulateTableActionUser/repository.yml new file mode 100644 index 0000000000..a47c261499 --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_v14ActionsApprovalAndTrustPopulateTableActionUser/repository.yml @@ -0,0 +1,5 @@ +- + id: 15 + +- + id: 63 diff --git a/models/gitea_migrations/fixtures/Test_v14ActionsApprovalAndTrustPopulateTableActionUser/user.yml b/models/gitea_migrations/fixtures/Test_v14ActionsApprovalAndTrustPopulateTableActionUser/user.yml new file mode 100644 index 0000000000..8063fa12af --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_v14ActionsApprovalAndTrustPopulateTableActionUser/user.yml @@ -0,0 +1,11 @@ +- # NOTE: this user (id=1) is the admin + id: 1 + +- + id: 2 + +- + id: 3 + +- + id: 4 diff --git a/services/repository/delete.go b/services/repository/delete.go index a9ef4d8515..9eca085da3 100644 --- a/services/repository/delete.go +++ b/services/repository/delete.go @@ -177,6 +177,7 @@ func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID &actions_model.ActionScheduleSpec{RepoID: repoID}, &actions_model.ActionSchedule{RepoID: repoID}, &actions_model.ActionArtifact{RepoID: repoID}, + &actions_model.ActionUser{RepoID: repoID}, &repo_model.RepoArchiveDownloadCount{RepoID: repoID}, &actions_model.ActionRunnerToken{RepoID: repoID}, ); err != nil { diff --git a/services/user/delete.go b/services/user/delete.go index 2edc95ab58..0c2a0b63ed 100644 --- a/services/user/delete.go +++ b/services/user/delete.go @@ -94,6 +94,7 @@ func deleteUser(ctx context.Context, u *user_model.User, purge bool) (err error) &pull_model.ReviewState{UserID: u.ID}, &user_model.Redirect{RedirectUserID: u.ID}, &actions_model.ActionRunner{OwnerID: u.ID}, + &actions_model.ActionUser{UserID: u.ID}, &user_model.BlockedUser{BlockID: u.ID}, &user_model.BlockedUser{UserID: u.ID}, &actions_model.ActionRunnerToken{OwnerID: u.ID}, From 14e329b33ceec145028237ef8bf2370d161b39b5 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 30 Oct 2025 15:44:30 +0100 Subject: [PATCH 06/16] chore(tests): allow actions.InsertRun to be used when no job exists It will never happen in production but is convenient when unit testing. --- models/actions/run.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/models/actions/run.go b/models/actions/run.go index 79a6683df9..d8a37d0b60 100644 --- a/models/actions/run.go +++ b/models/actions/run.go @@ -350,8 +350,10 @@ func InsertRun(ctx context.Context, run *ActionRun, jobs []*jobparser.SingleWork Status: status, }) } - if err := db.Insert(ctx, runJobs); err != nil { - return err + if len(runJobs) > 0 { + if err := db.Insert(ctx, runJobs); err != nil { + return err + } } // if there is a job in the waiting status, increase tasks version. From 5da1d8dcd7aa59bd18c1ef9e1dfede093ab233db Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 30 Oct 2025 15:45:55 +0100 Subject: [PATCH 07/16] feat: add model pull request IsForkPullRequest helper So the logic by which a pull request is considered to be a fork from a security standpoint is in one place. --- models/issues/pull.go | 17 +++++++++++++++++ models/issues/pull_test.go | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/models/issues/pull.go b/models/issues/pull.go index 44ead3b9f1..6f65ef3af9 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -470,6 +470,23 @@ func (pr *PullRequest) GetReviewCommentsCount(ctx context.Context) int { return int(count) } +func (pr *PullRequest) IsForkPullRequest() bool { + var isForkPullRequest bool + + switch pr.Flow { + case PullRequestFlowGithub: + isForkPullRequest = pr.IsFromFork() + case PullRequestFlowAGit: + // there is no fork concept in AGit flow, anyone with read permission can push refs/for// to the repo. + // So we must treat it as a fork pull request because it may be from an untrusted user + isForkPullRequest = true + default: + // unknown flow, treat it as it's a fork pull request + isForkPullRequest = true + } + return isForkPullRequest +} + // IsChecking returns true if this pull request is still checking conflict. func (pr *PullRequest) IsChecking() bool { return pr.Status == PullRequestStatusChecking diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index db10386b5a..dd97ed34b2 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -466,6 +466,43 @@ func TestGetPullRequestByMergedCommit(t *testing.T) { require.ErrorAs(t, err, &issues_model.ErrPullRequestNotExist{}) } +func TestPullRequest_IsForkPullRequest(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + t.Run("FlowGithub from a fork", func(t *testing.T) { + pr := &issues_model.PullRequest{ + Flow: issues_model.PullRequestFlowGithub, + HeadRepoID: 111, + BaseRepoID: 222, + } + assert.True(t, pr.IsForkPullRequest()) + }) + + t.Run("FlowGithub from the same repository", func(t *testing.T) { + pr := &issues_model.PullRequest{ + Flow: issues_model.PullRequestFlowGithub, + HeadRepoID: 111, + BaseRepoID: 111, + } + assert.False(t, pr.IsForkPullRequest()) + }) + + t.Run("PullRequestFlowAGit", func(t *testing.T) { + pr := &issues_model.PullRequest{ + Flow: issues_model.PullRequestFlowAGit, + } + assert.True(t, pr.IsForkPullRequest()) + }) + + t.Run("Other", func(t *testing.T) { + unknown := issues_model.PullRequestFlow(4854) + pr := &issues_model.PullRequest{ + Flow: unknown, + } + assert.True(t, pr.IsForkPullRequest()) + }) +} + func TestMigrate_InsertPullRequests(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) reponame := "repo1" From 71439965d64622d6245f9a54fea053aa6426e987 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 30 Oct 2025 15:54:55 +0100 Subject: [PATCH 08/16] feat: add the actions CancelRun and ApproveRun helpers CancelRun Cancels all the jobs of a given run. It is very similar to the less generic web/repo/actions/view.go with two differences: - It updates NeedApproval - The commit status are created within the transaction It is also very similar to cancelJobsForRun in services/actions/schedule_tasks.go Keeping those DRY would require a small refactor that does not feel necessary at this moment. ApproveRun Approves all the jobs of a given run. --- .../action_run.yml | 38 +++++++ .../action_run_job.yml | 42 +++++++ .../action_task.yml | 40 +++++++ services/actions/run.go | 70 ++++++++++++ services/actions/run_test.go | 104 ++++++++++++++++++ 5 files changed, 294 insertions(+) create mode 100644 services/actions/TestActions_CancelOrApproveRun/action_run.yml create mode 100644 services/actions/TestActions_CancelOrApproveRun/action_run_job.yml create mode 100644 services/actions/TestActions_CancelOrApproveRun/action_task.yml create mode 100644 services/actions/run.go create mode 100644 services/actions/run_test.go diff --git a/services/actions/TestActions_CancelOrApproveRun/action_run.yml b/services/actions/TestActions_CancelOrApproveRun/action_run.yml new file mode 100644 index 0000000000..b084007503 --- /dev/null +++ b/services/actions/TestActions_CancelOrApproveRun/action_run.yml @@ -0,0 +1,38 @@ +- + id: 900 + title: "run running" + repo_id: 63 + owner_id: 2 + workflow_id: "test.yaml" + index: 4 + trigger_user_id: 2 + ref: "refs/heads/main" + commit_sha: "97f29ee599c373c729132a5c46a046978311e0ee" + trigger_event: "synchronized" + is_fork_pull_request: 0 + status: 6 # running + started: 1683636528 + created: 1683636108 + updated: 1683636626 + need_approval: 0 + approved_by: 0 + concurrency_group: abc123 +- + id: 800 + title: "run waiting because jobs need approval" + repo_id: 63 + owner_id: 2 + workflow_id: "test.yaml" + index: 5 + trigger_user_id: 2 + ref: "refs/heads/main" + commit_sha: "97f29ee599c373c729132a5c46a046978311e0ee" + trigger_event: "synchronized" + is_fork_pull_request: 0 + status: 5 # waiting + started: 1683636528 + created: 1683636108 + updated: 1683636626 + need_approval: 1 + approved_by: 0 + concurrency_group: def435 diff --git a/services/actions/TestActions_CancelOrApproveRun/action_run_job.yml b/services/actions/TestActions_CancelOrApproveRun/action_run_job.yml new file mode 100644 index 0000000000..610f34e274 --- /dev/null +++ b/services/actions/TestActions_CancelOrApproveRun/action_run_job.yml @@ -0,0 +1,42 @@ +- + id: 10900 + run_id: 900 + repo_id: 63 + owner_id: 2 + commit_sha: 97f29ee599c373c729132a5c46a046978311e0ee + is_fork_pull_request: 0 + name: job_1 + attempt: 0 + job_id: job_1 + task_id: 0 + status: 1 # success + runs_on: '["docker"]' + started: 1683636528 +- + id: 11900 + run_id: 900 + repo_id: 63 + owner_id: 2 + commit_sha: 97f29ee599c373c729132a5c46a046978311e0ee + is_fork_pull_request: 0 + name: job_2 + attempt: 0 + job_id: job_2 + task_id: 711900 + status: 6 # running + runs_on: '["docker"]' + started: 1683636528 +- + id: 10800 + run_id: 800 + repo_id: 63 + owner_id: 2 + commit_sha: 97f29ee599c373c729132a5c46a046978311e0ee + is_fork_pull_request: 0 + name: job_1 + attempt: 0 + job_id: job_1 + task_id: 0 + status: 7 # blocked + runs_on: '["docker"]' + started: 1683636528 diff --git a/services/actions/TestActions_CancelOrApproveRun/action_task.yml b/services/actions/TestActions_CancelOrApproveRun/action_task.yml new file mode 100644 index 0000000000..5aaafe97a1 --- /dev/null +++ b/services/actions/TestActions_CancelOrApproveRun/action_task.yml @@ -0,0 +1,40 @@ +- + id: 710900 + job_id: 10900 + attempt: 0 + runner_id: 1 + status: 1 # success + started: 1683636528 + stopped: 1683636626 + repo_id: 63 + owner_id: 2 + commit_sha: c2d72f548424103f01ee1dc02889c1e2bff816b0 + is_fork_pull_request: 0 + token_hash: 8d8ef48297195edcc8e22c70b3020eaa06c52976db67d39b4260c64a69a2cc1508825121b7b8394e48e00b1bf8718b2a867e + token_salt: jVuKnSPGgy + token_last_eight: eeb1a71a + log_filename: artifact-test2/2f/47.log + log_in_storage: 1 + log_length: 707 + log_size: 90179 + log_expired: 0 +- + id: 711900 + job_id: 11900 + attempt: 0 + runner_id: 1 + status: 6 # running + started: 1683636528 + stopped: 1683636626 + repo_id: 63 + owner_id: 2 + commit_sha: c2d72f548424103f01ee1dc02889c1e2bff816b0 + is_fork_pull_request: 0 + token_hash: 7d8ef48297195edcc8e22c70b3020eaa06c52976db67d39b4260c64a69a2cc1508825121b7b8394e48e00b1bf8718b2a867e + token_salt: jVuKnSPGgy + token_last_eight: eeb1a71a + log_filename: artifact-test2/2f/47.log + log_in_storage: 1 + log_length: 707 + log_size: 90179 + log_expired: 0 diff --git a/services/actions/run.go b/services/actions/run.go new file mode 100644 index 0000000000..7f64e00b92 --- /dev/null +++ b/services/actions/run.go @@ -0,0 +1,70 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package actions + +import ( + "context" + + actions_model "forgejo.org/models/actions" + "forgejo.org/models/db" + "forgejo.org/modules/timeutil" +) + +func CancelRun(ctx context.Context, run *actions_model.ActionRun) error { + return db.WithTx(ctx, func(ctx context.Context) error { + jobs, err := actions_model.GetRunJobsByRunID(ctx, run.ID) + if err != nil { + return err + } + for _, job := range jobs { + status := job.Status + if status.IsDone() { + continue + } + if job.TaskID == 0 { + job.Status = actions_model.StatusCancelled + job.Stopped = timeutil.TimeStampNow() + _, err := actions_model.UpdateRunJobWithoutNotification(ctx, job, nil, "status", "stopped") + if err != nil { + return err + } + continue + } + if err := StopTask(ctx, job.TaskID, actions_model.StatusCancelled); err != nil { + return err + } + } + + if run.NeedApproval { + if err := actions_model.UpdateRunApprovalByID(ctx, run.ID, actions_model.DoesNotNeedApproval, 0); err != nil { + return err + } + } + + CreateCommitStatus(ctx, jobs...) + + return nil + }) +} + +func ApproveRun(ctx context.Context, run *actions_model.ActionRun, doerID int64) error { + return db.WithTx(ctx, func(ctx context.Context) error { + jobs, err := actions_model.GetRunJobsByRunID(ctx, run.ID) + if err != nil { + return err + } + for _, job := range jobs { + if len(job.Needs) == 0 && job.Status.IsBlocked() { + job.Status = actions_model.StatusWaiting + _, err := UpdateRunJob(ctx, job, nil, "status") + if err != nil { + return err + } + } + } + CreateCommitStatus(ctx, jobs...) + + return actions_model.UpdateRunApprovalByID(ctx, run.ID, actions_model.DoesNotNeedApproval, doerID) + }) +} diff --git a/services/actions/run_test.go b/services/actions/run_test.go new file mode 100644 index 0000000000..15d64a9346 --- /dev/null +++ b/services/actions/run_test.go @@ -0,0 +1,104 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package actions + +import ( + "testing" + + actions_model "forgejo.org/models/actions" + "forgejo.org/models/unittest" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestActions_CancelOrApproveRun(t *testing.T) { + t.Run("run, job and task Running changes to run, job and task Cancelled", func(t *testing.T) { + defer unittest.OverrideFixtures("services/actions/TestActions_CancelOrApproveRun")() + require.NoError(t, unittest.PrepareTestDatabase()) + + taskID := int64(711900) + task := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: taskID}) + require.Equal(t, actions_model.StatusRunning.String(), task.Status.String()) + job := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{ID: task.JobID}) + require.Equal(t, actions_model.StatusRunning.String(), job.Status.String()) + require.Zero(t, job.Stopped) + run := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: job.RunID}) + require.Equal(t, actions_model.StatusRunning.String(), run.Status.String()) + + require.NoError(t, CancelRun(t.Context(), run)) + + run = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: job.RunID}) + assert.Equal(t, actions_model.StatusCancelled.String(), run.Status.String()) + job = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{ID: task.JobID}) + assert.Equal(t, actions_model.StatusCancelled.String(), job.Status.String()) + assert.NotZero(t, job.Stopped) + task = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: taskID}) + require.Equal(t, actions_model.StatusCancelled.String(), task.Status.String()) + }) + + t.Run("run Running, job and task Success changes to run Cancelled", func(t *testing.T) { + defer unittest.OverrideFixtures("services/actions/TestActions_CancelOrApproveRun")() + require.NoError(t, unittest.PrepareTestDatabase()) + + taskID := int64(710900) + task := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: taskID}) + require.Equal(t, actions_model.StatusSuccess.String(), task.Status.String()) + job := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{ID: task.JobID}) + require.Equal(t, actions_model.StatusSuccess.String(), job.Status.String()) + run := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: job.RunID}) + require.Equal(t, actions_model.StatusRunning.String(), run.Status.String()) + + require.NoError(t, CancelRun(t.Context(), run)) + + run = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: job.RunID}) + assert.Equal(t, actions_model.StatusCancelled.String(), run.Status.String()) + job = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{ID: task.JobID}) + assert.Equal(t, actions_model.StatusSuccess, job.Status) + task = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: taskID}) + require.Equal(t, actions_model.StatusSuccess, task.Status) + }) + + t.Run("run Waiting and job Blocked for Approval changes to run and job Cancelled", func(t *testing.T) { + defer unittest.OverrideFixtures("services/actions/TestActions_CancelOrApproveRun")() + require.NoError(t, unittest.PrepareTestDatabase()) + + jobID := int64(10800) + job := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{ID: jobID}) + require.Equal(t, actions_model.StatusBlocked.String(), job.Status.String()) + run := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: job.RunID}) + require.Equal(t, actions_model.StatusWaiting.String(), run.Status.String()) + require.True(t, run.NeedApproval) + + require.NoError(t, CancelRun(t.Context(), run)) + + run = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: job.RunID}) + assert.Equal(t, actions_model.StatusCancelled.String(), run.Status.String()) + assert.False(t, run.NeedApproval) + job = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{ID: jobID}) + assert.Equal(t, actions_model.StatusCancelled, job.Status) + }) + + t.Run("run Waiting and job Blocked for Approval changes to job Waiting", func(t *testing.T) { + defer unittest.OverrideFixtures("services/actions/TestActions_CancelOrApproveRun")() + require.NoError(t, unittest.PrepareTestDatabase()) + + jobID := int64(10800) + job := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{ID: jobID}) + require.Equal(t, actions_model.StatusBlocked.String(), job.Status.String()) + run := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: job.RunID}) + require.Equal(t, actions_model.StatusWaiting.String(), run.Status.String()) + require.True(t, run.NeedApproval) + + doerID := int64(30) + require.NoError(t, ApproveRun(t.Context(), run, doerID)) + + run = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: job.RunID}) + assert.Equal(t, actions_model.StatusWaiting.String(), run.Status.String()) + assert.False(t, run.NeedApproval) + assert.Equal(t, doerID, run.ApprovedBy) + job = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{ID: jobID}) + assert.Equal(t, actions_model.StatusWaiting, job.Status) + }) +} From e6522c1ecca457b69fb4357af0a6fa6442e3d0b0 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 30 Oct 2025 15:58:51 +0100 Subject: [PATCH 09/16] feat: trust management for runs created from a forked pull request - UpdateTrustedWithPullRequest - cancels or approves runs and keep track of posters that are to always be trusted - GetPullRequestUserIsTrustedWithActions - logic to determine if a user is to be implicitly trusted (e.g. the admin of the instance), explicitly trusted (i.e. it is in the ActionUser table) or not at all. - PullRequestCancel & PullRequestApprove will either cancel or approve all runs of a given pull request. - RevokeTrust is almost the same as PullRequestCancel except it operates as if revoking all pull requests of a given poster, cancelling ongoing jobs. This is expected to be used when blocking a user. - AlwaysTrust is almost the same as PullRequestApprove except it operates as if allways approving all pull requests of a given poster, switching their jobs to waiting. - SetRunTrustForPullRequest helper to set the fields of ActionRun - CleanupActionUser - get rid of unused trust records --- .../action_user.yml | 12 + .../issue.yml | 84 ++++ .../pull_request.yml | 64 +++ .../repo_unit.yml | 6 + services/actions/trust.go | 326 +++++++++++++++ services/actions/trust_test.go | 377 ++++++++++++++++++ 6 files changed, 869 insertions(+) create mode 100644 services/actions/TestActionsTrust_GetPullRequestUserIsTrustedWithActions/action_user.yml create mode 100644 services/actions/TestActionsTrust_GetPullRequestUserIsTrustedWithActions/issue.yml create mode 100644 services/actions/TestActionsTrust_GetPullRequestUserIsTrustedWithActions/pull_request.yml create mode 100644 services/actions/TestActionsTrust_GetPullRequestUserIsTrustedWithActions/repo_unit.yml create mode 100644 services/actions/trust.go create mode 100644 services/actions/trust_test.go diff --git a/services/actions/TestActionsTrust_GetPullRequestUserIsTrustedWithActions/action_user.yml b/services/actions/TestActionsTrust_GetPullRequestUserIsTrustedWithActions/action_user.yml new file mode 100644 index 0000000000..bce18ebf96 --- /dev/null +++ b/services/actions/TestActionsTrust_GetPullRequestUserIsTrustedWithActions/action_user.yml @@ -0,0 +1,12 @@ +- + id: 10 + repo_id: 10 + user_id: 4 + trusted_with_pull_requests: true + last_access: 1683636528 +- + id: 11 + repo_id: 10 + user_id: 29 + trusted_with_pull_requests: true + last_access: 1683636528 diff --git a/services/actions/TestActionsTrust_GetPullRequestUserIsTrustedWithActions/issue.yml b/services/actions/TestActionsTrust_GetPullRequestUserIsTrustedWithActions/issue.yml new file mode 100644 index 0000000000..4eae79b5a4 --- /dev/null +++ b/services/actions/TestActionsTrust_GetPullRequestUserIsTrustedWithActions/issue.yml @@ -0,0 +1,84 @@ +- + id: 81000 + repo_id: 10 + index: 1000 + poster_id: 4 # regular user + original_author_id: 0 + name: pr1000 + content: pull request + milestone_id: 0 + priority: 0 + is_closed: false + is_pull: true + num_comments: 0 + created_unix: 946684820 + updated_unix: 978307180 + is_locked: false + +- + id: 82000 + repo_id: 11 + index: 2000 + poster_id: 2 # regular user + original_author_id: 0 + name: pr2000 + content: pull request + milestone_id: 0 + priority: 0 + is_closed: false + is_pull: true + num_comments: 0 + created_unix: 946684820 + updated_unix: 978307180 + is_locked: false + +- + id: 83000 + repo_id: 10 + index: 3000 + poster_id: 1 # admin + original_author_id: 0 + name: pr3000 + content: pull request + milestone_id: 0 + priority: 0 + is_closed: false + is_pull: true + num_comments: 0 + created_unix: 946684820 + updated_unix: 978307180 + is_locked: false + +- + id: 84000 + repo_id: 10 + index: 4000 + poster_id: 5 # regular user + original_author_id: 0 + name: pr4000 + content: pull request + milestone_id: 0 + priority: 0 + is_closed: false + is_pull: true + num_comments: 0 + created_unix: 946684820 + updated_unix: 978307180 + is_locked: false + +- + id: 85000 + repo_id: 10 + index: 5000 + poster_id: 29 # restricted user + original_author_id: 0 + name: pr5000 + content: pull request + milestone_id: 0 + priority: 0 + is_closed: false + is_pull: true + num_comments: 0 + created_unix: 946684820 + updated_unix: 978307180 + is_locked: false diff --git a/services/actions/TestActionsTrust_GetPullRequestUserIsTrustedWithActions/pull_request.yml b/services/actions/TestActionsTrust_GetPullRequestUserIsTrustedWithActions/pull_request.yml new file mode 100644 index 0000000000..c46dfc063a --- /dev/null +++ b/services/actions/TestActionsTrust_GetPullRequestUserIsTrustedWithActions/pull_request.yml @@ -0,0 +1,64 @@ +- + id: 1000 + type: 0 # pull request + status: 2 # mergeable + issue_id: 81000 + index: 1000 + head_repo_id: 11 + base_repo_id: 10 + head_branch: branch2000 + base_branch: master + merge_base: 0abcb056019adb83 + has_merged: false + +- + id: 2000 + type: 0 # pull request + status: 2 # mergeable + issue_id: 82000 + index: 2000 + head_repo_id: 11 + base_repo_id: 11 + head_branch: branch2000 + base_branch: master + merge_base: 0abcb056019adb83 + has_merged: false + +- + id: 3000 + type: 0 # pull request + status: 2 # mergeable + issue_id: 83000 + index: 3000 + head_repo_id: 11 # different from base_repo + base_repo_id: 10 + head_branch: branch3000 + base_branch: master + merge_base: 0abcb056019adb83 + has_merged: false + +- + id: 4000 + type: 0 # pull request + status: 2 # mergeable + issue_id: 84000 + index: 4000 + head_repo_id: 11 # different from base_repo + base_repo_id: 10 + head_branch: branch4000 + base_branch: master + merge_base: 0abcb056019adb83 + has_merged: false + +- + id: 5000 + type: 0 # pull request + status: 2 # mergeable + issue_id: 85000 + index: 5000 + head_repo_id: 11 # different from base_repo + base_repo_id: 10 + head_branch: branch5000 + base_branch: master + merge_base: 0abcb056019adb83 + has_merged: false diff --git a/services/actions/TestActionsTrust_GetPullRequestUserIsTrustedWithActions/repo_unit.yml b/services/actions/TestActionsTrust_GetPullRequestUserIsTrustedWithActions/repo_unit.yml new file mode 100644 index 0000000000..25d90c17b9 --- /dev/null +++ b/services/actions/TestActionsTrust_GetPullRequestUserIsTrustedWithActions/repo_unit.yml @@ -0,0 +1,6 @@ +- + id: 11500 + repo_id: 10 + type: 10 # TypeActions + config: "{}" + created_unix: 946684810 diff --git a/services/actions/trust.go b/services/actions/trust.go new file mode 100644 index 0000000000..55d97a5756 --- /dev/null +++ b/services/actions/trust.go @@ -0,0 +1,326 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package actions + +import ( + "context" + "fmt" + + actions_model "forgejo.org/models/actions" + issues_model "forgejo.org/models/issues" + access_model "forgejo.org/models/perm/access" + repo_model "forgejo.org/models/repo" + unit_model "forgejo.org/models/unit" + user_model "forgejo.org/models/user" + actions_module "forgejo.org/modules/actions" + "forgejo.org/modules/log" + webhook_module "forgejo.org/modules/webhook" +) + +type TrustUpdate string + +const ( + UserTrustDenied = TrustUpdate("deny") + UserAlwaysTrusted = TrustUpdate("always") + UserTrustedOnce = TrustUpdate("once") + UserTrustRevoked = TrustUpdate("revoke") +) + +func CleanupActionUser(ctx context.Context) error { + return actions_model.RevokeInactiveActionUser(ctx) +} + +func loadPullRequestAttributes(ctx context.Context, pr *issues_model.PullRequest) error { + if err := pr.LoadIssue(ctx); err != nil { + return err + } + + return pr.Issue.LoadRepo(ctx) +} + +func getIssuePoster(ctx context.Context, issue *issues_model.Issue) (*user_model.User, error) { + if issue.Poster != nil { + return issue.Poster, nil + } + if issue.PosterID == 0 { + return nil, nil + } + + poster, err := user_model.GetPossibleUserByID(ctx, issue.PosterID) + if err != nil { + if user_model.IsErrUserNotExist(err) { + return nil, nil + } + return nil, fmt.Errorf("getIssuePoster [%d]: %w", issue.PosterID, err) + } + issue.Poster = poster + return poster, nil +} + +func mustGetIssuePoster(ctx context.Context, issue *issues_model.Issue) (*user_model.User, error) { + poster, err := getIssuePoster(ctx, issue) + if err != nil { + return nil, err + } + if poster == nil { + return nil, user_model.ErrUserNotExist{UID: issue.PosterID} + } + return poster, nil +} + +type useHeadOrBaseCommit int + +const ( + useHeadCommit = 1 << iota + useBaseCommit +) + +func getPullRequestCommitAndApproval(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, event webhook_module.HookEventType) (useHeadOrBaseCommit, actions_model.ApprovalType, error) { + if pr == nil || actions_module.IsDefaultBranchWorkflow(event) || !pr.IsForkPullRequest() { + return useHeadCommit, actions_model.DoesNotNeedApproval, nil + } + + posterTrust, err := GetPullRequestPosterIsTrustedWithActions(ctx, pr) + if err != nil { + return useHeadCommit, actions_model.UndefinedApproval, err + } + + if posterTrust.IsTrusted() { + return useHeadCommit, actions_model.DoesNotNeedApproval, nil + } + + doerTrust, err := getPullRequestUserIsTrustedWithActions(ctx, pr, doer) + if err != nil { + return useHeadCommit, actions_model.UndefinedApproval, err + } + + if doerTrust.IsTrusted() { + if event == webhook_module.HookEventPullRequestSync { + // a synchronized event action (i.e. the doer pushed a commit to the pull request) + // can run from the head + return useHeadCommit, actions_model.DoesNotNeedApproval, nil + } + // other events run from workflows found in the base, not + // from possibly modified workflows found in the head + return useBaseCommit, actions_model.DoesNotNeedApproval, nil + } + // the poster and the doer are not trusted, approval is needed + return useHeadCommit, actions_model.NeedApproval, nil +} + +// cancels or approves runs and keep track of posters that are to always be trusted +func UpdateTrustedWithPullRequest(ctx context.Context, doerID int64, pr *issues_model.PullRequest, trusted TrustUpdate) error { + if err := loadPullRequestAttributes(ctx, pr); err != nil { + return err + } + + switch trusted { + case UserAlwaysTrusted: + poster, err := mustGetIssuePoster(ctx, pr.Issue) + if err != nil { + return err + } + return AlwaysTrust(ctx, doerID, pr.Issue.RepoID, poster.ID) + case UserTrustedOnce: + return pullRequestApprove(ctx, doerID, pr.Issue.RepoID, pr.ID) + case UserTrustRevoked: + poster, err := mustGetIssuePoster(ctx, pr.Issue) + if err != nil { + return err + } + return RevokeTrust(ctx, pr.Issue.RepoID, poster.ID) + case UserTrustDenied: + return pullRequestCancel(ctx, pr.Issue.RepoID, pr.ID) + default: + return fmt.Errorf("UpdateTrustedWithPullRequest: unknown trust %v", trusted) + } +} + +func setRunTrustForPullRequest(ctx context.Context, run *actions_model.ActionRun, pr *issues_model.PullRequest, needApproval actions_model.ApprovalType) error { + if pr == nil { + return nil + } + + if err := loadPullRequestAttributes(ctx, pr); err != nil { + return err + } + + run.IsForkPullRequest = pr.IsForkPullRequest() + run.PullRequestPosterID = pr.Issue.PosterID + run.PullRequestID = pr.ID + run.NeedApproval = bool(needApproval) + + return nil +} + +type UserTrust string + +const ( + UserTrustIsNotRelevant = UserTrust("irrelevant") + UserIsNotTrustedWithActions = UserTrust("no") + UserIsExplicitlyTrustedWithActions = UserTrust("explicitly") + UserIsImplicitlyTrustedWithActions = UserTrust("implicitly") +) + +func (t UserTrust) IsTrusted() bool { + return t != UserIsNotTrustedWithActions +} + +func GetPullRequestPosterIsTrustedWithActions(ctx context.Context, pr *issues_model.PullRequest) (UserTrust, error) { + if err := loadPullRequestAttributes(ctx, pr); err != nil { + return "", err + } + poster, err := mustGetIssuePoster(ctx, pr.Issue) + if err != nil { + return UserIsNotTrustedWithActions, err + } + + return getPullRequestUserIsTrustedWithActions(ctx, pr, poster) +} + +func getPullRequestUserIsTrustedWithActions(ctx context.Context, pr *issues_model.PullRequest, user *user_model.User) (UserTrust, error) { + if err := loadPullRequestAttributes(ctx, pr); err != nil { + return "", err + } + + return userIsTrustedWithPullRequest(ctx, pr, user) +} + +func userIsTrustedWithPullRequest(ctx context.Context, pr *issues_model.PullRequest, user *user_model.User) (UserTrust, error) { + implicitlyTrusted, err := userIsImplicitlyTrustedWithPullRequest(ctx, pr, user) + if err != nil { + return "", err + } + if implicitlyTrusted { + log.Trace("%s is implicitly trusted to run actions in repository %s", user, pr.Issue.Repo) + return UserIsImplicitlyTrustedWithActions, nil + } + + explicitlyTrusted, err := userIsExplicitlyTrustedWithPullRequest(ctx, pr, user) + if err != nil { + return "", err + } + if explicitlyTrusted { + log.Trace("%s is explicitly trusted to run actions in repository %s", user, pr.Issue.Repo) + return UserIsExplicitlyTrustedWithActions, nil + } + + log.Trace("%s is not trusted to run actions in repository %s", user, pr.Issue.Repo) + return UserIsNotTrustedWithActions, nil +} + +func userIsImplicitlyTrustedWithPullRequest(ctx context.Context, pr *issues_model.PullRequest, user *user_model.User) (bool, error) { + // users that are trusted to create a pull request that is not from a fork + // are also implicitly trusted to run workflows + if !pr.IsForkPullRequest() { + log.Trace("a pull request that is not from a fork nor AGit is implicitly trusted to run actions") + return true, nil + } + + return userCanWriteActionsOnRepo(ctx, pr.Issue.Repo, user) +} + +func userCanWriteActionsOnRepo(ctx context.Context, repo *repo_model.Repository, user *user_model.User) (bool, error) { + // users with write permission to the actions unit are trusted to + // run actions + permission, err := access_model.GetUserRepoPermission(ctx, repo, user) + if err != nil { + return false, err + } + if permission.CanWrite(unit_model.TypeActions) { + log.Trace("%s has write permissions to the Action unit on %s", user, repo) + return true, nil + } + + return false, nil +} + +func userIsExplicitlyTrustedWithPullRequest(ctx context.Context, pr *issues_model.PullRequest, user *user_model.User) (bool, error) { + // there is no need to check if the user is blocked because it is not + // allowed to create a pull request + if user.IsRestricted { + log.Trace("%v is restricted and cannot be trusted with pull requests", user) + return false, nil + } + + actionUser, err := actions_model.GetActionUserByUserIDAndRepoIDAndUpdateAccess(ctx, user.ID, pr.Issue.Repo.ID) + if err != nil { + log.Trace("%v is not explicitly trusted with pull requests on repository %v", user, pr.Issue.Repo) + if actions_model.IsErrUserNotExist(err) { + return false, nil + } + return false, err + } + + log.Trace("%v is explicitly trusted with pull requests on repository %v", user, pr.Issue.Repo) + return actionUser.TrustedWithPullRequests, nil +} + +func RevokeTrust(ctx context.Context, repoID, posterID int64) error { + if err := actions_model.DeleteActionUserByUserIDAndRepoID(ctx, posterID, repoID); err != nil { + return err + } + + runs, err := actions_model.GetRunsNotDoneByRepoIDAndPullRequestPosterID(ctx, repoID, posterID) + if err != nil { + return err + } + + for _, run := range runs { + if err := CancelRun(ctx, run); err != nil { + return err + } + } + return nil +} + +func AlwaysTrust(ctx context.Context, doerID, repoID, posterID int64) error { + if err := actions_model.InsertActionUser(ctx, &actions_model.ActionUser{ + UserID: posterID, + RepoID: repoID, + TrustedWithPullRequests: true, + }); err != nil { + return err + } + + runs, err := actions_model.GetRunsNotDoneByRepoIDAndPullRequestPosterID(ctx, repoID, posterID) + if err != nil { + return err + } + + for _, run := range runs { + if err := ApproveRun(ctx, run, doerID); err != nil { + return err + } + } + return nil +} + +func pullRequestCancel(ctx context.Context, repoID, pullRequestID int64) error { + runs, err := actions_model.GetRunsNotDoneByRepoIDAndPullRequestID(ctx, repoID, pullRequestID) + if err != nil { + return err + } + + for _, run := range runs { + if err := CancelRun(ctx, run); err != nil { + return err + } + } + return nil +} + +func pullRequestApprove(ctx context.Context, doerID, repoID, pullRequestID int64) error { + runs, err := actions_model.GetRunsThatNeedApprovalByRepoIDAndPullRequestID(ctx, repoID, pullRequestID) + if err != nil { + return err + } + + for _, run := range runs { + if err := ApproveRun(ctx, run, doerID); err != nil { + return err + } + } + return nil +} diff --git a/services/actions/trust_test.go b/services/actions/trust_test.go new file mode 100644 index 0000000000..d5aa08e35f --- /dev/null +++ b/services/actions/trust_test.go @@ -0,0 +1,377 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package actions + +import ( + "testing" + + actions_model "forgejo.org/models/actions" + issues_model "forgejo.org/models/issues" + "forgejo.org/models/unittest" + user_model "forgejo.org/models/user" + actions_module "forgejo.org/modules/actions" + webhook_module "forgejo.org/modules/webhook" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestActionsTrust_ChangeStatus(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + repoID := int64(10) + pullRequestPosterID := int64(30) + + runDone := &actions_model.ActionRun{ + RepoID: repoID, + PullRequestPosterID: pullRequestPosterID, + Status: actions_model.StatusSuccess, + } + require.NoError(t, actions_model.InsertRun(t.Context(), runDone, nil)) + + runNotByPoster := &actions_model.ActionRun{ + RepoID: repoID, + PullRequestPosterID: 43243, + Status: actions_model.StatusRunning, + } + require.NoError(t, actions_model.InsertRun(t.Context(), runNotByPoster, nil)) + + runNotInTheSameRepository := &actions_model.ActionRun{ + RepoID: 5, + PullRequestPosterID: pullRequestPosterID, + Status: actions_model.StatusSuccess, + } + require.NoError(t, actions_model.InsertRun(t.Context(), runNotInTheSameRepository, nil)) + + t.Run("RevokeTrust", func(t *testing.T) { + singleWorkflows, err := actions_module.JobParser([]byte(` +jobs: + job: + runs-on: docker + steps: + - run: echo OK +`)) + require.NoError(t, err) + require.Len(t, singleWorkflows, 1) + runNotDone := &actions_model.ActionRun{ + TriggerUserID: 2, + RepoID: repoID, + Status: actions_model.StatusWaiting, + PullRequestPosterID: pullRequestPosterID, + } + require.NoError(t, actions_model.InsertRun(t.Context(), runNotDone, singleWorkflows)) + require.NoError(t, actions_model.InsertActionUser(t.Context(), &actions_model.ActionUser{ + UserID: pullRequestPosterID, + RepoID: repoID, + TrustedWithPullRequests: true, + })) + + previousCancelledCount := unittest.GetCount(t, &actions_model.ActionRun{Status: actions_model.StatusCancelled}) + _, err = actions_model.GetActionUserByUserIDAndRepoIDAndUpdateAccess(t.Context(), pullRequestPosterID, repoID) + require.NoError(t, err) + + require.NoError(t, RevokeTrust(t.Context(), repoID, pullRequestPosterID)) + + _, err = actions_model.GetActionUserByUserIDAndRepoIDAndUpdateAccess(t.Context(), pullRequestPosterID, repoID) + assert.True(t, actions_model.IsErrUserNotExist(err)) + currentCancelledCount := unittest.GetCount(t, &actions_model.ActionRun{Status: actions_model.StatusCancelled}) + assert.Equal(t, previousCancelledCount+1, currentCancelledCount) + run := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: runNotDone.ID}) + assert.Equal(t, actions_model.StatusCancelled.String(), run.Status.String()) + }) + + createPullRequestRun := func(t *testing.T, pullRequestID, repoID int64) *actions_model.ActionRun { + t.Helper() + singleWorkflows, err := actions_module.JobParser([]byte(` +jobs: + job: + runs-on: docker + steps: + - run: echo OK +`)) + require.NoError(t, err) + require.Len(t, singleWorkflows, 1) + runNotApproved := &actions_model.ActionRun{ + TriggerUserID: 2, + RepoID: repoID, + Status: actions_model.StatusWaiting, + NeedApproval: true, + PullRequestID: pullRequestID, + PullRequestPosterID: pullRequestPosterID, + } + require.NoError(t, actions_model.InsertRun(t.Context(), runNotApproved, singleWorkflows)) + return runNotApproved + } + + t.Run("PullRequestCancel", func(t *testing.T) { + pullRequestID := int64(485) + runNotApproved := createPullRequestRun(t, pullRequestID, repoID) + + previousCancelledCount := unittest.GetCount(t, &actions_model.ActionRun{Status: actions_model.StatusCancelled}) + + require.NoError(t, pullRequestCancel(t.Context(), repoID, pullRequestID)) + + currentCancelledCount := unittest.GetCount(t, &actions_model.ActionRun{Status: actions_model.StatusCancelled}) + assert.Equal(t, previousCancelledCount+1, currentCancelledCount) + run := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: runNotApproved.ID}) + assert.Equal(t, actions_model.StatusCancelled.String(), run.Status.String()) + }) + + t.Run("UpdateTrustedWithPullRequest deny", func(t *testing.T) { + pullRequestID := int64(485) + runNotApproved := createPullRequestRun(t, pullRequestID, repoID) + + previousCancelledCount := unittest.GetCount(t, &actions_model.ActionRun{Status: actions_model.StatusCancelled}) + + require.NoError(t, UpdateTrustedWithPullRequest(t.Context(), 0, &issues_model.PullRequest{ + ID: pullRequestID, + Issue: &issues_model.Issue{ + RepoID: repoID, + }, + }, UserTrustDenied)) + + currentCancelledCount := unittest.GetCount(t, &actions_model.ActionRun{Status: actions_model.StatusCancelled}) + assert.Equal(t, previousCancelledCount+1, currentCancelledCount) + run := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: runNotApproved.ID}) + assert.Equal(t, actions_model.StatusCancelled.String(), run.Status.String()) + }) + + t.Run("PullRequestApprove", func(t *testing.T) { + pullRequestID := int64(534) + runNotApproved := createPullRequestRun(t, pullRequestID, repoID) + + previousWaitingCount := unittest.GetCount(t, &actions_model.ActionRunJob{Status: actions_model.StatusWaiting}) + + doerID := int64(84322) + require.NoError(t, pullRequestApprove(t.Context(), doerID, repoID, pullRequestID)) + + currentWaitingCount := unittest.GetCount(t, &actions_model.ActionRunJob{Status: actions_model.StatusWaiting}) + assert.Equal(t, previousWaitingCount+1, currentWaitingCount) + run := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: runNotApproved.ID}) + assert.Equal(t, actions_model.StatusWaiting.String(), run.Status.String()) + assert.Equal(t, doerID, run.ApprovedBy) + assert.False(t, run.NeedApproval) + }) + + t.Run("UpdateTrustedWithPullRequest once", func(t *testing.T) { + pullRequestID := int64(534) + runNotApproved := createPullRequestRun(t, pullRequestID, repoID) + + previousWaitingCount := unittest.GetCount(t, &actions_model.ActionRunJob{Status: actions_model.StatusWaiting}) + + doerID := int64(84322) + require.NoError(t, UpdateTrustedWithPullRequest(t.Context(), doerID, &issues_model.PullRequest{ + ID: pullRequestID, + Issue: &issues_model.Issue{ + RepoID: repoID, + }, + }, UserTrustedOnce)) + + currentWaitingCount := unittest.GetCount(t, &actions_model.ActionRunJob{Status: actions_model.StatusWaiting}) + assert.Equal(t, previousWaitingCount+1, currentWaitingCount) + run := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: runNotApproved.ID}) + assert.Equal(t, actions_model.StatusWaiting.String(), run.Status.String()) + assert.Equal(t, doerID, run.ApprovedBy) + assert.False(t, run.NeedApproval) + }) + + t.Run("UpdateTrustedWithPullRequest always", func(t *testing.T) { + pullRequestIDs := []int64{534, 645} + var runsNotApproved []*actions_model.ActionRun + for _, pullRequestID := range pullRequestIDs { + runsNotApproved = append(runsNotApproved, createPullRequestRun(t, pullRequestID, repoID)) + } + + previousWaitingCount := unittest.GetCount(t, &actions_model.ActionRunJob{Status: actions_model.StatusWaiting}) + + doerID := int64(84322) + require.NoError(t, UpdateTrustedWithPullRequest(t.Context(), doerID, &issues_model.PullRequest{ + ID: pullRequestIDs[0], + Issue: &issues_model.Issue{ + RepoID: repoID, + PosterID: pullRequestPosterID, + }, + }, UserAlwaysTrusted)) + + currentWaitingCount := unittest.GetCount(t, &actions_model.ActionRunJob{Status: actions_model.StatusWaiting}) + assert.Equal(t, previousWaitingCount+len(pullRequestIDs), currentWaitingCount) + + for _, run := range runsNotApproved { + run := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: run.ID}) + assert.Equal(t, actions_model.StatusWaiting.String(), run.Status.String()) + assert.Equal(t, doerID, run.ApprovedBy) + assert.False(t, run.NeedApproval) + } + }) + + t.Run("UpdateTrustedWithPullRequest revoke", func(t *testing.T) { + pullRequestIDs := []int64{748, 953} + var runsNotApproved []*actions_model.ActionRun + for _, pullRequestID := range pullRequestIDs { + runsNotApproved = append(runsNotApproved, createPullRequestRun(t, pullRequestID, repoID)) + } + + doerID := int64(84322) + require.NoError(t, UpdateTrustedWithPullRequest(t.Context(), doerID, &issues_model.PullRequest{ + ID: pullRequestIDs[0], + Issue: &issues_model.Issue{ + RepoID: repoID, + PosterID: pullRequestPosterID, + }, + }, UserTrustRevoked)) + + for _, run := range runsNotApproved { + run := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: run.ID}) + assert.Equal(t, actions_model.StatusCancelled.String(), run.Status.String()) + assert.False(t, run.NeedApproval) + } + }) +} + +func TestActionsTrust_GetPullRequestUserIsTrustedWithActions(t *testing.T) { + defer unittest.OverrideFixtures("services/actions/TestActionsTrust_GetPullRequestUserIsTrustedWithActions")() + require.NoError(t, unittest.PrepareTestDatabase()) + + t.Run("implicitly trusted because the pull request is not from a fork", func(t *testing.T) { + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2000}) + trust, err := GetPullRequestPosterIsTrustedWithActions(t.Context(), pr) + require.NoError(t, err) + require.False(t, pr.IsForkPullRequest()) + assert.Equal(t, UserIsImplicitlyTrustedWithActions, trust) + }) + + t.Run("implicitly trusted on a forked pull request when the poster is admin", func(t *testing.T) { + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 3000}) + trust, err := GetPullRequestPosterIsTrustedWithActions(t.Context(), pr) + require.NoError(t, err) + require.True(t, pr.IsForkPullRequest()) + require.True(t, pr.Issue.Poster.IsAdmin) + assert.Equal(t, UserIsImplicitlyTrustedWithActions, trust) + }) + + t.Run("explicitly trusted on a forked pull request when the poster was permanently approved", func(t *testing.T) { + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1000}) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) // regular user + trust, err := GetPullRequestPosterIsTrustedWithActions(t.Context(), pr) + require.NoError(t, err) + require.True(t, pr.IsForkPullRequest()) + _, err = actions_model.GetActionUserByUserIDAndRepoIDAndUpdateAccess(t.Context(), user.ID, pr.Issue.RepoID) + require.NoError(t, err) + assert.Equal(t, UserIsExplicitlyTrustedWithActions, trust) + }) + + t.Run("not trusted because on a forked pull request when the user has has no privileges", func(t *testing.T) { + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 4000}) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) // regular user + trust, err := GetPullRequestPosterIsTrustedWithActions(t.Context(), pr) + require.NoError(t, err) + assert.Equal(t, user.ID, pr.Issue.PosterID) + require.True(t, pr.IsForkPullRequest()) + assert.Equal(t, UserIsNotTrustedWithActions, trust) + }) + + t.Run("not trusted on a forked pull request because the user is restricted", func(t *testing.T) { + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 5000}) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 29}) // restricted user + trust, err := getPullRequestUserIsTrustedWithActions(t.Context(), pr, user) + require.NoError(t, err) + assert.Equal(t, user.ID, pr.Issue.PosterID) + require.True(t, pr.IsForkPullRequest()) + _, err = actions_model.GetActionUserByUserIDAndRepoIDAndUpdateAccess(t.Context(), user.ID, pr.Issue.RepoID) + require.NoError(t, err) + require.True(t, user.IsRestricted) + assert.Equal(t, UserIsNotTrustedWithActions, trust) + }) + + t.Run("approval not needed because the pr is not from a fork", func(t *testing.T) { + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2000}) + useCommit, approval, err := getPullRequestCommitAndApproval(t.Context(), pr, nil, webhook_module.HookEventPullRequest) + require.NoError(t, err) + assert.Equal(t, actions_model.DoesNotNeedApproval, approval) + assert.EqualValues(t, useHeadCommit, useCommit) + }) + + t.Run("approval not needed because the event is known to run out of the default branch", func(t *testing.T) { + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 3000}) + useCommit, approval, err := getPullRequestCommitAndApproval(t.Context(), pr, nil, webhook_module.HookEventPullRequestComment) + require.NoError(t, err) + assert.Equal(t, actions_model.DoesNotNeedApproval, approval) + assert.EqualValues(t, useHeadCommit, useCommit) + }) + + t.Run("approval not needed because it is not a pr", func(t *testing.T) { + useCommit, approval, err := getPullRequestCommitAndApproval(t.Context(), nil, nil, webhook_module.HookEventPullRequestComment) + require.NoError(t, err) + assert.Equal(t, actions_model.DoesNotNeedApproval, approval) + assert.EqualValues(t, useHeadCommit, useCommit) + }) + + t.Run("approval not needed for a forked pr because the poster is trusted", func(t *testing.T) { + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 3000}) + useCommit, approval, err := getPullRequestCommitAndApproval(t.Context(), pr, nil, webhook_module.HookEventPullRequestSync) + require.NoError(t, err) + require.True(t, pr.Issue.Poster.IsAdmin) + assert.Equal(t, actions_model.DoesNotNeedApproval, approval) + assert.EqualValues(t, useHeadCommit, useCommit) + }) + + t.Run("approval needed for a forked pr because the poster and the doer are not trusted", func(t *testing.T) { + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 4000}) + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) // regular user + useCommit, approval, err := getPullRequestCommitAndApproval(t.Context(), pr, doer, webhook_module.HookEventPullRequestSync) + require.NoError(t, err) + posterTrust, err := GetPullRequestPosterIsTrustedWithActions(t.Context(), pr) + require.NoError(t, err) + require.Equal(t, UserIsNotTrustedWithActions, posterTrust) + doerTrust, err := getPullRequestUserIsTrustedWithActions(t.Context(), pr, doer) + require.NoError(t, err) + require.Equal(t, UserIsNotTrustedWithActions, doerTrust) + assert.Equal(t, actions_model.NeedApproval, approval) + assert.EqualValues(t, useHeadCommit, useCommit) + }) + + t.Run("approval not needed for a forked pr because the doer is trusted and runs from the base", func(t *testing.T) { + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 4000}) + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) // admin + useCommit, approval, err := getPullRequestCommitAndApproval(t.Context(), pr, doer, webhook_module.HookEventPullRequestLabel) + require.NoError(t, err) + posterTrust, err := GetPullRequestPosterIsTrustedWithActions(t.Context(), pr) + require.NoError(t, err) + require.Equal(t, UserIsNotTrustedWithActions, posterTrust) + doerTrust, err := getPullRequestUserIsTrustedWithActions(t.Context(), pr, doer) + require.NoError(t, err) + require.Equal(t, UserIsImplicitlyTrustedWithActions, doerTrust) + assert.Equal(t, actions_model.DoesNotNeedApproval, approval) + assert.EqualValues(t, useBaseCommit, useCommit) + }) + + t.Run("approval not needed for a forked pr because the doer is trusted and pushed new commits", func(t *testing.T) { + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 4000}) + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) // admin + useCommit, approval, err := getPullRequestCommitAndApproval(t.Context(), pr, doer, webhook_module.HookEventPullRequestSync) + require.NoError(t, err) + posterTrust, err := GetPullRequestPosterIsTrustedWithActions(t.Context(), pr) + require.NoError(t, err) + require.Equal(t, UserIsNotTrustedWithActions, posterTrust) + doerTrust, err := getPullRequestUserIsTrustedWithActions(t.Context(), pr, doer) + require.NoError(t, err) + require.Equal(t, UserIsImplicitlyTrustedWithActions, doerTrust) + assert.Equal(t, actions_model.DoesNotNeedApproval, approval) + assert.EqualValues(t, useHeadCommit, useCommit) + }) + + t.Run("run for a pull request is set with info related to trust", func(t *testing.T) { + run := &actions_model.ActionRun{ + IsForkPullRequest: true, + } + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 5000}) + needApproval := actions_model.NeedApproval + require.NoError(t, setRunTrustForPullRequest(t.Context(), run, nil, needApproval)) + require.NoError(t, setRunTrustForPullRequest(t.Context(), run, pr, needApproval)) + assert.True(t, run.NeedApproval) + assert.True(t, run.IsForkPullRequest) + assert.Equal(t, pr.Issue.PosterID, run.PullRequestPosterID) + assert.Equal(t, pr.ID, run.PullRequestID) + }) +} From 6a99709a1cf1aff48daab2ba15cb478ca4e881f1 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 6 Nov 2025 06:59:57 +0100 Subject: [PATCH 10/16] chore(refactor): detectWorkflow process pull_request_target first Collecting pull_request_target workflows before the others changes nothing. They will be first in the list but there is no guarantee or need for ordering. This is in preparation of a future commit that needs to know the base commit before detecting workflows that are not pull_request_target. --- services/actions/notifier_helper.go | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/services/actions/notifier_helper.go b/services/actions/notifier_helper.go index 6a9f7534b8..ed6139e8cb 100644 --- a/services/actions/notifier_helper.go +++ b/services/actions/notifier_helper.go @@ -242,17 +242,6 @@ func detectWorkflows(ctx context.Context, input *notifyInput, gitRepo *git.Repos len(schedules), ) - for _, wf := range workflows { - if actionsConfig.IsWorkflowDisabled(wf.EntryName) { - log.Trace("repo %s has disable workflows %s", input.Repo.RepoPath(), wf.EntryName) - continue - } - - if wf.TriggerEvent.Name != actions_module.GithubEventPullRequestTarget { - detectedWorkflows = append(detectedWorkflows, wf) - } - } - if input.PullRequest != nil && !actions_module.IsDefaultBranchWorkflow(input.Event) { // detect pull_request_target workflows baseRef := git.BranchPrefix + input.PullRequest.BaseBranch @@ -280,6 +269,18 @@ func detectWorkflows(ctx context.Context, input *notifyInput, gitRepo *git.Repos } } } + + for _, wf := range workflows { + if actionsConfig.IsWorkflowDisabled(wf.EntryName) { + log.Trace("repo %s has disable workflows %s", input.Repo.RepoPath(), wf.EntryName) + continue + } + + if wf.TriggerEvent.Name != actions_module.GithubEventPullRequestTarget { + detectedWorkflows = append(detectedWorkflows, wf) + } + } + return detectedWorkflows, schedules, nil } From 3fa7ceeb76d33dbeb72ab6de178a47863fba1c90 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 30 Oct 2025 16:09:18 +0100 Subject: [PATCH 11/16] chore(refactor): replace ifNeedApproval with trust management What previously handled by ifNeedApproval is replaced with two calls implemented in trust.go: - getPullRequestCommitAndApproval when workflows are collected and before runs are generated from them, figure out if - they need approval - they should run from the base or the head - setRunTrustForPullRequest when a pull request run is created from a detected workflow, set the information it will need for trust management --- modules/actions/workflows.go | 2 + .../TestActionsNotifier_IsTrusted/issue.yml | 16 +++ .../pull_request.yml | 12 ++ .../repo_unit.yml | 6 + services/actions/notifier_helper.go | 104 +++++------------- services/actions/notifier_helper_test.go | 24 ++++ 6 files changed, 90 insertions(+), 74 deletions(-) create mode 100644 services/actions/TestActionsNotifier_IsTrusted/issue.yml create mode 100644 services/actions/TestActionsNotifier_IsTrusted/pull_request.yml create mode 100644 services/actions/TestActionsNotifier_IsTrusted/repo_unit.yml diff --git a/modules/actions/workflows.go b/modules/actions/workflows.go index f50e5f8289..211761e6c1 100644 --- a/modules/actions/workflows.go +++ b/modules/actions/workflows.go @@ -8,6 +8,7 @@ import ( "io" "strings" + actions_model "forgejo.org/models/actions" "forgejo.org/modules/git" "forgejo.org/modules/log" api "forgejo.org/modules/structs" @@ -25,6 +26,7 @@ type DetectedWorkflow struct { TriggerEvent *jobparser.Event Content []byte EventDetectionError error + NeedApproval actions_model.ApprovalType } func init() { diff --git a/services/actions/TestActionsNotifier_IsTrusted/issue.yml b/services/actions/TestActionsNotifier_IsTrusted/issue.yml new file mode 100644 index 0000000000..11d0032926 --- /dev/null +++ b/services/actions/TestActionsNotifier_IsTrusted/issue.yml @@ -0,0 +1,16 @@ +- + id: 8000 + repo_id: 10 + index: 1000 + poster_id: 1 # admin + original_author_id: 0 + name: pr8000 + content: pull request 8000 + milestone_id: 0 + priority: 0 + is_closed: false + is_pull: true + num_comments: 0 + created_unix: 946684820 + updated_unix: 978307180 + is_locked: false diff --git a/services/actions/TestActionsNotifier_IsTrusted/pull_request.yml b/services/actions/TestActionsNotifier_IsTrusted/pull_request.yml new file mode 100644 index 0000000000..6187a7ecc2 --- /dev/null +++ b/services/actions/TestActionsNotifier_IsTrusted/pull_request.yml @@ -0,0 +1,12 @@ +- + id: 3000 + type: 0 # pull request + status: 2 # mergeable + issue_id: 8000 + index: 1000 + head_repo_id: 11 + base_repo_id: 10 + head_branch: branch3000 + base_branch: master + merge_base: 0abcb056019adb83 + has_merged: false diff --git a/services/actions/TestActionsNotifier_IsTrusted/repo_unit.yml b/services/actions/TestActionsNotifier_IsTrusted/repo_unit.yml new file mode 100644 index 0000000000..25d90c17b9 --- /dev/null +++ b/services/actions/TestActionsNotifier_IsTrusted/repo_unit.yml @@ -0,0 +1,6 @@ +- + id: 11500 + repo_id: 10 + type: 10 # TypeActions + config: "{}" + created_unix: 946684810 diff --git a/services/actions/notifier_helper.go b/services/actions/notifier_helper.go index ed6139e8cb..7f46784d40 100644 --- a/services/actions/notifier_helper.go +++ b/services/actions/notifier_helper.go @@ -268,6 +268,19 @@ func detectWorkflows(ctx context.Context, input *notifyInput, gitRepo *git.Repos } } } + + useHeadOrBaseCommit, pullRequestNeedApproval, err := getPullRequestCommitAndApproval(ctx, input.PullRequest, input.Doer, input.Event) + if err != nil { + return nil, nil, fmt.Errorf("getPullRequestTrust: %w", err) + } + + if useHeadOrBaseCommit == useBaseCommit { + workflows = baseWorkflows + } else if pullRequestNeedApproval { + for _, wf := range workflows { + wf.NeedApproval = pullRequestNeedApproval + } + } } for _, wf := range workflows { @@ -342,35 +355,25 @@ func handleWorkflows( return fmt.Errorf("json.Marshal: %w", err) } - isForkPullRequest := false - if pr := input.PullRequest; pr != nil && !actions_module.IsDefaultBranchWorkflow(input.Event) { - switch pr.Flow { - case issues_model.PullRequestFlowGithub: - isForkPullRequest = pr.IsFromFork() - case issues_model.PullRequestFlowAGit: - // There is no fork concept in agit flow, anyone with read permission can push refs/for// to the repo. - // So we can treat it as a fork pull request because it may be from an untrusted user - isForkPullRequest = true - default: - // unknown flow, assume it's a fork pull request to be safe - isForkPullRequest = true - } - } - for _, dwf := range detectedWorkflows { run := &actions_model.ActionRun{ - Title: strings.SplitN(commit.CommitMessage, "\n", 2)[0], - RepoID: input.Repo.ID, - OwnerID: input.Repo.OwnerID, - WorkflowID: dwf.EntryName, - TriggerUserID: input.Doer.ID, - Ref: ref, - CommitSHA: commit.ID.String(), - IsForkPullRequest: isForkPullRequest, - Event: input.Event, - EventPayload: string(p), - TriggerEvent: dwf.TriggerEvent.Name, - Status: actions_model.StatusWaiting, + Title: strings.SplitN(commit.CommitMessage, "\n", 2)[0], + RepoID: input.Repo.ID, + OwnerID: input.Repo.OwnerID, + WorkflowID: dwf.EntryName, + TriggerUserID: input.Doer.ID, + Ref: ref, + CommitSHA: commit.ID.String(), + Event: input.Event, + EventPayload: string(p), + TriggerEvent: dwf.TriggerEvent.Name, + Status: actions_model.StatusWaiting, + } + + if !actions_module.IsDefaultBranchWorkflow(input.Event) { + if err := setRunTrustForPullRequest(ctx, run, input.PullRequest, dwf.NeedApproval); err != nil { + return fmt.Errorf("setTrustForPullRequest: %w", err) + } } workflow, err := model.ReadWorkflow(bytes.NewReader(dwf.Content), false) @@ -384,14 +387,6 @@ func handleWorkflows( } run.NotifyEmail = notifications - need, err := ifNeedApproval(ctx, run, input.Repo, input.Doer) - if err != nil { - log.Error("check if need approval for repo %d with user %d: %v", input.Repo.ID, input.Doer.ID, err) - continue - } - - run.NeedApproval = need - if err := run.LoadAttributes(ctx); err != nil { log.Error("LoadAttributes: %v", err) continue @@ -500,45 +495,6 @@ func notifyPackage(ctx context.Context, sender *user_model.User, pd *packages_mo Notify(ctx) } -func ifNeedApproval(ctx context.Context, run *actions_model.ActionRun, repo *repo_model.Repository, user *user_model.User) (bool, error) { - // 1. don't need approval if it's not a fork PR - // 2. don't need approval if the event is `pull_request_target` since the workflow will run in the context of base branch - // see https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks#about-workflow-runs-from-public-forks - if !run.IsForkPullRequest || run.TriggerEvent == actions_module.GithubEventPullRequestTarget { - return false, nil - } - - // always need approval if the user is restricted - if user.IsRestricted { - log.Trace("need approval because user %d is restricted", user.ID) - return true, nil - } - - // don't need approval if the user can write - if perm, err := access_model.GetUserRepoPermission(ctx, repo, user); err != nil { - return false, fmt.Errorf("GetUserRepoPermission: %w", err) - } else if perm.CanWrite(unit_model.TypeActions) { - log.Trace("do not need approval because user %d can write", user.ID) - return false, nil - } - - // don't need approval if the user has been approved before - if count, err := db.Count[actions_model.ActionRun](ctx, actions_model.FindRunOptions{ - RepoID: repo.ID, - TriggerUserID: user.ID, - Approved: true, - }); err != nil { - return false, fmt.Errorf("CountRuns: %w", err) - } else if count > 0 { - log.Trace("do not need approval because user %d has been approved before", user.ID) - return false, nil - } - - // otherwise, need approval - log.Trace("need approval because it's the first time user %d triggered actions", user.ID) - return true, nil -} - func handleSchedules( ctx context.Context, detectedWorkflows []*actions_module.DetectedWorkflow, diff --git a/services/actions/notifier_helper_test.go b/services/actions/notifier_helper_test.go index 5080585c7c..88145760d5 100644 --- a/services/actions/notifier_helper_test.go +++ b/services/actions/notifier_helper_test.go @@ -241,3 +241,27 @@ func TestActionsNotifier_PreExecutionEventDetectionError(t *testing.T) { assert.Equal(t, actions_model.StatusFailure, createdRun.Status) assert.Equal(t, "actions.workflow.event_detection_error%!(EXTRA *errors.errorString=nothing is not a valid event)", createdRun.PreExecutionError) } + +func TestActionsNotifier_handleWorkflows_setRunTrustForPullRequest(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) + // poster is not trusted implicitly + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 3}) + + testActionsNotifierPullRequest(t, repo, pr, &actions_module.DetectedWorkflow{ + NeedApproval: true, + }, webhook_module.HookEventPullRequest) + + runs, err := db.Find[actions_model.ActionRun](db.DefaultContext, actions_model.FindRunOptions{ + RepoID: repo.ID, + }) + require.NoError(t, err) + require.Len(t, runs, 1) + + run := runs[0] + assert.True(t, run.IsForkPullRequest) + assert.Equal(t, pr.Issue.PosterID, run.PullRequestPosterID) + assert.Equal(t, pr.ID, run.PullRequestID) + assert.True(t, run.NeedApproval) +} From 917d0e9fa024a27097005d447b73cb038fda179a Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 30 Oct 2025 16:11:08 +0100 Subject: [PATCH 12/16] feat: cancel all pull requests runs when blocking a user --- services/user/block.go | 8 ++++++++ services/user/block_test.go | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/services/user/block.go b/services/user/block.go index 6be8dc5f70..6182df7ca7 100644 --- a/services/user/block.go +++ b/services/user/block.go @@ -9,6 +9,7 @@ import ( "forgejo.org/models/db" repo_model "forgejo.org/models/repo" user_model "forgejo.org/models/user" + actions_service "forgejo.org/services/actions" "xorm.io/builder" ) @@ -91,5 +92,12 @@ func BlockUser(ctx context.Context, userID, blockID int64) error { return err } + err = db.Iterate(ctx, builder.Eq{"owner_id": userID}, func(ctx context.Context, repo *repo_model.Repository) error { + return actions_service.RevokeTrust(ctx, repo.ID, blockID) + }) + if err != nil { + return err + } + return committer.Commit() } diff --git a/services/user/block_test.go b/services/user/block_test.go index a2ba5d71a7..b9f23a7cda 100644 --- a/services/user/block_test.go +++ b/services/user/block_test.go @@ -7,11 +7,13 @@ import ( "testing" model "forgejo.org/models" + actions_model "forgejo.org/models/actions" "forgejo.org/models/db" issues_model "forgejo.org/models/issues" repo_model "forgejo.org/models/repo" "forgejo.org/models/unittest" user_model "forgejo.org/models/user" + actions_module "forgejo.org/modules/actions" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -110,4 +112,37 @@ func TestBlockUser(t *testing.T) { _, err = issues_model.ChangeIssueStatus(db.DefaultContext, issue, blockedUser, false) require.Error(t, err) }) + + t.Run("Pull requests actions are cancelled", func(t *testing.T) { + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2, OwnerID: doer.ID}) + blockedUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + defer user_model.UnblockUser(db.DefaultContext, doer.ID, blockedUser.ID) + + pullRequestPosterID := blockedUser.ID + singleWorkflows, err := actions_module.JobParser([]byte(` +jobs: + job: + runs-on: docker + steps: + - run: echo OK +`)) + require.NoError(t, err) + require.Len(t, singleWorkflows, 1) + runWaiting := &actions_model.ActionRun{ + TriggerUserID: 2, + RepoID: repo.ID, + Status: actions_model.StatusWaiting, + PullRequestPosterID: pullRequestPosterID, + } + require.NoError(t, actions_model.InsertRun(t.Context(), runWaiting, singleWorkflows)) + + run := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: runWaiting.ID}) + require.Equal(t, actions_model.StatusWaiting.String(), run.Status.String()) + + require.NoError(t, BlockUser(db.DefaultContext, doer.ID, blockedUser.ID)) + + run = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: runWaiting.ID}) + require.Equal(t, actions_model.StatusCancelled.String(), run.Status.String()) + }) } From e41bcf5048efaa36d19f21d67e37377e0f77c422 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 30 Oct 2025 16:13:04 +0100 Subject: [PATCH 13/16] feat: add task to cleanup the ActionUser table weekly test coverage is provided by TestAPICron which runs registerCleanupActionUser() --- services/cron/tasks_actions.go | 11 +++++++++++ tests/integration/api_admin_test.go | 4 ++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/services/cron/tasks_actions.go b/services/cron/tasks_actions.go index 2cd484fa69..25c16bdd8d 100644 --- a/services/cron/tasks_actions.go +++ b/services/cron/tasks_actions.go @@ -22,6 +22,7 @@ func initActionsTasks() { registerScheduleTasks() registerActionsCleanup() registerOfflineRunnersCleanup() + registerCleanupActionUser() } func registerStopZombieTasks() { @@ -95,3 +96,13 @@ func registerOfflineRunnersCleanup() { ) }) } + +func registerCleanupActionUser() { + RegisterTaskFatal("actions_action_user", &BaseConfig{ + Enabled: true, + RunAtStart: true, + Schedule: "@weekly", + }, func(ctx context.Context, _ *user_model.User, _ Config) error { + return actions_service.CleanupActionUser(ctx) + }) +} diff --git a/tests/integration/api_admin_test.go b/tests/integration/api_admin_test.go index 7f91ac3874..ce63cf259c 100644 --- a/tests/integration/api_admin_test.go +++ b/tests/integration/api_admin_test.go @@ -357,11 +357,11 @@ func TestAPICron(t *testing.T) { AddTokenAuth(token) resp := MakeRequest(t, req, http.StatusOK) - assert.Equal(t, "29", resp.Header().Get("X-Total-Count")) + assert.Equal(t, "30", resp.Header().Get("X-Total-Count")) var crons []api.Cron DecodeJSON(t, resp, &crons) - assert.Len(t, crons, 29) + assert.Len(t, crons, 30) }) t.Run("Execute", func(t *testing.T) { From daa161ca9bf3887cf4caf6fa7a6ce5f2940a67c8 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 30 Oct 2025 16:15:11 +0100 Subject: [PATCH 14/16] chore: remove the approve endpoint for runs It will be replaced with a link to the trust panel in the pull request. --- routers/web/repo/actions/view.go | 36 -------------------------------- routers/web/web.go | 1 - 2 files changed, 37 deletions(-) diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index 95f383b9a6..55999702d2 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -680,42 +680,6 @@ func Cancel(ctx *context_module.Context) { ctx.JSON(http.StatusOK, struct{}{}) } -func Approve(ctx *context_module.Context) { - runIndex := ctx.ParamsInt64("run") - - current, jobs := getRunJobs(ctx, runIndex, -1) - if ctx.Written() { - return - } - run := current.Run - doer := ctx.Doer - - if err := db.WithTx(ctx, func(ctx context.Context) error { - run.NeedApproval = false - run.ApprovedBy = doer.ID - if err := actions_service.UpdateRun(ctx, run, "need_approval", "approved_by"); err != nil { - return err - } - for _, job := range jobs { - if len(job.Needs) == 0 && job.Status.IsBlocked() { - job.Status = actions_model.StatusWaiting - _, err := actions_service.UpdateRunJob(ctx, job, nil, "status") - if err != nil { - return err - } - } - } - return nil - }); err != nil { - ctx.Error(http.StatusInternalServerError, err.Error()) - return - } - - actions_service.CreateCommitStatus(ctx, jobs...) - - ctx.JSON(http.StatusOK, struct{}{}) -} - // getRunJobs gets the jobs of runIndex, and returns jobs[jobIndex], jobs. // Any error will be written to the ctx. // It never returns a nil job of an empty jobs, if the jobIndex is out of range, it will be treated as 0. diff --git a/routers/web/web.go b/routers/web/web.go index 8d1a886554..fb0306e6d2 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1458,7 +1458,6 @@ func registerRoutes(m *web.Route) { Post(web.Bind(actions.ViewRequest{}), actions.ViewPost) }) m.Post("/cancel", reqRepoActionsWriter, actions.Cancel) - m.Post("/approve", reqRepoActionsWriter, actions.Approve) m.Get("/artifacts", actions.ArtifactsView) m.Get("/artifacts/{artifact_name_or_id}", actions.ArtifactsDownloadView) m.Delete("/artifacts/{artifact_name}", reqRepoActionsWriter, actions.ArtifactsDeleteView) From 465d057fae70d7fc4f2bb656029cfc8d87593517 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 30 Oct 2025 16:27:43 +0100 Subject: [PATCH 15/16] chore(docs): a team member who can write actions can delegate trust --- options/locale/locale_en-US.ini | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 7d71282e40..5baf2fcfe8 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2793,8 +2793,8 @@ projects.read = Read: Access repository project boards. projects.write = Write: Create projects and columns and edit them. packages.read = Read: View and download packages assigned to the repository. packages.write = Write: Publish and delete packages assigned to the repository. -actions.read = Read: View integrated CI/CD pipelines and their logs. -actions.write = Write: Manually trigger, restart, cancel or approve pending CI/CD pipelines. +actions.read = Read: View workflow runs and their logs. +actions.write = Write: Trigger, restart, and cancel workflows. Manage trust delegation to pull requests posters. ext_issues = Access the link to an external issue tracker. The permissions are managed externally. ext_wiki = Access the link to an external wiki. The permissions are managed externally. From 57f986c7b162d28e91b2947f1d1a986d67312051 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 30 Oct 2025 16:16:59 +0100 Subject: [PATCH 16/16] feat: UI for the pull request trust management panel See the documentation pull request for a description https://codeberg.org/forgejo/docs/pulls/1567 https://forgejo.codeberg.page/@docs_pull_1567/docs/next/user/actions/security-pull-request/ --- options/locale_next/locale_en-US.json | 14 + routers/web/repo/pull.go | 71 ++++ routers/web/web.go | 2 + services/context/permission.go | 13 + templates/repo/actions/view.tmpl | 2 +- templates/repo/issue/view_content/pull.tmpl | 2 + templates/repo/pulls/trust.tmpl | 61 ++++ tests/integration/actions_trust_test.go | 355 +++++++++++++++++++ web_src/js/components/RepoActionView.test.js | 38 ++ web_src/js/components/RepoActionView.vue | 3 +- 10 files changed, 559 insertions(+), 2 deletions(-) create mode 100644 templates/repo/pulls/trust.tmpl create mode 100644 tests/integration/actions_trust_test.go diff --git a/options/locale_next/locale_en-US.json b/options/locale_next/locale_en-US.json index a707e564a7..edca1072a8 100644 --- a/options/locale_next/locale_en-US.json +++ b/options/locale_next/locale_en-US.json @@ -56,6 +56,19 @@ "relativetime.2months": "two months ago", "relativetime.1year": "last year", "relativetime.2years": "two years ago", + "repo.pulls.poster_manage_approval": "Manage approval", + "repo.pulls.poster_requires_approval": "Some workflows are waiting to be reviewed.", + "repo.pulls.poster_requires_approval.tooltip": "The author of this pull request is not trusted to run workflows triggered by a pull request created from a forked repository or with AGit. The workflows triggered by a `pull_request` event will not run until they are approved.", + "repo.pulls.poster_is_trusted": "The author of this pull request is always trusted to run workflows.", + "repo.pulls.poster_is_trusted.tooltip": "The author of this pull request is explicitly trusted to run workflows triggered by `pull_request` events.", + "repo.pulls.poster_trust_deny": "Deny", + "repo.pulls.poster_trust_deny.tooltip": "The workflows waiting approval will be canceled.", + "repo.pulls.poster_trust_once": "Approve once", + "repo.pulls.poster_trust_once.tooltip": "The workflows triggered by a `pull_request` event will run on this commit but will need to be approved for all future commits pushed to this pull request.", + "repo.pulls.poster_trust_always": "Approve always", + "repo.pulls.poster_trust_always.tooltip": "The workflows triggered by a `pull_request` event will run on this commit and there will be no need to approve runs from this pull request or future pull requests authored by the same user.", + "repo.pulls.poster_trust_revoke": "Revoke", + "repo.pulls.poster_trust_revoke.tooltip": "The author of this pull request will no longer be trusted to run workflows triggered by a `pull_request` event, each run will have to be manually approved.", "repo.pulls.already_merged": "Merge failed: This pull request has already been merged.", "repo.pulls.merged_title_desc": { "one": "merged %[1]d commit from %[2]s into %[3]s %[4]s", @@ -137,6 +150,7 @@ "admin.auths.allow_username_change.description": "Allow users to change their username in the profile settings", "admin.dashboard.cleanup_offline_runners": "Cleanup offline runners", "admin.dashboard.remove_resolved_reports": "Remove resolved reports", + "admin.dashboard.actions_action_user": "Revoke Forgejo Actions trust for inactive users", "admin.config.security": "Security configuration", "admin.config.global_2fa_requirement.title": "Global two-factor requirement", "admin.config.global_2fa_requirement.none": "No", diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index dee630d96b..453637e7d8 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -18,6 +18,7 @@ import ( "strings" "forgejo.org/models" + actions_model "forgejo.org/models/actions" activities_model "forgejo.org/models/activities" asymkey_model "forgejo.org/models/asymkey" "forgejo.org/models/db" @@ -44,6 +45,7 @@ import ( "forgejo.org/modules/util" "forgejo.org/modules/web" "forgejo.org/routers/utils" + actions_service "forgejo.org/services/actions" asymkey_service "forgejo.org/services/asymkey" "forgejo.org/services/automerge" "forgejo.org/services/context" @@ -764,6 +766,11 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C ctx.Data["IsNothingToCompare"] = true } + PrepareViewPullInfoActions(ctx, pull) + if ctx.Written() { + return nil + } + if pull.IsWorkInProgress(ctx) { ctx.Data["IsPullWorkInProgress"] = true ctx.Data["WorkInProgressPrefix"] = pull.GetWorkInProgressPrefix(ctx) @@ -1943,3 +1950,67 @@ func SetAllowEdits(ctx *context.Context) { "allow_maintainer_edit": pr.AllowMaintainerEdit, }) } + +func PrepareViewPullInfoActions(ctx *context.Context, pull *issues_model.PullRequest) { + canReadUnitActions := ctx.Repo.CanRead(unit.TypeActions) + ctx.Data["CanReadUnitActions"] = canReadUnitActions + + if !canReadUnitActions { + return + } + + PrepareViewPullInfoActionsTrust(ctx, pull) + if ctx.Written() { + return + } +} + +func PrepareViewPullInfoActionsTrust(ctx *context.Context, pull *issues_model.PullRequest) { + trusted, err := actions_service.GetPullRequestPosterIsTrustedWithActions(ctx, pull) + if err != nil { + ctx.ServerError("GetPullRequestUserIsTrustedWithActions", err) + return + } + ctx.Data["PullRequestPosterIsNotTrustedWithActions"] = trusted == actions_service.UserIsNotTrustedWithActions + ctx.Data["PullRequestPosterIsExplicitlyTrustedWithActions"] = trusted == actions_service.UserIsExplicitlyTrustedWithActions + ctx.Data["PullRequestPosterIsImplicitlyTrustedWithActions"] = trusted == actions_service.UserIsImplicitlyTrustedWithActions + + someRunsNeedApproval, err := actions_model.HasRunThatNeedApproval(ctx, pull.Issue.RepoID, pull.ID) + if err != nil { + ctx.ServerError("HasRunThatNeedApproval", err) + } + ctx.Data["SomePullRequestRunsNeedApproval"] = someRunsNeedApproval + + ctx.Data["UserCanDelegateTrustWithPullRequest"] = context.CheckRepoDelegateActionTrust(ctx) +} + +func UpdateTrustWithPullRequestActions(ctx *context.Context) { + pr, err := issues_model.GetPullRequestByIndex(ctx, ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) + if err != nil { + if issues_model.IsErrPullRequestNotExist(err) { + ctx.NotFound("GetPullRequestByIndex", err) + } else { + ctx.ServerError("GetPullRequestByIndex", err) + } + return + } + + trust := ctx.FormString("trust") + + if err := actions_service.UpdateTrustedWithPullRequest(ctx, ctx.Doer.ID, pr, actions_service.TrustUpdate(trust)); err != nil { + ctx.Error(http.StatusInternalServerError, err.Error()) + return + } + + if err := pr.LoadIssue(ctx); err != nil { + ctx.Error(http.StatusInternalServerError, err.Error()) + return + } + + if err := pr.Issue.LoadRepo(ctx); err != nil { + ctx.Error(http.StatusInternalServerError, err.Error()) + return + } + + ctx.Redirect(fmt.Sprintf("%s#pull-request-trust-panel", pr.Issue.Link())) +} diff --git a/routers/web/web.go b/routers/web/web.go index fb0306e6d2..4e064ed40a 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -846,6 +846,7 @@ func registerRoutes(m *web.Route) { reqRepoProjectsWriter := context.RequireRepoWriter(unit.TypeProjects) reqRepoActionsReader := context.RequireRepoReader(unit.TypeActions) reqRepoActionsWriter := context.RequireRepoWriter(unit.TypeActions) + reqRepoDelegateActionTrust := context.RequireRepoDelegateActionTrust() reqPackageAccess := func(accessMode perm.AccessMode) func(ctx *context.Context) { return func(ctx *context.Context) { @@ -1215,6 +1216,7 @@ func registerRoutes(m *web.Route) { m.Group("/{type:issues|pulls}", func() { m.Group("/{index}", func() { m.Post("/title", repo.UpdateIssueTitle) + m.Post("/action-user-trust", reqRepoActionsReader, actions.MustEnableActions, reqRepoDelegateActionTrust, repo.UpdateTrustWithPullRequestActions) m.Post("/content", repo.UpdateIssueContent) m.Post("/deadline", web.Bind(structs.EditDeadlineOption{}), repo.UpdateIssueDeadline) m.Post("/watch", repo.IssueWatch) diff --git a/services/context/permission.go b/services/context/permission.go index b6af87f912..483758141c 100644 --- a/services/context/permission.go +++ b/services/context/permission.go @@ -111,6 +111,19 @@ func RequireRepoReaderOr(unitTypes ...unit.Type) func(ctx *Context) { } } +func RequireRepoDelegateActionTrust() func(ctx *Context) { + return func(ctx *Context) { + if CheckRepoDelegateActionTrust(ctx) { + return + } + ctx.NotFound(ctx.Req.URL.RequestURI(), nil) + } +} + +func CheckRepoDelegateActionTrust(ctx *Context) bool { + return ctx.Repo.IsAdmin() || (ctx.IsSigned && ctx.Doer.IsAdmin) || ctx.Repo.CanWrite(unit.TypeActions) +} + // CheckRepoScopedToken check whether personal access token has repo scope func CheckRepoScopedToken(ctx *Context, repo *repo_model.Repository, level auth_model.AccessTokenScopeLevel) { if !ctx.IsBasicAuth || ctx.Data["IsApiToken"] != true { diff --git a/templates/repo/actions/view.tmpl b/templates/repo/actions/view.tmpl index 5b8b44639e..6adc91417c 100644 --- a/templates/repo/actions/view.tmpl +++ b/templates/repo/actions/view.tmpl @@ -12,7 +12,7 @@ data-workflow-url="{{.WorkflowURL}}" data-initial-post-response="{{.InitialData}}" data-initial-artifacts-response="{{.InitialArtifactsData}}" - data-locale-approve="{{ctx.Locale.Tr "repo.diff.review.approve"}}" + data-locale-approve="{{ctx.Locale.Tr "repo.pulls.poster_manage_approval"}}" data-locale-cancel="{{ctx.Locale.Tr "cancel"}}" data-locale-rerun="{{ctx.Locale.Tr "rerun"}}" data-locale-rerun-all="{{ctx.Locale.Tr "rerun_all"}}" diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 101474d0ff..21f0d613ed 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -94,6 +94,7 @@ {{end}} + {{template "repo/pulls/trust" .}} {{template "repo/issue/view_content/update_branch_by_merge" $}} {{else if .Issue.PullRequest.IsChecking}}
@@ -187,6 +188,7 @@
{{end}} {{end}} + {{template "repo/pulls/trust" .}} {{template "repo/issue/view_content/update_branch_by_merge" $}} {{if .Issue.PullRequest.IsEmpty}}
diff --git a/templates/repo/pulls/trust.tmpl b/templates/repo/pulls/trust.tmpl new file mode 100644 index 0000000000..f715f27bbf --- /dev/null +++ b/templates/repo/pulls/trust.tmpl @@ -0,0 +1,61 @@ +{{/* +Template Attributes: +* CanReadUnitActions: true if the actions unit is active and readable +* SomePullRequestRunsNeedApproval: true if there is at least one run waiting for approval +* UserCanDelegateTrustWithPullRequest: true if the user can delegate trust in the context of pull requests +* PullRequestPosterIsNotTrustedWithActions: true if the poster of the pull request needs to be approved to run actions +* PullRequestPosterIsExplicitlyTrustedWithActions: true if the poster of the pull request is trusted to run actions (once or always) +* PullRequestPosterIsImplicitlyTrustedWithActions: true if the poster of the pull request is trusted to run actions because of elevated permissions +* Link: URL to the pull request +*/}} + +{{if .CanReadUnitActions}} + {{if and .UserCanDelegateTrustWithPullRequest .PullRequestPosterIsExplicitlyTrustedWithActions}} +
+
+
+
+ {{ctx.Locale.Tr "repo.pulls.poster_is_trusted" "https://forgejo.org/docs/latest/user/actions/security-pull-request/"}} +
+
+
+ + +
+
+
+
+ {{else if and .PullRequestPosterIsNotTrustedWithActions .SomePullRequestRunsNeedApproval}} +
+
+
+
+ {{svg "octicon-alert" 16 "text red"}} + {{ctx.Locale.Tr "repo.pulls.poster_requires_approval" "https://forgejo.org/docs/latest/user/actions/security-pull-request/"}} +
+ {{if .UserCanDelegateTrustWithPullRequest}} +
+
+
+ + +
+
+
+
+ + +
+
+
+
+ + +
+
+
+ {{end}} +
+
+ {{end}} +{{end}} diff --git a/tests/integration/actions_trust_test.go b/tests/integration/actions_trust_test.go new file mode 100644 index 0000000000..7a683d5625 --- /dev/null +++ b/tests/integration/actions_trust_test.go @@ -0,0 +1,355 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package integration + +import ( + "fmt" + "net/http" + "net/url" + "strings" + "testing" + "time" + + actions_model "forgejo.org/models/actions" + issues_model "forgejo.org/models/issues" + repo_model "forgejo.org/models/repo" + unit_model "forgejo.org/models/unit" + "forgejo.org/models/unittest" + user_model "forgejo.org/models/user" + actions_module "forgejo.org/modules/actions" + "forgejo.org/modules/git" + "forgejo.org/modules/structs" + actions_service "forgejo.org/services/actions" + pull_service "forgejo.org/services/pull" + repo_service "forgejo.org/services/repository" + files_service "forgejo.org/services/repository/files" + "forgejo.org/tests" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func actionsTrustTestClickTrustPanel(t *testing.T, session *TestSession, url, trust string) { + // an admin approves the run once + req := NewRequest(t, "GET", url) + resp := session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + htmlDoc.AssertElement(t, "#pull-request-trust-panel", true) + link, exists := htmlDoc.doc.Find("#pull-request-trust-panel-" + trust).Attr("action") + require.True(t, exists) + actualTrust, exists := htmlDoc.doc.Find(fmt.Sprintf("#pull-request-trust-panel-%s input[name='trust']", trust)).Attr("value") + require.True(t, exists) + require.Equal(t, trust, actualTrust) + req = NewRequestWithValues(t, "POST", link, map[string]string{ + "trust": trust, + }) + session.MakeRequest(t, req, http.StatusSeeOther) +} + +func actionsTrustTestAssertTrustPanelPresence(t *testing.T, session *TestSession, url string, present bool) { + t.Helper() + req := NewRequest(t, "GET", url) + resp := session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + htmlDoc.AssertElement(t, ".error-code", false) + htmlDoc.AssertElement(t, "#pull-request-trust-panel", present) +} + +func actionsTrustTestAssertTrustPanel(t *testing.T, session *TestSession, url string) { + t.Helper() + actionsTrustTestAssertTrustPanelPresence(t, session, url, true) +} + +func actionsTrustTestAssertNoTrustPanel(t *testing.T, session *TestSession, url string) { + t.Helper() + actionsTrustTestAssertTrustPanelPresence(t, session, url, false) +} + +func actionsTrustTestCreateBaseRepo(t *testing.T, owner *user_model.User) (*repo_model.Repository, func()) { + t.Helper() + + // create the base repo + baseRepo, _, f := tests.CreateDeclarativeRepo(t, owner, "repo-pull-request", + []unit_model.Type{unit_model.TypeActions}, nil, nil, + ) + + // add workflow file to the base repo + addWorkflowToBaseResp, err := files_service.ChangeRepoFiles(git.DefaultContext, baseRepo, owner, &files_service.ChangeRepoFilesOptions{ + Files: []*files_service.ChangeRepoFile{ + { + Operation: "create", + TreePath: ".forgejo/workflows/pr.yml", + ContentReader: strings.NewReader(` +on: + pull_request: + +jobs: + test: + runs-on: docker + steps: + - run: echo helloworld +`), + }, + }, + Message: "add workflow", + OldBranch: "main", + NewBranch: "main", + Author: &files_service.IdentityOptions{ + Name: owner.Name, + Email: owner.Email, + }, + Committer: &files_service.IdentityOptions{ + Name: owner.Name, + Email: owner.Email, + }, + Dates: &files_service.CommitDateOptions{ + Author: time.Now(), + Committer: time.Now(), + }, + }) + require.NoError(t, err) + require.NotEmpty(t, addWorkflowToBaseResp) + return baseRepo, f +} + +func actionsTrustTestRequireRun(t *testing.T, repo *repo_model.Repository, modifiedFiles *structs.FilesResponse) { + t.Helper() + + actionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: repo.ID, CommitSHA: modifiedFiles.Commit.SHA}) + require.Equal(t, actions_module.GithubEventPullRequest, actionRun.TriggerEvent) + require.Equal(t, actions_model.StatusWaiting.String(), actionRun.Status.String()) + unittest.BeanExists(t, &actions_model.ActionRunJob{RunID: actionRun.ID, RepoID: repo.ID}) +} + +func actionsTrustTestRepoCreateBranch(t *testing.T, doer *user_model.User, repo *repo_model.Repository) *structs.FilesResponse { + t.Helper() + + return actionsTrustTestModifyRepo(t, doer, repo, "file_in_fork.txt", "main", "fork-branch-1") +} + +func actionsTrustTestRepoModify(t *testing.T, doer *user_model.User, baseRepo, headRepo *repo_model.Repository, filename string) *structs.FilesResponse { + t.Helper() + + modified := actionsTrustTestModifyRepo(t, doer, headRepo, filename, "fork-branch-1", "fork-branch-1") + // the creation of the run is not synchronous + require.Eventually(t, func() bool { + return unittest.BeanExists(t, &actions_model.ActionRun{RepoID: baseRepo.ID, CommitSHA: modified.Commit.SHA}) + }, 60*time.Second, time.Millisecond*100) + return modified +} + +func actionsTrustTestModifyRepo(t *testing.T, doer *user_model.User, repo *repo_model.Repository, filename, oldBranch, newBranch string) *structs.FilesResponse { + t.Helper() + + // add a new file to the forked repo + addFile, err := files_service.ChangeRepoFiles(git.DefaultContext, repo, doer, &files_service.ChangeRepoFilesOptions{ + Files: []*files_service.ChangeRepoFile{ + { + Operation: "create", + TreePath: filename, + ContentReader: strings.NewReader("content"), + }, + }, + Message: "add " + filename, + OldBranch: oldBranch, + NewBranch: newBranch, + Author: &files_service.IdentityOptions{ + Name: doer.Name, + Email: doer.Email, + }, + Committer: &files_service.IdentityOptions{ + Name: doer.Name, + Email: doer.Email, + }, + Dates: &files_service.CommitDateOptions{ + Author: time.Now(), + Committer: time.Now(), + }, + }) + require.NoError(t, err) + require.NotEmpty(t, addFile) + return addFile +} + +func actionsTrustTestCreatePullRequestFromForkedRepo(t *testing.T, baseUser *user_model.User, baseRepo *repo_model.Repository, headUser *user_model.User) (*repo_model.Repository, *issues_model.PullRequest, *structs.FilesResponse) { + t.Helper() + + forkRepo := func(t *testing.T, baseUser *user_model.User, baseRepo *repo_model.Repository, headUser *user_model.User) *repo_model.Repository { + t.Helper() + + // create the forked repo + forkedRepo, err := repo_service.ForkRepositoryAndUpdates(git.DefaultContext, baseUser, headUser, repo_service.ForkRepoOptions{ + BaseRepo: baseRepo, + Name: "forked-repo-pull-request", + Description: "test pull-request event", + }) + require.NoError(t, err) + require.NotEmpty(t, forkedRepo) + return forkedRepo + } + + forkedRepo := forkRepo(t, baseUser, baseRepo, headUser) + addFileToForkedResp := actionsTrustTestRepoCreateBranch(t, headUser, forkedRepo) + + // create Pull + pullIssue := &issues_model.Issue{ + RepoID: baseRepo.ID, + Title: "Test pull-request", + PosterID: headUser.ID, + Poster: headUser, + IsPull: true, + } + pullRequest := &issues_model.PullRequest{ + HeadRepoID: forkedRepo.ID, + BaseRepoID: baseRepo.ID, + HeadBranch: "fork-branch-1", + BaseBranch: "main", + HeadRepo: forkedRepo, + BaseRepo: baseRepo, + Type: issues_model.PullRequestGitea, + } + // create the pull request + err := pull_service.NewPullRequest(git.DefaultContext, baseRepo, pullIssue, nil, nil, pullRequest, nil) + require.NoError(t, err) + + actionsTrustTestRequireRun(t, baseRepo, addFileToForkedResp) + + return forkedRepo, pullRequest, addFileToForkedResp +} + +func TestActionsPullRequestTrustPanel(t *testing.T) { + onApplicationRun(t, func(t *testing.T, u *url.URL) { + ownerUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) // owner of the repo + + regularUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) // a regular user with no specific permission + regularSession := loginUser(t, regularUser.Name) + + userAdmin := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) // the instance admin + adminSession := loginUser(t, userAdmin.Name) + + baseRepo, f := actionsTrustTestCreateBaseRepo(t, ownerUser) + defer f() + + forkedRepo, pullRequest, addFileToForkedResp := actionsTrustTestCreatePullRequestFromForkedRepo(t, ownerUser, baseRepo, regularUser) + pullRequestLink := pullRequest.Issue.Link() + + t.Run("Regular user sees a pending approval on a newly created pull request from a fork", func(t *testing.T) { + actionsTrustTestAssertTrustPanel(t, regularSession, pullRequestLink) + }) + + t.Run("Admin approves runs once", func(t *testing.T) { + actionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, CommitSHA: addFileToForkedResp.Commit.SHA}) + + { + actionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, CommitSHA: addFileToForkedResp.Commit.SHA}) + assert.True(t, actionRun.NeedApproval) + actionRunJob := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{RunID: actionRun.ID, RepoID: baseRepo.ID}) + assert.Equal(t, actions_model.StatusBlocked.String(), actionRunJob.Status.String()) + } + + actionsTrustTestAssertTrustPanel(t, adminSession, pullRequestLink) + actionsTrustTestClickTrustPanel(t, adminSession, pullRequestLink, string(actions_service.UserTrustedOnce)) + + { + actionRunJob := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{RunID: actionRun.ID, RepoID: baseRepo.ID}) + assert.Equal(t, actions_model.StatusWaiting.String(), actionRunJob.Status.String()) + actionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID}) + assert.False(t, actionRun.NeedApproval) + } + }) + + t.Run("All users sees no pending approval because it was approved once", func(t *testing.T) { + actionsTrustTestAssertNoTrustPanel(t, regularSession, pullRequestLink) + actionsTrustTestAssertNoTrustPanel(t, adminSession, pullRequestLink) + }) + + modifiedForkedResp := actionsTrustTestRepoModify(t, regularUser, baseRepo, forkedRepo, "add_file_one.txt") + + t.Run("Regular user sees a pending approval on a modified pull request from a fork (2)", func(t *testing.T) { + actionsTrustTestAssertTrustPanel(t, regularSession, pullRequestLink) + }) + + t.Run("Admin denies runs", func(t *testing.T) { + actionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, CommitSHA: modifiedForkedResp.Commit.SHA}) + + { + actionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, CommitSHA: modifiedForkedResp.Commit.SHA}) + assert.True(t, actionRun.NeedApproval) + actionRunJob := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{RunID: actionRun.ID, RepoID: baseRepo.ID}) + assert.Equal(t, actions_model.StatusBlocked.String(), actionRunJob.Status.String()) + } + + actionsTrustTestAssertTrustPanel(t, adminSession, pullRequestLink) + actionsTrustTestClickTrustPanel(t, adminSession, pullRequestLink, string(actions_service.UserTrustDenied)) + + { + actionRunJob := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{RunID: actionRun.ID, RepoID: baseRepo.ID}) + assert.Equal(t, actions_model.StatusCancelled.String(), actionRunJob.Status.String()) + actionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID}) + assert.False(t, actionRun.NeedApproval) + } + }) + + t.Run("All users sees no pending approval because it was denied", func(t *testing.T) { + actionsTrustTestAssertNoTrustPanel(t, regularSession, pullRequestLink) + actionsTrustTestAssertNoTrustPanel(t, adminSession, pullRequestLink) + }) + + modifiedForkedResp = actionsTrustTestRepoModify(t, regularUser, baseRepo, forkedRepo, "add_file_two.txt") + + t.Run("Regular user sees a pending approval on a modified pull request from a fork (2)", func(t *testing.T) { + actionsTrustTestAssertTrustPanel(t, regularSession, pullRequestLink) + }) + + t.Run("Admin always trusts the poster", func(t *testing.T) { + actionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, CommitSHA: modifiedForkedResp.Commit.SHA}) + + { + actionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, CommitSHA: modifiedForkedResp.Commit.SHA}) + assert.True(t, actionRun.NeedApproval) + actionRunJob := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{RunID: actionRun.ID, RepoID: baseRepo.ID}) + assert.Equal(t, actions_model.StatusBlocked.String(), actionRunJob.Status.String()) + } + + actionsTrustTestAssertTrustPanel(t, adminSession, pullRequestLink) + actionsTrustTestClickTrustPanel(t, adminSession, pullRequestLink, string(actions_service.UserAlwaysTrusted)) + + { + actionRunJob := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{RunID: actionRun.ID, RepoID: baseRepo.ID}) + assert.Equal(t, actions_model.StatusWaiting.String(), actionRunJob.Status.String()) + actionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID}) + assert.False(t, actionRun.NeedApproval) + } + }) + + t.Run("Regular users sees no pending approval because it was approved", func(t *testing.T) { + actionsTrustTestAssertNoTrustPanel(t, regularSession, pullRequestLink) + }) + + modifiedForkedResp = actionsTrustTestRepoModify(t, regularUser, baseRepo, forkedRepo, "add_file_three.txt") + + t.Run("No need for approval because the poster is always trusted", func(t *testing.T) { + actionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, CommitSHA: modifiedForkedResp.Commit.SHA}) + assert.False(t, actionRun.NeedApproval) + actionRunJob := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{RunID: actionRun.ID, RepoID: baseRepo.ID}) + assert.Equal(t, actions_model.StatusWaiting.String(), actionRunJob.Status.String()) + }) + + t.Run("Admin revokes the trusted poster", func(t *testing.T) { + actionsTrustTestAssertTrustPanel(t, adminSession, pullRequestLink) + actionsTrustTestClickTrustPanel(t, adminSession, pullRequestLink, string(actions_service.UserTrustRevoked)) + }) + + modifiedForkedResp = actionsTrustTestRepoModify(t, regularUser, baseRepo, forkedRepo, "add_file_four.txt") + + t.Run("There needs to be an approval again because the user is no longer trusted", func(t *testing.T) { + actionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, CommitSHA: modifiedForkedResp.Commit.SHA}) + assert.True(t, actionRun.NeedApproval) + actionRunJob := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{RunID: actionRun.ID, RepoID: baseRepo.ID}) + assert.Equal(t, actions_model.StatusBlocked.String(), actionRunJob.Status.String()) + }) + }) +} diff --git a/web_src/js/components/RepoActionView.test.js b/web_src/js/components/RepoActionView.test.js index 6f575ca8da..b06751b8cf 100644 --- a/web_src/js/components/RepoActionView.test.js +++ b/web_src/js/components/RepoActionView.test.js @@ -397,6 +397,44 @@ test('historical attempt dropdown interactions', async () => { expect(window.location.href).toEqual(toAbsoluteUrl('/user1/repo2/actions/runs/123/jobs/1/attempt/2')); }); +test('run approval interaction', async () => { + const pullRequestLink = '/example-org/example-repo/pulls/456'; + const wrapper = mount(RepoActionView, { + props: { + ...defaultTestProps, + initialJobData: { + state: { + run: { + canApprove: true, + status: 'waiting', + commit: { + pusher: {}, + branch: { + link: toAbsoluteUrl(pullRequestLink), + }, + }, + }, + currentJob: { + steps: [ + { + summary: 'Test Job', + }, + ], + }, + }, + logs: { + stepsLog: [], + }, + }, + }, + }); + await flushPromises(); + const approve = wrapper.findAll('button').filter((button) => button.text() === 'Locale Approve'); + expect(approve.length).toEqual(1); + approve[0].trigger('click'); + expect(window.location.href).toEqual(toAbsoluteUrl(`${pullRequestLink}#pull-request-trust-panel`)); +}); + test('artifacts download links', async () => { Object.defineProperty(document.documentElement, 'lang', {value: 'en'}); vi.spyOn(global, 'fetch').mockImplementation((url, opts) => { diff --git a/web_src/js/components/RepoActionView.vue b/web_src/js/components/RepoActionView.vue index 097676cac2..9b2ba0166c 100644 --- a/web_src/js/components/RepoActionView.vue +++ b/web_src/js/components/RepoActionView.vue @@ -223,7 +223,8 @@ export default { }, // approve a run approveRun() { - POST(`${this.run.link}/approve`); + const url = `${this.run.commit.branch.link}#pull-request-trust-panel`; + window.location.href = url; }, // show/hide the step logs for a group toggleGroupLogs(event) {