org.xwiki.platform:xwiki-platform-attachment-api vulnerable to Missing Authorization on Attachment Move

@@ -38,6 +38,7 @@
 import org.xwiki.model.reference.EntityReferenceSerializer;
 import org.xwiki.refactoring.internal.job.AbstractEntityJob;
 import org.xwiki.refactoring.job.EntityJobStatus;
+import org.xwiki.security.authorization.Right;
 import org.xwiki.user.UserReference;
 import org.xwiki.user.UserReferenceResolver;
 
@@ -106,19 +107,41 @@ protected void process(EntityReference source)
         // Update the author for the attribution of the attachment uploader.
         this.modelBridge.setContextUserReference(this.request.getUserReference());
         try {
-            this.progressManager.startStep(this);
-            moveAttachment(source, destination, autoRedirect, wiki);
-            this.progressManager.endStep(this);
-
-            this.progressManager.startStep(this);
-            this.observationManager.notify(new AttachmentMovedEvent((AttachmentReference) source, destination), this,
-                this.request);
-            this.progressManager.endStep(this);
+            if (checkMoveRights(source, destination)) {
+                this.progressManager.startStep(this);
+                moveAttachment(source, destination, autoRedirect, wiki);
+                this.progressManager.endStep(this);
+
+                this.progressManager.startStep(this);
+                this.observationManager.notify(new AttachmentMovedEvent((AttachmentReference) source, destination),
+                    this,
+                    this.request);
+                this.progressManager.endStep(this);
+            }
         } finally {
             this.progressManager.popLevelProgress(this);
         }
     }
 
+    private boolean checkMoveRights(EntityReference source, EntityReference destination)
+    {
+        // Check view and edit right on source to ensure that the user doesn't get view right through the move.
+        // While edit right implies view right, the view right might be explicitly denied.
+        boolean hasSourceRight = hasAccess(Right.VIEW, source) && hasAccess(Right.EDIT, source);
+        boolean hasDestinationRight = hasAccess(Right.EDIT, destination);
+
+        if (!hasSourceRight) {
+            this.logger.error("You don't have sufficient permissions over the source attachment [{}].", source);
+        } else if (!hasDestinationRight) {
+            // The destination's document might be the same as the source, therefore, only log the error when there is
+            // no error regarding the source.
+            this.logger.error("You don't have sufficient permissions over the destination attachment [{}].",
+                destination);
+        }
+
+        return hasSourceRight && hasDestinationRight;
+    }
+
     private void moveAttachment(EntityReference source, AttachmentReference destination, boolean autoRedirect,
         XWiki wiki)
     {

@@ -25,6 +25,8 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 import org.mockito.Mock;
 import org.xwiki.attachment.internal.AttachmentsManager;
 import org.xwiki.attachment.internal.RedirectAttachmentClassDocumentInitializer;
@@ -36,6 +38,8 @@
 import org.xwiki.model.reference.EntityReference;
 import org.xwiki.model.reference.EntityReferenceSerializer;
 import org.xwiki.refactoring.internal.ModelBridge;
+import org.xwiki.security.authorization.AuthorizationManager;
+import org.xwiki.security.authorization.Right;
 import org.xwiki.test.LogLevel;
 import org.xwiki.test.junit5.LogCaptureExtension;
 import org.xwiki.test.junit5.mockito.ComponentTest;
@@ -54,8 +58,11 @@
 
 import static java.util.Collections.singletonList;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoInteractions;
@@ -77,6 +84,8 @@ class MoveAttachmentJobTest
 
     private static final DocumentReference AUTHOR_REFERENCE = new DocumentReference("xwiki", "XWiki", "User1");
 
+    private static final DocumentReference USER2_REFERENCE = new DocumentReference("xwiki", "XWiki", "User2");
+
     private static final DocumentReference TARGET_LOCATION = new DocumentReference("xwiki", "Space", "Target");
 
     private static final AttachmentReference TARGET_ATTACHMENT_LOCATION =
@@ -101,12 +110,15 @@ class MoveAttachmentJobTest
     private AttachmentsManager attachmentsManager;
 
     @MockComponent
-    protected ModelBridge modelBridge;
+    private ModelBridge modelBridge;
 
     @MockComponent
     @Named("document")
     private UserReferenceResolver<DocumentReference> documentReferenceUserReferenceResolver;
 
+    @MockComponent
+    private AuthorizationManager authorizationManager;
+
     @Mock
     private XWikiContext context;
 
@@ -129,7 +141,7 @@ class MoveAttachmentJobTest
     private UserReference authorReference;
 
     @RegisterExtension
-    LogCaptureExtension logCapture = new LogCaptureExtension(LogLevel.WARN);
+    private LogCaptureExtension logCapture = new LogCaptureExtension(LogLevel.WARN);
 
     private MoveAttachmentRequest request;
 
@@ -153,6 +165,12 @@ void setUp() throws Exception
         when(this.referenceSerializer.serialize(TARGET_LOCATION)).thenReturn("xwiki:Space.Target");
         when(this.referenceSerializer.serialize(SOURCE_LOCATION)).thenReturn("xwiki:Space.Source");
         when(this.documentReferenceUserReferenceResolver.resolve(AUTHOR_REFERENCE)).thenReturn(this.authorReference);
+
+        // Grant global view and edit right.
+        when(this.authorizationManager.hasAccess(eq(Right.VIEW), eq(AUTHOR_REFERENCE), any(AttachmentReference.class)))
+            .thenReturn(true);
+        when(this.authorizationManager.hasAccess(eq(Right.EDIT), eq(AUTHOR_REFERENCE), any(AttachmentReference.class)))
+            .thenReturn(true);
     }
 
     @Test
@@ -164,6 +182,7 @@ void process() throws Exception
         this.request.setProperty(MoveAttachmentRequest.AUTO_REDIRECT, true);
         this.request.setInteractive(false);
         this.request.setUserReference(AUTHOR_REFERENCE);
+        this.request.setAuthorReference(AUTHOR_REFERENCE);
 
         XWikiAttachment sourceAttachment = mock(XWikiAttachment.class);
         XWikiAttachment targetAttachment = mock(XWikiAttachment.class);
@@ -204,6 +223,8 @@ void processTargetSaveFail() throws Exception
         this.request.setProperty(MoveAttachmentRequest.DESTINATION, TARGET_ATTACHMENT_LOCATION);
         this.request.setProperty(MoveAttachmentRequest.AUTO_REDIRECT, true);
         this.request.setInteractive(false);
+        this.request.setUserReference(AUTHOR_REFERENCE);
+        this.request.setAuthorReference(AUTHOR_REFERENCE);
 
         XWikiAttachment sourceAttachment = mock(XWikiAttachment.class);
         XWikiAttachment targetAttachment = mock(XWikiAttachment.class);
@@ -257,6 +278,7 @@ void processRename() throws Exception
         this.request.setProperty(MoveAttachmentRequest.AUTO_REDIRECT, true);
         this.request.setInteractive(false);
         this.request.setUserReference(AUTHOR_REFERENCE);
+        this.request.setAuthorReference(AUTHOR_REFERENCE);
 
         XWikiAttachment sourceAttachment = mock(XWikiAttachment.class);
         XWikiAttachment targetAttachment = mock(XWikiAttachment.class);
@@ -282,4 +304,45 @@ void processRename() throws Exception
         verify(this.sourceAuthors, times(2)).setEffectiveMetadataAuthor(this.authorReference);
         verify(this.sourceAuthors, times(2)).setOriginalMetadataAuthor(this.authorReference);
     }
+
+    @ParameterizedTest
+    @CsvSource({
+        "true, true",
+        "false, true",
+        "true, false",
+        "false, false"
+    })
+    void failWithoutRights(boolean canView, boolean canEdit) throws Exception
+    {
+        // Request initialization.
+        this.request.setEntityReferences(singletonList(SOURCE_ATTACHMENT_LOCATION));
+        this.request.setProperty(MoveAttachmentRequest.DESTINATION, TARGET_ATTACHMENT_LOCATION);
+        this.request.setProperty(MoveAttachmentRequest.AUTO_REDIRECT, true);
+        this.request.setInteractive(false);
+        this.request.setUserReference(USER2_REFERENCE);
+        this.request.setAuthorReference(AUTHOR_REFERENCE);
+
+        when(this.authorizationManager.hasAccess(Right.EDIT, USER2_REFERENCE, SOURCE_ATTACHMENT_LOCATION))
+            .thenReturn(canEdit);
+        when(this.authorizationManager.hasAccess(Right.VIEW, USER2_REFERENCE, SOURCE_ATTACHMENT_LOCATION))
+            .thenReturn(canView);
+        when(this.authorizationManager.hasAccess(Right.EDIT, USER2_REFERENCE, TARGET_ATTACHMENT_LOCATION))
+            .thenReturn(false);
+
+        this.job.process(SOURCE_ATTACHMENT_LOCATION);
+
+        // Verify nothing has been modified.
+        verifyNoInteractions(this.sourceDocument, this.targetDocument);
+        verifyNoInteractions(this.attachmentsManager);
+        verify(this.wiki, never()).saveDocument(any(), any(), any());
+        verifyNoInteractions(this.targetAuthors, this.sourceAuthors);
+
+        if (!canEdit || !canView) {
+            assertEquals("You don't have sufficient permissions over the source attachment "
+                + "[Attachment xwiki:Space.Source@oldName].", this.logCapture.getMessage(0));
+        } else {
+            assertEquals("You don't have sufficient permissions over the destination attachment "
+                + "[Attachment xwiki:Space.Target@newName].", this.logCapture.getMessage(0));
+        }
+    }
 }