diff --git a/.github/workflows/reusable-dispatch.yml b/.github/workflows/reusable-dispatch.yml index 9fb229c708..c0abbc52bd 100644 --- a/.github/workflows/reusable-dispatch.yml +++ b/.github/workflows/reusable-dispatch.yml @@ -124,11 +124,15 @@ jobs: # checkout@v7 blocks fork PR checkouts on pull_request_target by # default. Safe here: only .fullsend/ config is read, no fork code # is executed, and credentials are not persisted. Pin to base branch - # so kill-switch / role gating always reads trusted config. - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + # SHA for PR-scoped events so kill-switch / role gating always reads + # trusted config — PR authors cannot self-authorize. + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} - sparse-checkout: .fullsend/config.yaml + sparse-checkout: | + .fullsend/config.yaml + /OWNERS + /OWNERS_ALIASES sparse-checkout-cone-mode: false - name: Determine stage @@ -159,11 +163,63 @@ jobs: STAGE="" TRIGGER_SOURCE="" + # Check direct membership or alias membership in an OWNERS list. + # Used by has_repo_permission when OWNERS-file auth is enabled. + # yq errors are intentionally suppressed (&>/dev/null): a parse + # failure or missing key means "no match", falling through to + # the collaborator API — not a fail-open gate. + _owners_has_user() { + local key="${1}" user="${2}" + if yq -e ".${key}[] | select((. | downcase) == \"${user}\")" OWNERS &>/dev/null; then + return 0 + fi + if [[ -f "OWNERS_ALIASES" ]]; then + local entry + while IFS= read -r entry; do + [[ -z "${entry}" ]] && continue + [[ ! "${entry}" =~ ^[a-zA-Z0-9_-]+$ ]] && continue + if yq -e ".aliases.\"${entry}\"[] | select((. | downcase) == \"${user}\")" OWNERS_ALIASES &>/dev/null; then + return 0 + fi + done < <(yq ".${key}[]" OWNERS 2>/dev/null) + fi + return 1 + } + # Collaborator role_name vs min (write|triage). See #5223 / ADR 0054. # API resolves org membership regardless of visibility (gh-aw-mcpg#2862). has_repo_permission() { local username="${1:-}" min="${2:-write}" role api_err [[ -z "${username}" ]] && return 1 + + # OWNERS-file authorization (opt-in via authorization providers list in config.yaml). + # Approvers get write-equivalent access; reviewers get triage-equivalent. + # Safe: sparse-checkout pins to base branch SHA, so PR authors cannot + # self-authorize by adding themselves to OWNERS. + if [[ -f "OWNERS" && -f ".fullsend/config.yaml" ]]; then + if [[ "$(yq '(.authorization // []) | any_c(.provider == "owners_file")' .fullsend/config.yaml)" == "true" ]]; then + if [[ ! "${username}" =~ ^[a-zA-Z0-9-]+$ ]]; then + echo "::warning::OWNERS auth skipped: username '${username}' contains unexpected characters" >&2 + else + local lc_user="${username,,}" + case "${min}" in + write|triage) + if _owners_has_user approvers "${lc_user}"; then + echo "::notice::OWNERS file resolved user '${username}' as approver (requested: ${min})" + return 0 + fi + ;;& + triage) + if _owners_has_user reviewers "${lc_user}"; then + echo "::notice::OWNERS file resolved user '${username}' as reviewer (requested: ${min})" + return 0 + fi + ;; + esac + fi + fi + fi + api_err=$(mktemp) || { echo "::warning::Failed to create temp file for permission check of ${username}" >&2 return 1 @@ -600,7 +656,7 @@ jobs: - name: Checkout config repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} @@ -711,7 +767,7 @@ jobs: - name: Checkout config repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} @@ -849,7 +905,7 @@ jobs: - name: Checkout config repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} @@ -978,7 +1034,7 @@ jobs: - name: Checkout config repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} @@ -1256,7 +1312,7 @@ jobs: - name: Checkout config repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} @@ -1366,7 +1422,7 @@ jobs: - name: Checkout config repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} @@ -1462,11 +1518,13 @@ jobs: - name: Checkout caller repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} sparse-checkout: | .fullsend/ + /OWNERS + /OWNERS_ALIASES .defaults/action.yml .defaults/.github/actions/install-fullsend-cli/ .fullsend/.defaults/action.yml @@ -1622,7 +1680,7 @@ jobs: - name: Checkout config repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} diff --git a/docs/ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md b/docs/ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md index 13a3e59873..9b89697648 100644 --- a/docs/ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md +++ b/docs/ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md @@ -189,6 +189,35 @@ permission list, not by bypassing the check. > any closer can trigger read-only lifecycle accounting. This follows > the extension path above rather than bypassing the check. +> **Note (2026-08-10, [#6042](https://github.com/fullsend-ai/fullsend/issues/6042)):** +> Prow-based repositories (e.g., OpenShift) use OWNERS files rather than +> GitHub collaborator roles to define contributor authority. +> `has_repo_permission` now supports an opt-in OWNERS-file authorization +> path: when `owners_file` is listed in the `authorization` providers in +> `.fullsend/config.yaml`, the function checks the repo-root `OWNERS` +> (and `OWNERS_ALIASES`) before falling back to the collaborator API. +> OWNERS approvers get write-equivalent access; reviewers get +> triage-equivalent. The sparse-checkout pins to the base branch SHA for +> PR-scoped events (`pull_request_target`, `pull_request_review`) and the +> default-branch head otherwise, so PR authors cannot self-authorize by +> modifying OWNERS in their PR. +> This follows the extension path above (extending the allowed permission +> sources in `has_repo_permission`) rather than bypassing the check. +> OWNERS auth applies to both built-in stages (bash routing) and the +> harness/custom-agent dispatch path (`internal/harnessdispatch`), where +> `owners.Resolve` computes an effective role for the `IsAuthorized` +> gate without mutating the original event. +> +> OWNERS reviewer access (triage-equivalent) applies to built-in +> bash-routed stages only (e.g. `/fs-triage`, `/fs-review`). Custom +> harness dispatch requires write-level access — OWNERS approver or +> GitHub write+ collaborator — because `IsAuthorized` gates all +> harness triggers at the write level. +> +> v1 limitation: only repo-root flat `approvers`/`reviewers` lists are +> read. Prow `filters:` blocks and nested per-directory OWNERS files +> are not supported. + ## Consequences - All dispatch paths require write-level repository permission, diff --git a/docs/contributing/workflow-contracts.md b/docs/contributing/workflow-contracts.md index 052e6d86f5..2b28177719 100644 --- a/docs/contributing/workflow-contracts.md +++ b/docs/contributing/workflow-contracts.md @@ -9,6 +9,8 @@ **Silent failures and required-flag consistency:** Omitting a secret that is `required: true` at every hop in the chain fails loudly at workflow-call validation time and self-enforces. However, a secret whose `required` flag is `false` at any upstream hop can still arrive as an empty string at a downstream `required: true` consumer — GitHub Actions' required-secret validation only checks key presence, not that the resolved value is non-empty. For example, `FULLSEND_GCP_WIF_PROVIDER` is `required: false` in `reusable-dispatch.yml` but `required: true` in every downstream `reusable-.yml`, so an installer that never sets it satisfies the key-presence check while the actual value is empty. Treat a missing forwarding hop the same as a missing sync — it is a correctness bug, not a cosmetic issue. Required-flag consistency across the *whole* chain matters, not just the flag at the final consumer. +**OWNERS-file authorization:** `has_repo_permission` in both `reusable-dispatch.yml` and the scaffold `dispatch.yml` supports an opt-in OWNERS-file path gated by `owners_file` in the `authorization` providers list in `.fullsend/config.yaml`. When enabled, the function checks the repo-root `OWNERS` (and `OWNERS_ALIASES` if present) before falling back to the collaborator API. Approvers get write-equivalent access; reviewers get triage-equivalent. Changes to `_owners_has_user` or the OWNERS authorization block must be applied to both workflow files — `TestDispatchPerStageAuthorization` checks `has_repo_permission` parity including OWNERS role-mapping invariants. The Go harness-dispatch path (`internal/harnessdispatch/core.go`) has equivalent OWNERS resolution via `internal/owners`; changes to the OWNERS schema or role mapping must be kept in sync across both implementations. + **Security — consuming threaded inputs:** When a newly-threaded entry carries user- or event-controlled data, consume it via `env:` in the final `run:` step — never interpolate `${{ ... }}` directly into a shell block (see the Security note atop `reusable-dispatch.yml`). This prevents the GHA script-injection class of bugs the project defends against elsewhere. **When reviewing PRs:** If a diff adds or renames a `secrets:` or `inputs:` entry in a reusable workflow, check that all callers in both chains have been updated. Flag a missing forwarding hop as a medium-severity or higher finding. New secrets/inputs must be forwarded only to the hop(s)/stage(s) that need them — do not use `secrets: inherit` as a substitute for explicit forwarding (`OTEL_EXPORTER_OTLP_TRACES_HEADERS`, for example, is explicitly forwarded to every inline stage job in `reusable-dispatch.yml` because each stage runs an agent that emits traces). `workflow_call_alignment_test.go` already automates much of this verification — see `TestWorkflowCallInputAlignment` (validates required inputs/secrets are threaded through both chains) and `TestOTELHeadersSecretThreading` (bespoke test for optional secrets). For new optional secrets/inputs, extend those tests or add a similar one rather than relying solely on manual tracing. diff --git a/docs/guides/infrastructure/layered-config-reference.md b/docs/guides/infrastructure/layered-config-reference.md index 4231770f3d..e8927a7ff1 100644 --- a/docs/guides/infrastructure/layered-config-reference.md +++ b/docs/guides/infrastructure/layered-config-reference.md @@ -90,6 +90,16 @@ the overlay → base → code defaults chain. | `models.aliases` | `map[string]string` (nested) | Per-key merge | `nil` (fleet defaults) | | `create_issues` | `*CreateIssuesConfig` | Replace whole object if set | `nil` | | `status_notifications` | `*StatusNotificationConfig` | Replace whole object if set | `nil` | +| `authorization`¹ | `[]AuthorizationProvider` | Overlay only (not layered) | `nil` | + +> ¹ `authorization` is a list of authorization providers +> (`AuthorizationProvider` / `AuthorizationOwnersFile()`) consumed +> by both the dispatch workflow's bash/yq and `internal/harnessdispatch`. +> It is intentionally **overlay-only**: `AuthorizationOwnersFile()` +> does not fall through to the parent config, so setting it in +> `config.base.yaml` has no effect — each repo must opt in explicitly. +> See [#6072](https://github.com/fullsend-ai/fullsend/issues/6072) for +> the planned migration to the Go config layer. ### Per-agent `runtime`, `model`, `effort` on `agents:` entries @@ -366,6 +376,39 @@ The `status_notifications` field uses the same replace-if-set semantics as - Non-nil — replaces the parent value entirely, including nested `comment.start`/`comment.completion` settings. +### `authorization` — provider list + +The `authorization` field is a list of authorization providers that +supplement the default collaborator-API permission check. Native GitHub +collaborator-API auth always runs implicitly; the list names additional +backends. Currently one provider is supported: + +- `owners_file` — the dispatch routing logic checks the repo-root + `OWNERS` file (and `OWNERS_ALIASES` if present) before falling back + to the GitHub collaborator API. OWNERS approvers get write-equivalent + access; reviewers get triage-equivalent. If the user is not listed in + OWNERS, authorization falls through to the collaborator API — OWNERS + never blocks a collaborator who isn't in the file. + +This applies to both the bash routing path (built-in stages) and the +Go harness-dispatch path (custom agents). A missing or malformed OWNERS +file fails closed: the OWNERS check is skipped and authorization falls +through to the collaborator API. + +v1 limitation: only the repo-root flat `approvers`/`reviewers` lists +are read. Prow `filters:` blocks and nested per-directory OWNERS files +are not supported. + +Example: + +```yaml +authorization: + - provider: owners_file +``` + +See [ADR 0054](../../ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md) +for the full design rationale. + ## Code defaults reference When neither the overlay nor the base layer sets a field, the following diff --git a/e2e/behaviour/features/dispatch/owners-auth.feature b/e2e/behaviour/features/dispatch/owners-auth.feature new file mode 100644 index 0000000000..cb53ca26b9 --- /dev/null +++ b/e2e/behaviour/features/dispatch/owners-auth.feature @@ -0,0 +1,81 @@ +Feature: OWNERS file authorization for bash routing + + Verify that the OWNERS-file authorization path fires when + owners_file is in the authorization providers list in config.yaml. Scenarios + trigger via issues.opened, which unconditionally calls + is_event_actor_authorized and exercises has_repo_permission. + + The outsider identity (TEST_ACTOR_OUTSIDER_PAT) has no collaborator + access, so authorization succeeds only through OWNERS — the denial + scenario verifies a reviewer cannot escalate to write-level access. + The write actor (TEST_ACTOR_WRITE_PAT) has write-level collaborator + access and is NOT in OWNERS, so fallthrough scenarios verify that + OWNERS misses correctly fall back to the collaborator API. + + Background: + Given the enrolled test repository + + Scenario: Triage dispatches via OWNERS approver path when enabled + Given an OWNERS file listing the outsider as an approver + And OWNERS authorization is enabled + And a dummy agent that would: + | description | op | args | + | Prove execution | write_fixture | output/agent-result.json, fixtures/triage/sufficient.json | + When the outsider opens an issue for OWNERS auth testing + Then the triage workflow completes successfully + And the agent will succeed to Prove execution + And the triage workflow logs contain "##[notice]OWNERS file resolved user" + And the triage workflow logs contain "as approver (requested: triage)" + + Scenario: OWNERS alias resolves to grant access + Given an OWNERS file with alias "test-team" as approver + And an OWNERS_ALIASES file mapping "test-team" to the outsider + And OWNERS authorization is enabled + And a dummy agent that would: + | description | op | args | + | Prove execution | write_fixture | output/agent-result.json, fixtures/triage/sufficient.json | + When the outsider opens an issue for OWNERS auth testing + Then the triage workflow completes successfully + And the agent will succeed to Prove execution + And the triage workflow logs contain "##[notice]OWNERS file resolved user" + And the triage workflow logs contain "as approver (requested: triage)" + + Scenario: OWNERS reviewer can triage + Given an OWNERS file listing the outsider as a reviewer only + And OWNERS authorization is enabled + And a dummy agent that would: + | description | op | args | + | Prove execution | write_fixture | output/agent-result.json, fixtures/triage/sufficient.json | + When the outsider opens an issue for OWNERS auth testing + Then the triage workflow completes successfully + And the agent will succeed to Prove execution + And the triage workflow logs contain "##[notice]OWNERS file resolved user" + And the triage workflow logs contain "as reviewer (requested: triage)" + + Scenario: OWNERS reviewer is denied write-level access + Given an OWNERS file listing the outsider as a reviewer + And OWNERS authorization is enabled + And an issue + When the outsider posts "/fs-code" on the issue + Then the dispatch run does not authorize via OWNERS + + Scenario: Unlisted collaborator falls through to API authorization + Given an OWNERS file listing the outsider as a reviewer only + And OWNERS authorization is enabled + And a dummy agent that would: + | description | op | args | + | Prove execution | write_fixture | output/agent-result.json, fixtures/triage/sufficient.json | + When the write actor opens an issue for OWNERS auth testing + Then the triage workflow completes successfully + And the agent will succeed to Prove execution + And the triage workflow logs do not contain "##[notice]OWNERS file resolved user" + + Scenario: Triage dispatches without OWNERS path when not opted in + Given an OWNERS file listing the outsider as an approver + And a dummy agent that would: + | description | op | args | + | Prove execution | write_fixture | output/agent-result.json, fixtures/triage/sufficient.json | + When the write actor opens an issue for OWNERS auth testing + Then the triage workflow completes successfully + And the agent will succeed to Prove execution + And the triage workflow logs do not contain "##[notice]OWNERS file resolved user" diff --git a/e2e/behaviour/suite_test.go b/e2e/behaviour/suite_test.go index 037b4ddcd6..8e3deb0bef 100644 --- a/e2e/behaviour/suite_test.go +++ b/e2e/behaviour/suite_test.go @@ -123,6 +123,26 @@ func TestBehaviourSuite(t *testing.T) { RepoOwner: org, } + if outsiderPAT := os.Getenv("TEST_ACTOR_OUTSIDER_PAT"); outsiderPAT != "" { + outsiderClient := e2etest.NewLiveClient(outsiderPAT) + template.OutsiderSCM = scmgh.New(outsiderClient) + login, err := outsiderClient.GetAuthenticatedUser(ctx) + if err != nil { + t.Fatalf("resolving outsider login: %v", err) + } + template.OutsiderLogin = login + } + + if writePAT := os.Getenv("TEST_ACTOR_WRITE_PAT"); writePAT != "" { + writeClient := e2etest.NewLiveClient(writePAT) + template.WriteSCM = scmgh.New(writeClient) + login, err := writeClient.GetAuthenticatedUser(ctx) + if err != nil { + t.Fatalf("resolving write actor login: %v", err) + } + template.WriteLogin = login + } + suiteRunner := godog.TestSuite{ Name: "behaviour", ScenarioInitializer: func(sc *godog.ScenarioContext) { suite.InitScenario(sc, template) }, diff --git a/internal/config/config.go b/internal/config/config.go index 5781297a69..b87c8ea4d4 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -275,6 +275,12 @@ type CreateIssuesConfig struct { AllowTargets AllowTargets `yaml:"allow_targets"` } +// AuthorizationProvider identifies an opt-in authorization backend +// that supplements the default collaborator-API permission check. +type AuthorizationProvider struct { + Provider string `yaml:"provider"` +} + // orgConfig is the top-level configuration for a fullsend organization. // Consumer packages should use the OrgConfigReader or OrgConfigWriter // interfaces rather than referencing this type directly. @@ -375,6 +381,11 @@ func ValidEffortLevels() []string { return slices.Clone(validEffortLevels) } // ValidEffort reports whether level is an accepted effort value. func ValidEffort(level string) bool { return slices.Contains(validEffortLevels, level) } +// ValidAuthorizationProviders returns the set of recognized authorization provider names. +func ValidAuthorizationProviders() []string { + return []string{"owners_file"} +} + // DefaultAgentRoles returns the standard set of agent roles installed // when no custom roles are specified. The fix stage reuses the coder // app (role: coder) so it does not need a separate app or PEM. @@ -780,8 +791,9 @@ type perRepoConfig struct { // resource prefixes. MarshalYAML preserves the nil-vs-empty // distinction: nil (unset) is omitted, empty (deny-all) is // marshaled as `allowed_remote_resources: []`. - AllowedRemoteResources []string `yaml:"allowed_remote_resources,omitempty"` - CreateIssues *CreateIssuesConfig `yaml:"create_issues,omitempty"` + AllowedRemoteResources []string `yaml:"allowed_remote_resources,omitempty"` + CreateIssues *CreateIssuesConfig `yaml:"create_issues,omitempty"` + Authorization []AuthorizationProvider `yaml:"authorization,omitempty"` // Notifications backs the StatusNotifications() accessor. Named // distinctly from the method (unlike CreateIssues/IssueCreationConfig) // because "StatusNotifications" is the established accessor name @@ -982,6 +994,7 @@ type perRepoConfigMarshal struct { Agents []AgentEntry `yaml:"agents,omitempty"` AllowedRemoteResources *[]string `yaml:"allowed_remote_resources,omitempty"` CreateIssues *CreateIssuesConfig `yaml:"create_issues,omitempty"` + Authorization []AuthorizationProvider `yaml:"authorization,omitempty"` StatusNotifications *StatusNotificationConfig `yaml:"status_notifications,omitempty"` MintURL string `yaml:"mint_url,omitempty"` Inference *PerRepoInferenceConfig `yaml:"inference,omitempty"` @@ -1002,6 +1015,7 @@ func (c *perRepoConfig) MarshalYAML() (interface{}, error) { Runtime: c.Runtime, Agents: c.Agents, CreateIssues: c.CreateIssues, + Authorization: c.Authorization, StatusNotifications: c.Notifications, MintURL: c.MintURL, } @@ -1071,6 +1085,17 @@ func (c *perRepoConfig) Validate() error { return fmt.Errorf("invalid inference provider %q: must be one of %s", c.Inference.Provider, strings.Join(validProviders, ", ")) } } + validAuthProviders := ValidAuthorizationProviders() + seenProviders := make(map[string]bool, len(c.Authorization)) + for i, p := range c.Authorization { + if !slices.Contains(validAuthProviders, p.Provider) { + return fmt.Errorf("authorization[%d]: invalid provider %q: must be one of %s", i, p.Provider, strings.Join(validAuthProviders, ", ")) + } + if seenProviders[p.Provider] { + return fmt.Errorf("authorization[%d]: duplicate provider %q", i, p.Provider) + } + seenProviders[p.Provider] = true + } // Validate the merged view, as ValidateAgentEntries does above: a bad // key in config.base.yaml must not slip through because the overlay // omits models:. diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 26df5e6440..a29a16676a 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -706,6 +706,34 @@ func TestPerRepoConfigValidate_Runtime(t *testing.T) { assert.Contains(t, err.Error(), "invalid runtime") } +func TestPerRepoConfigValidate_AuthorizationValidProvider(t *testing.T) { + cfg := &perRepoConfig{ + Version: "1", + Authorization: []AuthorizationProvider{{Provider: "owners_file"}}, + } + assert.NoError(t, cfg.Validate()) +} + +func TestPerRepoConfigValidate_AuthorizationInvalidProvider(t *testing.T) { + cfg := &perRepoConfig{ + Version: "1", + Authorization: []AuthorizationProvider{{Provider: "ldap"}}, + } + err := cfg.Validate() + assert.Error(t, err) + assert.Contains(t, err.Error(), "invalid provider") +} + +func TestPerRepoConfigValidate_AuthorizationDuplicateProvider(t *testing.T) { + cfg := &perRepoConfig{ + Version: "1", + Authorization: []AuthorizationProvider{{Provider: "owners_file"}, {Provider: "owners_file"}}, + } + err := cfg.Validate() + assert.Error(t, err) + assert.Contains(t, err.Error(), "duplicate provider") +} + func TestParsePerRepoConfig(t *testing.T) { yamlData := ` version: "1" diff --git a/internal/config/defaults.go b/internal/config/defaults.go index 494984339e..04bd11548f 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -62,6 +62,9 @@ func (d *perRepoDefaults) StatusNotifications() *StatusNotificationConfig { retu // IsOrgMode returns false — per-repo configs are never org mode. func (d *perRepoDefaults) IsOrgMode() bool { return false } +// AuthorizationOwnersFile returns false — OWNERS auth is off by default. +func (d *perRepoDefaults) AuthorizationOwnersFile() bool { return false } + // ConfigMintURL returns the default mint URL (hosted public mint). func (d *perRepoDefaults) ConfigMintURL() string { return DefaultPerRepoMintURL } diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index afdd5898da..41b2e0f2c3 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -52,6 +52,7 @@ type ConfigReader interface { StatusNotificationsReader ConfigVersion() string IsOrgMode() bool + AuthorizationOwnersFile() bool } // --- Mode-specific read interfaces --- @@ -96,6 +97,7 @@ type PerRepoConfigReader interface { type ConfigWriter interface { ConfigReader SetKillSwitch(bool) + SetAuthorizationOwnersFile(bool) SetAgents([]AgentEntry) SetAllowedRemoteResources([]string) SetStatusNotifications(*StatusNotificationConfig) @@ -185,6 +187,14 @@ func (c *orgConfig) StatusNotifications() *StatusNotificationConfig { // SetKillSwitch sets the kill switch state. func (c *orgConfig) SetKillSwitch(v bool) { c.KillSwitch = v } +// AuthorizationOwnersFile returns false for org configs; OWNERS +// authorization is per-repo only. +func (c *orgConfig) AuthorizationOwnersFile() bool { return false } + +// SetAuthorizationOwnersFile is a no-op for org configs; OWNERS +// authorization is per-repo only. +func (c *orgConfig) SetAuthorizationOwnersFile(bool) {} + // SetAgents replaces the registered agent entries. func (c *orgConfig) SetAgents(agents []AgentEntry) { c.Agents = agents } @@ -399,6 +409,18 @@ func (c *perRepoConfig) ConfigVersion() string { // IsOrgMode reports that this is a per-repo configuration. func (c *perRepoConfig) IsOrgMode() bool { return false } +// AuthorizationOwnersFile returns whether OWNERS-file authorization is enabled. +// Intentionally no parent fallback: OWNERS auth is a per-repo opt-in that must +// not be inheritable from config.base.yaml. +func (c *perRepoConfig) AuthorizationOwnersFile() bool { + for _, p := range c.Authorization { + if p.Provider == "owners_file" { + return true + } + } + return false +} + // ConfigRoles returns the configured agent roles. nil (key omitted) // falls through to parent. Non-nil (including empty) replaces the // parent list entirely. @@ -563,6 +585,30 @@ func (c *perRepoConfig) ConfigModelAliases() map[string]string { // an explicit false is distinguishable from unset (nil) across layers. func (c *perRepoConfig) SetKillSwitch(v bool) { c.KillSwitch = &v } +// SetAuthorizationOwnersFile enables or disables OWNERS-file authorization. +func (c *perRepoConfig) SetAuthorizationOwnersFile(v bool) { + if v { + for _, p := range c.Authorization { + if p.Provider == "owners_file" { + return + } + } + c.Authorization = append(c.Authorization, AuthorizationProvider{Provider: "owners_file"}) + } else { + filtered := make([]AuthorizationProvider, 0, len(c.Authorization)) + for _, p := range c.Authorization { + if p.Provider != "owners_file" { + filtered = append(filtered, p) + } + } + if len(filtered) == 0 { + c.Authorization = nil + } else { + c.Authorization = filtered + } + } +} + // SetAgents replaces the registered agent entries. func (c *perRepoConfig) SetAgents(agents []AgentEntry) { c.Agents = agents } diff --git a/internal/config/interfaces_test.go b/internal/config/interfaces_test.go index 435e1e164b..c025e5e269 100644 --- a/internal/config/interfaces_test.go +++ b/internal/config/interfaces_test.go @@ -891,3 +891,82 @@ func TestPerRepoConfig_InferenceOpenAI_Fallback(t *testing.T) { }) assert.Equal(t, []string{"identity_provider_id", "service_account_id"}, OpenAIWIFConfig{Audience: "a"}.Missing()) } + +// --- AuthorizationOwnersFile: intentionally no parent fallback --- + +func TestPerRepoConfig_AuthorizationOwnersFile_NoFallback(t *testing.T) { + t.Run("returns false when unset", func(t *testing.T) { + cfg := &perRepoConfig{} + assert.False(t, cfg.AuthorizationOwnersFile()) + }) + + t.Run("does not fall through to parent", func(t *testing.T) { + parent := &perRepoConfig{ + Authorization: []AuthorizationProvider{{Provider: "owners_file"}}, + } + child := &perRepoConfig{parent: parent} + assert.False(t, child.AuthorizationOwnersFile()) + }) + + t.Run("returns true when set locally", func(t *testing.T) { + cfg := &perRepoConfig{ + Authorization: []AuthorizationProvider{{Provider: "owners_file"}}, + } + assert.True(t, cfg.AuthorizationOwnersFile()) + }) +} + +func TestOrgConfig_AuthorizationOwnersFile(t *testing.T) { + cfg := &orgConfig{} + assert.False(t, cfg.AuthorizationOwnersFile()) + cfg.SetAuthorizationOwnersFile(true) + assert.False(t, cfg.AuthorizationOwnersFile()) +} + +func TestPerRepoConfig_SetAuthorizationOwnersFile(t *testing.T) { + t.Run("enable adds provider", func(t *testing.T) { + cfg := &perRepoConfig{} + cfg.SetAuthorizationOwnersFile(true) + assert.True(t, cfg.AuthorizationOwnersFile()) + assert.Len(t, cfg.Authorization, 1) + }) + + t.Run("enable is idempotent", func(t *testing.T) { + cfg := &perRepoConfig{} + cfg.SetAuthorizationOwnersFile(true) + cfg.SetAuthorizationOwnersFile(true) + assert.Len(t, cfg.Authorization, 1) + }) + + t.Run("disable removes provider", func(t *testing.T) { + cfg := &perRepoConfig{ + Authorization: []AuthorizationProvider{{Provider: "owners_file"}}, + } + cfg.SetAuthorizationOwnersFile(false) + assert.False(t, cfg.AuthorizationOwnersFile()) + assert.Nil(t, cfg.Authorization) + }) + + t.Run("disable preserves other providers", func(t *testing.T) { + cfg := &perRepoConfig{ + Authorization: []AuthorizationProvider{ + {Provider: "owners_file"}, + {Provider: "other"}, + }, + } + cfg.SetAuthorizationOwnersFile(false) + assert.False(t, cfg.AuthorizationOwnersFile()) + assert.Equal(t, []AuthorizationProvider{{Provider: "other"}}, cfg.Authorization) + }) + + t.Run("disable is no-op when not set", func(t *testing.T) { + cfg := &perRepoConfig{} + cfg.SetAuthorizationOwnersFile(false) + assert.Nil(t, cfg.Authorization) + }) +} + +func TestPerRepoDefaults_AuthorizationOwnersFile(t *testing.T) { + d := &perRepoDefaults{} + assert.False(t, d.AuthorizationOwnersFile()) +} diff --git a/internal/harnessdispatch/core.go b/internal/harnessdispatch/core.go index 42a2ec72ee..da0aea309c 100644 --- a/internal/harnessdispatch/core.go +++ b/internal/harnessdispatch/core.go @@ -3,14 +3,20 @@ package harnessdispatch import ( "context" "fmt" + "log" + "path/filepath" "github.com/fullsend-ai/fullsend/internal/config" "github.com/fullsend-ai/fullsend/internal/fetch" "github.com/fullsend-ai/fullsend/internal/normevent" + "github.com/fullsend-ai/fullsend/internal/owners" ) // Options configures a dispatch run. type Options struct { + // ConfigDir is the fullsend config directory (e.g. ".fullsend"). + // Must be a direct child of the repo root; filepath.Dir is used + // to locate OWNERS and OWNERS_ALIASES for authorization. ConfigDir string Event *normevent.Event @@ -38,7 +44,27 @@ func Dispatch(ctx context.Context, opts Options) ([]ExecutionRef, error) { return nil, nil } - if !IsAuthorized(opts.Event) { + // Compute the effective role for the auth gate without mutating the + // caller's event. The OWNERS-upgraded role is used only for + // IsAuthorized; downstream CEL evaluation sees the original + // collaborator-API role. + repoRoot := filepath.Dir(opts.ConfigDir) + effectiveRole := opts.Event.Actor.Role + if dirCfg.AuthorizationOwnersFile() && opts.Event.Actor.ID != "" { + ownersPath := filepath.Join(repoRoot, "OWNERS") + aliasesPath := filepath.Join(repoRoot, "OWNERS_ALIASES") + role, err := owners.Resolve(ownersPath, aliasesPath, opts.Event.Actor.ID) + if err != nil { + log.Printf("harness dispatch: OWNERS resolution failed for %s: %v", opts.Event.Actor.ID, err) + } else if role != owners.None { + effectiveRole = owners.MapToActorRole(role, effectiveRole) + log.Printf("harness dispatch: OWNERS file resolved user %s as %s", opts.Event.Actor.ID, role) + } + } + + authCheck := *opts.Event + authCheck.Actor.Role = effectiveRole + if !IsAuthorized(&authCheck) { return nil, nil } diff --git a/internal/harnessdispatch/core_test.go b/internal/harnessdispatch/core_test.go index da38168652..ab6161736a 100644 --- a/internal/harnessdispatch/core_test.go +++ b/internal/harnessdispatch/core_test.go @@ -87,6 +87,18 @@ func mustEvent(t *testing.T, name string) *normevent.Event { return ev } +func issueOpenedHarnessYAML() string { + return `agent: agents/triage.md +role: triage +slug: fullsend-ai-issue-triage +model: opus +image: ghcr.io/fullsend-ai/fullsend-sandbox:latest +trigger: | + event.entity.kind == "work_item" + && event.transition.kind == "opened" +` +} + func issuePingHarnessYAML() string { return `agent: agents/triage.md role: triage @@ -100,6 +112,82 @@ trigger: | ` } +func TestDispatch_OwnersUpgradesActorRole(t *testing.T) { + // Use issue-opened (not label-added) so IsAuthorized requires + // write-level access. Without the OWNERS approver upgrade the + // actor's RoleNone would be denied — this proves the OWNERS path + // is actually exercised. + ev := mustEvent(t, "issue-opened.json") + + dir := t.TempDir() + + configDir := writeHarnessConfigSubdir(t, dir, issueOpenedHarnessYAML(), func(cfg config.PerRepoConfigWriter) { + cfg.SetAuthorizationOwnersFile(true) + }) + ev.Actor.ID = "test-approver" + ev.Actor.Role = normevent.RoleNone + + require.NoError(t, os.WriteFile( + filepath.Join(dir, "OWNERS"), + []byte("approvers:\n - test-approver\n"), + 0o644, + )) + + refs, err := Dispatch(context.Background(), Options{ConfigDir: configDir, Event: ev}) + require.NoError(t, err) + require.Len(t, refs, 1, "OWNERS approver upgrade should grant write-level access") + // The caller's event must NOT be mutated — the OWNERS upgrade is + // used only for the auth gate, not leaked to downstream CEL or + // the caller. + assert.Equal(t, normevent.RoleNone, ev.Actor.Role) +} + +func TestDispatch_OwnersReviewerDeniedWriteLevel(t *testing.T) { + dir := t.TempDir() + + configDir := writeHarnessConfigSubdir(t, dir, issuePingHarnessYAML(), func(cfg config.PerRepoConfigWriter) { + cfg.SetAuthorizationOwnersFile(true) + }) + + ev := mustEvent(t, "issue-opened.json") + ev.Actor.ID = "test-reviewer" + ev.Actor.Role = normevent.RoleNone + + require.NoError(t, os.WriteFile( + filepath.Join(dir, "OWNERS"), + []byte("approvers: []\nreviewers:\n - test-reviewer\n"), + 0o644, + )) + + refs, err := Dispatch(context.Background(), Options{ConfigDir: configDir, Event: ev}) + require.NoError(t, err) + assert.Empty(t, refs, "OWNERS reviewer should be denied: triage-equivalent does not satisfy write-level harness auth") +} + +func TestDispatch_OwnersDisabledNoUpgrade(t *testing.T) { + // Use issue-opened (not label-added) so IsAuthorized requires + // write-level access. The OWNERS file lists the actor as an + // approver, but with the config flag off, OWNERS should not be + // consulted — the actor stays at RoleNone and is denied. + ev := mustEvent(t, "issue-opened.json") + + dir := t.TempDir() + + configDir := writeHarnessConfigSubdir(t, dir, issueOpenedHarnessYAML()) + ev.Actor.ID = "test-approver" + ev.Actor.Role = normevent.RoleNone + + require.NoError(t, os.WriteFile( + filepath.Join(dir, "OWNERS"), + []byte("approvers:\n - test-approver\n"), + 0o644, + )) + + refs, err := Dispatch(context.Background(), Options{ConfigDir: configDir, Event: ev}) + require.NoError(t, err) + assert.Empty(t, refs, "OWNERS auth is disabled — approver in OWNERS should not grant access") +} + func TestDispatch_NilEvent(t *testing.T) { _, err := Dispatch(context.Background(), Options{ConfigDir: t.TempDir()}) require.Error(t, err) @@ -112,14 +200,38 @@ func TestMergedConfigAgents_InvalidYAML(t *testing.T) { require.Error(t, err) } -func writeHarnessConfig(t *testing.T, dir, harnessYAML string) { +func writeHarnessConfig(t *testing.T, dir, harnessYAML string, opts ...func(config.PerRepoConfigWriter)) { t.Helper() harnessDir := filepath.Join(dir, "harness") require.NoError(t, os.MkdirAll(harnessDir, 0o755)) require.NoError(t, os.WriteFile(filepath.Join(harnessDir, "issue-ping.yaml"), []byte(harnessYAML), 0o644)) cfg := config.NewPerRepoConfig(nil, "fullsend-ai/demo") cfg.SetAgents([]config.AgentEntry{{Name: "issue-ping", Source: "harness/issue-ping.yaml"}}) + for _, opt := range opts { + opt(cfg) + } data, err := yaml.Marshal(cfg) require.NoError(t, err) require.NoError(t, os.WriteFile(filepath.Join(dir, "config.yaml"), data, 0o644)) } + +// writeHarnessConfigSubdir creates a .fullsend/ subdirectory inside +// repoRoot that mirrors the production layout and returns its path for +// use as ConfigDir. This ensures filepath.Dir(configDir) resolves to +// repoRoot, which is required for OWNERS-file resolution. +func writeHarnessConfigSubdir(t *testing.T, repoRoot, harnessYAML string, opts ...func(config.PerRepoConfigWriter)) string { + t.Helper() + configDir := filepath.Join(repoRoot, ".fullsend") + harnessDir := filepath.Join(configDir, "harness") + require.NoError(t, os.MkdirAll(harnessDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(harnessDir, "issue-ping.yaml"), []byte(harnessYAML), 0o644)) + cfg := config.NewPerRepoConfig(nil, "fullsend-ai/demo") + cfg.SetAgents([]config.AgentEntry{{Name: "issue-ping", Source: "harness/issue-ping.yaml"}}) + for _, opt := range opts { + opt(cfg) + } + data, err := yaml.Marshal(cfg) + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(configDir, "config.yaml"), data, 0o644)) + return configDir +} diff --git a/internal/owners/owners.go b/internal/owners/owners.go new file mode 100644 index 0000000000..4480bd8eca --- /dev/null +++ b/internal/owners/owners.go @@ -0,0 +1,118 @@ +package owners + +import ( + "fmt" + "os" + "regexp" + "strings" + + "gopkg.in/yaml.v3" + + "github.com/fullsend-ai/fullsend/internal/normevent" +) + +var validUsername = regexp.MustCompile(`^[a-zA-Z0-9-]+$`) + +// Role is the OWNERS-file role for a resolved user. +type Role int + +const ( + None Role = iota + Reviewer // triage-equivalent + Approver // write-equivalent +) + +func (r Role) String() string { + switch r { + case Reviewer: + return "reviewer" + case Approver: + return "approver" + default: + return "none" + } +} + +// ownersFile represents a root-level Prow OWNERS file with flat +// approvers/reviewers lists. v1 limitation: filters: blocks and +// nested per-directory OWNERS files are not supported. +type ownersFile struct { + Approvers []string `yaml:"approvers"` + Reviewers []string `yaml:"reviewers"` +} + +type aliasesFile struct { + Aliases map[string][]string `yaml:"aliases"` +} + +// Resolve checks whether username appears in the OWNERS file at +// ownersPath (directly or via aliases in aliasesPath). Returns +// Approver if the user is an approver, Reviewer if only a reviewer, +// or None if not listed. Matching is case-insensitive. +// +// A missing OWNERS file returns an error. A missing OWNERS_ALIASES +// file is not an error — alias resolution is skipped. +func Resolve(ownersPath, aliasesPath, username string) (Role, error) { + if !validUsername.MatchString(username) { + return None, nil + } + data, err := os.ReadFile(ownersPath) + if err != nil { + return None, fmt.Errorf("reading OWNERS: %w", err) + } + var owners ownersFile + if err := yaml.Unmarshal(data, &owners); err != nil { + return None, fmt.Errorf("parsing OWNERS: %w", err) + } + + var aliases aliasesFile + if aliasData, err := os.ReadFile(aliasesPath); err == nil { + if err := yaml.Unmarshal(aliasData, &aliases); err != nil { + return None, fmt.Errorf("parsing OWNERS_ALIASES: %w", err) + } + } + + if hasMember(owners.Approvers, username, aliases.Aliases) { + return Approver, nil + } + if hasMember(owners.Reviewers, username, aliases.Aliases) { + return Reviewer, nil + } + return None, nil +} + +// hasMember checks if username is in entries, either directly or by +// expanding alias names through the aliases map. +func hasMember(entries []string, username string, aliases map[string][]string) bool { + for _, entry := range entries { + if strings.EqualFold(entry, username) { + return true + } + if members, ok := aliases[entry]; ok { + for _, m := range members { + if strings.EqualFold(m, username) { + return true + } + } + } + } + return false +} + +// MapToActorRole upgrades currentRole based on the OWNERS role. +// Approver grants at least write; reviewer grants at least triage. +// Never downgrades — if the collaborator API already granted a +// higher role, it is preserved. +func MapToActorRole(role Role, currentRole normevent.ActorRole) normevent.ActorRole { + switch role { + case Approver: + if !normevent.IsWriteAuthorized(currentRole) { + return normevent.RoleWrite + } + case Reviewer: + if currentRole == normevent.RoleNone || currentRole == normevent.RoleExternal || currentRole == normevent.RoleRead { + return normevent.RoleTriage + } + } + return currentRole +} diff --git a/internal/owners/owners_test.go b/internal/owners/owners_test.go new file mode 100644 index 0000000000..8f81320cd4 --- /dev/null +++ b/internal/owners/owners_test.go @@ -0,0 +1,176 @@ +package owners + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/fullsend-ai/fullsend/internal/normevent" +) + +func writeFile(t *testing.T, dir, name, content string) string { + t.Helper() + p := filepath.Join(dir, name) + require.NoError(t, os.WriteFile(p, []byte(content), 0o644)) + return p +} + +func TestResolve(t *testing.T) { + t.Parallel() + + t.Run("direct approver", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers:\n - alice\nreviewers: []\n") + role, err := Resolve(op, filepath.Join(dir, "OWNERS_ALIASES"), "alice") + require.NoError(t, err) + assert.Equal(t, Approver, role) + }) + + t.Run("direct reviewer", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers: []\nreviewers:\n - bob\n") + role, err := Resolve(op, filepath.Join(dir, "OWNERS_ALIASES"), "bob") + require.NoError(t, err) + assert.Equal(t, Reviewer, role) + }) + + t.Run("alias approver", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers:\n - team-alpha\n") + ap := writeFile(t, dir, "OWNERS_ALIASES", "aliases:\n team-alpha:\n - carol\n - dave\n") + role, err := Resolve(op, ap, "carol") + require.NoError(t, err) + assert.Equal(t, Approver, role) + }) + + t.Run("alias reviewer", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers: []\nreviewers:\n - team-beta\n") + ap := writeFile(t, dir, "OWNERS_ALIASES", "aliases:\n team-beta:\n - eve\n") + role, err := Resolve(op, ap, "eve") + require.NoError(t, err) + assert.Equal(t, Reviewer, role) + }) + + t.Run("not listed", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers:\n - alice\nreviewers:\n - bob\n") + role, err := Resolve(op, filepath.Join(dir, "OWNERS_ALIASES"), "mallory") + require.NoError(t, err) + assert.Equal(t, None, role) + }) + + t.Run("case insensitive", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers:\n - alice\n") + role, err := Resolve(op, filepath.Join(dir, "OWNERS_ALIASES"), "Alice") + require.NoError(t, err) + assert.Equal(t, Approver, role) + }) + + t.Run("case insensitive alias", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers:\n - my-team\n") + ap := writeFile(t, dir, "OWNERS_ALIASES", "aliases:\n my-team:\n - Alice\n") + role, err := Resolve(op, ap, "alice") + require.NoError(t, err) + assert.Equal(t, Approver, role) + }) + + t.Run("missing OWNERS file is error", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + _, err := Resolve(filepath.Join(dir, "OWNERS"), filepath.Join(dir, "OWNERS_ALIASES"), "alice") + require.Error(t, err) + }) + + t.Run("missing OWNERS_ALIASES is not error", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers:\n - alice\n") + role, err := Resolve(op, filepath.Join(dir, "nonexistent"), "alice") + require.NoError(t, err) + assert.Equal(t, Approver, role) + }) + + t.Run("empty lists", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers: []\nreviewers: []\n") + role, err := Resolve(op, filepath.Join(dir, "OWNERS_ALIASES"), "alice") + require.NoError(t, err) + assert.Equal(t, None, role) + }) + + t.Run("malformed OWNERS", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "not: [valid: yaml: {{") + _, err := Resolve(op, filepath.Join(dir, "OWNERS_ALIASES"), "alice") + require.Error(t, err) + }) + + t.Run("invalid username returns None", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers:\n - $(whoami)\n") + role, err := Resolve(op, filepath.Join(dir, "OWNERS_ALIASES"), "$(whoami)") + require.NoError(t, err) + assert.Equal(t, None, role) + }) + + t.Run("approver takes precedence over reviewer", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers:\n - alice\nreviewers:\n - alice\n") + role, err := Resolve(op, filepath.Join(dir, "OWNERS_ALIASES"), "alice") + require.NoError(t, err) + assert.Equal(t, Approver, role) + }) +} + +func TestRoleString(t *testing.T) { + t.Parallel() + assert.Equal(t, "approver", Approver.String()) + assert.Equal(t, "reviewer", Reviewer.String()) + assert.Equal(t, "none", None.String()) +} + +func TestMapToActorRole(t *testing.T) { + t.Parallel() + + t.Run("approver upgrades none to write", func(t *testing.T) { + t.Parallel() + assert.Equal(t, normevent.RoleWrite, MapToActorRole(Approver, normevent.RoleNone)) + }) + + t.Run("approver does not downgrade admin", func(t *testing.T) { + t.Parallel() + assert.Equal(t, normevent.RoleAdmin, MapToActorRole(Approver, normevent.RoleAdmin)) + }) + + t.Run("reviewer upgrades none to triage", func(t *testing.T) { + t.Parallel() + assert.Equal(t, normevent.RoleTriage, MapToActorRole(Reviewer, normevent.RoleNone)) + }) + + t.Run("reviewer does not downgrade write", func(t *testing.T) { + t.Parallel() + assert.Equal(t, normevent.RoleWrite, MapToActorRole(Reviewer, normevent.RoleWrite)) + }) + + t.Run("none does not change role", func(t *testing.T) { + t.Parallel() + assert.Equal(t, normevent.RoleRead, MapToActorRole(None, normevent.RoleRead)) + }) +} diff --git a/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml b/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml index 80501fe487..591f00dbb1 100644 --- a/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml +++ b/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml @@ -1,5 +1,5 @@ --- -# lint-workflow-size: max-lines=612 +# lint-workflow-size: max-lines=682 # Dispatcher workflow that routes events to agent workflows based on stage. # Routing logic determines the stage from event context — the shim only # forwards the raw event. Adding a new stage requires only a case branch @@ -30,6 +30,18 @@ jobs: outputs: dispatched_count: ${{ steps.dispatch.outputs.dispatched_count }} steps: + - name: Checkout config and OWNERS files + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} + persist-credentials: false + allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} + sparse-checkout: | + .fullsend/config.yaml + /OWNERS + /OWNERS_ALIASES + sparse-checkout-cone-mode: false + - name: Determine stage id: route env: @@ -58,11 +70,63 @@ jobs: STAGE="" TRIGGER_SOURCE="" + # Check direct membership or alias membership in an OWNERS list. + # Used by has_repo_permission when OWNERS-file auth is enabled. + # yq errors are intentionally suppressed (&>/dev/null): a parse + # failure or missing key means "no match", falling through to + # the collaborator API — not a fail-open gate. + _owners_has_user() { + local key="${1}" user="${2}" + if yq -e ".${key}[] | select((. | downcase) == \"${user}\")" OWNERS &>/dev/null; then + return 0 + fi + if [[ -f "OWNERS_ALIASES" ]]; then + local entry + while IFS= read -r entry; do + [[ -z "${entry}" ]] && continue + [[ ! "${entry}" =~ ^[a-zA-Z0-9_-]+$ ]] && continue + if yq -e ".aliases.\"${entry}\"[] | select((. | downcase) == \"${user}\")" OWNERS_ALIASES &>/dev/null; then + return 0 + fi + done < <(yq ".${key}[]" OWNERS 2>/dev/null) + fi + return 1 + } + # Collaborator role_name vs min (write|triage). See #5223 / ADR 0054. # API resolves org membership regardless of visibility (gh-aw-mcpg#2862). has_repo_permission() { local username="${1:-}" min="${2:-write}" role api_err [[ -z "${username}" ]] && return 1 + + # OWNERS-file authorization (opt-in via authorization providers list in config.yaml). + # Approvers get write-equivalent access; reviewers get triage-equivalent. + # Safe: sparse-checkout pins to base branch SHA for PR-scoped events, + # so PR authors cannot self-authorize by adding themselves to OWNERS. + if [[ -f "OWNERS" && -f ".fullsend/config.yaml" ]]; then + if [[ "$(yq '(.authorization // []) | any_c(.provider == "owners_file")' .fullsend/config.yaml)" == "true" ]]; then + if [[ ! "${username}" =~ ^[a-zA-Z0-9-]+$ ]]; then + echo "::warning::OWNERS auth skipped: username '${username}' contains unexpected characters" >&2 + else + local lc_user="${username,,}" + case "${min}" in + write|triage) + if _owners_has_user approvers "${lc_user}"; then + echo "::notice::OWNERS file resolved user '${username}' as approver (requested: ${min})" + return 0 + fi + ;;& + triage) + if _owners_has_user reviewers "${lc_user}"; then + echo "::notice::OWNERS file resolved user '${username}' as reviewer (requested: ${min})" + return 0 + fi + ;; + esac + fi + fi + fi + api_err=$(mktemp) || { echo "::warning::Failed to create temp file for permission check of ${username}" >&2 return 1 diff --git a/internal/scaffold/workflow_call_alignment_test.go b/internal/scaffold/workflow_call_alignment_test.go index 43bf064e68..946b9d06da 100644 --- a/internal/scaffold/workflow_call_alignment_test.go +++ b/internal/scaffold/workflow_call_alignment_test.go @@ -645,6 +645,62 @@ func TestDispatchPerStageAuthorization(t *testing.T) { // Retro on PR close remains intentionally ungated (documented) assert.Regexp(t, `(?s)closed\)\s*\n\s+# Intentional ungated:.*\n\s+STAGE="retro"`, s) + + // OWNERS role→permission mapping: approvers in write|triage arm, + // reviewers in triage-only arm, connected by ;;& (pattern-retest). + // A ;& (unconditional fallthrough) would silently give reviewers + // write-level access — this assertion catches that. + assert.Regexp(t, `(?s)write\|triage\).*_owners_has_user approvers`, s, + "OWNERS approvers must be checked in the write|triage case arm") + assert.Regexp(t, `(?s);;&\s*\n\s+triage\).*_owners_has_user reviewers`, s, + "OWNERS reviewers must be in the triage-only arm after ;;& (not ;&)") + assert.Contains(t, s, `lc_user="${username,,}"`, + "OWNERS username comparison must be case-insensitive") + assert.Contains(t, s, `_owners_has_user approvers "${lc_user}"`, + "OWNERS approver check must use lowercased lc_user, not original username") + assert.Contains(t, s, `_owners_has_user reviewers "${lc_user}"`, + "OWNERS reviewer check must use lowercased lc_user, not original username") + assert.Regexp(t, `::notice::OWNERS file resolved user '\$\{username\}'`, s, + "OWNERS audit log must use original username casing, not lc_user") + }) + } +} + +// TestOwnersCheckoutRefPin validates that every checkout step whose +// sparse-checkout includes OWNERS files pins to base branch SHA for +// pull_request_review events. Without this, a PR author can add +// themselves to OWNERS in their branch and self-authorize on the +// pull_request_review dispatch path. +func TestOwnersCheckoutRefPin(t *testing.T) { + cases := []struct { + name string + content func(t *testing.T) []byte + }{ + {"reusable-dispatch.yml", loadRepoFile(".github/workflows/reusable-dispatch.yml")}, + {"scaffold/dispatch.yml", loadScaffoldFile(".github/workflows/dispatch.yml")}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + s := string(tc.content(t)) + // Split into sections by checkout step boundary. Each section + // starting with "uses: actions/checkout@" contains one step's + // with: block up to the next step or job boundary. + sections := regexp.MustCompile(`(?m)^[ \t]*- name:`).Split(s, -1) + + var ownersCheckouts int + for _, section := range sections { + if !strings.Contains(section, "actions/checkout@") { + continue + } + if !strings.Contains(section, "OWNERS") { + continue + } + ownersCheckouts++ + assert.Contains(t, section, "pull_request_review", + "checkout that sparse-checks-out OWNERS must pin ref for pull_request_review events") + } + require.NotZero(t, ownersCheckouts, + "should find at least one checkout step with OWNERS in sparse-checkout") }) } } diff --git a/pkg/behaviourtest/steps/artifacts.go b/pkg/behaviourtest/steps/artifacts.go index 95818482a1..206fa4bac0 100644 --- a/pkg/behaviourtest/steps/artifacts.go +++ b/pkg/behaviourtest/steps/artifacts.go @@ -12,6 +12,7 @@ import ( ) const issueOpenEvent = "issues" +const issueCommentEvent = "issue_comment" func triageWorkflowEvent(w *world.World) string { if w.TriageTriggerEvent != "" { diff --git a/pkg/behaviourtest/steps/cleanup.go b/pkg/behaviourtest/steps/cleanup.go index cef4bbedd9..003782a394 100644 --- a/pkg/behaviourtest/steps/cleanup.go +++ b/pkg/behaviourtest/steps/cleanup.go @@ -8,6 +8,7 @@ import ( "strings" "time" + "github.com/fullsend-ai/fullsend/internal/config" "github.com/fullsend-ai/fullsend/internal/forge" "github.com/fullsend-ai/fullsend/pkg/behaviourtest/world" ) @@ -47,6 +48,39 @@ func cleanupRetry(logf func(string, ...any), desc string, fn func() error) error return lastErr } +// ValidateSlotClean reads the repo slot's config and fails if a previous +// scenario left mutable state behind (e.g. kill switch on, OWNERS auth +// enabled). Called before every scenario so stale state is caught +// immediately rather than causing silent false positives downstream. +func ValidateSlotClean(w *world.World) error { + // Unit tests construct bare worlds with no SCM driver; + // there is no remote repo to validate in that case. + if w.SCM == nil || w.Org == "" || w.RepoName == "" { + return nil + } + cfgPath := filepath.Join(".fullsend", "config.yaml") + cfgData, err := w.SCM.GetFileContent(context.Background(), + w.Org, w.RepoName, cfgPath) + if err != nil { + return nil + } + cfg, err := config.ParsePerRepoConfigWriter(cfgData) + if err != nil { + return nil + } + var stale []string + if cfg.IsKillSwitchActive() { + stale = append(stale, "kill_switch is active") + } + if cfg.AuthorizationOwnersFile() { + stale = append(stale, "owners_file authorization is enabled") + } + if len(stale) > 0 { + return fmt.Errorf("repo slot has stale state from a previous scenario (cleanup likely failed): %s", strings.Join(stale, ", ")) + } + return nil +} + func CleanupScenario(w *world.World) { ctx := context.Background() @@ -244,6 +278,11 @@ func CleanupScenario(w *world.World) { } } + // --- OWNERS auth cleanup --- + if w.OwnersAuthActivated { + cleanupOwnersAuth(w) + } + // --- Reaction notification cleanup --- // Disable reaction notifications so the next scenario on this slot // is not affected by sticky config state. diff --git a/pkg/behaviourtest/steps/owners.go b/pkg/behaviourtest/steps/owners.go new file mode 100644 index 0000000000..d0dd8bf978 --- /dev/null +++ b/pkg/behaviourtest/steps/owners.go @@ -0,0 +1,317 @@ +package steps + +import ( + "context" + "fmt" + "path/filepath" + "strings" + "time" + + "github.com/cucumber/godog" + "github.com/fullsend-ai/fullsend/internal/config" + "github.com/fullsend-ai/fullsend/internal/forge" + "github.com/fullsend-ai/fullsend/pkg/behaviourtest/drivers/install" + scmgh "github.com/fullsend-ai/fullsend/pkg/behaviourtest/drivers/scm/github" + "github.com/fullsend-ai/fullsend/pkg/behaviourtest/world" +) + +func registerOwnersSteps(sc *godog.ScenarioContext) { + sc.Step(`^an OWNERS file listing the (bot|outsider) as (?:an? )?(approver|reviewer)(?: only)?$`, func(ctx context.Context, actor, role string) (context.Context, error) { + return ctx, givenActorInOwners(world.FromContext(ctx), actor, role) + }) + sc.Step(`^an OWNERS file with alias "([^"]+)" as approver$`, func(ctx context.Context, alias string) (context.Context, error) { + return ctx, givenOwnersFileWithAlias(world.FromContext(ctx), alias) + }) + sc.Step(`^an OWNERS_ALIASES file mapping "([^"]+)" to the (bot|outsider)$`, func(ctx context.Context, alias, actor string) (context.Context, error) { + return ctx, givenOwnersAliasesFile(world.FromContext(ctx), alias, actor) + }) + sc.Step(`^OWNERS authorization is enabled$`, func(ctx context.Context) (context.Context, error) { + return ctx, givenOwnersAuthEnabled(world.FromContext(ctx)) + }) + sc.Step(`^the triage workflow logs contain "([^"]+)"$`, func(ctx context.Context, needle string) (context.Context, error) { + return ctx, thenWorkflowLogsContain(world.FromContext(ctx), needle) + }) + sc.Step(`^the triage workflow logs do not contain "([^"]+)"$`, func(ctx context.Context, needle string) (context.Context, error) { + return ctx, thenWorkflowLogsDoNotContain(world.FromContext(ctx), needle) + }) + sc.Step(`^the (bot|outsider|write actor) opens an issue for OWNERS auth testing$`, func(ctx context.Context, actor string) (context.Context, error) { + return ctx, whenIssueOpenedForOwnersAuth(world.FromContext(ctx), actor) + }) + sc.Step(`^the outsider posts "([^"]+)" on the issue$`, func(ctx context.Context, command string) (context.Context, error) { + return ctx, whenOutsiderPostsCommand(world.FromContext(ctx), command) + }) + sc.Step(`^the dispatch run does not authorize via OWNERS$`, func(ctx context.Context) (context.Context, error) { + return ctx, thenDispatchRunDoesNotAuthorizeViaOwners(world.FromContext(ctx)) + }) +} + +func resolveActorLogin(w *world.World, actor string) (string, error) { + switch actor { + case "bot": + ghDriver, ok := w.SCM.(*scmgh.Driver) + if !ok { + return "", fmt.Errorf("OWNERS test requires GitHub SCM driver") + } + return ghDriver.Client.GetAuthenticatedUser(context.Background()) + case "outsider": + if err := requireOutsider(w); err != nil { + return "", err + } + return w.OutsiderLogin, nil + case "write actor": + if err := requireWriteActor(w); err != nil { + return "", err + } + return w.WriteLogin, nil + default: + return "", fmt.Errorf("unknown actor %q", actor) + } +} + +func commitFile(w *world.World, path, message, content string) error { + if err := w.SCM.CommitFile(context.Background(), + w.Org, w.RepoName, + path, message, []byte(content)); err != nil { + return fmt.Errorf("committing %s: %w", path, err) + } + return nil +} + +func givenActorInOwners(w *world.World, actor, role string) error { + login, err := resolveActorLogin(w, actor) + if err != nil { + return err + } + var content string + switch role { + case "approver": + content = fmt.Sprintf("approvers:\n - %s\nreviewers: []\n", login) + case "reviewer": + content = fmt.Sprintf("approvers: []\nreviewers:\n - %s\n", login) + default: + return fmt.Errorf("unknown OWNERS role %q", role) + } + if err := commitFile(w, "OWNERS", "behaviour: add OWNERS file for auth test", content); err != nil { + return err + } + w.OwnersAuthActivated = true + return nil +} + +func givenOwnersFileWithAlias(w *world.World, alias string) error { + if err := commitFile(w, "OWNERS", "behaviour: add OWNERS file for auth test", + fmt.Sprintf("approvers:\n - %s\n", alias)); err != nil { + return err + } + w.OwnersAuthActivated = true + return nil +} + +func givenOwnersAliasesFile(w *world.World, alias, actor string) error { + login, err := resolveActorLogin(w, actor) + if err != nil { + return err + } + if err := commitFile(w, "OWNERS_ALIASES", "behaviour: add OWNERS_ALIASES for auth test", + fmt.Sprintf("aliases:\n %s:\n - %s\n", alias, login)); err != nil { + return err + } + w.OwnersAuthActivated = true + return nil +} + +func givenOwnersAuthEnabled(w *world.World) error { + cfgPath := filepath.Join(".fullsend", "config.yaml") + cfgData, err := w.SCM.GetFileContent(context.Background(), + w.Org, w.RepoName, cfgPath) + if err != nil { + return fmt.Errorf("reading config: %w", err) + } + cfg, err := config.ParsePerRepoConfigWriter(cfgData) + if err != nil { + return fmt.Errorf("parsing config: %w", err) + } + cfg.SetAuthorizationOwnersFile(true) + merged, err := cfg.Marshal() + if err != nil { + return err + } + if err := w.SCM.CommitFile(context.Background(), + w.Org, w.RepoName, + cfgPath, "behaviour: enable OWNERS authorization", + merged); err != nil { + return fmt.Errorf("updating config: %w", err) + } + w.OwnersAuthActivated = true + return nil +} + +func whenIssueOpenedForOwnersAuth(w *world.World, actor string) error { + if w.RepoOwner == "" || w.RepoName == "" { + return fmt.Errorf("no repo configured; call 'Given the enrolled test repository' before creating issues") + } + scmDriver := w.SCM + switch actor { + case "outsider": + if err := requireOutsider(w); err != nil { + return err + } + scmDriver = w.OutsiderSCM + case "write actor": + if err := requireWriteActor(w); err != nil { + return err + } + scmDriver = w.WriteSCM + } + w.ScenarioStart = time.Now().Add(-issueOpenDrainSkewBuffer) + w.TriageTriggerEvent = issueOpenEvent + title := fmt.Sprintf("behaviour-owners-auth-%d", time.Now().UnixNano()) + body := "Behaviour test issue for OWNERS authorization path." + issue, err := scmDriver.CreateIssue(context.Background(), w.RepoOwner, w.RepoName, title, body) + if err != nil { + return err + } + w.IssueNumber = issue.Number + w.IssueTitle = title + return nil +} + +func thenWorkflowLogsContain(w *world.World, needle string) error { + logs, err := getWorkflowLogs(w) + if err != nil { + return err + } + if !strings.Contains(logs, needle) { + return fmt.Errorf("workflow logs do not contain %q", needle) + } + return nil +} + +func thenWorkflowLogsDoNotContain(w *world.World, needle string) error { + logs, err := getWorkflowLogs(w) + if err != nil { + return err + } + if strings.Contains(logs, needle) { + return fmt.Errorf("workflow logs unexpectedly contain %q", needle) + } + return nil +} + +func getWorkflowLogs(w *world.World) (string, error) { + if err := ensureTriageWorkflowComplete(w); err != nil { + return "", err + } + if w.WorkflowRun == nil { + return "", fmt.Errorf("no workflow run recorded") + } + return w.CI.GetRunLogs(context.Background(), + w.RepoOwner, w.RepoName, w.WorkflowRun.ID) +} + +func requireWriteActor(w *world.World) error { + if w.WriteSCM == nil { + return fmt.Errorf("TEST_ACTOR_WRITE_PAT not set") + } + return nil +} + +func requireOutsider(w *world.World) error { + if w.OutsiderSCM == nil { + return fmt.Errorf("TEST_ACTOR_OUTSIDER_PAT not set") + } + return nil +} + +func whenOutsiderPostsCommand(w *world.World, command string) error { + if err := requireOutsider(w); err != nil { + return err + } + if w.IssueNumber == 0 { + return fmt.Errorf("no issue created") + } + w.ScenarioStart = time.Now() + _, err := w.OutsiderSCM.AddComment(context.Background(), + w.RepoOwner, w.RepoName, w.IssueNumber, command) + return err +} + +func thenDispatchRunDoesNotAuthorizeViaOwners(w *world.World) error { + run, err := waitForDispatchRun(w) + if err != nil { + return err + } + logs, err := w.CI.GetRunLogs(context.Background(), + w.RepoOwner, w.RepoName, run.ID) + if err != nil { + return fmt.Errorf("fetching dispatch run logs: %w", err) + } + if strings.Contains(logs, "##[notice]OWNERS file resolved user") { + return fmt.Errorf("dispatch run %d (%s) logs unexpectedly contain OWNERS authorization", run.ID, run.HTMLURL) + } + return nil +} + +func waitForDispatchRun(w *world.World) (*forge.WorkflowRun, error) { + ctx := context.Background() + repo := w.RepoName + file := install.PerRepoTriageWorkflow + + run, err := w.CI.WaitForWorkflow(ctx, w.RepoOwner, repo, file, w.ScenarioStart, issueCommentEvent) + if err == nil { + return run, nil + } + + worldLogf(w, "dispatch run: retrying with skew buffer: %v", err) + buffered := w.ScenarioStart.Add(-issueOpenDrainSkewBuffer) + if retryRun, retryErr := w.CI.WaitForWorkflow(ctx, w.RepoOwner, repo, file, buffered, issueCommentEvent); retryErr == nil { + return retryRun, nil + } + return nil, fmt.Errorf("waiting for dispatch workflow (issue_comment): %w", err) +} + +func disableOwnersAuth(w *world.World) error { + cfgPath := filepath.Join(".fullsend", "config.yaml") + cfgData, err := w.SCM.GetFileContent(context.Background(), + w.Org, w.RepoName, cfgPath) + if err != nil { + return fmt.Errorf("reading config: %w", err) + } + cfg, err := config.ParsePerRepoConfigWriter(cfgData) + if err != nil { + return fmt.Errorf("parsing config: %w", err) + } + cfg.SetAuthorizationOwnersFile(false) + merged, err := cfg.Marshal() + if err != nil { + return err + } + if err := w.SCM.CommitFile(context.Background(), + w.Org, w.RepoName, + cfgPath, "behaviour: disable OWNERS authorization", + merged); err != nil { + return fmt.Errorf("updating config: %w", err) + } + return nil +} + +func cleanupOwnersAuth(w *world.World) { + ctx := context.Background() + owner := w.Org + repo := w.RepoName + + if err := disableOwnersAuth(w); err != nil { + worldLogf(w, "behaviour cleanup: disable OWNERS auth: %v", err) + } + + empty := []byte("approvers: []\nreviewers: []\n") + if err := w.SCM.CommitFile(ctx, owner, repo, + "OWNERS", "behaviour: clear OWNERS file", empty); err != nil { + worldLogf(w, "behaviour cleanup: remove OWNERS file: %v", err) + } + emptyAliases := []byte("aliases: {}\n") + if err := w.SCM.CommitFile(ctx, owner, repo, + "OWNERS_ALIASES", "behaviour: clear OWNERS_ALIASES file", emptyAliases); err != nil { + worldLogf(w, "behaviour cleanup: remove OWNERS_ALIASES file: %v", err) + } +} diff --git a/pkg/behaviourtest/steps/owners_test.go b/pkg/behaviourtest/steps/owners_test.go new file mode 100644 index 0000000000..1b795f66a6 --- /dev/null +++ b/pkg/behaviourtest/steps/owners_test.go @@ -0,0 +1,91 @@ +package steps + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/fullsend-ai/fullsend/internal/config" +) + +func TestAuthorizationOwnersFileRoundTrip(t *testing.T) { + t.Parallel() + + t.Run("enable then marshal", func(t *testing.T) { + t.Parallel() + input := []byte("version: \"1\"\nruntime: claude\n") + cfg, err := config.ParsePerRepoConfigWriter(input) + require.NoError(t, err) + cfg.SetAuthorizationOwnersFile(true) + out, err := cfg.Marshal() + require.NoError(t, err) + s := string(out) + assert.Contains(t, s, "authorization:") + assert.Contains(t, s, "provider: owners_file") + assert.Contains(t, s, "runtime: claude") + }) + + t.Run("enable is idempotent", func(t *testing.T) { + t.Parallel() + input := []byte("version: \"1\"\n") + cfg, err := config.ParsePerRepoConfigWriter(input) + require.NoError(t, err) + cfg.SetAuthorizationOwnersFile(true) + cfg.SetAuthorizationOwnersFile(true) + out, err := cfg.Marshal() + require.NoError(t, err) + assert.Contains(t, string(out), "provider: owners_file") + }) + + t.Run("disable removes authorization block", func(t *testing.T) { + t.Parallel() + input := []byte("version: \"1\"\n") + cfg, err := config.ParsePerRepoConfigWriter(input) + require.NoError(t, err) + cfg.SetAuthorizationOwnersFile(true) + cfg.SetAuthorizationOwnersFile(false) + out, err := cfg.Marshal() + require.NoError(t, err) + assert.NotContains(t, string(out), "authorization") + }) + + t.Run("parse existing authorization from YAML", func(t *testing.T) { + t.Parallel() + input := []byte("version: \"1\"\nauthorization:\n - provider: owners_file\n") + cfg, err := config.ParsePerRepoConfigWriter(input) + require.NoError(t, err) + assert.True(t, cfg.AuthorizationOwnersFile()) + out, err := cfg.Marshal() + require.NoError(t, err) + assert.Contains(t, string(out), "provider: owners_file") + }) + + t.Run("disable when never enabled is no-op", func(t *testing.T) { + t.Parallel() + input := []byte("version: \"1\"\nruntime: claude\n") + cfg, err := config.ParsePerRepoConfigWriter(input) + require.NoError(t, err) + cfg.SetAuthorizationOwnersFile(false) + out, err := cfg.Marshal() + require.NoError(t, err) + s := string(out) + assert.NotContains(t, s, "authorization") + assert.Contains(t, s, "runtime: claude") + }) + + t.Run("round-trip preserves other fields", func(t *testing.T) { + t.Parallel() + input := []byte("version: \"1\"\nruntime: claude\nkill_switch: false\nroles:\n - coder\n - reviewer\n") + cfg, err := config.ParsePerRepoConfigWriter(input) + require.NoError(t, err) + cfg.SetAuthorizationOwnersFile(true) + out, err := cfg.Marshal() + require.NoError(t, err) + s := string(out) + assert.Contains(t, s, "runtime: claude") + assert.Contains(t, s, "kill_switch: false") + assert.Contains(t, s, "- coder") + assert.Contains(t, s, "provider: owners_file") + }) +} diff --git a/pkg/behaviourtest/steps/registry.go b/pkg/behaviourtest/steps/registry.go index 1a2cf20493..82aa843b7c 100644 --- a/pkg/behaviourtest/steps/registry.go +++ b/pkg/behaviourtest/steps/registry.go @@ -18,4 +18,5 @@ func Register(sc *godog.ScenarioContext) { registerJiraPollSteps(sc) registerBranchSteps(sc) registerReactionSteps(sc) + registerOwnersSteps(sc) } diff --git a/pkg/behaviourtest/suite/init.go b/pkg/behaviourtest/suite/init.go index a5be4cb9a2..a7934b0d4f 100644 --- a/pkg/behaviourtest/suite/init.go +++ b/pkg/behaviourtest/suite/init.go @@ -39,6 +39,10 @@ func beforeScenario(ctx context.Context, tags []string, template *world.World) ( w := template.Clone() resetScenarioWorld(w) + if err := steps.ValidateSlotClean(w); err != nil { + return ctx, err + } + ctx = world.WithWorld(ctx, w) return ctx, nil } @@ -102,6 +106,7 @@ func resetScenarioWorld(w *world.World) { w.AllowedResourcesOriginal = nil w.AgentsOverridden = false w.AgentsOriginal = nil + w.OwnersAuthActivated = false w.JiraMockServer = nil w.JiraMockState = nil w.JiraConfigDir = "" diff --git a/pkg/behaviourtest/world/world.go b/pkg/behaviourtest/world/world.go index 3d351f8473..d3f2ed78f2 100644 --- a/pkg/behaviourtest/world/world.go +++ b/pkg/behaviourtest/world/world.go @@ -85,6 +85,20 @@ type World struct { // Nil when no driver is configured. Driver install.Driver + // OutsiderSCM is an SCM driver authenticated as fstest-outsider, a + // GitHub User with no collaborator access. Used by the OWNERS + // write-denial E2E scenario. Nil when TEST_ACTOR_OUTSIDER_PAT is + // not set. Shared across scenarios, never reset. + OutsiderSCM scm.Driver + OutsiderLogin string + + // WriteSCM is an SCM driver authenticated as a GitHub User with + // write-level collaborator access. Used by OWNERS fallthrough + // scenarios that need an actor passing the collaborator API but + // not listed in OWNERS. Nil when TEST_ACTOR_WRITE_PAT is not set. + WriteSCM scm.Driver + WriteLogin string + // KillSwitchActivated records whether this scenario activated the // repo-level kill switch. CleanupScenario uses this to deactivate // the switch so the next scenario on this slot is not affected. @@ -113,6 +127,11 @@ type World struct { AgentsOverridden bool AgentsOriginal []config.AgentEntry + // OwnersAuthActivated records whether this scenario committed an + // OWNERS file and/or added owners_file to the authorization providers in config.yaml. + // CleanupScenario removes both. + OwnersAuthActivated bool + // Jira mock state — set by the "Given a mock Jira server" step. JiraMockServer *httptest.Server JiraMockState *jiramock.State