diff --git a/ordinis-rest-api/src/main/java/cloud/nstart/terravault/ordinis/restapi/service/DictionaryDefinitionService.java b/ordinis-rest-api/src/main/java/cloud/nstart/terravault/ordinis/restapi/service/DictionaryDefinitionService.java index 2cc34ce..da6df63 100644 --- a/ordinis-rest-api/src/main/java/cloud/nstart/terravault/ordinis/restapi/service/DictionaryDefinitionService.java +++ b/ordinis-rest-api/src/main/java/cloud/nstart/terravault/ordinis/restapi/service/DictionaryDefinitionService.java @@ -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()); diff --git a/ordinis-rest-api/src/main/java/cloud/nstart/terravault/ordinis/restapi/service/SchemaPatchService.java b/ordinis-rest-api/src/main/java/cloud/nstart/terravault/ordinis/restapi/service/SchemaPatchService.java index b232e8c..8c81d1f 100644 --- a/ordinis-rest-api/src/main/java/cloud/nstart/terravault/ordinis/restapi/service/SchemaPatchService.java +++ b/ordinis-rest-api/src/main/java/cloud/nstart/terravault/ordinis/restapi/service/SchemaPatchService.java @@ -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", diff --git a/ordinis-rest-api/src/main/java/cloud/nstart/terravault/ordinis/restapi/service/schema/SchemaCompatibility.java b/ordinis-rest-api/src/main/java/cloud/nstart/terravault/ordinis/restapi/service/schema/SchemaCompatibility.java new file mode 100644 index 0000000..f7506d9 --- /dev/null +++ b/ordinis-rest-api/src/main/java/cloud/nstart/terravault/ordinis/restapi/service/schema/SchemaCompatibility.java @@ -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. + * + *

Used by two callers with different stakes: + * + *

    + *
  1. 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. + * Security-sensitive — false negatives here let a maker + * bypass review by mislabeling a real breaking change.
    2. + *
  2. Semver bump ({@code SchemaDraftService.computeNextVersion}): + * breaking → major, otherwise minor. False negatives just under-bump + * the version, no security impact.
    3. + *
+ * + *

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 oldReq = readStringArray(oldSchema.path("required")); + Set 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 oldEnum = readStringArray(oldField.path("enum")); + Set 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 readStringArray(JsonNode arr) { + Set 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; + } +}