Manage, not view: persistence (Save), rule edit and move

DESIGN.md

@@ -0,0 +1,23 @@
+# Design notes
+
+## Per-rule enable/disable (not implemented)
+
+The manage feature set (Save, edit in place, move) deliberately leaves
+per-rule disable out. nftables has no disabled state: a rule either is in the
+ruleset or it is not, so "disable" means deleting the rule while remembering
+enough to put it back exactly where it was.
+
+Doing that honestly needs a second persistence format — call it v2 of the
+saved file — that stores the tool's own view of the ruleset rather than nft's
+listing: a list of rule *specs* (the same `RuleSpec` the add and edit forms
+produce), each with an enabled flag and its position. Disabling would then be
+"delete the rule, keep its spec"; enabling, "insert the spec at its recorded
+position"; and Save would write both the active ruleset (as the `.nft` file it
+writes today, still loadable by plain `nft -f` on boot) and the spec sidecar
+that carries the disabled entries.
+
+That sidecar is a real format with compatibility duties, and it only pays for
+itself once the tool also reconciles it against a ruleset that changed behind
+its back. Until that reconciliation exists, a fake disable would silently drop
+rules on the floor, so the feature waits for persistence v2 instead of
+shipping as a trap.

README.md

@@ -307,13 +307,16 @@ firewall nor `sudo` is available, it says so and points at `--demo`.
 | `pgup` / `pgdn` | Scroll a page |
 | `/` | Filter rules (matches any column; `esc` clears) |
 | `a` | Add a rule |
+| `E` | Edit the selected rule in place — same handle, same position (nftables) |
+| `K` / `J` | Move the selected rule up / down, as one atomic transaction (nftables) |
 | `d` | Delete the selected rule |
 | `e` | Enable or disable the firewall (ufw only) |
 | `r` | Reload the firewall |
 | `p` | Change a default policy (ufw) or a zone target (firewalld) |
 | `L` | Change the logging level (ufw) or the log-denied value (firewalld) |
 | `l` | Toggle logging on the selected rule (nftables) |
 | `w` | Watch the live firewall log of the logged rules (nftables) |
+| `W` | Save the tool's own table to a file loaded on boot, with a diff preview (nftables) |
 | `x` | Actions this backend offers beyond these keys |
 | `[` / `]` | Previous / next group — the firewalld zones and policy objects |
 | `R` | Re-read the firewall |
@@ -464,6 +467,44 @@ is restored automatically when the timer expires, and you are back on the rulese
 that was working. It is the standard `iptables-apply` idea, done as one nft
 transaction.
 
+### Manage, not view: edit, move and save
+
+The nftables backend does not stop at adding and deleting.
+
+**Edit in place.** Press `E` on a rule and the add-rule form opens pre-filled
+with the rule as it stands — interfaces, connection state, alias source,
+comment, all of it read back from the modelled match. Submitting builds one
+`nft replace rule … handle H`, the same replace-by-handle the log toggle uses,
+so the rule keeps its handle and its position. A rule the tool cannot hold in
+full — one with a match shown only as text, a NAT translation, a source-port
+match, a log statement with a level or an nflog group — is refused before the
+form opens, with the reason, rather than silently rewritten with half of
+itself missing.
+
+**Move.** `K` and `J` move a rule one position up or down. nftables has no
+move command, so a move is a copy written at the new position plus a delete of
+the old handle — two statements that only make sense together, which is why
+they always run as **one `nft -f` transaction**: wrapped as one when staging
+is off, or staged into the atomic batch when it is on. The rule is never
+duplicated or missing in between; the moved copy gets a fresh handle and a
+reset counter, and the preview says so.
+
+**Save.** Everything above changes the *running* ruleset, which a reboot
+throws away. Press `W` and the tool serialises its own table — `nft list
+table inet tui`, nft's own text, which is a valid nft script — and installs it
+with `install -m 644 /dev/stdin <path>`. The confirm dialog shows a **unified
+diff** against the file as it currently stands, so "what does saving change"
+is answered before a byte is written. On an Omarchy router the file is
+`/etc/omarchy/router/tui-firewall.nft`, which the router profile includes on
+boot; on any other machine it is `/etc/nftables.d/tui-firewall.nft`, loaded by
+the usual include from `/etc/nftables.conf` (the dialog spells this out), and
+`TUI_FIREWALL_SAVE_PATH` overrides the path for tests and labs. Saving writes
+the running table: staged changes that were not applied yet are not in the
+file, and the dialog says so.
+
+Per-rule enable/disable is deliberately absent for now; `DESIGN.md` describes
+the persisted-spec format it is waiting for.
+
 ### Per-rule logging and the live log
 
 Press `l` on a rule to mark it for logging. nft rules are immutable, so the rule
@@ -535,6 +576,17 @@ are capped, so a firewall under a scan cannot grow it without bound.
   table, and say why when it will not. Delete by handle.
 - Stage changes and apply them as one atomic `nft -f` transaction, with an
   automatic connectivity-safe rollback if the apply is not kept in time.
+- Edit a rule in place with `E`: the form opens pre-filled from the rule's own
+  modelled match and the submit is one `nft replace rule … handle H`, keeping
+  the handle and the position; a rule the model does not hold in full is
+  refused, never half-rewritten.
+- Move a rule up and down with `K` and `J`, as one atomic `nft -f` transaction
+  (a copy at the new position plus a delete of the old handle), staged into
+  the batch when staging is on.
+- Save the tool's own table to disk with `W` — `nft list table inet tui`
+  installed to `/etc/omarchy/router/tui-firewall.nft` on an Omarchy router,
+  `/etc/nftables.d/tui-firewall.nft` elsewhere — with a unified diff against
+  the current file in the confirm dialog.
 - Mark a rule to be logged with `l`: the rule is replaced in place, keeping its
   handle and position, with a stable `log prefix "tui:<chain> <verdict> "` the
   live view greps for. Toggle it off the same way.
@@ -545,7 +597,8 @@ are capped, so a firewall under a scan cannot grow it without bound.
 
 ## What v0.1 cannot do
 
-- **No rule editing.** Change a rule by deleting it and adding the new one.
+- **No rule editing on ufw or firewalld.** Change a rule by deleting it and
+  adding the new one; only the nftables backend edits in place with `E`.
 - **No `ufw` interface qualifiers** (`ufw allow in on eth0 …`): they are parsed
   and displayed, but the form cannot create them.
 - **No `ufw` application profile management** (only using existing profiles).
@@ -556,10 +609,13 @@ are capped, so a firewall under a scan cannot grow it without bound.
   editable here.
 - **No live log tail on ufw or firewalld** — it is an nftables feature, since it
   reads the kernel log the nftables `log` statement writes.
-- **No nftables rule editing or reordering**, and no set-element editing: a rule
-  is changed by deleting it and adding the new one, and a named set is created or
-  used, not edited member by member.
-- Rules follow the backend's own order; there is no reordering key.
+- **No nftables set-element editing**: a named set is created or used, and its
+  members are added and removed one at a time from the actions menu, not
+  edited in bulk.
+- **No per-rule enable/disable**: nftables has no disabled state, and faking
+  one honestly needs the persisted-spec format `DESIGN.md` describes.
+- On ufw and firewalld, rules follow the backend's own order; the move keys
+  are an nftables feature.
 
 ## Compatibility
 

cmd/tui-firewall/app.go

@@ -99,6 +99,11 @@ type app struct {
 	// the applied batch is on screen before its keep window begins.
 	keepTickPending bool
 
+	// editing marks the open rule form as an edit: submitting replaces
+	// editTarget in place rather than adding a rule.
+	editing    bool
+	editTarget firewall.Rule
+
 	// live holds the live-log view: the open stream, the events retained, and
 	// whether the feed is paused. It is only reachable on the nftables backend
 	// and its demo, the ones that implement logStreamer.
@@ -273,6 +278,14 @@ func (a *app) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
 		a.openApplyConfirm(msg.change)
 		return a, nil
 
+	case saveReadyMsg:
+		if msg.err != nil {
+			a.setStatus(ui.StatusError, msg.err.Error())
+			return a, nil
+		}
+		a.openSaveConfirm(msg)
+		return a, nil
+
 	case logEventMsg:
 		return a, a.handleLogEvent(msg)
 
@@ -452,6 +465,7 @@ func (a *app) handleForm(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
 	switch msg.String() {
 	case "esc":
 		a.mode = modeTable
+		a.editing = false
 		a.setStatus(ui.StatusInfo, "cancelled")
 		return a, nil
 	case "tab", "down":
@@ -484,18 +498,32 @@ func (a *app) handleForm(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
 	return a, a.form.updateActive(msg)
 }
 
-// submitForm builds the add command from the form and opens the confirm dialog.
+// submitForm builds the add — or, in edit mode, the replace-in-place — command
+// from the form and opens the confirm dialog.
 func (a *app) submitForm() tea.Cmd {
 	spec, err := a.form.spec()
 	if err != nil {
 		a.setStatus(ui.StatusError, err.Error())
 		return nil
 	}
-	change, err := a.backend.BuildAddRule(a.group, spec)
+	var change firewall.Change
+	if a.editing {
+		editor, ok := a.backend.(ruleEditor)
+		if !ok {
+			// Unreachable: editing is only set by startEdit, which required the
+			// interface. Refusing beats replacing a rule with an add.
+			a.setStatus(ui.StatusError, "this backend cannot edit a rule in place")
+			return nil
+		}
+		change, err = editor.BuildEditRule(a.group, a.editTarget, spec)
+	} else {
+		change, err = a.backend.BuildAddRule(a.group, spec)
+	}
 	if err != nil {
 		a.setStatus(ui.StatusError, err.Error())
 		return nil
 	}
+	a.editing = false
 	a.stageOrConfirm(change.Description, "The firewall will be changed as follows.", change)
 	return nil
 }
@@ -534,6 +562,14 @@ func (a *app) handleTableKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
 		return a, a.load()
 	case "a":
 		return a, a.startAdd()
+	case "E":
+		return a, a.startEdit()
+	case "K":
+		return a, a.confirmMove(-1)
+	case "J":
+		return a, a.confirmMove(1)
+	case "W":
+		return a, a.beginSave()
 	case "s":
 		a.toggleStaging()
 	case "S":
@@ -575,6 +611,7 @@ func (a *app) startAdd() tea.Cmd {
 		return a.openExtrasMenu()
 	default:
 		a.form = newRuleForm(a.caps, a.model.Services)
+		a.editing = false
 		a.mode = modeForm
 		return nil
 	}

cmd/tui-firewall/edit.go

@@ -0,0 +1,101 @@
+package main
+
+import (
+	"fmt"
+	"strings"
+
+	tea "github.com/charmbracelet/bubbletea"
+	"github.com/tui-tools/tui-firewall/internal/firewall"
+	"github.com/tui-tools/tui-firewall/internal/nftables/staging"
+	"github.com/tui-tools/tui-kit/ui"
+)
+
+// ruleEditor is the part of a backend the edit-in-place flow needs: the spec a
+// rule decomposes into, to pre-fill the form, and the replace-by-handle
+// command the submitted form builds. Only the nftables backend and its demo
+// implement it.
+type ruleEditor interface {
+	RuleSpecFor(group string, rule firewall.Rule) (firewall.RuleSpec, error)
+	BuildEditRule(group string, rule firewall.Rule,
+		spec firewall.RuleSpec) (firewall.Change, error)
+}
+
+// ruleMover is the part of a backend the move keys need. The Change it builds
+// is a copy plus a delete that only make sense as one transaction, which the
+// caller guarantees: staged into the atomic batch, or wrapped in a single
+// `nft -f` when staging is off.
+type ruleMover interface {
+	BuildMoveRule(group string, rule firewall.Rule,
+		delta int) (firewall.Change, error)
+}
+
+// startEdit opens the add-rule form pre-filled with the selected rule, in edit
+// mode: submitting replaces the rule in place, keeping its handle and
+// position. A rule the backend cannot decompose into the form is refused here,
+// before the form opens, with the backend's own reason.
+func (a *app) startEdit() tea.Cmd {
+	editor, ok := a.backend.(ruleEditor)
+	if !ok {
+		a.setStatus(ui.StatusWarn,
+			"this backend does not edit a rule in place: delete it with d and "+
+				"add the new one with a")
+		return nil
+	}
+	rule, ok := a.selectedRule()
+	if !ok {
+		a.setStatus(ui.StatusWarn, "no rule selected")
+		return nil
+	}
+	spec, err := editor.RuleSpecFor(a.group, rule)
+	if err != nil {
+		a.setStatus(ui.StatusError, err.Error())
+		return nil
+	}
+	a.form = newRuleForm(a.caps, a.model.Services)
+	a.form.title = fmt.Sprintf("Edit rule (replaces handle %s in place)", rule.ID)
+	a.form.prefill(spec)
+	a.editing = true
+	a.editTarget = rule
+	a.mode = modeForm
+	return nil
+}
+
+// confirmMove builds the up-or-down move for the selected rule. With staging
+// on it joins the pending batch, where the whole batch is already one nft
+// transaction; with staging off it is wrapped as a single `nft -f` so the copy
+// and the delete still apply together or not at all.
+func (a *app) confirmMove(delta int) tea.Cmd {
+	mover, ok := a.backend.(ruleMover)
+	if !ok {
+		a.setStatus(ui.StatusWarn,
+			"this backend does not reorder rules; rule order is only "+
+				"meaningful on nftables")
+		return nil
+	}
+	rule, ok := a.selectedRule()
+	if !ok {
+		a.setStatus(ui.StatusWarn, "no rule selected")
+		return nil
+	}
+	change, err := mover.BuildMoveRule(a.group, rule, delta)
+	if err != nil {
+		a.setStatus(ui.StatusError, err.Error())
+		return nil
+	}
+	if a.stagingOn && a.staging != nil {
+		a.stageOrConfirm(change.Description, "", change)
+		return nil
+	}
+
+	atomic := staging.AtomicCommand(change)
+	a.mode = modeConfirm
+	a.confirm = ui.Confirm{
+		Title: change.Description,
+		Body: "The statements below apply as one nft -f transaction: both of " +
+			"them, or neither.\n" + change.Note + ".",
+		Command: strings.TrimRight(atomic.Stdin, "\n"),
+		Danger:  true,
+		Payload: firewall.One(atomic),
+	}
+	return nil
+}