vole
Clone
Read only, no credentials:
https://rodent.software/vole.git
Write over SSH with a registered key:
git clone ssh://git@rodent.software:2222/vole.git
A free and open-source forge that lives in your Git repo.
Make access edits and approvals survive a concurrent change
62b59a76f9064a58a45d0997f6889d2a7f6265e7 · Keenan Nemetz · 13d ago · parent 78cd3437
Make access edits and approvals survive a concurrent change
Access rules were one row, and every editor read it, changed a field, and
wrote the whole document back with no version check. Two people on the
page at once silently overwrote each other, and the specific case that
mattered was worse than a lost edit: a repository admin's stale settings
save could restore a role an instance admin had just revoked.
The row now carries a revision, and a rule set read through the store
carries the revision it was read at. Writing it back is a
compare-and-swap: if the row moved, the write is refused with ErrConflict
instead of undoing the other change. A rule set built from scratch is a
create and refuses if a row appeared meanwhile, so no stale document can
replace a live one. Existing instances get the column added on open.
On top of that, the operations that own one field apply it to the row as
it is now (Store.Update, retried against a moving row):
- the repository Settings page writes only the visibility. It no longer
rewrites entries, runners or the approval policy, so an instance
admin's revocation survives a page that was loaded before it.
- adding one account writes that one entry.
- the access form changes only the rows it rendered; an entry or a
runners-list membership added while the page was open is kept.
Approvals and runners are also bound to current standing. An approval
counts only while its author is an active account that still holds
maintain or admin on the repository, filtered in FindApprovedMatch and
counted the same way on the request page, so the page cannot promise a
merge the gate then refuses. ACL.CanReport refuses a retired account even
while its name is in the runners list; the row outlives the account, so
membership alone would let a revoked account keep vouching for builds and
leasing jobs. The roster's Active predicate is attached by the store, the
same way the instance role already was, in both the server and the hook
process.
Tests: a stale whole-document write is refused and cannot restore a
revoked role; concurrent field-scoped updates all land; Update reports
the row as it is now; a retired account stops counting as an approver and
as a runner; a demoted maintainer's approval no longer lands a change; a
settings save leaves entries, runners, approval policy and protection
alone; a retired runner's record push is refused.
forge/access/store_test.go +187−1
| @@ -4,9 +4,12 @@ package access | |||
| 4 | 4 | // two things the file-based version gave for free: a repository with no rules denies | |
| 5 | 5 | // everything, and a change leaves a trace. | |
| 6 | 6 | import ( | |
| 7 | "errors" | ||
| 8 | "fmt" | ||
| 7 | 9 | "os" | |
| 8 | 10 | "path/filepath" | |
| 9 | 11 | "strings" | |
| 12 | "sync" | ||
| 10 | 13 | "testing" | |
| 11 | 14 | "time" | |
| 12 | 15 | ) | |
| @@ -47,8 +50,16 @@ func TestRulesRoundTripAndReplace(t *testing.T) { | |||
| 47 | 50 | len(got.Protected) != 1 { | |
| 48 | 51 | t.Fatalf("rules came back different: %+v", got) | |
| 49 | 52 | } | |
| 50 | // A replacement is a replacement: nothing of the old list survives. | ||
| 53 | // A blind replacement is refused: a rule set that was not read from the row | ||
| 54 | // cannot know what it is overwriting, so it could restore a role somebody just | ||
| 55 | // revoked. | ||
| 56 | if err := s.Set("repo-1", Empty("01M5OWNER0000000000000000A"), "keenan", time.Now().UTC()); !errors.Is(err, ErrConflict) { | ||
| 57 | t.Fatalf("a blind replacement = %v, want ErrConflict", err) | ||
| 58 | } | ||
| 59 | // A replacement through the rule set that was read is allowed, and nothing of | ||
| 60 | // the old list survives it. | ||
| 51 | 61 | next := Empty("01M5OWNER0000000000000000A") | |
| 62 | next.revision = got.revision | ||
| 52 | 63 | if err := s.Set("repo-1", next, "keenan", time.Now().UTC()); err != nil { | |
| 53 | 64 | t.Fatal(err) | |
| 54 | 65 | } | |
| @@ -187,3 +198,178 @@ func TestAuditFieldsCannotInjectColumns(t *testing.T) { | |||
| 187 | 198 | t.Fatalf("injected field wrote %d columns: %q", got, raw) | |
| 188 | 199 | } | |
| 189 | 200 | } | |
| 201 | |||
| 202 | // The race the settings page used to lose: a repository admin's page was loaded | ||
| 203 | // while they were still an admin, an instance admin revokes that role, and the | ||
| 204 | // stale save then wrote the whole rule set back and restored it. A write against a | ||
| 205 | // rule set that was read before somebody else's change is now refused, so the | ||
| 206 | // revocation wins. | ||
| 207 | func TestARepoAdminSaveCannotRestoreARevokedRole(t *testing.T) { | ||
| 208 | t.Parallel() | ||
| 209 | s, _ := open(t) | ||
| 210 | owner := "01M5OWNER0000000000000000A" | ||
| 211 | if err := s.Set("repo-1", Empty(owner), "keenan", time.Now().UTC()); err != nil { | ||
| 212 | t.Fatal(err) | ||
| 213 | } | ||
| 214 | // Two readers load the same revision; the second is about to save a rule set | ||
| 215 | // that says somebody is an admin. | ||
| 216 | first, err := s.Get("repo-1") | ||
| 217 | if err != nil { | ||
| 218 | t.Fatal(err) | ||
| 219 | } | ||
| 220 | stale, err := s.Get("repo-1") | ||
| 221 | if err != nil { | ||
| 222 | t.Fatal(err) | ||
| 223 | } | ||
| 224 | if err := first.SetEntry("eve", "admin"); err != nil { | ||
| 225 | t.Fatal(err) | ||
| 226 | } | ||
| 227 | if err := s.Set("repo-1", first, "keenan", time.Now().UTC()); err != nil { | ||
| 228 | t.Fatal(err) | ||
| 229 | } | ||
| 230 | // The revocation lands. | ||
| 231 | if _, err := s.Update("repo-1", "root", func(a *ACL) error { | ||
| 232 | return a.SetEntry("eve", "none") | ||
| 233 | }); err != nil { | ||
| 234 | t.Fatal(err) | ||
| 235 | } | ||
| 236 | // The stale save cannot restore it. | ||
| 237 | if err := s.Set("repo-1", stale, "keenan", time.Now().UTC()); !errors.Is(err, ErrConflict) { | ||
| 238 | t.Fatalf("a stale write = %v, want ErrConflict", err) | ||
| 239 | } | ||
| 240 | got, err := s.Get("repo-1") | ||
| 241 | if err != nil { | ||
| 242 | t.Fatal(err) | ||
| 243 | } | ||
| 244 | if got.RoleFor("eve") == Admin { | ||
| 245 | t.Fatalf("a stale write restored a revoked role: %+v", got.Entries) | ||
| 246 | } | ||
| 247 | } | ||
| 248 | |||
| 249 | // Update changes the row as it is now, so a change to one field cannot undo a | ||
| 250 | // concurrent change to another. | ||
| 251 | func TestUpdateKeepsAConcurrentChange(t *testing.T) { | ||
| 252 | t.Parallel() | ||
| 253 | s, _ := open(t) | ||
| 254 | if _, err := s.Update("repo-1", "keenan", func(a *ACL) error { | ||
| 255 | return a.SetEntry("owner", "admin") | ||
| 256 | }); err != nil { | ||
| 257 | t.Fatal(err) | ||
| 258 | } | ||
| 259 | // Both writers loaded nothing in particular; each applies the field it owns. | ||
| 260 | if _, err := s.Update("repo-1", "keenan", func(a *ACL) error { | ||
| 261 | return a.SetEntry("alice", "maintain") | ||
| 262 | }); err != nil { | ||
| 263 | t.Fatal(err) | ||
| 264 | } | ||
| 265 | if _, err := s.Update("repo-1", "keenan", func(a *ACL) error { | ||
| 266 | return a.SetEntry("bob", "read") | ||
| 267 | }); err != nil { | ||
| 268 | t.Fatal(err) | ||
| 269 | } | ||
| 270 | got, err := s.Get("repo-1") | ||
| 271 | if err != nil { | ||
| 272 | t.Fatal(err) | ||
| 273 | } | ||
| 274 | for name, want := range map[string]Role{"owner": Admin, "alice": Maintain, "bob": Read} { | ||
| 275 | if got.RoleFor(name) != want { | ||
| 276 | t.Fatalf("%s = %v, want %v (entries %+v)", name, got.RoleFor(name), want, got.Entries) | ||
| 277 | } | ||
| 278 | } | ||
| 279 | } | ||
| 280 | |||
| 281 | // Concurrent field-scoped edits all survive: the store retries the compare-and-swap | ||
| 282 | // instead of dropping the losers. | ||
| 283 | func TestConcurrentUpdatesAllLand(t *testing.T) { | ||
| 284 | t.Parallel() | ||
| 285 | s, _ := open(t) | ||
| 286 | const writers = 8 | ||
| 287 | var wg sync.WaitGroup | ||
| 288 | errs := make(chan error, writers) | ||
| 289 | for i := 0; i < writers; i++ { | ||
| 290 | wg.Add(1) | ||
| 291 | go func(i int) { | ||
| 292 | defer wg.Done() | ||
| 293 | name := fmt.Sprintf("account-%d", i) | ||
| 294 | _, err := s.Update("repo-1", "keenan", func(a *ACL) error { | ||
| 295 | return a.SetEntry(name, "read") | ||
| 296 | }) | ||
| 297 | if err != nil { | ||
| 298 | errs <- err | ||
| 299 | } | ||
| 300 | }(i) | ||
| 301 | } | ||
| 302 | wg.Wait() | ||
| 303 | close(errs) | ||
| 304 | for err := range errs { | ||
| 305 | t.Fatalf("Update: %v", err) | ||
| 306 | } | ||
| 307 | got, err := s.Get("repo-1") | ||
| 308 | if err != nil { | ||
| 309 | t.Fatal(err) | ||
| 310 | } | ||
| 311 | for i := 0; i < writers; i++ { | ||
| 312 | if name := fmt.Sprintf("account-%d", i); got.RoleFor(name) != Read { | ||
| 313 | t.Fatalf("%s did not land: %+v", name, got.Entries) | ||
| 314 | } | ||
| 315 | } | ||
| 316 | } | ||
| 317 | |||
| 318 | // A retired account stops counting even while its name is still in the rules: the | ||
| 319 | // row outlives the account, so membership alone is not a grant that lasts. | ||
| 320 | func TestRetiredAccountsStopCounting(t *testing.T) { | ||
| 321 | t.Parallel() | ||
| 322 | s, _ := open(t) | ||
| 323 | s.Active = func(name string) bool { return name != "retired" } | ||
| 324 | if _, err := s.Update("repo-1", "keenan", func(a *ACL) error { | ||
| 325 | if err := a.SetEntry("retired", "maintain"); err != nil { | ||
| 326 | return err | ||
| 327 | } | ||
| 328 | a.SetRunners([]string{"retired", "active-runner"}) | ||
| 329 | return nil | ||
| 330 | }); err != nil { | ||
| 331 | t.Fatal(err) | ||
| 332 | } | ||
| 333 | got, err := s.Get("repo-1") | ||
| 334 | if err != nil { | ||
| 335 | t.Fatal(err) | ||
| 336 | } | ||
| 337 | if got.EligibleApprover("retired") { | ||
| 338 | t.Fatal("a retired maintainer still counts as an approver") | ||
| 339 | } | ||
| 340 | if !got.EligibleApprover("keenan") && got.RoleFor("keenan") == Admin { | ||
| 341 | t.Fatal("an active admin stopped counting") | ||
| 342 | } | ||
| 343 | if got.CanReport("retired") { | ||
| 344 | t.Fatal("a retired runner still counts as a runner") | ||
| 345 | } | ||
| 346 | if !got.CanReport("active-runner") { | ||
| 347 | t.Fatal("an active runner stopped counting") | ||
| 348 | } | ||
| 349 | } | ||
| 350 | |||
| 351 | // An approval needs current standing, not just a file on the branch: a contributor | ||
| 352 | // who writes approvals/<their-own-name>.md cannot satisfy a review. | ||
| 353 | func TestEligibleApproverNeedsMaintainOrAdmin(t *testing.T) { | ||
| 354 | t.Parallel() | ||
| 355 | s, _ := open(t) | ||
| 356 | if _, err := s.Update("repo-1", "keenan", func(a *ACL) error { | ||
| 357 | if err := a.SetEntry("writer", "contribute"); err != nil { | ||
| 358 | return err | ||
| 359 | } | ||
| 360 | a.Default = "read" | ||
| 361 | return nil | ||
| 362 | }); err != nil { | ||
| 363 | t.Fatal(err) | ||
| 364 | } | ||
| 365 | got, err := s.Get("repo-1") | ||
| 366 | if err != nil { | ||
| 367 | t.Fatal(err) | ||
| 368 | } | ||
| 369 | if got.EligibleApprover("writer") { | ||
| 370 | t.Fatal("a contributor's approval counts") | ||
| 371 | } | ||
| 372 | if got.EligibleApprover("stranger") { | ||
| 373 | t.Fatal("an approval from somebody with only the default role counts") | ||
| 374 | } | ||
| 375 | } | ||