Sign in

Search issue titles and bodies, change requests and comments. Private repositories you cannot read are not included.

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.

#54

Make access edits and approvals survive a concurrent change

merged · by keenan 12d ago · merged into main at 62b59a76 by keenan 12d ago

keenan opened this 12d ago

Closes #10.

What was wrong

access.Store had no version check, and every editor read the row, changed a field and wrote the whole document back. SaveSettings in particular loaded the ACL and rewrote it when the visibility changed. A repository admin’s stale save could therefore restore a role an instance admin had just revoked.

Separately, approvals were counted by file alone and ACL.CanReport accepted any name in the runners list, so a demoted maintainer’s approval and a retired account’s CI report kept satisfying the gates.

What this does

Compare-and-swap on the rules. repo_acl gains a revision column, added on open for existing instances. Get attaches the revision it read; Set updates with WHERE revision=? and returns access.ErrConflict when it lost a race, and a from-scratch rule set inserts only if no row exists. No stale document can replace a live one, even from a caller that never learned about revisions.

Field-scoped updates. Store.Update re-reads, applies a function, and retries the swap. The paths that own one field now use it:

  • SaveSettings writes only the visibility; entries, runners and the approval policy are untouched, which is what closes the settings/role-removal race.
  • AddRepositoryAccess writes one entry.
  • SaveRepositoryAccess applies the submitted fields to the current row and preserves entries/runners for accounts the form did not render.

Approvals bound to current standing. FindApprovedMatch takes an eligibility predicate, and landing.Gate supplies ACL.EligibleApprover: active account, currently maintain or admin. The request page and list count with the same rule and show an approval that is current but no longer counts as “no longer counts”, so the page cannot promise a merge the gate refuses.

Runners bound to current standing. ACL.CanReport refuses a retired account even while its name is in the list: the ACL row outlives the account, so membership alone would let a revoked account keep vouching for builds and leasing jobs. The roster predicate is attached by the store in both the server and the hook process, beside the instance-role lookup that was already wired there. An explicit runner still does not need a repository role, which is the documented design.

Scope notes

  • The SSH admin and CLI acl commands still replace the whole document, but now through the same compare-and-swap, so a concurrent edit produces ErrConflict rather than a silent overwrite. Converting each of those subcommands to a field-scoped update is a follow-up, not a correctness gap.
  • Two instance admins saving the access form at once get last-writer-wins per field. The issue’s race (repository admin vs instance admin) is closed, and a hidden revision field on the form is the next step if we want that case reported instead of merged.

Verification

  • gofmt, go vet ./..., go test -race ./... clean.
  • ./vole workflow check and ./vole workflow run verify pass.

Tests, each failing before its fix

  • TestARepoAdminSaveCannotRestoreARevokedRole: read, revoke, then write the stale rule set — refused, and the role stays revoked.
  • TestConcurrentUpdatesAllLand: eight concurrent one-entry updates all survive.
  • TestUpdateKeepsAConcurrentChange, TestRetiredAccountsStopCounting, TestEligibleApproverNeedsMaintainOrAdmin.
  • TestLandRefusesAnApprovalFromADemotedMaintainer, TestLandRefusesAnApprovalFromARetiredAccount.
  • TestSettingsSaveLeavesAccessRulesAlone: a settings save leaves entries, runners, approval policy and protection as they were.
  • TestARetiredRunnerMayNotReportAResult (hook).

Docs: accounts-and-access.md describes the compare-and-swap and that retirement reaches the runners list; change-requests.md describes when an approval stops counting.

Discussion (0)

No discussion yet.