vole
Clone
Read only, no credentials:
https://rodent.software/git/vole.git
Write over SSH with a registered key:
git clone ssh://git@0.0.0.0:2222/vole.git
A free and open-source forge that lives in your Git repo.
Use flags for acl runner edits, not signs
96cbb6fab5aaaf79cc12f4c83877db22214890ca · Keenan Nemetz · 17d ago · parent 852c067d
Use flags for acl runner edits, not signs
The first cut of `acl runners` took `+name` to add and `none` to clear. That is a
local dialect, and the project's flag parser exists to prevent exactly that: git
edits a list with flags (`config --add`, `config --unset-all`, `remote set-url
--add`) or with a subcommand (`sparse-checkout add`), never with a sigil. The `+`
appeared because `-name` collides with short options — which is a reason to use
flags, not a reason to invent syntax.
vole acl runners <repo> # list
vole acl runners <repo> <name>... # replace, like `acl protect`
vole acl runners --add <repo> <name>... # add
vole acl runners --remove <repo> <name>... # remove
vole acl runners --clear <repo> # back to admins only
Two edits at once is refused rather than applied in sequence, and `--remove` fails
when the account is not on the list — `git config --unset` behaves the same way, and
a revocation that never happened should not look like one that did. The push refusal
that quotes the command now quotes this form.
Checked by hand against a throwaway instance as well as in tests, since the point of
the change is what the terminal says:
$ vole acl runners demo
runners: none named; only admins may report CI results for demo
admin 01M5N98MV0GKB48CAFBGGTMMJF keenan
$ vole acl runners --add demo wilbur
runner 01M5N98MVK8Q3S5S6D0DQJ8QG2 wilbur
(admins may also report CI results)
$ vole acl runners --add --clear demo wilbur
vole: use one of --add, --remove or --clear (exit 129)
$ vole acl runners --remove demo keenan
vole: keenan is not a runner for demo (exit 1)
scripts/ci.sh green; TestACLRunnersEditsTheList now covers --add (including a repeat
that must not duplicate), replacement, --remove of an absent account, --clear, and the
two-edits-at-once refusal.
internal/server/integration_test.go +43−9
| @@ -2904,7 +2904,7 @@ func TestOnlyAllowedRunnersGreenLightABranch(t *testing.T) { | |||
| 2904 | 2904 | } | |
| 2905 | 2905 | ||
| 2906 | 2906 | // Allow the account, through the command a host admin would use. | |
| 2907 | out, errs, err := f.cli("", "acl", "runners", f.repo, "+wilbur", "--config", f.cfgFile(t)) | ||
| 2907 | out, errs, err := f.cli("", "acl", "runners", "--add", f.repo, "wilbur", "--config", f.cfgFile(t)) | ||
| 2908 | 2908 | if err != nil { | |
| 2909 | 2909 | t.Fatalf("acl runners: %v\n%s", err, errs) | |
| 2910 | 2910 | } | |
| @@ -2941,7 +2941,7 @@ func TestOnlyAllowedRunnersGreenLightABranch(t *testing.T) { | |||
| 2941 | 2941 | } | |
| 2942 | 2942 | ||
| 2943 | 2943 | // Allowing it again is enough: nothing else changed, and nothing was lost. | |
| 2944 | if _, errs, err := f.cli("", "acl", "runners", f.repo, "+wilbur", "--config", f.cfgFile(t)); err != nil { | ||
| 2944 | if _, errs, err := f.cli("", "acl", "runners", "--add", f.repo, "wilbur", "--config", f.cfgFile(t)); err != nil { | ||
| 2945 | 2945 | t.Fatalf("acl runners: %v\n%s", err, errs) | |
| 2946 | 2946 | } | |
| 2947 | 2947 | if _, errs, err := f.gitWithKey(writerKey, feature, "push", f.sshURL(f.repo), "HEAD:refs/heads/main"); err != nil { | |
| @@ -2965,13 +2965,21 @@ func TestACLRunnersEditsTheList(t *testing.T) { | |||
| 2965 | 2965 | return a.Runners | |
| 2966 | 2966 | } | |
| 2967 | 2967 | ||
| 2968 | if out, errs, err := f.cli("", "acl", "runners", f.repo, "+wilbur", "--config", cfg); err != nil { | ||
| 2968 | if out, errs, err := f.cli("", "acl", "runners", "--add", f.repo, "wilbur", "--config", cfg); err != nil { | ||
| 2969 | 2969 | t.Fatalf("add: %v\n%s", err, errs) | |
| 2970 | 2970 | } else if !strings.Contains(out, "wilbur") { | |
| 2971 | 2971 | t.Fatalf("add did not report the account:\n%s", out) | |
| 2972 | 2972 | } | |
| 2973 | 2973 | if got := runners(); len(got) != 1 || got[0] != f.accountID("wilbur") { | |
| 2974 | t.Fatalf("runners after +wilbur = %#v", got) | ||
| 2974 | t.Fatalf("runners after --add wilbur = %#v", got) | ||
| 2975 | } | ||
| 2976 | |||
| 2977 | // Adding again is a no-op rather than a duplicate in the file. | ||
| 2978 | if _, errs, err := f.cli("", "acl", "runners", "--add", f.repo, "wilbur", "--config", cfg); err != nil { | ||
| 2979 | t.Fatalf("second add: %v\n%s", err, errs) | ||
| 2980 | } | ||
| 2981 | if got := runners(); len(got) != 1 { | ||
| 2982 | t.Fatalf("a repeated --add duplicated the account: %#v", got) | ||
| 2975 | 2983 | } | |
| 2976 | 2984 | ||
| 2977 | 2985 | // The owner is an admin, so they already count; naming them is legal but adds | |
| @@ -2990,18 +2998,44 @@ func TestACLRunnersEditsTheList(t *testing.T) { | |||
| 2990 | 2998 | t.Fatalf("runners after replacement = %#v", got) | |
| 2991 | 2999 | } | |
| 2992 | 3000 | ||
| 2993 | // Revoking trust is a replacement, not an inverse operation, and `none` is how | ||
| 2994 | // "back to admins only" is said — the same word `acl set` uses for no role. | ||
| 2995 | if _, errs, err := f.cli("", "acl", "runners", f.repo, "none", "--config", cfg); err != nil { | ||
| 3001 | // Removing somebody who is not on the list is an error, the way `git config | ||
| 3002 | // --unset` refuses a key that is not set: a revocation that never happened | ||
| 3003 | // should not look like one that did. | ||
| 3004 | if _, errs, err := f.cli("", "acl", "runners", "--remove", f.repo, "wilbur", "--config", cfg); err == nil { | ||
| 3005 | t.Fatalf("removing a non-runner was accepted:\n%s", errs) | ||
| 3006 | } else if !strings.Contains(errs, "is not a runner for") { | ||
| 3007 | t.Fatalf("the refusal does not say what happened:\n%s", errs) | ||
| 3008 | } | ||
| 3009 | if got := runners(); len(got) != 1 { | ||
| 3010 | t.Fatalf("a refused removal changed the list: %#v", got) | ||
| 3011 | } | ||
| 3012 | |||
| 3013 | // Removal is a flag, like `git config --unset`. | ||
| 3014 | if _, errs, err := f.cli("", "acl", "runners", "--remove", f.repo, "keenan", "--config", cfg); err != nil { | ||
| 3015 | t.Fatalf("remove: %v\n%s", err, errs) | ||
| 3016 | } | ||
| 3017 | if got := runners(); len(got) != 0 { | ||
| 3018 | t.Fatalf("runners after --remove = %#v", got) | ||
| 3019 | } | ||
| 3020 | |||
| 3021 | // And --clear returns to admins-only, which is where a new repository starts. | ||
| 3022 | if _, errs, err := f.cli("", "acl", "runners", "--add", f.repo, "wilbur", "--config", cfg); err != nil { | ||
| 3023 | t.Fatalf("re-add: %v\n%s", err, errs) | ||
| 3024 | } | ||
| 3025 | if _, errs, err := f.cli("", "acl", "runners", "--clear", f.repo, "--config", cfg); err != nil { | ||
| 2996 | 3026 | t.Fatalf("clear: %v\n%s", err, errs) | |
| 2997 | 3027 | } | |
| 2998 | 3028 | if got := runners(); len(got) != 0 { | |
| 2999 | t.Fatalf("runners after none = %#v", got) | ||
| 3029 | t.Fatalf("runners after --clear = %#v", got) | ||
| 3030 | } | ||
| 3031 | // Two edits at once is a mistake, not a sequence. | ||
| 3032 | if _, errs, err := f.cli("", "acl", "runners", "--add", "--clear", f.repo, "wilbur", "--config", cfg); err == nil { | ||
| 3033 | t.Fatalf("--add --clear was accepted:\n%s", errs) | ||
| 3000 | 3034 | } | |
| 3001 | 3035 | ||
| 3002 | 3036 | // An unknown account is refused rather than written as a typo that silently | |
| 3003 | 3037 | // permits nobody. | |
| 3004 | _, errs, err := f.cli("", "acl", "runners", f.repo, "+nobody", "--config", cfg) | ||
| 3038 | _, errs, err := f.cli("", "acl", "runners", "--add", f.repo, "nobody", "--config", cfg) | ||
| 3005 | 3039 | if err == nil { | |
| 3006 | 3040 | t.Fatalf("an unknown account was accepted:\n%s", errs) | |
| 3007 | 3041 | } | |