From 3fa7ceeb76d33dbeb72ab6de178a47863fba1c90 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 30 Oct 2025 16:09:18 +0100 Subject: [PATCH] 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) +}