Skip to content

Rewrite the CSV file through a private copy next to it and never replace it with a fragment #131

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vharseko wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Rewrite the CSV file through a private copy next to it and never replace it with a fragment #131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions OpenICF-csvfile-connector/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
-->
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
Expand Down Expand Up @@ -46,10 +46,6 @@
<artifactId>super-csv</artifactId>
<version>2.3.1</version>
</dependency>
<dependency>
<groupId>commons-io</groupId>
<artifactId>commons-io</artifactId>
</dependency>

<!-- Test Dependencies -->
<dependency>
Expand Down
140 changes: 84 additions & 56 deletions OpenICF-csvfile-connector/src/main/java/org/forgerock/openicf/csvfile/CSVFileConnector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -1003,14 +1007,15 @@ private Uid doCreate(Set<Attribute> 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;
Expand All @@ -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);
Expand All @@ -1046,42 +1052,24 @@ 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);
} catch (IOException e) {
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();
}
}

Expand All @@ -1098,12 +1086,13 @@ private Uid doUpdate(UpdateType type, Uid uid, Set<Attribute> 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);
Expand All @@ -1129,41 +1118,80 @@ private Uid doUpdate(UpdateType type, Uid uid, Set<Attribute> 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);
} catch (IOException e) {
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) {
Expand Down
137 changes: 137 additions & 0 deletions OpenICF-csvfile-connector/src/test/java/org/forgerock/openicf/csvfile/RewriteSafetyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<PosixFilePermission> 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<PosixFilePermission> 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<PosixFilePermission> restrictToOwner() throws Exception {
if (!FileSystems.getDefault().supportedFileAttributeViews().contains("posix")) {
throw new SkipException("POSIX file permissions are not supported here");
}
Set<PosixFilePermission> ownerOnly = PosixFilePermissions.fromString("rw-------");
Files.setPosixFilePermissions(csv.toPath(), ownerOnly);
return ownerOnly;
}

private static Set<Attribute> 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);
}
}
Loading
Loading