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..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 @@ -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())) { @@ -322,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); @@ -342,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); } @@ -351,6 +354,19 @@ 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."); + } + // Without a __UID__ element the uid is the name, as create() makes it. + if (!isSupportUid(objClass)) { + newUid = new Uid(value); + } + } + } + checkedAttributes.add(new CheckedAttribute(attributeName, values)); } @@ -382,7 +398,7 @@ public Uid update(ObjectClass objClass, Uid uid, Set replaceAttribute log.info("Exit {0}", method); - return uid; + return newUid != null ? newUid : uid; } @Override @@ -735,6 +751,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/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 9429f126..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 @@ -89,12 +89,13 @@ 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"), - Collections.singleton(AttributeBuilder.build(Name.NAME, "dup"))); + Collections.singleton(AttributeBuilder.build(Name.NAME, "renamed-again"))); handler.create(ObjectClass.ACCOUNT, account("renamed")); } @@ -122,6 +123,110 @@ 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. __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"); + } + + @Test + 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); + // 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() 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()); + } + } + + @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: @@ -281,9 +386,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");