fix(approval): allow non-breaking schema changes inline

2026-05-14 12:35:39 +00:00
parent 9ec723dd17
commit d338c52c63
3 changed files with 177 additions and 38 deletions
@@ -9,6 +9,7 @@ import cloud.nstart.terravault.ordinis.outbox.OutboxRecorder;
 import cloud.nstart.terravault.ordinis.restapi.audit.AuditLogger;
 import cloud.nstart.terravault.ordinis.restapi.dto.CreateDictionaryRequest;
 import cloud.nstart.terravault.ordinis.restapi.error.OrdinisException;
+import cloud.nstart.terravault.ordinis.restapi.service.schema.SchemaCompatibility;
 import com.fasterxml.jackson.databind.JsonNode;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Service;
@@ -126,39 +127,46 @@ public class DictionaryDefinitionService {
    var existing = findByName(name);
    var prevSchema = existing.getSchemaJson();

-    // Approval gate: dictionaries marked approvalRequired must route SCHEMA
-    // changes through the schema-draft workflow. Non-schema metadata (display
-    // name, description, locales, projection flag) are still allowed inline —
-    // those don't affect record validation. Two backdoors closed here:
-    //   1. Editing schemaJson on an approvalRequired dict bypassed review
-    //      (the SchemaDrivenForm "edit schema" button hits PUT /{name},
-    //      which had no gate). Reported 2026-05-14.
-    //   2. Same request could simultaneously flip approvalRequired:true→false
-    //      (lines below) and rewrite schemaJson — review-bypass via toggle.
-    //      We block both: if the dict is currently approvalRequired and the
-    //      request either changes schemaJson or tries to flip the flag off,
-    //      reject with 422 approval_required.
-    boolean schemaChanged = !java.util.Objects.equals(prevSchema, req.schemaJson());
+    // Approval gate (revised 2026-05-14 after first-day-in-prod feedback):
+    // approvalRequired dicts must route BREAKING schema changes + governance
+    // toggles through the schema-draft workflow. Non-breaking edits — adding
+    // an optional field, widening an enum, editing description / title —
+    // pass inline (verified by SchemaCompatibility.isBreaking).
+    //
+    // Three clauses, all with the same remedy (open a draft):
+    //   1. schemaBreaking          — add required field, type change, etc.
+    //   2. tryingToDisableApproval — flip approvalRequired:true→false
+    //                                 (rewrites the gate itself).
+    //   3. tryingToChangeMinRole   — change/clear approvalMinRole; would let
+    //                                 a maker self-approve restricted-dict
+    //                                 drafts via a generic reviewer role.
+    //
+    // SchemaCompatibility.isBreaking is conservative — false positives are
+    // recoverable (open a draft that turns out to be trivial) but false
+    // negatives let real breaking changes slip past review.
+    boolean schemaBreaking =
+        SchemaCompatibility.isBreaking(prevSchema, req.schemaJson());
    boolean tryingToDisableApproval =
        req.approvalRequired() != null && !req.approvalRequired();
-    // Approval-min-role flip is a third backdoor: a maker with PUT permission
-    // could clear approvalMinRole (send "" → stored as null) and then approve
-    // their own restricted-dict drafts using a generic RECORD_REVIEWER role.
-    // Treat any change to approvalMinRole the same as a schema rewrite —
-    // route through the draft workflow (or eventually a dedicated admin
-    // endpoint that requires a higher role than `approvalMinRole` itself).
+    // Approval-min-role flip is a separate backdoor: a maker with PUT
+    // permission could clear approvalMinRole (send "" → stored as null) and
+    // then approve their own restricted-dict drafts using a generic
+    // RECORD_REVIEWER role. Treat any change to approvalMinRole the same as
+    // a breaking schema change — route through the draft workflow.
    boolean tryingToChangeMinRole =
        req.approvalMinRole() != null
            && !java.util.Objects.equals(
                req.approvalMinRole().isBlank() ? null : req.approvalMinRole(),
                existing.getApprovalMinRole());
    if (existing.isApprovalRequired()
-        && (schemaChanged || tryingToDisableApproval || tryingToChangeMinRole)) {
+        && (schemaBreaking || tryingToDisableApproval || tryingToChangeMinRole)) {
      throw OrdinisException.unprocessableEntity(
          "approval_required",
-          "Schema changes, approvalRequired toggle-off, and approvalMinRole "
-              + "edits all require the draft workflow on this dictionary. "
-              + "Use POST /api/v1/admin/dictionaries/" + name + "/schema-drafts.");
+          "Breaking schema changes, approvalRequired toggle-off, and "
+              + "approvalMinRole edits require the draft workflow on this "
+              + "dictionary. Use POST /api/v1/admin/dictionaries/" + name
+              + "/schema-drafts. Non-breaking edits (add optional field, "
+              + "widen enum, edit description) pass inline.");
    }

    existing.setDisplayName(req.displayName());
@@ -89,21 +89,14 @@ public class SchemaPatchService {
      throw OrdinisException.notFound(
          "dictionary_not_found", "Dictionary not found: " + dictName);
    }
-    // Approval gate: dictionaries marked approvalRequired must route ALL
-    // schema changes — even non-breaking inline patches — through the
-    // schema-draft maker-checker workflow. Otherwise a maker could bypass
-    // review by relabeling fields (description / title) or expanding
-    // numeric/enum ranges. Records service already enforces this for row
-    // CRUD (see DictionaryRecordService line ~270); this closes the same
-    // gap for schema patches. Reported 2026-05-14 — schema edit on an
-    // approvalRequired dict was applying inline without review.
-    if (d.isApprovalRequired()) {
-      throw OrdinisException.unprocessableEntity(
-          "approval_required",
-          "Schema changes require the draft workflow on this dictionary. "
-              + "Use POST /api/v1/admin/dictionaries/" + dictName
-              + "/schema-drafts instead.");
-    }
+    // No approval gate here on purpose. PATCH /schema is restricted by
+    // construction to non-breaking mutations: ALLOWED_KEYS = description /
+    // title / minimum / maximum / enum, with validators that reject numeric
+    // narrowing and enum removal (lines below). After the policy revision
+    // 2026-05-14 — non-breaking edits pass inline even on approvalRequired
+    // dicts — this endpoint is safe to leave open. PUT /{name} carries the
+    // breaking-vs-non-breaking gate via SchemaCompatibility because it
+    // accepts arbitrary schema rewrites.
    if (!d.getSchemaVersion().equals(req.expectedHeadVersion())) {
      throw OrdinisException.conflict(
          "version_mismatch",
@@ -0,0 +1,138 @@
+package cloud.nstart.terravault.ordinis.restapi.service.schema;
+
+import com.fasterxml.jackson.databind.JsonNode;
+
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Detect whether a JSON-schema change is "breaking" — i.e. could invalidate
+ * existing records or break downstream consumers.
+ *
+ * <p>Used by two callers with different stakes:
+ *
+ * <ol>
+ *   <li>Approval gate ({@code DictionaryDefinitionService.updateSchema} when
+ *       the dict has {@code approvalRequired=true}): breaking changes MUST
+ *       go through the schema-draft workflow; non-breaking changes (add
+ *       optional field, widen enum, edit description) may apply inline.
+ *       <b>Security-sensitive</b> — false negatives here let a maker
+ *       bypass review by mislabeling a real breaking change.</li>
+ *   <li>Semver bump ({@code SchemaDraftService.computeNextVersion}):
+ *       breaking → major, otherwise minor. False negatives just under-bump
+ *       the version, no security impact.</li>
+ * </ol>
+ *
+ * <p>Heuristic, not a complete JSON-schema diff. Conservative wins ties:
+ * if we can't tell, return {@code true} (treat as breaking) — false
+ * positives are recoverable (admin opens a draft that turns out to be
+ * trivial) but false negatives are not.
+ */
+public final class SchemaCompatibility {
+
+  private SchemaCompatibility() {
+    // utility
+  }
+
+  /**
+   * @return {@code true} if the transition from {@code oldSchema} to
+   *         {@code newSchema} is breaking.
+   *         {@code false} when both are null, equal, or differ only in
+   *         compatible ways (added optional field, description / title /
+   *         maximum widened, enum values added, etc.).
+   */
+  public static boolean isBreaking(JsonNode oldSchema, JsonNode newSchema) {
+    if (oldSchema == null || newSchema == null) {
+      // No baseline → can't classify. Caller decides: definitionService
+      // never calls this when prevSchema is null (only on edits).
+      return false;
+    }
+    if (oldSchema.equals(newSchema)) return false;
+
+    // === required[] changes ===
+    Set<String> oldReq = readStringArray(oldSchema.path("required"));
+    Set<String> newReq = readStringArray(newSchema.path("required"));
+
+    // Removed required key → breaking (consumers may have relied on the
+    // field always being present).
+    for (String k : oldReq) {
+      if (!newReq.contains(k)) return true;
+    }
+    // Added required key → breaking (existing records lacking that field
+    // fail validation).
+    for (String k : newReq) {
+      if (!oldReq.contains(k)) return true;
+    }
+
+    // === properties changes ===
+    JsonNode oldProps = oldSchema.path("properties");
+    JsonNode newProps = newSchema.path("properties");
+
+    if (oldProps.isObject()) {
+      var oldFieldNames = oldProps.fieldNames();
+      while (oldFieldNames.hasNext()) {
+        String fname = oldFieldNames.next();
+        JsonNode oldField = oldProps.path(fname);
+        JsonNode newField = newProps.path(fname);
+
+        // Removed field → breaking (consumers + reverse references break).
+        if (newField.isMissingNode()) return true;
+
+        // type changed → breaking.
+        JsonNode oldType = oldField.path("type");
+        JsonNode newType = newField.path("type");
+        if (!oldType.isMissingNode() && !newType.isMissingNode()
+            && !oldType.equals(newType)) {
+          return true;
+        }
+
+        // x-references changed → breaking (FK semantics — different target
+        // dict invalidates existing values).
+        JsonNode oldXref = oldField.path("x-references");
+        JsonNode newXref = newField.path("x-references");
+        if (!oldXref.isMissingNode() && !oldXref.equals(newXref)) {
+          return true;
+        }
+
+        // Numeric narrowing: minimum increased OR maximum decreased.
+        if (numericNarrowed(oldField, newField, "minimum", true)) return true;
+        if (numericNarrowed(oldField, newField, "maximum", false)) return true;
+
+        // Enum narrowed: any value present in old.enum missing in new.enum.
+        Set<String> oldEnum = readStringArray(oldField.path("enum"));
+        Set<String> newEnum = readStringArray(newField.path("enum"));
+        if (!oldEnum.isEmpty() && !newEnum.containsAll(oldEnum)) return true;
+      }
+    }
+
+    // Adding a NEW property (in newProps but not oldProps) is non-breaking
+    // ONLY when it's not in newRequired. That case is already covered by
+    // the "added required key" check above.
+
+    return false;
+  }
+
+  private static Set<String> readStringArray(JsonNode arr) {
+    Set<String> out = new HashSet<>();
+    if (arr.isArray()) {
+      for (JsonNode item : arr) {
+        out.add(item.asText());
+      }
+    }
+    return out;
+  }
+
+  /**
+   * @param tighten {@code true} for "minimum" (narrowing means going UP),
+   *                {@code false} for "maximum" (narrowing means going DOWN).
+   */
+  private static boolean numericNarrowed(
+      JsonNode oldField, JsonNode newField, String key, boolean tighten) {
+    JsonNode oldVal = oldField.path(key);
+    JsonNode newVal = newField.path(key);
+    if (!oldVal.isNumber() || !newVal.isNumber()) return false;
+    double o = oldVal.doubleValue();
+    double n = newVal.doubleValue();
+    return tighten ? n > o : n < o;
+  }
+}