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..6c3a367b15ee8 100644 --- a/server/src/main/java/org/elasticsearch/common/util/Maps.java +++ b/server/src/main/java/org/elasticsearch/common/util/Maps.java @@ -99,4 +99,23 @@ 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 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 + * @return {@code true} if the two maps are equal + */ + public static boolean deepEquals(Map left, Map right) { + 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()) && 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 6edffee268ecc..b670d6badab1c 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,31 @@ public void testOfEntries() { assertMapEntriesAndImmutability(map, entries); } + 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.deepEquals(map, mapCopy)); + + final Map mapModified = mapCopy; + if (mapModified.isEmpty()) { + mapModified.put(keyGenerator.get(), arrayValueGenerator.get()); + } else { + if (randomBoolean()) { + 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.deepEquals(map, mapModified)); + } + 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 +190,10 @@ private void assertMapEntriesAndImmutability( assertMapImmutability(map); } + private static Map randomMap(int size, Supplier keyGenerator, Supplier valueGenerator) { + 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 bbb5d77b90f4a..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 @@ -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; @@ -191,9 +192,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()); + final Map previousUsers = users; users = parseFileLenient(file, logger, settings); - notifyRefresh(); + + 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 e79621964e70e..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 @@ -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; @@ -206,9 +207,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()); + final Map previousUserRoles = userRoles; userRoles = parseFileLenient(file, logger); - notifyRefresh(); + + 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 1018c591617a9..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,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()); + final 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..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 @@ -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,20 @@ public void testStore_AutoReload() throws Exception { watcherService.start(); + try (BufferedWriter writer = Files.newBufferedWriter(file, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) { + writer.append("\n"); + } + + watcherService.notifyNow(ResourceWatcherService.Frequency.HIGH); + if (latch.getCount() != 1) { + 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..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 @@ -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,17 @@ public void testStoreAutoReload() throws Exception { watcherService.start(); + try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) { + writer.append("\n"); + } + + watcherService.notifyNow(ResourceWatcherService.Frequency.HIGH); + if (latch.getCount() != 1) { + 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..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 @@ -112,6 +112,18 @@ public void testMapper_AutoReload() throws Exception { watcherService.start(); + try (BufferedWriter writer = Files.newBufferedWriter(file, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) { + writer.append("\n"); + } + + watcherService.notifyNow(ResourceWatcherService.Frequency.HIGH); + if (latch.getCount() != 1) { + 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..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 @@ -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,19 @@ public void testAutoReload() throws Exception { watcherService.start(); + try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) { + writer.append("\n"); + } + + watcherService.notifyNow(ResourceWatcherService.Frequency.HIGH); + if (latch.getCount() != 1) { + 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();