[#250] Reject invalid temporal constraint durations and tolerate stored ones

[vharseko]

Fixes #250

Problem

effectiveRoles is a virtual property returned by default, so its onRetrieve script runs on every read and every update of a user. The script parses each temporal constraint duration with Joda's Interval.parse, which throws on an interval whose end is before its start. Nothing validated the duration when a grant or a role was written, so once such a value was stored the user could neither be read nor fixed through managed/user (500 effectiveRoles onRetrieve script encountered exception).

The admin UI produced such a value itself: with a start date in the future and an empty end date, convertToIntervalString sent the current time as the end. Replaying that exact request on a build without the fix: POST managed/role/<id>/members returns 500 after the grant is already stored, and from then on GET managed/user/<id> and even the user's grant list return 500.

Changes

Reject on write (400 instead of storing the value)

• DateUtil.isValidInterval(String): true only if Interval.parse accepts the string (null, garbage and reversed intervals are invalid; datetime/period forms stay valid).

• RelationshipValidator.validateTemporalConstraints: grant constraints must be an array of at most one constraint with a valid duration. It runs:

  • in validateRelationship, before the managed object is written and before virtual properties are computed (fields with "validate": true, as in the default managed.json). For a grant that a managed object write updates (matched to the stored grant by _refProperties._id), only if its temporal constraints changed; a grant equal to one stored grant but carrying the _id of another is compared with the grant of its _id, which persisting it updates. A grant re-sent equal to a stored one but without its _refProperties._id is checked like a new grant, since persisting it deletes the stored grant and creates it again;
  • in RelationshipProvider.createInstance for every created grant, whatever the validate flag;
  • in RelationshipProvider.updateIfChanged (PUT and PATCH of a grant, and the grants persisted by a managed object update) only if the update changes the grant's temporal constraints.

  It replaces the "Only 1 temporal constraint" check in convertToRepoObject, which also ran on the stored value of a PATCH and on every unchanged grant persisted after a managed object update.

• conditionalRoles.roleCreate, and roleUpdate if the update changes the role's temporal constraints: the same check for a role's own temporal constraints.

Tolerate values that are already stored

• effectiveRoles.processConstraints and temporalConstraints.areConstraintsExpired skip an invalid constraint with a warning; it never grants the role.
• postOperation-roles.createJobsForConstraint logs and creates no schedules for an invalid duration or a null constraint instead of failing a request whose resource is already stored.
• A write that does not change a stored invalid constraint is not rejected, and a write that repairs it (e.g. PATCH managed/user/<id>/roles/<grantId> replacing the duration) succeeds.

Admin UI

• TemporalConstraintsUtils.isValidInterval requires both dates and the end after the start; TemporalConstraintsFormView shows an error under the end date, and EditRoleView (Save) and MembersDialog (Add) stay disabled while the form is invalid. EditRoleView.save also refuses to send an invalid form.

Upgrade note

Grants and roles stored before this change with an invalid temporal constraint duration (e.g. an end before the start) are left as they are. They no longer break reading the user and never grant the role; whenever the effective roles of such a user are calculated, the log shows Ignoring temporal constraint with an invalid duration <duration>. To find them, look for that warning, or list the grants with GET managed/user/<id>/roles?_queryFilter=true&_fields=_ref,_refProperties and the roles with GET managed/role?_queryFilter=true&_fields=temporalConstraints.

Writes that leave such a constraint unchanged keep working, whether through the relationship endpoint or through a PATCH of the managed object or a PUT with _fields=*,roles (_fields=*,members for a role), including writes that change other _refProperties of the grant. A plain PUT of the managed object does not read the stored roles/members, which are not returned by default, so every grant it carries is checked like a new one: a grant with a stored invalid constraint is rejected with 400 before anything is written. A write that changes it must make it valid, otherwise it is rejected with 400. Repair it by replacing the duration, e.g. PATCH managed/user/<id>/roles/<grantId> with replace /_refProperties/temporalConstraints/0/duration, or remove the constraint or the grant.

A managed object write that re-sends such a grant without its _refProperties._id is rejected with 400 before anything is written: persisting a grant without its _id deletes the stored grant and creates it again, so it is checked like a new grant. Send the grants with the _refProperties._id that a read of the managed object returns.

The admin UI does not save a role whose stored temporal constraint is invalid, not even an edit of another field: the form shows the error under the end date. Correct the dates in the form, or turn the temporal constraint off, to save the role. The server would reject most of these saves anyway, since the form keeps whole minutes and the end that the admin UI used to store carried seconds, so the rebuilt duration differs from the stored one.

Only when a relationship field does not have "validate": true (the default managed.json sets it on every relationship field), a managed object write that adds a grant with an invalid duration, or re-sends one without its _id, is rejected after the managed object itself has been written, since the check then runs when the grant is persisted.

Testing

• DateUtilTest; RelationshipValidatorTest (valid / invalid _refProperties with the expected message, changed / unchanged constraints, validateRelationship); CollectionRelationshipProviderTest on the managed object path: creating an invalid grant, updating a grant with a changed invalid / unchanged stored invalid constraint, PATCH that repairs / breaks the duration; validateRelationshipField with a grant that keeps a stored invalid constraint while another field changes, that changes it to an invalid one, that is re-sent with / without / with a null _id (a valid one re-sent without its _id is not read again), that carries the _id of another stored grant, and two changed grants each compared with the stored grant of its own _id. All openidm-util, openidm-core and openidm-zip tests pass.
• JS (ScriptRunnerTest): effectiveRolesTest, temporalConstraintsTest, conditionalRolesTest cover reversed, unparseable and null constraints, the warning for a skipped duration, and roleCreate / roleUpdate (rejecting a new or changed invalid constraint, keeping an unchanged stored one); the new postOperationRolesTest checks that an invalid duration or a null constraint on a role or a created / updated grant creates no schedule and does not fail. testRunner.js provides a no-op logger, as the scripts log through the binding OpenIDM supplies at runtime.
• Each new Java and JS test was checked against a mutant that removes the check it covers (or, for the unchanged / repair cases, restores the check in convertToRepoObject or makes roleUpdate validate unconditionally); every mutant fails the suite.
• QUnit: isValidInterval, isTemporalConstraintsFormValid (129 tests, 0 failed); eslint clean on the changed UI files.
• Manually, on a build of the first revision of this PR (the admin UI is unchanged since): role temporal constraint and "Add Role Members" with an empty end date show the error and keep Save / Add disabled; with a valid end date the role is saved (200) and the member is added (201). Over REST a reversed interval on a grant or a role is rejected with 400.

[maximthomas]

praise: The read side is fixed at the right place and is tested.

• effectiveRoles.isNowWithinConstraint skips an invalid constraint without granting the role. With the base effectiveRoles.js, effectiveRolesTest fails with the exception from the issue ("The end instant must be greater the start").
• DateUtil.isValidInterval catches both IllegalArgumentException and ArithmeticException, so null, garbage and reversed intervals all return false instead of throwing.
• RelationshipValidator.validateRelationship rejects the duration (:119) before it reads the referenced object.

---

issue (blocking): patchInstance validates the stored grant, so a PATCH that repairs an invalid duration is rejected.

openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipProvider.java:700-702, :849

patchInstance passes the old stored value (oldResource.getContent()) through convertToRepoObject before the patched one, and convertToRepoObject now calls validateTemporalConstraints. Take a grant stored with a reversed duration, which is the value #250 is about. PATCH managed/user/<id>/roles/<relId> that replaces /_refProperties/temporalConstraints/0/duration with a valid interval gets a 400: "Temporal constraint duration is not a valid ISO 8601 interval…". The admin UI edits a grant this way (RelationshipArrayView.updateRelationship → patchResourceDifferences). On the base this PATCH failed with a 500 at the getManagedObject read. With effectiveRoles fixed, this check is now the only thing stopping the edit. A PUT on the relationship, or DELETE and re-add, still works.

// RelationshipProvider.convertToRepoObject: remove
//     RelationshipValidator.validateTemporalConstraints(properties);

// RelationshipProvider.patchInstance, after the `if (!modified)` return:
RelationshipValidator.validateTemporalConstraints(newValue.get(FIELD_PROPERTIES));

Creates are still validated: validateRelationship (RelationshipValidator.java:119) runs on both create paths. On a direct create it runs through validateRelationshipOperand. On the managed-object path it runs through ManagedObjectSet.validateRelationshipFields.

---

issue (blocking): A managed-object write that carries roles/members re-validates every unchanged stored grant after the object is committed.

openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipProvider.java:503, openidm-core/src/main/java/org/forgerock/openidm/managed/CollectionRelationshipProvider.java:197-201, openidm-core/src/main/java/org/forgerock/openidm/managed/ManagedObjectSet.java:574-596

ManagedObjectSet.update validates only new or changed items before the write: validateRelationshipField skips items equal to the stored ones. It then commits the object (:590), and persistRelationships sends every item that has an _id through updateInstance, whose first statement is convertToRepoObject. Take a user holding a stored invalid grant and send PATCH managed/user/<id> with add /roles/-. The same applies to a PUT with roles, a recon that maps roles, and a conditional-role update that rewrites members (conditionalRoles.js:117-118). The response is a 400 naming a grant the request never touched. By then the user document, the clearNotIn deletions and the new grant are already persisted. On the base the same request failed with a 500 in populateVirtualProperties, before the commit. I traced this by reading the code and did not run it, which would need a server with a seeded grant.

// RelationshipProvider.updateInstance, with the call removed from convertToRepoObject:
if (!context.containsContext(ManagedObjectContext.class)) {
    RelationshipValidator.validateTemporalConstraints(request.getContent().get(FIELD_PROPERTIES));
}
final JsonValue newValue = convertToRepoObject(firstResourcePath(context, request), request.getContent());

The managed-object path has already validated every changed item before the commit, in validateRelationship.

---

suggestion (non-blocking): Neither Java call site of validateTemporalConstraints is covered by a test.

openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipValidator.java:119, openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipProvider.java:849, openidm-core/src/test/java/org/forgerock/openidm/managed/RelationshipValidatorTest.java:264

The new data-provider cases call the static method directly. Deleting both calls also drops the old "Only 1 temporal constraint" check from the write path, and openidm-core still passes, 95/95 (mvn -o -pl openidm-util,openidm-core test).

@Test(expectedExceptions = BadRequestException.class,
        expectedExceptionsMessageRegExp = "Temporal constraint duration .*")
public void testValidateRelationshipRejectsInvalidDuration() throws ResourceException {
    final SchemaField schemaField = mock(SchemaField.class);
    when(schemaField.isReverseRelationship()).thenReturn(false);
    when(schemaField.getName()).thenReturn("roles");
    final CollectionRelationshipProvider relationshipProvider = new CollectionRelationshipProvider(connectionFactory,
            new ResourcePath("managed/user"), schemaField, activityLogger, managedObjectSyncService);
    final JsonValue grant = json(object(
            field(RelationshipUtil.REFERENCE_ID, "managed/role/r1"),
            field(RelationshipUtil.REFERENCE_PROPERTIES,
                    makeTemporalConstraints("2016-01-02T00:00:00.000Z/2016-01-01T00:00:00.000Z").getObject())));
    relationshipProvider.relationshipValidator.validateRelationship(grant, new ResourcePath("managed/user/u1"),
            new RootContext(), false);
}

Pin: without :119, the call falls through to the unstubbed read and the message no longer matches. Whichever write-path call remains after the fix above needs its own case with the same duration.

---

suggestion (non-blocking): Neither validateTemporalConstraintDurations call, in roleCreate or in roleUpdate, is covered by a test.

openidm-zip/src/main/resources/bin/defaults/script/roles/conditionalRoles.js:110, :136, openidm-zip/src/test/resources/bin/defaults/script/conditionalRolesTest.js:48

conditionalRolesTest calls the exported helper only. Deleting either call leaves ScriptRunnerTest passing (Tests run: 1, Failures: 0). As a control, return; at the top of the helper fails at conditionalRolesTest.js:56.

// conditionalRolesTest.js, inside validateTemporalConstraintDurations()
[
    function (role) { conditionalRoles.roleCreate(role); },
    function (role) { conditionalRoles.roleUpdate({ "_id": role._id }, role); }
].forEach(function (write) {
    var rejected = false;
    try {
        write({ "_id": "roleWithReversedConstraint", "temporalConstraints": [ { "duration": reversedDuration } ] });
    } catch (e) {
        if (e.code !== 400) {
            throw e;
        }
        rejected = true;
    }
    if (!rejected) {
        throw { "message": "A role with a reversed temporal constraint was not rejected on write" };
    }
});

Pin: the role is not conditional, so no openidm call is reached. Deleting :110 or :136 makes the case fail.

---

suggestion (non-blocking): The guard for a stored invalid duration in isNowWithinConstraint is not tested with a null element.

openidm-zip/src/main/resources/bin/defaults/script/roles/effectiveRoles.js:104

The new cases cover {}, reversed durations and unparseable ones, but no temporalConstraints: [null]. Replacing the ternary with duration = constraint.duration leaves ScriptRunnerTest passing. A null element stored before this PR would then throw in onRetrieve again.

[
    {
        "_id" : "role9",
        "temporalConstraints" : [ null ]
    },
    false
],

Pin: with this row added to the effectiveRolesTest table, the mutant throws a TypeError on null.duration.

---

suggestion (non-blocking): No test checks the warning that is logged when an invalid duration is skipped, because the new logger in testRunner.js discards every call.

openidm-zip/src/test/resources/testRunner.js:23-26, openidm-zip/src/main/resources/bin/defaults/script/roles/effectiveRoles.js:106

Deleting the logger.warn line in isNowWithinConstraint leaves ScriptRunnerTest passing. That warning is how an operator finds the stored invalid grants after the upgrade.

// effectiveRolesTest.js, inside testProcessTemporalConstraintsForRole()
var warned = [], warn = logger.warn;
logger.warn = function () { warned.push(arguments); };
try {
    effectiveRoles.processTemporalConstraints({ "_id": "role5", "temporalConstraints": [ { "duration": reversedDuration } ] });
} finally {
    logger.warn = warn;
}
if (warned.length !== 1) {
    throw { "message": "Expected one warning for an invalid duration, got " + warned.length };
}

---

question (non-blocking): Should roleUpdate reject an edit that leaves a stored invalid role constraint untouched?

openidm-zip/src/main/resources/bin/defaults/script/roles/conditionalRoles.js:110

roleUpdate validates the full object. A PATCH builds that object from the stored role, so PATCH managed/role/<id> that only replaces /description gets a 400 if the role was stored with a reversed duration. On the base this edit succeeded, because postUpdate skips an unchanged constraint by comparing JSON.stringify output. The same request can also fix the constraint, so the role is not stuck. This is minor if the rejection is intended, and major if stored role constraints were meant to stay editable. Either way, an upgrade note would help: how to find stored invalid durations, and which writes they now block.

if (JSON.stringify(oldRole.temporalConstraints) !== JSON.stringify(newRole.temporalConstraints)) {
    validateTemporalConstraintDurations(newRole);
}

---

suggestion (non-blocking): The guard in createJobsForConstraint has no test, because no test loads postOperation-roles.js.

openidm-zip/src/main/resources/bin/defaults/script/roles/postOperation-roles.js:250-254

testRunner.js loads no postOperation test, and nothing else evaluates the script. Reverting the guard therefore leaves every suite passing. A reversed duration would then fail a postCreate/postUpdate after the resource is stored, and nothing would report it.

Pin: add a ScriptRunnerTest module that binds resourceName/object for a role whose constraint has a reversed duration, stubs openidm.create, loads postOperation-roles.js, and asserts that loading neither throws nor creates a schedule. Without the guard, getStartOfInterval throws "The end instant must be greater the start".

---

suggestion (non-blocking): The admin UI checks that disable Save and Add are not covered by any test. Only the pure isValidInterval has a QUnit case.

openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/util/TemporalConstraintsUtils.js:135-141, openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/TemporalConstraintsFormView.js:160-172, openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/EditRoleView.js:94, openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/MembersDialog.js:97

EditRoleViewTest, MembersDialogTest and TemporalConstraintsFormViewTest contain no QUnit.test. No case calls isTemporalConstraintsFormValid, validate or the validationCallbacks. A mutant such as isTemporalConstraintsFormValid returning true cannot make the admin QUnit run fail.

// TemporalConstraintsUtilsTest.js, with "jquery" added to the define list as $
QUnit.test("isTemporalConstraintsFormValid", (assert) => {
    const form = (end) => $("<div><div class='temporalConstraint'>"
        + "<input class='temporalConstraintStartDate' value='04/25/2016 7:00 AM'>"
        + "<input class='temporalConstraintEndDate' value='" + end + "'></div></div>");
    assert.ok(TemporalConstraintsUtils.isTemporalConstraintsFormValid(form("04/30/2016 7:00 AM")), "valid end date");
    assert.notOk(TemporalConstraintsUtils.isTemporalConstraintsFormValid(form("")), "empty end date");
});

---

suggestion (non-blocking): testInvalidTemporalConstraints checks the exception type, not which guard threw it.

openidm-core/src/test/java/org/forgerock/openidm/managed/RelationshipValidatorTest.java:285

There is no expectedExceptionsMessageRegExp, so a change to any of the three messages, or to the value formatted into {0}, still passes. Deleting a branch is still caught, because each row reaches only one guard.

@Test(dataProvider = "invalidTemporalConstraints")
public void testInvalidTemporalConstraints(JsonValue refProperties, String expectedMessage) {
    try {
        RelationshipValidator.validateTemporalConstraints(refProperties);
        fail("Expected BadRequestException");
    } catch (BadRequestException e) {
        assertTrue(e.getMessage().startsWith(expectedMessage), e.getMessage());
    }
}

Pin: add a second data-provider column with the expected start of the message: "Temporal constraint duration", "Temporal constraints must be an array.", or "Only 1 temporal constraint".

[vharseko]

Both blocking issues are confirmed and fixed in 8652d28; the branch is also rebased onto the current master.

patchInstance validates the stored grant / unchanged grants re-validated after the commit. Fixed together, but not by gating on ManagedObjectContext: the managed-object path validates in validateRelationship only for fields with "validate": true (SchemaField:225, ManagedObjectSet.validateRelationshipFields). That holds for every relationship field in the default managed.json, but in a configuration without it a managed-object write would store an invalid duration unchecked, which is #250 again. So convertToRepoObject no longer validates at all, and the check now runs:

• in createInstance, for every created grant, before the managed-object branch;
• in updateIfChanged, only if the update changes the grant's temporalConstraints (RelationshipValidator.validateChangedTemporalConstraints).

A PATCH that repairs the duration succeeds, the unchanged grants persisted by persistRelationships are not checked, and a changed grant is checked whatever the validate flag. Without validate: true, a managed-object write that adds an invalid grant is still rejected after the object is committed; this is noted in the PR description.

Tests for the Java call sites. CollectionRelationshipProviderTest now drives the managed-object path: create with an invalid duration (400), update keeping a stored invalid constraint (succeeds), update to an invalid one (400), PATCH repairing it (succeeds, the repaired value is written), PATCH breaking it (400). RelationshipValidatorTest adds your validateRelationship case and validateChangedTemporalConstraints. Each case fails with the mutant that removes its check, or with the check put back into convertToRepoObject.

roleCreate / roleUpdate calls. Covered in conditionalRolesTest along the lines of your snippet, plus an update that keeps a stored invalid constraint.

Question: should roleUpdate reject an edit that leaves a stored invalid role constraint untouched? No, that was not intended. roleUpdate now validates only if temporalConstraints changed, using your comparison, which matches the grant side. The PR description has an upgrade note: how to find stored invalid durations (the effectiveRoles warning, or the queries) and which writes they now block (only writes that change them).

[null] element. Added to effectiveRolesTest, and to temporalConstraintsTest, since areConstraintsExpired has the same guard.

Warning for a skipped duration. effectiveRolesTest captures logger.warn and checks that one warning names the duration.

createJobsForConstraint guard. New postOperationRolesTest evaluates postOperation-roles.js with the postCreate globals (stubbed openidm.create) for a role and for a user grant. An invalid duration creates no schedule and does not throw; without the guard it fails with "The end instant must be greater the start". A valid pending duration creates two schedules as a control.

Admin UI checks. isTemporalConstraintsFormValid has a QUnit case (no constraints, valid, empty end date, one reversed among two). Making it return true fails the run.

Message assertions. testInvalidTemporalConstraints now asserts the start of each message.

[maximthomas]

praise: Both round-1 blockers are fixed where they arose, and each call site now has a test.

• RelationshipValidator.validateChangedTemporalConstraints validates only the new value, and only when the constraints differ. updateIfChanged (RelationshipProvider.java:651) uses it, so a PATCH that repairs a stored reversed duration succeeds, and unchanged grants persisted after a managed-object update are left alone.
• conditionalRoles.roleUpdate compares old and new temporalConstraints. conditionalRolesTest pins both directions: a changed invalid constraint is rejected, an unchanged stored one is kept.

---

issue (blocking): A managed-object PATCH/PUT that re-sends a stored invalid grant without _refProperties._id commits the object, deletes the grant, and answers 400.

openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipProvider.java:394, openidm-core/src/main/java/org/forgerock/openidm/managed/CollectionRelationshipProvider.java:174-181, :207, :607-611, openidm-core/src/main/java/org/forgerock/openidm/managed/ManagedObjectSet.java:589-596

RelationshipEqualityHash ignores _id/_rev, so validateRelationshipField treats a grant re-sent without its _id as an existing grant and does not validate it. ManagedObjectSet.update then commits the object (:589) and runs persistRelationships (:596). There the item has no _id, so it is put on the create list: clearNotIn deletes the stored grant, and createInstance rejects the same stored duration (:394). Example: a user holds a pre-upgrade grant with a reversed duration, and the client sends PATCH managed/user/<id> with replace /roles (or PUT ?_fields=*,roles) carrying that grant unchanged except for the missing _id. The user document is rewritten, the grant is gone, and the client gets 400. On the base the request failed with 500 before anything was written. This contradicts the upgrade note on two points: "Writes that leave such a constraint unchanged keep working", and post-commit rejection "only when a relationship field does not have "validate": true" (roles has it). In-repo callers never send grants this way: the admin UI uses the relationship endpoints, and conditional grants carry only _grantType. External REST clients and mappings do. I traced this through the code and did not run it, since that needs a server with a seeded grant.

// CollectionRelationshipProvider.validateRelationshipField
for (JsonValue newItem : newValue) {
    if (!oldReferences.contains(new RelationshipEqualityHash(newItem))) {
        logger.debug("validating new relationship {} for {}: ", newItem, propertyPtr);
        relationshipValidator.validateRelationship(newItem, referrerId, context, performDuplicateAssignmentCheck);
    } else {
        // Equal to a stored relationship but without its _id: persistRelationships deletes the stored one and
        // creates this one after the managed object is committed, where createInstance checks it again
        final JsonValue id = newItem.get(FIELD_ID);
        if (id == null || id.isNull()) {
            RelationshipValidator.validateTemporalConstraints(newItem.get(FIELD_PROPERTIES));
        }
    }
}

Or, to make the upgrade note hold as written: hand the stored grants to setRelationshipValueForResource and give an id-less item that is RelationshipEqualityHash-equal to a stored grant that grant's _id. The item then goes through updateInstance, and validateChangedTemporalConstraints lets the unchanged constraint through.

Pin: in CollectionRelationshipProviderTest, store grant g1 with a reversed duration, then persist [g1 without _refProperties._id] on the managed-object path. The test fails if g1 is deleted before a BadRequestException.

---

question (non-blocking): Should the managed-object road also accept an unchanged stored constraint when another _refProperties field of the same grant changes?

openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipValidator.java:121, openidm-core/src/main/java/org/forgerock/openidm/managed/CollectionRelationshipProvider.java:607-611

Every item whose hash differs from the stored items goes to validateRelationship, and validateRelationship checks the whole _refProperties without comparing it to the stored value. A PUT/PATCH managed/user/<id> that keeps a grant's stored reversed duration but changes _grantType or a custom refProperty therefore gets a 400 that names a duration the request did not change. The check runs before the commit, so nothing is written. The same edit through PATCH managed/user/<id>/roles/<grantId> succeeds. A probe on validateRelationshipField reproduces the 400, and deleting :121 removes it. testUpdateKeepsUnchangedInvalidTemporalConstraint enters at updateInstance, after this gate, so it does not cover it. If the upgrade note is meant to cover this road, this is a bug: match the item to the stored one by _refProperties._id and use validateChangedTemporalConstraints. Otherwise, narrow the note to the relationship endpoints.

---

issue (non-blocking): createJobsForConstraint reads constraint.duration without checking for a null constraint.

openidm-zip/src/main/resources/bin/defaults/script/roles/postOperation-roles.js:250, :346-347

hasConstraints accepts [null], so a null entry throws a TypeError at the new guard. effectiveRoles.js and temporalConstraints.js both check for null before reading the duration. Only a [null] stored before the upgrade can reach this, because the Java check now rejects new ones. The base threw on the same line. Not run: I did not settle whether the user's postUpdate receives the roles at all, since roles is returnByDefault: false.

if (constraint === null || constraint === undefined || !dateUtil.isValidInterval(constraint.duration)) {

Pin: a postOperationRolesTest case with temporalConstraints: [null] on a changed grant.

---

question (non-blocking): Is it intended that the admin UI refuses to save any edit of a role whose stored constraint is reversed?

openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/EditRoleView.js:105, :113-114, :119

The form loads the stored start and end dates, and areTemporalConstraintsValid then fails, so Save is disabled and save() returns early, even for a description edit. Over REST, roleUpdate accepts the unchanged constraint. This is a UI-only block if convertToIntervalString rebuilds the stored string exactly; otherwise the server would reject the save anyway, as the code comment says. Not run: a convertFromIntervalString → convertToIntervalString round trip on a duration that the pre-fix UI produced would settle which case applies. If it is intended, the upgrade note could say that such a role must be repaired before it can be edited in the admin UI.

[vharseko]

Round 2 is addressed in 42daf14.

Id-less re-sent grant is deleted and answered with 400 after the commit. Confirmed, and fixed with your first variant: validateRelationshipField now checks the temporal constraints of an item that is equal to a stored grant but carries no _refProperties._id, so the write gets a 400 before the managed object is written and nothing is deleted. I did not take the second variant (giving the item the stored grant's _id): today an id-less grant is deleted and created again with a new _id on every relationship field, and changing that is beyond #250. The upgrade note now says that such a grant is checked like a new one, and to send the _refProperties._id that a read returns. testValidateFieldRejectsResentInvalidGrantWithoutId follows your pin: the stored g1 with a reversed duration, [g1 without _id] on input, a BadRequestException and no call on the connection; testValidateFieldKeepsResentInvalidGrant is the control with the _id.

Question: unchanged stored constraint while another _refProperties field changes on the managed-object road. Treated as a bug, since the upgrade note promises it. validateRelationship takes the _refProperties of the stored grant that the item updates, matched by _refProperties._id, and checks the temporal constraints through validateChangedTemporalConstraints; for a new or id-less item that value is null, which is the full check as before. The singleton provider and the direct create pass null, so they are unchanged. Tests: the same _id with a changed _grantType and the stored reversed duration passes (and still reads the role); the same _id changed from a valid to a reversed duration gets 400.

createJobsForConstraint with a null constraint. Fixed with the same guard as effectiveRoles.js. postOperationRolesTest adds [null] on a created role, a created grant and an updated grant (whose old value had a pending duration); without the guard they fail with the TypeError.

Question: admin UI refuses to save a role whose stored constraint is reversed. Intended. For the values from #250 the server would reject the save as well: the form keeps whole minutes (MM/DD/YYYY h:mm A), while the end that the old admin UI stored was the current time with seconds and milliseconds, so convertToIntervalString rebuilds a different string and roleUpdate checks it. The UI differs from the server only for a reversed interval stored over REST with whole minutes. The upgrade note now says to correct the dates in the form, or turn the constraint off, to save such a role.

Each new Java and JS case fails with the mutant that removes its check (or that validates the unchanged constraint again); the full openidm-util, openidm-core and openidm-zip test run passes.

[maximthomas]

es the post-commit failures that were left after round 2.

• CollectionRelationshipProvider.validateRelationshipField now checks a grant that is re-sent without its _id before anything is written (CollectionRelationshipProvider.java:624-628). Before this change, the stored grant was deleted first and the 400 came afterwards. testValidateFieldRejectsResentInvalidGrantWithoutId fails without the fix.
• The stored _refProperties are looked up by _id and passed to validateChangedTemporalConstraints (CollectionRelationshipProvider.java:621-623, RelationshipValidator.java:142). Changing _grantType on a grant whose stored invalid constraint is unchanged no longer returns 400 (testValidateFieldKeepsUnchangedInvalidTemporalConstraintOfChangedGrant).
• postOperation-roles.js:248-249 now accepts a null constraint. postOperationRolesTest.js has [ null ] cases for both create and update.

---

issue (non-blocking): A grant that carries one stored grant's _id and another stored grant's content is not checked before the commit.

openidm-core/src/main/java/org/forgerock/openidm/managed/CollectionRelationshipProvider.java:618, :624

RelationshipEqualityHash covers _ref and every _refProperties field except _id/_rev. Because of that, an item {_id: g1, content of stored g2} matches g2's hash and skips validation. The new else if only covers items without an _id. Example: a user holds g1 (role r1, valid) and g2 (role r1, reversed). They send PATCH replace /roles (or PUT ?_fields=*,roles) with [{_ref r1, _refProperties {_id g1, temporalConstraints = g2's}}]. The user is committed, clearNotIn deletes g2, and then updateIfChanged on g1 returns 400 (RelationshipProvider.java:651). This contradicts "matched to the stored grant by _refProperties._id". A probe with stored [g1 valid, g2 reversed] and new [g1-id with g2's content] made no call at all. No in-repo writer builds such a body.

            } else if (!hasId) {
                // Equal to a stored relationship but without its _id: persisting it deletes the stored relationship
                // and creates this one, which must not fail on its temporal constraints after the commit
                RelationshipValidator.validateTemporalConstraints(newItem.get(FIELD_PROPERTIES));
            } else if (id.isString()) {
                // Equal to a stored relationship, but persisting it updates the one with this _id
                RelationshipValidator.validateChangedTemporalConstraints(oldRefPropertiesById.get(id.asString()),
                        newItem.get(FIELD_PROPERTIES));
            }

---

question (non-blocking): Is "keep working … through the managed object" in the upgrade note meant to cover a plain PUT without _fields=*,roles?

openidm-core/src/main/java/org/forgerock/openidm/managed/CollectionRelationshipProvider.java:622, openidm-core/src/main/java/org/forgerock/openidm/managed/ManagedObjectSet.java:1008, :919

The by-_id lookup reads the stored grants from oldValue. On PUT, handleUpdate fills oldValue only with the relationship fields named in _fields plus the returnByDefault ones. User roles and role members are returnByDefault: false (openidm-zip/src/main/resources/conf/managed.json:234-239, :841-847). So when a client reads a user with ?_fields=*,roles and sends the same body back with a plain PUT, every grant is treated as new. An unchanged stored invalid grant then gets a 400 before anything is written. PATCH and PUT ?_fields=*,roles are not affected. At BASE the same PUT returned 500, so this is not a regression.
If yes: before validateRelationshipFields, fetch the stored state of each relationship field that newValue carries and oldValue lacks, the way updateRelationshipFields already does for fields an onUpdate script adds. That would make this a required fix in this PR. If no: the upgrade note should name PUT ?_fields=*,roles or PATCH as the road that keeps such grants.

---

suggestion (non-blocking): No test pins that the stored _refProperties are looked up by the item's own _id.

openidm-core/src/test/java/org/forgerock/openidm/managed/CollectionRelationshipProviderTest.java:222, openidm-core/src/main/java/org/forgerock/openidm/managed/CollectionRelationshipProvider.java:622

All four new tests store a single grant, so oldRefPropertiesById.get(id) and "the first stored value" return the same entry. The mutant values().iterator().next() survives openidm-core 109/109.

    @Test(expectedExceptions = BadRequestException.class,
            expectedExceptionsMessageRegExp = "Temporal constraint duration " + REVERSED_DURATION + " .*")
    public void testValidateFieldComparesWithTheStoredGrantOfTheSameId() throws Exception {
        final JsonValue storedG2 = grant("g2", VALID_DURATION);
        storedG2.put(new JsonPointer("/_ref"), "managed/role/r2");
        final JsonValue changedG2 = grant("g2", REVERSED_DURATION);
        changedG2.put(new JsonPointer("/_ref"), "managed/role/r2");

        newRolesProvider(connectionWithReadableRole()).validateRelationshipField(managedObjectContext(),
                json(array(grant("g1", REVERSED_DURATION).getObject(), storedG2.getObject())),
                json(array(grant("g1", REVERSED_DURATION).getObject(), changedG2.getObject())),
                REFERRING_OBJECT_ID, false);
    }

Pin: this test is red on the first-value mutant, because the HashMap iterates "g1" before "g2".

---

suggestion (non-blocking): No test pins that the new else if (!hasId) runs only the temporal check.

openidm-core/src/test/java/org/forgerock/openidm/managed/CollectionRelationshipProviderTest.java:244, openidm-core/src/main/java/org/forgerock/openidm/managed/CollectionRelationshipProvider.java:624-628

The only id-less case uses an invalid grant. Full validateRelationship throws the same message at RelationshipValidator.java:142, before the role read at :144. So replacing the branch body with relationshipValidator.validateRelationship(newItem, referrerId, context, performDuplicateAssignmentCheck) survives 9/9. That regression would add a read for every re-sent grant. On a reverse array field with the duplicate check on, it would also reject the grant as a duplicate.

    @Test
    public void testValidateFieldDoesNotReadResentValidGrantWithoutId() throws Exception {
        final Connection connection = mock(Connection.class);

        newRolesProvider(connection).validateRelationshipField(managedObjectContext(),
                json(array(grant("g1", VALID_DURATION).getObject())),
                json(array(grant(null, VALID_DURATION).getObject())), REFERRING_OBJECT_ID, false);

        verifyZeroInteractions(connection);
    }

---

suggestion (non-blocking): No test sends an explicit "_id": null, so the id.isNotNull() half of hasId is unpinned.

openidm-core/src/test/java/org/forgerock/openidm/managed/CollectionRelationshipProviderTest.java:250, openidm-core/src/main/java/org/forgerock/openidm/managed/CollectionRelationshipProvider.java:616

grant(null, …) leaves the _id key out, so hasId = id != null alone survives 9/9. The difference is reachable: JsonValue.get(JsonPointer) returns JsonValue(null) for a key that is present with a null value. With _refProperties._id: null, the mutant would skip the check before the commit, while persistRelationships (:173) still deletes the stored grant and creates the new one.

    @Test
    public void testValidateFieldRejectsResentInvalidGrantWithNullId() throws Exception {
        final Connection connection = mock(Connection.class);
        final JsonValue resent = grant(null, REVERSED_DURATION);
        resent.put(new JsonPointer("/_refProperties/_id"), null);
        try {
            newRolesProvider(connection).validateRelationshipField(managedObjectContext(),
                    json(array(grant("g1", REVERSED_DURATION).getObject())),
                    json(array(resent.getObject())), REFERRING_OBJECT_ID, false);
            fail("Expected BadRequestException");
        } catch (BadRequestException e) {
            assertTrue(e.getMessage().startsWith("Temporal constraint duration " + REVERSED_DURATION),
                    e.getMessage());
        }
        verifyZeroInteractions(connection);
    }

[vharseko]

Round 3 is addressed in beff1e6; the branch is also rebased onto the current master.

A grant with one stored grant's _id and another's content. Confirmed, and fixed with your variant: validateRelationshipField now compares an item that is equal to a stored grant and carries an _id with the stored grant of that _id, through validateChangedTemporalConstraints. If the _id is not stored, the comparison is with null, which is the full check. testValidateFieldRejectsGrantWithTheContentOfAnotherStoredGrant follows your example: stored [g1 valid, g2 reversed], [g1-id with g2's content] on input, a BadRequestException and no call on the connection.

Question: plain PUT without _fields=*,roles. No, the note was not meant to cover it, and it now names the roads: the relationship endpoint, PATCH of the managed object, or PUT with _fields=*,roles (_fields=*,members for a role). It also says that a plain PUT checks every grant it carries like a new one, and rejects a stored invalid constraint with 400 before anything is written. I did not fetch the missing relationship fields in update: putting them into the old value would also change the oldObject that postUpdate and sync see, e.g. which grants postOperation-roles takes as new when it creates the constraint schedules, and that changes every plain PUT that carries relationships. As you note, at BASE the same request returned 500.

Lookup by the item's own _id. Added, but not depending on the HashMap order: testValidateFieldComparesWithTheStoredGrantOfTheSameId stores g1 and g2 with two different reversed durations, and changes _grantType of both while keeping their constraints. It passes only if each grant is compared with its own stored value: any single stored value (the first-value mutant included) differs from one of the two durations and gets a 400.

else if (!hasId) runs only the temporal check. Added testValidateFieldDoesNotReadResentValidGrantWithoutId as suggested.

Explicit "_id": null. Added testValidateFieldRejectsResentInvalidGrantWithNullId as suggested.

Each new test fails with the mutant it pins, and only that one: the new else if turned off, values().iterator().next() for the lookup, the full validateRelationship in the !hasId branch, and hasId = id != null. The full openidm-util and openidm-core test run passes.

[maximthomas]

praise: A re-sent grant that equals a stored grant is now checked against the stored grant of its own _id, so round 3's foreign-_id case is rejected before anything is written.

• CollectionRelationshipProvider.validateRelationshipField (:628-633) looks up oldRefPropertiesById by the item's own _id. An _id that is not stored compares with null, which runs the full check (RelationshipValidator.validateChangedTemporalConstraints, :199-204).
• Each new test kills a measured mutant. Deleting the new else if turns testValidateFieldRejectsGrantWithTheContentOfAnotherStoredGrant red. testValidateFieldComparesWithTheStoredGrantOfTheSameId fails when the lookup returns the first stored grant.
• The upgrade note now names the roads that keep a stored constraint. It also says that a plain PUT checks every grant like a new one and answers 400 before anything is written.