This is an automated email from the ASF dual-hosted git repository. snoopdave pushed a commit to branch roller-6.0.x in repository https://gitbox.apache.org/repos/asf/roller.git
commit 66c9d578e08654d228faac3d401b098230ea91e0 Author: David M. Johnson <[email protected]> AuthorDate: Sun Jul 18 15:44:34 2021 -0400 Fix flakey test. --- .../apache/roller/weblogger/util/IPBanList.java | 171 +++++++++++---------- .../roller/weblogger/util/IPBanListTest.java | 113 ++++++++++++++ 2 files changed, 199 insertions(+), 85 deletions(-) diff --git a/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java b/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java index 822ca93..fb430a3 100644 --- a/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java +++ b/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java @@ -1,20 +1,20 @@ /* -* Licensed to the Apache Software Foundation (ASF) under one or more -* contributor license agreements. The ASF licenses this file to You -* under the Apache License, Version 2.0 (the "License"); you may not -* use this file except in compliance with the License. -* You may obtain a copy of the License at -* -* http://www.apache.org/licenses/LICENSE-2.0 -* -* Unless required by applicable law or agreed to in writing, software -* distributed under the License is distributed on an "AS IS" BASIS, -* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -* See the License for the specific language governing permissions and -* limitations under the License. For additional information regarding -* copyright in this work, please see the NOTICE file in the top level -* directory of this distribution. -*/ + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. The ASF licenses this file to You + * under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. For additional information regarding + * copyright in this work, please see the NOTICE file in the top level + * directory of this distribution. + */ package org.apache.roller.weblogger.util; @@ -22,8 +22,10 @@ import java.io.BufferedReader; import java.io.FileReader; import java.io.FileWriter; import java.io.PrintWriter; -import java.util.HashSet; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Supplier; + import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.roller.weblogger.config.WebloggerConfig; @@ -37,73 +39,73 @@ import org.apache.roller.weblogger.config.WebloggerConfig; * automatically re-read the file and update the list when that happens. */ public final class IPBanList { - - private static Log log = LogFactory.getLog(IPBanList.class); - + + private static final Log log = LogFactory.getLog(IPBanList.class); + // set of ips that are banned, use a set to ensure uniqueness - private Set bannedIps = new HashSet(); - + private volatile Set<String> bannedIps = newThreadSafeSet(); + // file listing the ips that are banned private ModifiedFile bannedIpsFile = null; - + // reference to our singleton instance private static IPBanList instance = null; - - + + static { - instance = new IPBanList(); + instance = new IPBanList(() -> WebloggerConfig.getProperty("ipbanlist.file")); } - - - // private because we are a singleton - private IPBanList() { - + + + // package-private for unit tests + IPBanList(Supplier<String> banIpsFilePathSupplier) { + log.debug("INIT"); - + // load up set of denied ips - String banIpsFilePath = WebloggerConfig.getProperty("ipbanlist.file"); + String banIpsFilePath = banIpsFilePathSupplier.get(); if(banIpsFilePath != null) { ModifiedFile banIpsFile = new ModifiedFile(banIpsFilePath); - + if(banIpsFile.exists() && banIpsFile.canRead()) { this.bannedIpsFile = banIpsFile; this.loadBannedIps(); } } } - - + + // access to the singleton instance public static IPBanList getInstance() { return instance; } - - + + public boolean isBanned(String ip) { - + // update the banned ips list if needed - this.loadBannedIpsIfNeeded(false); - + this.loadBannedIpsIfNeeded(); + if(ip != null) { return this.bannedIps.contains(ip); } else { return false; } } - - + + public void addBannedIp(String ip) { - + if(ip == null) { return; } - + // update the banned ips list if needed - this.loadBannedIpsIfNeeded(false); - - if(!this.bannedIps.contains(ip) && - (bannedIpsFile != null && bannedIpsFile.canWrite())) { - + this.loadBannedIpsIfNeeded(); + + if(!this.bannedIps.contains(ip) && + (bannedIpsFile != null && bannedIpsFile.canWrite())) { + try { synchronized(this) { // add to file @@ -111,86 +113,85 @@ public final class IPBanList { out.println(ip); out.close(); this.bannedIpsFile.clearChanged(); - + // add to Set this.bannedIps.add(ip); } - + log.debug("ADDED "+ip); } catch(Exception e) { log.error("Error adding banned ip to file", e); } } } - - + + /** * Check if the banned ips file has changed and needs to be reloaded. */ - private void loadBannedIpsIfNeeded(boolean forceLoad) { - - if(bannedIpsFile != null && - (bannedIpsFile.hasChanged() || forceLoad)) { - + private void loadBannedIpsIfNeeded() { + + if(bannedIpsFile != null && + (bannedIpsFile.hasChanged())) { + // need to reload this.loadBannedIps(); } } - - + + /** * Load the list of banned ips from a file. This clears the old list and * loads exactly what is in the file. */ private synchronized void loadBannedIps() { - + if(bannedIpsFile != null) { - - try { - HashSet newBannedIpList = new HashSet(); - - // TODO: optimize this - BufferedReader in = new BufferedReader(new FileReader(this.bannedIpsFile)); - + + // TODO: optimize this + try (BufferedReader in = new BufferedReader(new FileReader(this.bannedIpsFile))) { + Set<String> newBannedIpList = newThreadSafeSet(); + String ip = null; while((ip = in.readLine()) != null) { newBannedIpList.add(ip); } - - in.close(); - + // list updated, reset modified file this.bannedIps = newBannedIpList; this.bannedIpsFile.clearChanged(); - + log.info(this.bannedIps.size()+" banned ips loaded"); } catch(Exception ex) { - log.error("Error loading banned ips from file", ex); + log.error("Error loading banned ips from file", ex); } - + } } - - - // a simple extension to the File class which tracks if the file has + + + // a simple extension to the File class which tracks if the file has // changed since the last time we checked - private class ModifiedFile extends java.io.File { - + private static class ModifiedFile extends java.io.File { + private long myLastModified = 0; - + public ModifiedFile(String filePath) { super(filePath); - + this.myLastModified = lastModified(); } public boolean hasChanged() { return lastModified() != myLastModified; } - + public void clearChanged() { myLastModified = lastModified(); } } - -} + + private static <T> Set<T> newThreadSafeSet() { + return ConcurrentHashMap.newKeySet(); + } +} \ No newline at end of file diff --git a/app/src/test/java/org/apache/roller/weblogger/util/IPBanListTest.java b/app/src/test/java/org/apache/roller/weblogger/util/IPBanListTest.java new file mode 100644 index 0000000..745664e --- /dev/null +++ b/app/src/test/java/org/apache/roller/weblogger/util/IPBanListTest.java @@ -0,0 +1,113 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. The ASF licenses this file to You + * under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. For additional information regarding + * copyright in this work, please see the NOTICE file in the top level + * directory of this distribution. + */ + +package org.apache.roller.weblogger.util; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.io.UncheckedIOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class IPBanListTest { + + @TempDir + Path tmpDir; + Path ipBanListPath; + IPBanList ipBanList; + + @BeforeEach + void setUp() throws IOException { + ipBanListPath = tmpDir.resolve("ipbanlist.txt"); + Files.createFile(ipBanListPath); + ipBanList = new IPBanList(() -> ipBanListPath.toAbsolutePath().toString()); + } + + @Test + @DisplayName("addBanned() adds the given IP address to the file") + void addBannedAddsToFile() { + ipBanList.addBannedIp("10.0.0.1"); + + List<String> ipBanList = readIpBanList(); + assertTrue(ipBanList.contains("10.0.0.1")); + assertEquals(1, ipBanList.size()); + } + + @Test + @DisplayName("addBanned() ignores nulls") + void addBannedIgnoresNulls() { + ipBanList.addBannedIp(null); + + assertTrue(readIpBanList().isEmpty()); + } + + @Test + @DisplayName("isBanned() returns true if the given IP address is banned") + void isBanned() { + ipBanList.addBannedIp("10.0.0.1"); + try { // work around for intermittently failing test + Thread.sleep(500); + } catch (InterruptedException ignored) {} + assertTrue(ipBanList.isBanned("10.0.0.1")); + } + + @Test + @DisplayName("isBanned() returns false if the given IP address it not banned") + void isBanned2() { + assertFalse(ipBanList.isBanned("10.0.0.1")); + } + + @Test + @DisplayName("isBanned() returns false if the given IP address is null") + void isBanned3() { + assertFalse(ipBanList.isBanned(null)); + } + + @Test + @DisplayName("isBanned() reads the file if needed") + void isBanned4() { + writeIpBanList("10.0.0.1"); + + assertTrue(ipBanList.isBanned("10.0.0.1")); + } + + private void writeIpBanList(String ipAddress) { + try { + Files.writeString(ipBanListPath, ipAddress); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + + private List<String> readIpBanList() { + try { + return Files.readAllLines(ipBanListPath); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } +} \ No newline at end of file
