refactor(executor): review fixes for the token permissions display
Corpus lookup: ERMITE_CONFORMANCE_DIR (fatal when wrong), else the main checkout next to the repository (skip), no .worktrees paths. Scope table gets a readAll column instead of the copilot-requests special case; the no-op ToLower calls are gone. The table test spells out every expected group; the corpus test checks the recorded steps and fails when the job does not run.
- commit
- bdd5903cc8e71b3417a73be3a4187563bccf125b
- author
- brice · · 2026-10-12 23:51 UTC
- parent
- 9a7971c
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
3 files · +112 -73
M internal/executor/corpus_test.go +35 -22
| @@ -20,7 +20,7 @@ import ( | |||
| 20 | 20 | // conclusions and their logs with the recordings made on github.com. They need | |
| 21 | 21 | // a checkout of ermite-conformance (the corpus and recordings directories), | |
| 22 | 22 | // found through ERMITE_CONFORMANCE_DIR or as a sibling of this repository; | |
| 23 | - | // they skip without it. | |
| 23 | + | // they skip without it (and fail when ERMITE_CONFORMANCE_DIR is wrong). | |
| 24 | 24 | ||
| 25 | 25 | func conformanceDir(t *testing.T) string { | |
| 26 | 26 | t.Helper() | |
| @@ -28,33 +28,40 @@ func conformanceDir(t *testing.T) string { | |||
| 28 | 28 | } | |
| 29 | 29 | ||
| 30 | 30 | // conformanceDirFor is conformanceDir for a checkout that has the recording of | |
| 31 | - | // scenario (any checkout with a corpus when scenario is empty): the scenario | |
| 32 | - | // may be on a branch that is not merged yet. | |
| 31 | + | // scenario (any checkout with a corpus when scenario is empty). The checkout is | |
| 32 | + | // ERMITE_CONFORMANCE_DIR when set (the test fails if it lacks the corpus or the | |
| 33 | + | // recording: a wrong path must not look like a pass), else the main checkout | |
| 34 | + | // ermite-conformance next to this repository (skipped when it has none). | |
| 33 | 35 | func conformanceDirFor(t *testing.T, scenario string) string { | |
| 34 | 36 | t.Helper() | |
| 35 | - | cands := []string{os.Getenv("ERMITE_CONFORMANCE_DIR")} | |
| 36 | - | wd, _ := os.Getwd() | |
| 37 | - | for d := wd; d != "/" && d != "."; d = filepath.Dir(d) { | |
| 38 | - | cands = append(cands, | |
| 39 | - | filepath.Join(d, "ermite-conformance", ".worktrees", "permissions"), | |
| 40 | - | filepath.Join(d, "ermite-conformance", ".worktrees", "corpus"), | |
| 41 | - | filepath.Join(d, "ermite-conformance")) | |
| 37 | + | has := func(dir string) (corpus, rec bool) { | |
| 38 | + | if st, err := os.Stat(filepath.Join(dir, "corpus")); err == nil && st.IsDir() { | |
| 39 | + | corpus = true | |
| 40 | + | } | |
| 41 | + | if scenario == "" { | |
| 42 | + | return corpus, true | |
| 43 | + | } | |
| 44 | + | _, err := os.Stat(filepath.Join(dir, "recordings", scenario+".json")) | |
| 45 | + | return corpus, err == nil | |
| 42 | 46 | } | |
| 43 | - | for _, c := range cands { | |
| 44 | - | if c == "" { | |
| 45 | - | continue | |
| 47 | + | if dir := os.Getenv("ERMITE_CONFORMANCE_DIR"); dir != "" { | |
| 48 | + | corpus, rec := has(dir) | |
| 49 | + | if !corpus { | |
| 50 | + | t.Fatalf("ERMITE_CONFORMANCE_DIR=%s has no corpus directory", dir) | |
| 46 | 51 | } | |
| 47 | - | if st, err := os.Stat(filepath.Join(c, "corpus")); err != nil || !st.IsDir() { | |
| 48 | - | continue | |
| 52 | + | if !rec { | |
| 53 | + | t.Fatalf("ERMITE_CONFORMANCE_DIR=%s has no recordings/%s.json", dir, scenario) | |
| 49 | 54 | } | |
| 50 | - | if scenario != "" { | |
| 51 | - | if _, err := os.Stat(filepath.Join(c, "recordings", scenario+".json")); err != nil { | |
| 52 | - | continue | |
| 53 | - | } | |
| 55 | + | return dir | |
| 56 | + | } | |
| 57 | + | wd, _ := os.Getwd() | |
| 58 | + | for d := wd; d != "/" && d != "."; d = filepath.Dir(d) { | |
| 59 | + | c := filepath.Join(d, "ermite-conformance") | |
| 60 | + | if corpus, rec := has(c); corpus && rec { | |
| 61 | + | return c | |
| 54 | 62 | } | |
| 55 | - | return c | |
| 56 | 63 | } | |
| 57 | - | t.Skip("ermite-conformance checkout not found (set ERMITE_CONFORMANCE_DIR)") | |
| 64 | + | t.Skip("ermite-conformance checkout not found next to this repository (set ERMITE_CONFORMANCE_DIR)") | |
| 58 | 65 | return "" | |
| 59 | 66 | } | |
| 60 | 67 | ||
| @@ -362,11 +369,17 @@ func TestCorpusPermissionsDisplay(t *testing.T) { | |||
| 362 | 369 | yaml := readFile(t, filepath.Join(dir, "corpus", "permissions-display", "tree", filepath.FromSlash(file))) | |
| 363 | 370 | for _, jr := range run.Jobs { | |
| 364 | 371 | t.Run(strings.TrimSuffix(filepath.Base(file), ".yml")+"/"+jr.Name, func(t *testing.T) { | |
| 372 | + | if len(jr.Steps) == 0 { | |
| 373 | + | t.Fatal("no steps in the recording") | |
| 374 | + | } | |
| 365 | 375 | want := permissionsGroup(jr.Steps[0].Log) | |
| 366 | 376 | if want == nil { | |
| 367 | 377 | t.Fatalf("no group in the recording: %v", jr.Steps[0].Log) | |
| 368 | 378 | } | |
| 369 | - | got, _ := runLocal(t, yaml, jr.Name, nil) | |
| 379 | + | got, res := runLocal(t, yaml, jr.Name, nil) | |
| 380 | + | if res.Conclusion != ConclusionSuccess || len(got.steps) == 0 { | |
| 381 | + | t.Fatalf("job did not run: %+v %v", res, got.names()) | |
| 382 | + | } | |
| 370 | 383 | if have := permissionsGroup(got.steps[0].Log); strings.Join(have, "\n") != strings.Join(want, "\n") { | |
| 371 | 384 | t.Errorf("got\n%s\nwant\n%s", strings.Join(have, "\n"), strings.Join(want, "\n")) | |
| 372 | 385 | } | |
| @@ -20,7 +20,7 @@ import ( | |||||
| 20 | // conclusions and their logs with the recordings made on github.com. They need | 20 | // conclusions and their logs with the recordings made on github.com. They need | ||
| 21 | // a checkout of ermite-conformance (the corpus and recordings directories), | 21 | // a checkout of ermite-conformance (the corpus and recordings directories), | ||
| 22 | // found through ERMITE_CONFORMANCE_DIR or as a sibling of this repository; | 22 | // found through ERMITE_CONFORMANCE_DIR or as a sibling of this repository; | ||
| 23 | - | // they skip without it. | 23 | + | // they skip without it (and fail when ERMITE_CONFORMANCE_DIR is wrong). |
| 24 | 24 | ||||
| 25 | func conformanceDir(t *testing.T) string { | 25 | func conformanceDir(t *testing.T) string { | ||
| 26 | t.Helper() | 26 | t.Helper() | ||
| @@ -28,33 +28,40 @@ func conformanceDir(t *testing.T) string { | |||||
| 28 | } | 28 | } | ||
| 29 | 29 | ||||
| 30 | // conformanceDirFor is conformanceDir for a checkout that has the recording of | 30 | // conformanceDirFor is conformanceDir for a checkout that has the recording of | ||
| 31 | - | // scenario (any checkout with a corpus when scenario is empty): the scenario | 31 | + | // scenario (any checkout with a corpus when scenario is empty). The checkout is |
| 32 | - | // may be on a branch that is not merged yet. | 32 | + | // ERMITE_CONFORMANCE_DIR when set (the test fails if it lacks the corpus or the |
| 33 | + | // recording: a wrong path must not look like a pass), else the main checkout | |||
| 34 | + | // ermite-conformance next to this repository (skipped when it has none). | |||
| 33 | func conformanceDirFor(t *testing.T, scenario string) string { | 35 | func conformanceDirFor(t *testing.T, scenario string) string { | ||
| 34 | t.Helper() | 36 | t.Helper() | ||
| 35 | - | cands := []string{os.Getenv("ERMITE_CONFORMANCE_DIR")} | 37 | + | has := func(dir string) (corpus, rec bool) { |
| 36 | - | wd, _ := os.Getwd() | 38 | + | if st, err := os.Stat(filepath.Join(dir, "corpus")); err == nil && st.IsDir() { |
| 37 | - | for d := wd; d != "/" && d != "."; d = filepath.Dir(d) { | 39 | + | corpus = true |
| 38 | - | cands = append(cands, | 40 | + | } |
| 39 | - | filepath.Join(d, "ermite-conformance", ".worktrees", "permissions"), | 41 | + | if scenario == "" { |
| 40 | - | filepath.Join(d, "ermite-conformance", ".worktrees", "corpus"), | 42 | + | return corpus, true |
| 41 | - | filepath.Join(d, "ermite-conformance")) | 43 | + | } |
| 44 | + | _, err := os.Stat(filepath.Join(dir, "recordings", scenario+".json")) | |||
| 45 | + | return corpus, err == nil | |||
| 42 | } | 46 | } | ||
| 43 | - | for _, c := range cands { | 47 | + | if dir := os.Getenv("ERMITE_CONFORMANCE_DIR"); dir != "" { |
| 44 | - | if c == "" { | 48 | + | corpus, rec := has(dir) |
| 45 | - | continue | 49 | + | if !corpus { |
| 50 | + | t.Fatalf("ERMITE_CONFORMANCE_DIR=%s has no corpus directory", dir) | |||
| 46 | } | 51 | } | ||
| 47 | - | if st, err := os.Stat(filepath.Join(c, "corpus")); err != nil || !st.IsDir() { | 52 | + | if !rec { |
| 48 | - | continue | 53 | + | t.Fatalf("ERMITE_CONFORMANCE_DIR=%s has no recordings/%s.json", dir, scenario) |
| 49 | } | 54 | } | ||
| 50 | - | if scenario != "" { | 55 | + | return dir |
| 51 | - | if _, err := os.Stat(filepath.Join(c, "recordings", scenario+".json")); err != nil { | 56 | + | } |
| 52 | - | continue | 57 | + | wd, _ := os.Getwd() |
| 53 | - | } | 58 | + | for d := wd; d != "/" && d != "."; d = filepath.Dir(d) { |
| 59 | + | c := filepath.Join(d, "ermite-conformance") | |||
| 60 | + | if corpus, rec := has(c); corpus && rec { | |||
| 61 | + | return c | |||
| 54 | } | 62 | } | ||
| 55 | - | return c | |||
| 56 | } | 63 | } | ||
| 57 | - | t.Skip("ermite-conformance checkout not found (set ERMITE_CONFORMANCE_DIR)") | 64 | + | t.Skip("ermite-conformance checkout not found next to this repository (set ERMITE_CONFORMANCE_DIR)") |
| 58 | return "" | 65 | return "" | ||
| 59 | } | 66 | } | ||
| 60 | 67 | ||||
| @@ -362,11 +369,17 @@ func TestCorpusPermissionsDisplay(t *testing.T) { | |||||
| 362 | yaml := readFile(t, filepath.Join(dir, "corpus", "permissions-display", "tree", filepath.FromSlash(file))) | 369 | yaml := readFile(t, filepath.Join(dir, "corpus", "permissions-display", "tree", filepath.FromSlash(file))) | ||
| 363 | for _, jr := range run.Jobs { | 370 | for _, jr := range run.Jobs { | ||
| 364 | t.Run(strings.TrimSuffix(filepath.Base(file), ".yml")+"/"+jr.Name, func(t *testing.T) { | 371 | t.Run(strings.TrimSuffix(filepath.Base(file), ".yml")+"/"+jr.Name, func(t *testing.T) { | ||
| 372 | + | if len(jr.Steps) == 0 { | |||
| 373 | + | t.Fatal("no steps in the recording") | |||
| 374 | + | } | |||
| 365 | want := permissionsGroup(jr.Steps[0].Log) | 375 | want := permissionsGroup(jr.Steps[0].Log) | ||
| 366 | if want == nil { | 376 | if want == nil { | ||
| 367 | t.Fatalf("no group in the recording: %v", jr.Steps[0].Log) | 377 | t.Fatalf("no group in the recording: %v", jr.Steps[0].Log) | ||
| 368 | } | 378 | } | ||
| 369 | - | got, _ := runLocal(t, yaml, jr.Name, nil) | 379 | + | got, res := runLocal(t, yaml, jr.Name, nil) |
| 380 | + | if res.Conclusion != ConclusionSuccess || len(got.steps) == 0 { | |||
| 381 | + | t.Fatalf("job did not run: %+v %v", res, got.names()) | |||
| 382 | + | } | |||
| 370 | if have := permissionsGroup(got.steps[0].Log); strings.Join(have, "\n") != strings.Join(want, "\n") { | 383 | if have := permissionsGroup(got.steps[0].Log); strings.Join(have, "\n") != strings.Join(want, "\n") { | ||
| 371 | t.Errorf("got\n%s\nwant\n%s", strings.Join(have, "\n"), strings.Join(want, "\n")) | 384 | t.Errorf("got\n%s\nwant\n%s", strings.Join(have, "\n"), strings.Join(want, "\n")) | ||
| 372 | } | 385 | } | ||
M internal/executor/job_test.go +52 -27
| @@ -696,6 +696,50 @@ func TestSetUpJobSelfHostedNamesTheRunner(t *testing.T) { | |||
| 696 | 696 | } | |
| 697 | 697 | } | |
| 698 | 698 | ||
| 699 | + | // The expected groups of the whole-token cases, as recorded on github.com | |
| 700 | + | // (recordings/permissions-display.json: p-read-all, p-write-all). | |
| 701 | + | const ( | |
| 702 | + | recordedReadAll = `Actions: read | |
| 703 | + | ArtifactMetadata: read | |
| 704 | + | Attestations: read | |
| 705 | + | Checks: read | |
| 706 | + | CodeQuality: read | |
| 707 | + | Contents: read | |
| 708 | + | Deployments: read | |
| 709 | + | Discussions: read | |
| 710 | + | Drives: read | |
| 711 | + | Issues: read | |
| 712 | + | Metadata: read | |
| 713 | + | Models: read | |
| 714 | + | Packages: read | |
| 715 | + | Pages: read | |
| 716 | + | PullRequests: read | |
| 717 | + | RepositoryProjects: read | |
| 718 | + | SecurityEvents: read | |
| 719 | + | Statuses: read | |
| 720 | + | VulnerabilityAlerts: read` | |
| 721 | + | recordedWriteAll = `Actions: write | |
| 722 | + | ArtifactMetadata: write | |
| 723 | + | Attestations: write | |
| 724 | + | Checks: write | |
| 725 | + | CodeQuality: write | |
| 726 | + | Contents: write | |
| 727 | + | CopilotRequests: write | |
| 728 | + | Deployments: write | |
| 729 | + | Discussions: write | |
| 730 | + | Drives: write | |
| 731 | + | Issues: write | |
| 732 | + | Metadata: read | |
| 733 | + | Models: read | |
| 734 | + | Packages: write | |
| 735 | + | Pages: write | |
| 736 | + | PullRequests: write | |
| 737 | + | RepositoryProjects: write | |
| 738 | + | SecurityEvents: write | |
| 739 | + | Statuses: write | |
| 740 | + | VulnerabilityAlerts: read` | |
| 741 | + | ) | |
| 742 | + | ||
| 699 | 743 | // The GITHUB_TOKEN Permissions group follows the effective permissions of the | |
| 700 | 744 | // job (the job's replace the workflow's), as recorded on github.com | |
| 701 | 745 | // (recordings/permissions-display.json). | |
| @@ -714,38 +758,19 @@ func TestSetUpJobPrintsTheTokenPermissionsGitHubGrants(t *testing.T) { | |||
| 714 | 758 | "Checks: read\nMetadata: read"}, | |
| 715 | 759 | {"job replaces workflow", "permissions:\n contents: write\n issues: write\n" + wfHead + " permissions:\n pull-requests: read\n" + steps, | |
| 716 | 760 | "Metadata: read\nPullRequests: read"}, | |
| 717 | - | {"job read-all over workflow map", "permissions:\n contents: write\n" + wfHead + " permissions: read-all\n" + steps, "read-all"}, | |
| 761 | + | {"job read-all over workflow map", "permissions:\n contents: write\n" + wfHead + " permissions: read-all\n" + steps, recordedReadAll}, | |
| 718 | 762 | {"workflow write-all, job narrows", "permissions: write-all\n" + wfHead + " permissions:\n contents: read\n" + steps, | |
| 719 | 763 | "Contents: read\nMetadata: read"}, | |
| 720 | - | {"write-all", "permissions: write-all\n" + wfHead + steps, "write-all"}, | |
| 764 | + | {"read-all", "permissions: read-all\n" + wfHead + steps, recordedReadAll}, | |
| 765 | + | {"write-all", "permissions: write-all\n" + wfHead + steps, recordedWriteAll}, | |
| 721 | 766 | {"metadata is not asked for", wfHead + " permissions:\n copilot-requests: write\n security-events: read\n pull-requests: write\n" + steps, | |
| 722 | 767 | "CopilotRequests: write\nMetadata: read\nPullRequests: write\nSecurityEvents: read"}, | |
| 723 | 768 | } { | |
| 724 | - | got := setupLog(t, tc.yaml, "j", nil) | |
| 725 | - | start := strings.Index(got, "##[group]GITHUB_TOKEN Permissions\n") | |
| 726 | - | end := strings.Index(got, "\n##[endgroup]") | |
| 727 | - | if start < 0 || end < 0 || !strings.Contains(got, "##[endgroup]\nSecret source: Actions\nCache mode: write") { | |
| 728 | - | t.Errorf("%s:\n%s", tc.name, got) | |
| 729 | - | continue | |
| 730 | - | } | |
| 731 | - | have := strings.Split(got[start+len("##[group]GITHUB_TOKEN Permissions\n"):end], "\n") | |
| 732 | - | switch tc.want { | |
| 733 | - | case "read-all": | |
| 734 | - | if len(have) != 19 || have[0] != "Actions: read" || strings.Contains(strings.Join(have, ","), "write") || strings.Contains(strings.Join(have, ","), "CopilotRequests") { | |
| 735 | - | t.Errorf("%s: %v", tc.name, have) | |
| 736 | - | } | |
| 737 | - | case "write-all": | |
| 738 | - | // recordings: p-write-all | |
| 739 | - | wantAll := "Actions: write,ArtifactMetadata: write,Attestations: write,Checks: write,CodeQuality: write,Contents: write,CopilotRequests: write," + | |
| 740 | - | "Deployments: write,Discussions: write,Drives: write,Issues: write,Metadata: read,Models: read,Packages: write,Pages: write,PullRequests: write," + | |
| 741 | - | "RepositoryProjects: write,SecurityEvents: write,Statuses: write,VulnerabilityAlerts: read" | |
| 742 | - | if strings.Join(have, ",") != wantAll { | |
| 743 | - | t.Errorf("%s: %v", tc.name, have) | |
| 744 | - | } | |
| 745 | - | default: | |
| 746 | - | if strings.Join(have, "\n") != tc.want { | |
| 747 | - | t.Errorf("%s:\ngot %v\nwant %q", tc.name, have, tc.want) | |
| 748 | - | } | |
| 769 | + | want := strings.Join([]string{"Current runner version: 'test'", "##[group]GITHUB_TOKEN Permissions", tc.want, | |
| 770 | + | "##[endgroup]", "Secret source: Actions", "Cache mode: write", "Prepare workflow directory", | |
| 771 | + | "Prepare all required actions", "Complete job name: j"}, "\n") | |
| 772 | + | if got := setupLog(t, tc.yaml, "j", nil); got != want { | |
| 773 | + | t.Errorf("%s:\ngot\n%s\nwant\n%s", tc.name, got, want) | |
| 749 | 774 | } | |
| 750 | 775 | } | |
| 751 | 776 | } | |
| @@ -696,6 +696,50 @@ func TestSetUpJobSelfHostedNamesTheRunner(t *testing.T) { | |||||
| 696 | } | 696 | } | ||
| 697 | } | 697 | } | ||
| 698 | 698 | ||||
| 699 | + | // The expected groups of the whole-token cases, as recorded on github.com | |||
| 700 | + | // (recordings/permissions-display.json: p-read-all, p-write-all). | |||
| 701 | + | const ( | |||
| 702 | + | recordedReadAll = `Actions: read | |||
| 703 | + | ArtifactMetadata: read | |||
| 704 | + | Attestations: read | |||
| 705 | + | Checks: read | |||
| 706 | + | CodeQuality: read | |||
| 707 | + | Contents: read | |||
| 708 | + | Deployments: read | |||
| 709 | + | Discussions: read | |||
| 710 | + | Drives: read | |||
| 711 | + | Issues: read | |||
| 712 | + | Metadata: read | |||
| 713 | + | Models: read | |||
| 714 | + | Packages: read | |||
| 715 | + | Pages: read | |||
| 716 | + | PullRequests: read | |||
| 717 | + | RepositoryProjects: read | |||
| 718 | + | SecurityEvents: read | |||
| 719 | + | Statuses: read | |||
| 720 | + | VulnerabilityAlerts: read` | |||
| 721 | + | recordedWriteAll = `Actions: write | |||
| 722 | + | ArtifactMetadata: write | |||
| 723 | + | Attestations: write | |||
| 724 | + | Checks: write | |||
| 725 | + | CodeQuality: write | |||
| 726 | + | Contents: write | |||
| 727 | + | CopilotRequests: write | |||
| 728 | + | Deployments: write | |||
| 729 | + | Discussions: write | |||
| 730 | + | Drives: write | |||
| 731 | + | Issues: write | |||
| 732 | + | Metadata: read | |||
| 733 | + | Models: read | |||
| 734 | + | Packages: write | |||
| 735 | + | Pages: write | |||
| 736 | + | PullRequests: write | |||
| 737 | + | RepositoryProjects: write | |||
| 738 | + | SecurityEvents: write | |||
| 739 | + | Statuses: write | |||
| 740 | + | VulnerabilityAlerts: read` | |||
| 741 | + | ) | |||
| 742 | + | ||||
| 699 | // The GITHUB_TOKEN Permissions group follows the effective permissions of the | 743 | // The GITHUB_TOKEN Permissions group follows the effective permissions of the | ||
| 700 | // job (the job's replace the workflow's), as recorded on github.com | 744 | // job (the job's replace the workflow's), as recorded on github.com | ||
| 701 | // (recordings/permissions-display.json). | 745 | // (recordings/permissions-display.json). | ||
| @@ -714,38 +758,19 @@ func TestSetUpJobPrintsTheTokenPermissionsGitHubGrants(t *testing.T) { | |||||
| 714 | "Checks: read\nMetadata: read"}, | 758 | "Checks: read\nMetadata: read"}, | ||
| 715 | {"job replaces workflow", "permissions:\n contents: write\n issues: write\n" + wfHead + " permissions:\n pull-requests: read\n" + steps, | 759 | {"job replaces workflow", "permissions:\n contents: write\n issues: write\n" + wfHead + " permissions:\n pull-requests: read\n" + steps, | ||
| 716 | "Metadata: read\nPullRequests: read"}, | 760 | "Metadata: read\nPullRequests: read"}, | ||
| 717 | - | {"job read-all over workflow map", "permissions:\n contents: write\n" + wfHead + " permissions: read-all\n" + steps, "read-all"}, | 761 | + | {"job read-all over workflow map", "permissions:\n contents: write\n" + wfHead + " permissions: read-all\n" + steps, recordedReadAll}, |
| 718 | {"workflow write-all, job narrows", "permissions: write-all\n" + wfHead + " permissions:\n contents: read\n" + steps, | 762 | {"workflow write-all, job narrows", "permissions: write-all\n" + wfHead + " permissions:\n contents: read\n" + steps, | ||
| 719 | "Contents: read\nMetadata: read"}, | 763 | "Contents: read\nMetadata: read"}, | ||
| 720 | - | {"write-all", "permissions: write-all\n" + wfHead + steps, "write-all"}, | 764 | + | {"read-all", "permissions: read-all\n" + wfHead + steps, recordedReadAll}, |
| 765 | + | {"write-all", "permissions: write-all\n" + wfHead + steps, recordedWriteAll}, | |||
| 721 | {"metadata is not asked for", wfHead + " permissions:\n copilot-requests: write\n security-events: read\n pull-requests: write\n" + steps, | 766 | {"metadata is not asked for", wfHead + " permissions:\n copilot-requests: write\n security-events: read\n pull-requests: write\n" + steps, | ||
| 722 | "CopilotRequests: write\nMetadata: read\nPullRequests: write\nSecurityEvents: read"}, | 767 | "CopilotRequests: write\nMetadata: read\nPullRequests: write\nSecurityEvents: read"}, | ||
| 723 | } { | 768 | } { | ||
| 724 | - | got := setupLog(t, tc.yaml, "j", nil) | 769 | + | want := strings.Join([]string{"Current runner version: 'test'", "##[group]GITHUB_TOKEN Permissions", tc.want, |
| 725 | - | start := strings.Index(got, "##[group]GITHUB_TOKEN Permissions\n") | 770 | + | "##[endgroup]", "Secret source: Actions", "Cache mode: write", "Prepare workflow directory", |
| 726 | - | end := strings.Index(got, "\n##[endgroup]") | 771 | + | "Prepare all required actions", "Complete job name: j"}, "\n") |
| 727 | - | if start < 0 || end < 0 || !strings.Contains(got, "##[endgroup]\nSecret source: Actions\nCache mode: write") { | 772 | + | if got := setupLog(t, tc.yaml, "j", nil); got != want { |
| 728 | - | t.Errorf("%s:\n%s", tc.name, got) | 773 | + | t.Errorf("%s:\ngot\n%s\nwant\n%s", tc.name, got, want) |
| 729 | - | continue | |||
| 730 | - | } | |||
| 731 | - | have := strings.Split(got[start+len("##[group]GITHUB_TOKEN Permissions\n"):end], "\n") | |||
| 732 | - | switch tc.want { | |||
| 733 | - | case "read-all": | |||
| 734 | - | if len(have) != 19 || have[0] != "Actions: read" || strings.Contains(strings.Join(have, ","), "write") || strings.Contains(strings.Join(have, ","), "CopilotRequests") { | |||
| 735 | - | t.Errorf("%s: %v", tc.name, have) | |||
| 736 | - | } | |||
| 737 | - | case "write-all": | |||
| 738 | - | // recordings: p-write-all | |||
| 739 | - | wantAll := "Actions: write,ArtifactMetadata: write,Attestations: write,Checks: write,CodeQuality: write,Contents: write,CopilotRequests: write," + | |||
| 740 | - | "Deployments: write,Discussions: write,Drives: write,Issues: write,Metadata: read,Models: read,Packages: write,Pages: write,PullRequests: write," + | |||
| 741 | - | "RepositoryProjects: write,SecurityEvents: write,Statuses: write,VulnerabilityAlerts: read" | |||
| 742 | - | if strings.Join(have, ",") != wantAll { | |||
| 743 | - | t.Errorf("%s: %v", tc.name, have) | |||
| 744 | - | } | |||
| 745 | - | default: | |||
| 746 | - | if strings.Join(have, "\n") != tc.want { | |||
| 747 | - | t.Errorf("%s:\ngot %v\nwant %q", tc.name, have, tc.want) | |||
| 748 | - | } | |||
| 749 | } | 774 | } | ||
| 750 | } | 775 | } | ||
| 751 | } | 776 | } | ||
M internal/executor/token_permissions.go +25 -24
| @@ -4,7 +4,6 @@ package executor | |||
| 4 | 4 | ||
| 5 | 5 | import ( | |
| 6 | 6 | "sort" | |
| 7 | - | "strings" | |
| 8 | 7 | ||
| 9 | 8 | "bnema.dev/ermite-actions/workflow" | |
| 10 | 9 | ) | |
| @@ -18,6 +17,8 @@ type tokenScope struct { | |||
| 18 | 17 | // writeAll is the level "write-all" grants: GitHub keeps models and | |
| 19 | 18 | // vulnerability-alerts at read, whatever the workflow asks. | |
| 20 | 19 | writeAll string | |
| 20 | + | // readAll is the level "read-all" grants, "" when it grants nothing. | |
| 21 | + | readAll string | |
| 21 | 22 | } | |
| 22 | 23 | ||
| 23 | 24 | // tokenScopes are the scopes GitHub lists in "GITHUB_TOKEN Permissions" for | |
| @@ -27,25 +28,25 @@ type tokenScope struct { | |||
| 27 | 28 | // write-all only (read-all grants nothing for it, and "read" is rejected). | |
| 28 | 29 | // "metadata" is not a scope a workflow can set (rejected); it is always read. | |
| 29 | 30 | var tokenScopes = []tokenScope{ | |
| 30 | - | {"actions", "Actions", "write"}, | |
| 31 | - | {"artifact-metadata", "ArtifactMetadata", "write"}, | |
| 32 | - | {"attestations", "Attestations", "write"}, | |
| 33 | - | {"checks", "Checks", "write"}, | |
| 34 | - | {"code-quality", "CodeQuality", "write"}, | |
| 35 | - | {"contents", "Contents", "write"}, | |
| 36 | - | {"copilot-requests", "CopilotRequests", "write"}, | |
| 37 | - | {"deployments", "Deployments", "write"}, | |
| 38 | - | {"discussions", "Discussions", "write"}, | |
| 39 | - | {"drives", "Drives", "write"}, | |
| 40 | - | {"issues", "Issues", "write"}, | |
| 41 | - | {"models", "Models", "read"}, | |
| 42 | - | {"packages", "Packages", "write"}, | |
| 43 | - | {"pages", "Pages", "write"}, | |
| 44 | - | {"pull-requests", "PullRequests", "write"}, | |
| 45 | - | {"repository-projects", "RepositoryProjects", "write"}, | |
| 46 | - | {"security-events", "SecurityEvents", "write"}, | |
| 47 | - | {"statuses", "Statuses", "write"}, | |
| 48 | - | {"vulnerability-alerts", "VulnerabilityAlerts", "read"}, | |
| 31 | + | {"actions", "Actions", "write", "read"}, | |
| 32 | + | {"artifact-metadata", "ArtifactMetadata", "write", "read"}, | |
| 33 | + | {"attestations", "Attestations", "write", "read"}, | |
| 34 | + | {"checks", "Checks", "write", "read"}, | |
| 35 | + | {"code-quality", "CodeQuality", "write", "read"}, | |
| 36 | + | {"contents", "Contents", "write", "read"}, | |
| 37 | + | {"copilot-requests", "CopilotRequests", "write", ""}, | |
| 38 | + | {"deployments", "Deployments", "write", "read"}, | |
| 39 | + | {"discussions", "Discussions", "write", "read"}, | |
| 40 | + | {"drives", "Drives", "write", "read"}, | |
| 41 | + | {"issues", "Issues", "write", "read"}, | |
| 42 | + | {"models", "Models", "read", "read"}, | |
| 43 | + | {"packages", "Packages", "write", "read"}, | |
| 44 | + | {"pages", "Pages", "write", "read"}, | |
| 45 | + | {"pull-requests", "PullRequests", "write", "read"}, | |
| 46 | + | {"repository-projects", "RepositoryProjects", "write", "read"}, | |
| 47 | + | {"security-events", "SecurityEvents", "write", "read"}, | |
| 48 | + | {"statuses", "Statuses", "write", "read"}, | |
| 49 | + | {"vulnerability-alerts", "VulnerabilityAlerts", "read", "read"}, | |
| 49 | 50 | } | |
| 50 | 51 | ||
| 51 | 52 | // permissionLine is one line of the group: "<Display>: <level>". | |
| @@ -80,11 +81,11 @@ func tokenPermissionLines(wf *workflow.Workflow, jb *workflow.Job) []permissionL | |||
| 80 | 81 | return []permissionLine{{"Contents", "read"}, {"Metadata", "read"}, {"Packages", "read"}} | |
| 81 | 82 | } | |
| 82 | 83 | lines := []permissionLine{{"Metadata", "read"}} | |
| 83 | - | switch strings.ToLower(p.All) { | |
| 84 | + | switch p.All { | |
| 84 | 85 | case "read-all": | |
| 85 | 86 | for _, s := range tokenScopes { | |
| 86 | - | if s.key != "copilot-requests" { | |
| 87 | - | lines = append(lines, permissionLine{s.display, "read"}) | |
| 87 | + | if s.readAll != "" { | |
| 88 | + | lines = append(lines, permissionLine{s.display, s.readAll}) | |
| 88 | 89 | } | |
| 89 | 90 | } | |
| 90 | 91 | case "write-all": | |
| @@ -94,7 +95,7 @@ func tokenPermissionLines(wf *workflow.Workflow, jb *workflow.Job) []permissionL | |||
| 94 | 95 | default: | |
| 95 | 96 | level := map[string]string{} | |
| 96 | 97 | for _, sc := range p.Scopes { | |
| 97 | - | level[strings.ToLower(sc.Scope)] = strings.ToLower(sc.Level) | |
| 98 | + | level[sc.Scope] = sc.Level | |
| 98 | 99 | } | |
| 99 | 100 | for _, s := range tokenScopes { | |
| 100 | 101 | if l := level[s.key]; l == "read" || l == "write" { | |
| @@ -4,7 +4,6 @@ package executor | |||||
| 4 | 4 | ||||
| 5 | import ( | 5 | import ( | ||
| 6 | "sort" | 6 | "sort" | ||
| 7 | - | "strings" | |||
| 8 | 7 | ||||
| 9 | "bnema.dev/ermite-actions/workflow" | 8 | "bnema.dev/ermite-actions/workflow" | ||
| 10 | ) | 9 | ) | ||
| @@ -18,6 +17,8 @@ type tokenScope struct { | |||||
| 18 | // writeAll is the level "write-all" grants: GitHub keeps models and | 17 | // writeAll is the level "write-all" grants: GitHub keeps models and | ||
| 19 | // vulnerability-alerts at read, whatever the workflow asks. | 18 | // vulnerability-alerts at read, whatever the workflow asks. | ||
| 20 | writeAll string | 19 | writeAll string | ||
| 20 | + | // readAll is the level "read-all" grants, "" when it grants nothing. | |||
| 21 | + | readAll string | |||
| 21 | } | 22 | } | ||
| 22 | 23 | ||||
| 23 | // tokenScopes are the scopes GitHub lists in "GITHUB_TOKEN Permissions" for | 24 | // tokenScopes are the scopes GitHub lists in "GITHUB_TOKEN Permissions" for | ||
| @@ -27,25 +28,25 @@ type tokenScope struct { | |||||
| 27 | // write-all only (read-all grants nothing for it, and "read" is rejected). | 28 | // write-all only (read-all grants nothing for it, and "read" is rejected). | ||
| 28 | // "metadata" is not a scope a workflow can set (rejected); it is always read. | 29 | // "metadata" is not a scope a workflow can set (rejected); it is always read. | ||
| 29 | var tokenScopes = []tokenScope{ | 30 | var tokenScopes = []tokenScope{ | ||
| 30 | - | {"actions", "Actions", "write"}, | 31 | + | {"actions", "Actions", "write", "read"}, |
| 31 | - | {"artifact-metadata", "ArtifactMetadata", "write"}, | 32 | + | {"artifact-metadata", "ArtifactMetadata", "write", "read"}, |
| 32 | - | {"attestations", "Attestations", "write"}, | 33 | + | {"attestations", "Attestations", "write", "read"}, |
| 33 | - | {"checks", "Checks", "write"}, | 34 | + | {"checks", "Checks", "write", "read"}, |
| 34 | - | {"code-quality", "CodeQuality", "write"}, | 35 | + | {"code-quality", "CodeQuality", "write", "read"}, |
| 35 | - | {"contents", "Contents", "write"}, | 36 | + | {"contents", "Contents", "write", "read"}, |
| 36 | - | {"copilot-requests", "CopilotRequests", "write"}, | 37 | + | {"copilot-requests", "CopilotRequests", "write", ""}, |
| 37 | - | {"deployments", "Deployments", "write"}, | 38 | + | {"deployments", "Deployments", "write", "read"}, |
| 38 | - | {"discussions", "Discussions", "write"}, | 39 | + | {"discussions", "Discussions", "write", "read"}, |
| 39 | - | {"drives", "Drives", "write"}, | 40 | + | {"drives", "Drives", "write", "read"}, |
| 40 | - | {"issues", "Issues", "write"}, | 41 | + | {"issues", "Issues", "write", "read"}, |
| 41 | - | {"models", "Models", "read"}, | 42 | + | {"models", "Models", "read", "read"}, |
| 42 | - | {"packages", "Packages", "write"}, | 43 | + | {"packages", "Packages", "write", "read"}, |
| 43 | - | {"pages", "Pages", "write"}, | 44 | + | {"pages", "Pages", "write", "read"}, |
| 44 | - | {"pull-requests", "PullRequests", "write"}, | 45 | + | {"pull-requests", "PullRequests", "write", "read"}, |
| 45 | - | {"repository-projects", "RepositoryProjects", "write"}, | 46 | + | {"repository-projects", "RepositoryProjects", "write", "read"}, |
| 46 | - | {"security-events", "SecurityEvents", "write"}, | 47 | + | {"security-events", "SecurityEvents", "write", "read"}, |
| 47 | - | {"statuses", "Statuses", "write"}, | 48 | + | {"statuses", "Statuses", "write", "read"}, |
| 48 | - | {"vulnerability-alerts", "VulnerabilityAlerts", "read"}, | 49 | + | {"vulnerability-alerts", "VulnerabilityAlerts", "read", "read"}, |
| 49 | } | 50 | } | ||
| 50 | 51 | ||||
| 51 | // permissionLine is one line of the group: "<Display>: <level>". | 52 | // permissionLine is one line of the group: "<Display>: <level>". | ||
| @@ -80,11 +81,11 @@ func tokenPermissionLines(wf *workflow.Workflow, jb *workflow.Job) []permissionL | |||||
| 80 | return []permissionLine{{"Contents", "read"}, {"Metadata", "read"}, {"Packages", "read"}} | 81 | return []permissionLine{{"Contents", "read"}, {"Metadata", "read"}, {"Packages", "read"}} | ||
| 81 | } | 82 | } | ||
| 82 | lines := []permissionLine{{"Metadata", "read"}} | 83 | lines := []permissionLine{{"Metadata", "read"}} | ||
| 83 | - | switch strings.ToLower(p.All) { | 84 | + | switch p.All { |
| 84 | case "read-all": | 85 | case "read-all": | ||
| 85 | for _, s := range tokenScopes { | 86 | for _, s := range tokenScopes { | ||
| 86 | - | if s.key != "copilot-requests" { | 87 | + | if s.readAll != "" { |
| 87 | - | lines = append(lines, permissionLine{s.display, "read"}) | 88 | + | lines = append(lines, permissionLine{s.display, s.readAll}) |
| 88 | } | 89 | } | ||
| 89 | } | 90 | } | ||
| 90 | case "write-all": | 91 | case "write-all": | ||
| @@ -94,7 +95,7 @@ func tokenPermissionLines(wf *workflow.Workflow, jb *workflow.Job) []permissionL | |||||
| 94 | default: | 95 | default: | ||
| 95 | level := map[string]string{} | 96 | level := map[string]string{} | ||
| 96 | for _, sc := range p.Scopes { | 97 | for _, sc := range p.Scopes { | ||
| 97 | - | level[strings.ToLower(sc.Scope)] = strings.ToLower(sc.Level) | 98 | + | level[sc.Scope] = sc.Level |
| 98 | } | 99 | } | ||
| 99 | for _, s := range tokenScopes { | 100 | for _, s := range tokenScopes { | ||
| 100 | if l := level[s.key]; l == "read" || l == "write" { | 101 | if l := level[s.key]; l == "read" || l == "write" { | ||