Skip to content

Consolidate path setting files entitlements to config #123649

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

Merged
merged 4 commits into from
Feb 28, 2025
Merged

Consolidate path setting files entitlements to config #123649

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a45fd61
Consolidate path setting files entitlements to config
rjernst Feb 27, 2025
1a70ebd
iter
rjernst Feb 28, 2025
20c7192
[CI] Auto commit changes from spotless
Feb 28, 2025
6bfcd7d
test
rjernst Feb 28, 2025
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
87 changes: 22 additions & 65 deletions ...main/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,12 +91,8 @@ static FileData ofRelativePath(Path relativePath, BaseDir baseDir, Mode mode) {
return new RelativePathFileData(relativePath, baseDir, mode, null, false);
}

static FileData ofPathSetting(String setting, Mode mode, boolean ignoreUrl) {
return new PathSettingFileData(setting, mode, ignoreUrl, null, false);
}

static FileData ofRelativePathSetting(String setting, BaseDir baseDir, Mode mode, boolean ignoreUrl) {
return new RelativePathSettingFileData(setting, baseDir, mode, ignoreUrl, null, false);
static FileData ofConfigPathSetting(String setting, Mode mode, boolean ignoreUrl) {
return new ConfigPathSettingFileData(setting, mode, ignoreUrl, null, false);
}

/**
Expand Down Expand Up @@ -225,71 +221,39 @@ public FileData withPlatform(Platform platform) {
}
}

private record PathSettingFileData(String setting, Mode mode, boolean ignoreUrl, Platform platform, boolean exclusive)
private record ConfigPathSettingFileData(String setting, Mode mode, boolean ignoreUrl, Platform platform, boolean exclusive)
implements
FileData {

@Override
public PathSettingFileData withExclusive(boolean exclusive) {
return new PathSettingFileData(setting, mode, ignoreUrl, platform, exclusive);
public ConfigPathSettingFileData withExclusive(boolean exclusive) {
return new ConfigPathSettingFileData(setting, mode, ignoreUrl, platform, exclusive);
}

@Override
public Stream<Path> resolvePaths(PathLookup pathLookup) {
return resolvePathSettings(pathLookup, setting, ignoreUrl);
}

@Override
public FileData withPlatform(Platform platform) {
if (platform == platform()) {
return this;
Stream<String> result;
if (setting.contains("*")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering why we need this asterisk check. Does the settingGlobResolver path do the wrong thing if the setting contains no asterisk?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the setting glob resolver only knows how to deal with glob.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But a glob with no asterisks is still a glob right? Even if it's trivial.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just a way to understand if we want to call the "regular" Settings resolved (which maps to Settings#get) or the glob resolver (which maps to Settings#getGlobValues). The latter throws if you don't pass down something that contains a ".*."
Btw I think we should match that here, and look for ".*.", not just *.

result = pathLookup.settingGlobResolver().apply(setting);
} else {
String path = pathLookup.settingResolver().apply(setting);
result = path == null ? Stream.of() : Stream.of(path);
}
return new PathSettingFileData(setting, mode, ignoreUrl, platform, exclusive);
}
}

private record RelativePathSettingFileData(
String setting,
BaseDir baseDir,
Mode mode,
boolean ignoreUrl,
Platform platform,
boolean exclusive
) implements FileData, RelativeFileData {

@Override
public RelativePathSettingFileData withExclusive(boolean exclusive) {
return new RelativePathSettingFileData(setting, baseDir, mode, ignoreUrl, platform, exclusive);
}

@Override
public Stream<Path> resolveRelativePaths(PathLookup pathLookup) {
return resolvePathSettings(pathLookup, setting, ignoreUrl);
if (ignoreUrl) {
result = result.filter(s -> s.toLowerCase(Locale.ROOT).startsWith("https://") == false);
}
return result.map(pathLookup.configDir()::resolve);
}

@Override
public FileData withPlatform(Platform platform) {
if (platform == platform()) {
return this;
}
return new RelativePathSettingFileData(setting, baseDir, mode, ignoreUrl, platform, exclusive);
return new ConfigPathSettingFileData(setting, mode, ignoreUrl, platform, exclusive);
}
}

private static Stream<Path> resolvePathSettings(PathLookup pathLookup, String setting, boolean ignoreUrl) {
Stream<String> result;
if (setting.contains("*")) {
result = pathLookup.settingGlobResolver().apply(setting);
} else {
String path = pathLookup.settingResolver().apply(setting);
result = path == null ? Stream.of() : Stream.of(path);
}
if (ignoreUrl) {
result = result.filter(s -> s.toLowerCase(Locale.ROOT).startsWith("https://") == false);
}
return result.map(Path::of);
}

private static Mode parseMode(String mode) {
if (mode.equals("read")) {
return Mode.READ;
Expand Down Expand Up @@ -370,8 +334,7 @@ public static FilesEntitlement build(List<Object> paths) {
String pathAsString = checkString.apply(file, "path");
String relativePathAsString = checkString.apply(file, "relative_path");
String relativeTo = checkString.apply(file, "relative_to");
String pathSetting = checkString.apply(file, "path_setting");
String relativePathSetting = checkString.apply(file, "relative_path_setting");
String configPathSetting = checkString.apply(file, "config_path_setting");
String modeAsString = checkString.apply(file, "mode");
String platformAsString = checkString.apply(file, "platform");
Boolean ignoreUrlAsStringBoolean = checkBoolean.apply(file, "ignore_url");
Expand All @@ -382,11 +345,10 @@ public static FilesEntitlement build(List<Object> paths) {
if (file.isEmpty() == false) {
throw new PolicyValidationException("unknown key(s) [" + file + "] in a listed file for files entitlement");
}
int foundKeys = (pathAsString != null ? 1 : 0) + (relativePathAsString != null ? 1 : 0) + (pathSetting != null ? 1 : 0)
+ (relativePathSetting != null ? 1 : 0);
int foundKeys = (pathAsString != null ? 1 : 0) + (relativePathAsString != null ? 1 : 0) + (configPathSetting != null ? 1 : 0);
if (foundKeys != 1) {
throw new PolicyValidationException(
"a files entitlement entry must contain one of " + "[path, relative_path, path_setting, relative_path_setting]"
"a files entitlement entry must contain one of " + "[path, relative_path, config_path_setting]"
);
}

Expand All @@ -405,7 +367,7 @@ public static FilesEntitlement build(List<Object> paths) {
}

if (ignoreUrlAsStringBoolean != null && (relativePathAsString != null || pathAsString != null)) {
throw new PolicyValidationException("'ignore_url' may only be used with `path_setting` or `relative_path_setting`");
throw new PolicyValidationException("'ignore_url' may only be used with 'config_path_setting'");
}

final FileData fileData;
Expand All @@ -424,13 +386,8 @@ public static FilesEntitlement build(List<Object> paths) {
throw new PolicyValidationException("'path' [" + pathAsString + "] must be absolute");
}
fileData = FileData.ofPath(path, mode);
} else if (pathSetting != null) {
fileData = FileData.ofPathSetting(pathSetting, mode, ignoreUrlAsString);
} else if (relativePathSetting != null) {
if (baseDir == null) {
throw new PolicyValidationException("files entitlement with a 'relative_path_setting' must specify 'relative_to'");
}
fileData = FileData.ofRelativePathSetting(relativePathSetting, baseDir, mode, ignoreUrlAsString);
} else if (configPathSetting != null) {
fileData = FileData.ofConfigPathSetting(configPathSetting, mode, ignoreUrlAsString);
} else {
throw new AssertionError("File entry validation error");
}
Expand Down
8 changes: 2 additions & 6 deletions ...tlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/PolicyParserTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,10 +182,7 @@ public void testParseFiles() throws IOException {
mode: "read"
- path: '%s'
mode: "read_write"
- path_setting: foo.bar
mode: read
- relative_path_setting: foo.bar
relative_to: config
- config_path_setting: foo.bar
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. This is the first entitlement whose meaning is not immediately obvious to me. It could be particularly confusing if the path has nothing to do with the config dir (ie. if it's an absolute path). path_setting made sense to me, because it's a setting representing a path.

Anyway I don't feel strongly about it. If you think plugin authors will understand this, that's what matters. And assuming we want all three words "config", "path", and "setting" in the name, I think you've picked the best possible order. 😂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I find it a bit awkward too. I also tend to think path_setting makes sense but it doesn't convey the implicit handling paths relative to config. But, to be honest, I'm not sure prefixing config to the name makes things much clearer.

This also keeps the door open to introducing other "path setting" entitlements in the future that have slightly different semantics.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure there's a need to convey that relative paths relate to config in the name of this field. That could be just how path_setting works, and mentioned in documentation. The first time someone says "huh, a relative path; I wonder what it's relative to?" the first place they'd look would be the docs for path_setting.

mode: read
""", relativePathToFile, relativePathToDir, TEST_ABSOLUTE_PATH_TO_FILE).getBytes(StandardCharsets.UTF_8)),
"test-policy.yaml",
Expand All @@ -202,8 +199,7 @@ public void testParseFiles() throws IOException {
Map.of("relative_path", relativePathToFile, "mode", "read_write", "relative_to", "data"),
Map.of("relative_path", relativePathToDir, "mode", "read", "relative_to", "config"),
Map.of("path", TEST_ABSOLUTE_PATH_TO_FILE, "mode", "read_write"),
Map.of("path_setting", "foo.bar", "mode", "read"),
Map.of("relative_path_setting", "foo.bar", "relative_to", "config", "mode", "read")
Map.of("config_path_setting", "foo.bar", "mode", "read")
)
)
)
Expand Down
67 changes: 32 additions & 35 deletions ...java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlementTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import java.util.List;
import java.util.Map;

import static org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement.BaseDir.CONFIG;
import static org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement.Mode.READ;
import static org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement.Mode.READ_WRITE;
import static org.hamcrest.Matchers.contains;
Expand Down Expand Up @@ -98,72 +97,70 @@ public void testFileDataRelativeWithEmptyDirectory() {
}

public void testPathSettingResolve() {
var entitlement = FilesEntitlement.build(List.of(Map.of("path_setting", "foo.bar", "mode", "read")));
var entitlement = FilesEntitlement.build(List.of(Map.of("config_path_setting", "foo.bar", "mode", "read")));
var filesData = entitlement.filesData();
assertThat(filesData, contains(FileData.ofPathSetting("foo.bar", READ, false)));
assertThat(filesData, contains(FileData.ofConfigPathSetting("foo.bar", READ, false)));

var fileData = FileData.ofPathSetting("foo.bar", READ, false);
var fileData = FileData.ofConfigPathSetting("foo.bar", READ, false);
// empty settings
assertThat(fileData.resolvePaths(TEST_PATH_LOOKUP).toList(), empty());

fileData = FileData.ofPathSetting("foo.bar", READ, false);
fileData = FileData.ofConfigPathSetting("foo.bar", READ, false);
settings = Settings.builder().put("foo.bar", "/setting/path").build();
assertThat(fileData.resolvePaths(TEST_PATH_LOOKUP).toList(), contains(Path.of("/setting/path")));

fileData = FileData.ofPathSetting("foo.*.bar", READ, false);
fileData = FileData.ofConfigPathSetting("foo.*.bar", READ, false);
settings = Settings.builder().put("foo.baz.bar", "/setting/path").build();
assertThat(fileData.resolvePaths(TEST_PATH_LOOKUP).toList(), contains(Path.of("/setting/path")));

fileData = FileData.ofPathSetting("foo.*.bar", READ, false);
fileData = FileData.ofConfigPathSetting("foo.*.bar", READ, false);
settings = Settings.builder().put("foo.baz.bar", "/setting/path").put("foo.baz2.bar", "/other/path").build();
assertThat(fileData.resolvePaths(TEST_PATH_LOOKUP).toList(), containsInAnyOrder(Path.of("/setting/path"), Path.of("/other/path")));
}

public void testExclusiveParsing() throws Exception {
Policy parsedPolicy = new PolicyParser(new ByteArrayInputStream("""
entitlement-module-name:
- files:
- path: /test
mode: read
exclusive: true
""".getBytes(StandardCharsets.UTF_8)), "test-policy.yaml", true).parsePolicy();
Policy expected = new Policy(
"test-policy.yaml",
List.of(
new Scope(
"entitlement-module-name",
List.of(FilesEntitlement.build(List.of(Map.of("path", "/test", "mode", "read", "exclusive", true))))
)
)
);
assertEquals(expected, parsedPolicy);
fileData = FileData.ofConfigPathSetting("foo.bar", READ, false);
settings = Settings.builder().put("foo.bar", "relative_path").build();
assertThat(fileData.resolvePaths(TEST_PATH_LOOKUP).toList(), contains(Path.of("/config/relative_path")));
}

public void testPathSettingIgnoreUrl() {
var fileData = FileData.ofPathSetting("foo.*.bar", READ, true);
var fileData = FileData.ofConfigPathSetting("foo.*.bar", READ, true);
settings = Settings.builder().put("foo.nonurl.bar", "/setting/path").put("foo.url.bar", "https://mysite").build();
assertThat(fileData.resolvePaths(TEST_PATH_LOOKUP).toList(), contains(Path.of("/setting/path")));
}

public void testRelativePathSettingIgnoreUrl() {
var fileData = FileData.ofRelativePathSetting("foo.*.bar", CONFIG, READ, true);
settings = Settings.builder().put("foo.nonurl.bar", "path").put("foo.url.bar", "https://mysite").build();
assertThat(fileData.resolvePaths(TEST_PATH_LOOKUP).toList(), contains(Path.of("/config/path")));
}

public void testIgnoreUrlValidation() {
var e = expectThrows(
PolicyValidationException.class,
() -> FilesEntitlement.build(List.of(Map.of("path", "/foo", "mode", "read", "ignore_url", true)))
);
assertThat(e.getMessage(), is("'ignore_url' may only be used with `path_setting` or `relative_path_setting`"));
assertThat(e.getMessage(), is("'ignore_url' may only be used with 'config_path_setting'"));

e = expectThrows(
PolicyValidationException.class,
() -> FilesEntitlement.build(
List.of(Map.of("relative_path", "foo", "relative_to", "config", "mode", "read", "ignore_url", true))
)
);
assertThat(e.getMessage(), is("'ignore_url' may only be used with `path_setting` or `relative_path_setting`"));
assertThat(e.getMessage(), is("'ignore_url' may only be used with 'config_path_setting'"));
}

public void testExclusiveParsing() throws Exception {
Policy parsedPolicy = new PolicyParser(new ByteArrayInputStream("""
entitlement-module-name:
- files:
- path: /test
mode: read
exclusive: true
""".getBytes(StandardCharsets.UTF_8)), "test-policy.yaml", true).parsePolicy();
Policy expected = new Policy(
"test-policy.yaml",
List.of(
new Scope(
"entitlement-module-name",
List.of(FilesEntitlement.build(List.of(Map.of("path", "/test", "mode", "read", "exclusive", true))))
)
)
);
assertEquals(expected, parsedPolicy);
}
}