From bb97d85d3ba1788a4ad8551560d7142e6f74b455 Mon Sep 17 00:00:00 2001 From: Wouter Born Date: Fri, 26 Nov 2021 23:06:34 +0100 Subject: [PATCH] Fix ConcurrentModificationException in ExpiringUserSecurityContextCache (#2579) The issue occurs when expired entries are removed from the cache. Also adds some unit tests in which the same issue could be reproduced. Fixes #2528 Signed-off-by: Wouter Born --- .../ExpiringUserSecurityContextCache.java | 7 +- .../ExpiringUserSecurityContextCacheTest.java | 132 ++++++++++++++++++ 2 files changed, 136 insertions(+), 3 deletions(-) create mode 100644 bundles/org.openhab.core.io.rest.auth/src/test/java/org/openhab/core/io/rest/auth/internal/ExpiringUserSecurityContextCacheTest.java diff --git a/bundles/org.openhab.core.io.rest.auth/src/main/java/org/openhab/core/io/rest/auth/internal/ExpiringUserSecurityContextCache.java b/bundles/org.openhab.core.io.rest.auth/src/main/java/org/openhab/core/io/rest/auth/internal/ExpiringUserSecurityContextCache.java index 6c651a53c51..bfc8babfc5a 100644 --- a/bundles/org.openhab.core.io.rest.auth/src/main/java/org/openhab/core/io/rest/auth/internal/ExpiringUserSecurityContextCache.java +++ b/bundles/org.openhab.core.io.rest.auth/src/main/java/org/openhab/core/io/rest/auth/internal/ExpiringUserSecurityContextCache.java @@ -12,6 +12,7 @@ */ package org.openhab.core.io.rest.auth.internal; +import java.util.HashSet; import java.util.LinkedHashMap; import java.util.Map; @@ -28,8 +29,8 @@ */ @NonNullByDefault public class ExpiringUserSecurityContextCache { - private static final int MAX_SIZE = 10; - private static final int CLEANUP_FREQUENCY = 10; + static final int MAX_SIZE = 10; + static final int CLEANUP_FREQUENCY = 10; private final long keepPeriod; private final Map entryMap; @@ -50,7 +51,7 @@ protected boolean removeEldestEntry(Map.@Nullable Entry eldest) { synchronized @Nullable UserSecurityContext get(String key) { calls++; if (calls >= CLEANUP_FREQUENCY) { - entryMap.keySet().forEach(k -> getEntry(k)); + new HashSet<>(entryMap.keySet()).forEach(k -> getEntry(k)); calls = 0; } Entry entry = getEntry(key); diff --git a/bundles/org.openhab.core.io.rest.auth/src/test/java/org/openhab/core/io/rest/auth/internal/ExpiringUserSecurityContextCacheTest.java b/bundles/org.openhab.core.io.rest.auth/src/test/java/org/openhab/core/io/rest/auth/internal/ExpiringUserSecurityContextCacheTest.java new file mode 100644 index 00000000000..a701c2bc375 --- /dev/null +++ b/bundles/org.openhab.core.io.rest.auth/src/test/java/org/openhab/core/io/rest/auth/internal/ExpiringUserSecurityContextCacheTest.java @@ -0,0 +1,132 @@ +/** + * Copyright (c) 2010-2021 Contributors to the openHAB project + * + * See the NOTICE file(s) distributed with this work for additional + * information. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License 2.0 which is available at + * http://www.eclipse.org/legal/epl-2.0 + * + * SPDX-License-Identifier: EPL-2.0 + */ +package org.openhab.core.io.rest.auth.internal; + +import static org.hamcrest.CoreMatchers.*; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.openhab.core.io.rest.auth.internal.ExpiringUserSecurityContextCache.*; + +import java.time.Duration; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Map.Entry; + +import org.eclipse.jdt.annotation.NonNullByDefault; +import org.junit.jupiter.api.Test; +import org.openhab.core.auth.Authentication; +import org.openhab.core.auth.GenericUser; + +/** + * Tests {@link ExpiringUserSecurityContextCache}. + * + * @author Wouter Born - Initial contribution + */ +@NonNullByDefault +public class ExpiringUserSecurityContextCacheTest { + + private static final Duration ONE_HOUR = Duration.ofHours(1); + + private ExpiringUserSecurityContextCache createCache(Duration expirationDuration) { + return new ExpiringUserSecurityContextCache(expirationDuration.toMillis()); + } + + private Map createValues(int count) { + Map map = new LinkedHashMap<>(); + for (int i = 0; i < count; i++) { + String userName = "user" + i; + UserSecurityContext userSecurityContext = new UserSecurityContext(new GenericUser(userName), + new Authentication(userName), userName + " token"); + map.put("key" + i, userSecurityContext); + } + return map; + } + + private void addValues(ExpiringUserSecurityContextCache cache, Map values) { + values.entrySet().stream().forEach(entry -> cache.put(entry.getKey(), entry.getValue())); + } + + private void assertValuesAreCached(Map values, + ExpiringUserSecurityContextCache cache) { + for (Entry entry : values.entrySet()) { + assertThat(cache.get(entry.getKey()), is(entry.getValue())); + } + } + + private void assertValuesAreNotCached(Map values, + ExpiringUserSecurityContextCache cache) { + for (Entry entry : values.entrySet()) { + assertThat(cache.get(entry.getKey()), is(nullValue())); + } + } + + @Test + public void cachedValuesAreReturned() { + ExpiringUserSecurityContextCache cache = createCache(ONE_HOUR); + Map values = createValues(MAX_SIZE); + addValues(cache, values); + assertValuesAreCached(values, cache); + } + + @Test + public void nonCachedValuesAreNotReturned() { + ExpiringUserSecurityContextCache cache = createCache(ONE_HOUR); + Map values = createValues(MAX_SIZE); + assertValuesAreNotCached(values, cache); + } + + @Test + public void clearedValuesAreNotReturned() { + ExpiringUserSecurityContextCache cache = createCache(ONE_HOUR); + Map values = createValues(MAX_SIZE); + addValues(cache, values); + cache.clear(); + assertValuesAreNotCached(values, cache); + } + + @Test + public void eldestEntriesAreRemovedWhenMaxSizeIsExceeded() { + ExpiringUserSecurityContextCache cache = createCache(ONE_HOUR); + int removed = 20; + Map values = createValues(MAX_SIZE + removed); + addValues(cache, values); + + Map removedValues = new LinkedHashMap<>(); + Map cachedValues = new LinkedHashMap<>(); + + int i = 0; + for (Entry entry : values.entrySet()) { + if (i < removed) { + removedValues.put(entry.getKey(), entry.getValue()); + } else { + cachedValues.put(entry.getKey(), entry.getValue()); + } + i++; + } + + assertValuesAreNotCached(removedValues, cache); + assertValuesAreCached(cachedValues, cache); + } + + @Test + public void expiredEntriesAreRemoved() { + ExpiringUserSecurityContextCache cache = createCache(Duration.ZERO); + Map values = createValues(MAX_SIZE); + addValues(cache, values); + + for (int i = 0; i < CLEANUP_FREQUENCY; i++) { + cache.get("key"); + } + + assertValuesAreNotCached(values, cache); + } +}