From 2909e7a0114650d76da1a06cee32fa1fb1bb9881 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Tue, 6 Oct 2026 16:47:48 +0300 Subject: [PATCH 1/3] [#250] Reject invalid temporal constraint durations and tolerate stored ones A temporal constraint whose duration is not a valid ISO 8601 interval (e.g. its end is before its start) made the effectiveRoles onRetrieve script throw, so every read and update of the user failed with 500. The admin UI produced such a value itself: an empty end date was sent as the current time. - Reject such a constraint with 400 when a role grant or a role is written - Skip, with a warning, an invalid constraint that is already stored when calculating effective roles, expired constraints and schedules - Admin UI: require both dates with the end after the start before a role or a role member with a temporal constraint can be saved Fixes #250 --- .../openidm/managed/RelationshipProvider.java | 6 +-- .../managed/RelationshipValidator.java | 39 ++++++++++++++ .../managed/RelationshipValidatorTest.java | 43 +++++++++++++++ .../openidm/ui/admin/role/EditRoleView.js | 32 +++++++++++ .../openidm/ui/admin/role/MembersDialog.js | 5 ++ .../admin/role/TemporalConstraintsFormView.js | 30 ++++++++++- .../role/util/TemporalConstraintsUtils.js | 32 +++++++++++ .../partials/role/_temporalConstraint.html | 2 + .../role/util/TemporalConstraintsUtilsTest.js | 25 +++++++++ .../resources/locales/en/translation.json | 1 + .../org/forgerock/openidm/util/DateUtil.java | 21 +++++++- .../forgerock/openidm/util/DateUtilTest.java | 24 ++++++++- .../defaults/script/roles/conditionalRoles.js | 28 ++++++++++ .../defaults/script/roles/effectiveRoles.js | 22 +++++++- .../script/roles/postOperation-roles.js | 10 +++- .../script/roles/temporalConstraints.js | 13 +++-- .../defaults/script/conditionalRolesTest.js | 39 ++++++++++++++ .../bin/defaults/script/effectiveRolesTest.js | 53 ++++++++++++++++++- .../script/temporalConstraintsTest.js | 29 ++++++++++ openidm-zip/src/test/resources/testRunner.js | 6 +++ 20 files changed, 444 insertions(+), 16 deletions(-) diff --git a/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipProvider.java b/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipProvider.java index bace4d70d7..5906965b9b 100644 --- a/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipProvider.java +++ b/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipProvider.java @@ -846,11 +846,7 @@ protected JsonValue convertToRepoObject(final ResourcePath firstResourcePath, fi // Remove "soft" fields that were placed in properties for the ResourceResponse properties.remove(FIELD_CONTENT_ID); properties.remove(FIELD_CONTENT_REVISION); - // Currently only 1 temporal constraint is allowed per grant - if (properties.get("temporalConstraints").isNotNull() - && properties.get("temporalConstraints").expect(List.class).asList().size() > 1) { - throw new BadRequestException("Only 1 temporal constraint is supported per grant."); - } + RelationshipValidator.validateTemporalConstraints(properties); } if (schemaField.isReverseRelationship()) { diff --git a/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipValidator.java b/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipValidator.java index 15bc5d43f6..7c4b38bc29 100644 --- a/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipValidator.java +++ b/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipValidator.java @@ -17,7 +17,9 @@ import static java.text.MessageFormat.format; import static org.forgerock.openidm.util.RelationshipUtil.REFERENCE_ID; +import static org.forgerock.openidm.util.RelationshipUtil.REFERENCE_PROPERTIES; +import org.forgerock.openidm.util.DateUtil; import org.forgerock.openidm.util.ResourceUtil; import org.forgerock.util.annotations.VisibleForTesting; import org.forgerock.json.JsonValue; @@ -42,6 +44,7 @@ abstract class RelationshipValidator { static final String TEMPORAL_CONSTRAINTS = "temporalConstraints"; static final String GRANT_TYPE = "_grantType"; + static final String DURATION = "duration"; private static final Logger logger = LoggerFactory.getLogger(RelationshipValidator.class); @@ -113,6 +116,7 @@ final void validateRelationship(final JsonValue relationshipField, ResourcePath logger.debug(message); throw new BadRequestException(message); } + validateTemporalConstraints(relationshipField.get(REFERENCE_PROPERTIES)); try { validateSuccessfulReadResponse(context, relationshipField, referrerId, relationshipProvider.getConnection() .read(context, newValidateRequest(relationshipField, context)), performDuplicateAssignmentCheck); @@ -124,6 +128,41 @@ final void validateRelationship(final JsonValue relationshipField, ResourcePath } } + /** + * Validates the temporal constraints of a relationship. The effectiveRoles and temporal constraint scripts parse + * the duration of every constraint whenever the managed object is read, so a duration that is not a valid ISO 8601 + * interval (e.g. one whose end is before its start) must be rejected before it is stored. + * + * @param refProperties the _refProperties of the relationship, may be null. + * @throws BadRequestException if there is more than one temporal constraint, or if a constraint does not carry a + * valid ISO 8601 interval as its duration. + */ + static void validateTemporalConstraints(final JsonValue refProperties) throws BadRequestException { + if (refProperties == null || !refProperties.isMap()) { + return; + } + final JsonValue constraints = refProperties.get(TEMPORAL_CONSTRAINTS); + if (constraints.isNull()) { + return; + } + if (!constraints.isList()) { + throw new BadRequestException("Temporal constraints must be an array."); + } + // Currently only 1 temporal constraint is allowed per grant + if (constraints.size() > 1) { + throw new BadRequestException("Only 1 temporal constraint is supported per grant."); + } + for (final JsonValue constraint : constraints) { + final JsonValue duration = constraint.isMap() ? constraint.get(DURATION) : null; + if (duration == null || !duration.isString() + || !DateUtil.getDateUtil().isValidInterval(duration.asString())) { + throw new BadRequestException(format("Temporal constraint duration {0} is not a valid ISO 8601 " + + "interval whose end is not before its start.", + duration == null ? null : duration.getObject())); + } + } + } + /** * Called to determine if the _refProperties of two relationships are equal. * @param existingRefProps the _refProperties of the existing relationship whose _ref matches that diff --git a/openidm-core/src/test/java/org/forgerock/openidm/managed/RelationshipValidatorTest.java b/openidm-core/src/test/java/org/forgerock/openidm/managed/RelationshipValidatorTest.java index a6fdaa65c6..4c7f4b8f03 100644 --- a/openidm-core/src/test/java/org/forgerock/openidm/managed/RelationshipValidatorTest.java +++ b/openidm-core/src/test/java/org/forgerock/openidm/managed/RelationshipValidatorTest.java @@ -249,6 +249,49 @@ public void testDuplicateRefProperties(String grantType1, String temporalConstra "ref props should be flagged as identical"); } + @DataProvider(name = "validTemporalConstraints") + public Object[][] createValidTemporalConstraintData() { + return new Object[][] { + { null }, + { json(object()) }, + { json(object(field(RelationshipValidator.TEMPORAL_CONSTRAINTS, null))) }, + { json(object(field(RelationshipValidator.TEMPORAL_CONSTRAINTS, array()))) }, + { makeTemporalConstraints("2016-01-01T00:00:00.000Z/2016-01-02T00:00:00.000Z") }, + { makeTemporalConstraints("2016-01-01T00:00:00.000Z/P1D") } + }; + } + + @Test(dataProvider = "validTemporalConstraints") + public void testValidTemporalConstraints(JsonValue refProperties) throws BadRequestException { + RelationshipValidator.validateTemporalConstraints(refProperties); + } + + @DataProvider(name = "invalidTemporalConstraints") + public Object[][] createInvalidTemporalConstraintData() { + return new Object[][] { + // end before start + { makeTemporalConstraints("2016-01-02T00:00:00.000Z/2016-01-01T00:00:00.000Z") }, + { makeTemporalConstraints("not an interval") }, + { makeTemporalConstraints(42) }, + { json(object(field(RelationshipValidator.TEMPORAL_CONSTRAINTS, array(object())))) }, + { json(object(field(RelationshipValidator.TEMPORAL_CONSTRAINTS, array("2016-01-01T00:00:00.000Z/P1D")))) }, + { json(object(field(RelationshipValidator.TEMPORAL_CONSTRAINTS, "2016-01-01T00:00:00.000Z/P1D"))) }, + { json(object(field(RelationshipValidator.TEMPORAL_CONSTRAINTS, array( + object(field(RelationshipValidator.DURATION, "2016-01-01T00:00:00.000Z/P1D")), + object(field(RelationshipValidator.DURATION, "2016-02-01T00:00:00.000Z/P1D")))))) } + }; + } + + @Test(dataProvider = "invalidTemporalConstraints", expectedExceptions = BadRequestException.class) + public void testInvalidTemporalConstraints(JsonValue refProperties) throws BadRequestException { + RelationshipValidator.validateTemporalConstraints(refProperties); + } + + private JsonValue makeTemporalConstraints(Object duration) { + return json(object(field(RelationshipValidator.TEMPORAL_CONSTRAINTS, + array(object(field(RelationshipValidator.DURATION, duration)))))); + } + private Map makeRelationship(String referenceId, String grantType, String temporalConstraint) { return json(object( makeField(RelationshipUtil.REFERENCE_ID, referenceId), diff --git a/openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/EditRoleView.js b/openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/EditRoleView.js index ebacb6f83e..61c37d7329 100644 --- a/openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/EditRoleView.js +++ b/openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/EditRoleView.js @@ -12,6 +12,7 @@ * information: "Portions copyright [year] [name of copyright owner]". * * Copyright 2011-2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ define([ @@ -90,10 +91,41 @@ function ($, _, Handlebars, toggleCallback: function () { _this.showPendingChanges(); }, + validationCallback: function () { + _this.showPendingChanges(); + }, temporalConstraints: temporalConstraints }); }; + /* + * Returns false when the temporal constraints are enabled and a constraint lacks a date or ends before it starts, + * as the server rejects such a role. + */ + EditRoleView.prototype.areTemporalConstraintsValid = function () { + return !this.$el.find(".enableTemporalConstraintsCheckbox").prop("checked") + || TemporalConstraintsUtils.isTemporalConstraintsFormValid(this.$el.find('.temporalConstraintsForm')); + }; + + EditRoleView.prototype.showPendingChanges = function () { + GenericEditResourceView.showPendingChanges.call(this); + + if (!this.areTemporalConstraintsValid()) { + this.$el.find("#saveBtn").attr("disabled", true); + } + }; + + EditRoleView.prototype.save = function (e, callback) { + if (!this.areTemporalConstraintsValid()) { + if (e) { + e.preventDefault(); + } + return; + } + + return GenericEditResourceView.save.call(this, e, callback); + }; + EditRoleView.prototype.addConditionForm = function () { var resourceDetailsForm = this.$el.find("#resource-details form"), conditionContent = Handlebars.compile("{{> role/_conditionForm}}"); diff --git a/openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/MembersDialog.js b/openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/MembersDialog.js index 6de55b3df7..a858cf016a 100644 --- a/openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/MembersDialog.js +++ b/openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/MembersDialog.js @@ -12,6 +12,7 @@ * information: "Portions copyright [year] [name of copyright owner]". * * Copyright 2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ define([ @@ -92,6 +93,10 @@ function ($, _, Handlebars, temporalConstraintsView.render({ element: "#" + formContainerId, temporalConstraints: temporalConstraints, + // the server rejects a grant whose temporal constraint lacks a date or ends before it starts + validationCallback: function (isValid) { + $("#resourceCollectionSearchDialogSaveBtn").prop("disabled", !isValid); + }, dialogView: true }); } diff --git a/openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/TemporalConstraintsFormView.js b/openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/TemporalConstraintsFormView.js index 6c22a974c3..db54e53a86 100644 --- a/openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/TemporalConstraintsFormView.js +++ b/openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/TemporalConstraintsFormView.js @@ -30,7 +30,9 @@ define([ template: "templates/admin/role/TemporalConstraintsFormView.html", events: { "change .enableTemporalConstraintsCheckbox": "toggleForm", - "change .temporalConstraintTimezone": "adjustDateToTimezone" + "change .temporalConstraintTimezone": "adjustDateToTimezone", + "change :input.datetimepicker": "validate", + "blur :input.datetimepicker": "validate" }, partials: [ "partials/role/_temporalConstraint.html" @@ -43,6 +45,7 @@ define([ * temporalConstraints: an array of temporalConstraint objects produced by looping over temporal constraint * duration strings and passing each one of them TemporalConstraintsUtils.convertFromIntervalString * toggleCallback: a function to call when the enable temporal constraints toggle switch is changed + * validationCallback: a function to call with the validity of the form whenever it is validated * dialogView: a boolean value telling the view whether the display is in a dialog or notAssigned * * example of how to call this view: @@ -58,6 +61,7 @@ define([ render: function(args, callback) { this.element = args.element; this.toggleCallback = args.toggleCallback; + this.validationCallback = args.validationCallback; this.data.temporalConstraints = args.temporalConstraints; this.data.hasTemporalConstraints = args.temporalConstraints.length > 0; this.data.timezone = TemporalConstraintsUtils.getDefaultTimezone(); @@ -118,8 +122,11 @@ define([ } endInput.data("DateTimePicker").minDate(e.date); + this.validate(); }, this)); + this.$el.find('.temporalConstraintEndDate').on("dp.change", _.bind(this.validate, this)); + this.$el.find(".temporalConstraintTimezone").selectize(); if (isOnRender && this.data.temporalConstraints && this.data.temporalConstraints.length) { @@ -138,10 +145,31 @@ define([ this.$el.find(".temporalConstraintsFields").hide(); } + this.validate(); + if (this.toggleCallback) { this.toggleCallback(); } }, + /* + * Shows an error and reports the form as invalid when the temporal constraints are enabled and a constraint + * lacks a date or its end date is not after its start date. + * + * @returns {boolean} - true if the temporal constraints are disabled or valid + */ + validate : function () { + var enabled = this.$el.find(".enableTemporalConstraintsCheckbox").prop("checked"), + isValid = !enabled || TemporalConstraintsUtils.isTemporalConstraintsFormValid(this.$el.find(".temporalConstraintsForm")); + + this.$el.find(".temporalConstraintEndDate").closest(".form-group").toggleClass("has-error", !isValid); + this.$el.find(".temporalConstraintError").toggle(!isValid); + + if (this.validationCallback) { + this.validationCallback(isValid); + } + + return isValid; + }, adjustDateToTimezone : function (e) { var newTimezone = $(e.target).val(), constraint = $(e.target).closest(".temporalConstraint"), diff --git a/openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/util/TemporalConstraintsUtils.js b/openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/util/TemporalConstraintsUtils.js index 14ba89ff38..5aaa5dffa1 100644 --- a/openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/util/TemporalConstraintsUtils.js +++ b/openidm-ui/openidm-ui-admin/src/main/js/org/forgerock/openidm/ui/admin/role/util/TemporalConstraintsUtils.js @@ -12,6 +12,7 @@ * information: "Portions copyright [year] [name of copyright owner]". * * Copyright 2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ define([ @@ -108,6 +109,37 @@ define([ return intervalString; }; + /* + * This function checks the human readable start and end dates of a temporal constraint. The server rejects an + * interval whose end is before its start, and convertToIntervalString would replace an empty date with the + * current time, so both dates are required and the end date must be after the start date. + * + * @param {string} intervalStart - human readable startDate + * @param {string} intervalEnd - human readable endDate + * @returns {boolean} - true if both dates are valid and the end date is after the start date + */ + obj.isValidInterval = function (intervalStart, intervalEnd) { + var start = moment(intervalStart, format, true), + end = moment(intervalEnd, format, true); + + return start.isValid() && end.isValid() && end.isAfter(start); + }; + + /* + * This function takes a jquery object representing a temporal constraints form + * and returns true if each of its temporal constraints has a valid interval + * + * @param {obj} el - jquery object + * @returns {boolean} - true if every temporal constraint in the form is valid + */ + obj.isTemporalConstraintsFormValid = function (el) { + return _.every(el.find(".temporalConstraint"), (constraint) => { + return this.isValidInterval( + $(constraint).find(".temporalConstraintStartDate").val(), + $(constraint).find(".temporalConstraintEndDate").val()); + }); + }; + /* * This function takes a jquery object representing a temporal constraints form * and returns an array of temporal constraint objects diff --git a/openidm-ui/openidm-ui-admin/src/main/resources/partials/role/_temporalConstraint.html b/openidm-ui/openidm-ui-admin/src/main/resources/partials/role/_temporalConstraint.html index 82b0b0ca9c..16e2939d5b 100644 --- a/openidm-ui/openidm-ui-admin/src/main/resources/partials/role/_temporalConstraint.html +++ b/openidm-ui/openidm-ui-admin/src/main/resources/partials/role/_temporalConstraint.html @@ -1,4 +1,5 @@
@@ -32,6 +33,7 @@
+
diff --git a/openidm-ui/openidm-ui-admin/src/test/qunit/org/forgerock/openidm/ui/admin/role/util/TemporalConstraintsUtilsTest.js b/openidm-ui/openidm-ui-admin/src/test/qunit/org/forgerock/openidm/ui/admin/role/util/TemporalConstraintsUtilsTest.js index 3244f36e8e..b6451d6282 100644 --- a/openidm-ui/openidm-ui-admin/src/test/qunit/org/forgerock/openidm/ui/admin/role/util/TemporalConstraintsUtilsTest.js +++ b/openidm-ui/openidm-ui-admin/src/test/qunit/org/forgerock/openidm/ui/admin/role/util/TemporalConstraintsUtilsTest.js @@ -1,3 +1,19 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Portions Copyright 2026 3A Systems, LLC. + */ + define([ "org/forgerock/openidm/ui/admin/role/util/TemporalConstraintsUtils" ], function (TemporalConstraintsUtils) { @@ -34,4 +50,13 @@ define([ assert.equal(intervalString, '2016-04-25T07:00:00.000Z/2016-04-30T07:00:00.000Z', "start and end dates are correctly converted to an invervalString with offset"); }); + + QUnit.test("isValidInterval", (assert) => { + assert.ok(TemporalConstraintsUtils.isValidInterval("04/25/2016 7:00 AM", "04/30/2016 7:00 AM"), "end after start is valid"); + assert.notOk(TemporalConstraintsUtils.isValidInterval("04/30/2016 7:00 AM", "04/25/2016 7:00 AM"), "end before start is invalid"); + assert.notOk(TemporalConstraintsUtils.isValidInterval("04/25/2016 7:00 AM", "04/25/2016 7:00 AM"), "end equal to start is invalid"); + assert.notOk(TemporalConstraintsUtils.isValidInterval("04/25/2016 7:00 AM", ""), "an empty end date is invalid"); + assert.notOk(TemporalConstraintsUtils.isValidInterval("", "04/30/2016 7:00 AM"), "an empty start date is invalid"); + assert.notOk(TemporalConstraintsUtils.isValidInterval("04/25/2016 7:00 AM", "not a date"), "an unparseable end date is invalid"); + }); }); diff --git a/openidm-ui/openidm-ui-common/src/main/resources/locales/en/translation.json b/openidm-ui/openidm-ui-common/src/main/resources/locales/en/translation.json index 94314a4fc8..a70a190bd6 100644 --- a/openidm-ui/openidm-ui-common/src/main/resources/locales/en/translation.json +++ b/openidm-ui/openidm-ui-common/src/main/resources/locales/en/translation.json @@ -508,6 +508,7 @@ "temporalConstraintsDescription" : "Enable role only during a selected temporal constraint.", "startDate" : "Start Date", "endDate" : "End Date", + "temporalConstraintsInvalid" : "Enter both dates, with the end date after the start date.", "grantType" : "Grant Type", "conditional" : "Conditional", "timezone" : "Timezone" diff --git a/openidm-util/src/main/java/org/forgerock/openidm/util/DateUtil.java b/openidm-util/src/main/java/org/forgerock/openidm/util/DateUtil.java index 7f3ff081a5..70828263ba 100644 --- a/openidm-util/src/main/java/org/forgerock/openidm/util/DateUtil.java +++ b/openidm-util/src/main/java/org/forgerock/openidm/util/DateUtil.java @@ -217,7 +217,26 @@ public DateTime parseIfDate(String timestamp) { */ public boolean isNowWithinInterval(String intervalString) throws IllegalArgumentException { Interval interval = Interval.parse(intervalString); - return interval.contains(DateTime.now()); + return interval.contains(DateTime.now()); + } + + /** + * Returns true if the specified string is an ISO 8601 time interval that the interval methods of this class can + * parse. An interval whose end is before its start is not valid. + * + * @param intervalString a {@link String} object representing an ISO 8601 time interval, may be null. + * @return true if the string parses as a time interval, false otherwise. + */ + public boolean isValidInterval(String intervalString) { + if (intervalString == null) { + return false; + } + try { + Interval.parse(intervalString); + return true; + } catch (IllegalArgumentException | ArithmeticException e) { + return false; + } } /** diff --git a/openidm-util/src/test/java/org/forgerock/openidm/util/DateUtilTest.java b/openidm-util/src/test/java/org/forgerock/openidm/util/DateUtilTest.java index de6dd66c6f..fa358a98a3 100644 --- a/openidm-util/src/test/java/org/forgerock/openidm/util/DateUtilTest.java +++ b/openidm-util/src/test/java/org/forgerock/openidm/util/DateUtilTest.java @@ -135,7 +135,29 @@ public void testIsNowWithinInterval() { assertThat(dateUtil.isNowWithinInterval(passInterval)).isEqualTo(true); assertThat(dateUtil.isNowWithinInterval(failInterval)).isEqualTo(false); } - + + @DataProvider + public Object[][] intervalValidityData() { + return new Object[][] { + { "2016-01-01T09:00:00.000Z/2016-01-02T09:00:00.000Z", true }, + { "2016-01-01T09:00:00.000Z/2016-01-01T09:00:00.000Z", true }, + { "2016-01-01T09:00:00.000Z/P1D", true }, + { "P1D/2016-01-01T09:00:00.000Z", true }, + // end before start + { "2016-01-02T09:00:00.000Z/2016-01-01T09:00:00.000Z", false }, + { "2016-01-01T09:01:00....invalid", false }, + { "2016-01-01T09:00:00.000Z", false }, + { "P1D/P2D", false }, + { "", false }, + { null, false } + }; + } + + @Test(dataProvider = "intervalValidityData") + public void testIsValidInterval(String interval, boolean valid) { + assertThat(dateUtil.isValidInterval(interval)).isEqualTo(valid); + } + @Test(dataProvider = "dateDifferenceInDaysData") public void testGetDateDifferenceInDays(String format, String start, String end, Boolean includeDay, int diff) throws ParseException { diff --git a/openidm-zip/src/main/resources/bin/defaults/script/roles/conditionalRoles.js b/openidm-zip/src/main/resources/bin/defaults/script/roles/conditionalRoles.js index 594c8bdb57..e8a4aa7e3f 100644 --- a/openidm-zip/src/main/resources/bin/defaults/script/roles/conditionalRoles.js +++ b/openidm-zip/src/main/resources/bin/defaults/script/roles/conditionalRoles.js @@ -12,6 +12,7 @@ * information: "Portions copyright [year] [name of copyright owner]". * * Copyright 2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ @@ -106,6 +107,7 @@ if (isTemporalConstraintsMultiValue(newRole)) { throw {code : 400, message: "Only 1 temporal constraint is supported per role."} } + validateTemporalConstraintDurations(newRole); /* Only iterate through all of the users if we are dealing with a conditional role, and if the role condition has changed. And if the role's condition has been removed, the new role grantees will be only @@ -131,6 +133,7 @@ if (isTemporalConstraintsMultiValue(newRole)) { throw {code : 400, message: "Only 1 temporal constraint is supported per role."} } + validateTemporalConstraintDurations(newRole); if (relationshipHelper.isRoleConditional(newRole)) { newRole.members = processCreatedConditionalRole(newRole); } @@ -290,6 +293,31 @@ } } + exports.validateTemporalConstraintDurations = validateTemporalConstraintDurations; + /** + * Rejects a role whose temporal constraint does not carry a valid ISO 8601 interval as its duration, e.g. an + * interval whose end is before its start. Such a constraint cannot be evaluated when the effective roles of the + * role members are calculated. + * + * @param role the role to inspect + * @throws BadRequestException if the duration of a temporal constraint is not a valid interval + */ + function validateTemporalConstraintDurations(role) { + var dateUtil = org.forgerock.openidm.util.DateUtil.getDateUtil(), + index, + duration; + if (isNil(role.temporalConstraints)) { + return; + } + for (index in role.temporalConstraints) { + duration = isNil(role.temporalConstraints[index]) ? null : role.temporalConstraints[index].duration; + if (isNil(duration) || !dateUtil.isValidInterval(duration)) { + throw {code : 400, message: "Temporal constraint duration " + duration + + " is not a valid ISO 8601 interval whose end is not before its start."} + } + } + } + /** * Determines if a role's condition has changed * @param oldRole the old role diff --git a/openidm-zip/src/main/resources/bin/defaults/script/roles/effectiveRoles.js b/openidm-zip/src/main/resources/bin/defaults/script/roles/effectiveRoles.js index e6d299a47d..970a6cff47 100644 --- a/openidm-zip/src/main/resources/bin/defaults/script/roles/effectiveRoles.js +++ b/openidm-zip/src/main/resources/bin/defaults/script/roles/effectiveRoles.js @@ -70,7 +70,8 @@ /** * Processes the temporal constraints of a given object. If any temporal constraints are defined, this function will * return true if the current time instant (now) is contained within any of the temporal constraints, false - * otherwise. If no constraints are defined the function will return true. + * otherwise. If no constraints are defined the function will return true. A constraint whose duration is not a + * valid interval is logged and never includes the current time instant. * * @param role the role to process. * @returns false if temporal constraints are defined and don't include the current time instant, true otherwise. @@ -81,7 +82,7 @@ for (var index in object.temporalConstraints) { var constraint = object.temporalConstraints[index]; // If at least one constraint passes, the role is in effect - if (org.forgerock.openidm.util.DateUtil.getDateUtil().isNowWithinInterval(constraint.duration)) { + if (isNowWithinConstraint(constraint)) { return true; } } @@ -91,4 +92,21 @@ return true; }; + /** + * Returns true if the current time instant is contained within the duration of a temporal constraint. An invalid + * duration (e.g. an interval whose end is before its start) must not break reading the object, nor grant the role. + * + * @param constraint the temporal constraint. + * @returns true if the duration is a valid interval which includes the current time instant, false otherwise. + */ + function isNowWithinConstraint(constraint) { + var dateUtil = org.forgerock.openidm.util.DateUtil.getDateUtil(), + duration = (constraint !== undefined && constraint !== null) ? constraint.duration : constraint; + if (!dateUtil.isValidInterval(duration)) { + logger.warn("Ignoring temporal constraint with an invalid duration {}", String(duration)); + return false; + } + return dateUtil.isNowWithinInterval(duration); + } + }()); diff --git a/openidm-zip/src/main/resources/bin/defaults/script/roles/postOperation-roles.js b/openidm-zip/src/main/resources/bin/defaults/script/roles/postOperation-roles.js index 141010f71c..eca98943ed 100644 --- a/openidm-zip/src/main/resources/bin/defaults/script/roles/postOperation-roles.js +++ b/openidm-zip/src/main/resources/bin/defaults/script/roles/postOperation-roles.js @@ -245,8 +245,14 @@ function deleteJobsForRoleConstraint(index) { */ function createJobsForConstraint(constraint, startJobId, endJobId, script) { logger.debug("creating new jobs for: " + constraint); - var dateUtil = org.forgerock.openidm.util.DateUtil.getDateUtil(), - startDate = dateUtil.getStartOfInterval(constraint.duration), + var dateUtil = org.forgerock.openidm.util.DateUtil.getDateUtil(); + // The resource has already been stored, so an invalid duration must not fail the request + if (!dateUtil.isValidInterval(constraint.duration)) { + logger.warn("Not creating schedules for temporal constraint on resource " + resourceName + + " with an invalid duration: " + constraint.duration); + return false; + } + var startDate = dateUtil.getStartOfInterval(constraint.duration), endDate = dateUtil.getEndOfInterval(constraint.duration), startExpression = dateUtil.getSchedulerExpression(startDate.plusSeconds(1)), endExpression = dateUtil.getSchedulerExpression(endDate.plusSeconds(1)); diff --git a/openidm-zip/src/main/resources/bin/defaults/script/roles/temporalConstraints.js b/openidm-zip/src/main/resources/bin/defaults/script/roles/temporalConstraints.js index ca42bdf7c2..60968234cf 100644 --- a/openidm-zip/src/main/resources/bin/defaults/script/roles/temporalConstraints.js +++ b/openidm-zip/src/main/resources/bin/defaults/script/roles/temporalConstraints.js @@ -94,6 +94,8 @@ * Returns true if: ß * 1. none of the temporal constraints for the role/grant are currently in effect AND * 2. no temporal constraint is pending, and at least one temporal constraint is expired. + * A temporal constraint whose duration is not a valid interval is logged and is neither in effect, pending nor + * expired. * @param object the grant or role * @returns {boolean} true or false, depending on the rules defined above. */ @@ -103,13 +105,18 @@ index; for (index in object.temporalConstraints) { - var constraint = object.temporalConstraints[index]; + var constraint = object.temporalConstraints[index], + duration = isNil(constraint) ? constraint : constraint.duration; + if (!dateUtil.isValidInterval(duration)) { + logger.warn("Ignoring temporal constraint with an invalid duration {}", String(duration)); + continue; + } // If one constraint passes, the role is in effect, and not expired. If one constraint is in the future, it is // also not expired. - if (dateUtil.isNowWithinInterval(constraint.duration) || dateUtil.isIntervalInFuture(constraint.duration)) { + if (dateUtil.isNowWithinInterval(duration) || dateUtil.isIntervalInFuture(duration)) { return false; } - constraintExpired = constraintExpired || dateUtil.isIntervalInPast(constraint.duration); + constraintExpired = constraintExpired || dateUtil.isIntervalInPast(duration); } return constraintExpired; }; diff --git a/openidm-zip/src/test/resources/bin/defaults/script/conditionalRolesTest.js b/openidm-zip/src/test/resources/bin/defaults/script/conditionalRolesTest.js index 91dfc522ea..bd3628eab7 100644 --- a/openidm-zip/src/test/resources/bin/defaults/script/conditionalRolesTest.js +++ b/openidm-zip/src/test/resources/bin/defaults/script/conditionalRolesTest.js @@ -12,6 +12,7 @@ * information: "Portions copyright [year] [name of copyright owner]". * * Copyright 2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ /** @@ -22,6 +23,44 @@ exports.test = function() { var _ = require('lib/lodash'); isTemporalConstraintsMultiValue(); + validateTemporalConstraintDurations(); + + function validateTemporalConstraintDurations() { + var dateUtil = org.forgerock.openidm.util.DateUtil.getDateUtil(), + now = dateUtil.currentDateTime(), + currentDuration = dateUtil.formatDateTime(now.minusDays(1)) + + "/" + dateUtil.formatDateTime(now.plusDays(1)), + // end before start, see issue #250 + reversedDuration = dateUtil.formatDateTime(now.plusDays(1)) + + "/" + dateUtil.formatDateTime(now.minusDays(1)); + [ + [ { "_id": "roleWithoutConstraints" }, false ], + [ { "_id": "roleWithValidConstraint", "temporalConstraints": [ { "duration": currentDuration } ] }, false ], + [ { "_id": "roleWithPeriodConstraint", "temporalConstraints": [ { "duration": dateUtil.formatDateTime(now) + "/P1D" } ] }, false ], + [ { "_id": "roleWithReversedConstraint", "temporalConstraints": [ { "duration": reversedDuration } ] }, true ], + [ { "_id": "roleWithInvalidConstraint", "temporalConstraints": [ { "duration": "not an interval" } ] }, true ], + [ { "_id": "roleWithoutDuration", "temporalConstraints": [ { } ] }, true ] + ].map( + function (testcase) { + (function (role, expectedRejection) { + var rejected = false; + try { + conditionalRoles.validateTemporalConstraintDurations(role); + } catch (e) { + if (e.code !== 400) { + throw e; + } + rejected = true; + } + if (rejected !== expectedRejection) { + throw { + "message": "Validating temporal constraint durations of role " + role._id + ", rejected <" + + rejected + ">, expected <" + expectedRejection + ">" + }; + } + }).apply(null, testcase); + }); + } function isTemporalConstraintsMultiValue() { var dateUtil = org.forgerock.openidm.util.DateUtil.getDateUtil(), diff --git a/openidm-zip/src/test/resources/bin/defaults/script/effectiveRolesTest.js b/openidm-zip/src/test/resources/bin/defaults/script/effectiveRolesTest.js index aafe58eaca..38118e72ab 100644 --- a/openidm-zip/src/test/resources/bin/defaults/script/effectiveRolesTest.js +++ b/openidm-zip/src/test/resources/bin/defaults/script/effectiveRolesTest.js @@ -12,6 +12,7 @@ * information: "Portions copyright [year] [name of copyright owner]". * * Copyright 2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ /** @@ -33,7 +34,11 @@ exports.test = function() { failDuration1 = dateUtil.formatDateTime(now.plusDays(1)) + "/" + dateUtil.formatDateTime(now.plusDays(2)), failDuration2 = dateUtil.formatDateTime(now.plusHours(1)) - + "/" + dateUtil.formatDateTime(now.plusHours(2)); + + "/" + dateUtil.formatDateTime(now.plusHours(2)), + // end before start, see issue #250 + reversedDuration = dateUtil.formatDateTime(now.plusDays(1)) + + "/" + dateUtil.formatDateTime(now.minusDays(1)), + invalidDuration = "not an interval"; // test cases for applyConstraint [ @@ -89,6 +94,52 @@ exports.test = function() { ] }, false + ], + [ + { + "_id" : "role5", + "temporalConstraints" : [ + { + "duration" : reversedDuration + } + ] + }, + false + ], + [ + { + "_id" : "role6", + "temporalConstraints" : [ + { + "duration" : invalidDuration + } + ] + }, + false + ], + [ + { + "_id" : "role7", + "temporalConstraints" : [ + { + } + ] + }, + false + ], + [ + { + "_id" : "role8", + "temporalConstraints" : [ + { + "duration" : reversedDuration + }, + { + "duration" : passDuration1 + } + ] + }, + true ] ].map( function (testcase) { diff --git a/openidm-zip/src/test/resources/bin/defaults/script/temporalConstraintsTest.js b/openidm-zip/src/test/resources/bin/defaults/script/temporalConstraintsTest.js index e384951b59..8e49046bab 100644 --- a/openidm-zip/src/test/resources/bin/defaults/script/temporalConstraintsTest.js +++ b/openidm-zip/src/test/resources/bin/defaults/script/temporalConstraintsTest.js @@ -12,6 +12,7 @@ * information: "Portions copyright [year] [name of copyright owner]". * * Copyright 2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ /** @@ -31,6 +32,9 @@ exports.test = function() { pendingDuration = dateUtil.formatDateTime(now.plusDays(1)) + "/" + dateUtil.formatDateTime(now.plusDays(2)), expiredDuration = dateUtil.formatDateTime(now.minusDays(2)) + + "/" + dateUtil.formatDateTime(now.minusDays(1)), + // end before start, see issue #250 + reversedDuration = dateUtil.formatDateTime(now.plusDays(1)) + "/" + dateUtil.formatDateTime(now.minusDays(1)); [ @@ -66,6 +70,31 @@ exports.test = function() { ] }, true + ], + [ + { + "_id" : "role3", + "temporalConstraints" : [ + { + "duration" : reversedDuration + } + ] + }, + false + ], + [ + { + "_id" : "role4", + "temporalConstraints" : [ + { + "duration" : reversedDuration + }, + { + "duration" : expiredDuration + } + ] + }, + true ] ].map( function (testcase) { diff --git a/openidm-zip/src/test/resources/testRunner.js b/openidm-zip/src/test/resources/testRunner.js index 8f6e5c6b2f..4456f19628 100644 --- a/openidm-zip/src/test/resources/testRunner.js +++ b/openidm-zip/src/test/resources/testRunner.js @@ -19,6 +19,12 @@ * Backend script module test runner. For each module to be tested, create a suitable *Test module that * exports a "test" method, and add it to the array of test modules below. */ +// The backend scripts log through the logger binding that OpenIDM supplies at runtime. +var logger = (function () { + function noOp() {} + return { trace: noOp, debug: noOp, info: noOp, warn: noOp, error: noOp }; +}()); + [ "policyFilterTest", "policyUniqueTest", "routerAuthzTest", From 8652d286e7a269e02c229616a25df976d0a24bfa Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Wed, 7 Oct 2026 13:01:21 +0300 Subject: [PATCH 2/3] [#250] Validate temporal constraints only when a write changes them - Move the duration check out of RelationshipProvider.convertToRepoObject, which also ran on the stored value of a PATCH and on every unchanged relationship persisted after a managed object update. Check a created relationship in createInstance and an updated one in updateIfChanged, only if its temporal constraints changed; this no longer depends on the schema field's "validate" flag. - conditionalRoles.roleUpdate: check a role's durations only if its temporal constraints changed. - Test each call site (create, update, PATCH, validateRelationship, roleCreate/roleUpdate), null constraint elements, the warning for a skipped duration, postOperation-roles schedules and isTemporalConstraintsFormValid; assert the rejection messages. --- .../openidm/managed/RelationshipProvider.java | 6 +- .../managed/RelationshipValidator.java | 25 ++++ .../CollectionRelationshipProviderTest.java | 118 ++++++++++++++++++ .../managed/RelationshipValidatorTest.java | 76 +++++++++-- .../role/util/TemporalConstraintsUtilsTest.js | 19 ++- .../defaults/script/roles/conditionalRoles.js | 5 +- .../defaults/script/conditionalRolesTest.js | 37 ++++++ .../bin/defaults/script/effectiveRolesTest.js | 28 +++++ .../defaults/script/postOperationRolesTest.js | 84 +++++++++++++ .../script/temporalConstraintsTest.js | 12 ++ openidm-zip/src/test/resources/testRunner.js | 1 + 11 files changed, 397 insertions(+), 14 deletions(-) create mode 100644 openidm-zip/src/test/resources/bin/defaults/script/postOperationRolesTest.js diff --git a/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipProvider.java b/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipProvider.java index 5906965b9b..0539dd4600 100644 --- a/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipProvider.java +++ b/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipProvider.java @@ -389,6 +389,9 @@ public abstract void validateRelationshipField(Context context, JsonValue oldVal public Promise createInstance(final Context context, final CreateRequest request) { try { + // The relationship validator checks the temporal constraints too, but on the managed object path only + // when the schema field requires validation + RelationshipValidator.validateTemporalConstraints(request.getContent().get(FIELD_PROPERTIES)); final CreateRequest createRequest = Requests.copyOfCreateRequest(request); createRequest.setResourcePath(REPO_RESOURCE_PATH); createRequest.setContent(convertToRepoObject(firstResourcePath(context, request), request.getContent())); @@ -645,6 +648,8 @@ private Promise updateIfChanged(final Conte .then(formatResponse(context, request)); } else { // resource has changed, update the relationship + RelationshipValidator.validateChangedTemporalConstraints( + oldResource.getContent().get(REPO_FIELD_PROPERTIES), newValue.get(REPO_FIELD_PROPERTIES)); UpdateRequest updateRequest = Requests.newUpdateRequest(REPO_RESOURCE_PATH.child(id), newValue).setRevision(rev); return syncReferencedObjectUpdateHandler @@ -846,7 +851,6 @@ protected JsonValue convertToRepoObject(final ResourcePath firstResourcePath, fi // Remove "soft" fields that were placed in properties for the ResourceResponse properties.remove(FIELD_CONTENT_ID); properties.remove(FIELD_CONTENT_REVISION); - RelationshipValidator.validateTemporalConstraints(properties); } if (schemaField.isReverseRelationship()) { diff --git a/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipValidator.java b/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipValidator.java index 7c4b38bc29..1332838d0d 100644 --- a/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipValidator.java +++ b/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipValidator.java @@ -12,6 +12,7 @@  * information: "Portions copyright [year] [name of copyright owner]".  *  * Copyright 2015-2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC.  */ package org.forgerock.openidm.managed; @@ -34,6 +35,7 @@ import org.slf4j.LoggerFactory; import java.util.HashSet; +import java.util.Objects; import java.util.Set; /** @@ -163,6 +165,29 @@ static void validateTemporalConstraints(final JsonValue refProperties) throws Ba } } + /** + * Validates the temporal constraints of an updated relationship if they differ from the stored ones. A stored + * constraint that is not valid must neither prevent the update that repairs it, nor the writes which carry it + * unchanged, e.g. a managed object update that persists all of its relationships. + * + * @param oldRefProperties the stored _refProperties of the relationship, may be null. + * @param newRefProperties the updated _refProperties of the relationship, may be null. + * @throws BadRequestException if the temporal constraints have changed and are not valid. + * @see #validateTemporalConstraints(JsonValue) + */ + static void validateChangedTemporalConstraints(final JsonValue oldRefProperties, final JsonValue newRefProperties) + throws BadRequestException { + if (!Objects.equals(getTemporalConstraints(oldRefProperties), getTemporalConstraints(newRefProperties))) { + validateTemporalConstraints(newRefProperties); + } + } + + private static Object getTemporalConstraints(final JsonValue refProperties) { + return refProperties == null || !refProperties.isMap() + ? null + : refProperties.get(TEMPORAL_CONSTRAINTS).getObject(); + } + /** * Called to determine if the _refProperties of two relationships are equal. * @param existingRefProps the _refProperties of the existing relationship whose _ref matches that diff --git a/openidm-core/src/test/java/org/forgerock/openidm/managed/CollectionRelationshipProviderTest.java b/openidm-core/src/test/java/org/forgerock/openidm/managed/CollectionRelationshipProviderTest.java index 5550c8ed47..80328df6e3 100644 --- a/openidm-core/src/test/java/org/forgerock/openidm/managed/CollectionRelationshipProviderTest.java +++ b/openidm-core/src/test/java/org/forgerock/openidm/managed/CollectionRelationshipProviderTest.java @@ -12,6 +12,7 @@  * information: "Portions copyright [year] [name of copyright owner]".  *  * Copyright 2015-2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC.  */ package org.forgerock.openidm.managed; @@ -20,22 +21,35 @@ import static org.mockito.Mockito.*; import static org.testng.Assert.*; +import org.forgerock.http.routing.UriRouterContext; +import org.forgerock.json.JsonPointer; import org.forgerock.json.JsonValue; +import org.forgerock.json.resource.BadRequestException; import org.forgerock.json.resource.Connection; import org.forgerock.json.resource.ConnectionFactory; +import org.forgerock.json.resource.CreateRequest; +import org.forgerock.json.resource.PatchOperation; import org.forgerock.json.resource.PreconditionFailedException; import org.forgerock.json.resource.ReadRequest; +import org.forgerock.json.resource.Requests; import org.forgerock.json.resource.ResourcePath; +import org.forgerock.json.resource.ResourceResponse; +import org.forgerock.json.resource.UpdateRequest; import org.forgerock.openidm.audit.util.ActivityLogger; import org.forgerock.openidm.util.RelationshipUtil; import org.forgerock.services.context.Context; import org.forgerock.services.context.RootContext; +import org.mockito.ArgumentCaptor; import org.mockito.ArgumentMatcher; import org.testng.annotations.BeforeTest; import org.testng.annotations.Test; +import java.util.Collections; + public class CollectionRelationshipProviderTest { private static final ResourcePath REFERRING_OBJECT_ID = new ResourcePath("managed/user/foo"); + private static final String VALID_DURATION = "2016-01-01T00:00:00.000Z/2016-01-02T00:00:00.000Z"; + private static final String REVERSED_DURATION = "2016-01-02T00:00:00.000Z/2016-01-01T00:00:00.000Z"; private ManagedObjectSetService managedObjectSyncService; private ConnectionFactory connectionFactory; private ActivityLogger activityLogger; @@ -147,6 +161,110 @@ public void testValidateFieldOnReverseRelationshipField() throws Exception { } } + @Test(expectedExceptions = BadRequestException.class, + expectedExceptionsMessageRegExp = "Temporal constraint duration " + REVERSED_DURATION + " .*") + public void testCreateRejectsInvalidTemporalConstraint() throws Exception { + final Connection connection = mock(Connection.class); + when(connection.createAsync(any(Context.class), any(CreateRequest.class))).thenAnswer(invocation -> + newResourceResponse("g1", "1", ((CreateRequest) invocation.getArguments()[1]).getContent()).asPromise()); + + newRolesProvider(connection).createInstance(managedObjectContext(), + Requests.newCreateRequest("", grant(null, REVERSED_DURATION))).getOrThrow(); + } + + @Test + public void testUpdateKeepsUnchangedInvalidTemporalConstraint() throws Exception { + // a managed object update persists every relationship it carries, including a stored invalid one + final Connection connection = connectionWithStoredGrant(REVERSED_DURATION); + final JsonValue grant = grant("g1", REVERSED_DURATION); + grant.put(new JsonPointer("/_refProperties/_grantType"), "conditional"); + + newRolesProvider(connection).updateInstance(managedObjectContext(), "g1", + Requests.newUpdateRequest("", grant)).getOrThrow(); + + verify(connection).updateAsync(any(Context.class), any(UpdateRequest.class)); + } + + @Test(expectedExceptions = BadRequestException.class, + expectedExceptionsMessageRegExp = "Temporal constraint duration " + REVERSED_DURATION + " .*") + public void testUpdateRejectsChangedInvalidTemporalConstraint() throws Exception { + final Connection connection = connectionWithStoredGrant(VALID_DURATION); + + newRolesProvider(connection).updateInstance(managedObjectContext(), "g1", + Requests.newUpdateRequest("", grant("g1", REVERSED_DURATION))).getOrThrow(); + } + + @Test + public void testPatchRepairsInvalidTemporalConstraint() throws Exception { + final Connection connection = connectionWithStoredGrant(REVERSED_DURATION); + + newRolesProvider(connection).patchInstance(managedObjectContext(), "g1", + Requests.newPatchRequest("", PatchOperation.replace( + "/_refProperties/temporalConstraints/0/duration", VALID_DURATION))).getOrThrow(); + + final ArgumentCaptor update = ArgumentCaptor.forClass(UpdateRequest.class); + verify(connection).updateAsync(any(Context.class), update.capture()); + assertEquals(update.getValue().getContent() + .get(new JsonPointer("/properties/temporalConstraints/0/duration")).asString(), VALID_DURATION); + } + + @Test(expectedExceptions = BadRequestException.class, + expectedExceptionsMessageRegExp = "Temporal constraint duration " + REVERSED_DURATION + " .*") + public void testPatchRejectsInvalidTemporalConstraint() throws Exception { + final Connection connection = connectionWithStoredGrant(VALID_DURATION); + + newRolesProvider(connection).patchInstance(managedObjectContext(), "g1", + Requests.newPatchRequest("", PatchOperation.replace( + "/_refProperties/temporalConstraints/0/duration", REVERSED_DURATION))).getOrThrow(); + } + + private CollectionRelationshipProvider newRolesProvider(final Connection connection) throws Exception { + final ConnectionFactory factory = mock(ConnectionFactory.class); + when(factory.getConnection()).thenReturn(connection); + final SchemaField schemaField = mock(SchemaField.class); + when(schemaField.isReverseRelationship()).thenReturn(false); + when(schemaField.getName()).thenReturn("roles"); + return new CollectionRelationshipProvider(factory, ResourcePath.resourcePath("managed/user"), schemaField, + activityLogger, managedObjectSyncService); + } + + /** The context of a relationship request made by the managed object user/u1. */ + private static Context managedObjectContext() { + return new ManagedObjectContext(new UriRouterContext(new RootContext(), "", "", + Collections.singletonMap(RelationshipProvider.PARAM_MANAGED_OBJECT_ID, "u1"))); + } + + /** A connection whose repository holds the grant g1 of role r1 to user u1, and which accepts any update. */ + private static Connection connectionWithStoredGrant(final String duration) throws Exception { + final Connection connection = mock(Connection.class); + when(connection.readAsync(any(Context.class), any(ReadRequest.class))).thenAnswer(invocation -> + newResourceResponse("g1", "1", json(object( + field(RelationshipProvider.REPO_FIELD_FIRST_ID, "managed/user/u1"), + field(RelationshipProvider.REPO_FIELD_FIRST_PROPERTY_NAME, "roles"), + field(RelationshipProvider.REPO_FIELD_SECOND_ID, "managed/role/r1"), + field(RelationshipProvider.REPO_FIELD_SECOND_PROPERTY_NAME, null), + field(RelationshipProvider.REPO_FIELD_PROPERTIES, object( + field(RelationshipValidator.TEMPORAL_CONSTRAINTS, + array(object(field(RelationshipValidator.DURATION, duration)))))))) + ).asPromise()); + when(connection.updateAsync(any(Context.class), any(UpdateRequest.class))).thenAnswer(invocation -> + newResourceResponse("g1", "2", ((UpdateRequest) invocation.getArguments()[1]).getContent()).asPromise()); + return connection; + } + + /** A grant of role r1 with a temporal constraint, as a relationship request carries it. */ + private static JsonValue grant(final String id, final String duration) { + final JsonValue refProperties = json(object(field(RelationshipValidator.TEMPORAL_CONSTRAINTS, + array(object(field(RelationshipValidator.DURATION, duration)))))); + if (id != null) { + refProperties.put("_id", id); + refProperties.put("_rev", "1"); + } + return json(object( + field(RelationshipUtil.REFERENCE_ID, "managed/role/r1"), + field(RelationshipUtil.REFERENCE_PROPERTIES, refProperties.getObject()))); + } + private static class IsRouteMatcher extends ArgumentMatcher { private final String route; diff --git a/openidm-core/src/test/java/org/forgerock/openidm/managed/RelationshipValidatorTest.java b/openidm-core/src/test/java/org/forgerock/openidm/managed/RelationshipValidatorTest.java index 4c7f4b8f03..505e8f9c9b 100644 --- a/openidm-core/src/test/java/org/forgerock/openidm/managed/RelationshipValidatorTest.java +++ b/openidm-core/src/test/java/org/forgerock/openidm/managed/RelationshipValidatorTest.java @@ -12,6 +12,7 @@  * information: "Portions copyright [year] [name of copyright owner]".  *  * Copyright 2015-2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC.  */ package org.forgerock.openidm.managed; @@ -59,6 +60,8 @@ public class RelationshipValidatorTest { public static final JsonValue TEST_RELATIONSHIP = json(object(field(RelationshipUtil.REFERENCE_ID, "managed/widgetPart/part1"))); private static final String RELATIONSHIP_ID = "the_id"; + private static final String VALID_DURATION = "2016-01-01T00:00:00.000Z/2016-01-02T00:00:00.000Z"; + private static final String REVERSED_DURATION = "2016-01-02T00:00:00.000Z/2016-01-01T00:00:00.000Z"; private ManagedObjectSetService managedObjectSyncService; private ConnectionFactory connectionFactory; private ActivityLogger activityLogger; @@ -256,7 +259,7 @@ public Object[][] createValidTemporalConstraintData() { { json(object()) }, { json(object(field(RelationshipValidator.TEMPORAL_CONSTRAINTS, null))) }, { json(object(field(RelationshipValidator.TEMPORAL_CONSTRAINTS, array()))) }, - { makeTemporalConstraints("2016-01-01T00:00:00.000Z/2016-01-02T00:00:00.000Z") }, + { makeTemporalConstraints(VALID_DURATION) }, { makeTemporalConstraints("2016-01-01T00:00:00.000Z/P1D") } }; } @@ -270,21 +273,72 @@ public void testValidTemporalConstraints(JsonValue refProperties) throws BadRequ public Object[][] createInvalidTemporalConstraintData() { return new Object[][] { // end before start - { makeTemporalConstraints("2016-01-02T00:00:00.000Z/2016-01-01T00:00:00.000Z") }, - { makeTemporalConstraints("not an interval") }, - { makeTemporalConstraints(42) }, - { json(object(field(RelationshipValidator.TEMPORAL_CONSTRAINTS, array(object())))) }, - { json(object(field(RelationshipValidator.TEMPORAL_CONSTRAINTS, array("2016-01-01T00:00:00.000Z/P1D")))) }, - { json(object(field(RelationshipValidator.TEMPORAL_CONSTRAINTS, "2016-01-01T00:00:00.000Z/P1D"))) }, + { makeTemporalConstraints(REVERSED_DURATION), "Temporal constraint duration " + REVERSED_DURATION }, + { makeTemporalConstraints("not an interval"), "Temporal constraint duration not an interval" }, + { makeTemporalConstraints(42), "Temporal constraint duration 42" }, + { json(object(field(RelationshipValidator.TEMPORAL_CONSTRAINTS, array(object())))), + "Temporal constraint duration null" }, + { json(object(field(RelationshipValidator.TEMPORAL_CONSTRAINTS, array("2016-01-01T00:00:00.000Z/P1D")))), + "Temporal constraint duration null" }, + { json(object(field(RelationshipValidator.TEMPORAL_CONSTRAINTS, "2016-01-01T00:00:00.000Z/P1D"))), + "Temporal constraints must be an array." }, { json(object(field(RelationshipValidator.TEMPORAL_CONSTRAINTS, array( object(field(RelationshipValidator.DURATION, "2016-01-01T00:00:00.000Z/P1D")), - object(field(RelationshipValidator.DURATION, "2016-02-01T00:00:00.000Z/P1D")))))) } + object(field(RelationshipValidator.DURATION, "2016-02-01T00:00:00.000Z/P1D")))))), + "Only 1 temporal constraint" } }; } - @Test(dataProvider = "invalidTemporalConstraints", expectedExceptions = BadRequestException.class) - public void testInvalidTemporalConstraints(JsonValue refProperties) throws BadRequestException { - RelationshipValidator.validateTemporalConstraints(refProperties); + @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()); + } + } + + @DataProvider(name = "changedTemporalConstraints") + public Object[][] createChangedTemporalConstraintData() { + return new Object[][] { + // the stored constraint is replaced, or a constraint is added + { makeTemporalConstraints(VALID_DURATION), makeTemporalConstraints(REVERSED_DURATION) }, + { json(object()), makeTemporalConstraints(REVERSED_DURATION) }, + { null, makeTemporalConstraints(REVERSED_DURATION) } + }; + } + + @Test(dataProvider = "changedTemporalConstraints", expectedExceptions = BadRequestException.class, + expectedExceptionsMessageRegExp = "Temporal constraint duration " + REVERSED_DURATION + " .*") + public void testChangedInvalidTemporalConstraints(JsonValue oldRefProperties, JsonValue newRefProperties) + throws BadRequestException { + RelationshipValidator.validateChangedTemporalConstraints(oldRefProperties, newRefProperties); + } + + @Test + public void testUnchangedInvalidTemporalConstraints() throws BadRequestException { + final JsonValue newRefProperties = makeTemporalConstraints(REVERSED_DURATION); + newRefProperties.put(RelationshipValidator.GRANT_TYPE, "conditional"); + RelationshipValidator.validateChangedTemporalConstraints(makeTemporalConstraints(REVERSED_DURATION), + newRefProperties); + } + + @Test(expectedExceptions = BadRequestException.class, + expectedExceptionsMessageRegExp = "Temporal constraint duration " + REVERSED_DURATION + " .*") + public void testValidateRelationshipRejectsInvalidTemporalConstraint() throws ResourceException { + // the duration is checked before the referenced object is read, which this connection factory cannot do + final SchemaField schemaField = mock(SchemaField.class); + when(schemaField.isReverseRelationship()).thenReturn(false); + when(schemaField.getName()).thenReturn("roles"); + final CollectionRelationshipProvider relationshipProvider = new CollectionRelationshipProvider( + mock(ConnectionFactory.class), new ResourcePath("managed/user"), schemaField, activityLogger, + managedObjectSyncService); + final JsonValue grant = json(object( + field(RelationshipUtil.REFERENCE_ID, "managed/role/r1"), + field(RelationshipUtil.REFERENCE_PROPERTIES, makeTemporalConstraints(REVERSED_DURATION).getObject()))); + relationshipProvider.relationshipValidator.validateRelationship(grant, new ResourcePath("managed/user/u1"), + new RootContext(), false); } private JsonValue makeTemporalConstraints(Object duration) { diff --git a/openidm-ui/openidm-ui-admin/src/test/qunit/org/forgerock/openidm/ui/admin/role/util/TemporalConstraintsUtilsTest.js b/openidm-ui/openidm-ui-admin/src/test/qunit/org/forgerock/openidm/ui/admin/role/util/TemporalConstraintsUtilsTest.js index b6451d6282..dc8f4a1ed8 100644 --- a/openidm-ui/openidm-ui-admin/src/test/qunit/org/forgerock/openidm/ui/admin/role/util/TemporalConstraintsUtilsTest.js +++ b/openidm-ui/openidm-ui-admin/src/test/qunit/org/forgerock/openidm/ui/admin/role/util/TemporalConstraintsUtilsTest.js @@ -15,8 +15,9 @@ */ define([ + "jquery", "org/forgerock/openidm/ui/admin/role/util/TemporalConstraintsUtils" -], function (TemporalConstraintsUtils) { +], function ($, TemporalConstraintsUtils) { QUnit.module('TemporalConstraintsUtils Tests'); QUnit.test("convertFromIntervalString", (assert) => { @@ -59,4 +60,20 @@ define([ assert.notOk(TemporalConstraintsUtils.isValidInterval("", "04/30/2016 7:00 AM"), "an empty start date is invalid"); assert.notOk(TemporalConstraintsUtils.isValidInterval("04/25/2016 7:00 AM", "not a date"), "an unparseable end date is invalid"); }); + + QUnit.test("isTemporalConstraintsFormValid", (assert) => { + const constraint = (start, end) => "
" + + "" + + "
", + form = (...constraints) => $("
" + constraints.join("") + "
"); + + assert.ok(TemporalConstraintsUtils.isTemporalConstraintsFormValid(form()), "a form without constraints is valid"); + assert.ok(TemporalConstraintsUtils.isTemporalConstraintsFormValid( + form(constraint("04/25/2016 7:00 AM", "04/30/2016 7:00 AM"))), "end after start is valid"); + assert.notOk(TemporalConstraintsUtils.isTemporalConstraintsFormValid( + form(constraint("04/25/2016 7:00 AM", ""))), "an empty end date is invalid"); + assert.notOk(TemporalConstraintsUtils.isTemporalConstraintsFormValid( + form(constraint("04/25/2016 7:00 AM", "04/30/2016 7:00 AM"), constraint("04/30/2016 7:00 AM", "04/25/2016 7:00 AM"))), + "one constraint with the end before the start makes the form invalid"); + }); }); diff --git a/openidm-zip/src/main/resources/bin/defaults/script/roles/conditionalRoles.js b/openidm-zip/src/main/resources/bin/defaults/script/roles/conditionalRoles.js index e8a4aa7e3f..2b66a8c49e 100644 --- a/openidm-zip/src/main/resources/bin/defaults/script/roles/conditionalRoles.js +++ b/openidm-zip/src/main/resources/bin/defaults/script/roles/conditionalRoles.js @@ -107,7 +107,10 @@ if (isTemporalConstraintsMultiValue(newRole)) { throw {code : 400, message: "Only 1 temporal constraint is supported per role."} } - validateTemporalConstraintDurations(newRole); + // A stored invalid constraint must not prevent other changes to the role; a change to it must make it valid + if (JSON.stringify(oldRole.temporalConstraints) !== JSON.stringify(newRole.temporalConstraints)) { + validateTemporalConstraintDurations(newRole); + } /* Only iterate through all of the users if we are dealing with a conditional role, and if the role condition has changed. And if the role's condition has been removed, the new role grantees will be only diff --git a/openidm-zip/src/test/resources/bin/defaults/script/conditionalRolesTest.js b/openidm-zip/src/test/resources/bin/defaults/script/conditionalRolesTest.js index bd3628eab7..661a87e259 100644 --- a/openidm-zip/src/test/resources/bin/defaults/script/conditionalRolesTest.js +++ b/openidm-zip/src/test/resources/bin/defaults/script/conditionalRolesTest.js @@ -60,6 +60,43 @@ exports.test = function() { } }).apply(null, testcase); }); + + // roleCreate and roleUpdate reject a role with an invalid duration; the roles are not conditional, so no + // openidm call is reached + [ + [ "roleCreate rejects an invalid constraint", function (role) { + conditionalRoles.roleCreate(role); + }, true ], + [ "roleUpdate rejects a constraint changed to an invalid one", function (role) { + conditionalRoles.roleUpdate({ "_id": role._id, "temporalConstraints": [ { "duration": currentDuration } ] }, role); + }, true ], + [ "roleUpdate rejects an added invalid constraint", function (role) { + conditionalRoles.roleUpdate({ "_id": role._id }, role); + }, true ], + [ "roleUpdate keeps a stored invalid constraint that the update does not change", function (role) { + conditionalRoles.roleUpdate({ "_id": role._id, "description": "before", + "temporalConstraints": [ { "duration": reversedDuration } ] }, role); + }, false ] + ].map( + function (testcase) { + (function (description, write, expectedRejection) { + var rejected = false; + try { + write({ "_id": "roleWithReversedConstraint", "description": "after", + "temporalConstraints": [ { "duration": reversedDuration } ] }); + } catch (e) { + if (e.code !== 400) { + throw e; + } + rejected = true; + } + if (rejected !== expectedRejection) { + throw { + "message": description + ": rejected <" + rejected + ">, expected <" + expectedRejection + ">" + }; + } + }).apply(null, testcase); + }); } function isTemporalConstraintsMultiValue() { diff --git a/openidm-zip/src/test/resources/bin/defaults/script/effectiveRolesTest.js b/openidm-zip/src/test/resources/bin/defaults/script/effectiveRolesTest.js index 38118e72ab..b009b09530 100644 --- a/openidm-zip/src/test/resources/bin/defaults/script/effectiveRolesTest.js +++ b/openidm-zip/src/test/resources/bin/defaults/script/effectiveRolesTest.js @@ -140,6 +140,13 @@ exports.test = function() { ] }, true + ], + [ + { + "_id" : "role9", + "temporalConstraints" : [ null ] + }, + false ] ].map( function (testcase) { @@ -154,5 +161,26 @@ exports.test = function() { } }).apply(null, testcase); }); + + // an operator finds the stored invalid constraints by this warning + (function () { + var warnings = [], + warn = logger.warn; + logger.warn = function () { + warnings.push(Array.prototype.slice.call(arguments)); + }; + try { + effectiveRoles.processTemporalConstraints( + { "_id" : "role5", "temporalConstraints" : [ { "duration" : reversedDuration } ] }); + } finally { + logger.warn = warn; + } + if (warnings.length !== 1 || String(warnings[0][1]) !== reversedDuration) { + throw { + "message": "Expected one warning naming the invalid duration " + reversedDuration + ", got " + + JSON.stringify(warnings) + }; + } + }()); } } \ No newline at end of file diff --git a/openidm-zip/src/test/resources/bin/defaults/script/postOperationRolesTest.js b/openidm-zip/src/test/resources/bin/defaults/script/postOperationRolesTest.js new file mode 100644 index 0000000000..76ae7e8e91 --- /dev/null +++ b/openidm-zip/src/test/resources/bin/defaults/script/postOperationRolesTest.js @@ -0,0 +1,84 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ + +/** + * Tests against roles/postOperation-roles.js, which is not a module: it runs with the globals of a managed object + * postCreate/postUpdate/postDelete script, so it is evaluated here as the body of a function taking those globals. + */ +exports.test = function() { + var source = readScript("bin/defaults/script/roles/postOperation-roles.js"), + postOperationRoles = new Function("request", "context", "oldObject", "newObject", "resourceName", "openidm", + source); + + createJobsForConstraint(); + + function readScript(path) { + var stream = java.lang.Thread.currentThread().getContextClassLoader().getResourceAsStream(path); + if (stream === null) { + throw { "message": "Script not found on the classpath: " + path }; + } + try { + return String(new java.lang.String(stream.readAllBytes(), "UTF-8")); + } finally { + stream.close(); + } + } + + /** + * Runs the script for a created object and returns the ids of the schedules it created. + */ + function createdJobs(resourceName, newObject) { + var jobs = [], + openidm = { + "create": function (resourceContainer, newResourceId) { + jobs.push(String(newResourceId)); + } + }; + postOperationRoles({ "method": "create" }, null, null, newObject, new java.lang.String(resourceName), openidm); + return jobs; + } + + function createJobsForConstraint() { + var dateUtil = org.forgerock.openidm.util.DateUtil.getDateUtil(), + now = dateUtil.currentDateTime(), + pendingDuration = dateUtil.formatDateTime(now.plusDays(1)) + + "/" + dateUtil.formatDateTime(now.plusDays(2)), + // end before start, see issue #250 + reversedDuration = dateUtil.formatDateTime(now.plusDays(1)) + + "/" + dateUtil.formatDateTime(now.minusDays(1)); + [ + [ "managed/role/r1", { "temporalConstraints": [ { "duration": pendingDuration } ] }, 2 ], + // an invalid duration that is already stored must not fail the request, nor create a schedule + [ "managed/role/r1", { "temporalConstraints": [ { "duration": reversedDuration } ] }, 0 ], + [ "managed/role/r1", { "temporalConstraints": [ { "duration": "not an interval" } ] }, 0 ], + [ "managed/user/u1", { "roles": [ { "_ref": "managed/role/r1", + "_refProperties": { "_id": "g1", "temporalConstraints": [ { "duration": pendingDuration } ] } } ] }, 2 ], + [ "managed/user/u1", { "roles": [ { "_ref": "managed/role/r1", + "_refProperties": { "_id": "g1", "temporalConstraints": [ { "duration": reversedDuration } ] } } ] }, 0 ] + ].map( + function (testcase) { + (function (resourceName, newObject, expectedJobs) { + var jobs = createdJobs(resourceName, newObject); + if (jobs.length !== expectedJobs) { + throw { + "message": "Creating " + resourceName + " " + JSON.stringify(newObject) + " created jobs " + + JSON.stringify(jobs) + ", expected " + expectedJobs + " jobs" + }; + } + }).apply(null, testcase); + }); + } +} diff --git a/openidm-zip/src/test/resources/bin/defaults/script/temporalConstraintsTest.js b/openidm-zip/src/test/resources/bin/defaults/script/temporalConstraintsTest.js index 8e49046bab..cfc383fb94 100644 --- a/openidm-zip/src/test/resources/bin/defaults/script/temporalConstraintsTest.js +++ b/openidm-zip/src/test/resources/bin/defaults/script/temporalConstraintsTest.js @@ -95,6 +95,18 @@ exports.test = function() { ] }, true + ], + [ + { + "_id" : "role5", + "temporalConstraints" : [ + null, + { + "duration" : expiredDuration + } + ] + }, + true ] ].map( function (testcase) { diff --git a/openidm-zip/src/test/resources/testRunner.js b/openidm-zip/src/test/resources/testRunner.js index 4456f19628..50a75e39ba 100644 --- a/openidm-zip/src/test/resources/testRunner.js +++ b/openidm-zip/src/test/resources/testRunner.js @@ -34,6 +34,7 @@ var logger = (function () { "effectiveAssignmentsTest", "temporalConstraintsTest", "conditionalRolesTest", + "postOperationRolesTest", "managedPatchHelperTest", "connectionPoolPatchHelperTest"] .forEach(function (module) { From 42daf147268759caf958593318bda2817956dc10 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Wed, 7 Oct 2026 14:20:03 +0300 Subject: [PATCH 3/3] [#250] Accept unchanged stored constraints on the managed object path - CollectionRelationshipProvider.validateRelationshipField: check a relationship that a managed object write updates (matched by _refProperties._id) only if its temporal constraints changed, and check one re-sent equal to a stored relationship but without its _id before the commit, since persisting it deletes the stored relationship and creates it again. - postOperation-roles.createJobsForConstraint: skip a null constraint. - Test validateRelationshipField for a grant that keeps, changes or re-sends a stored invalid constraint, and postOperation-roles for null constraints on a role and on a created or updated grant. --- .../CollectionRelationshipProvider.java | 18 +++++- .../managed/RelationshipValidator.java | 23 +++++++- .../CollectionRelationshipProviderTest.java | 57 +++++++++++++++++++ .../script/roles/postOperation-roles.js | 7 ++- .../defaults/script/postOperationRolesTest.js | 42 ++++++++------ 5 files changed, 126 insertions(+), 21 deletions(-) diff --git a/openidm-core/src/main/java/org/forgerock/openidm/managed/CollectionRelationshipProvider.java b/openidm-core/src/main/java/org/forgerock/openidm/managed/CollectionRelationshipProvider.java index 87cb82ebff..2d6c31b68e 100644 --- a/openidm-core/src/main/java/org/forgerock/openidm/managed/CollectionRelationshipProvider.java +++ b/openidm-core/src/main/java/org/forgerock/openidm/managed/CollectionRelationshipProvider.java @@ -25,8 +25,10 @@ import static org.forgerock.util.query.QueryFilter.*; import java.util.ArrayList; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; import org.forgerock.http.routing.RoutingMode; @@ -599,16 +601,30 @@ public void validateRelationshipField(Context context, JsonValue oldValue, JsonV relationships are re-checked, then the invocation will be rejected because the relationship currently exists. */ Set oldReferences = new HashSet<>(); + Map oldRefPropertiesById = new HashMap<>(); if (oldValue.isNotNull()) { for (JsonValue oldItem : oldValue) { oldReferences.add(new RelationshipEqualityHash(oldItem)); + final JsonValue oldId = oldItem.get(FIELD_ID); + if (oldId != null && oldId.isString()) { + oldRefPropertiesById.put(oldId.asString(), oldItem.get(FIELD_PROPERTIES)); + } } } for (JsonValue newItem : newValue) { + final JsonValue id = newItem.get(FIELD_ID); + final boolean hasId = id != null && id.isNotNull(); // If the relationship is found in the existing/old relationships, then must skip validation. if (!oldReferences.contains(new RelationshipEqualityHash(newItem))) { logger.debug("validating new relationship {} for {}: ", newItem, propertyPtr); - relationshipValidator.validateRelationship(newItem, referrerId, context, performDuplicateAssignmentCheck); + // An updated relationship may keep the temporal constraints it was stored with, even invalid ones + relationshipValidator.validateRelationship(newItem, + hasId && id.isString() ? oldRefPropertiesById.get(id.asString()) : null, + referrerId, context, performDuplicateAssignmentCheck); + } 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)); } } } diff --git a/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipValidator.java b/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipValidator.java index 1332838d0d..e06faf11a5 100644 --- a/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipValidator.java +++ b/openidm-core/src/main/java/org/forgerock/openidm/managed/RelationshipValidator.java @@ -108,6 +108,27 @@ abstract void validateSuccessfulReadResponse(Context context, JsonValue relation final void validateRelationship(final JsonValue relationshipField, ResourcePath referrerId, Context context, boolean performDuplicateAssignmentCheck) throws ResourceException { + validateRelationship(relationshipField, null, referrerId, context, performDuplicateAssignmentCheck); + } + + /** + * Validates that the relationshipField will not create an invalid condition, like + * {@link #validateRelationship(JsonValue, ResourcePath, Context, boolean)}, but checks its temporal constraints + * only if they differ from those of the stored relationship it updates. + * + * @param relationshipField the field defining an individual relationship which will be validated. + * @param storedRefProperties the _refProperties of the stored relationship which the relationshipField updates, + * or null if it creates a relationship. + * @param referrerId the id of the object 'hosting' the relationships, aka the referrer + * @param context context of the request working with the relationship. + * @param performDuplicateAssignmentCheck set to true if invocation state should be compared to repository state to determine if + * existing relationships are specified in the invocation + * @throws ResourceException BadRequestException when the relationship is invalid, otherwise for other issues. + * @see #validateChangedTemporalConstraints(JsonValue, JsonValue) + */ + final void validateRelationship(final JsonValue relationshipField, final JsonValue storedRefProperties, + ResourcePath referrerId, Context context, boolean performDuplicateAssignmentCheck) + throws ResourceException { if (relationshipField.isNull()) { // if the new object has the relationshipField removed, we do not need to validate the null // relationshipField because there is no relationship to validate @@ -118,7 +139,7 @@ final void validateRelationship(final JsonValue relationshipField, ResourcePath logger.debug(message); throw new BadRequestException(message); } - validateTemporalConstraints(relationshipField.get(REFERENCE_PROPERTIES)); + validateChangedTemporalConstraints(storedRefProperties, relationshipField.get(REFERENCE_PROPERTIES)); try { validateSuccessfulReadResponse(context, relationshipField, referrerId, relationshipProvider.getConnection() .read(context, newValidateRequest(relationshipField, context)), performDuplicateAssignmentCheck); diff --git a/openidm-core/src/test/java/org/forgerock/openidm/managed/CollectionRelationshipProviderTest.java b/openidm-core/src/test/java/org/forgerock/openidm/managed/CollectionRelationshipProviderTest.java index 80328df6e3..723a0c9ce8 100644 --- a/openidm-core/src/test/java/org/forgerock/openidm/managed/CollectionRelationshipProviderTest.java +++ b/openidm-core/src/test/java/org/forgerock/openidm/managed/CollectionRelationshipProviderTest.java @@ -218,6 +218,55 @@ public void testPatchRejectsInvalidTemporalConstraint() throws Exception { "/_refProperties/temporalConstraints/0/duration", REVERSED_DURATION))).getOrThrow(); } + @Test + public void testValidateFieldKeepsUnchangedInvalidTemporalConstraintOfChangedGrant() throws Exception { + final Connection connection = connectionWithReadableRole(); + final JsonValue changedGrant = grant("g1", REVERSED_DURATION); + changedGrant.put(new JsonPointer("/_refProperties/_grantType"), "conditional"); + + newRolesProvider(connection).validateRelationshipField(managedObjectContext(), + json(array(grant("g1", REVERSED_DURATION).getObject())), json(array(changedGrant.getObject())), + REFERRING_OBJECT_ID, false); + + // the changed grant is still validated + verify(connection).read(any(Context.class), any(ReadRequest.class)); + } + + @Test(expectedExceptions = BadRequestException.class, + expectedExceptionsMessageRegExp = "Temporal constraint duration " + REVERSED_DURATION + " .*") + public void testValidateFieldRejectsChangedInvalidTemporalConstraint() throws Exception { + newRolesProvider(connectionWithReadableRole()).validateRelationshipField(managedObjectContext(), + json(array(grant("g1", VALID_DURATION).getObject())), + json(array(grant("g1", REVERSED_DURATION).getObject())), REFERRING_OBJECT_ID, false); + } + + @Test + public void testValidateFieldRejectsResentInvalidGrantWithoutId() throws Exception { + // persisting a grant without its _id deletes the stored grant and creates it again, after the commit + final Connection connection = mock(Connection.class); + try { + newRolesProvider(connection).validateRelationshipField(managedObjectContext(), + json(array(grant("g1", REVERSED_DURATION).getObject())), + json(array(grant(null, REVERSED_DURATION).getObject())), REFERRING_OBJECT_ID, false); + fail("Expected BadRequestException"); + } catch (BadRequestException e) { + assertTrue(e.getMessage().startsWith("Temporal constraint duration " + REVERSED_DURATION), + e.getMessage()); + } + verifyZeroInteractions(connection); + } + + @Test + public void testValidateFieldKeepsResentInvalidGrant() throws Exception { + final Connection connection = mock(Connection.class); + + newRolesProvider(connection).validateRelationshipField(managedObjectContext(), + json(array(grant("g1", REVERSED_DURATION).getObject())), + json(array(grant("g1", REVERSED_DURATION).getObject())), REFERRING_OBJECT_ID, false); + + verifyZeroInteractions(connection); + } + private CollectionRelationshipProvider newRolesProvider(final Connection connection) throws Exception { final ConnectionFactory factory = mock(ConnectionFactory.class); when(factory.getConnection()).thenReturn(connection); @@ -252,6 +301,14 @@ private static Connection connectionWithStoredGrant(final String duration) throw return connection; } + /** A connection which reads the role r1, as the validation of a changed grant does. */ + private static Connection connectionWithReadableRole() throws Exception { + final Connection connection = mock(Connection.class); + when(connection.read(any(Context.class), any(ReadRequest.class))) + .thenReturn(newResourceResponse("r1", "1", json(object(field("_id", "r1"))))); + return connection; + } + /** A grant of role r1 with a temporal constraint, as a relationship request carries it. */ private static JsonValue grant(final String id, final String duration) { final JsonValue refProperties = json(object(field(RelationshipValidator.TEMPORAL_CONSTRAINTS, diff --git a/openidm-zip/src/main/resources/bin/defaults/script/roles/postOperation-roles.js b/openidm-zip/src/main/resources/bin/defaults/script/roles/postOperation-roles.js index eca98943ed..b289edd062 100644 --- a/openidm-zip/src/main/resources/bin/defaults/script/roles/postOperation-roles.js +++ b/openidm-zip/src/main/resources/bin/defaults/script/roles/postOperation-roles.js @@ -245,11 +245,12 @@ function deleteJobsForRoleConstraint(index) { */ function createJobsForConstraint(constraint, startJobId, endJobId, script) { logger.debug("creating new jobs for: " + constraint); - var dateUtil = org.forgerock.openidm.util.DateUtil.getDateUtil(); + var dateUtil = org.forgerock.openidm.util.DateUtil.getDateUtil(), + duration = (constraint !== undefined && constraint !== null) ? constraint.duration : constraint; // The resource has already been stored, so an invalid duration must not fail the request - if (!dateUtil.isValidInterval(constraint.duration)) { + if (!dateUtil.isValidInterval(duration)) { logger.warn("Not creating schedules for temporal constraint on resource " + resourceName - + " with an invalid duration: " + constraint.duration); + + " with an invalid duration: " + duration); return false; } var startDate = dateUtil.getStartOfInterval(constraint.duration), diff --git a/openidm-zip/src/test/resources/bin/defaults/script/postOperationRolesTest.js b/openidm-zip/src/test/resources/bin/defaults/script/postOperationRolesTest.js index 76ae7e8e91..cb4c9d1237 100644 --- a/openidm-zip/src/test/resources/bin/defaults/script/postOperationRolesTest.js +++ b/openidm-zip/src/test/resources/bin/defaults/script/postOperationRolesTest.js @@ -38,19 +38,26 @@ exports.test = function() { } /** - * Runs the script for a created object and returns the ids of the schedules it created. + * Runs the script for a created or updated object and returns the ids of the schedules it created. */ - function createdJobs(resourceName, newObject) { + function createdJobs(method, resourceName, oldObject, newObject) { var jobs = [], openidm = { "create": function (resourceContainer, newResourceId) { jobs.push(String(newResourceId)); - } + }, + "delete": function () {} }; - postOperationRoles({ "method": "create" }, null, null, newObject, new java.lang.String(resourceName), openidm); + postOperationRoles({ "method": method }, null, oldObject, newObject, new java.lang.String(resourceName), + openidm); return jobs; } + function grant(constraints) { + return { "roles": [ { "_ref": "managed/role/r1", + "_refProperties": { "_id": "g1", "temporalConstraints": constraints } } ] }; + } + function createJobsForConstraint() { var dateUtil = org.forgerock.openidm.util.DateUtil.getDateUtil(), now = dateUtil.currentDateTime(), @@ -60,22 +67,25 @@ exports.test = function() { reversedDuration = dateUtil.formatDateTime(now.plusDays(1)) + "/" + dateUtil.formatDateTime(now.minusDays(1)); [ - [ "managed/role/r1", { "temporalConstraints": [ { "duration": pendingDuration } ] }, 2 ], - // an invalid duration that is already stored must not fail the request, nor create a schedule - [ "managed/role/r1", { "temporalConstraints": [ { "duration": reversedDuration } ] }, 0 ], - [ "managed/role/r1", { "temporalConstraints": [ { "duration": "not an interval" } ] }, 0 ], - [ "managed/user/u1", { "roles": [ { "_ref": "managed/role/r1", - "_refProperties": { "_id": "g1", "temporalConstraints": [ { "duration": pendingDuration } ] } } ] }, 2 ], - [ "managed/user/u1", { "roles": [ { "_ref": "managed/role/r1", - "_refProperties": { "_id": "g1", "temporalConstraints": [ { "duration": reversedDuration } ] } } ] }, 0 ] + [ "create", "managed/role/r1", null, { "temporalConstraints": [ { "duration": pendingDuration } ] }, 2 ], + // an invalid constraint that is already stored must not fail the request, nor create a schedule + [ "create", "managed/role/r1", null, { "temporalConstraints": [ { "duration": reversedDuration } ] }, 0 ], + [ "create", "managed/role/r1", null, { "temporalConstraints": [ { "duration": "not an interval" } ] }, 0 ], + [ "create", "managed/role/r1", null, { "temporalConstraints": [ null ] }, 0 ], + [ "create", "managed/user/u1", null, grant([ { "duration": pendingDuration } ]), 2 ], + [ "create", "managed/user/u1", null, grant([ { "duration": reversedDuration } ]), 0 ], + [ "create", "managed/user/u1", null, grant([ null ]), 0 ], + [ "update", "managed/user/u1", grant([ { "duration": reversedDuration } ]), + grant([ { "duration": pendingDuration } ]), 2 ], + [ "update", "managed/user/u1", grant([ { "duration": pendingDuration } ]), grant([ null ]), 0 ] ].map( function (testcase) { - (function (resourceName, newObject, expectedJobs) { - var jobs = createdJobs(resourceName, newObject); + (function (method, resourceName, oldObject, newObject, expectedJobs) { + var jobs = createdJobs(method, resourceName, oldObject, newObject); if (jobs.length !== expectedJobs) { throw { - "message": "Creating " + resourceName + " " + JSON.stringify(newObject) + " created jobs " - + JSON.stringify(jobs) + ", expected " + expectedJobs + " jobs" + "message": method + " of " + resourceName + " " + JSON.stringify(newObject) + + " created jobs " + JSON.stringify(jobs) + ", expected " + expectedJobs + " jobs" }; } }).apply(null, testcase);