fix: failed renames in library forms, and two review follow-ups

CHANGELOG.md

@@ -55,9 +55,13 @@ follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 - The peer form's role-specific checkbox group for the *other* role (the iBGP
   switches on an eBGP peer, and vice versa) was visible, greyed out, instead of
   hidden: the group's flex layout overrode the `hidden` attribute.
-- A peer rename that failed validation re-rendered the form posting to the new
-  name, so the corrected resubmit hit "not found" and the edit was lost. The form
-  now posts back to the name the peer is stored under.
+- **A rename that failed validation could overwrite another object.** The peer,
+  prefix set, AS set, community, RPKI server, BMP station and policy forms
+  re-rendered posting to the name the operator had typed. If that name was free,
+  the corrected resubmit hit "not found" and the edit was lost; if it was taken —
+  the usual reason a rename is refused — the resubmit saved over the object that
+  held it, replacing its contents, and left the one being edited as it was. Every
+  form now posts back to the name the object is stored under.
 - **Two objects with one BIRD name are caught before BIRD is.** BIRD keeps
   defines, filters, functions, templates and protocols in one namespace, so a peer
   and a prefix set (or community, AS set, RPKI server, BMP station) of the same

internal/web/assets.go

@@ -29,6 +29,9 @@ type asSetFormView struct {
 	Errs       map[string]string
 	Preview    string
 	PreviewErr string
+	// StoredName is the name the set is saved under. A form re-rendered after
+	// a failed rename posts back to it, not to the name the operator typed.
+	StoredName string
 }
 
 func (s *Server) handleASSetsList(w http.ResponseWriter, r *http.Request) {
@@ -146,7 +149,8 @@ func (s *Server) handleASSetSave(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	s.renderASSetForm(w, asSetFormView{
-		Active: "library", ReadOnly: s.readOnly, IsNew: isNew, Set: as, EntryText: entryText, Errs: errs,
+		Active: "library", ReadOnly: s.readOnly, IsNew: isNew, StoredName: r.PathValue("name"),
+		Set: as, EntryText: entryText, Errs: errs,
 	})
 }
 

internal/web/bmp.go

@@ -24,6 +24,9 @@ type bmpFormView struct {
 	IsNew    bool
 	Station  store.BMPStation
 	Errs     map[string]string
+	// StoredName is the name the station is saved under. A form re-rendered
+	// after a failed rename posts back to it, not to the name the operator typed.
+	StoredName string
 }
 
 func (s *Server) handleBMPPage(w http.ResponseWriter, r *http.Request) {
@@ -110,7 +113,7 @@ func (s *Server) handleBMPSave(w http.ResponseWriter, r *http.Request) {
 		s.flashRedirect(w, r, "/bmp", "Saved "+st.Name, false)
 		return
 	}
-	render(w, s.log, "bmp_form.html", bmpFormView{Active: "bmp", ReadOnly: s.readOnly, IsNew: isNew, Station: st, Errs: errs})
+	render(w, s.log, "bmp_form.html", bmpFormView{Active: "bmp", ReadOnly: s.readOnly, IsNew: isNew, StoredName: r.PathValue("name"), Station: st, Errs: errs})
 }
 
 func (s *Server) handleBMPDelete(w http.ResponseWriter, r *http.Request) {

internal/web/communities_handler_test.go

@@ -101,6 +101,22 @@ func TestCommunityReferenceGuards(t *testing.T) {
 	}
 }
 
+// The in-use check is what keeps a delete from leaving an undefined symbol
+// behind; when the check cannot run, the delete must not go ahead blind.
+func TestCommunityDeleteRefusesWhenTheUseCheckFails(t *testing.T) {
+	env := newTestEnv(t, false)
+	if rec := env.do(t, "POST", "/library/communities/new", url.Values{"name": {"CUST_TAG"}, "value": {"65000:7"}}); rec.Code != 303 {
+		t.Fatalf("create: %d %s", rec.Code, rec.Body.String())
+	}
+	sabotageDB(t, env, `ALTER TABLE peer_templates RENAME TO peer_templates_gone`)
+	if rec := env.do(t, "POST", "/library/communities/CUST_TAG/delete", nil); rec.Code == 303 {
+		t.Errorf("the delete went ahead although the in-use check failed: %q", flashOf(rec))
+	}
+	if _, err := env.store.GetCommunityDefByName("CUST_TAG"); err != nil {
+		t.Errorf("the community should still exist: %v", err)
+	}
+}
+
 // A reserved name and a malformed value are rejected without creating anything.
 func TestCommunityValidation(t *testing.T) {
 	env := applyReady(t)

internal/web/library.go

@@ -26,6 +26,9 @@ type prefixSetFormView struct {
 	Preview    string
 	PreviewErr string
 	Bgpq4      bool // whether IRR expansion is enabled
+	// StoredName is the name the set is saved under. A form re-rendered after
+	// a failed rename posts back to it, not to the name the operator typed.
+	StoredName string
 }
 
 func (s *Server) handlePrefixSetsList(w http.ResponseWriter, r *http.Request) {
@@ -148,7 +151,7 @@ func (s *Server) handlePrefixSetSave(w http.ResponseWriter, r *http.Request) {
 		s.flashRedirect(w, r, "/library/prefix-sets", "Saved "+ps.Name, false)
 		return
 	}
-	s.renderPrefixSetForm(w, prefixSetFormView{Active: "library", ReadOnly: s.readOnly, IsNew: isNew, Set: ps, Errs: errs})
+	s.renderPrefixSetForm(w, prefixSetFormView{Active: "library", ReadOnly: s.readOnly, IsNew: isNew, StoredName: r.PathValue("name"), Set: ps, Errs: errs})
 }
 
 func (s *Server) handlePrefixSetDelete(w http.ResponseWriter, r *http.Request) {

internal/web/library_communities.go

@@ -26,6 +26,9 @@ type communityFormView struct {
 	Value   string
 	Errs    map[string]string
 	Preview string
+	// StoredName is the name the community is saved under. A form re-rendered
+	// after a failed rename posts back to it, not to the name the operator typed.
+	StoredName string
 }
 
 func (s *Server) handleCommunitiesList(w http.ResponseWriter, r *http.Request) {
@@ -125,7 +128,7 @@ func (s *Server) handleCommunitySave(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	s.renderCommunityForm(w, communityFormView{
-		Active: "library", ReadOnly: s.readOnly, IsNew: isNew, Def: cd,
+		Active: "library", ReadOnly: s.readOnly, IsNew: isNew, StoredName: r.PathValue("name"), Def: cd,
 		Value: r.FormValue("value"), Errs: errs,
 	})
 }

internal/web/library_rename_test.go

@@ -0,0 +1,99 @@
+package web
+
+import (
+	"fmt"
+	"net/http"
+	"net/url"
+	"regexp"
+	"testing"
+)
+
+// editorAction is where a re-rendered library form posts.
+var editorAction = regexp.MustCompile(`<form method="post" action="([^"]*)"[^>]*data-editor`)
+
+// A rename that fails validation re-renders the form with what the operator
+// typed, and the form must still post to the name the object is stored under.
+// Posting to the typed name turns the corrected resubmit into an edit of
+// whatever already has that name: renaming one prefix set onto a sibling's
+// name, then fixing the name, overwrote the sibling with the first set's
+// entries and left the first set unrenamed.
+func TestFailedLibraryRenamePostsBackToTheStoredName(t *testing.T) {
+	for _, c := range []struct {
+		kind, base string
+		form       func(name string, n int) url.Values
+		id         func(env *testEnv, name string) (int64, error)
+	}{
+		{"prefix set", "/library/prefix-sets", func(name string, n int) url.Values {
+			return url.Values{"name": {name}, "family": {"ipv4"}, "entries": {fmt.Sprintf("198.51.%d.0/24", n)}}
+		}, func(env *testEnv, name string) (int64, error) {
+			v, err := env.store.GetPrefixSetByName(name)
+			return v.ID, err
+		}},
+		{"AS set", "/library/as-sets", func(name string, n int) url.Values {
+			return url.Values{"name": {name}, "entries": {fmt.Sprint(64600 + n)}}
+		}, func(env *testEnv, name string) (int64, error) {
+			v, err := env.store.GetASSetByName(name)
+			return v.ID, err
+		}},
+		{"community", "/library/communities", func(name string, n int) url.Values {
+			return url.Values{"name": {name}, "value": {fmt.Sprintf("65000:%d", n)}}
+		}, func(env *testEnv, name string) (int64, error) {
+			v, err := env.store.GetCommunityDefByName(name)
+			return v.ID, err
+		}},
+		{"RPKI server", "/rpki", func(name string, n int) url.Values {
+			return url.Values{"name": {name}, "host": {fmt.Sprintf("rtr%d.example.net", n)}, "port": {"8282"},
+				"refresh": {"900"}, "expire": {"172800"}, "enabled": {"on"}}
+		}, func(env *testEnv, name string) (int64, error) {
+			v, err := env.store.GetRPKIServerByName(name)
+			return v.ID, err
+		}},
+		{"BMP station", "/bmp", func(name string, n int) url.Values {
+			return url.Values{"name": {name}, "address": {fmt.Sprintf("203.0.113.%d", n)}, "port": {"1790"}, "enabled": {"on"}}
+		}, func(env *testEnv, name string) (int64, error) {
+			v, err := env.store.GetBMPStationByName(name)
+			return v.ID, err
+		}},
+		{"policy", "/policies", func(name string, n int) url.Values {
+			return url.Values{"name": {name}, "direction": {"import"}, "defaultRoute": {"reject"},
+				"bogonAsns": {"off"}, "rov": {"off"}}
+		}, func(env *testEnv, name string) (int64, error) {
+			v, err := env.store.GetPolicyByName(name)
+			return v.ID, err
+		}},
+	} {
+		t.Run(c.kind, func(t *testing.T) {
+			env := newTestEnv(t, false)
+			for i, name := range []string{"CUST_A", "CUST_C"} {
+				if rec := env.do(t, "POST", c.base+"/new", c.form(name, i+1)); rec.Code != http.StatusSeeOther {
+					t.Fatalf("create %s: %d %s", name, rec.Code, rec.Body)
+				}
+			}
+			idA, _ := c.id(env, "CUST_A")
+			idC, _ := c.id(env, "CUST_C")
+
+			rec := env.do(t, "POST", c.base+"/CUST_A/edit", c.form("CUST_C", 1))
+			if rec.Code != http.StatusOK {
+				t.Fatalf("a rename onto a sibling's name should re-render the form: %d", rec.Code)
+			}
+			m := editorAction.FindStringSubmatch(rec.Body.String())
+			if m == nil {
+				t.Fatal("the re-rendered page has no editor form")
+			}
+			if want := c.base + "/CUST_A/edit"; m[1] != want {
+				t.Errorf("the re-rendered form posts to %s, want %s", m[1], want)
+			}
+
+			// The operator corrects the name and resubmits the same form.
+			if rec := env.do(t, "POST", m[1], c.form("CUST_D", 1)); rec.Code != http.StatusSeeOther {
+				t.Fatalf("corrected resubmit: %d %s", rec.Code, rec.Body)
+			}
+			if id, err := c.id(env, "CUST_D"); err != nil || id != idA {
+				t.Errorf("CUST_D should be CUST_A renamed (id %d); got id %d, err %v", idA, id, err)
+			}
+			if id, err := c.id(env, "CUST_C"); err != nil || id != idC {
+				t.Errorf("CUST_C should be untouched (id %d); got id %d, err %v", idC, id, err)
+			}
+		})
+	}
+}

internal/web/peer_templates_handler_test.go

@@ -599,6 +599,21 @@ func TestDeletingALinkedTemplateSaysToDetachFirst(t *testing.T) {
 	}
 }
 
+// Only a template peers link to is fixed by detaching them; a delete that fails
+// for another reason must not send the operator off to detach peers.
+func TestAnUnlinkedTemplatesFailedDeleteDoesNotSayDetach(t *testing.T) {
+	env := newTestEnv(t, false)
+	createTemplate(env, t) // no peers linked
+	sabotageDB(t, env, `CREATE TRIGGER keep_templates BEFORE DELETE ON peer_templates BEGIN SELECT RAISE(ABORT, 'disk on fire'); END`)
+	flash := flashOf(env.do(t, "POST", "/peers/templates/IX_PEERS/delete", nil))
+	if !strings.Contains(flash, "disk on fire") {
+		t.Fatalf("the delete should fail and say why: %q", flash)
+	}
+	if strings.Contains(flash, "Detach") {
+		t.Errorf("an unlinked template's failed delete should not say to detach peers: %q", flash)
+	}
+}
+
 // Every library object that renders a BIRD symbol refuses a template's name at
 // save, not only at the next render.
 func TestLibraryObjectsCannotTakeATemplatesName(t *testing.T) {

internal/web/policies.go

@@ -37,6 +37,9 @@ type policyFormView struct {
 	PreviewErr string
 	// Communities feed the match-community input's autocomplete datalist.
 	Communities []store.CommunityDef
+	// StoredName is the name the policy is saved under. A form re-rendered
+	// after a failed rename posts back to it, not to the name the operator typed.
+	StoredName string
 }
 
 func (s *Server) handlePoliciesList(w http.ResponseWriter, r *http.Request) {
@@ -229,7 +232,7 @@ func (s *Server) handlePolicySave(w http.ResponseWriter, r *http.Request) {
 		s.flashRedirect(w, r, "/policies", "Saved "+p.Name, false)
 		return
 	}
-	s.renderPolicyForm(w, policyFormView{Active: "policies", ReadOnly: s.readOnly, IsNew: isNew, Policy: p, Errs: errs})
+	s.renderPolicyForm(w, policyFormView{Active: "policies", ReadOnly: s.readOnly, IsNew: isNew, StoredName: r.PathValue("name"), Policy: p, Errs: errs})
 }
 
 func (s *Server) handlePolicyDelete(w http.ResponseWriter, r *http.Request) {

internal/web/rpki.go

@@ -71,6 +71,9 @@ type rpkiFormView struct {
 	IsNew    bool
 	Server   store.RPKIServer
 	Errs     map[string]string
+	// StoredName is the name the server is saved under. A form re-rendered
+	// after a failed rename posts back to it, not to the name the operator typed.
+	StoredName string
 }
 
 func (s *Server) handleRPKIPage(w http.ResponseWriter, r *http.Request) {
@@ -234,7 +237,7 @@ func (s *Server) handleRPKISave(w http.ResponseWriter, r *http.Request) {
 		s.flashRedirect(w, r, "/rpki", "Saved "+srv.Name, false)
 		return
 	}
-	render(w, s.log, "rpki_form.html", rpkiFormView{Active: "rpki", ReadOnly: s.readOnly, IsNew: isNew, Server: srv, Errs: errs})
+	render(w, s.log, "rpki_form.html", rpkiFormView{Active: "rpki", ReadOnly: s.readOnly, IsNew: isNew, StoredName: r.PathValue("name"), Server: srv, Errs: errs})
 }
 
 // lastEnabledServerGuard returns a message when turning this server off would

internal/web/seed.go

@@ -165,7 +165,12 @@ func (s *Server) discoverSeedRows(ctx context.Context) ([]seedRow, string) {
 		row.Role, row.Multihop, row.Note = p.Role, p.Multihop, note
 		row.V6 = p.IsV6()
 		probe := p
-		if errs := probe.Validate(); len(errs) > 0 {
+		errs := probe.Validate()
+		// The save skips a session named like a template, so say so here
+		// rather than offer it.
+		if err := s.refuseTemplateName(p.Name, errs); err != nil {
+			row.Invalid = "could not check the name: " + err.Error()
+		} else if len(errs) > 0 {
 			row.Invalid = firstFieldError(errs)
 		}
 		rows = append(rows, row)

internal/web/seed_handler_test.go

@@ -84,9 +84,10 @@ func TestSeedDetectsIBGP(t *testing.T) {
 	}
 }
 
-// A live session named like a template cannot be imported as a peer: the model
-// would then declare that name twice. It is skipped, not created.
-func TestSeedSkipsASessionNamedLikeATemplate(t *testing.T) {
+// templateNamedLikeSession is the fake router's live edge_v4 session plus a
+// peer template that already took the name edge_v4.
+func templateNamedLikeSession(t *testing.T) *testEnv {
+	t.Helper()
 	env := applyReady(t)
 	env.fc.details["edge_v4"] = seedDetail("edge_v4", "198.51.100.1", "64500", "65551", "external")
 	tmpl := store.PeerTemplate{Name: "edge_v4", Role: store.RoleUpstream, ImportLimitAction: "restart"}
@@ -96,6 +97,23 @@ func TestSeedSkipsASessionNamedLikeATemplate(t *testing.T) {
 	if _, err := env.store.CreatePeerTemplate(tmpl, nil, nil); err != nil {
 		t.Fatal(err)
 	}
+	return env
+}
+
+// The seed page says up front that such a session cannot be imported, rather
+// than offering it and skipping it on save.
+func TestSeedPageFlagsASessionNamedLikeATemplate(t *testing.T) {
+	env := templateNamedLikeSession(t)
+	body := env.do(t, "GET", "/peers/seed", nil).Body.String()
+	if !strings.Contains(body, "can't import") || !strings.Contains(body, `peer template "edge_v4" already uses this name`) {
+		t.Errorf("the seed page should flag a session named like a template, body:\n%s", body)
+	}
+}
+
+// A live session named like a template cannot be imported as a peer: the model
+// would then declare that name twice. It is skipped, not created.
+func TestSeedSkipsASessionNamedLikeATemplate(t *testing.T) {
+	env := templateNamedLikeSession(t)
 
 	env.do(t, "POST", "/peers/seed", url.Values{"include": {"edge_v4"}, "role_edge_v4": {"upstream"}})
 	if _, err := env.store.GetPeerByName("edge_v4"); err != store.ErrNotFound {

internal/web/templates/as_set_form.html

@@ -21,7 +21,7 @@ <h1>{{if .IsNew}}Add AS set{{else}}Edit {{.Set.Name}}{{end}}</h1>
 				</div>
 			</div>
 
-			<form method="post" action="{{if .IsNew}}/library/as-sets/new{{else}}/library/as-sets/{{.Set.Name}}/edit{{end}}" data-preview-url="/library/as-sets/preview" data-editor>
+			<form method="post" action="{{if .IsNew}}/library/as-sets/new{{else}}/library/as-sets/{{or .StoredName .Set.Name}}/edit{{end}}" data-preview-url="/library/as-sets/preview" data-editor>
 			<div class="split">
 				<div>
 					<div class="card">

internal/web/templates/bmp_form.html

@@ -21,7 +21,7 @@ <h1>{{if .IsNew}}Add BMP station{{else}}Edit {{.Station.Name}}{{end}}</h1>
 				</div>
 			</div>
 
-			<form method="post" action="{{if .IsNew}}/bmp/new{{else}}/bmp/{{.Station.Name}}/edit{{end}}" data-editor>
+			<form method="post" action="{{if .IsNew}}/bmp/new{{else}}/bmp/{{or .StoredName .Station.Name}}/edit{{end}}" data-editor>
 			<div class="card">
 				<div class="card-header"><span class="card-title">{{template "icon-activity"}}Station</span></div>
 				<div class="card-body">

internal/web/templates/community_form.html

@@ -15,7 +15,7 @@
 				<div class="breadcrumb"><a href="/">Dashboard</a><span class="sep">/</span><a href="/library/communities">Library</a><span class="sep">/</span><span class="current mono">{{if .IsNew}}new{{else}}{{.Def.Name}}{{end}}</span></div>
 			</div>
 
-			<form method="post" action="{{if .IsNew}}/library/communities/new{{else}}/library/communities/{{.Def.Name}}/edit{{end}}" data-editor>
+			<form method="post" action="{{if .IsNew}}/library/communities/new{{else}}/library/communities/{{or .StoredName .Def.Name}}/edit{{end}}" data-editor>
 			<div class="card">
 				<div class="card-body">
 					{{if .Def.Builtin}}<p class="card-intro">This community ships with birdy. Your edits are kept.</p>{{end}}

internal/web/templates/policy_form.html

@@ -24,7 +24,7 @@ <h1>{{if .IsNew}}New {{.Policy.Direction}} policy{{else}}Edit {{.Policy.Name}}{{
 				</div>
 			</div>
 
-			<form method="post" action="{{if .IsNew}}/policies/new{{else}}/policies/{{.Policy.Name}}/edit{{end}}" data-preview-url="/policies/preview" data-editor>
+			<form method="post" action="{{if .IsNew}}/policies/new{{else}}/policies/{{or .StoredName .Policy.Name}}/edit{{end}}" data-preview-url="/policies/preview" data-editor>
 			<input type="hidden" name="direction" value="{{.Policy.Direction}}">
 			<div class="split">
 				<div>

internal/web/templates/prefix_set_form.html

@@ -21,7 +21,7 @@ <h1>{{if .IsNew}}Add prefix set{{else}}Edit {{.Set.Name}}{{end}}</h1>
 				</div>
 			</div>
 
-			<form method="post" action="{{if .IsNew}}/library/prefix-sets/new{{else}}/library/prefix-sets/{{.Set.Name}}/edit{{end}}" data-preview-url="/library/prefix-sets/preview" data-editor>
+			<form method="post" action="{{if .IsNew}}/library/prefix-sets/new{{else}}/library/prefix-sets/{{or .StoredName .Set.Name}}/edit{{end}}" data-preview-url="/library/prefix-sets/preview" data-editor>
 			<div class="split">
 				<div>
 					{{if .Set.System}}

internal/web/templates/rpki_form.html

@@ -21,7 +21,7 @@ <h1>{{if .IsNew}}Add RTR server{{else}}Edit {{.Server.Name}}{{end}}</h1>
 				</div>
 			</div>
 
-			<form method="post" action="{{if .IsNew}}/rpki/new{{else}}/rpki/{{.Server.Name}}/edit{{end}}" data-editor>
+			<form method="post" action="{{if .IsNew}}/rpki/new{{else}}/rpki/{{or .StoredName .Server.Name}}/edit{{end}}" data-editor>
 			<div class="card">
 				<div class="card-header"><span class="card-title">{{template "icon-shield"}}RTR endpoint</span></div>
 				<div class="card-body">