From 08f686b0d3470a8dbf4619d5367883270f58320d Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 16 Oct 2026 09:53:28 +0300 Subject: [PATCH 1/5] [#159] Refuse an XML update that renames an entry to a name another entry has update() now checks a new __NAME__ against the entry index in its check phase, before the entry changes, and refuses it with AlreadyExistsException when another entry of the same class has it. An entry that already has the name may send it again, even when another entry has it too. --- .../connectors/xml/XMLHandlerImpl.java | 16 +++++++ .../connectors/xml/XMLHandlerIndexTests.java | 42 +++++++++++++++++-- .../connectors/xml/XMLHandlerReloadTests.java | 20 +++++++++ 3 files changed, 74 insertions(+), 4 deletions(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java index 586a0b76..efe0efcc 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java @@ -351,6 +351,15 @@ public Uid update(ObjectClass objClass, Uid uid, Set replaceAttribute throw new IllegalArgumentException("Data field: " + attributeName + " is not multivalued can not have more than one value"); } + if (Name.NAME.equals(attributeName)) { + for (String value : values) { + if (nameTakenByAnother(objClass, entry, value)) { + throw new AlreadyExistsException("Could not update entry. An entry with the " + Name.NAME + " of " + + value + " already exists."); + } + } + } + checkedAttributes.add(new CheckedAttribute(attributeName, values)); } @@ -735,6 +744,13 @@ private boolean entryExists(ObjectClass objClass, Uid uid, ElementIdentifierFiel return getEntry(objClass, uid, identifierField) != null; } + /** An entry that already has {@code name} keeps it, even when another entry has it too. */ + private boolean nameTakenByAnother(ObjectClass objClass, Element entry, String name) { + List holders = entryIndex.find(objClass.getObjectClassValue(), + identifierElement(objClass, ElementIdentifierFieldType.BY_NAME), name); + return !holders.isEmpty() && !holders.contains(entry); + } + private String getElementIdentifierField(ObjectClass objClass, ElementIdentifierFieldType identifierField) { String elementField = ""; diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java index 9429f126..8a898a70 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java @@ -94,7 +94,7 @@ public void renamedEntryIsFoundByItsNewNameOnly() { expectThrows(AlreadyExistsException.class, () -> handler.create(ObjectClass.ACCOUNT, account("renamed"))); handler.update(ObjectClass.ACCOUNT, new Uid("u1"), - Collections.singleton(AttributeBuilder.build(Name.NAME, "dup"))); + Collections.singleton(AttributeBuilder.build(Name.NAME, "renamed-again"))); handler.create(ObjectClass.ACCOUNT, account("renamed")); } @@ -122,6 +122,41 @@ public void rejectedRenameKeepsTheOldName() { Arrays.asList("u1|dup", "u2|dup")); } + @Test + public void renameIntoANameAnotherEntryHasIsRefused() { + Uid fresh = handler.create(ObjectClass.ACCOUNT, account("fresh")); + AlreadyExistsException e = expectThrows(AlreadyExistsException.class, () -> handler.update(ObjectClass.ACCOUNT, + fresh, Collections.singleton(AttributeBuilder.build(Name.NAME, "dup")))); + assertEquals(e.getMessage(), "Could not update entry. An entry with the __NAME__ of dup already exists."); + + // Only a GROUP has the name The Empire. + handler.create(ObjectClass.GROUP, getRequiredGroupAttributes()); + expectThrows(AlreadyExistsException.class, () -> handler.update(ObjectClass.GROUP, + new Uid("dup"), Collections.singleton(AttributeBuilder.build(Name.NAME, ATTR_GROUP_VALUE_NAME)))); + + // u10's name is "" in the index: a blank name is refused as blank, not as taken. + IllegalArgumentException blank = expectThrows(IllegalArgumentException.class, () -> handler.update( + ObjectClass.ACCOUNT, new Uid("u1"), Collections.singleton(AttributeBuilder.build(Name.NAME, "")))); + assertEquals(blank.getMessage(), "Parameter '__NAME__' must not be blank."); + } + + @Test + public void keepingANameAnotherEntryAlsoHasIsAllowed() { + Set keepName = new LinkedHashSet(); + keepName.add(AttributeBuilder.build(Name.NAME, "dup")); + keepName.add(AttributeBuilder.build(ATTR_ACCOUNT_LAST_NAME, "L2b")); + handler.update(ObjectClass.ACCOUNT, new Uid("u2"), keepName); + assertEquals(describe(handler.search(equalsQuery(Name.NAME, "dup"), ObjectClass.ACCOUNT)), + Arrays.asList("u1|dup", "u2|dup")); + + // Only __NAME__ is checked: another attribute may have a value that is someone's name. + handler.update(ObjectClass.ACCOUNT, new Uid("u4"), + Collections.singleton(AttributeBuilder.build(ATTR_ACCOUNT_LAST_NAME, "dup"))); + + // Only an ACCOUNT has the name no-uid. + handler.update(ObjectClass.GROUP, new Uid("dup"), Collections.singleton(AttributeBuilder.build(Name.NAME, "no-uid"))); + } + /** * Renames to {@code newName} along with an unknown attribute. update() rejects the unknown attribute * before it changes the entry, so the rename is not applied. A two-valued __NAME__ cannot be used: @@ -281,9 +316,8 @@ public void indexAgreesWithXQueryAfterChanges() throws Exception { assertIndexAgreesWithXQuery("loaded"); handler.create(ObjectClass.ACCOUNT, account("dup-new")); assertIndexAgreesWithXQuery("created"); - handler.update(ObjectClass.ACCOUNT, new Uid("u2"), Collections.singleton(AttributeBuilder.build(Name.NAME, "renamed"))); - handler.update(ObjectClass.ACCOUNT, new Uid("u2"), Collections.singleton(AttributeBuilder.build(Name.NAME, "dup"))); - assertIndexAgreesWithXQuery("renamed back to a duplicate"); + handler.update(ObjectClass.ACCOUNT, new Uid("u1"), Collections.singleton(AttributeBuilder.build(Name.NAME, "dup"))); + assertIndexAgreesWithXQuery("first duplicate re-keyed"); handler.delete(ObjectClass.ACCOUNT, new Uid("u1")); assertIndexAgreesWithXQuery("first duplicate deleted"); handler.update(ObjectClass.ACCOUNT, new Uid("u11"), Collections.singleton(AttributeBuilder.build(ATTR_ACCOUNT_LAST_NAME, "L11"))); diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java index 563509f6..c69d0a82 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java @@ -52,6 +52,7 @@ import org.forgerock.openicf.connectors.xml.query.QueryBuilder; import org.forgerock.openicf.connectors.xml.xsdparser.SchemaParser; import org.identityconnectors.common.security.GuardedString; +import org.identityconnectors.framework.common.exceptions.AlreadyExistsException; import org.identityconnectors.framework.common.exceptions.ConnectorException; import org.identityconnectors.framework.common.objects.Attribute; import org.identityconnectors.framework.common.objects.AttributeBuilder; @@ -186,6 +187,25 @@ public void rejectedUpdateLeavesTheEntryAndTheFileAlone(Set attribute assertEquals(call(h -> firstAndLastNames(h.search(allAccounts(), ObjectClass.ACCOUNT))), List.of("null|Last-alice")); } + @Test + public void renameIntoATakenNameLeavesTheEntryAndTheFileAlone() throws Exception { + writeAccounts(file, "alice", "bob"); + FileTime past = FileTime.fromMillis(now() - HOUR); + Files.setLastModifiedTime(file.toPath(), past); + byte[] before = Files.readAllBytes(file.toPath()); + + // The name comes after an attribute that would be applied first. + AlreadyExistsException e = expectThrows(AlreadyExistsException.class, () -> call(h -> h.update(ObjectClass.ACCOUNT, + new Uid("uid-alice"), attributes(AttributeBuilder.build(ATTR_ACCOUNT_FIRST_NAME, "CHANGED"), + AttributeBuilder.build(Name.NAME, "bob"))))); + + assertEquals(e.getMessage(), "Could not update entry. An entry with the __NAME__ of bob already exists."); + assertEquals(Files.getLastModifiedTime(file.toPath()), past); + assertEquals(Files.readAllBytes(file.toPath()), before); + assertEquals(call(h -> firstAndLastNames(h.search(allAccounts(), ObjectClass.ACCOUNT))), + List.of("null|Last-alice", "null|Last-bob")); + } + @Test public void updateIsInTheFileWhenTheLastUserLeaves() throws Exception { writeAccounts(file, "alice"); From b5da0458f0002a34d711c1c44b7aca47fc6faf3b Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 16 Oct 2026 09:57:39 +0300 Subject: [PATCH 2/5] [#159] Return the new uid when an XML update renames an entry whose uid is its name For an object class without a __UID__ element, create() makes the uid the name, but update() returned the uid it was given after a rename, so the caller kept a uid that no longer finds the entry. update() now returns the new name as the uid in that case. --- .../connectors/xml/XMLHandlerImpl.java | 7 +++++- .../connectors/xml/XMLConnectorTests.java | 22 +++++++++++++++++++ .../connectors/xml/XMLHandlerIndexTests.java | 12 +++++++--- 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java index efe0efcc..f4f87811 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java @@ -308,6 +308,7 @@ public Uid update(ObjectClass objClass, Uid uid, Set replaceAttribute // Every attribute is checked before the entry changes, so a rejected update leaves the document as it was (#158). List checkedAttributes = new ArrayList(); + Uid newUid = null; for (Attribute attribute : replaceAttributes) { if (!objAttributes.containsKey(attribute.getName())) { @@ -357,6 +358,10 @@ public Uid update(ObjectClass objClass, Uid uid, Set replaceAttribute throw new AlreadyExistsException("Could not update entry. An entry with the " + Name.NAME + " of " + value + " already exists."); } + // Without a __UID__ element the uid is the name, as create() makes it. + if (!isSupportUid(objClass)) { + newUid = new Uid(value); + } } } @@ -391,7 +396,7 @@ public Uid update(ObjectClass objClass, Uid uid, Set replaceAttribute log.info("Exit {0}", method); - return uid; + return newUid != null ? newUid : uid; } @Override diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java index db27f916..7d980aba 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java @@ -50,6 +50,7 @@ import org.identityconnectors.framework.api.APIConfiguration; import org.identityconnectors.framework.api.ConnectorFacade; import org.identityconnectors.framework.api.ConnectorFacadeFactory; +import org.identityconnectors.framework.common.exceptions.AlreadyExistsException; import org.identityconnectors.framework.common.exceptions.ConnectorException; import org.identityconnectors.framework.common.objects.Attribute; import org.identityconnectors.framework.common.objects.AttributeBuilder; @@ -197,6 +198,27 @@ public void updateShouldReturnUidWhenGivenValidParameters() { AssertJUnit.assertEquals(insertedUid.getUidValue(), updatedUid.getUidValue()); } + @Test + public void renamingAGroupReturnsItsNewNameAsUid() { + // __GROUP__ has no __UID__: its uid is the name. + facade.create(ObjectClass.GROUP, getRequiredGroupAttributes(), null); + Set rebels = new HashSet(); + rebels.add(AttributeBuilder.build(ATTR_NAME, "Rebels")); + rebels.add(AttributeBuilder.build(ATTR_DESCRIPTION, ATTR_GROUP_VALUE_DESCRIPTION)); + rebels.add(AttributeBuilder.build(ATTR_SHORT_NAME, ATTR_GROUP_VALUE_SHORT_NAME)); + facade.create(ObjectClass.GROUP, rebels, null); + + Uid uid = facade.update(ObjectClass.GROUP, new Uid("Rebels"), + Collections.singleton(AttributeBuilder.build(ATTR_NAME, "Alliance")), null); + + AssertJUnit.assertEquals("Alliance", uid.getUidValue()); + AssertJUnit.assertNull(facade.getObject(ObjectClass.GROUP, new Uid("Rebels"), null)); + AssertJUnit.assertEquals("Alliance", + facade.getObject(ObjectClass.GROUP, new Uid("Alliance"), null).getName().getNameValue()); + expectThrows(AlreadyExistsException.class, () -> facade.update(ObjectClass.GROUP, new Uid("Alliance"), + Collections.singleton(AttributeBuilder.build(ATTR_NAME, ATTR_GROUP_VALUE_NAME)), null)); + } + @Test public void failedAddAttributeValuesKeepsTheOldValue() { Set attributes = getRequiredAccountAttributes(); diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java index 8a898a70..805560d7 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java @@ -89,8 +89,9 @@ public void deleteRemovesTheFirstDuplicateAndExposesTheNext() { @Test public void renamedEntryIsFoundByItsNewNameOnly() { - handler.update(ObjectClass.ACCOUNT, new Uid("u1"), + Uid uid = handler.update(ObjectClass.ACCOUNT, new Uid("u1"), Collections.singleton(AttributeBuilder.build(Name.NAME, "renamed"))); + assertEquals(uid.getUidValue(), "u1"); // __ACCOUNT__ has a __UID__: the rename keeps it expectThrows(AlreadyExistsException.class, () -> handler.create(ObjectClass.ACCOUNT, account("renamed"))); handler.update(ObjectClass.ACCOUNT, new Uid("u1"), @@ -153,8 +154,13 @@ public void keepingANameAnotherEntryAlsoHasIsAllowed() { handler.update(ObjectClass.ACCOUNT, new Uid("u4"), Collections.singleton(AttributeBuilder.build(ATTR_ACCOUNT_LAST_NAME, "dup"))); - // Only an ACCOUNT has the name no-uid. - handler.update(ObjectClass.GROUP, new Uid("dup"), Collections.singleton(AttributeBuilder.build(Name.NAME, "no-uid"))); + // Only an ACCOUNT has the name no-uid. __GROUP__ has no __UID__: its uid is the name, so the rename changes it. + Uid renamed = handler.update(ObjectClass.GROUP, new Uid("dup"), + Collections.singleton(AttributeBuilder.build(Name.NAME, "no-uid"))); + assertEquals(renamed.getUidValue(), "no-uid"); + Uid kept = handler.update(ObjectClass.GROUP, new Uid("no-uid"), + Collections.singleton(AttributeBuilder.build(ATTR_DESCRIPTION, "D2"))); + assertEquals(kept.getUidValue(), "no-uid"); } /** From eebdda6b4db877c6a5d3918102d331e9bffe9336 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 16 Oct 2026 10:26:05 +0300 Subject: [PATCH 3/5] [#159] Let an XML update clear an optional __NAME__ on more than one entry create() refuses a blank name, so a blank name identifies no entry. The new uniqueness check treated a cleared optional __NAME__ as taken once another entry had cleared its name too; it now skips blank values. --- .../connectors/xml/XMLHandlerImpl.java | 5 ++++ .../connectors/xml/XMLHandlerIndexTests.java | 26 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java index f4f87811..0ac3440f 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java @@ -75,6 +75,7 @@ import org.identityconnectors.framework.common.objects.ObjectClassInfo; import org.identityconnectors.framework.common.objects.Schema; import org.identityconnectors.common.Assertions; +import org.identityconnectors.common.StringUtil; import org.identityconnectors.framework.common.exceptions.ConnectorException; import org.identityconnectors.framework.common.objects.AttributeInfoUtil; import org.identityconnectors.framework.common.objects.Uid; @@ -751,6 +752,10 @@ private boolean entryExists(ObjectClass objClass, Uid uid, ElementIdentifierFiel /** An entry that already has {@code name} keeps it, even when another entry has it too. */ private boolean nameTakenByAnother(ObjectClass objClass, Element entry, String name) { + if (StringUtil.isBlank(name)) { + // create() refuses a blank name, so it is no entry's: an optional __NAME__ may be cleared on several entries. + return false; + } List holders = entryIndex.find(objClass.getObjectClassValue(), identifierElement(objClass, ElementIdentifierFieldType.BY_NAME), name); return !holders.isEmpty() && !holders.contains(entry); diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java index 805560d7..b6f0136d 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java @@ -163,6 +163,32 @@ public void keepingANameAnotherEntryAlsoHasIsAllowed() { assertEquals(kept.getUidValue(), "no-uid"); } + @Test + public void clearingAnOptionalNameOfTwoEntriesIsAllowed() throws Exception { + // Next to the original, so that its import of resource-schema-1.xsd resolves. + File optionalNameXsd = getTestFile("optional-name.xsd"); + String xsd = new String(Files.readAllBytes(XSD_SCHEMA_FILEPATH.toPath()), StandardCharsets.UTF_8); + // The first __NAME__ reference is __ACCOUNT__'s. + Files.write(optionalNameXsd.toPath(), xsd.replaceFirst("", + "").getBytes(StandardCharsets.UTF_8)); + try { + File file = getRandomXMLFile(); + writeAccounts(file, "alice", "bob"); + XMLConfiguration config = new XMLConfiguration(); + config.setXmlFilePath(file); + config.setXsdFilePath(optionalNameXsd); + SchemaParser parser = new SchemaParser(XMLConnector.class, optionalNameXsd); + XMLHandlerImpl optional = new XMLHandlerImpl(config, parser.parseSchema(), parser.getXsdSchema()); + optional.init(); + + // create() refuses a blank name, so a cleared name is no other entry's. + optional.update(ObjectClass.ACCOUNT, new Uid("uid-alice"), Collections.singleton(AttributeBuilder.build(Name.NAME, ""))); + optional.update(ObjectClass.ACCOUNT, new Uid("uid-bob"), Collections.singleton(AttributeBuilder.build(Name.NAME, ""))); + } finally { + Files.delete(optionalNameXsd.toPath()); + } + } + /** * Renames to {@code newName} along with an unknown attribute. update() rejects the unknown attribute * before it changes the entry, so the rename is not applied. A two-valued __NAME__ cannot be used: From 9e7d4dc0995bac77ba0d3edfbdc86b4859906679 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 16 Oct 2026 11:07:12 +0300 Subject: [PATCH 4/5] [#159] Refuse a blank __NAME__ in an XML update of a class without __UID__ Without a __UID__ element the uid is the name, so a cleared name leaves an entry that no search of the class can read back. With an optional __NAME__ the required-name blank check is skipped, and update failed on new Uid("") with the framework's "Uid value must not be blank!"; it now refuses the blank name first with "Parameter '__NAME__' must not be blank.", as for a required name. --- .../connectors/xml/XMLHandlerImpl.java | 7 ++-- .../connectors/xml/XMLHandlerIndexTests.java | 32 +++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java index 0ac3440f..288ae6c6 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java @@ -359,8 +359,10 @@ public Uid update(ObjectClass objClass, Uid uid, Set replaceAttribute throw new AlreadyExistsException("Could not update entry. An entry with the " + Name.NAME + " of " + value + " already exists."); } - // Without a __UID__ element the uid is the name, as create() makes it. + // Without a __UID__ element the uid is the name, as create() makes it, so even an optional + // __NAME__ cannot be cleared: no entry could be read back without a uid. if (!isSupportUid(objClass)) { + Assertions.blankCheck(value, attributeName); newUid = new Uid(value); } } @@ -753,7 +755,8 @@ private boolean entryExists(ObjectClass objClass, Uid uid, ElementIdentifierFiel /** An entry that already has {@code name} keeps it, even when another entry has it too. */ private boolean nameTakenByAnother(ObjectClass objClass, Element entry, String name) { if (StringUtil.isBlank(name)) { - // create() refuses a blank name, so it is no entry's: an optional __NAME__ may be cleared on several entries. + // create() refuses a blank name, so it is no entry's: in a class with __UID__, an optional __NAME__ + // may be cleared on several entries. return false; } List holders = entryIndex.find(objClass.getObjectClassValue(), diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java index b6f0136d..6b2116db 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java @@ -189,6 +189,38 @@ public void clearingAnOptionalNameOfTwoEntriesIsAllowed() throws Exception { } } + @Test + public void clearingTheOptionalNameOfAnEntryWithoutUidIsRefused() throws Exception { + File optionalNameXsd = getTestFile("optional-group-name.xsd"); + String xsd = new String(Files.readAllBytes(XSD_SCHEMA_FILEPATH.toPath()), StandardCharsets.UTF_8); + String ref = ""; + // The second __NAME__ reference is __GROUP__'s, a class without __UID__. + int group = xsd.indexOf(ref, xsd.indexOf(ref) + 1); + Files.write(optionalNameXsd.toPath(), (xsd.substring(0, group) + "" + + xsd.substring(group + ref.length())).getBytes(StandardCharsets.UTF_8)); + try { + File file = getRandomXMLFile(); + Files.copy(getTestFile("index-fixture.xml").toPath(), file.toPath()); + XMLConfiguration config = new XMLConfiguration(); + config.setXmlFilePath(file); + config.setXsdFilePath(optionalNameXsd); + SchemaParser parser = new SchemaParser(XMLConnector.class, optionalNameXsd); + XMLHandlerImpl optional = new XMLHandlerImpl(config, parser.parseSchema(), parser.getXsdSchema()); + optional.init(); + + // The uid is the name, so a cleared name would leave an entry that no search could read back. + for (String blank : Arrays.asList("", " ")) { + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> optional.update( + ObjectClass.GROUP, new Uid("dup"), Collections.singleton(AttributeBuilder.build(Name.NAME, blank)))); + assertEquals(e.getMessage(), "Parameter '__NAME__' must not be blank."); + } + assertEquals(describe(optional.search(equalsQuery(Name.NAME, "dup"), ObjectClass.GROUP)), + Collections.singletonList("dup|dup")); + } finally { + Files.delete(optionalNameXsd.toPath()); + } + } + /** * Renames to {@code newName} along with an unknown attribute. update() rejects the unknown attribute * before it changes the entry, so the rename is not applied. A two-valued __NAME__ cannot be used: From 36def07e95c825fad3bccb60ea7eb870285f6aaa Mon Sep 17 00:00:00 2001 From: maximthomas Date: Mon, 19 Oct 2026 15:31:27 +0300 Subject: [PATCH 5/5] [#159] Refuse a blank __NAME__ in an XML update of any class create() requires __NAME__ whatever the XSD says, and the framework builds no ConnectorObject without a Name. With an optional __NAME__ in a class with __UID__, update accepted "" and left an entry that made every search of its class fail with "The Attribute set must contain a 'Name'.", the search by the entry's own __UID__ included; " " did the same once the file was saved and loaded again. update now checks __NAME__ as a required attribute in every class, so a blank one is refused with "Parameter '__NAME__' must not be blank." before the entry changes. The blank branch in nameTakenByAnother and the separate check for a class without __UID__ are no longer needed. --- .../openicf/connectors/xml/XMLHandlerImpl.java | 16 +++++----------- .../connectors/xml/XMLHandlerIndexTests.java | 14 ++++++++++---- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java index 288ae6c6..0bbf2629 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java @@ -75,7 +75,6 @@ import org.identityconnectors.framework.common.objects.ObjectClassInfo; import org.identityconnectors.framework.common.objects.Schema; import org.identityconnectors.common.Assertions; -import org.identityconnectors.common.StringUtil; import org.identityconnectors.framework.common.exceptions.ConnectorException; import org.identityconnectors.framework.common.objects.AttributeInfoUtil; import org.identityconnectors.framework.common.objects.Uid; @@ -324,7 +323,9 @@ public Uid update(ObjectClass objClass, Uid uid, Set replaceAttribute } List values = null; - if (attributeInfo.isRequired()) { + // create() requires __NAME__ whatever the XSD says, and an object without a name cannot be built, so a + // cleared __NAME__ would leave an entry that no search of the class can read back. + if (attributeInfo.isRequired() || Name.NAME.equals(attributeName)) { values = AttributeTypeUtil.findAttributeValue(attribute, attributeInfo); if (values.isEmpty()) { throw new IllegalArgumentException("No values provided for required attribute: " + attributeName); @@ -344,7 +345,7 @@ public Uid update(ObjectClass objClass, Uid uid, Set replaceAttribute } } - // A required attribute's values were read above; reading them again would decrypt a password twice. + // Values read above are not read again: that would decrypt a password twice. if (values == null) { values = AttributeTypeUtil.findAttributeValue(attribute, attributeInfo); } @@ -359,10 +360,8 @@ public Uid update(ObjectClass objClass, Uid uid, Set replaceAttribute throw new AlreadyExistsException("Could not update entry. An entry with the " + Name.NAME + " of " + value + " already exists."); } - // Without a __UID__ element the uid is the name, as create() makes it, so even an optional - // __NAME__ cannot be cleared: no entry could be read back without a uid. + // Without a __UID__ element the uid is the name, as create() makes it. if (!isSupportUid(objClass)) { - Assertions.blankCheck(value, attributeName); newUid = new Uid(value); } } @@ -754,11 +753,6 @@ private boolean entryExists(ObjectClass objClass, Uid uid, ElementIdentifierFiel /** An entry that already has {@code name} keeps it, even when another entry has it too. */ private boolean nameTakenByAnother(ObjectClass objClass, Element entry, String name) { - if (StringUtil.isBlank(name)) { - // create() refuses a blank name, so it is no entry's: in a class with __UID__, an optional __NAME__ - // may be cleared on several entries. - return false; - } List holders = entryIndex.find(objClass.getObjectClassValue(), identifierElement(objClass, ElementIdentifierFieldType.BY_NAME), name); return !holders.isEmpty() && !holders.contains(entry); diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java index 6b2116db..f2704d00 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerIndexTests.java @@ -164,7 +164,7 @@ public void keepingANameAnotherEntryAlsoHasIsAllowed() { } @Test - public void clearingAnOptionalNameOfTwoEntriesIsAllowed() throws Exception { + public void clearingAnOptionalNameIsRefused() throws Exception { // Next to the original, so that its import of resource-schema-1.xsd resolves. File optionalNameXsd = getTestFile("optional-name.xsd"); String xsd = new String(Files.readAllBytes(XSD_SCHEMA_FILEPATH.toPath()), StandardCharsets.UTF_8); @@ -181,9 +181,15 @@ public void clearingAnOptionalNameOfTwoEntriesIsAllowed() throws Exception { XMLHandlerImpl optional = new XMLHandlerImpl(config, parser.parseSchema(), parser.getXsdSchema()); optional.init(); - // create() refuses a blank name, so a cleared name is no other entry's. - optional.update(ObjectClass.ACCOUNT, new Uid("uid-alice"), Collections.singleton(AttributeBuilder.build(Name.NAME, ""))); - optional.update(ObjectClass.ACCOUNT, new Uid("uid-bob"), Collections.singleton(AttributeBuilder.build(Name.NAME, ""))); + // create() requires a name whatever the XSD says, and an object without one cannot be built: + // a cleared entry would fail every search of the class. + for (String blank : Arrays.asList("", " ")) { + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> optional.update( + ObjectClass.ACCOUNT, new Uid("uid-alice"), Collections.singleton(AttributeBuilder.build(Name.NAME, blank)))); + assertEquals(e.getMessage(), "Parameter '__NAME__' must not be blank."); + } + assertEquals(describe(optional.search((Query) null, ObjectClass.ACCOUNT)), + Arrays.asList("uid-alice|alice", "uid-bob|bob")); } finally { Files.delete(optionalNameXsd.toPath()); }