XWiki exposed whole content of all documents of all wikis to anybody with view right on Solr suggest service

@@ -233,36 +233,34 @@ private String[] toStringArray(Iterable iterable)
     protected void filterResponse(QueryResponse response, List<DocumentReference> usersToCheck)
     {
         SolrDocumentList results = response.getResults();
-        long numFound = results.getNumFound();
+        long numResults = results.size();
 
-        // Since we are modifying the results collection, we need to iterate over its copy.
-        for (SolrDocument result : new ArrayList<SolrDocument>(results)) {
+        results.removeIf(result -> {
+            boolean keep = false;
             try {
                 DocumentReference resultDocumentReference = this.solrDocumentReferenceResolver.resolve(result);
 
-                if (!isAllowed(resultDocumentReference, usersToCheck)) {
+                keep = isAllowed(resultDocumentReference, usersToCheck);
+            } catch (Exception e) {
+                // Don't take any risk of including a result for which we cannot determine the document reference and
+                // thus cannot determine if the given users have access to it or not.
+                this.logger.warn("Removing bad result: {}", result, e);
+            }
 
-                    // Remove the current incompatible result.
-                    results.remove(result);
+            // FIXME: We should update maxScore as well when removing the top scored item. How do we do that?
+            // Sorting based on score might be a not so expensive option.
 
-                    // Decrement the number of results.
-                    numFound--;
+            // FIXME: What about highlighting, facets and all the other data inside the QueryResponse?
 
-                    // FIXME: We should update maxScore as well when removing the top scored item. How do we do that?
-                    // Sorting based on score might be a not so expensive option.
+            return !keep;
+        });
 
-                    // FIXME: What about highlighting, facets and all the other data inside the QueryResponse?
-                }
-            } catch (Exception e) {
-                this.logger.warn("Skipping bad result: {}", result, e);
-            }
-        }
+        long numFilteredResults = numResults - results.size();
+
+        // Update the number of results, excluding the filtered ones.
+        // Lower bound guard for the total number of results.
+        long numFound = Math.max(0, response.getResults().getNumFound() - numFilteredResults);
 
-        // Update the new number of results, excluding the filtered ones.
-        if (numFound < 0) {
-            // Lower bound guard for the total number of results.
-            numFound = 0;
-        }
         results.setNumFound(numFound);
     }
 

@@ -21,6 +21,7 @@
 
 import java.lang.reflect.ParameterizedType;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.Locale;
 
 import org.apache.solr.client.solrj.SolrQuery;
@@ -72,19 +73,19 @@ public class SolrQueryExecutorTest
 {
     private static final String ITERABLE_PARAM_NAME = "multiParam";
 
-    private static final String[] ITERABLE_PARAM_EXPECTED = {"value1", "value2"};
+    private static final String[] ITERABLE_PARAM_EXPECTED = { "value1", "value2" };
 
     private static final Iterable<String> ITERABLE_PARAM_VALUE = Arrays.asList(ITERABLE_PARAM_EXPECTED);
 
     private static final String INT_ARR_PARAM_NAME = "intArrayParam";
 
-    private static final String[] INT_ARR_PARAM_EXPECTED = {"-42", "4711"};
+    private static final String[] INT_ARR_PARAM_EXPECTED = { "-42", "4711" };
 
-    private static final int[] INT_ARR_PARAM_VALUE = {-42, 4711};
+    private static final int[] INT_ARR_PARAM_VALUE = { -42, 4711 };
 
     private static final String STR_ARR_PARAM_NAME = "stringArrayParam";
 
-    private static final String[] STR_ARR_PARAM_EXPECTED = {"valueA", "valueB"};
+    private static final String[] STR_ARR_PARAM_EXPECTED = { "valueA", "valueB" };
 
     private static final String[] STR_ARR_PARAM_VALUE = STR_ARR_PARAM_EXPECTED;
 
@@ -228,4 +229,62 @@ public void filterResponse() throws Exception
         results = ((QueryResponse) this.componentManager.getComponentUnderTest().execute(query).get(0)).getResults();
         assertEquals(Arrays.asList(alice, bob), results);
     }
-}
+
+    @Test
+    public void filterResponseWithException() throws Exception
+    {
+        ParameterizedType resolverType =
+            new DefaultParameterizedType(null, DocumentReferenceResolver.class, SolrDocument.class);
+        DocumentReferenceResolver<SolrDocument> resolver = this.componentManager.getInstance(resolverType);
+
+        AuthorizationManager authorizationManager = this.componentManager.getInstance(AuthorizationManager.class);
+
+        DocumentReference currentUserReference = new DocumentReference("xwiki", "XWiki", "currentuser");
+        this.oldCore.getXWikiContext().setUserReference(currentUserReference);
+
+        DocumentReference aliceReference = new DocumentReference("wiki", "Users", "Alice");
+        SolrDocument alice = new SolrDocument();
+        when(resolver.resolve(alice)).thenReturn(aliceReference);
+
+        DocumentReference bobReference = new DocumentReference("wiki", "Users", "Bob");
+        when(authorizationManager.hasAccess(Right.VIEW, currentUserReference, bobReference)).thenReturn(true);
+        SolrDocument bob = new SolrDocument();
+        when(resolver.resolve(bob)).thenReturn(bobReference);
+
+        SolrDocumentList sourceResults = new SolrDocumentList();
+        sourceResults.addAll(Arrays.asList(alice, bob));
+        sourceResults.setNumFound(2);
+
+        QueryResponse response = mock(QueryResponse.class);
+        when(this.solr.query(any(SolrParams.class))).thenReturn(response);
+
+        DefaultQuery query = new DefaultQuery("", null);
+
+        // No right check, verify that the setup works.
+        when(response.getResults()).thenReturn((SolrDocumentList) sourceResults.clone());
+        SolrDocumentList results =
+            ((QueryResponse) this.componentManager.getComponentUnderTest().execute(query).get(0)).getResults();
+        assertEquals(Arrays.asList(alice, bob), results);
+        assertEquals(2, results.getNumFound());
+
+        // Check current user right
+        query.checkCurrentUser(true);
+
+        // Throw an exception when resolving Alice
+        when(resolver.resolve(alice)).thenThrow(new RuntimeException("Alice"));
+        when(response.getResults()).thenReturn((SolrDocumentList) sourceResults.clone());
+
+        results = ((QueryResponse) this.componentManager.getComponentUnderTest().execute(query).get(0)).getResults();
+        assertEquals(Collections.singletonList(bob), results);
+        assertEquals(1, results.getNumFound());
+
+        // Throw also an exception when resolving Bob
+        when(resolver.resolve(bob)).thenThrow(new RuntimeException("Bob"));
+        when(response.getResults()).thenReturn((SolrDocumentList) sourceResults.clone());
+
+        // Assert that the results are empty when both throw an exception
+        results = ((QueryResponse) this.componentManager.getComponentUnderTest().execute(query).get(0)).getResults();
+        assertEquals(Collections.emptyList(), results);
+        assertEquals(0, results.getNumFound());
+    }
+}
\ No newline at end of file