From 12376df5e3ca6824176309c5e09d7a0bf2604d6d Mon Sep 17 00:00:00 2001 From: "Mark H. Wood" Date: Tue, 15 Dec 2020 13:46:23 -0500 Subject: [PATCH 1/6] Better support IPv6 by using standard JRE lookup methods. #3094 Also avoid NPE when there is no current user. --- .../statistics/SolrLoggerServiceImpl.java | 47 ++++++++++--------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/statistics/SolrLoggerServiceImpl.java b/dspace-api/src/main/java/org/dspace/statistics/SolrLoggerServiceImpl.java index 14987aae10..696b051beb 100644 --- a/dspace-api/src/main/java/org/dspace/statistics/SolrLoggerServiceImpl.java +++ b/dspace-api/src/main/java/org/dspace/statistics/SolrLoggerServiceImpl.java @@ -18,6 +18,7 @@ import java.io.UnsupportedEncodingException; import java.net.InetAddress; import java.net.URI; import java.net.URLEncoder; +import java.net.UnknownHostException; import java.nio.charset.StandardCharsets; import java.nio.file.Path; import java.nio.file.Paths; @@ -32,6 +33,7 @@ import java.util.EnumSet; import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Set; import javax.servlet.http.HttpServletRequest; @@ -95,7 +97,6 @@ import org.dspace.eperson.Group; import org.dspace.service.ClientInfoService; import org.dspace.services.ConfigurationService; import org.dspace.statistics.service.SolrLoggerService; -import org.dspace.statistics.util.DnsLookup; import org.dspace.statistics.util.LocationUtils; import org.dspace.statistics.util.SpiderDetector; import org.dspace.usage.UsageWorkflowEvent; @@ -263,8 +264,9 @@ public class SolrLoggerServiceImpl implements SolrLoggerService, InitializingBea } catch (RuntimeException re) { throw re; } catch (Exception e) { + String email = null == currentUser ? "[anonymous]" : currentUser.getEmail(); log.error("Error saving VIEW event to Solr for DSpaceObject {} by EPerson {}", - dspaceObject.getID(), currentUser.getEmail(), e); + dspaceObject.getID(), email, e); } } @@ -337,12 +339,13 @@ public class SolrLoggerServiceImpl implements SolrLoggerService, InitializingBea doc1.addField("referrer", request.getHeader("referer")); } + InetAddress ipAddress = null; try { - String dns = DnsLookup.reverseDns(ip); - doc1.addField("dns", dns.toLowerCase()); - } catch (Exception e) { - log.info("Failed DNS Lookup for IP:" + ip); - log.debug(e.getMessage(), e); + ipAddress = InetAddress.getByName(ip); + String dns = ipAddress.getHostName(); + doc1.addField("dns", dns.toLowerCase(Locale.ROOT)); + } catch (UnknownHostException e) { + log.info("Failed DNS resolution of address {}", ip, e); } if (request.getHeader("User-Agent") != null) { doc1.addField("userAgent", request.getHeader("User-Agent")); @@ -350,9 +353,8 @@ public class SolrLoggerServiceImpl implements SolrLoggerService, InitializingBea doc1.addField("isBot", isSpiderBot); // Save the location information if valid, save the event without // location information if not valid - if (locationService != null) { + if (locationService != null && ipAddress != null) { try { - InetAddress ipAddress = InetAddress.getByName(ip); CityResponse location = locationService.city(ipAddress); String countryCode = location.getCountry().getIsoCode(); double latitude = location.getLocation().getLatitude(); @@ -366,16 +368,17 @@ public class SolrLoggerServiceImpl implements SolrLoggerService, InitializingBea doc1.addField("continent", LocationUtils .getContinentCode(countryCode)); } catch (Exception e) { - System.out - .println("COUNTRY ERROR: " + countryCode); + log.warn("Failed to load country/continent table: {}", countryCode); } doc1.addField("countryCode", countryCode); doc1.addField("city", location.getCity().getName()); doc1.addField("latitude", latitude); doc1.addField("longitude", longitude); } - } catch (IOException | GeoIp2Exception e) { - log.error("Unable to get location of request: {}", e.getMessage()); + } catch (IOException e) { + log.warn("GeoIP lookup failed.", e); + } catch (GeoIp2Exception e) { + log.info("Unable to get location of request: {}", e.getMessage()); } } } @@ -408,12 +411,13 @@ public class SolrLoggerServiceImpl implements SolrLoggerService, InitializingBea ip = clientInfoService.getClientIp(ip, xforwardedfor); doc1.addField("ip", ip); + InetAddress ipAddress = null; try { - String dns = DnsLookup.reverseDns(ip); - doc1.addField("dns", dns.toLowerCase()); - } catch (Exception e) { - log.info("Failed DNS Lookup for IP:" + ip); - log.debug(e.getMessage(), e); + ipAddress = InetAddress.getByName(ip); + String dns = ipAddress.getHostName(); + doc1.addField("dns", dns.toLowerCase(Locale.ROOT)); + } catch (UnknownHostException e) { + log.info("Failed DNS resolution of address {}", ip, e); } if (userAgent != null) { doc1.addField("userAgent", userAgent); @@ -423,7 +427,6 @@ public class SolrLoggerServiceImpl implements SolrLoggerService, InitializingBea // location information if not valid if (locationService != null) { try { - InetAddress ipAddress = InetAddress.getByName(ip); CityResponse location = locationService.city(ipAddress); String countryCode = location.getCountry().getIsoCode(); double latitude = location.getLocation().getLatitude(); @@ -445,8 +448,10 @@ public class SolrLoggerServiceImpl implements SolrLoggerService, InitializingBea doc1.addField("latitude", latitude); doc1.addField("longitude", longitude); } - } catch (GeoIp2Exception | IOException e) { - log.error("Unable to get location of request: {}", e.getMessage()); + } catch (IOException e) { + log.warn("GeoIP lookup failed.", e); + } catch (GeoIp2Exception e) { + log.info("Unable to get location of request: {}", e.getMessage()); } } From dcd9d2e9a5fc1488fddbfdec1c927b05af76e604 Mon Sep 17 00:00:00 2001 From: "Mark H. Wood" Date: Tue, 15 Dec 2020 13:50:11 -0500 Subject: [PATCH 2/6] Work-around: treat IPv6 address as non-match, not format error. #3094 Also improve documentation. This class should be reworked to properly support IPv6. --- .../org/dspace/statistics/util/IPTable.java | 43 +++++++++++++------ 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/statistics/util/IPTable.java b/dspace-api/src/main/java/org/dspace/statistics/util/IPTable.java index 6833fd1792..b33da174bd 100644 --- a/dspace-api/src/main/java/org/dspace/statistics/util/IPTable.java +++ b/dspace-api/src/main/java/org/dspace/statistics/util/IPTable.java @@ -12,6 +12,9 @@ import java.util.HashSet; import java.util.Map; import java.util.Set; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + /** * A Spare v4 IPTable implementation that uses nested HashMaps * to optimize IP address matching over ranges of IP addresses. @@ -19,13 +22,23 @@ import java.util.Set; * @author mdiggory at atmire.com */ public class IPTable { + private static final Logger log = LogManager.getLogger(IPTable.class); /* A lookup tree for IP addresses and SubnetRanges */ - private Map>>> map = - new HashMap>>>(); + private final Map>>> map + = new HashMap<>(); /** - * Can be full v4 IP, subnet or range string + * Can be full v4 IP, subnet or range string. + *
    + *
  • A full address is a complete dotted-quad: {@code "1.2.3.4".} + *
  • A subnet is a dotted-triplet: {@code "1.2.3"}. It means an entire + * Class C subnet: "1.2.3.0-1.2.3.255". + *
  • A range is two dotted-quad addresses separated by hyphen: + * {@code "1.2.3.4-1.2.3.14"}. Only the final octet may be different. + *
+ * + * Any attempt at CIDR notation is ignored. * * @param ip IP address(es) * @throws IPFormatException Exception Class to deal with IPFormat errors. @@ -59,7 +72,6 @@ public class IPTable { if (subnets.length < 3) { throw new IPFormatException(ip + " - require at least three subnet places (255.255.255.0"); - } start = subnets; @@ -67,27 +79,24 @@ public class IPTable { } if (start.length >= 3) { - - Map>> first = map.get(start[0]); if (first == null) { - first = new HashMap>>(); + first = new HashMap<>(); map.put(start[0], first); } Map> second = first.get(start[1]); - if (second == null) { - second = new HashMap>(); + second = new HashMap<>(); first.put(start[1], second); } Set third = second.get(start[2]); if (third == null) { - third = new HashSet(); + third = new HashSet<>(); second.put(start[2], third); } @@ -115,14 +124,22 @@ public class IPTable { * Check whether a given address is contained in this netblock. * * @param ip the address to be tested - * @return true if {@code ip} is within this table's limits + * @return true if {@code ip} is within this table's limits. Returns false + * if {@link ip} looks like an IPv6 address. * @throws IPFormatException Exception Class to deal with IPFormat errors. */ public boolean contains(String ip) throws IPFormatException { String[] subnets = ip.split("\\."); - if (subnets.length != 4) { + // Does it look like IPv6? + if (subnets.length > 4 || ip.contains("::")) { + log.warn("Address {} assumed not to match. IPv6 is not implemented.", ip); + return false; + } + + // Does it look like a subnet? + if (subnets.length < 4) { throw new IPFormatException("needs to be a single IP address"); } @@ -154,7 +171,7 @@ public class IPTable { * @return this table's content as a Set */ public Set toSet() { - HashSet set = new HashSet(); + HashSet set = new HashSet<>(); for (Map.Entry>>> first : map.entrySet()) { String firstString = first.getKey(); From 654412eb889df486d63e51aa390baef10c53b045 Mon Sep 17 00:00:00 2001 From: "Mark H. Wood" Date: Thu, 28 Jan 2021 16:58:46 -0500 Subject: [PATCH 3/6] Add tests. #3094 --- .../dspace/statistics/util/IPTableTest.java | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 dspace-api/src/test/java/org/dspace/statistics/util/IPTableTest.java diff --git a/dspace-api/src/test/java/org/dspace/statistics/util/IPTableTest.java b/dspace-api/src/test/java/org/dspace/statistics/util/IPTableTest.java new file mode 100644 index 0000000000..8e9c9c81a6 --- /dev/null +++ b/dspace-api/src/test/java/org/dspace/statistics/util/IPTableTest.java @@ -0,0 +1,78 @@ +package org.dspace.statistics.util; + +import org.dspace.statistics.util.IPTable.IPFormatException; +import org.junit.After; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; +import static org.junit.Assert.*; +import org.junit.Ignore; + +/** + * + * @author mwood + */ +public class IPTableTest { + + public IPTableTest() { + } + + @BeforeClass + public static void setUpClass() { + } + + @AfterClass + public static void tearDownClass() { + } + + @Before + public void setUp() { + } + + @After + public void tearDown() { + } + + /** + * Test of add method, of class IPTable. + * @throws java.lang.Exception passed through. + */ + @Ignore + @Test + public void testAdd() throws Exception { + } + + /** + * Test of contains method, of class IPTable. + * @throws java.lang.Exception passed through. + */ + @Test(expected=IPFormatException.class) + public void testContains() throws Exception { + String localhost = "127.0.0.1"; + IPTable instance = new IPTable(); + instance.add(localhost); + boolean contains; + + contains = instance.contains(localhost); + assertTrue("Address that was add()ed should match", contains); + + contains = instance.contains("192.168.1.1"); + assertFalse("Address that was not add()ed should not match", contains); + + contains = instance.contains("fec0:0:0:1::2"); + assertFalse("IPv6 address should not match anything.", contains); + + // This should throw an IPFormatException. + contains = instance.contains("axolotl"); + assertFalse("Nonsense string should raise an exception.", contains); + } + + /** + * Test of toSet method, of class IPTable. + */ + @Ignore + @Test + public void testToSet() { + } +} From 945ee9af7696fd7bc1de4cbd5850e7aad8527725 Mon Sep 17 00:00:00 2001 From: "Mark H. Wood" Date: Fri, 29 Jan 2021 10:37:14 -0500 Subject: [PATCH 4/6] Satisfy checkstyle. #3094 --- .../org/dspace/statistics/util/IPTableTest.java | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/dspace-api/src/test/java/org/dspace/statistics/util/IPTableTest.java b/dspace-api/src/test/java/org/dspace/statistics/util/IPTableTest.java index 8e9c9c81a6..123e6b8a1a 100644 --- a/dspace-api/src/test/java/org/dspace/statistics/util/IPTableTest.java +++ b/dspace-api/src/test/java/org/dspace/statistics/util/IPTableTest.java @@ -1,13 +1,22 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ package org.dspace.statistics.util; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + import org.dspace.statistics.util.IPTable.IPFormatException; import org.junit.After; import org.junit.AfterClass; import org.junit.Before; import org.junit.BeforeClass; -import org.junit.Test; -import static org.junit.Assert.*; import org.junit.Ignore; +import org.junit.Test; /** * @@ -47,7 +56,7 @@ public class IPTableTest { * Test of contains method, of class IPTable. * @throws java.lang.Exception passed through. */ - @Test(expected=IPFormatException.class) + @Test(expected = IPFormatException.class) public void testContains() throws Exception { String localhost = "127.0.0.1"; IPTable instance = new IPTable(); From 75aa37dfb6ebd532bf4f8924dd025d42dfd14372 Mon Sep 17 00:00:00 2001 From: "Mark H. Wood" Date: Fri, 29 Jan 2021 13:54:37 -0500 Subject: [PATCH 5/6] Move exception case to a separate test method. #3094 This now properly shows that querying an IPv6 address does not raise an exception. --- .../dspace/statistics/util/IPTableTest.java | 23 +++++++++++++++---- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/dspace-api/src/test/java/org/dspace/statistics/util/IPTableTest.java b/dspace-api/src/test/java/org/dspace/statistics/util/IPTableTest.java index 123e6b8a1a..0fe93af256 100644 --- a/dspace-api/src/test/java/org/dspace/statistics/util/IPTableTest.java +++ b/dspace-api/src/test/java/org/dspace/statistics/util/IPTableTest.java @@ -23,6 +23,7 @@ import org.junit.Test; * @author mwood */ public class IPTableTest { + private static final String LOCALHOST = "127.0.0.1"; public IPTableTest() { } @@ -56,14 +57,14 @@ public class IPTableTest { * Test of contains method, of class IPTable. * @throws java.lang.Exception passed through. */ - @Test(expected = IPFormatException.class) - public void testContains() throws Exception { - String localhost = "127.0.0.1"; + @Test + public void testContains() + throws Exception { IPTable instance = new IPTable(); - instance.add(localhost); + instance.add(LOCALHOST); boolean contains; - contains = instance.contains(localhost); + contains = instance.contains(LOCALHOST); assertTrue("Address that was add()ed should match", contains); contains = instance.contains("192.168.1.1"); @@ -71,6 +72,18 @@ public class IPTableTest { contains = instance.contains("fec0:0:0:1::2"); assertFalse("IPv6 address should not match anything.", contains); + } + + /** + * Test of contains method when presented with an invalid address. + * @throws Exception passed through. + */ + @Test(expected = IPFormatException.class) + public void testContainsBadFormat() + throws Exception { + IPTable instance = new IPTable(); + instance.add(LOCALHOST); + boolean contains; // This should throw an IPFormatException. contains = instance.contains("axolotl"); From c2a1287771757541c46517b3142769add9d9a6b6 Mon Sep 17 00:00:00 2001 From: "Mark H. Wood" Date: Mon, 8 Feb 2021 16:03:14 -0500 Subject: [PATCH 6/6] Restore Locale lost while merging with 'main'. #3094 --- .../java/org/dspace/statistics/SolrLoggerServiceImpl.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/statistics/SolrLoggerServiceImpl.java b/dspace-api/src/main/java/org/dspace/statistics/SolrLoggerServiceImpl.java index b81d98bb73..910845e77a 100644 --- a/dspace-api/src/main/java/org/dspace/statistics/SolrLoggerServiceImpl.java +++ b/dspace-api/src/main/java/org/dspace/statistics/SolrLoggerServiceImpl.java @@ -336,7 +336,7 @@ public class SolrLoggerServiceImpl implements SolrLoggerService, InitializingBea } else { dns = configurationService.getProperty("anonymize_statistics.dns_mask", "anonymized"); } - doc1.addField("dns", dns.toLowerCase()); + doc1.addField("dns", dns.toLowerCase(Locale.ROOT)); } catch (UnknownHostException e) { log.info("Failed DNS Lookup for IP: {}", ip); log.debug(e.getMessage(), e); @@ -422,7 +422,7 @@ public class SolrLoggerServiceImpl implements SolrLoggerService, InitializingBea } else { dns = configurationService.getProperty("anonymize_statistics.dns_mask", "anonymized"); } - doc1.addField("dns", dns.toLowerCase()); + doc1.addField("dns", dns.toLowerCase(Locale.ROOT)); } catch (UnknownHostException e) { log.info("Failed DNS Lookup for IP: {}", ip); log.debug(e.getMessage(), e);