[#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. Called from validateRelationship (before the managed object is written and before virtual properties are computed) and from RelationshipProvider.convertToRepoObject (direct writes to a relationship endpoint), replacing the existing "Only 1 temporal constraint" check there.
• conditionalRoles.roleCreate/roleUpdate: 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 instead of failing a request whose resource is already stored.

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.

Testing

• DateUtilTest, RelationshipValidatorTest (valid / invalid _refProperties), all openidm-core tests.
• JS: effectiveRolesTest, temporalConstraintsTest, conditionalRolesTest cover reversed and unparseable durations; without the effectiveRoles.js change the new test fails with the exception from the issue. testRunner.js now provides a no-op logger, as the scripts log through the binding OpenIDM supplies at runtime.
• QUnit: isValidInterval (125 tests, 0 failed); eslint clean on the changed UI files.
• Manually in the admin UI on a build from this branch: 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".