XWIKI-22424: Improve attachment filtering

Author: tmortagne

xwiki-platform-core/xwiki-platform-rest/xwiki-platform-rest-server/src/main/java/org/xwiki/rest/internal/resources/BaseAttachmentsResource.java

@@ -56,6 +56,8 @@
 import org.xwiki.rest.internal.Utils;
 import org.xwiki.rest.model.jaxb.Attachment;
 import org.xwiki.rest.model.jaxb.Attachments;
+import org.xwiki.security.authorization.ContextualAuthorizationManager;
+import org.xwiki.security.authorization.Right;
 import org.xwiki.user.UserReferenceResolver;
 
 import com.xpn.xwiki.XWiki;
@@ -113,6 +115,9 @@ public boolean isAlreadyExisting()
         FILTER_TO_QUERY.put("author", "attachment.author");
     }
 
+    @Inject
+    protected ContextualAuthorizationManager authorization;
+
     @Inject
     private ModelFactory modelFactory;
 
@@ -157,15 +162,19 @@ protected Attachments getAttachments(EntityReference scope, Map<String, String>
 
             List<Object> queryResults = getAttachmentsQuery(scope, filters).setLimit(limit).setOffset(offset).execute();
             attachments.withAttachments(queryResults.stream().map(this::processAttachmentsQueryResult)
+                // Apply passed filters
                 .filter(getFileTypeFilter(filters.getOrDefault(FILTER_FILE_TYPES, "")))
+                // Filter out attachments the current user is not allowed to see
+                .filter(a -> authorization.hasAccess(Right.VIEW, a.getReference()))
+                // Convert XWikiAttachment to REST Attachment
                 .map(xwikiAttachment -> toRestAttachment(xwikiAttachment, withPrettyNames))
                 .toList());
         } catch (QueryException e) {
             throw new XWikiRestException(e);
         } finally {
             xcontext.setWikiId(database);
         }
-
+        
         return attachments;
     }
 

xwiki-platform-core/xwiki-platform-rest/xwiki-platform-rest-server/src/main/java/org/xwiki/rest/internal/resources/attachments/AttachmentsResourceImpl.java

@@ -49,7 +49,6 @@
 import org.xwiki.rest.model.jaxb.Attachments;
 import org.xwiki.rest.resources.attachments.AttachmentResource;
 import org.xwiki.rest.resources.attachments.AttachmentsResource;
-import org.xwiki.security.authorization.ContextualAuthorizationManager;
 import org.xwiki.security.authorization.Right;
 
 import com.xpn.xwiki.XWikiException;
@@ -64,9 +63,6 @@ public class AttachmentsResourceImpl extends BaseAttachmentsResource implements
 {
     private static final String NAME = "name";
 
-    @Inject
-    private ContextualAuthorizationManager authorization;
-
     @Inject
     private Container container;
 

xwiki-platform-core/xwiki-platform-rest/xwiki-platform-rest-server/src/test/java/org/xwiki/rest/internal/resources/attachments/AttachmentsResourceImplTest.java

@@ -66,6 +66,7 @@
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.ArgumentMatchers.same;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
@@ -157,6 +158,8 @@ void getAttachments() throws Exception
             new Object[] {"Path.To", "Page", "1.3", imageAttachment});
         when(query.execute()).thenReturn(results);
 
+        when(this.authorization.hasAccess(same(Right.VIEW), any())).thenReturn(true);
+
         DocumentReference documentReference = new DocumentReference("test", Arrays.asList("Path", "To"), "Page");
         when(this.defaultSpaceReferenceResover.resolve(eq("Path.To"), any()))
             .thenReturn(documentReference.getLastSpaceReference());

xwiki-platform-core/xwiki-platform-rest/xwiki-platform-rest-server/src/test/java/org/xwiki/rest/internal/resources/spaces/SpaceAttachmentsResourceImplTest.java

@@ -19,17 +19,22 @@
  */
 package org.xwiki.rest.internal.resources.spaces;
 
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.xwiki.model.reference.AttachmentReference;
 import org.xwiki.model.reference.SpaceReference;
 import org.xwiki.query.Query;
 import org.xwiki.rest.internal.resources.AbstractAttachmentsResourceTest;
 import org.xwiki.rest.model.jaxb.Attachment;
 import org.xwiki.rest.model.jaxb.Attachments;
+import org.xwiki.security.authorization.ContextualAuthorizationManager;
+import org.xwiki.security.authorization.Right;
 import org.xwiki.test.junit5.mockito.InjectMockComponents;
+import org.xwiki.test.junit5.mockito.MockComponent;
 
 import com.xpn.xwiki.doc.XWikiAttachment;
 import com.xpn.xwiki.test.junit5.mockito.OldcoreTest;
@@ -52,6 +57,9 @@ class SpaceAttachmentsResourceImplTest extends AbstractAttachmentsResourceTest
     @InjectMockComponents
     private SpaceAttachmentsResourceImpl spaceAttachmentsResource;
 
+    @MockComponent
+    private ContextualAuthorizationManager authorization;
+
     @BeforeEach
     @Override
     public void setUp() throws Exception
@@ -74,7 +82,17 @@ void getAttachments() throws Exception
         when(query.setLimit(5)).thenReturn(query);
 
         XWikiAttachment xwikiAttachment = mock(XWikiAttachment.class);
-        List<Object> results = Collections.singletonList(new Object[] {"Path.To", "Page", "1.3", xwikiAttachment});
+        AttachmentReference xwikiAttachmentReference = mock(AttachmentReference.class, "image");
+        when(xwikiAttachment.getReference()).thenReturn(xwikiAttachmentReference);
+        when(this.authorization.hasAccess(Right.VIEW, xwikiAttachmentReference)).thenReturn(true);
+
+        XWikiAttachment forbiddenAttachment = mock(XWikiAttachment.class);
+        AttachmentReference forbiddenAttachmentReference = mock(AttachmentReference.class, "forbidden");
+        when(forbiddenAttachment.getReference()).thenReturn(forbiddenAttachmentReference);
+        when(this.authorization.hasAccess(Right.VIEW, forbiddenAttachmentReference)).thenReturn(false);
+
+        List<Object> results = Arrays.asList(new Object[] {"Path.To", "Page", "1.3", xwikiAttachment},
+            new Object[] {"Path.To", "ForbiddenPage", "1.3", forbiddenAttachment});
         when(query.execute()).thenReturn(results);
 
         SpaceReference spaceReference = new SpaceReference("test", "Path", "To");
@@ -89,6 +107,8 @@ void getAttachments() throws Exception
             this.spaceAttachmentsResource.getAttachments("test", "Path/spaces/To", "", "xyz", "", "", 10, 5, false);
 
         verify(query).bindValue("localSpaceReference", "Path.To");
+        verify(this.authorization).hasAccess(Right.VIEW, xwikiAttachmentReference);
+        verify(this.authorization).hasAccess(Right.VIEW, forbiddenAttachmentReference);
 
         assertEquals(Collections.singletonList(attachment), attachments.getAttachments());
     }

xwiki-platform-core/xwiki-platform-rest/xwiki-platform-rest-server/src/test/java/org/xwiki/rest/internal/resources/wikis/WikiAttachmentsResourceImplTest.java

@@ -19,17 +19,22 @@
  */
 package org.xwiki.rest.internal.resources.wikis;
 
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.xwiki.model.reference.AttachmentReference;
 import org.xwiki.model.reference.SpaceReference;
 import org.xwiki.query.Query;
 import org.xwiki.rest.internal.resources.AbstractAttachmentsResourceTest;
 import org.xwiki.rest.model.jaxb.Attachment;
 import org.xwiki.rest.model.jaxb.Attachments;
+import org.xwiki.security.authorization.ContextualAuthorizationManager;
+import org.xwiki.security.authorization.Right;
 import org.xwiki.test.junit5.mockito.InjectMockComponents;
+import org.xwiki.test.junit5.mockito.MockComponent;
 
 import com.xpn.xwiki.doc.XWikiAttachment;
 import com.xpn.xwiki.test.junit5.mockito.OldcoreTest;
@@ -38,6 +43,7 @@
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 /**
@@ -51,6 +57,9 @@ class WikiAttachmentsResourceImplTest extends AbstractAttachmentsResourceTest
     @InjectMockComponents
     private WikiAttachmentsResourceImpl wikiAttachmentsResource;
 
+    @MockComponent
+    private ContextualAuthorizationManager authorization;
+
     @BeforeEach
     @Override
     public void setUp() throws Exception
@@ -71,7 +80,17 @@ void getAttachments() throws Exception
         when(query.setLimit(10)).thenReturn(query);
 
         XWikiAttachment xwikiAttachment = mock(XWikiAttachment.class);
-        List<Object> results = Collections.singletonList(new Object[] {"Path.To", "Page", "1.3", xwikiAttachment});
+        AttachmentReference xwikiAttachmentReference = mock(AttachmentReference.class, "image");
+        when(xwikiAttachment.getReference()).thenReturn(xwikiAttachmentReference);
+        when(this.authorization.hasAccess(Right.VIEW, xwikiAttachmentReference)).thenReturn(true);
+
+        XWikiAttachment forbiddenAttachment = mock(XWikiAttachment.class);
+        AttachmentReference forbiddenAttachmentReference = mock(AttachmentReference.class, "forbidden");
+        when(forbiddenAttachment.getReference()).thenReturn(forbiddenAttachmentReference);
+        when(this.authorization.hasAccess(Right.VIEW, forbiddenAttachmentReference)).thenReturn(false);
+
+        List<Object> results = Arrays.asList(new Object[] {"Path.To", "Page", "1.3", xwikiAttachment},
+            new Object[] {"Path.To", "ForbiddenPage", "1.3", forbiddenAttachment});
         when(query.execute()).thenReturn(results);
 
         when(this.defaultSpaceReferenceResover.resolve(eq("Path.To"), any()))
@@ -84,6 +103,9 @@ void getAttachments() throws Exception
         Attachments attachments =
             this.wikiAttachmentsResource.getAttachments("test", "", "", "abc", "", "", 0, 10, true);
 
+        verify(this.authorization).hasAccess(Right.VIEW, xwikiAttachmentReference);
+        verify(this.authorization).hasAccess(Right.VIEW, forbiddenAttachmentReference);
+
         assertEquals(Collections.singletonList(attachment), attachments.getAttachments());
     }
 }