From 3ca69767ba433aeedd662f8dd5585ce7cb2836a3 Mon Sep 17 00:00:00 2001 From: Henry Kuijpers Date: Wed, 2 Jun 2021 18:43:47 +0200 Subject: [PATCH 1/9] SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions --- .../mapping/MapConfigurationProvider.java | 1 - .../impl/mapping/MapEntries.java | 149 +++++++++++------- .../mapping/InMemoryResourceProvider.java | 10 +- .../impl/mapping/MapEntriesTest.java | 35 ++-- 4 files changed, 116 insertions(+), 79 deletions(-) diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapConfigurationProvider.java b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapConfigurationProvider.java index c8733a93..956b7de4 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapConfigurationProvider.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapConfigurationProvider.java @@ -83,7 +83,6 @@ public int compareTo(VanityPathConfig o2) { /** * A set of allow prefixes all ending with a slash. * If empty set is returned, all paths are allowed. - * @return */ Set getAllowedAliasLocations(); } diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java index 42408759..2a74832c 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java @@ -20,7 +20,11 @@ import org.apache.commons.lang3.StringUtils; import org.apache.sling.api.SlingConstants; -import org.apache.sling.api.resource.*; +import org.apache.sling.api.resource.LoginException; +import org.apache.sling.api.resource.Resource; +import org.apache.sling.api.resource.ResourceResolver; +import org.apache.sling.api.resource.ResourceUtil; +import org.apache.sling.api.resource.ValueMap; import org.apache.sling.api.resource.observation.ExternalResourceChangeListener; import org.apache.sling.api.resource.observation.ResourceChange; import org.apache.sling.api.resource.observation.ResourceChangeListener; @@ -36,17 +40,40 @@ import org.slf4j.LoggerFactory; import javax.servlet.http.HttpServletResponse; -import java.io.*; +import java.io.DataInputStream; +import java.io.File; +import java.io.FileInputStream; +import java.io.FileOutputStream; +import java.io.IOException; import java.net.MalformedURLException; import java.net.URL; -import java.util.*; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.Dictionary; +import java.util.HashMap; +import java.util.Hashtable; +import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; import java.util.Map.Entry; +import java.util.NoSuchElementException; +import java.util.Optional; +import java.util.Set; +import java.util.SortedMap; +import java.util.Timer; +import java.util.TimerTask; +import java.util.TreeMap; +import java.util.TreeSet; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.locks.ReentrantLock; import java.util.function.Supplier; +import java.util.stream.Collectors; import java.util.stream.Stream; @@ -86,9 +113,11 @@ public class MapEntries implements public static final int DEFAULT_DEFAULT_VANITY_PATH_REDIRECT_STATUS = HttpServletResponse.SC_FOUND; - private static final String JCR_SYSTEM_PREFIX = "/jcr:system/"; + private static final String JCR_SYSTEM_PATH = "/jcr:system"; - static final String ALIAS_BASE_QUERY_DEFAULT = "SELECT sling:alias FROM nt:base AS page"; + private static final String VANITY_PATH_BASE_QUERY_DEFAULT = "SELECT sling:vanityPath, sling:redirect, sling:redirectStatus FROM nt:base AS page"; + + static final String ALIAS_BASE_QUERY_DEFAULT = "SELECT sling:alias FROM nt:base AS page"; static final String ANY_SCHEME_HOST = "[^/]+/[^/]+"; @@ -707,7 +736,7 @@ public void onChange(final List changes) { log.debug("onChange, type={}, path={}", rc.getType(), path); // don't care for system area - if (path.startsWith(JCR_SYSTEM_PREFIX)) { + if (path.startsWith(JCR_SYSTEM_PATH + '/')) { continue; } @@ -814,9 +843,16 @@ private Map> getVanityPaths(String vanityPath) { Map> entryMap = new HashMap<>(); - // sling:vanityPath (lowercase) is the property name - final String queryString = "SELECT sling:vanityPath, sling:redirect, sling:redirectStatus FROM nt:base WHERE sling:vanityPath =" - + "'"+escapeIllegalXpathSearchChars(vanityPath).replaceAll("'", "''")+"' OR sling:vanityPath ="+ "'"+escapeIllegalXpathSearchChars(vanityPath.substring(1)).replaceAll("'", "''")+"' ORDER BY sling:vanityOrder DESC"; + // sling:vanityPath (lowercase) is the property name + final String queryString = createVanityPathQuery() + + " AND (" + + Stream + .of(vanityPath, vanityPath.substring(1)) + .map(MapEntries::escapeIllegalXpathSearchChars) + .map(value -> StringUtils.replace(value, "'", "''")) + .map(escapedValue -> "sling:vanityPath = '" + escapedValue + "'") + .collect(Collectors.joining(" OR ")) + + ")"; ResourceResolver queryResolver = null; @@ -862,12 +898,6 @@ private boolean isValidVanityPath(final String path){ throw new IllegalArgumentException("Unexpected null path"); } - // ignore system tree - if (path.startsWith(JCR_SYSTEM_PREFIX)) { - log.debug("isValidVanityPath: not valid {}", path); - return false; - } - // check white list if ( this.factory.getVanityPathConfig() != null ) { boolean allowed = false; @@ -1019,7 +1049,7 @@ private boolean addEntry(final Map> entryMap, final Strin */ private Map> loadAliases(final ResourceResolver resolver) { final Map> map = new ConcurrentHashMap<>(); - final String queryString = updateAliasQuery(); + final String queryString = createAliasQuery(); final Iterator i = resolver.findResources(queryString, "sql"); while (i.hasNext()) { final Resource resource = i.next(); @@ -1032,34 +1062,33 @@ private Map> loadAliases(final ResourceResolver reso /* * Update alias query based on configured alias locations */ - private String updateAliasQuery(){ + private String createAliasQuery(){ final Set allowedLocations = this.factory.getAllowedAliasLocations(); - StringBuilder baseQuery = new StringBuilder(ALIAS_BASE_QUERY_DEFAULT); - baseQuery.append(" ").append("WHERE"); + StringBuilder query = new StringBuilder(ALIAS_BASE_QUERY_DEFAULT); + query.append(" ").append("WHERE"); if(allowedLocations.isEmpty()){ - String jcrSystemPath = StringUtils.removeEnd(JCR_SYSTEM_PREFIX, "/"); - baseQuery.append(" ").append("(").append("NOT ISDESCENDANTNODE(page,") - .append("\"").append(jcrSystemPath).append("\"").append("))"); + query.append(" ").append("(").append("NOT ISDESCENDANTNODE(page,") + .append("\"").append(JCR_SYSTEM_PATH).append("\"").append("))"); }else{ Iterator pathIterator = allowedLocations.iterator(); - baseQuery.append("("); + query.append("("); while(pathIterator.hasNext()){ String prefix = pathIterator.next(); - baseQuery.append(" ").append("ISDESCENDANTNODE(page,") + query.append(" ").append("ISDESCENDANTNODE(page,") .append("\"").append(prefix).append("\"") .append(")").append(" ").append("OR"); } //Remove last "OR" keyword - int orLastIndex = baseQuery.lastIndexOf("OR"); - baseQuery.delete(orLastIndex,baseQuery.length()); - baseQuery.append(")"); + int orLastIndex = query.lastIndexOf("OR"); + query.delete(orLastIndex,query.length()); + query.append(")"); } - baseQuery.append(" AND sling:alias IS NOT NULL "); - String aliasQuery = baseQuery.toString(); + query.append(" AND sling:alias IS NOT NULL"); + String aliasQuery = query.toString(); logger.debug("Query to fetch alias [{}] ", aliasQuery); return aliasQuery; @@ -1069,11 +1098,6 @@ private String updateAliasQuery(){ * Load alias given a resource */ private boolean loadAlias(final Resource resource, Map> map) { - // ignore system tree - if(resource.getPath() == null){ - throw new IllegalArgumentException("Unexpected null path"); - } - final String resourceName; final String parentPath; if (JCR_CONTENT.equals(resource.getName())) { @@ -1155,7 +1179,7 @@ private boolean loadAlias(final Resource resource, Map> loadVanityPaths(boolean createVanityBloomFilter) { // sling:vanityPath (lowercase) is the property name final Map > targetPaths = new ConcurrentHashMap <>(); - final String queryString = "SELECT sling:vanityPath, sling:redirect, sling:redirectStatus FROM nt:base WHERE sling:vanityPath IS NOT NULL"; + final String queryString = createVanityPathQuery(); final Iterator i = resolver.findResources(queryString, "sql"); Supplier isCacheComplete = () -> isAllVanityPathEntriesCached() || vanityCounter.longValue() < this.factory.getMaxCachedVanityPathEntries(); @@ -1174,11 +1198,6 @@ private Map > loadVanityPaths(boolean createVanityBloomFilt * Load vanity path given a resource */ private boolean loadVanityPath(final Resource resource, final Map> entryMap, final Map > targetPaths, boolean addToCache, boolean newVanity) { - - if (!isValidVanityPath(resource.getPath())) { - return false; - } - final ValueMap props = resource.getValueMap(); long vanityOrder = props.get(PROP_VANITY_ORDER, 0L); @@ -1255,6 +1274,33 @@ private boolean loadVanityPath(final Resource resource, final Map restriction.length() != 0) + .map(restriction -> " AND (" + restriction + ")") + .collect(Collectors.joining()); + logger.debug("Query to fetch vanity paths [{}] ", query); + return query; + } + + private String createVanityPathQueryPathRestriction(final boolean include) { + return Stream.concat( + Stream.of(new VanityPathConfig(JCR_SYSTEM_PATH, true)), + Optional.ofNullable(this.factory.getVanityPathConfig()).map(List::stream).orElseGet(Stream::empty) + ) + .filter(cfg -> cfg.isExclude != include) + .map(vanityPathConfig -> String.format( + "%sISDESCENDANTNODE(page, '%s')", + vanityPathConfig.isExclude ? "NOT " : StringUtils.EMPTY, + StringUtils.removeEnd(vanityPathConfig.prefix, String.valueOf('/')))) + .collect(Collectors.joining(include ? " OR " : " AND ")); + } + private void updateTargetPaths(final Map> targetPaths, final String key, final String entry) { if (entry == null) { return; @@ -1288,7 +1334,7 @@ private String[] getVanityPathDefinition(final String pVanityPath) { log.warn("Ignoring malformed vanity path {}", pVanityPath); } } else { - prefix = "^" + ANY_SCHEME_HOST; + prefix = StringUtils.EMPTY; if (!info.startsWith("/")) { path = "/" + info; } else { @@ -1322,7 +1368,7 @@ private void loadConfiguration(final MapConfigurationProvider factory, final Lis // this regular expression must match the whole URL !! final String url = "^" + ANY_SCHEME_HOST + extPath + "$"; final String redirect = intPath; - MapEntry mapEntry = getMapEntry(url, -1, false, redirect); + MapEntry mapEntry = getMapEntry(url, -1, false, 0, redirect); if (mapEntry!=null){ entries.add(mapEntry); } @@ -1350,7 +1396,7 @@ private void loadConfiguration(final MapConfigurationProvider factory, final Lis } for (final Entry> entry : map.entrySet()) { - MapEntry mapEntry = getMapEntry(ANY_SCHEME_HOST + entry.getKey(), -1, false, entry.getValue().toArray(new String[0])); + MapEntry mapEntry = getMapEntry(ANY_SCHEME_HOST + entry.getKey(), -1, false, 0, entry.getValue().toArray(new String[0])); if (mapEntry!=null){ entries.add(mapEntry); } @@ -1393,13 +1439,13 @@ private void loadMapConfiguration(final MapConfigurationProvider factory, final private void addMapEntry(final Map entries, final String path, final String url, final int status) { MapEntry entry = entries.get(path); if (entry == null) { - entry = getMapEntry(path, status, false, url); + entry = getMapEntry(path, status, false, 0, url); } else { final String[] redir = entry.getRedirect(); final String[] newRedir = new String[redir.length + 1]; System.arraycopy(redir, 0, newRedir, 0, redir.length); newRedir[redir.length] = url; - entry = getMapEntry(entry.getPattern(), entry.getStatus(), false, newRedir); + entry = getMapEntry(entry.getPattern(), entry.getStatus(), false, 0, newRedir); } if (entry!=null){ entries.put(path, entry); @@ -1540,19 +1586,6 @@ private void seek() { this.nextSpecial = null; } } - }; - - private MapEntry getMapEntry(String url, final int status, final boolean trailingSlash, - final String... redirect){ - - MapEntry mapEntry = null; - try{ - mapEntry = new MapEntry(url, status, trailingSlash, 0, redirect); - }catch (IllegalArgumentException iae){ - //ignore this entry - log.debug("ignored entry due exception ",iae); - } - return mapEntry; } private MapEntry getMapEntry(String url, final int status, final boolean trailingSlash, long order, diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/InMemoryResourceProvider.java b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/InMemoryResourceProvider.java index 718ed55d..771683ef 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/InMemoryResourceProvider.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/InMemoryResourceProvider.java @@ -96,15 +96,15 @@ public String[] getSupportedLanguages(@NotNull ResolveContext ctx) { @Override public Iterator findResources(@NotNull ResolveContext ctx, String query, String language) { - - if ( "SELECT sling:alias FROM nt:base WHERE sling:alias IS NOT NULL".equals(query) ) { + + if ("SELECT sling:alias FROM nt:base AS page WHERE (NOT ISDESCENDANTNODE(page,\"/jcr:system\")) AND sling:alias IS NOT NULL".equals(query)) { return resourcesWithProperty(ctx, "sling:alias") .iterator(); } - - if ( "SELECT sling:vanityPath, sling:redirect, sling:redirectStatus FROM nt:base WHERE sling:vanityPath IS NOT NULL".equals(query) ) { + + if ("SELECT sling:vanityPath, sling:redirect, sling:redirectStatus FROM nt:base AS page WHERE sling:vanityPath IS NOT NULL AND (NOT ISDESCENDANTNODE(page, '/jcr:system'))".equals(query)) { return resourcesWithProperty(ctx, "sling:vanityPath") - .iterator(); + .iterator(); } throw new UnsupportedOperationException("Unsupported query: '" + query + "'"); diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java index 92310e45..66f003bf 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java @@ -370,26 +370,31 @@ public Iterator answer(InvocationOnMock invocation) throws Throwable { @Test public void test_vanity_path_registration_include_exclude() throws IOException { final String[] validPaths = {"/libs/somewhere", "/libs/a/b", "/foo/a", "/baa/a"}; - final String[] invalidPaths = {"/libs/denied/a", "/libs/denied/b/c", "/nowhere"}; final List resources = new ArrayList<>(); for(final String val : validPaths) { resources.add(getVanityPathResource(val)); } - for(final String val : invalidPaths) { - resources.add(getVanityPathResource(val)); - } - - - when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer(new Answer>() { - - @Override - public Iterator answer(InvocationOnMock invocation) throws Throwable { - if (invocation.getArguments()[0].toString().contains("sling:vanityPath")) { - return resources.iterator(); - } else { - return Collections. emptySet().iterator(); - } + when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer((Answer>) invocation -> { + if (("SELECT sling:vanityPath, sling:redirect, sling:redirectStatus FROM nt:base AS page" + + " WHERE sling:vanityPath IS NOT NULL" + + " AND" + + " (ISDESCENDANTNODE(page, '/redirectingVanityPath301')" + + " OR ISDESCENDANTNODE(page, '/vanityPathOnJcrContent')" + + " OR ISDESCENDANTNODE(page, '/redirectingVanityPath')" + + " OR ISDESCENDANTNODE(page, '/justVanityPath2')" + + " OR ISDESCENDANTNODE(page, '/justVanityPath')" + + " OR ISDESCENDANTNODE(page, '/badVanityPath')" + + " OR ISDESCENDANTNODE(page, '/libs')" + + " OR ISDESCENDANTNODE(page, '/foo')" + + " OR ISDESCENDANTNODE(page, '/baa')" + + ") AND " + + "(NOT ISDESCENDANTNODE(page, '/jcr:system')" + + " AND NOT ISDESCENDANTNODE(page, '/libs/denied')" + + ")").equals(invocation.getArguments()[0].toString())) { + return resources.iterator(); + } else { + return Collections.emptyIterator(); } }); From cdc3133e757a06208892b56f2aa3ff968d0f791f Mon Sep 17 00:00:00 2001 From: Henry Kuijpers Date: Wed, 2 Jun 2021 19:03:50 +0200 Subject: [PATCH 2/9] SLING-10447 Add @SuppressWarnings for content path constant --- .../apache/sling/resourceresolver/impl/mapping/MapEntries.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java index 2a74832c..fe9b37d5 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java @@ -113,6 +113,7 @@ public class MapEntries implements public static final int DEFAULT_DEFAULT_VANITY_PATH_REDIRECT_STATUS = HttpServletResponse.SC_FOUND; + @SuppressWarnings("java:S1075") // Content path private static final String JCR_SYSTEM_PATH = "/jcr:system"; private static final String VANITY_PATH_BASE_QUERY_DEFAULT = "SELECT sling:vanityPath, sling:redirect, sling:redirectStatus FROM nt:base AS page"; From d4eff20cfddd3fc1604f90a144da0f36cf8a6cff Mon Sep 17 00:00:00 2001 From: Henry Kuijpers Date: Wed, 2 Jun 2021 19:38:29 +0200 Subject: [PATCH 3/9] SLING-10447 Remove resource.resolver.map.observation setting, as it is now superseded by the alias, vanity & mapping root settings --- .../CommonResourceResolverFactoryImpl.java | 5 -- .../ResourceResolverFactoryActivator.java | 34 +++++------ .../impl/ResourceResolverFactoryConfig.java | 5 -- .../mapping/MapConfigurationProvider.java | 2 - .../impl/mapping/MapEntries.java | 56 ++++++++++--------- .../impl/EtcMappingResourceResolverTest.java | 1 - .../impl/MockedResourceResolverImplTest.java | 5 -- .../AbstractMappingMapEntriesTest.java | 1 - .../mapping/EtcMappingMapEntriesTest.java | 1 - 9 files changed, 44 insertions(+), 66 deletions(-) diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/CommonResourceResolverFactoryImpl.java b/src/main/java/org/apache/sling/resourceresolver/impl/CommonResourceResolverFactoryImpl.java index d9b6c083..6749429a 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/CommonResourceResolverFactoryImpl.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/CommonResourceResolverFactoryImpl.java @@ -438,11 +438,6 @@ public boolean hasVanityPathPrecedence() { return this.activator.hasVanityPathPrecedence(); } - @Override - public Path[] getObservationPaths() { - return this.activator.getObservationPaths(); - } - @Override public List getVanityPathConfig() { final String[] includes = this.activator.getVanityPathWhiteList(); diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java index f2feca69..601df116 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java @@ -18,20 +18,14 @@ */ package org.apache.sling.resourceresolver.impl; -import java.lang.reflect.InvocationHandler; -import java.lang.reflect.Method; -import java.lang.reflect.Proxy; -import java.util.*; - - import org.apache.commons.collections4.BidiMap; import org.apache.commons.collections4.bidimap.TreeBidiMap; import org.apache.commons.lang3.StringUtils; import org.apache.sling.api.resource.ResourceDecorator; import org.apache.sling.api.resource.ResourceResolverFactory; -import org.apache.sling.api.resource.path.Path; import org.apache.sling.api.resource.runtime.RuntimeService; import org.apache.sling.resourceresolver.impl.helper.ResourceDecoratorTracker; +import org.apache.sling.resourceresolver.impl.mapping.MapEntries; import org.apache.sling.resourceresolver.impl.mapping.Mapping; import org.apache.sling.resourceresolver.impl.mapping.StringInterpolationProvider; import org.apache.sling.resourceresolver.impl.observation.ResourceChangeListenerWhiteboard; @@ -56,6 +50,19 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Dictionary; +import java.util.HashSet; +import java.util.Hashtable; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.TreeSet; + /** * The ResourceResolverFactoryActivator/code> keeps track of required services for the * resource resolver factory. @@ -132,9 +139,6 @@ private static final class FactoryRegistration { /** Vanity path blacklist */ private volatile String[] vanityPathBlackList; - /** Observation paths */ - private volatile Path[] observationPaths; - private final FactoryPreconditions preconds = new FactoryPreconditions(); /** Factory registration. */ @@ -239,10 +243,6 @@ public boolean shouldLogResourceResolverClosing() { return this.config.resource_resolver_log_closing(); } - public Path[] getObservationPaths() { - return this.observationPaths; - } - // ---------- SCR Integration --------------------------------------------- /** @@ -300,12 +300,6 @@ protected void activate(final BundleContext bundleContext, final ResourceResolve mapRoot = config.resource_resolver_map_location(); mapRootPrefix = mapRoot + '/'; - final String[] paths = config.resource_resolver_map_observation(); - this.observationPaths = new Path[paths.length]; - for(int i=0;i getVirtualURLMap(); Mapping[] getMappings(); diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java index fe9b37d5..eeaa14db 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java @@ -28,7 +28,6 @@ import org.apache.sling.api.resource.observation.ExternalResourceChangeListener; import org.apache.sling.api.resource.observation.ResourceChange; import org.apache.sling.api.resource.observation.ResourceChangeListener; -import org.apache.sling.api.resource.path.Path; import org.apache.sling.resourceresolver.impl.ResourceResolverImpl; import org.apache.sling.resourceresolver.impl.mapping.MapConfigurationProvider.VanityPathConfig; import org.osgi.framework.BundleContext; @@ -48,11 +47,11 @@ import java.net.MalformedURLException; import java.net.URL; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.Dictionary; import java.util.HashMap; +import java.util.HashSet; import java.util.Hashtable; import java.util.Iterator; import java.util.LinkedHashMap; @@ -173,12 +172,29 @@ public MapEntries(final MapConfigurationProvider factory, final BundleContext bu this.useOptimizeAliasResolution = doInit(); final Dictionary props = new Hashtable<>(); // NOSONAR - required by OSGi APIs - final String[] paths = new String[factory.getObservationPaths().length]; - for(int i=0 ; i < paths.length; i++) { - paths[i] = factory.getObservationPaths()[i].getPath(); + final Set paths = new HashSet<>(); + if (factory.getAllowedAliasLocations().isEmpty()) { + paths.add(String.valueOf('/')); + } else { + paths.addAll(factory.getAllowedAliasLocations()); + } + if (factory.isVanityPathEnabled()) { + final List vanityPathLocations = Optional.ofNullable(factory.getVanityPathConfig()).orElseGet(Collections::emptyList) + .stream() + .filter(vanityPathConfig -> !vanityPathConfig.isExclude) + .map(vanityPathConfig -> StringUtils.removeEnd(vanityPathConfig.prefix, String.valueOf('/'))) + .collect(Collectors.toList()); + if (vanityPathLocations.isEmpty()) { + paths.add(String.valueOf('/')); + } else { + paths.addAll(vanityPathLocations); + } + } + if (factory.getMapRoot() != null) { + paths.add(factory.getMapRoot()); } - props.put(ResourceChangeListener.PATHS, paths); - log.info("Registering for {}", Arrays.toString(factory.getObservationPaths())); + props.put(ResourceChangeListener.PATHS, paths.toArray(new String[0])); + log.info("Registering for {}", paths); props.put(Constants.SERVICE_DESCRIPTION, "Apache Sling Map Entries Observation"); props.put(Constants.SERVICE_VENDOR, "The Apache Software Foundation"); this.registration = bundleContext.registerService(ResourceChangeListener.class, this, props); @@ -862,21 +878,12 @@ private Map> getVanityPaths(String vanityPath) { final Iterator i = queryResolver.findResources(queryString, "sql"); while (i.hasNext()) { final Resource resource = i.next(); - boolean isValid = false; - for(final Path sPath : this.factory.getObservationPaths()) { - if ( sPath.matches(resource.getPath())) { - isValid = true; - break; - } - } - if ( isValid ) { - if (this.factory.isMaxCachedVanityPathEntriesStartup() || vanityCounter.longValue() < this.factory.getMaxCachedVanityPathEntries()) { - loadVanityPath(resource, resolveMapsMap, vanityTargets, true, false); - entryMap = resolveMapsMap; - } else { - final Map > targetPaths = new HashMap <>(); - loadVanityPath(resource, entryMap, targetPaths, true, false); - } + if (this.factory.isMaxCachedVanityPathEntriesStartup() || vanityCounter.longValue() < this.factory.getMaxCachedVanityPathEntries()) { + loadVanityPath(resource, resolveMapsMap, vanityTargets, true, false); + entryMap = resolveMapsMap; + } else { + final Map> targetPaths = new HashMap<>(); + loadVanityPath(resource, entryMap, targetPaths, true, false); } } } catch (LoginException e) { @@ -1186,10 +1193,7 @@ private Map > loadVanityPaths(boolean createVanityBloomFilt Supplier isCacheComplete = () -> isAllVanityPathEntriesCached() || vanityCounter.longValue() < this.factory.getMaxCachedVanityPathEntries(); while (i.hasNext() && (createVanityBloomFilter || isCacheComplete.get())) { final Resource resource = i.next(); - final String resourcePath = resource.getPath(); - if (Stream.of(this.factory.getObservationPaths()).anyMatch(path -> path.matches(resourcePath))) { - loadVanityPath(resource, resolveMapsMap, targetPaths, isCacheComplete.get(), createVanityBloomFilter); - } + loadVanityPath(resource, resolveMapsMap, targetPaths, isCacheComplete.get(), createVanityBloomFilter); } return targetPaths; diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/EtcMappingResourceResolverTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/EtcMappingResourceResolverTest.java index ca2d6a64..468b310b 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/EtcMappingResourceResolverTest.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/EtcMappingResourceResolverTest.java @@ -127,7 +127,6 @@ public void setup() throws Exception { setInaccessibleField("stringInterpolationProvider", activator, stringInterpolationProvider); setInaccessibleField("mapRoot", activator, "/etc/map"); setInaccessibleField("mapRootPrefix", activator, "/etc/map"); - setInaccessibleField("observationPaths", activator, new Path[] {new Path("/")}); ServiceUserMapper serviceUserMapper = mock(ServiceUserMapper.class); setInaccessibleField("serviceUserMapper", activator, serviceUserMapper); commonFactory = spy(new CommonResourceResolverFactoryImpl(activator)); diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/MockedResourceResolverImplTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/MockedResourceResolverImplTest.java index d0c7b848..f55a6c74 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/MockedResourceResolverImplTest.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/MockedResourceResolverImplTest.java @@ -249,11 +249,6 @@ public String[] resource_resolver_mapping() { "/content/:/", "/system/docroot/:/", "/content.html-/$" }; } - @Override - public String[] resource_resolver_map_observation() { - return new String[] {"/"}; - } - @Override public String resource_resolver_map_location() { return MapEntries.DEFAULT_MAP_ROOT; diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/AbstractMappingMapEntriesTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/AbstractMappingMapEntriesTest.java index 44e04a99..8be37e31 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/AbstractMappingMapEntriesTest.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/AbstractMappingMapEntriesTest.java @@ -106,7 +106,6 @@ public void setup() throws Exception { when(resourceResolverFactory.isVanityPathEnabled()).thenReturn(true); when(resourceResolverFactory.getVanityPathConfig()).thenReturn(configs); when(resourceResolverFactory.isOptimizeAliasResolutionEnabled()).thenReturn(true); - when(resourceResolverFactory.getObservationPaths()).thenReturn(new Path[] {new Path("/")}); when(resourceResolverFactory.getMapRoot()).thenReturn(MapEntries.DEFAULT_MAP_ROOT); when(resourceResolverFactory.getMaxCachedVanityPathEntries()).thenReturn(-1L); when(resourceResolverFactory.isMaxCachedVanityPathEntriesStartup()).thenReturn(true); diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/EtcMappingMapEntriesTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/EtcMappingMapEntriesTest.java index a98ca6ef..a8576374 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/EtcMappingMapEntriesTest.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/EtcMappingMapEntriesTest.java @@ -179,7 +179,6 @@ public Iterator listChildren(ResolveContext ctx, Resource pare when(activator.getBundleContext()).thenReturn(bundleContext); when(activator.getStringInterpolationProvider()).thenReturn(stringInterpolationProvider); when(activator.getMapRoot()).thenReturn("/etc/map"); - when(activator.getObservationPaths()).thenReturn(new Path[] {new Path("/")}); CommonResourceResolverFactoryImpl commonFactory = spy(new CommonResourceResolverFactoryImpl(activator)); when(bundleContext.getBundle()).thenReturn(bundle); ServiceUserMapper serviceUserMapper = mock(ServiceUserMapper.class); From f8bca4d982473d11e1a1bb281b6ba20939602516 Mon Sep 17 00:00:00 2001 From: Henry Kuijpers Date: Wed, 2 Jun 2021 19:57:49 +0200 Subject: [PATCH 4/9] SLING-10447 Remove resource.resolver.observation.paths --- .../sling/resourceresolver/impl/mapping/MapEntriesTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java index 66f003bf..e0149a97 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java @@ -110,7 +110,6 @@ public void setup() throws Exception { when(resourceResolverFactory.isVanityPathEnabled()).thenReturn(true); when(resourceResolverFactory.getVanityPathConfig()).thenReturn(configs); when(resourceResolverFactory.isOptimizeAliasResolutionEnabled()).thenReturn(true); - when(resourceResolverFactory.getObservationPaths()).thenReturn(new Path[] {new Path("/")}); when(resourceResolverFactory.getMapRoot()).thenReturn(MapEntries.DEFAULT_MAP_ROOT); when(resourceResolverFactory.getMaxCachedVanityPathEntries()).thenReturn(-1L); when(resourceResolverFactory.isMaxCachedVanityPathEntriesStartup()).thenReturn(true); From 5f4c04df819d79208bc20e8fded7744c7f9c8c66 Mon Sep 17 00:00:00 2001 From: Henry Kuijpers Date: Fri, 4 Jun 2021 13:56:50 +0200 Subject: [PATCH 5/9] SLING-10447 Separate blacklisted/whitelisted vanity paths into separate sets, make /jcr:system exclude default in ResourceResolverFactoryActivator, make query building code easier to read/understand --- .../CommonResourceResolverFactoryImpl.java | 55 ++++---- .../ResourceResolverFactoryActivator.java | 59 ++++----- .../mapping/MapConfigurationProvider.java | 38 +++--- .../impl/mapping/MapEntries.java | 76 ++++------- .../impl/EtcMappingResourceResolverTest.java | 5 - .../AbstractMappingMapEntriesTest.java | 6 - .../mapping/InMemoryResourceProvider.java | 2 +- .../impl/mapping/MapEntriesTest.java | 118 +++++++++--------- 8 files changed, 143 insertions(+), 216 deletions(-) diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/CommonResourceResolverFactoryImpl.java b/src/main/java/org/apache/sling/resourceresolver/impl/CommonResourceResolverFactoryImpl.java index 6749429a..831a3011 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/CommonResourceResolverFactoryImpl.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/CommonResourceResolverFactoryImpl.java @@ -18,19 +18,10 @@ */ package org.apache.sling.resourceresolver.impl; -import java.lang.ref.ReferenceQueue; -import java.lang.ref.WeakReference; -import java.util.*; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.atomic.AtomicBoolean; - -import org.jetbrains.annotations.NotNull; - import org.apache.commons.collections4.BidiMap; import org.apache.sling.api.resource.LoginException; import org.apache.sling.api.resource.ResourceResolver; import org.apache.sling.api.resource.ResourceResolverFactory; -import org.apache.sling.api.resource.path.Path; import org.apache.sling.resourceresolver.impl.console.ResourceResolverWebConsolePlugin; import org.apache.sling.resourceresolver.impl.helper.ResourceDecoratorTracker; import org.apache.sling.resourceresolver.impl.helper.ResourceResolverControl; @@ -41,11 +32,25 @@ import org.apache.sling.resourceresolver.impl.providers.ResourceProviderTracker; import org.apache.sling.serviceusermapping.ServiceUserMapper; import org.apache.sling.spi.resource.provider.ResourceProvider; +import org.jetbrains.annotations.NotNull; import org.osgi.framework.Bundle; import org.osgi.framework.BundleContext; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.lang.ref.ReferenceQueue; +import java.lang.ref.WeakReference; +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.Stack; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; + /** * The CommonResourceResolverFactoryImpl is a singleton * implementing the shared/common functionality of all resource @@ -438,28 +443,6 @@ public boolean hasVanityPathPrecedence() { return this.activator.hasVanityPathPrecedence(); } - @Override - public List getVanityPathConfig() { - final String[] includes = this.activator.getVanityPathWhiteList(); - final String[] excludes = this.activator.getVanityPathBlackList(); - if ( includes == null && excludes == null ) { - return null; - } - final List configs = new ArrayList<>(); - if ( includes != null ) { - for(final String val : includes) { - configs.add(new VanityPathConfig(val, false)); - } - } - if ( excludes != null ) { - for(final String val : excludes) { - configs.add(new VanityPathConfig(val, true)); - } - } - Collections.sort(configs); - return configs; - } - @Override public Set getAllowedAliasLocations() { return this.activator.getAllowedAliasLocations(); @@ -511,6 +494,16 @@ public Map getServiceUserAuthenticationInfo(final String subServ return authenticationInfo; } + @Override + public @NotNull Set getVanityPathWhiteList() { + return activator.getVanityPathWhiteList(); + } + + @Override + public @NotNull Set getVanityPathBlackList() { + return activator.getVanityPathBlackList(); + } + /** * Extension of a weak reference to be able to get the control object * that is used for cleaning up. diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java index 601df116..288875dc 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java @@ -20,6 +20,7 @@ import org.apache.commons.collections4.BidiMap; import org.apache.commons.collections4.bidimap.TreeBidiMap; +import org.apache.commons.lang3.ArrayUtils; import org.apache.commons.lang3.StringUtils; import org.apache.sling.api.resource.ResourceDecorator; import org.apache.sling.api.resource.ResourceResolverFactory; @@ -33,6 +34,7 @@ import org.apache.sling.resourceresolver.impl.providers.ResourceProviderTracker.ChangeListener; import org.apache.sling.resourceresolver.impl.providers.RuntimeServiceImpl; import org.apache.sling.serviceusermapping.ServiceUserMapper; +import org.jetbrains.annotations.NotNull; import org.osgi.framework.Bundle; import org.osgi.framework.BundleContext; import org.osgi.framework.Constants; @@ -54,6 +56,7 @@ import java.lang.reflect.Method; import java.lang.reflect.Proxy; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.Dictionary; import java.util.HashSet; @@ -62,6 +65,8 @@ import java.util.Map; import java.util.Set; import java.util.TreeSet; +import java.util.stream.Collectors; +import java.util.stream.Stream; /** * The ResourceResolverFactoryActivator/code> keeps track of required services for the @@ -134,10 +139,10 @@ private static final class FactoryRegistration { private volatile Set allowedAliasLocations = Collections.emptySet(); /** Vanity path whitelist */ - private volatile String[] vanityPathWhiteList; + private volatile Set vanityPathWhiteList = Collections.emptySet(); /** Vanity path blacklist */ - private volatile String[] vanityPathBlackList; + private volatile Set vanityPathBlackList = Collections.emptySet(); private final FactoryPreconditions preconds = new FactoryPreconditions(); @@ -215,11 +220,13 @@ public boolean isLogUnclosedResourceResolvers() { return this.config.resource_resolver_log_unclosed(); } - public String[] getVanityPathWhiteList() { + @NotNull + public Set getVanityPathWhiteList() { return this.vanityPathWhiteList; } - public String[] getVanityPathBlackList() { + @NotNull + public Set getVanityPathBlackList() { return this.vanityPathBlackList; } @@ -321,41 +328,17 @@ protected void activate(final BundleContext bundleContext, final ResourceResolve } // vanity path white list - this.vanityPathWhiteList = null; - String[] vanityPathPrefixes = config.resource_resolver_vanitypath_whitelist(); - if ( vanityPathPrefixes != null ) { - final List prefixList = new ArrayList<>(); - for(final String value : vanityPathPrefixes) { - if ( value.trim().length() > 0 ) { - if ( value.trim().endsWith("/") ) { - prefixList.add(value.trim()); - } else { - prefixList.add(value.trim() + "/"); - } - } - } - if ( prefixList.size() > 0 ) { - this.vanityPathWhiteList = prefixList.toArray(new String[prefixList.size()]); - } - } + this.vanityPathWhiteList = Collections.unmodifiableSet(Arrays + .stream(ArrayUtils.nullToEmpty(config.resource_resolver_vanitypath_whitelist())) + .map(String::trim) + .map(value -> StringUtils.appendIfMissing(value, String.valueOf('/'))) + .collect(Collectors.toSet())); // vanity path black list - this.vanityPathBlackList = null; - vanityPathPrefixes = config.resource_resolver_vanitypath_blacklist(); - if ( vanityPathPrefixes != null ) { - final List prefixList = new ArrayList<>(); - for(final String value : vanityPathPrefixes) { - if ( value.trim().length() > 0 ) { - if ( value.trim().endsWith("/") ) { - prefixList.add(value.trim()); - } else { - prefixList.add(value.trim() + "/"); - } - } - } - if ( prefixList.size() > 0 ) { - this.vanityPathBlackList = prefixList.toArray(new String[prefixList.size()]); - } - } + this.vanityPathBlackList = Collections.unmodifiableSet(Stream.concat(Arrays + .stream(ArrayUtils.nullToEmpty(config.resource_resolver_vanitypath_blacklist())), Stream.of("/jcr:system")) + .map(String::trim) + .map(value -> StringUtils.appendIfMissing(value, String.valueOf('/'))) + .collect(Collectors.toSet())); // check for required property Set requiredResourceProvidersLegacy = getStringSet(config.resource_resolver_required_providers()); diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapConfigurationProvider.java b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapConfigurationProvider.java index b7e8935a..c1c200b9 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapConfigurationProvider.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapConfigurationProvider.java @@ -16,13 +16,12 @@ */ package org.apache.sling.resourceresolver.impl.mapping; -import java.util.List; -import java.util.Map; -import java.util.Set; - import org.apache.sling.api.resource.LoginException; import org.apache.sling.api.resource.ResourceResolverFactory; -import org.apache.sling.api.resource.path.Path; +import org.jetbrains.annotations.NotNull; + +import java.util.Map; +import java.util.Set; /** * Internal interface representing the additional methods @@ -56,27 +55,20 @@ public interface MapConfigurationProvider extends ResourceResolverFactory { boolean hasVanityPathPrecedence(); Map getServiceUserAuthenticationInfo(final String subServiceName) throws LoginException; - - public class VanityPathConfig implements Comparable { - public final boolean isExclude; - public final String prefix; - - public VanityPathConfig(final String prefix, final boolean isExclude) { - this.prefix = prefix; - this.isExclude = isExclude; - } - - @Override - public int compareTo(VanityPathConfig o2) { - return new Integer(o2.prefix.length()).compareTo(this.prefix.length()); - } - } /** - * A list of white and black list prefixes all ending with a slash. - * If null is returned, all paths are allowed. + * A set of whitelisted prefixes all ending with a slash. + * If empty set is returned, all paths are allowed. + */ + @NotNull + Set getVanityPathWhiteList(); + + /** + * A set of blacklisted prefixes all ending with a slash. + * If empty set is returned, all paths are allowed. */ - List getVanityPathConfig(); + @NotNull + Set getVanityPathBlackList(); /** * A set of allow prefixes all ending with a slash. diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java index eeaa14db..2b895f65 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java @@ -29,7 +29,6 @@ import org.apache.sling.api.resource.observation.ResourceChange; import org.apache.sling.api.resource.observation.ResourceChangeListener; import org.apache.sling.resourceresolver.impl.ResourceResolverImpl; -import org.apache.sling.resourceresolver.impl.mapping.MapConfigurationProvider.VanityPathConfig; import org.osgi.framework.BundleContext; import org.osgi.framework.Constants; import org.osgi.framework.ServiceRegistration; @@ -59,7 +58,6 @@ import java.util.Map; import java.util.Map.Entry; import java.util.NoSuchElementException; -import java.util.Optional; import java.util.Set; import java.util.SortedMap; import java.util.Timer; @@ -179,15 +177,11 @@ public MapEntries(final MapConfigurationProvider factory, final BundleContext bu paths.addAll(factory.getAllowedAliasLocations()); } if (factory.isVanityPathEnabled()) { - final List vanityPathLocations = Optional.ofNullable(factory.getVanityPathConfig()).orElseGet(Collections::emptyList) - .stream() - .filter(vanityPathConfig -> !vanityPathConfig.isExclude) - .map(vanityPathConfig -> StringUtils.removeEnd(vanityPathConfig.prefix, String.valueOf('/'))) - .collect(Collectors.toList()); - if (vanityPathLocations.isEmpty()) { + final Set vanityPathWhiteList = factory.getVanityPathWhiteList(); + if (vanityPathWhiteList.isEmpty()) { paths.add(String.valueOf('/')); } else { - paths.addAll(vanityPathLocations); + paths.addAll(vanityPathWhiteList); } } if (factory.getMapRoot() != null) { @@ -317,7 +311,8 @@ private boolean addResource(final String path, final AtomicBoolean resolverRefre } private boolean updateResource(final String path, final AtomicBoolean resolverRefreshed) { - final boolean isValidVanityPath = this.isValidVanityPath(path); + // Blacklist cannot be configured with ResourceChangeListener, so do it here + final boolean isValidVanityPath = this.factory.getVanityPathBlackList().stream().noneMatch(path::startsWith); if ( this.useOptimizeAliasResolution || isValidVanityPath) { this.initializing.lock(); @@ -896,33 +891,6 @@ private Map> getVanityPaths(String vanityPath) { return entryMap; } - /** - * Check if the path is a valid vanity path - * @param path The resource path to check - * @return {@code true} if this is valid, {@code false} otherwise - */ - private boolean isValidVanityPath(final String path){ - if (path == null) { - throw new IllegalArgumentException("Unexpected null path"); - } - - // check white list - if ( this.factory.getVanityPathConfig() != null ) { - boolean allowed = false; - for(final VanityPathConfig config : this.factory.getVanityPathConfig()) { - if ( path.startsWith(config.prefix) ) { - allowed = !config.isExclude; - break; - } - } - if ( !allowed ) { - log.debug("isValidVanityPath: not valid as not in white list {}", path); - return false; - } - } - return true; - } - private String getActualContentPath(final String path){ final String checkPath; if ( path.endsWith(JCR_CONTENT_SUFFIX) ) { @@ -1078,7 +1046,7 @@ private String createAliasQuery(){ if(allowedLocations.isEmpty()){ query.append(" ").append("(").append("NOT ISDESCENDANTNODE(page,") - .append("\"").append(JCR_SYSTEM_PATH).append("\"").append("))"); + .append("'").append(JCR_SYSTEM_PATH).append("'").append("))"); }else{ Iterator pathIterator = allowedLocations.iterator(); @@ -1086,7 +1054,7 @@ private String createAliasQuery(){ while(pathIterator.hasNext()){ String prefix = pathIterator.next(); query.append(" ").append("ISDESCENDANTNODE(page,") - .append("\"").append(prefix).append("\"") + .append("'").append(prefix).append("'") .append(")").append(" ").append("OR"); } //Remove last "OR" keyword @@ -1283,27 +1251,27 @@ private String createVanityPathQuery() { final String query = VANITY_PATH_BASE_QUERY_DEFAULT + " WHERE sling:vanityPath IS NOT NULL" + - Stream - .of(true, false) - .map(this::createVanityPathQueryPathRestriction) - .filter(restriction -> restriction.length() != 0) + Stream.of( + createVanityPathQueryPathRestriction(factory.getVanityPathWhiteList(), true), + createVanityPathQueryPathRestriction(factory.getVanityPathBlackList(), false) + ) + .filter(StringUtils::isNotEmpty) .map(restriction -> " AND (" + restriction + ")") .collect(Collectors.joining()); logger.debug("Query to fetch vanity paths [{}] ", query); return query; } - private String createVanityPathQueryPathRestriction(final boolean include) { - return Stream.concat( - Stream.of(new VanityPathConfig(JCR_SYSTEM_PATH, true)), - Optional.ofNullable(this.factory.getVanityPathConfig()).map(List::stream).orElseGet(Stream::empty) - ) - .filter(cfg -> cfg.isExclude != include) - .map(vanityPathConfig -> String.format( + private String createVanityPathQueryPathRestriction(final Set paths, final boolean include) { + final String prefix = include ? StringUtils.EMPTY : "NOT "; + final String condition = include ? " OR " : " AND "; + return paths + .stream() + .map(path -> String.format( "%sISDESCENDANTNODE(page, '%s')", - vanityPathConfig.isExclude ? "NOT " : StringUtils.EMPTY, - StringUtils.removeEnd(vanityPathConfig.prefix, String.valueOf('/')))) - .collect(Collectors.joining(include ? " OR " : " AND ")); + prefix, + StringUtils.removeEnd(path, String.valueOf('/')))) + .collect(Collectors.joining(condition)); } private void updateTargetPaths(final Map> targetPaths, final String key, final String entry) { @@ -1339,7 +1307,7 @@ private String[] getVanityPathDefinition(final String pVanityPath) { log.warn("Ignoring malformed vanity path {}", pVanityPath); } } else { - prefix = StringUtils.EMPTY; + prefix = "^" + ANY_SCHEME_HOST; if (!info.startsWith("/")) { path = "/" + info; } else { diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/EtcMappingResourceResolverTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/EtcMappingResourceResolverTest.java index 468b310b..1d30fc8e 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/EtcMappingResourceResolverTest.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/EtcMappingResourceResolverTest.java @@ -112,7 +112,6 @@ public class EtcMappingResourceResolverTest { public void setup() throws Exception { MockitoAnnotations.initMocks(this); - List configs = getVanityPathConfigs(); vanityBloomFilterFile = new File("target/test-classes/resourcesvanityBloomFilter.txt"); List handlers = asList(createRPHandler(resourceProvider, "rp1", 0, "/")); ResourceProviderTracker resourceProviderTracker = mock(ResourceProviderTracker.class); @@ -144,10 +143,6 @@ public void setup() throws Exception { http = buildResource("/etc/map/http", map, resourceResolver, resourceProvider); } - List getVanityPathConfigs() { - return new ArrayList<>(); - } - /** * Changes to the /etc/map in our tests are not taking effect until there is an Change Event issued * diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/AbstractMappingMapEntriesTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/AbstractMappingMapEntriesTest.java index 8be37e31..a85d7451 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/AbstractMappingMapEntriesTest.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/AbstractMappingMapEntriesTest.java @@ -97,14 +97,12 @@ public abstract class AbstractMappingMapEntriesTest { public void setup() throws Exception { MockitoAnnotations.initMocks(this); - List configs = getVanityPathConfigs(); vanityBloomFilterFile = new File("target/test-classes/resourcesvanityBloomFilter.txt"); when(bundle.getSymbolicName()).thenReturn("TESTBUNDLE"); when(bundleContext.getBundle()).thenReturn(bundle); when(bundleContext.getDataFile("vanityBloomFilter.txt")).thenReturn(vanityBloomFilterFile); when(resourceResolverFactory.getServiceResourceResolver(any(Map.class))).thenReturn(resourceResolver); when(resourceResolverFactory.isVanityPathEnabled()).thenReturn(true); - when(resourceResolverFactory.getVanityPathConfig()).thenReturn(configs); when(resourceResolverFactory.isOptimizeAliasResolutionEnabled()).thenReturn(true); when(resourceResolverFactory.getMapRoot()).thenReturn(MapEntries.DEFAULT_MAP_ROOT); when(resourceResolverFactory.getMaxCachedVanityPathEntries()).thenReturn(-1L); @@ -124,10 +122,6 @@ public void setup() throws Exception { this.aliasMap = ( Map>) aliasMapField.get(mapEntries); } - List getVanityPathConfigs() { - return new ArrayList<>(); - } - @After public void tearDown() throws Exception { vanityBloomFilterFile.delete(); diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/InMemoryResourceProvider.java b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/InMemoryResourceProvider.java index 184e1fc5..f8af9e78 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/InMemoryResourceProvider.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/InMemoryResourceProvider.java @@ -98,7 +98,7 @@ public String[] getSupportedLanguages(@NotNull ResolveContext ctx) { public Iterator findResources(@NotNull ResolveContext ctx, String query, String language) { // we don't explicitly filter paths under jcr:system, but we don't expect to have such resources either // and this stub provider is not the proper location to test JCR queries - if ("SELECT sling:alias FROM nt:base AS page WHERE (NOT ISDESCENDANTNODE(page,\"/jcr:system\")) AND sling:alias IS NOT NULL".equals(query)) { + if ("SELECT sling:alias FROM nt:base AS page WHERE (NOT ISDESCENDANTNODE(page,'/jcr:system')) AND sling:alias IS NOT NULL".equals(query)) { return resourcesWithProperty(ctx, "sling:alias") .iterator(); } diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java index e0149a97..08062020 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java @@ -16,37 +16,13 @@ */ package org.apache.sling.resourceresolver.impl.mapping; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; -import static org.mockito.Matchers.any; -import static org.mockito.Matchers.anyString; -import static org.mockito.Matchers.eq; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - -import java.io.File; -import java.io.IOException; -import java.lang.reflect.Field; -import java.lang.reflect.Method; -import java.util.*; - -import java.util.concurrent.*; -import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.atomic.AtomicLong; - import org.apache.commons.lang3.StringUtils; import org.apache.sling.api.resource.Resource; import org.apache.sling.api.resource.ResourceResolver; import org.apache.sling.api.resource.observation.ResourceChange; import org.apache.sling.api.resource.observation.ResourceChange.ChangeType; -import org.apache.sling.api.resource.path.Path; import org.apache.sling.api.wrappers.ValueMapDecorator; import org.apache.sling.resourceresolver.impl.ResourceResolverImpl; -import org.apache.sling.resourceresolver.impl.mapping.MapConfigurationProvider.VanityPathConfig; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -59,6 +35,41 @@ import org.osgi.framework.BundleContext; import org.osgi.service.event.EventAdmin; +import java.io.File; +import java.io.IOException; +import java.lang.reflect.Field; +import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.Iterator; +import java.util.LinkedList; +import java.util.List; +import java.util.Map; +import java.util.Random; +import java.util.Set; +import java.util.SortedSet; +import java.util.TreeSet; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Semaphore; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicLong; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyString; +import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + public class MapEntriesTest extends AbstractMappingMapEntriesTest { private MapEntries mapEntries; @@ -89,32 +100,33 @@ public class MapEntriesTest extends AbstractMappingMapEntriesTest { public void setup() throws Exception { MockitoAnnotations.initMocks(this); - final List configs = new ArrayList<>(); - configs.add(new VanityPathConfig("/libs/", false)); - configs.add(new VanityPathConfig("/libs/denied", true)); - configs.add(new VanityPathConfig("/foo/", false)); - configs.add(new VanityPathConfig("/baa/", false)); - configs.add(new VanityPathConfig("/justVanityPath", false)); - configs.add(new VanityPathConfig("/justVanityPath2", false)); - configs.add(new VanityPathConfig("/badVanityPath", false)); - configs.add(new VanityPathConfig("/redirectingVanityPath", false)); - configs.add(new VanityPathConfig("/redirectingVanityPath301", false)); - configs.add(new VanityPathConfig("/vanityPathOnJcrContent", false)); - - Collections.sort(configs); + final SortedSet vanityPathBlackList = new TreeSet<>(); + vanityPathBlackList.add("/libs/denied/"); + vanityPathBlackList.add("/jcr:system/"); + final SortedSet vanityPathWhiteList = new TreeSet<>(); + vanityPathWhiteList.add("/libs/"); + vanityPathWhiteList.add("/foo/"); + vanityPathWhiteList.add("/baa/"); + vanityPathWhiteList.add("/justVanityPath"); + vanityPathWhiteList.add("/justVanityPath2"); + vanityPathWhiteList.add("/badVanityPath"); + vanityPathWhiteList.add("/redirectingVanityPath"); + vanityPathWhiteList.add("/redirectingVanityPath301"); + vanityPathWhiteList.add("/vanityPathOnJcrContent"); + vanityBloomFilterFile = new File("src/main/resourcesvanityBloomFilter.txt"); when(bundle.getSymbolicName()).thenReturn("TESTBUNDLE"); when(bundleContext.getBundle()).thenReturn(bundle); when(bundleContext.getDataFile("vanityBloomFilter.txt")).thenReturn(vanityBloomFilterFile); when(resourceResolverFactory.getServiceResourceResolver(any(Map.class))).thenReturn(resourceResolver); when(resourceResolverFactory.isVanityPathEnabled()).thenReturn(true); - when(resourceResolverFactory.getVanityPathConfig()).thenReturn(configs); + when(resourceResolverFactory.getVanityPathBlackList()).thenReturn(vanityPathBlackList); + when(resourceResolverFactory.getVanityPathWhiteList()).thenReturn(vanityPathWhiteList); when(resourceResolverFactory.isOptimizeAliasResolutionEnabled()).thenReturn(true); when(resourceResolverFactory.getMapRoot()).thenReturn(MapEntries.DEFAULT_MAP_ROOT); when(resourceResolverFactory.getMaxCachedVanityPathEntries()).thenReturn(-1L); when(resourceResolverFactory.isMaxCachedVanityPathEntriesStartup()).thenReturn(true); - when(resourceResolver.findResources(anyString(), eq("sql"))).thenReturn( - Collections. emptySet().iterator()); + when(resourceResolver.findResources(anyString(), eq("sql"))).thenReturn(Collections.emptyIterator()); //when(resourceResolverFactory.getAliasPath()).thenReturn(Arrays.asList("/child")); Set aliasPath = new TreeSet<>(); @@ -378,15 +390,15 @@ public void test_vanity_path_registration_include_exclude() throws IOException { if (("SELECT sling:vanityPath, sling:redirect, sling:redirectStatus FROM nt:base AS page" + " WHERE sling:vanityPath IS NOT NULL" + " AND" + - " (ISDESCENDANTNODE(page, '/redirectingVanityPath301')" + - " OR ISDESCENDANTNODE(page, '/vanityPathOnJcrContent')" + - " OR ISDESCENDANTNODE(page, '/redirectingVanityPath')" + - " OR ISDESCENDANTNODE(page, '/justVanityPath2')" + - " OR ISDESCENDANTNODE(page, '/justVanityPath')" + + " (ISDESCENDANTNODE(page, '/baa')" + " OR ISDESCENDANTNODE(page, '/badVanityPath')" + - " OR ISDESCENDANTNODE(page, '/libs')" + " OR ISDESCENDANTNODE(page, '/foo')" + - " OR ISDESCENDANTNODE(page, '/baa')" + + " OR ISDESCENDANTNODE(page, '/justVanityPath')" + + " OR ISDESCENDANTNODE(page, '/justVanityPath2')" + + " OR ISDESCENDANTNODE(page, '/libs')" + + " OR ISDESCENDANTNODE(page, '/redirectingVanityPath')" + + " OR ISDESCENDANTNODE(page, '/redirectingVanityPath301')" + + " OR ISDESCENDANTNODE(page, '/vanityPathOnJcrContent')" + ") AND " + "(NOT ISDESCENDANTNODE(page, '/jcr:system')" + " AND NOT ISDESCENDANTNODE(page, '/libs/denied')" + @@ -1636,16 +1648,6 @@ public void test_doRemoveAliasFromSibling() throws Exception { assertNull(aliasMapEntry); } - @Test - public void test_isValidVanityPath() throws Exception { - Method method = MapEntries.class.getDeclaredMethod("isValidVanityPath", String.class); - method.setAccessible(true); - - assertFalse((Boolean)method.invoke(mapEntries, "/jcr:system/node")); - - assertTrue((Boolean)method.invoke(mapEntries, "/justVanityPath")); - } - @Test //SLING-4847 public void test_doNodeAdded1() throws Exception { @@ -2106,7 +2108,7 @@ public void testLoadAliases_ValidAbsolutePath_DefaultPaths() { @Override public Iterator answer(InvocationOnMock invocation) throws Throwable { String query = StringUtils.trim((String)invocation.getArguments()[0]); - assertEquals("SELECT sling:alias FROM nt:base AS page WHERE (NOT ISDESCENDANTNODE(page,\"/jcr:system\")) AND sling:alias IS NOT NULL", query); + assertEquals("SELECT sling:alias FROM nt:base AS page WHERE (NOT ISDESCENDANTNODE(page,'/jcr:system')) AND sling:alias IS NOT NULL", query); return Collections. emptySet().iterator(); } }); From d16e01d0aecf123d8b406a3c1f80b91108dddbfa Mon Sep 17 00:00:00 2001 From: Henry Kuijpers Date: Thu, 10 Jun 2021 16:36:01 +0200 Subject: [PATCH 6/9] SLING-10447 Rename getters --- .../CommonResourceResolverFactoryImpl.java | 8 ++--- .../ResourceResolverFactoryActivator.java | 22 +++++++------- .../mapping/MapConfigurationProvider.java | 8 ++--- .../impl/mapping/MapEntries.java | 12 ++++---- .../impl/mapping/MapEntriesTest.java | 30 +++++++++---------- 5 files changed, 39 insertions(+), 41 deletions(-) diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/CommonResourceResolverFactoryImpl.java b/src/main/java/org/apache/sling/resourceresolver/impl/CommonResourceResolverFactoryImpl.java index 831a3011..bac115a6 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/CommonResourceResolverFactoryImpl.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/CommonResourceResolverFactoryImpl.java @@ -495,13 +495,13 @@ public Map getServiceUserAuthenticationInfo(final String subServ } @Override - public @NotNull Set getVanityPathWhiteList() { - return activator.getVanityPathWhiteList(); + public @NotNull Set getAllowedVanityPathLocations() { + return activator.getAllowedVanityPathLocations(); } @Override - public @NotNull Set getVanityPathBlackList() { - return activator.getVanityPathBlackList(); + public @NotNull Set getExcludedVanityPathLocations() { + return activator.getExcludedVanityPathLocations(); } /** diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java index 288875dc..903f03ee 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java @@ -138,11 +138,11 @@ private static final class FactoryRegistration { @SuppressWarnings("java:S3077") private volatile Set allowedAliasLocations = Collections.emptySet(); - /** Vanity path whitelist */ - private volatile Set vanityPathWhiteList = Collections.emptySet(); + /** Allowed vanity path locations */ + private volatile Set allowedVanityPathLocations = Collections.emptySet(); - /** Vanity path blacklist */ - private volatile Set vanityPathBlackList = Collections.emptySet(); + /** Excluded vanity path locations */ + private volatile Set excludedVanityPathLocations = Collections.emptySet(); private final FactoryPreconditions preconds = new FactoryPreconditions(); @@ -221,13 +221,13 @@ public boolean isLogUnclosedResourceResolvers() { } @NotNull - public Set getVanityPathWhiteList() { - return this.vanityPathWhiteList; + public Set getAllowedVanityPathLocations() { + return this.allowedVanityPathLocations; } @NotNull - public Set getVanityPathBlackList() { - return this.vanityPathBlackList; + public Set getExcludedVanityPathLocations() { + return this.excludedVanityPathLocations; } public boolean hasVanityPathPrecedence() { @@ -327,14 +327,12 @@ protected void activate(final BundleContext bundleContext, final ResourceResolve } } - // vanity path white list - this.vanityPathWhiteList = Collections.unmodifiableSet(Arrays + this.allowedVanityPathLocations = Collections.unmodifiableSet(Arrays .stream(ArrayUtils.nullToEmpty(config.resource_resolver_vanitypath_whitelist())) .map(String::trim) .map(value -> StringUtils.appendIfMissing(value, String.valueOf('/'))) .collect(Collectors.toSet())); - // vanity path black list - this.vanityPathBlackList = Collections.unmodifiableSet(Stream.concat(Arrays + this.excludedVanityPathLocations = Collections.unmodifiableSet(Stream.concat(Arrays .stream(ArrayUtils.nullToEmpty(config.resource_resolver_vanitypath_blacklist())), Stream.of("/jcr:system")) .map(String::trim) .map(value -> StringUtils.appendIfMissing(value, String.valueOf('/'))) diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapConfigurationProvider.java b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapConfigurationProvider.java index c1c200b9..b4acb64b 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapConfigurationProvider.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapConfigurationProvider.java @@ -57,18 +57,18 @@ public interface MapConfigurationProvider extends ResourceResolverFactory { Map getServiceUserAuthenticationInfo(final String subServiceName) throws LoginException; /** - * A set of whitelisted prefixes all ending with a slash. + * A set of allowed prefixes all ending with a slash. * If empty set is returned, all paths are allowed. */ @NotNull - Set getVanityPathWhiteList(); + Set getAllowedVanityPathLocations(); /** - * A set of blacklisted prefixes all ending with a slash. + * A set of excluded prefixes all ending with a slash. * If empty set is returned, all paths are allowed. */ @NotNull - Set getVanityPathBlackList(); + Set getExcludedVanityPathLocations(); /** * A set of allow prefixes all ending with a slash. diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java index 2b895f65..1599b1c3 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java @@ -177,11 +177,11 @@ public MapEntries(final MapConfigurationProvider factory, final BundleContext bu paths.addAll(factory.getAllowedAliasLocations()); } if (factory.isVanityPathEnabled()) { - final Set vanityPathWhiteList = factory.getVanityPathWhiteList(); - if (vanityPathWhiteList.isEmpty()) { + final Set allowedVanityPathLocations = factory.getAllowedVanityPathLocations(); + if (allowedVanityPathLocations.isEmpty()) { paths.add(String.valueOf('/')); } else { - paths.addAll(vanityPathWhiteList); + paths.addAll(allowedVanityPathLocations); } } if (factory.getMapRoot() != null) { @@ -312,7 +312,7 @@ private boolean addResource(final String path, final AtomicBoolean resolverRefre private boolean updateResource(final String path, final AtomicBoolean resolverRefreshed) { // Blacklist cannot be configured with ResourceChangeListener, so do it here - final boolean isValidVanityPath = this.factory.getVanityPathBlackList().stream().noneMatch(path::startsWith); + final boolean isValidVanityPath = this.factory.getExcludedVanityPathLocations().stream().noneMatch(path::startsWith); if ( this.useOptimizeAliasResolution || isValidVanityPath) { this.initializing.lock(); @@ -1252,8 +1252,8 @@ private String createVanityPathQuery() { VANITY_PATH_BASE_QUERY_DEFAULT + " WHERE sling:vanityPath IS NOT NULL" + Stream.of( - createVanityPathQueryPathRestriction(factory.getVanityPathWhiteList(), true), - createVanityPathQueryPathRestriction(factory.getVanityPathBlackList(), false) + createVanityPathQueryPathRestriction(factory.getAllowedVanityPathLocations(), true), + createVanityPathQueryPathRestriction(factory.getExcludedVanityPathLocations(), false) ) .filter(StringUtils::isNotEmpty) .map(restriction -> " AND (" + restriction + ")") diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java index 08062020..2c488de3 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java @@ -100,19 +100,19 @@ public class MapEntriesTest extends AbstractMappingMapEntriesTest { public void setup() throws Exception { MockitoAnnotations.initMocks(this); - final SortedSet vanityPathBlackList = new TreeSet<>(); - vanityPathBlackList.add("/libs/denied/"); - vanityPathBlackList.add("/jcr:system/"); - final SortedSet vanityPathWhiteList = new TreeSet<>(); - vanityPathWhiteList.add("/libs/"); - vanityPathWhiteList.add("/foo/"); - vanityPathWhiteList.add("/baa/"); - vanityPathWhiteList.add("/justVanityPath"); - vanityPathWhiteList.add("/justVanityPath2"); - vanityPathWhiteList.add("/badVanityPath"); - vanityPathWhiteList.add("/redirectingVanityPath"); - vanityPathWhiteList.add("/redirectingVanityPath301"); - vanityPathWhiteList.add("/vanityPathOnJcrContent"); + final SortedSet excludedVanityPathLocations = new TreeSet<>(); + excludedVanityPathLocations.add("/libs/denied/"); + excludedVanityPathLocations.add("/jcr:system/"); + final SortedSet allowedVanityPathLocations = new TreeSet<>(); + allowedVanityPathLocations.add("/libs/"); + allowedVanityPathLocations.add("/foo/"); + allowedVanityPathLocations.add("/baa/"); + allowedVanityPathLocations.add("/justVanityPath"); + allowedVanityPathLocations.add("/justVanityPath2"); + allowedVanityPathLocations.add("/badVanityPath"); + allowedVanityPathLocations.add("/redirectingVanityPath"); + allowedVanityPathLocations.add("/redirectingVanityPath301"); + allowedVanityPathLocations.add("/vanityPathOnJcrContent"); vanityBloomFilterFile = new File("src/main/resourcesvanityBloomFilter.txt"); when(bundle.getSymbolicName()).thenReturn("TESTBUNDLE"); @@ -120,8 +120,8 @@ public void setup() throws Exception { when(bundleContext.getDataFile("vanityBloomFilter.txt")).thenReturn(vanityBloomFilterFile); when(resourceResolverFactory.getServiceResourceResolver(any(Map.class))).thenReturn(resourceResolver); when(resourceResolverFactory.isVanityPathEnabled()).thenReturn(true); - when(resourceResolverFactory.getVanityPathBlackList()).thenReturn(vanityPathBlackList); - when(resourceResolverFactory.getVanityPathWhiteList()).thenReturn(vanityPathWhiteList); + when(resourceResolverFactory.getExcludedVanityPathLocations()).thenReturn(excludedVanityPathLocations); + when(resourceResolverFactory.getAllowedVanityPathLocations()).thenReturn(allowedVanityPathLocations); when(resourceResolverFactory.isOptimizeAliasResolutionEnabled()).thenReturn(true); when(resourceResolverFactory.getMapRoot()).thenReturn(MapEntries.DEFAULT_MAP_ROOT); when(resourceResolverFactory.getMaxCachedVanityPathEntries()).thenReturn(-1L); From c4411839e6a26a03279932fcf33da24109d02f65 Mon Sep 17 00:00:00 2001 From: Henry Kuijpers Date: Fri, 11 Jun 2021 10:13:20 +0200 Subject: [PATCH 7/9] SLING-10447 Fix Sonar warnings --- .../resourceresolver/impl/ResourceResolverFactoryActivator.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java index 903f03ee..5daff973 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java @@ -139,9 +139,11 @@ private static final class FactoryRegistration { private volatile Set allowedAliasLocations = Collections.emptySet(); /** Allowed vanity path locations */ + @SuppressWarnings("java:S3077") private volatile Set allowedVanityPathLocations = Collections.emptySet(); /** Excluded vanity path locations */ + @SuppressWarnings("java:S3077") private volatile Set excludedVanityPathLocations = Collections.emptySet(); private final FactoryPreconditions preconds = new FactoryPreconditions(); From ab807584eaaa45c50a8f0e23ce52b457b65a4412 Mon Sep 17 00:00:00 2001 From: Henry Kuijpers Date: Fri, 11 Jun 2021 14:23:49 +0200 Subject: [PATCH 8/9] SLING-10447 Add comment in tests with regards to query testing --- .../sling/resourceresolver/impl/mapping/MapEntriesTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java index 2c488de3..e99c0e5c 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java @@ -386,6 +386,7 @@ public void test_vanity_path_registration_include_exclude() throws IOException { for(final String val : validPaths) { resources.add(getVanityPathResource(val)); } + // Note that it's *way* better to test these querys with a real query engine - Idea for Sling starter tests? when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer((Answer>) invocation -> { if (("SELECT sling:vanityPath, sling:redirect, sling:redirectStatus FROM nt:base AS page" + " WHERE sling:vanityPath IS NOT NULL" + From 0fb1395235fa823d985d98f6ed8bfb0cf5f6af4b Mon Sep 17 00:00:00 2001 From: Henry Kuijpers Date: Thu, 19 Aug 2021 12:05:45 +0200 Subject: [PATCH 9/9] SLING-10447 Add back constant to avoid computing the value everytime --- .../apache/sling/resourceresolver/impl/mapping/MapEntries.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java index 1599b1c3..0099aa0c 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java @@ -112,6 +112,7 @@ public class MapEntries implements @SuppressWarnings("java:S1075") // Content path private static final String JCR_SYSTEM_PATH = "/jcr:system"; + private static final String JCR_SYSTEM_PATH_PREFIX = JCR_SYSTEM_PATH + '/'; private static final String VANITY_PATH_BASE_QUERY_DEFAULT = "SELECT sling:vanityPath, sling:redirect, sling:redirectStatus FROM nt:base AS page"; @@ -748,7 +749,7 @@ public void onChange(final List changes) { log.debug("onChange, type={}, path={}", rc.getType(), path); // don't care for system area - if (path.startsWith(JCR_SYSTEM_PATH + '/')) { + if (path.startsWith(JCR_SYSTEM_PATH_PREFIX)) { continue; }