fleetd #224 + #225: make worktreeRoot group-traversable, and stop two tests depending on the checkout location
Verified by the lead before merge: read the full production diff, confirmed `group` is normalised to null at GitWorktrees:148 so the `group == null` guard is complete, and confirmed the refusal runs before `git worktree add` so a failure leaves no half-made worktree. Independent build in the worker's worktree: BUILD SUCCESS, 1093 tests, 0 failures, 0 skipped.
This commit was merged in pull request #230.
This commit is contained in:
@@ -13,6 +13,7 @@ import java.nio.charset.StandardCharsets;
|
||||
import java.nio.file.Files;
|
||||
import java.nio.file.Path;
|
||||
import java.nio.file.StandardCopyOption;
|
||||
import java.nio.file.attribute.PosixFilePermissions;
|
||||
import java.security.SecureRandom;
|
||||
import java.util.ArrayList;
|
||||
import java.util.HashSet;
|
||||
@@ -161,6 +162,7 @@ public final class GitWorktrees implements Worktrees {
|
||||
} catch (IOException e) {
|
||||
throw new WorktreeException("cannot create worktree root " + root + ": " + e.getMessage(), e);
|
||||
}
|
||||
shareRootWithGroup(root);
|
||||
String wt = path.toAbsolutePath().toString();
|
||||
log.info("adding worktree branch={} path={} base={}", branch, wt, base);
|
||||
removeUserInfoFromHttpsOrigin(repoRoot);
|
||||
@@ -712,6 +714,54 @@ public final class GitWorktrees implements Worktrees {
|
||||
group, repoRoot, worktreePath, touched);
|
||||
}
|
||||
|
||||
/**
|
||||
* fleetd #224: make {@code worktreeRoot} ITSELF group-traversable — established once, here,
|
||||
* where the root is created, never at a use site. {@link #shareWithGroup} shares each worktree
|
||||
* (and the repo's common git dir) with {@link #group}, but never the PARENT directory that
|
||||
* contains every worktree — and under {@code memberHerdrSocket:} the member pane runs as a
|
||||
* different OS user, which needs the execute bit on every ancestor directory to reach anything
|
||||
* underneath, no matter how carefully each child is shared. Without this, a member cannot read
|
||||
* the ephemeral {@code opencode.json} #219 places under this root, cannot reach its own
|
||||
* worktree, and cannot read #213's ZDOTDIR scrub when placed here either.
|
||||
*
|
||||
* <p>No-op — no process spawned — when {@link #group} is null/blank, so behaviour with
|
||||
* {@code worktreeGroup:} unset (today's only live mode) is unchanged. Only {@code root} itself
|
||||
* is touched (non-recursive): each child underneath is shared individually, either by
|
||||
* {@link #shareWithGroup} for a worktree or by the launcher that generates it (fleetd #213/#219)
|
||||
* for a scrub/config directory — sharing this level again would just duplicate that policy in
|
||||
* the wrong layer.
|
||||
*
|
||||
* <p>Fails loudly, naming {@code root}, its mode at the time of the attempt, and {@link #group}:
|
||||
* a member that starts and then cannot see its own checkout is worse than a refused spawn, since
|
||||
* nothing about that failure mode points at a directory's permission bits.
|
||||
*/
|
||||
private void shareRootWithGroup(Path root) {
|
||||
if (group == null) {
|
||||
return;
|
||||
}
|
||||
String mode = currentPosixMode(root);
|
||||
try {
|
||||
shareGroupRunner.apply(new String[]{"chgrp", group, root.toString()});
|
||||
shareGroupRunner.apply(new String[]{"chmod", "g+x", root.toString()});
|
||||
} catch (WorktreeException e) {
|
||||
throw new WorktreeException("cannot make worktree root " + root + " (mode " + mode
|
||||
+ ") group-traversable for group '" + group + "': " + e.getMessage()
|
||||
+ " — the group must exist, and the fleetd operator (" + System.getProperty("user.name")
|
||||
+ ") must be a member of it", e);
|
||||
}
|
||||
log.info("worktreeGroup={} made worktree root {} group-traversable (was mode {})", group, root, mode);
|
||||
}
|
||||
|
||||
/** {@code root}'s current POSIX permission string, or {@code "unknown"} on a filesystem that does
|
||||
* not support POSIX permissions — used only to name the mode in a refusal message. */
|
||||
private static String currentPosixMode(Path root) {
|
||||
try {
|
||||
return PosixFilePermissions.toString(Files.getPosixFilePermissions(root));
|
||||
} catch (IOException | UnsupportedOperationException e) {
|
||||
return "unknown";
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The repo's <em>common</em> git directory as an absolute path — where {@code objects},
|
||||
* {@code refs} and {@code worktrees} actually live. {@code git rev-parse --git-common-dir}
|
||||
|
||||
@@ -114,6 +114,68 @@ class EnvAllowListScrubTest {
|
||||
assertNull(EnvAllowListScrub.readReport(dir));
|
||||
}
|
||||
|
||||
/**
|
||||
* fleetd #224 criterion 5 (a gap the #221 reviewer flagged): {@link EnvAllowListScrub#shareWithGroup}
|
||||
* lists one FLAT level of {@code dir} and shares every file it finds there — which does cover the
|
||||
* OPTIONAL files a caller may or may not have written before calling it ({@code member-charter.md},
|
||||
* {@code ide-rules.md} — both written by {@code OpenCodeLauncher#writeConfig}), but nothing
|
||||
* pinned that down.
|
||||
* Without this test, a future change that writes a file AFTER the sharing call, or into a
|
||||
* subdirectory, would pass every existing test while quietly leaving that file unreadable to a
|
||||
* different-uid member.
|
||||
*
|
||||
* <p>This drives {@code shareWithGroup} directly against a directory holding several flat files —
|
||||
* not only {@code opencode.json}, but also the two optional ones named above plus a third,
|
||||
* unrelated file, so the assertion is "every flat file", not "the two files someone thought of".
|
||||
*/
|
||||
@Test
|
||||
void shareWithGroupCoversEveryFlatFileIncludingTheOptionalOnes(@TempDir Path dir) throws Exception {
|
||||
String group = currentUserGroup();
|
||||
Files.writeString(dir.resolve("opencode.json"), "{}\n");
|
||||
Files.writeString(dir.resolve("member-charter.md"), "# charter\n");
|
||||
Files.writeString(dir.resolve("ide-rules.md"), "# ide rules\n");
|
||||
Files.writeString(dir.resolve("another-flat-file.txt"), "unrelated\n");
|
||||
|
||||
EnvAllowListScrub.shareWithGroup(dir, group);
|
||||
|
||||
assertEquals("rwxr-x---", java.nio.file.attribute.PosixFilePermissions.toString(
|
||||
Files.getPosixFilePermissions(dir)),
|
||||
"the directory itself must be group-traversable+readable, owner-only writable");
|
||||
for (String name : List.of("opencode.json", "member-charter.md", "ide-rules.md", "another-flat-file.txt")) {
|
||||
Path file = dir.resolve(name);
|
||||
assertEquals("rw-r-----", java.nio.file.attribute.PosixFilePermissions.toString(
|
||||
Files.getPosixFilePermissions(file)),
|
||||
name + " must be group-readable, never group-writable");
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The current process's REAL primary group — resolved via {@code id -gn}, never by reading a
|
||||
* directory's owning group (fleetd #225: that reads wherever Maven happened to be started from,
|
||||
* not the process's own group, and the two diverge outside a home checkout). Skips (never fails)
|
||||
* when {@code id} is unavailable or its primary group cannot be resolved on this host.
|
||||
*/
|
||||
private static String currentUserGroup() {
|
||||
String out;
|
||||
boolean ok;
|
||||
try {
|
||||
Process p = new ProcessBuilder("id", "-gn").redirectErrorStream(true).start();
|
||||
String raw = new String(p.getInputStream().readAllBytes(), StandardCharsets.UTF_8).trim();
|
||||
out = raw;
|
||||
ok = p.waitFor(5, java.util.concurrent.TimeUnit.SECONDS) && p.exitValue() == 0 && !raw.isBlank();
|
||||
} catch (IOException e) {
|
||||
out = null;
|
||||
ok = false;
|
||||
} catch (InterruptedException e) {
|
||||
Thread.currentThread().interrupt();
|
||||
out = null;
|
||||
ok = false;
|
||||
}
|
||||
assumeTrue(ok, "cannot resolve this process's real primary group via `id -gn` on this host "
|
||||
+ "— skipping a POSIX-group-dependent test rather than failing it");
|
||||
return out;
|
||||
}
|
||||
|
||||
/**
|
||||
* The same equality, for a shell that is INTERACTIVE but NOT a login shell — the shape herdr
|
||||
* opens on Linux.
|
||||
|
||||
+31
-6
@@ -18,7 +18,6 @@ import org.slf4j.LoggerFactory;
|
||||
import java.io.IOException;
|
||||
import java.nio.file.Files;
|
||||
import java.nio.file.Path;
|
||||
import java.nio.file.attribute.PosixFileAttributeView;
|
||||
import java.util.HashMap;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
@@ -513,11 +512,37 @@ class HerdrPeerLauncherAllowListWiringTest {
|
||||
+ "java.io.tmpdir, unchanged from before this fix: " + dir);
|
||||
}
|
||||
|
||||
/** The current process's own primary group — resolvable on whatever host runs this test. */
|
||||
private static String currentUserGroup() throws IOException {
|
||||
PosixFileAttributeView view = Files.getFileAttributeView(Path.of("."), PosixFileAttributeView.class);
|
||||
assumeTrue(view != null, "this host's filesystem does not support POSIX group ownership");
|
||||
return view.readAttributes().group().getName();
|
||||
/**
|
||||
* The current process's REAL primary group — resolved via {@code id -gn}, never by reading a
|
||||
* directory's owning group (fleetd #225). Those two coincide only by accident: a directory's
|
||||
* group is whichever group happened to own the path Maven was started from — {@code staff} in
|
||||
* a home checkout, {@code wheel} under {@code /private/tmp} on macOS — and the fix-up this test
|
||||
* exercises then fails for real when the operator is not a member of that borrowed group,
|
||||
* exactly the case {@code assumeTrue(view != null, ...)} never covered (it only detects a
|
||||
* filesystem with no POSIX groups at all, not a resolvable-but-wrong one). Skips (never fails)
|
||||
* when {@code id} is unavailable or its primary group cannot be resolved on this host.
|
||||
*/
|
||||
private static String currentUserGroup() {
|
||||
String out;
|
||||
boolean ok;
|
||||
try {
|
||||
Process p = new ProcessBuilder("id", "-gn").redirectErrorStream(true).start();
|
||||
try (java.io.BufferedReader r = new java.io.BufferedReader(
|
||||
new java.io.InputStreamReader(p.getInputStream(), java.nio.charset.StandardCharsets.UTF_8))) {
|
||||
out = r.lines().collect(java.util.stream.Collectors.joining("\n")).trim();
|
||||
}
|
||||
ok = p.waitFor(5, java.util.concurrent.TimeUnit.SECONDS) && p.exitValue() == 0 && !out.isBlank();
|
||||
} catch (IOException e) {
|
||||
out = null;
|
||||
ok = false;
|
||||
} catch (InterruptedException e) {
|
||||
Thread.currentThread().interrupt();
|
||||
out = null;
|
||||
ok = false;
|
||||
}
|
||||
assumeTrue(ok, "cannot resolve this process's real primary group via `id -gn` on this host "
|
||||
+ "— skipping a POSIX-group-dependent test rather than failing it");
|
||||
return out;
|
||||
}
|
||||
|
||||
/** Spawn once through the real launcher path, capturing every INFO+ line this class logs. */
|
||||
|
||||
@@ -23,7 +23,6 @@ import org.slf4j.LoggerFactory;
|
||||
import java.io.IOException;
|
||||
import java.nio.file.Files;
|
||||
import java.nio.file.Path;
|
||||
import java.nio.file.attribute.PosixFileAttributeView;
|
||||
import java.nio.file.attribute.PosixFilePermissions;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
@@ -657,11 +656,37 @@ class OpenCodeLauncherTest {
|
||||
0, System::currentTimeMillis, () -> { }, configRoot, discoveryRoot, null, null, config);
|
||||
}
|
||||
|
||||
/** The current process's own primary group — resolvable on whatever host runs this test. */
|
||||
private static String currentUserGroup() throws IOException {
|
||||
PosixFileAttributeView view = Files.getFileAttributeView(Path.of("."), PosixFileAttributeView.class);
|
||||
assumeTrue(view != null, "this host's filesystem does not support POSIX group ownership");
|
||||
return view.readAttributes().group().getName();
|
||||
/**
|
||||
* The current process's REAL primary group — resolved via {@code id -gn}, never by reading a
|
||||
* directory's owning group (fleetd #225). Those two coincide only by accident: a directory's
|
||||
* group is whichever group happened to own the path Maven was started from — {@code staff} in
|
||||
* a home checkout, {@code wheel} under {@code /private/tmp} on macOS — and the fix-up this test
|
||||
* exercises then fails for real when the operator is not a member of that borrowed group,
|
||||
* exactly the case {@code assumeTrue(view != null, ...)} never covered (it only detects a
|
||||
* filesystem with no POSIX groups at all, not a resolvable-but-wrong one). Skips (never fails)
|
||||
* when {@code id} is unavailable or its primary group cannot be resolved on this host.
|
||||
*/
|
||||
private static String currentUserGroup() {
|
||||
String out;
|
||||
boolean ok;
|
||||
try {
|
||||
Process p = new ProcessBuilder("id", "-gn").redirectErrorStream(true).start();
|
||||
try (java.io.BufferedReader r = new java.io.BufferedReader(
|
||||
new java.io.InputStreamReader(p.getInputStream(), java.nio.charset.StandardCharsets.UTF_8))) {
|
||||
out = r.lines().collect(java.util.stream.Collectors.joining("\n")).trim();
|
||||
}
|
||||
ok = p.waitFor(5, java.util.concurrent.TimeUnit.SECONDS) && p.exitValue() == 0 && !out.isBlank();
|
||||
} catch (IOException e) {
|
||||
out = null;
|
||||
ok = false;
|
||||
} catch (InterruptedException e) {
|
||||
Thread.currentThread().interrupt();
|
||||
out = null;
|
||||
ok = false;
|
||||
}
|
||||
assumeTrue(ok, "cannot resolve this process's real primary group via `id -gn` on this host "
|
||||
+ "— skipping a POSIX-group-dependent test rather than failing it");
|
||||
return out;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -1099,4 +1099,82 @@ class GitWorktreesTest {
|
||||
assertTrue(e.getMessage().contains("cb185-nonexistent-group-zz"),
|
||||
"exception must name the missing/refused group: " + e.getMessage());
|
||||
}
|
||||
|
||||
// --- fleetd #224: worktreeRoot itself must be group-traversable, established in add() ---------
|
||||
|
||||
/**
|
||||
* {@code add} must make the worktree ROOT itself group-traversable when a group is configured —
|
||||
* established once here, where the root is created, never at a use site (never inside a
|
||||
* launcher). A recording runner stands in for chgrp/chmod, the same seam
|
||||
* {@link #shareWithGroupRunsConfigThenChgrpChmodSetgidPerPath} uses for the per-worktree share.
|
||||
*/
|
||||
@Test
|
||||
void addSharesWorktreeRootWithGroupWhenConfigured(@TempDir Path tmp) throws Exception {
|
||||
Path repo = initRepo(tmp.resolve("repo"));
|
||||
Path root = tmp.resolve("wts");
|
||||
List<List<String>> recorded = new java.util.ArrayList<>();
|
||||
java.util.function.Function<String[], String> recordingRunner = cmd -> {
|
||||
recorded.add(joined(cmd));
|
||||
return "";
|
||||
};
|
||||
GitWorktrees gitWorktrees = new GitWorktrees(root.toString(), "devteam", _ -> {}, recordingRunner);
|
||||
|
||||
gitWorktrees.add(repo.toString(), "cb-224-branch", "HEAD");
|
||||
|
||||
assertTrue(recorded.contains(List.of("chgrp", "devteam", root.toString())),
|
||||
"the worktree root itself must be chgrp'd to the configured group: " + recorded);
|
||||
assertTrue(recorded.contains(List.of("chmod", "g+x", root.toString())),
|
||||
"the worktree root itself must gain group-execute so a different-uid member can "
|
||||
+ "traverse into it: " + recorded);
|
||||
}
|
||||
|
||||
/** {@code worktreeGroup} unset (today's only live mode) ⇒ {@code add} spawns no share process
|
||||
* for the root at all — behaviour must be byte-identical to before fleetd #224. */
|
||||
@Test
|
||||
void addSharesNothingForTheRootWhenNoGroupConfigured(@TempDir Path tmp) throws Exception {
|
||||
Path repo = initRepo(tmp.resolve("repo"));
|
||||
Path root = tmp.resolve("wts");
|
||||
List<List<String>> recorded = new java.util.ArrayList<>();
|
||||
java.util.function.Function<String[], String> recordingRunner = cmd -> {
|
||||
recorded.add(joined(cmd));
|
||||
return "";
|
||||
};
|
||||
GitWorktrees gitWorktrees = new GitWorktrees(root.toString(), null, _ -> {}, recordingRunner);
|
||||
|
||||
gitWorktrees.add(repo.toString(), "cb-224-nogroup", "HEAD");
|
||||
|
||||
assertTrue(recorded.isEmpty(), "no group configured must spawn no share process for the "
|
||||
+ "root at all: " + recorded);
|
||||
}
|
||||
|
||||
/**
|
||||
* fleetd #224, acceptance criterion 2/3: when the root cannot be made group-traversable — here
|
||||
* because the configured group does not exist, the same real-failure shape
|
||||
* {@link #shareWithGroupThrowsNamingTheGroupWhenChgrpFails} drives for the per-worktree share —
|
||||
* the spawn is refused with a message naming the root, its current mode, and the group. This
|
||||
* drives the REAL {@code chgrp} (no recording runner), and the refusal happens before {@code git
|
||||
* worktree add} ever runs, so no partial worktree is left behind either.
|
||||
*/
|
||||
@Test
|
||||
void addRefusesWhenWorktreeRootCannotBeMadeGroupTraversable(@TempDir Path tmp) throws Exception {
|
||||
Path repo = initRepo(tmp.resolve("repo"));
|
||||
Path root = tmp.resolve("wts");
|
||||
GitWorktrees gitWorktrees = new GitWorktrees(root.toString(), "cb224-nonexistent-group-zz");
|
||||
|
||||
WorktreeException e = assertThrows(WorktreeException.class,
|
||||
() -> gitWorktrees.add(repo.toString(), "cb-224-refuse", "HEAD"));
|
||||
|
||||
assertTrue(e.getMessage().contains(root.toString()),
|
||||
"refusal must name the worktree root: " + e.getMessage());
|
||||
assertTrue(e.getMessage().contains("cb224-nonexistent-group-zz"),
|
||||
"refusal must name the missing/refused group: " + e.getMessage());
|
||||
assertTrue(Files.isDirectory(root), "the root is created before the group check runs");
|
||||
String mode = java.nio.file.attribute.PosixFilePermissions.toString(Files.getPosixFilePermissions(root));
|
||||
assertTrue(e.getMessage().contains(mode),
|
||||
"refusal must name the root's current mode (" + mode + "): " + e.getMessage());
|
||||
try (java.util.stream.Stream<Path> children = Files.list(root)) {
|
||||
assertTrue(children.findAny().isEmpty(), "no worktree must be left behind under the root: "
|
||||
+ "the refusal must happen before `git worktree add` ever runs");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user