diff --git a/OpenICF-csvfile-connector/pom.xml b/OpenICF-csvfile-connector/pom.xml index 78a1cfb4..daece0a5 100644 --- a/OpenICF-csvfile-connector/pom.xml +++ b/OpenICF-csvfile-connector/pom.xml @@ -14,7 +14,7 @@ * own identifying information: "Portions copyright [year] [name of copyright owner]". * * Copyright 2011-2016 ForgeRock AS. - * Portions Copyrighted 2018-2024 3A Systems, LLC + * Portions Copyrighted 2018-2026 3A Systems, LLC */ --> @@ -46,10 +46,6 @@ super-csv 2.3.1 - - commons-io - commons-io - diff --git a/OpenICF-csvfile-connector/src/main/java/org/forgerock/openicf/csvfile/CSVFileConnector.java b/OpenICF-csvfile-connector/src/main/java/org/forgerock/openicf/csvfile/CSVFileConnector.java index f0a2b0a2..f41c13df 100644 --- a/OpenICF-csvfile-connector/src/main/java/org/forgerock/openicf/csvfile/CSVFileConnector.java +++ b/OpenICF-csvfile-connector/src/main/java/org/forgerock/openicf/csvfile/CSVFileConnector.java @@ -14,16 +14,21 @@ * Copyright 2015-2016 ForgeRock AS * Portions Copyright 2011 Viliam Repan * Portions Copyright 2011 Radovan Semancik + * Portions Copyright 2026 3A Systems, LLC. */ package org.forgerock.openicf.csvfile; import java.io.BufferedReader; +import java.io.Closeable; import java.io.File; import java.io.FileNotFoundException; import java.io.FileReader; import java.io.FileWriter; import java.io.IOException; +import java.nio.file.AtomicMoveNotSupportedException; import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardCopyOption; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -45,7 +50,6 @@ import java.util.regex.Pattern; import org.identityconnectors.common.Base64; -import org.apache.commons.io.FileUtils; import org.identityconnectors.common.logging.Log; import org.identityconnectors.common.security.GuardedString; import org.identityconnectors.common.security.SecurityUtil; @@ -1003,14 +1007,15 @@ private Uid doCreate(Set attributes, OperationOptions options) { } catch (IOException e) { throw new ConnectorException("Failed to create object", e); } finally { - if (mapWriter != null) { - try { + try { + if (mapWriter != null) { mapWriter.close(); - } catch (IOException e) { - log.error(e, "Failed to close CSV file after create"); } + } catch (Exception e) { + log.error(e, "Failed to close CSV file after create"); + } finally { + lock.unlock(); } - lock.unlock(); } return uid; @@ -1025,12 +1030,13 @@ private void doDelete(Uid uid, OperationOptions options) { ICsvMapReader reader = null; ICsvMapWriter writer = null; File tmp = null; + boolean rewritten = false; final WriteLock lock = fileNameToLockMap.get(csvFilePath).writeLock(); lock.lock(); try { reader = new CsvMapReader(new FileReader(config.getCsvFile()), csvPreference); - tmp = File.createTempFile("csvfile", "tmp"); + tmp = createRewriteFile(); writer = new CsvMapWriter(new FileWriter(tmp), csvPreference); final CellProcessor[] processors = getProcessors(header); @@ -1046,10 +1052,10 @@ private void doDelete(Uid uid, OperationOptions options) { } } if (!found) { - tmp.delete(); throw new UnknownUidException("Object for uid " + uid.toString() + " does not exist"); } totalRowCount.put(csvFilePath, totalRowCount.get(csvFilePath) - 1); + rewritten = true; } catch (FileNotFoundException e) { log.error(e, "File {0} does not exist!", config.getCsvFile().toString()); throw new ConnectorIOException("File " + config.getCsvFile().toString() + " does not exist", e); @@ -1057,31 +1063,13 @@ private void doDelete(Uid uid, OperationOptions options) { log.error(e, "Error reading from {0}!", config.getCsvFile().toString()); throw new ConnectorIOException("Error reading from file " + config.getCsvFile().toString(), e); } finally { - if (reader != null) { - try { - reader.close(); - } catch (Exception e) { - log.error(e, "Error closing file reader"); - } - } - if (writer != null) { - try { - writer.close(); - } catch (Exception e) { - log.error(e, "Error closing file writer"); - } - } - if (tmp != null && tmp.exists()) { - try { - if (config.getCsvFile().getAbsoluteFile().exists()) { - config.getCsvFile().getAbsoluteFile().delete(); - } - FileUtils.moveFile(tmp.getAbsoluteFile(), config.getCsvFile().getAbsoluteFile()); - } catch (Exception e) { - log.error(e, "Error renaming file"); - } + try { + closeQuietly(reader, "reader"); + closeQuietly(writer, "writer"); + finishRewrite(tmp, rewritten); + } finally { + lock.unlock(); } - lock.unlock(); } } @@ -1098,12 +1086,13 @@ private Uid doUpdate(UpdateType type, Uid uid, Set attributes, Operat ICsvMapReader reader = null; ICsvMapWriter writer = null; File tmp = null; + boolean rewritten = false; final WriteLock lock = fileNameToLockMap.get(csvFilePath).writeLock(); lock.lock(); try { reader = new CsvMapReader(new FileReader(config.getCsvFile()), csvPreference); - tmp = File.createTempFile("csvfile", "tmp"); + tmp = createRewriteFile(); writer = new CsvMapWriter(new FileWriter(tmp), csvPreference); writer.writeHeader(header); @@ -1129,6 +1118,7 @@ private Uid doUpdate(UpdateType type, Uid uid, Set attributes, Operat if (updated == null) { throw new UnknownUidException("Uid " + uid.getUidValue() + " does not exist"); } + rewritten = true; } catch (FileNotFoundException e) { log.error(e, "File {0} does not exist!", config.getCsvFile().toString()); throw new ConnectorIOException("File " + config.getCsvFile().toString() + " does not exist", e); @@ -1136,34 +1126,72 @@ private Uid doUpdate(UpdateType type, Uid uid, Set attributes, Operat log.error(e, "Error reading from {0}!", config.getCsvFile().toString()); throw new ConnectorIOException("Error reading from file " + config.getCsvFile().toString(), e); } finally { - if (reader != null) { - try { - reader.close(); - } catch (Exception e) { - log.error(e, "Error closing file reader"); - } + try { + closeQuietly(reader, "reader"); + closeQuietly(writer, "writer"); + finishRewrite(tmp, rewritten); + } finally { + lock.unlock(); } - if (writer != null) { - try { - writer.close(); - } catch (Exception e) { - log.error(e, "Error closing file writer"); - } + } + + return updated; + } + + /** + * Creates the file a rewritten copy of the CSV is assembled in: next to + * the CSV, so that the copy can take its place with a rename on the same + * file system, and readable by its owner only, so that the data is not + * exposed to other local users while it is being written. + */ + private File createRewriteFile() throws IOException { + File csv = config.getCsvFile().getAbsoluteFile(); + return Files.createTempFile(csv.getParentFile().toPath(), csv.getName() + ".", ".tmp") + .toFile(); + } + + /** + * Puts the rewritten copy in place of the CSV when the rewrite completed, + * keeping the CSV's permissions; a copy of a rewrite that failed half-way + * is discarded so that the CSV stays as it was. + */ + private void finishRewrite(File tmp, boolean rewritten) { + if (tmp == null) { + return; + } + if (!rewritten) { + if (!tmp.delete() && tmp.exists()) { + log.warn("Could not delete {0}", tmp); } - if (tmp != null) { - try { - if (config.getCsvFile().getAbsoluteFile().exists()) { - config.getCsvFile().getAbsoluteFile().delete(); - } - FileUtils.moveFile(tmp.getAbsoluteFile(), config.getCsvFile().getAbsoluteFile()); - } catch (Exception e) { - log.error(e, "Error renaming file"); - } + return; + } + Path csv = config.getCsvFile().getAbsoluteFile().toPath(); + try { + try { + Files.setPosixFilePermissions(tmp.toPath(), Files.getPosixFilePermissions(csv)); + } catch (UnsupportedOperationException e) { + // not a POSIX file system: the copy inherits the directory's ACL } - lock.unlock(); + try { + Files.move(tmp.toPath(), csv, StandardCopyOption.REPLACE_EXISTING, + StandardCopyOption.ATOMIC_MOVE); + } catch (AtomicMoveNotSupportedException e) { + Files.move(tmp.toPath(), csv, StandardCopyOption.REPLACE_EXISTING); + } + } catch (IOException e) { + throw new ConnectorIOException("Failed to replace " + csv + " with its rewritten copy " + + tmp, e); } + } - return updated; + private static void closeQuietly(Closeable closeable, String what) { + if (closeable != null) { + try { + closeable.close(); + } catch (Exception e) { + log.error(e, "Error closing file {0}", what); + } + } } private String getAttributeValue(Attribute attr) { diff --git a/OpenICF-csvfile-connector/src/test/java/org/forgerock/openicf/csvfile/RewriteSafetyTest.java b/OpenICF-csvfile-connector/src/test/java/org/forgerock/openicf/csvfile/RewriteSafetyTest.java new file mode 100644 index 00000000..51698fee --- /dev/null +++ b/OpenICF-csvfile-connector/src/test/java/org/forgerock/openicf/csvfile/RewriteSafetyTest.java @@ -0,0 +1,137 @@ +/* + * 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. + */ +package org.forgerock.openicf.csvfile; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.fail; + +import java.io.File; +import java.nio.charset.StandardCharsets; +import java.nio.file.FileSystems; +import java.nio.file.Files; +import java.nio.file.attribute.PosixFilePermission; +import java.nio.file.attribute.PosixFilePermissions; +import java.util.Collections; +import java.util.Set; + +import org.identityconnectors.framework.common.objects.Attribute; +import org.identityconnectors.framework.common.objects.AttributeBuilder; +import org.identityconnectors.framework.common.objects.ObjectClass; +import org.identityconnectors.framework.common.objects.Uid; +import org.testng.SkipException; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +/** + * Update and delete rewrite the whole CSV file through a temporary copy. + * That copy must not weaken the permissions of the data file, and a rewrite + * that fails half-way must not replace the data file with the fragment. + */ +public class RewriteSafetyTest { + + private static final String HEADER = "firstName,uid,lastName,password\n"; + private static final String VILO = "\"viliam\",\"vilo\",\"repan\",\"Z29vZA==\"\n"; + private static final String MALFORMED = "\"too\",\"few\"\n"; + private static final String MISO = "\"michal\",\"miso\",\"kovac\",\"Z29vZA==\"\n"; + + private File csv; + private CSVFileConnector connector; + + @BeforeMethod + public void before() throws Exception { + csv = File.createTempFile("rewrite", ".csv"); + write(HEADER + VILO + MISO); + + CSVFileConfiguration config = new CSVFileConfiguration(); + config.setCsvFile(csv); + config.setHeaderUid("uid"); + config.setHeaderPassword("password"); + + connector = new CSVFileConnector(); + connector.init(config); + } + + @AfterMethod + public void after() { + connector.dispose(); + connector = null; + csv.delete(); + } + + @Test + public void updateKeepsFilePermissions() throws Exception { + Set ownerOnly = restrictToOwner(); + + connector.update(ObjectClass.ACCOUNT, new Uid("vilo"), lastName("updated"), null); + + assertEquals(Files.getPosixFilePermissions(csv.toPath()), ownerOnly); + } + + @Test + public void deleteKeepsFilePermissions() throws Exception { + Set ownerOnly = restrictToOwner(); + + connector.delete(ObjectClass.ACCOUNT, new Uid("vilo"), null); + + assertEquals(Files.getPosixFilePermissions(csv.toPath()), ownerOnly); + } + + @Test + public void failedUpdateLeavesFileUntouched() throws Exception { + String original = HEADER + VILO + MALFORMED + MISO; + write(original); + try { + connector.update(ObjectClass.ACCOUNT, new Uid("vilo"), lastName("updated"), null); + fail("Expected the malformed row to fail the update"); + } catch (RuntimeException expected) { + assertEquals(read(), original, "the data file was replaced by the partial rewrite"); + } + } + + @Test + public void failedDeleteLeavesFileUntouched() throws Exception { + String original = HEADER + VILO + MALFORMED + MISO; + write(original); + try { + connector.delete(ObjectClass.ACCOUNT, new Uid("vilo"), null); + fail("Expected the malformed row to fail the delete"); + } catch (RuntimeException expected) { + assertEquals(read(), original, "the data file was replaced by the partial rewrite"); + } + } + + private Set restrictToOwner() throws Exception { + if (!FileSystems.getDefault().supportedFileAttributeViews().contains("posix")) { + throw new SkipException("POSIX file permissions are not supported here"); + } + Set ownerOnly = PosixFilePermissions.fromString("rw-------"); + Files.setPosixFilePermissions(csv.toPath(), ownerOnly); + return ownerOnly; + } + + private static Set lastName(String value) { + return Collections.singleton(AttributeBuilder.build("lastName", value)); + } + + private void write(String content) throws Exception { + Files.write(csv.toPath(), content.getBytes(StandardCharsets.UTF_8)); + } + + private String read() throws Exception { + return new String(Files.readAllBytes(csv.toPath()), StandardCharsets.UTF_8); + } +} diff --git a/OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/local/LocalConnectorInfoManagerImpl.java b/OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/local/LocalConnectorInfoManagerImpl.java index 445606b0..ab0bd34b 100644 --- a/OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/local/LocalConnectorInfoManagerImpl.java +++ b/OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/local/LocalConnectorInfoManagerImpl.java @@ -21,6 +21,7 @@ * ==================== * Portions Copyrighted 2010-2015 ForgeRock AS. * Portions Copyrighted 2010-2014 Tirasa. + * Portions Copyrighted 2026 3A Systems, LLC */ package org.identityconnectors.framework.impl.api.local; @@ -32,6 +33,7 @@ import java.lang.annotation.Annotation; import java.net.URISyntaxException; import java.net.URL; +import java.nio.file.Files; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -492,13 +494,9 @@ private File getBundleTempDir() throws IOException { if (!tempDir.canWrite()) { throw new IOException("Temporary directory " + tempDir + " is read/only"); } - File candidate; - do { - candidate = new File(tempDir, "bundle-" + nextRandom()); - } while (candidate.exists()); - if (!candidate.mkdir()) { - throw new IOException("Temporary directory " + tempDir + " is read/only"); - } + // created with a unique name and, on POSIX file systems, readable + // by the owner only: it holds the bundle's libraries + final File candidate = Files.createTempDirectory(tempDir.toPath(), "bundle-").toFile(); candidate.deleteOnExit(); _bundleTempDir = candidate; return candidate; diff --git a/OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/LocalConnectorInfoManagerTests.java b/OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/LocalConnectorInfoManagerTests.java index 881c07d2..2e7c918b 100644 --- a/OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/LocalConnectorInfoManagerTests.java +++ b/OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/LocalConnectorInfoManagerTests.java @@ -19,13 +19,19 @@ * enclosed by brackets [] replaced by your own identifying information: * "Portions Copyrighted [year] [name of copyright owner]" * ==================== + * Portions Copyrighted 2026 3A Systems, LLC */ package org.identityconnectors.framework.impl.api; +import java.io.File; import java.net.URL; +import java.nio.file.FileSystems; +import java.nio.file.Files; +import java.nio.file.attribute.PosixFilePermissions; import java.util.ArrayList; import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; import org.identityconnectors.common.Version; @@ -49,7 +55,9 @@ import org.identityconnectors.framework.common.objects.Uid; import org.identityconnectors.framework.api.operations.batch.BatchBuilder; import org.identityconnectors.framework.api.operations.batch.BatchTask; +import org.identityconnectors.framework.impl.api.local.LocalConnectorInfoManagerImpl; import org.testng.Assert; +import org.testng.SkipException; import org.testng.annotations.Test; import static org.testng.Assert.assertEquals; @@ -86,6 +94,44 @@ public void testCheckVersion() throws Exception { } } + /** + * The directory a bundle is expanded into (its lib/ and native/ entries) + * lives in java.io.tmpdir and must not be readable by other local users. + */ + @Test + public void testBundleTempDirectoryIsPrivate() throws Exception { + if (!FileSystems.getDefault().supportedFileAttributeViews().contains("posix")) { + throw new SkipException("POSIX file permissions are not supported here"); + } + File tmpDir = new File(System.getProperty("java.io.tmpdir")); + Set before = bundleTempDirs(tmpDir); + + // a manager of its own: the cached one has expanded the bundles already + new LocalConnectorInfoManagerImpl(getTestBundles(), ConnectorInfoManagerFactory.class + .getClassLoader()); + + Set created = bundleTempDirs(tmpDir); + created.removeAll(before); + assertFalse(created.isEmpty(), "no bundle temp directory was created in " + tmpDir); + for (String name : created) { + assertEquals(Files.getPosixFilePermissions(new File(tmpDir, name).toPath()), + PosixFilePermissions.fromString("rwx------"), name); + } + } + + private static Set bundleTempDirs(File tmpDir) { + Set names = new HashSet(); + String[] entries = tmpDir.list(); + if (entries != null) { + for (String name : entries) { + if (name.startsWith("bundle-") && new File(tmpDir, name).isDirectory()) { + names.add(name); + } + } + } + return names; + } + /** * To be overridden by subclasses to get different ConnectorInfoManagers *