From 69b0f4d64cc965107b7941c8ceaa0ef33fb7c814 Mon Sep 17 00:00:00 2001 From: Anton Shuvaev Date: Sun, 15 Dec 2019 15:32:22 +0300 Subject: [PATCH 1/3] Security should not reload files that haven't changed --- .../org/elasticsearch/common/util/Maps.java | 22 +++++++ .../elasticsearch/common/util/MapsTests.java | 57 +++++++++++++++++++ .../authc/file/FileUserPasswdStore.java | 12 +++- .../authc/file/FileUserRolesStore.java | 12 +++- .../security/authc/support/DnRoleMapper.java | 10 +++- .../security/authz/store/FileRolesStore.java | 8 ++- .../authc/file/FileUserPasswdStoreTests.java | 15 ++++- .../authc/file/FileUserRolesStoreTests.java | 12 +++- .../authc/support/DnRoleMapperTests.java | 11 ++++ .../authz/store/FileRolesStoreTests.java | 14 ++++- 10 files changed, 158 insertions(+), 15 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/util/Maps.java b/server/src/main/java/org/elasticsearch/common/util/Maps.java index 175a2a24db562..aa34a1d794e73 100644 --- a/server/src/main/java/org/elasticsearch/common/util/Maps.java +++ b/server/src/main/java/org/elasticsearch/common/util/Maps.java @@ -24,6 +24,7 @@ import java.util.Collection; import java.util.Map; import java.util.Objects; +import java.util.function.BiPredicate; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -99,4 +100,25 @@ public static Map ofEntries(final Collection> entri return map; } + /** + * Returns {@code true} if the two specified maps are equal to one another. Two maps are considered equal if both contain + * the same number of entries, and all values ​​of the corresponding keys are equal by the specified equivalence relationship + * The primary use case is to check if two maps with array values are equal. + * + * @param left one map to be tested for equality + * @param right the other map to be tested for equality + * @param valueEquivalence the equivalence relationship for comparing values + * @return {@code true} if the two maps are equal + */ + public static boolean equals(Map left, Map right, BiPredicate valueEquivalence) { + if (left == right) { + return true; + } + if (left == null || right == null || left.size() != right.size()) { + return false; + } + return left.entrySet().stream() + .allMatch(e -> right.containsKey(e.getKey()) && valueEquivalence.test(e.getValue(), right.get(e.getKey()))); + } + } diff --git a/server/src/test/java/org/elasticsearch/common/util/MapsTests.java b/server/src/test/java/org/elasticsearch/common/util/MapsTests.java index 6edffee268ecc..80d46eec2943e 100644 --- a/server/src/test/java/org/elasticsearch/common/util/MapsTests.java +++ b/server/src/test/java/org/elasticsearch/common/util/MapsTests.java @@ -22,13 +22,18 @@ import org.elasticsearch.test.ESTestCase; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; +import java.util.HashMap; import java.util.HashSet; import java.util.Map; +import java.util.function.Supplier; import java.util.stream.Collectors; +import java.util.stream.IntStream; import java.util.stream.Stream; import static java.util.Map.entry; +import static java.util.stream.Collectors.toMap; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasItem; @@ -103,6 +108,52 @@ public void testOfEntries() { assertMapEntriesAndImmutability(map, entries); } + public void testEqualsStringValue() { + Supplier keyGenerator = () -> randomAlphaOfLengthBetween(1, 5); + Supplier stringValueGenerator = () -> randomAlphaOfLengthBetween(1, 5); + Map map = randomMap(randomInt(5), keyGenerator, stringValueGenerator); + Map mapCopy = new HashMap<>(map); + Map mapModified = new HashMap<>(map); + if (mapModified.isEmpty()) { + mapModified.put(keyGenerator.get(), stringValueGenerator.get()); + } else { + if (randomBoolean()) { + String randomKey = mapModified.keySet().toArray(new String[0])[randomInt(mapModified.size() - 1)]; + mapModified.put(randomKey, randomValueOtherThan(mapModified.get(randomKey), stringValueGenerator)); + } else { + mapModified.put(randomValueOtherThanMany(mapModified::containsKey, keyGenerator), stringValueGenerator.get()); + } + } + + assertFalse(Maps.equals(map, mapModified, String::equals)); + assertTrue(Maps.equals(map, mapCopy, String::equals)); + } + + public void testEqualsArrayValue() { + Supplier keyGenerator = () -> randomAlphaOfLengthBetween(1, 5); + Supplier arrayValueGenerator = () -> random().ints(randomInt(5)).toArray(); + Map map = randomMap(randomInt(5), keyGenerator, arrayValueGenerator); + Map mapCopy = map.entrySet().stream() + .collect(toMap(Map.Entry::getKey, e -> Arrays.copyOf(e.getValue(), e.getValue().length))); + + assertTrue(Maps.equals(map, mapCopy, Arrays::equals)); + + Map mapModified = mapCopy; + if (mapModified.isEmpty()) { + mapModified.put(keyGenerator.get(), arrayValueGenerator.get()); + } else { + if (randomBoolean()) { + String randomKey = mapModified.keySet().toArray(new String[0])[randomInt(mapModified.size() - 1)]; + int[] value = mapModified.get(randomKey); + mapModified.put(randomKey, randomValueOtherThanMany((v) -> Arrays.equals(v, value), arrayValueGenerator)); + } else { + mapModified.put(randomValueOtherThanMany(mapModified::containsKey, keyGenerator), arrayValueGenerator.get()); + } + } + + assertFalse(Maps.equals(map, mapModified, Arrays::equals)); + } + private void assertMapEntries(final Map map, final Collection> entries) { for (var entry : entries) { assertThat("map [" + map + "] does not contain key [" + entry.getKey() + "]", map.keySet(), hasItem(entry.getKey())); @@ -160,4 +211,10 @@ private void assertMapEntriesAndImmutability( assertMapImmutability(map); } + private static Map randomMap(int size, Supplier keyGenerator, Supplier valueGenerator) { + Map map = new HashMap<>(); + IntStream.range(0, size).forEach(i -> map.put(keyGenerator.get(), valueGenerator.get())); + return map; + } + } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStore.java index bbb5d77b90f4a..abf7315796995 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStore.java @@ -5,14 +5,15 @@ */ package org.elasticsearch.xpack.security.authc.file; -import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.message.ParameterizedMessage; import org.apache.logging.log4j.util.Supplier; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.Maps; import org.elasticsearch.env.Environment; import org.elasticsearch.watcher.FileChangesListener; import org.elasticsearch.watcher.FileWatcher; @@ -32,6 +33,7 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -191,9 +193,13 @@ public void onFileDeleted(Path file) { @Override public void onFileChanged(Path file) { if (file.equals(FileUserPasswdStore.this.file)) { - logger.info("users file [{}] changed. updating users... )", file.toAbsolutePath()); + Map previousUsers = users; users = parseFileLenient(file, logger, settings); - notifyRefresh(); + + if (Maps.equals(previousUsers, users, Arrays::equals) == false) { + logger.info("users file [{}] changed. updating users... )", file.toAbsolutePath()); + notifyRefresh(); + } } } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java index e79621964e70e..9a637801667bb 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java @@ -5,13 +5,14 @@ */ package org.elasticsearch.xpack.security.authc.file; -import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.message.ParameterizedMessage; import org.apache.logging.log4j.util.Supplier; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.Strings; +import org.elasticsearch.common.util.Maps; import org.elasticsearch.env.Environment; import org.elasticsearch.watcher.FileChangesListener; import org.elasticsearch.watcher.FileWatcher; @@ -27,6 +28,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -206,9 +208,13 @@ public void onFileDeleted(Path file) { @Override public void onFileChanged(Path file) { if (file.equals(FileUserRolesStore.this.file)) { - logger.info("users roles file [{}] changed. updating users roles...", file.toAbsolutePath()); + Map previousUserRoles = userRoles; userRoles = parseFileLenient(file, logger); - notifyRefresh(); + + if (Maps.equals(previousUserRoles, userRoles, Arrays::equals) == false) { + logger.info("users roles file [{}] changed. updating users roles...", file.toAbsolutePath()); + notifyRefresh(); + } } } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapper.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapper.java index 1018c591617a9..02e3591fa67b6 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapper.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapper.java @@ -223,10 +223,14 @@ public void onFileDeleted(Path file) { @Override public void onFileChanged(Path file) { if (file.equals(DnRoleMapper.this.file)) { - logger.info("role mappings file [{}] changed for realm [{}/{}]. updating mappings...", file.toAbsolutePath(), - config.type(), config.name()); + Map> previousDnRoles = dnRoles; dnRoles = parseFileLenient(file, logger, config.type(), config.name()); - notifyRefresh(); + + if (previousDnRoles.equals(dnRoles) == false) { + logger.info("role mappings file [{}] changed for realm [{}/{}]. updating mappings...", file.toAbsolutePath(), + config.type(), config.name()); + notifyRefresh(); + } } } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java index b1059c46cc668..90b94d1275989 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java @@ -368,8 +368,6 @@ public synchronized void onFileChanged(Path file) { final Map previousPermissions = permissions; try { permissions = parseFile(file, logger, settings, licenseState, xContentRegistry); - logger.info("updated roles (roles file [{}] {})", file.toAbsolutePath(), - Files.exists(file) ? "changed" : "removed"); } catch (Exception e) { logger.error( (Supplier) () -> new ParameterizedMessage( @@ -383,7 +381,11 @@ public synchronized void onFileChanged(Path file) { .collect(Collectors.toSet()); final Set addedRoles = Sets.difference(permissions.keySet(), previousPermissions.keySet()); final Set changedRoles = Collections.unmodifiableSet(Sets.union(changedOrMissingRoles, addedRoles)); - listeners.forEach(c -> c.accept(changedRoles)); + if (changedRoles.isEmpty() == false) { + logger.info("updated roles (roles file [{}] {})", file.toAbsolutePath(), + Files.exists(file) ? "changed" : "removed"); + listeners.forEach(c -> c.accept(changedRoles)); + } } } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStoreTests.java index e6f099e389741..8d906f28f6e76 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStoreTests.java @@ -53,7 +53,7 @@ public class FileUserPasswdStoreTests extends ESTestCase { @Before public void init() { settings = Settings.builder() - .put("resource.reload.interval.high", "2s") + .put("resource.reload.interval.high", "100ms") .put("path.home", createTempDir()) .put("xpack.security.authc.password_hashing.algorithm", randomFrom("bcrypt", "bcrypt11", "pbkdf2", "pbkdf2_1000", "pbkdf2_50000")) @@ -103,6 +103,19 @@ public void testStore_AutoReload() throws Exception { watcherService.start(); + try (BufferedWriter writer = Files.newBufferedWriter(file, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) { + writer.append("\n"); + } + + if (latch.await(200, TimeUnit.MILLISECONDS)) { + fail("Listener should not be called as users passwords are not changed."); + } + + assertThat(store.userExists(username), is(true)); + result = store.verifyPassword(username, new SecureString("test123"), () -> user); + assertThat(result.getStatus(), is(AuthenticationResult.Status.SUCCESS)); + assertThat(result.getUser(), is(user)); + try (BufferedWriter writer = Files.newBufferedWriter(file, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) { writer.newLine(); writer.append("foobar:").append(new String(hasher.hash(new SecureString("barfoo")))); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStoreTests.java index 0fef08acefbd4..c0fd517ae4328 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStoreTests.java @@ -54,7 +54,7 @@ public class FileUserRolesStoreTests extends ESTestCase { @Before public void init() { settings = Settings.builder() - .put("resource.reload.interval.high", "2s") + .put("resource.reload.interval.high", "100ms") .put("path.home", createTempDir()) .build(); env = TestEnvironment.newEnvironment(settings); @@ -102,6 +102,16 @@ public void testStoreAutoReload() throws Exception { watcherService.start(); + try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) { + writer.append("\n"); + } + + if (latch.await(200, TimeUnit.MILLISECONDS)) { + fail("Listener should not be called as users roles are not changed."); + } + + assertThat(store.roles("user1"), arrayContaining("role1", "role2", "role3")); + try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) { writer.newLine(); writer.append("role4:user4\nrole5:user4\n"); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapperTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapperTests.java index 7f7899e65943c..781a321cad2f2 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapperTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapperTests.java @@ -112,6 +112,17 @@ public void testMapper_AutoReload() throws Exception { watcherService.start(); + try (BufferedWriter writer = Files.newBufferedWriter(file, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) { + writer.append("\n"); + } + + if (latch.await(200, TimeUnit.MILLISECONDS)) { + fail("Listener should not be called as roles mapping is not changed."); + } + + roles = mapper.resolveRoles("", Collections.singletonList("cn=shield,ou=marvel,o=superheros")); + assertThat(roles, contains("security")); + try (BufferedWriter writer = Files.newBufferedWriter(file, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) { writer.newLine(); writer.append("fantastic_four:\n") diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java index 99ae113e15fe9..62db5c3feaa32 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java @@ -332,7 +332,7 @@ public void testAutoReload() throws Exception { } Settings.Builder builder = Settings.builder() - .put("resource.reload.interval.high", "500ms") + .put("resource.reload.interval.high", "100ms") .put("path.home", home); Settings settings = builder.build(); Environment env = TestEnvironment.newEnvironment(settings); @@ -354,6 +354,18 @@ public void testAutoReload() throws Exception { watcherService.start(); + try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) { + writer.append("\n"); + } + + if (latch.await(200, TimeUnit.MILLISECONDS)) { + fail("Listener should not be called as roles are not changed."); + } + + descriptors = store.roleDescriptors(Collections.singleton("role1")); + assertThat(descriptors, notNullValue()); + assertEquals(1, descriptors.size()); + try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) { writer.newLine(); writer.newLine(); From c37b64f7b2fba14f89cc4b48a1f8114aa2d15251 Mon Sep 17 00:00:00 2001 From: Anton Shuvaev Date: Mon, 16 Dec 2019 10:14:29 +0300 Subject: [PATCH 2/3] Fix review issues --- .../org/elasticsearch/common/util/Maps.java | 15 +++---- .../elasticsearch/common/util/MapsTests.java | 43 +++++-------------- .../authc/file/FileUserPasswdStore.java | 4 +- .../authc/file/FileUserRolesStore.java | 4 +- .../security/authc/support/DnRoleMapper.java | 2 +- .../authc/file/FileUserPasswdStoreTests.java | 3 +- .../authc/file/FileUserRolesStoreTests.java | 3 +- .../authc/support/DnRoleMapperTests.java | 3 +- .../authz/store/FileRolesStoreTests.java | 3 +- 9 files changed, 30 insertions(+), 50 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/util/Maps.java b/server/src/main/java/org/elasticsearch/common/util/Maps.java index aa34a1d794e73..6c3a367b15ee8 100644 --- a/server/src/main/java/org/elasticsearch/common/util/Maps.java +++ b/server/src/main/java/org/elasticsearch/common/util/Maps.java @@ -24,7 +24,6 @@ import java.util.Collection; import java.util.Map; import java.util.Objects; -import java.util.function.BiPredicate; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -101,16 +100,14 @@ public static Map ofEntries(final Collection> entri } /** - * Returns {@code true} if the two specified maps are equal to one another. Two maps are considered equal if both contain - * the same number of entries, and all values ​​of the corresponding keys are equal by the specified equivalence relationship - * The primary use case is to check if two maps with array values are equal. + * Returns {@code true} if the two specified maps are equal to one another. Two maps are considered equal if both represent identical + * mappings where values are checked with Objects.deepEquals. The primary use case is to check if two maps with array values are equal. * - * @param left one map to be tested for equality - * @param right the other map to be tested for equality - * @param valueEquivalence the equivalence relationship for comparing values + * @param left one map to be tested for equality + * @param right the other map to be tested for equality * @return {@code true} if the two maps are equal */ - public static boolean equals(Map left, Map right, BiPredicate valueEquivalence) { + public static boolean deepEquals(Map left, Map right) { if (left == right) { return true; } @@ -118,7 +115,7 @@ public static boolean equals(Map left, Map right, BiPredicate return false; } return left.entrySet().stream() - .allMatch(e -> right.containsKey(e.getKey()) && valueEquivalence.test(e.getValue(), right.get(e.getKey()))); + .allMatch(e -> right.containsKey(e.getKey()) && Objects.deepEquals(e.getValue(), right.get(e.getKey()))); } } diff --git a/server/src/test/java/org/elasticsearch/common/util/MapsTests.java b/server/src/test/java/org/elasticsearch/common/util/MapsTests.java index 80d46eec2943e..b670d6badab1c 100644 --- a/server/src/test/java/org/elasticsearch/common/util/MapsTests.java +++ b/server/src/test/java/org/elasticsearch/common/util/MapsTests.java @@ -108,50 +108,29 @@ public void testOfEntries() { assertMapEntriesAndImmutability(map, entries); } - public void testEqualsStringValue() { - Supplier keyGenerator = () -> randomAlphaOfLengthBetween(1, 5); - Supplier stringValueGenerator = () -> randomAlphaOfLengthBetween(1, 5); - Map map = randomMap(randomInt(5), keyGenerator, stringValueGenerator); - Map mapCopy = new HashMap<>(map); - Map mapModified = new HashMap<>(map); - if (mapModified.isEmpty()) { - mapModified.put(keyGenerator.get(), stringValueGenerator.get()); - } else { - if (randomBoolean()) { - String randomKey = mapModified.keySet().toArray(new String[0])[randomInt(mapModified.size() - 1)]; - mapModified.put(randomKey, randomValueOtherThan(mapModified.get(randomKey), stringValueGenerator)); - } else { - mapModified.put(randomValueOtherThanMany(mapModified::containsKey, keyGenerator), stringValueGenerator.get()); - } - } - - assertFalse(Maps.equals(map, mapModified, String::equals)); - assertTrue(Maps.equals(map, mapCopy, String::equals)); - } - - public void testEqualsArrayValue() { - Supplier keyGenerator = () -> randomAlphaOfLengthBetween(1, 5); - Supplier arrayValueGenerator = () -> random().ints(randomInt(5)).toArray(); - Map map = randomMap(randomInt(5), keyGenerator, arrayValueGenerator); - Map mapCopy = map.entrySet().stream() + public void testDeepEquals() { + final Supplier keyGenerator = () -> randomAlphaOfLengthBetween(1, 5); + final Supplier arrayValueGenerator = () -> random().ints(randomInt(5)).toArray(); + final Map map = randomMap(randomInt(5), keyGenerator, arrayValueGenerator); + final Map mapCopy = map.entrySet().stream() .collect(toMap(Map.Entry::getKey, e -> Arrays.copyOf(e.getValue(), e.getValue().length))); - assertTrue(Maps.equals(map, mapCopy, Arrays::equals)); + assertTrue(Maps.deepEquals(map, mapCopy)); - Map mapModified = mapCopy; + final Map mapModified = mapCopy; if (mapModified.isEmpty()) { mapModified.put(keyGenerator.get(), arrayValueGenerator.get()); } else { if (randomBoolean()) { - String randomKey = mapModified.keySet().toArray(new String[0])[randomInt(mapModified.size() - 1)]; - int[] value = mapModified.get(randomKey); + final String randomKey = mapModified.keySet().toArray(new String[0])[randomInt(mapModified.size() - 1)]; + final int[] value = mapModified.get(randomKey); mapModified.put(randomKey, randomValueOtherThanMany((v) -> Arrays.equals(v, value), arrayValueGenerator)); } else { mapModified.put(randomValueOtherThanMany(mapModified::containsKey, keyGenerator), arrayValueGenerator.get()); } } - assertFalse(Maps.equals(map, mapModified, Arrays::equals)); + assertFalse(Maps.deepEquals(map, mapModified)); } private void assertMapEntries(final Map map, final Collection> entries) { @@ -212,7 +191,7 @@ private void assertMapEntriesAndImmutability( } private static Map randomMap(int size, Supplier keyGenerator, Supplier valueGenerator) { - Map map = new HashMap<>(); + final Map map = new HashMap<>(); IntStream.range(0, size).forEach(i -> map.put(keyGenerator.get(), valueGenerator.get())); return map; } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStore.java index abf7315796995..cf1bcdd01c117 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStore.java @@ -193,10 +193,10 @@ public void onFileDeleted(Path file) { @Override public void onFileChanged(Path file) { if (file.equals(FileUserPasswdStore.this.file)) { - Map previousUsers = users; + final Map previousUsers = users; users = parseFileLenient(file, logger, settings); - if (Maps.equals(previousUsers, users, Arrays::equals) == false) { + if (Maps.deepEquals(previousUsers, users) == false) { logger.info("users file [{}] changed. updating users... )", file.toAbsolutePath()); notifyRefresh(); } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java index 9a637801667bb..e30870f018bc2 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java @@ -208,10 +208,10 @@ public void onFileDeleted(Path file) { @Override public void onFileChanged(Path file) { if (file.equals(FileUserRolesStore.this.file)) { - Map previousUserRoles = userRoles; + final Map previousUserRoles = userRoles; userRoles = parseFileLenient(file, logger); - if (Maps.equals(previousUserRoles, userRoles, Arrays::equals) == false) { + if (Maps.deepEquals(previousUserRoles, userRoles) == false) { logger.info("users roles file [{}] changed. updating users roles...", file.toAbsolutePath()); notifyRefresh(); } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapper.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapper.java index 02e3591fa67b6..f62c8521a69ff 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapper.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapper.java @@ -223,7 +223,7 @@ public void onFileDeleted(Path file) { @Override public void onFileChanged(Path file) { if (file.equals(DnRoleMapper.this.file)) { - Map> previousDnRoles = dnRoles; + final Map> previousDnRoles = dnRoles; dnRoles = parseFileLenient(file, logger, config.type(), config.name()); if (previousDnRoles.equals(dnRoles) == false) { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStoreTests.java index 8d906f28f6e76..42cd5530f306d 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStoreTests.java @@ -107,7 +107,8 @@ public void testStore_AutoReload() throws Exception { writer.append("\n"); } - if (latch.await(200, TimeUnit.MILLISECONDS)) { + watcherService.notifyNow(ResourceWatcherService.Frequency.HIGH); + if (latch.getCount() != 1) { fail("Listener should not be called as users passwords are not changed."); } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStoreTests.java index c0fd517ae4328..4e1dd0644911c 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStoreTests.java @@ -106,7 +106,8 @@ public void testStoreAutoReload() throws Exception { writer.append("\n"); } - if (latch.await(200, TimeUnit.MILLISECONDS)) { + watcherService.notifyNow(ResourceWatcherService.Frequency.HIGH); + if (latch.getCount() != 1) { fail("Listener should not be called as users roles are not changed."); } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapperTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapperTests.java index 781a321cad2f2..2f62c35ba1130 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapperTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapperTests.java @@ -116,7 +116,8 @@ public void testMapper_AutoReload() throws Exception { writer.append("\n"); } - if (latch.await(200, TimeUnit.MILLISECONDS)) { + watcherService.notifyNow(ResourceWatcherService.Frequency.HIGH); + if (latch.getCount() != 1) { fail("Listener should not be called as roles mapping is not changed."); } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java index 62db5c3feaa32..f0cc9840e5e13 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java @@ -358,7 +358,8 @@ public void testAutoReload() throws Exception { writer.append("\n"); } - if (latch.await(200, TimeUnit.MILLISECONDS)) { + watcherService.notifyNow(ResourceWatcherService.Frequency.HIGH); + if (latch.getCount() != 1) { fail("Listener should not be called as roles are not changed."); } From 50895b038d1d6d32da36c1ee9024365cfc921549 Mon Sep 17 00:00:00 2001 From: Anton Shuvaev Date: Mon, 16 Dec 2019 19:41:50 +0300 Subject: [PATCH 3/3] Fix unused imports --- .../xpack/security/authc/file/FileUserPasswdStore.java | 1 - .../xpack/security/authc/file/FileUserRolesStore.java | 1 - 2 files changed, 2 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStore.java index cf1bcdd01c117..dab7152c538df 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStore.java @@ -33,7 +33,6 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; -import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java index e30870f018bc2..1eb2e468c8ef2 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java @@ -28,7 +28,6 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List;