diff --git a/CHANGELOG.md b/CHANGELOG.md index c9b3c67..9745bd9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -124,6 +124,11 @@ All notable changes to Housecarl are documented here. The format follows ### Changed +- Moving a rule several places is one change. The Rules page has a + **Move…** button that puts a rule before any other of its chain (or at + the end) at once, and in a changeset a move right after a move of the + same rule takes its place, so ↑ three times stages one operation (and + moving it back where it was takes the operation out). - Statistics' "Busiest rules" (the Statistics page, `hc stats`, `GET /stats/summary`) lists only rules and NAT objects. Built-ins and policies have their own list ("Built-ins and policies", `builtins` in diff --git a/docs/design.md b/docs/design.md index e5b12e2..643513d 100644 --- a/docs/design.md +++ b/docs/design.md @@ -56,7 +56,10 @@ As in Jarl, a change can be made at once or staged in a **changeset**, to be reviewed and applied together with others: * Each change is an **operation** (create, update, delete or move an - object, or change the settings), kept as the API received it. + object, or change the settings), kept as the API received it. A move + staged right after a move of the same object takes its place: moving a + rule up three steps is one operation, and moving it back where it was + takes it out. * Staged in a changeset, an operation is checked as it is added: the changeset with it must still be valid, and nftables must accept the ruleset it makes. Nothing is applied. diff --git a/docs/rules.md b/docs/rules.md index b17830d..a4e40b4 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -130,6 +130,11 @@ drops or rejects decides. What no rule decides gets the chain's **policy** (Settings): by default input and forward drop, output accepts. +On the Rules page, ↑ and ↓ move a rule one place; **Move…** puts it +before any other rule of its chain, or at the end, in one change +(`hc rule move ID --before ID`). In a changeset, moves of the same rule +one after another are kept as one operation. + ```sh hc rule add input --from trusted --service ssh --comment "SSH from home" hc rule add input --in wg --service dns diff --git a/internal/server/h_changesets.go b/internal/server/h_changesets.go index 3bbb0d5..2955533 100644 --- a/internal/server/h_changesets.go +++ b/internal/server/h_changesets.go @@ -89,6 +89,17 @@ func (s *Server) stage(w http.ResponseWriter, r *http.Request, p *auth.Principal if err := replay(before, cs.Ops); err != nil { return conflict("changeset %d can't be applied as it is (%v); review it first", id, err) } + // A move right after a move of the same object takes its place: + // moving a rule up three steps is one operation. + kept := cs.Ops + if n := len(kept); n > 0 && foldMove(before, &kept[n-1], &op) { + kept = kept[:n-1] + before = s.eng.State() + if err := replay(before, kept); err != nil { + return conflict("changeset %d can't be applied as it is (%v); review it first", id, err) + } + } + folded := len(kept) < len(cs.Ops) after := before.Clone() objID, err := applyOp(after, &op) if err != nil { @@ -98,6 +109,13 @@ func (s *Server) stage(w http.ResponseWriter, r *http.Request, p *auth.Principal after.Sort() changes := engine.Diff(before, after) if len(changes) == 0 { + // Moved back where it was: the earlier move goes too. + if folded && !queryBool(r, "dry_run") { + cs.Ops = kept + if err := s.store.UpdateChangeset(r.Context(), cs); err != nil { + return err + } + } writeJSON(w, http.StatusOK, api.MutationResponse{Changes: []model.Change{}, Changeset: cs}) return nil } @@ -108,7 +126,7 @@ func (s *Server) stage(w http.ResponseWriter, r *http.Request, p *auth.Principal op.Summary = engine.UndoDescription(&h) } op.AddedAt = s.nowFn() - for _, o := range cs.Ops { + for _, o := range kept { if o.Seq >= op.Seq { op.Seq = o.Seq + 1 } @@ -116,7 +134,7 @@ func (s *Server) stage(w http.ResponseWriter, r *http.Request, p *auth.Principal if op.Seq == 0 { op.Seq = 1 } - ops := append(append([]model.Op{}, cs.Ops...), op) + ops := append(append([]model.Op{}, kept...), op) dry := queryBool(r, "dry_run") // The whole changeset, checked as it would be applied. res, err := s.eng.Apply(r.Context(), p, "", engine.Options{DryRun: true, Via: via(r, p)}, func(st *model.State, _ func() int64) error { @@ -145,6 +163,31 @@ func (s *Server) stage(w http.ResponseWriter, r *http.Request, p *auth.Principal return nil } +// foldMove reports whether op, a move, can take the place of prev, the +// changeset's last operation: a move of the same object. A move puts +// the object relative to the others, which an earlier move of it +// doesn't change, so only the chain it moved to has to be carried over. +// st is the changeset so far. +func foldMove(st *model.State, prev, op *model.Op) bool { + if prev.Op != model.OpMove || op.Op != model.OpMove || prev.Kind != op.Kind { + return false + } + a, err1 := findObject(st, prev.Kind, prev.Ref) + b, err2 := findObject(st, op.Kind, op.Ref) + if err1 != nil || err2 != nil || a.GetID() != b.GetID() { + return false + } + var was, now api.MoveInput + if json.Unmarshal(prev.Data, &was) != nil || json.Unmarshal(op.Data, &now) != nil { + return false + } + if now.Chain == "" && was.Chain != "" { + now.Chain = was.Chain + op.Data, _ = json.Marshal(now) + } + return true +} + func (s *Server) createChangeset(w http.ResponseWriter, r *http.Request, p *auth.Principal) error { if err := require(p, auth.ResConfig, true); err != nil { return err diff --git a/internal/server/server_test.go b/internal/server/server_test.go index bd4badc..54fe2f1 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -1336,6 +1336,53 @@ func TestCreatePlaced(t *testing.T) { } } +// TestStagedMovesFold: moving a rule several steps in a changeset stays +// one operation, and moving it back where it was takes it out. +func TestStagedMovesFold(t *testing.T) { + e := newEnv(t) + var ids []int64 + for _, svc := range []string{"ssh", "dns", "http", "https"} { + var mr api.MutationResponse + var r model.Rule + e.call(e.token, "POST", "/rules", model.Rule{Chain: "input", Match: model.Match{Services: []string{svc}}}, &mr) + json.Unmarshal(mr.Object, &r) + ids = append(ids, r.ID) + } + var cs model.Changeset + e.call(e.token, "POST", "/changesets", nil, &cs) + q := "?changeset=" + itoa(cs.ID) + var mr api.MutationResponse + for _, before := range []int64{ids[2], ids[1], ids[0]} { // https up three steps + if code := e.call(e.token, "POST", "/rules/"+itoa(ids[3])+"/move"+q, api.MoveInput{Before: before}, &mr); code != 200 || !mr.Staged { + t.Fatalf("move before %d: %d %+v", before, code, mr) + } + } + if n := len(mr.Changeset.Ops); n != 1 { + t.Fatalf("%d operations, want 1: %+v", n, mr.Changeset.Ops) + } + // Another operation in between: the next move is its own. + e.call(e.token, "PATCH", "/rules/"+itoa(ids[0])+q, map[string]any{"comment": "x"}, nil) + e.call(e.token, "POST", "/rules/"+itoa(ids[3])+"/move"+q, api.MoveInput{After: ids[0]}, &mr) + if n := len(mr.Changeset.Ops); n != 3 { + t.Errorf("%d operations after a patch and a move, want 3", n) + } + // Back to the start: the same place as before that move, so it goes. + e.call(e.token, "POST", "/rules/"+itoa(ids[3])+"/move"+q, api.MoveInput{To: "start"}, &mr) + if n := len(mr.Changeset.Ops); n != 2 { + t.Errorf("%d operations after moving back, want 2", n) + } + if code := e.call(e.token, "POST", "/changesets/"+itoa(cs.ID)+"/apply", nil, nil); code != 200 { + t.Fatalf("apply: %d", code) + } + var out []string + for _, r := range e.eng.State().Rules { + out = append(out, strings.Join(r.Services, "")) + } + if o := strings.Join(out, ","); o != "https,ssh,dns,http" { + t.Errorf("order: %s", o) + } +} + // TestTokenForUser: an administrator makes a token for another user (the // web UI's New token on a user's row); that user can't make one for // someone else. diff --git a/internal/web/static/assets/pages/rules.js b/internal/web/static/assets/pages/rules.js index 6563f73..8e30e5f 100644 --- a/internal/web/static/assets/pages/rules.js +++ b/internal/web/static/assets/pages/rules.js @@ -62,6 +62,7 @@ export async function render(el, params, query) { return [ h("button", { class: "small reveal", title: "Move up", disabled: i === 0, onclick: () => move(r, { before: rs[i - 1].id }) }, "↑"), " ", h("button", { class: "small reveal", title: "Move down", disabled: i === rs.length - 1, onclick: () => move(r, { after: rs[i + 1].id }) }, "↓"), " ", + h("button", { class: "small reveal", title: "Move to another place, in one change", disabled: rs.length < 2, onclick: () => moveDialog(r, rs, (x) => live(rules).indexOf(x) + 1) }, "Move…"), " ", h("button", { class: "small reveal", onclick: () => ruleDialog(r, chain) }, "Edit"), " ", h("button", { class: "small reveal", onclick: () => toggle(r) }, r.disabled ? "Enable" : "Disable"), " ", h("button", { class: "small reveal", onclick: () => del(r) }, "Delete"), @@ -245,6 +246,28 @@ async function move(r, where) { try { await change("POST", "/rules/" + r.id + "/move", where, { quiet: true }); rerender(); } catch (e) { modal("Not moved", errorBox(e)); } } +// moveDialog puts a rule anywhere among rs (the rules it can move +// among) in one move: one change, or one staged operation. +async function moveDialog(r, rs, order) { + const others = rs.filter((x) => x !== r); + const label = (x) => order(x) + ". " + (x.comment || (x.raw ? x.raw : describe(x))) + " (#" + x.id + ")"; + const next = rs[rs.indexOf(r) + 1]; + const where = select([...others.map((x) => [String(x.id), "before " + label(x)]), ["end", "after " + label(others[others.length - 1])]], + next ? String(next.id) : "end"); + const err = h("div"); + await modal("Move " + (r.comment || "rule #" + r.id), h("div", null, h("div", { class: "form-grid" }, field("Put it", where)), err), [ + { label: "Cancel", value: null }, + { label: "Move", kind: "primary", onclick: async () => { + const v = where.value; + if (v === (next ? String(next.id) : "end")) return true; // where it is + try { + await change("POST", "/rules/" + r.id + "/move", v === "end" ? { after: others[others.length - 1].id } : { before: Number(v) }, { quiet: true }); + rerender(); + } catch (e) { mount(err, errorBox(e)); return false; } + } }, + ]); +} + async function toggle(r) { try { await change("PATCH", "/rules/" + r.id, { disabled: !r.disabled }); rerender(); } catch (e) { modal("Not changed", errorBox(e)); } }