From c17c140cd2843e82a26fe61eecf9abf56b54099b Mon Sep 17 00:00:00 2001 From: uboness Date: Thu, 28 Aug 2014 15:34:58 -0700 Subject: [PATCH] Small fixes - throw an error on initialization if the audit trail is configured with an unknown output - removed unnecessary null checks Original commit: elastic/x-pack-elasticsearch@8d267235530660affa9deace3f7ae679f11ce8bf --- .../shield/audit/AuditTrailModule.java | 3 +++ .../authc/esusers/FileUserPasswdStore.java | 6 ++++- .../authc/esusers/FileUserRolesStore.java | 8 +++++-- .../authc/esusers/tool/ESUsersTool.java | 22 ++++--------------- .../shield/audit/AuditTrailModuleTests.java | 14 ++++++++++++ 5 files changed, 32 insertions(+), 21 deletions(-) diff --git a/src/main/java/org/elasticsearch/shield/audit/AuditTrailModule.java b/src/main/java/org/elasticsearch/shield/audit/AuditTrailModule.java index b1ee6840066..2f9e94c63a1 100644 --- a/src/main/java/org/elasticsearch/shield/audit/AuditTrailModule.java +++ b/src/main/java/org/elasticsearch/shield/audit/AuditTrailModule.java @@ -5,6 +5,7 @@ */ package org.elasticsearch.shield.audit; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.collect.Sets; import org.elasticsearch.common.inject.AbstractModule; import org.elasticsearch.common.inject.multibindings.Multibinder; @@ -44,6 +45,8 @@ public class AuditTrailModule extends AbstractModule { case LoggingAuditTrail.NAME: binder.addBinding().to(LoggingAuditTrail.class); break; + default: + throw new ElasticsearchException("Unknown audit trail output [" + output + "]"); } } } diff --git a/src/main/java/org/elasticsearch/shield/authc/esusers/FileUserPasswdStore.java b/src/main/java/org/elasticsearch/shield/authc/esusers/FileUserPasswdStore.java index 1790cc36820..9af38e6dd62 100644 --- a/src/main/java/org/elasticsearch/shield/authc/esusers/FileUserPasswdStore.java +++ b/src/main/java/org/elasticsearch/shield/authc/esusers/FileUserPasswdStore.java @@ -81,7 +81,11 @@ public class FileUserPasswdStore extends AbstractComponent implements UserPasswd return Paths.get(location); } - public static Map parseFile(Path path, @Nullable ESLogger logger) { + /** + * parses the esusers file. Should never return {@code null}, if the file doesn't exist an + * empty map is returned + */ + public static ImmutableMap parseFile(Path path, @Nullable ESLogger logger) { if (!Files.exists(path)) { return ImmutableMap.of(); } diff --git a/src/main/java/org/elasticsearch/shield/authc/esusers/FileUserRolesStore.java b/src/main/java/org/elasticsearch/shield/authc/esusers/FileUserRolesStore.java index e3ee21f5bfa..583c5529d15 100644 --- a/src/main/java/org/elasticsearch/shield/authc/esusers/FileUserRolesStore.java +++ b/src/main/java/org/elasticsearch/shield/authc/esusers/FileUserRolesStore.java @@ -54,7 +54,7 @@ public class FileUserRolesStore extends AbstractComponent implements UserRolesSt FileUserRolesStore(Settings settings, Environment env, ResourceWatcherService watcherService, Listener listener) { super(settings); file = resolveFile(settings, env); - userRoles = ImmutableMap.copyOf(parseFile(file, logger)); + userRoles = parseFile(file, logger); FileWatcher watcher = new FileWatcher(file.getParent().toFile()); watcher.addListener(new FileListener()); watcherService.add(watcher); @@ -74,7 +74,11 @@ public class FileUserRolesStore extends AbstractComponent implements UserRolesSt return Paths.get(location); } - public static Map parseFile(Path path, @Nullable ESLogger logger) { + /** + * parses the users_roles file. Should never return return {@code null}, if the file doesn't exist + * an empty map is returned + */ + public static ImmutableMap parseFile(Path path, @Nullable ESLogger logger) { if (!Files.exists(path)) { return ImmutableMap.of(); } diff --git a/src/main/java/org/elasticsearch/shield/authc/esusers/tool/ESUsersTool.java b/src/main/java/org/elasticsearch/shield/authc/esusers/tool/ESUsersTool.java index 39fadea3163..03211d7a25c 100644 --- a/src/main/java/org/elasticsearch/shield/authc/esusers/tool/ESUsersTool.java +++ b/src/main/java/org/elasticsearch/shield/authc/esusers/tool/ESUsersTool.java @@ -11,16 +11,14 @@ import org.elasticsearch.common.cli.CliTool; import org.elasticsearch.common.cli.CliToolConfig; import org.elasticsearch.common.cli.Terminal; import org.elasticsearch.common.cli.commons.CommandLine; -import org.elasticsearch.common.collect.Lists; -import org.elasticsearch.common.collect.Maps; -import org.elasticsearch.common.collect.ObjectArrays; -import org.elasticsearch.common.collect.Sets; +import org.elasticsearch.common.collect.*; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.Environment; import org.elasticsearch.shield.authc.esusers.FileUserPasswdStore; import org.elasticsearch.shield.authc.esusers.FileUserRolesStore; import org.elasticsearch.shield.authc.support.Hasher; +import java.nio.file.Files; import java.nio.file.Path; import java.util.*; import java.util.regex.Pattern; @@ -117,10 +115,6 @@ public class ESUsersTool extends CliTool { public ExitStatus execute(Settings settings, Environment env) throws Exception { Path file = FileUserPasswdStore.resolveFile(settings, env); Map users = new HashMap<>(FileUserPasswdStore.parseFile(file, null)); - if (users == null) { - // file doesn't exist so we just create a new file - users = new HashMap<>(); - } if (users.containsKey(username)) { terminal.println("User [%s] already exists", username); return ExitStatus.CODE_ERROR; @@ -132,10 +126,6 @@ public class ESUsersTool extends CliTool { file = FileUserRolesStore.resolveFile(settings, env); Map userRoles = new HashMap<>(FileUserRolesStore.parseFile(file, null)); - if (userRoles == null) { - // file doesn't exist, so we just create a new file - userRoles = new HashMap<>(); - } userRoles.put(username, roles); FileUserRolesStore.writeFile(userRoles, file); return ExitStatus.OK; @@ -168,7 +158,7 @@ public class ESUsersTool extends CliTool { public ExitStatus execute(Settings settings, Environment env) throws Exception { Path file = FileUserPasswdStore.resolveFile(settings, env); Map users = new HashMap<>(FileUserPasswdStore.parseFile(file, null)); - if (users != null) { + if (Files.exists(file)) { char[] passwd = users.remove(username); if (passwd != null) { FileUserPasswdStore.writeFile(users, file); @@ -179,7 +169,7 @@ public class ESUsersTool extends CliTool { file = FileUserRolesStore.resolveFile(settings, env); Map userRoles = new HashMap<>(FileUserRolesStore.parseFile(file, null)); - if (userRoles != null) { + if (Files.exists(file)) { String[] roles = userRoles.remove(username); if (roles != null) { FileUserRolesStore.writeFile(userRoles, file); @@ -234,10 +224,6 @@ public class ESUsersTool extends CliTool { public ExitStatus execute(Settings settings, Environment env) throws Exception { Path file = FileUserPasswdStore.resolveFile(settings, env); Map users = new HashMap<>(FileUserPasswdStore.parseFile(file, null)); - if (users == null) { - // file doesn't exist so we just create a new file - users = new HashMap<>(); - } if (!users.containsKey(username)) { terminal.println("User [%s] doesn't exist", username); return ExitStatus.NO_USER; diff --git a/src/test/java/org/elasticsearch/shield/audit/AuditTrailModuleTests.java b/src/test/java/org/elasticsearch/shield/audit/AuditTrailModuleTests.java index 2aa181be5a0..2d8361094b0 100644 --- a/src/test/java/org/elasticsearch/shield/audit/AuditTrailModuleTests.java +++ b/src/test/java/org/elasticsearch/shield/audit/AuditTrailModuleTests.java @@ -53,4 +53,18 @@ public class AuditTrailModuleTests extends ElasticsearchTestCase { assertThat(service.auditTrails[0], instanceOf(LoggingAuditTrail.class)); } + @Test + public void testUnknownOutput() throws Exception { + Settings settings = ImmutableSettings.builder() + .put("shield.audit.enabled", true) + .put("shield.audit.outputs" , "foo") + .build(); + try { + Guice.createInjector(new SettingsModule(settings), new AuditTrailModule(settings)); + fail("Expect initialization to fail when an unknown audit trail output is configured"); + } catch (Throwable t) { + // expected + } + } + }