Put main's rule right again once a new repository is in the organisation: the forge drops a merger without access, and under the setup account the tools' account has none (issue 1124, ADR 0428 §3, review round 1)

modules/gitea/README.md

@@ -154,7 +154,9 @@ listed repository:
   naming the description, and a `merge-check.sh` declaring `# mesh-check-toolchain: go` that vets and runs the unit
   tests, then the integration tests (`-tags integration`), of every Go module in the tree, and says so and passes on
   a tree with none. The rule is then narrowed to main's rule of ADR 0398 §2 (no push, only `mesh-tools` merges),
-  requiring `mesh/repo-check` and `mesh/merge-gate`, and only then is the repository moved into the organisation;
+  requiring `mesh/repo-check` and `mesh/merge-gate`, and only then is the repository moved into the organisation.
+  There main's rule is read back and put right once more (the step `rule-after-move`): the forge drops from a merge
+  list a user without access to the repository, and under the setup account `mesh-tools` has none;
 - one found still under the setup account is finished: the steps it lacks are done, main is never written twice;
 - one in the organisation is only put right: main's rule narrowed, a missing one of the two statuses added and every
   status already there kept.

modules/gitea/cmd/repositories/fake_test.go

@@ -118,6 +118,28 @@ func lets(rule map[string]any, login string) bool {
 	return false
 }
 
+// reaches says whether a login has access to a repository, as the forge judges a merge whitelist: its owner does,
+// and in the organisation the tools' team does (it is on every repository there); nobody else here.
+func reaches(full, login string) bool {
+	owner := strings.SplitN(full, "/", 2)[0]
+	return owner == login || (owner == org && login == tools)
+}
+
+// keepReaching drops from a rule's merge whitelist every user without access to the repository, as gitea's
+// updateUserWhitelist does on creating or editing a branch protection.
+func keepReaching(full string, rule map[string]any) {
+	if _, ok := rule["merge_whitelist_usernames"]; !ok {
+		return
+	}
+	kept := []any{}
+	for _, u := range strs(rule["merge_whitelist_usernames"]) {
+		if reaches(full, u) {
+			kept = append(kept, u)
+		}
+	}
+	rule["merge_whitelist_usernames"] = kept
+}
+
 func (f *fakeForge) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	f.mu.Lock()
 	defer f.mu.Unlock()
@@ -197,6 +219,7 @@ func (f *fakeForge) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 				return
 			}
 			delete(in, "rule_name")
+			keepReaching(m[1]+"/"+m[2], in)
 			repo.Rules[name] = in
 			reply(201, in)
 		case r.Method == http.MethodPatch && m[3] != "":
@@ -208,6 +231,7 @@ func (f *fakeForge) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 			for k, v := range in {
 				rule[k] = v
 			}
+			keepReaching(m[1]+"/"+m[2], rule)
 			reply(200, rule)
 		default:
 			reply(405, nil)

modules/gitea/cmd/repositories/repositories.go

@@ -39,6 +39,9 @@ const (
 	StepNarrow      = "narrow"
 	StepMove        = "move"
 	StepPutRight    = "put-right"
+	// StepRuleAfterMove reads main's rule back once the repository is in the organisation and puts it right: the
+	// forge drops from a merge whitelist a user without access, and under the setup account the tools' have none.
+	StepRuleAfterMove = "rule-after-move"
 )
 
 // What a pass did with one repository.
@@ -250,6 +253,9 @@ func (s *Syncer) one(ctx context.Context, name string, r Repo) Result {
 		map[string]any{"new_owner": s.Org}, nil); err != nil {
 		return refused(name, StepMove, err)
 	}
+	if _, err := s.putRight(ctx, s.Org, name); err != nil {
+		return refused(name, StepRuleAfterMove, err)
+	}
 	return Result{Name: name, Did: DidMade}
 }
 

modules/gitea/cmd/repositories/repositories_test.go

@@ -31,6 +31,7 @@ func TestNewRepositoryIsMadeInOrder(t *testing.T) {
 		"POST /api/v1/repos/mesh-admin/mesh-delivery/contents",
 		"PATCH /api/v1/repos/mesh-admin/mesh-delivery/branch_protections/main",
 		"POST /api/v1/repos/mesh-admin/mesh-delivery/transfer",
+		"PATCH /api/v1/repos/novox/mesh-delivery/branch_protections/main",
 	}
 	if got := f.writes(); strings.Join(got, "\n") != strings.Join(want, "\n") {
 		t.Fatalf("writes, in order:\n%s\nwant:\n%s", strings.Join(got, "\n"), strings.Join(want, "\n"))
@@ -84,6 +85,33 @@ func TestNewRepositoryIsMadeInOrder(t *testing.T) {
 	}
 }
 
+// 1124/AC2: the forge drops from a merge whitelist a user without access to the repository, and under the setup
+// account the tools' account has none; so once the repository is in the organisation its main rule is read back and
+// put right: the tools' account the only one that merges, both statuses required.
+func TestOrganisationRuleListsTheToolsAfterTheMove(t *testing.T) {
+	f := newFake()
+	s, _ := syncer(t, f)
+	o := run(t, s, Value{"mesh-delivery": {}})
+	repo := f.repos["novox/mesh-delivery"]
+	if repo == nil {
+		t.Fatal("not moved into the organisation")
+	}
+	rule := repo.Rules[mainRule]
+	if rule["enable_merge_whitelist"] != true || strings.Join(strs(rule["merge_whitelist_usernames"]), ",") != tools {
+		t.Errorf("after the move main's merge whitelist is %v, want %s alone", rule["merge_whitelist_usernames"], tools)
+	}
+	if rule["enable_push"] != false {
+		t.Errorf("after the move main takes a push: %v", rule)
+	}
+	checks := strs(rule["status_check_contexts"])
+	if rule["enable_status_check"] != true || !has(checks, "mesh/repo-check") || !has(checks, "mesh/merge-gate") {
+		t.Errorf("after the move main does not require both statuses: %v", rule)
+	}
+	if r := result(o, "mesh-delivery"); r.Did != DidMade {
+		t.Errorf("outcome %+v, want made", r)
+	}
+}
+
 // 1124/AC2: the setup login's rule lets that login alone push, and it is what the first commit lands through.
 func TestAdminRuleLetsTheSetupLoginAlonePush(t *testing.T) {
 	f := newFake()
@@ -164,8 +192,10 @@ func TestRepositoryUnderTheSetupAccountIsFinished(t *testing.T) {
 		"POST /api/v1/repos/mesh-admin/empty/contents",
 		"PATCH /api/v1/repos/mesh-admin/empty/branch_protections/main",
 		"POST /api/v1/repos/mesh-admin/empty/transfer",
+		"PATCH /api/v1/repos/novox/empty/branch_protections/main",
 		"PATCH /api/v1/repos/mesh-admin/half/branch_protections/main",
 		"POST /api/v1/repos/mesh-admin/half/transfer",
+		"PATCH /api/v1/repos/novox/half/branch_protections/main",
 	}, "\n")
 	if got != want {
 		t.Fatalf("writes:\n%s\nwant:\n%s", got, want)
@@ -223,6 +253,9 @@ func TestRefusedStepStopsThatRepositoryOnly(t *testing.T) {
 		StepFirstCommit: func(m, p string) bool { return m == "POST" && strings.HasSuffix(p, "/a/contents") },
 		StepNarrow:      func(m, p string) bool { return m == "PATCH" && strings.Contains(p, "/a/branch_protections/") },
 		StepMove:        func(m, p string) bool { return m == "POST" && strings.HasSuffix(p, "/a/transfer") },
+		StepRuleAfterMove: func(m, p string) bool {
+			return m == "PATCH" && strings.HasPrefix(p, "/api/v1/repos/novox/a/branch_protections/")
+		},
 	}
 	for step, match := range steps {
 		t.Run(step, func(t *testing.T) {