fix(import): handle bindings that already exist on re-import

doc/user-guide/policies.md

@@ -302,6 +302,16 @@ The policy state is not exported.
     | **Skip that policy** | The existing policy is kept unchanged. |
     | **Update the existing draft** | The existing policy's content is replaced, but only if it is a draft; a released or archived policy is an error. |
 
+    The same option applies to a binding in the file whose policy and
+    node group are already bound on the server, which is the usual case
+    when a file is imported a second time:
+
+    | Option | Effect on an existing binding |
+    |---|---|
+    | **Fail the import (recommended)** | The document is an error and nothing is imported. |
+    | **Skip that policy** | The existing binding is kept unchanged. |
+    | **Update the existing draft** | The priority is set to the one in the file. A binding that already has that priority is reported as `skipped`. |
+
     Changing the option re-runs the preview.
 4. If the preview is valid, select **Import N resources**. Otherwise fix
    the file; if any document has an error, nothing is imported.
@@ -312,6 +322,9 @@ Rules:
   yourself.
 - Imported bindings are always created **disabled**. Enable them on
   [Policy Bindings](bindings.md) after releasing the policy.
+- An import never enables or disables a binding that already exists;
+  only its priority can change.
+- A file may bind a policy to a node group only once.
 - A binding's `spec.policy` refers to a `metadata.name` in the same file or
   to the name of a policy that already exists on the server. Its
   `spec.group` must be the name of an existing node group; node groups are

server/internal/api/export.go

@@ -30,6 +30,9 @@ type ExportHandler struct {
 	// az backs the systemd:exec check on import; nil refuses every
 	// command-running Systemd policy in a bundle (fail closed).
 	az authz.Authorizer
+	// OnBindingChange is called after an import changes the priority of an
+	// enabled binding, so the nodes of that group pick up the new order.
+	OnBindingChange func(b *models.PolicyBinding)
 }
 
 // NewExportHandler wires the export/import endpoints to their services.
@@ -334,12 +337,48 @@ func (s scopedPolicyStore) UpdatePolicy(ctx context.Context, id string, req *mod
 }
 
 // scopedBindingStore only creates bindings to in-scope groups, for policies
-// the caller can see.
+// the caller can see. Existing bindings are matched among the ones the caller
+// can view, and changing one needs the same reach as the bindings API.
 type scopedBindingStore struct {
 	h *ExportHandler
 	r *http.Request
 }
 
+func (s scopedBindingStore) ListBindings(ctx context.Context) ([]*models.PolicyBindingWithDetails, error) {
+	all, err := s.h.bindingSvc.ListBindings(ctx)
+	if err != nil {
+		return nil, err
+	}
+	scope := requestScope(s.r, "binding", "view")
+	visible := make([]*models.PolicyBindingWithDetails, 0, len(all))
+	for _, b := range all {
+		if scope.Allows(b.GroupID) {
+			visible = append(visible, b)
+		}
+	}
+	return visible, nil
+}
+
+func (s scopedBindingStore) UpdateBinding(ctx context.Context, id string, req *models.UpdatePolicyBindingRequest) (*models.PolicyBinding, error) {
+	existing, err := s.h.bindingSvc.GetBinding(ctx, id)
+	if err != nil || existing == nil || !requestScope(s.r, "binding", "view").Allows(existing.GroupID) {
+		return nil, fmt.Errorf("policy binding not found")
+	}
+	if !requestScope(s.r, "binding", "toggle").Allows(existing.GroupID) {
+		return nil, errOutOfScope
+	}
+	updated, err := s.h.bindingSvc.UpdateBinding(ctx, id, req)
+	if err != nil {
+		return nil, err
+	}
+	// A disabled binding delivers nothing, so its priority is not yet
+	// visible to any node.
+	if s.h.OnBindingChange != nil && updated != nil && updated.State == models.BindingStateEnabled {
+		s.h.OnBindingChange(updated)
+	}
+	return updated, nil
+}
+
 func (s scopedBindingStore) CreateBinding(ctx context.Context, req *models.CreatePolicyBindingRequest) (*models.PolicyBinding, error) {
 	if !requestScope(s.r, "binding", "create").Allows(req.GroupID) {
 		return nil, errOutOfScope

server/internal/export/export_test.go

@@ -188,7 +188,20 @@ func (f *fakePolicies) UpdatePolicy(_ context.Context, id string, req *models.Up
 }
 
 type fakeBindings struct {
-	created []*models.CreatePolicyBindingRequest
+	existing []*models.PolicyBindingWithDetails
+	created  []*models.CreatePolicyBindingRequest
+	updated  map[string]*models.UpdatePolicyBindingRequest
+}
+
+func (f *fakeBindings) ListBindings(_ context.Context) ([]*models.PolicyBindingWithDetails, error) {
+	return f.existing, nil
+}
+func (f *fakeBindings) UpdateBinding(_ context.Context, id string, req *models.UpdatePolicyBindingRequest) (*models.PolicyBinding, error) {
+	if f.updated == nil {
+		f.updated = map[string]*models.UpdatePolicyBindingRequest{}
+	}
+	f.updated[id] = req
+	return &models.PolicyBinding{ID: id}, nil
 }
 
 func (f *fakeBindings) CreateBinding(_ context.Context, req *models.CreatePolicyBindingRequest) (*models.PolicyBinding, error) {
@@ -307,6 +320,106 @@ func TestImportConflictModes(t *testing.T) {
 	}
 }
 
+// A bundle that is imported again meets the bindings it created the first
+// time; they follow the conflict mode instead of being inserted twice.
+func TestImportExistingBinding(t *testing.T) {
+	nameRefBundle := `apiVersion: bor.dev/v1
+kind: PolicyBinding
+metadata:
+  name: firefox-eng
+spec:
+  policy: "Firefox Baseline"
+  group: "Engineering"
+  priority: 10
+`
+	tests := []struct {
+		name         string
+		bundle       string
+		onConflict   string
+		priority     int // priority of the binding already on the server
+		wantOk       bool
+		wantStatus   string // status of the PolicyBinding document
+		wantMessage  string // substring of its message
+		wantPriority int    // priority written by UpdateBinding; 0 = no update
+	}{
+		{name: "new-version updates a different priority", bundle: importBundle, onConflict: OnConflictNewVersion, priority: 50, wantOk: true, wantStatus: "updated", wantMessage: "from 50 to 10", wantPriority: 10},
+		{name: "new-version leaves an equal priority", bundle: importBundle, onConflict: OnConflictNewVersion, priority: 10, wantOk: true, wantStatus: "skipped", wantMessage: "same priority"},
+		{name: "skip leaves the binding", bundle: importBundle, onConflict: OnConflictSkip, priority: 50, wantOk: true, wantStatus: "skipped", wantMessage: "binding exists"},
+		{name: "error rejects the bundle", bundle: nameRefBundle, onConflict: OnConflictError, priority: 50, wantOk: false, wantStatus: "error", wantMessage: "already exists"},
+		{name: "new-version by policy name", bundle: nameRefBundle, onConflict: OnConflictNewVersion, priority: 50, wantOk: true, wantStatus: "updated", wantMessage: "from 50 to 10", wantPriority: 10},
+		{name: "skip by policy name", bundle: nameRefBundle, onConflict: OnConflictSkip, priority: 50, wantOk: true, wantStatus: "skipped", wantMessage: "binding exists"},
+	}
+	for _, tc := range tests {
+		for _, dryRun := range []bool{true, false} {
+			t.Run(fmt.Sprintf("%s/dry_run=%t", tc.name, dryRun), func(t *testing.T) {
+				fp, fb, fg := importFixture()
+				fp.existing = []*models.Policy{{ID: "p1", Name: "Firefox Baseline", Type: "Firefox", State: "draft"}}
+				fb.existing = []*models.PolicyBindingWithDetails{
+					{PolicyBinding: models.PolicyBinding{ID: "b1", PolicyID: "p1", GroupID: "g1", State: models.BindingStateEnabled, Priority: tc.priority}},
+					// Same group, other policy: must not be mistaken for a match.
+					{PolicyBinding: models.PolicyBinding{ID: "b2", PolicyID: "p2", GroupID: "g1", Priority: 1}},
+				}
+				report, err := Import(context.Background(), fp, fb, fg, nil, []byte(tc.bundle), Options{DryRun: dryRun, OnConflict: tc.onConflict})
+				if err != nil {
+					t.Fatal(err)
+				}
+				if report.Ok != tc.wantOk {
+					t.Fatalf("ok = %t, want %t: %+v", report.Ok, tc.wantOk, report)
+				}
+				var got *DocResult
+				for i := range report.Results {
+					if report.Results[i].Kind == "PolicyBinding" {
+						got = &report.Results[i]
+					}
+				}
+				if got == nil || got.Status != tc.wantStatus || !strings.Contains(got.Message, tc.wantMessage) {
+					t.Fatalf("binding result = %+v, want status %q with %q", got, tc.wantStatus, tc.wantMessage)
+				}
+				if len(fb.created) != 0 {
+					t.Fatalf("existing binding inserted again: %+v", fb.created)
+				}
+				wantUpdates := 0
+				if tc.wantPriority != 0 && !dryRun {
+					wantUpdates = 1
+				}
+				if len(fb.updated) != wantUpdates {
+					t.Fatalf("got %d binding updates, want %d", len(fb.updated), wantUpdates)
+				}
+				if wantUpdates == 1 {
+					req := fb.updated["b1"]
+					if req == nil || req.Priority == nil || *req.Priority != tc.wantPriority {
+						t.Fatalf("priority not updated on b1: %+v", fb.updated)
+					}
+					if req.State != nil {
+						t.Fatalf("import must not change the binding state: %v", *req.State)
+					}
+				}
+			})
+		}
+	}
+}
+
+func TestImportDuplicateBindingInBundle(t *testing.T) {
+	bundle := importBundle + `---
+apiVersion: bor.dev/v1
+kind: PolicyBinding
+metadata:
+  name: firefox-eng-again
+spec:
+  policy: firefox-baseline
+  group: "Engineering"
+  priority: 20
+`
+	fp, fb, fg := importFixture()
+	report, err := Import(context.Background(), fp, fb, fg, nil, []byte(bundle), Options{})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if report.Ok || report.Errors != 1 || len(fp.created) != 0 || len(fb.created) != 0 {
+		t.Fatalf("duplicate binding must reject the bundle: %+v", report)
+	}
+}
+
 func TestImportMissingGroupRejectsBundle(t *testing.T) {
 	fp, fb, fg := importFixture()
 	fg.groups = nil

server/internal/export/import.go

@@ -23,6 +23,8 @@ type PolicyStore interface {
 
 // BindingStore is the slice of PolicyBindingService the importer needs.
 type BindingStore interface {
+	ListBindings(ctx context.Context) ([]*models.PolicyBindingWithDetails, error)
+	UpdateBinding(ctx context.Context, id string, req *models.UpdatePolicyBindingRequest) (*models.PolicyBinding, error)
 	CreateBinding(ctx context.Context, req *models.CreatePolicyBindingRequest) (*models.PolicyBinding, error)
 }
 
@@ -31,7 +33,8 @@ type GroupStore interface {
 	ListNodeGroups(ctx context.Context) ([]*models.NodeGroup, error)
 }
 
-// Conflict handling for policies whose display name already exists.
+// Conflict handling for policies whose display name already exists and for
+// bindings whose policy and node group are already bound.
 const (
 	OnConflictError      = "error"
 	OnConflictSkip       = "skip"
@@ -88,8 +91,14 @@ type pendingBinding struct {
 	priority   int
 	policySlug bool // ref resolved against an in-file slug
 	policyID   string
+	existing   *models.PolicyBinding // binding already on the server for this policy and group
+	skip       bool                  // leave the existing binding untouched
+	skipReason string
 }
 
+// bindingKey identifies a binding by the pair the database keeps unique.
+type bindingKey struct{ policyID, groupID string }
+
 // Import validates the whole bundle first and creates resources only when
 // every document is valid ("validate all, then execute"): nothing is written
 // for a bundle containing any invalid document. Execution-stage failures
@@ -131,11 +140,20 @@ func Import(ctx context.Context, policies PolicyStore, bindings BindingStore, gr
 	for _, g := range groupList {
 		groupByName[g.Name] = g.ID
 	}
+	bindingList, err := bindings.ListBindings(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("failed to list policy bindings: %w", err)
+	}
+	bindingByKey := make(map[bindingKey]*models.PolicyBinding, len(bindingList))
+	for _, eb := range bindingList {
+		bindingByKey[bindingKey{eb.PolicyID, eb.GroupID}] = &eb.PolicyBinding
+	}
 
 	// ── Pass 1: parse + validate every document ──
 	var pendingPolicies []*pendingPolicy
 	var pendingBindings []*pendingBinding
 	slugs := make(map[string]*pendingPolicy)
+	bindingsInFile := make(map[bindingKey]bool)
 	namesInFile := make(map[string]bool)
 
 	for i, doc := range docs {
@@ -238,14 +256,47 @@ func Import(ctx context.Context, policies PolicyStore, bindings BindingStore, gr
 				continue
 			}
 			b.groupID = gid
-			if _, ok := slugs[b.policyRef]; ok {
+			// existingPolicyID is the server-side policy the binding will
+			// attach to, when there is one already: a policy that is updated
+			// or skipped keeps its ID, a policy the bundle creates has none.
+			existingPolicyID := ""
+			if p, ok := slugs[b.policyRef]; ok {
 				b.policySlug = true
+				if prev, exists := byName[p.name]; exists {
+					existingPolicyID = prev.ID
+				}
 			} else if prev, ok := byName[b.policyRef]; ok {
 				b.policyID = prev.ID
+				existingPolicyID = prev.ID
 			} else {
 				fail(n, res.Kind, name, fmt.Sprintf("policy %q not found in bundle or on this server", b.policyRef))
 				continue
 			}
+			fileKey := bindingKey{existingPolicyID, gid}
+			if existingPolicyID == "" {
+				fileKey.policyID = "slug:" + b.policyRef
+			}
+			if bindingsInFile[fileKey] {
+				fail(n, res.Kind, name, "duplicate binding for this policy and node group in bundle")
+				continue
+			}
+			bindingsInFile[fileKey] = true
+			if prev, ok := bindingByKey[bindingKey{existingPolicyID, gid}]; ok && existingPolicyID != "" {
+				switch opts.OnConflict {
+				case OnConflictError:
+					fail(n, res.Kind, name, "a binding for this policy and node group already exists (on_conflict=error)")
+					continue
+				case OnConflictSkip:
+					b.skip = true
+					b.skipReason = "binding exists"
+				case OnConflictNewVersion:
+					b.existing = prev
+					if prev.Priority == b.priority {
+						b.skip = true
+						b.skipReason = "binding exists with the same priority"
+					}
+				}
+			}
 			pendingBindings = append(pendingBindings, b)
 		}
 	}
@@ -272,8 +323,18 @@ func Import(ctx context.Context, policies PolicyStore, bindings BindingStore, gr
 			}
 		}
 		for _, b := range pendingBindings {
-			report.Created++
-			report.Results = append(report.Results, DocResult{Doc: b.doc, Kind: "PolicyBinding", Name: b.name, Status: "created", Message: "would create"})
+			switch {
+			case b.skip:
+				report.Skipped++
+				report.Results = append(report.Results, DocResult{Doc: b.doc, Kind: "PolicyBinding", Name: b.name, Status: "skipped", Message: b.skipReason})
+			case b.existing != nil:
+				report.Updated++
+				report.Results = append(report.Results, DocResult{Doc: b.doc, Kind: "PolicyBinding", Name: b.name, Status: "updated",
+					Message: fmt.Sprintf("would change priority from %d to %d", b.existing.Priority, b.priority)})
+			default:
+				report.Created++
+				report.Results = append(report.Results, DocResult{Doc: b.doc, Kind: "PolicyBinding", Name: b.name, Status: "created", Message: "would create"})
+			}
 		}
 		report.Ok = true
 		return report, nil
@@ -324,6 +385,23 @@ func Import(ctx context.Context, policies PolicyStore, bindings BindingStore, gr
 		}
 	}
 	for _, b := range pendingBindings {
+		if b.skip {
+			report.Skipped++
+			report.Results = append(report.Results, DocResult{Doc: b.doc, Kind: "PolicyBinding", Name: b.name, Status: "skipped", Message: b.skipReason})
+			continue
+		}
+		if b.existing != nil {
+			// Only the priority follows the bundle; whether the binding is
+			// enabled stays an explicit decision made on the server.
+			if _, err := bindings.UpdateBinding(ctx, b.existing.ID, &models.UpdatePolicyBindingRequest{Priority: &b.priority}); err != nil {
+				fail(b.doc, "PolicyBinding", b.name, err.Error())
+				continue
+			}
+			report.Updated++
+			report.Results = append(report.Results, DocResult{Doc: b.doc, Kind: "PolicyBinding", Name: b.name, Status: "updated",
+				Message: fmt.Sprintf("priority changed from %d to %d", b.existing.Priority, b.priority)})
+			continue
+		}
 		id := b.policyID
 		if b.policySlug {
 			id = slugToID[b.policyRef]

server/pkg/app/app.go

@@ -585,6 +585,9 @@ func Run(opts Options) {
 	// import creates drafts and needs create. Both are audit-logged by the
 	// handler itself (exports are GETs, which the audit middleware skips).
 	exportHandler := api.NewExportHandler(policySvc, policyBindingSvc, nodeGroupSvc, auditSvc).WithAuthorizer(az)
+	exportHandler.OnBindingChange = func(b *models.PolicyBinding) {
+		policyHub.PublishResync(b.GroupID)
+	}
 	mux.Handle("/api/v1/policies/export", authMiddleware(gate.RequireScoped("policy", "view")(http.HandlerFunc(exportHandler.Export))))
 	mux.Handle("/api/v1/policies/import", authMiddleware(gate.RequireScoped("policy", "create")(http.HandlerFunc(exportHandler.Import))))