SLING-8120 - pass the ResourceResolver to CapabilitiesSource
authorBertrand Delacretaz <bdelacretaz@apache.org>
Tue, 20 Nov 2018 14:08:30 +0000 (15:08 +0100)
committerBertrand Delacretaz <bdelacretaz@apache.org>
Tue, 20 Nov 2018 14:08:30 +0000 (15:08 +0100)
pom.xml
src/main/java/org/apache/sling/capabilities/CapabilitiesSource.java
src/main/java/org/apache/sling/capabilities/defaultsources/SlingServletsSource.java
src/main/java/org/apache/sling/capabilities/internal/CapabilitiesServlet.java
src/main/java/org/apache/sling/capabilities/internal/JSONCapabilitiesWriter.java
src/test/java/org/apache/sling/capabilities/defaultsources/SlingServletsSourceTest.java
src/test/java/org/apache/sling/capabilities/internal/JSONCapabilitiesWriterTest.java
src/test/java/org/apache/sling/capabilities/internal/MockSource.java
src/test/java/org/apache/sling/capabilities/it/CapabilitiesBundleIT.java

diff --git a/pom.xml b/pom.xml
index 354a4e5..33637a5 100644 (file)
--- a/pom.xml
+++ b/pom.xml
       <version>${org.ops4j.pax.exam.version}</version>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.mockito</groupId>
+      <artifactId>mockito-core</artifactId>
+      <version>2.23.0</version>
+      <scope>test</scope>
+    </dependency>
   </dependencies>
 </project>
index e6ec9d5..62744e6 100644 (file)
@@ -19,6 +19,7 @@
 package org.apache.sling.capabilities;
 
 import java.util.Map;
+import org.apache.sling.api.resource.ResourceResolver;
 import org.osgi.annotation.versioning.ProviderType;
 
 /** A CapabilitiesSource provides capabilities, as a Map of key/value
@@ -35,9 +36,17 @@ public interface CapabilitiesSource {
      */
     String getNamespace();
     
-    /** @return zero to N capabilities, each being represented by
-     *      a key/value pair.
+    /** Return zero to N capabilities, each being represented by
+     *  a key/value pair.
+     *
+     *  Services implementing this interface must be careful to
+     *  avoid crossing trust boundaries. They should only expose data that
+     * is accessible to the ResourceResolver that's passed
+     *  as a parameter.
+     *
+     * @return a Map of capabilities
+     * @param resolver used to establish the user's identity
      * @throws Exception if the capabilities could not be computed.
      */
-    Map<String, Object> getCapabilities() throws Exception;
+    Map<String, Object> getCapabilities(ResourceResolver resolver) throws Exception;
 }
\ No newline at end of file
index bdb6e64..0de2c01 100644 (file)
@@ -22,6 +22,7 @@ import java.util.HashMap;
 import java.util.Map;
 import java.util.TreeMap;
 import javax.servlet.Servlet;
+import org.apache.sling.api.resource.ResourceResolver;
 import org.apache.sling.capabilities.CapabilitiesSource;
 import org.osgi.framework.BundleContext;
 import org.osgi.framework.ServiceReference;
@@ -35,6 +36,10 @@ import org.osgi.service.metatype.annotations.ObjectClassDefinition;
 /** Default CapabilitiesSource that provides information on available Sling
  *  Servlets, exposing their sling.servlet.* properties so that clients can
  *  find out which behaviors are available.
+ *
+ *  This should only be used to expose servlets to which all users have access,
+ *  generic functions such as searches etc., to avoid unwanted information
+ *  disclosure.
  */
 @Component(service = CapabilitiesSource.class)
 @Designate(
@@ -50,7 +55,9 @@ public class SlingServletsSource implements CapabilitiesSource {
     public static @interface Config {
         @AttributeDefinition(
             name = "LDAP filter",
-            description = "OSGi LDAP filter to select servlets to consider for the provided capabilites"
+            description = "OSGi LDAP filter to select servlets to consider for the provided capabilites. "
+                + "This should only expose general purpose servlets to which all users have access, like "
+                + "search functionality and similar features."
         )
         String servletsLdapFilter() default "";
         
@@ -83,7 +90,7 @@ public class SlingServletsSource implements CapabilitiesSource {
     }
 
     @Override
-    public Map<String, Object> getCapabilities() throws Exception {
+    public Map<String, Object> getCapabilities(ResourceResolver resolver) throws Exception {
         final Map<String, Object> result = new HashMap<>();
         final ServiceReference [] refs = bundleContext.getServiceReferences(Servlet.class.getName(), ldapFilter);
         if(refs != null) {
index b1a91a0..8e7b64c 100644 (file)
@@ -26,6 +26,7 @@ import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletResponse;
 import org.apache.sling.api.SlingHttpServletRequest;
 import org.apache.sling.api.SlingHttpServletResponse;
+import org.apache.sling.api.resource.Resource;
 import org.apache.sling.api.resource.ValueMap;
 import org.apache.sling.api.servlets.SlingSafeMethodsServlet;
 import org.apache.sling.capabilities.CapabilitiesSource;
@@ -78,12 +79,14 @@ public class CapabilitiesServlet extends SlingSafeMethodsServlet {
     @Override
     protected void doGet(SlingHttpServletRequest request, SlingHttpServletResponse response) throws ServletException, IOException {
         
+        final Resource resource = request.getResource();
+
         // Resource Path must match a configurable set of patterns, to prevent
         // users from getting this information by just creating a resource anywhere
         // with our resource type. The idea is that those paths will have suitable
         // access control (readonly for users) to control exactly which capabilities
         // are exposed.
-        final String path = request.getResource().getPath();
+        final String path = resource.getPath();
         if(!pathFilter.accept(path)) {
             response.sendError(HttpServletResponse.SC_FORBIDDEN, "Invalid path " + path);
             return;
@@ -92,7 +95,7 @@ public class CapabilitiesServlet extends SlingSafeMethodsServlet {
         // Resource must define which namespaces are exposed, also for
         // security reasons, to make sure administrators think about
         // what's exposed
-        final ValueMap m = request.getResource().adaptTo(ValueMap.class);
+        final ValueMap m = resource.adaptTo(ValueMap.class);
         final String [] namespacePatterns = m.get(NAMESPACES_PROP, String[].class);
         if(namespacePatterns == null) {
             response.sendError(HttpServletResponse.SC_FORBIDDEN, "Missing property " + NAMESPACES_PROP);
@@ -102,7 +105,7 @@ public class CapabilitiesServlet extends SlingSafeMethodsServlet {
         // All good, get capabilities
         response.setContentType("application/json");
         response.setCharacterEncoding("UTF-8");
-        new JSONCapabilitiesWriter().writeJson(response.getWriter(), sources, new RegexFilter(namespacePatterns));
+        new JSONCapabilitiesWriter().writeJson(resource.getResourceResolver(), response.getWriter(), sources, new RegexFilter(namespacePatterns));
         response.getWriter().flush();
     }
 
index bd85590..818ef28 100644 (file)
@@ -26,6 +26,7 @@ import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
 import org.apache.felix.utils.json.JSONWriter;
+import org.apache.sling.api.resource.ResourceResolver;
 import org.apache.sling.capabilities.CapabilitiesSource;
 
 /** Create the JSON output of our servlet */
@@ -35,7 +36,7 @@ class JSONCapabilitiesWriter {
     static final String DATA_KEY = "data";
     
     /** Write JSON to the supplied Writer, using the supplied sources */
-    void writeJson(Writer w, Collection<CapabilitiesSource> sources, RegexFilter namespacePatterns) throws IOException {
+    void writeJson(ResourceResolver resolver, Writer w, Collection<CapabilitiesSource> sources, RegexFilter namespacePatterns) throws IOException {
         final Set<String> namespaces = new HashSet<>();
 
         final JSONWriter jw = new JSONWriter(w);
@@ -58,7 +59,7 @@ class JSONCapabilitiesWriter {
             namespaces.add(namespace);
             
             try {
-                values = s.getCapabilities();
+                values = s.getCapabilities(resolver);
             } catch(Exception e) {
                 values = new HashMap<>();
                 values.put("_EXCEPTION_", e.getClass().getName() + ":" + e.getMessage());
index 9503603..e95562a 100644 (file)
@@ -85,7 +85,7 @@ public class SlingServletsSourceTest {
         assertNotNull("Expecting a CapabilitiesSource", src);
         assertEquals("Expecting namespace to match", "org.apache.sling.servlets.TEST_NS", src.getNamespace());
 
-        final Map<String, Object> caps = src.getCapabilities();
+        final Map<String, Object> caps = src.getCapabilities(null);
         assertNotNull("Expecting to get Capabilities", caps);
         assertEquals("Expecting capabilities for 2 json servlets", 2, caps.size());
 
index 27ee0aa..2cd2dfe 100644 (file)
@@ -23,17 +23,43 @@ import java.io.StringReader;
 import java.io.StringWriter;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.UUID;
 import javax.json.Json;
 import javax.json.JsonObject;
 import javax.json.JsonReader;
+import org.apache.sling.api.resource.ResourceResolver;
 import org.apache.sling.capabilities.CapabilitiesSource;
 import static org.junit.Assert.assertEquals;
+import org.junit.BeforeClass;
 import org.junit.Test;
+import org.mockito.Mockito;
 
 /** Test the JSONCapabilitiesWriter */
 public class JSONCapabilitiesWriterTest {
 
     private final RegexFilter namespaceFilter = new RegexFilter(".*");
+    private static ResourceResolver resolver;
+    private static final String RESOLVER_STRING = "resolver-" + UUID.randomUUID();
+
+    @BeforeClass
+    public static void setupMocks() {
+        resolver = Mockito.mock(ResourceResolver.class);
+        Mockito.when(resolver.toString()).thenReturn(RESOLVER_STRING);
+    }
+
+    @Test
+    public void testResolverIsUsed() throws IOException {
+        final List<CapabilitiesSource> sources = new ArrayList<>();
+        sources.add(new MockSource("A", 2));
+
+        final StringWriter w = new StringWriter();
+        new JSONCapabilitiesWriter().writeJson(resolver, w, sources, namespaceFilter);
+        final JsonReader r = Json.createReader(new StringReader(w.toString()));
+        final JsonObject rootJson = r.readObject();
+        final JsonObject json = rootJson.getJsonObject(JSONCapabilitiesWriter.CAPS_KEY).getJsonObject("data");
+
+        assertEquals(RESOLVER_STRING, json.getJsonObject("A").getString(ResourceResolver.class.getSimpleName()));
+    }
     
     @Test
     public void testWithTwoSources() throws IOException {
@@ -42,7 +68,7 @@ public class JSONCapabilitiesWriterTest {
         sources.add(new MockSource("B", 1));
         
         final StringWriter w = new StringWriter();
-        new JSONCapabilitiesWriter().writeJson(w, sources, namespaceFilter);
+        new JSONCapabilitiesWriter().writeJson(resolver, w, sources, namespaceFilter);
         
         final JsonReader r = Json.createReader(new StringReader(w.toString()));
         final JsonObject rootJson = r.readObject();
@@ -52,8 +78,8 @@ public class JSONCapabilitiesWriterTest {
         assertEquals("VALUE_0_B", json.getJsonObject("B").getString("KEY_0_B"));
         
         assertEquals("Expecting 1 root key", 1, rootJson.keySet().size());
-        assertEquals("Expecting 2 keys at A", 2, json.getJsonObject("A").keySet().size());
-        assertEquals("Expecting 1 key at B", 1, json.getJsonObject("B").keySet().size());
+        assertEquals("Expecting 3 keys at A", 3, json.getJsonObject("A").keySet().size());
+        assertEquals("Expecting 2 key at B", 2, json.getJsonObject("B").keySet().size());
     }
 
     @Test
@@ -64,7 +90,7 @@ public class JSONCapabilitiesWriterTest {
         sources.add(new MockSource("B", 1));
         
         final StringWriter w = new StringWriter();
-        new JSONCapabilitiesWriter().writeJson(w, sources, namespaceFilter);
+        new JSONCapabilitiesWriter().writeJson(resolver, w, sources, namespaceFilter);
         
         final JsonReader r = Json.createReader(new StringReader(w.toString()));
         final JsonObject rootJson = r.readObject();
@@ -84,6 +110,6 @@ public class JSONCapabilitiesWriterTest {
         sources.add(new MockSource("duplicate", 1));
 
         final StringWriter w = new StringWriter();
-        new JSONCapabilitiesWriter().writeJson(w, sources, namespaceFilter);
+        new JSONCapabilitiesWriter().writeJson(resolver, w, sources, namespaceFilter);
     }
 }
\ No newline at end of file
index 50b8652..a5bb934 100644 (file)
@@ -21,25 +21,33 @@ package org.apache.sling.capabilities.internal;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
+import org.apache.sling.api.resource.ResourceResolver;
 import org.apache.sling.capabilities.CapabilitiesSource;
 
 class MockSource implements CapabilitiesSource {
 
     private final String namespace;
-    private final Map<String, Object> props = new HashMap<>();
+    private final int propsCount;
 
     MockSource(String namespace, int propsCount) {
         this.namespace = namespace;
-        for (int i = 0; i < propsCount; i++) {
-            props.put("KEY_" + i + "_" + namespace, "VALUE_" + i + "_" + namespace);
-        }
+        this.propsCount = propsCount;
     }
 
     @Override
-    public Map<String, Object> getCapabilities() throws Exception {
+    public Map<String, Object> getCapabilities(ResourceResolver resolver) throws Exception {
         if (namespace.contains("EXCEPTION")) {
             throw new IllegalArgumentException("Simulating a problem");
         }
+        return getProps(resolver);
+    }
+
+    private Map<String, Object> getProps(ResourceResolver resolver) {
+        final Map<String, Object> props = new HashMap<>();
+        for (int i = 0; i < propsCount; i++) {
+            props.put("KEY_" + i + "_" + namespace, "VALUE_" + i + "_" + namespace);
+        }
+        props.put(ResourceResolver.class.getSimpleName(), resolver.toString());
         return Collections.unmodifiableMap(props);
     }
 
index 610c2cf..abd2bfd 100644 (file)
@@ -21,6 +21,7 @@ package org.apache.sling.capabilities.it;
 import java.util.Map;
 import javax.inject.Inject;
 import javax.servlet.Servlet;
+import org.apache.sling.api.resource.ResourceResolver;
 import org.apache.sling.capabilities.CapabilitiesSource;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
@@ -47,8 +48,11 @@ public class CapabilitiesBundleIT extends CapabilitiesTestSupport {
     private Bundle testBundle;
 
     private static class TestCapabilitiesSource implements CapabilitiesSource {
+        @Override
         public String getNamespace() { return null; }
-        public Map<String, Object> getCapabilities() { return null; }
+
+        @Override
+        public Map<String, Object> getCapabilities(ResourceResolver unused) { return null; }
     };
 
     @Before