org.xwiki.platform:xwiki-platform-store-filesystem-oldcore has Exposed Dangerous Method or Function

@@ -126,6 +126,18 @@
                  Single justification example:
             -->
             
+            <revapi.differences>
+              <differences>
+                <item>
+                  <ignore>true</ignore>
+                  <code>java.method.returnTypeChanged</code>
+                  <old>method com.xpn.xwiki.doc.XWikiAttachment org.xwiki.store.script.TemporaryAttachmentsScriptService::uploadTemporaryAttachment(org.xwiki.model.reference.DocumentReference, java.lang.String)</old>
+                  <new>method com.xpn.xwiki.api.Attachment org.xwiki.store.script.TemporaryAttachmentsScriptService::uploadTemporaryAttachment(org.xwiki.model.reference.DocumentReference, java.lang.String)</new>
+                  <justification>Unstable API change: Script services are not supposed to expose classes from com.xpn.xwiki.doc.*</justification>
+                  <criticality>documented</criticality>
+                </item>
+              </differences>
+            </revapi.differences>
           </analysisConfiguration>
         </configuration>
       </plugin>

@@ -31,7 +31,7 @@
   <name>XWiki Platform - Store - Filesystem - Old Core</name>
   <description>Implement various oldcore store APIs based on filesystem.</description>
   <properties>
-    <xwiki.jacoco.instructionRatio>0.37</xwiki.jacoco.instructionRatio>
+    <xwiki.jacoco.instructionRatio>0.38</xwiki.jacoco.instructionRatio>
     <!-- Old names of this module used for retro compatibility when resolving dependencies of old extensions -->
     <xwiki.extension.features>org.xwiki.platform:xwiki-platform-store-filesystem-attachments</xwiki.extension.features>
   </properties>

@@ -38,6 +38,7 @@
 
 import com.xpn.xwiki.XWikiContext;
 import com.xpn.xwiki.doc.XWikiAttachment;
+import com.xpn.xwiki.doc.XWikiDocument;
 import com.xpn.xwiki.plugin.fileupload.FileUploadPlugin;
 
 /**
@@ -99,6 +100,10 @@ public XWikiAttachment uploadAttachment(DocumentReference documentReference, Par
             xWikiAttachment.setFilename(part.getSubmittedFileName());
             xWikiAttachment.setContent(part.getInputStream());
             xWikiAttachment.setAuthorReference(context.getUserReference());
+            // Initialize an empty document with the right document reference and locale. We don't set the actual 
+            // document since it's a temporary attachment, but it is still useful to have a minimal knowledge of the
+            // document it is stored for.
+            xWikiAttachment.setDoc(new XWikiDocument(documentReference, documentReference.getLocale()), false);
             temporaryAttachmentSession.addAttachment(documentReference, xWikiAttachment);
         } catch (IOException e) {
             throw new TemporaryAttachmentException("Error while reading the content of a request part", e);

@@ -20,6 +20,7 @@
 package org.xwiki.store.script;
 
 import java.io.IOException;
+import java.util.Optional;
 
 import javax.inject.Inject;
 import javax.inject.Named;
@@ -28,7 +29,6 @@
 import javax.servlet.ServletException;
 import javax.servlet.http.Part;
 
-import org.apache.commons.lang3.exception.ExceptionUtils;
 import org.slf4j.Logger;
 import org.xwiki.component.annotation.Component;
 import org.xwiki.model.reference.DocumentReference;
@@ -38,13 +38,17 @@
 import org.xwiki.store.TemporaryAttachmentSessionsManager;
 
 import com.xpn.xwiki.XWikiContext;
+import com.xpn.xwiki.api.Attachment;
+import com.xpn.xwiki.api.Document;
 import com.xpn.xwiki.doc.XWikiAttachment;
 
+import static org.apache.commons.lang3.exception.ExceptionUtils.getRootCauseMessage;
+
 /**
  * Script service dedicated to the handling of temporary attachments.
  *
- * @see TemporaryAttachmentSessionsManager
  * @version $Id$
+ * @see TemporaryAttachmentSessionsManager
  * @since 14.3RC1
  */
 @Component
@@ -66,25 +70,30 @@ public class TemporaryAttachmentsScriptService implements ScriptService
      * Temporary upload the attachment identified by the given field name: the request should be of type
      * {@code multipart/form-data}.
      *
-     * @param documentReference the target document reference the attachment should be later attached to.
-     * @param fieldName the name of the field of the uploaded data.
-     * @return a temporary {@link XWikiAttachment} not yet persisted.
-     *          attachment.
+     * @param documentReference the target document reference the attachment should be later attached to
+     * @param fieldName the name of the field of the uploaded data
+     * @return a temporary {@link Attachment} not yet persisted attachment, or {@code null} in case of error
      */
-    public XWikiAttachment uploadTemporaryAttachment(DocumentReference documentReference, String fieldName)
+    public Attachment uploadTemporaryAttachment(DocumentReference documentReference, String fieldName)
     {
         XWikiContext context = this.contextProvider.get();
-        XWikiAttachment result = null;
+        Optional<XWikiAttachment> result = Optional.empty();
         try {
             Part part = context.getRequest().getPart(fieldName);
             if (part != null) {
-                result = this.temporaryAttachmentSessionsManager.uploadAttachment(documentReference, part);
+                result = Optional.of(this.temporaryAttachmentSessionsManager.uploadAttachment(documentReference, part));
             }
         } catch (IOException | ServletException e) {
-            logger.warn("Error while reading the request content part: [{}]", ExceptionUtils.getRootCauseMessage(e));
+            this.logger.warn("Error while reading the request content part: [{}]", getRootCauseMessage(e));
         } catch (TemporaryAttachmentException e) {
-            logger.warn("Error while uploading the attachment: [{}]", ExceptionUtils.getRootCauseMessage(e));
+            this.logger.warn("Error while uploading the attachment: [{}]", getRootCauseMessage(e));
         }
-        return result;
+
+        return result.map(attachment -> {
+            Document document = Optional.ofNullable(attachment.getDoc())
+                .map(doc -> doc.newDocument(context))
+                .orElse(null);
+            return new Attachment(document, attachment, context);
+        }).orElse(null);
     }
 }

@@ -45,10 +45,10 @@
 import com.xpn.xwiki.XWiki;
 import com.xpn.xwiki.XWikiContext;
 import com.xpn.xwiki.doc.XWikiAttachment;
-import com.xpn.xwiki.plugin.fileupload.FileUploadPlugin;
 import com.xpn.xwiki.web.Utils;
 import com.xpn.xwiki.web.XWikiRequest;
 
+import static com.xpn.xwiki.plugin.fileupload.FileUploadPlugin.UPLOAD_MAXSIZE_PARAMETER;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -104,9 +104,7 @@ void uploadAttachment() throws Exception
         String sessionId = "mySession";
         when(this.httpSession.getId()).thenReturn(sessionId);
 
-        DocumentReference documentReference = mock(DocumentReference.class);
-        SpaceReference spaceReference = mock(SpaceReference.class);
-        when(documentReference.getLastSpaceReference()).thenReturn(spaceReference);
+        DocumentReference documentReference = new DocumentReference("xwiki", "Space", "Document");
         Part part = mock(Part.class);
 
         String filename = "fileFoo.xml";
@@ -118,22 +116,23 @@ void uploadAttachment() throws Exception
         when(this.context.getWiki()).thenReturn(xwiki);
         DocumentReference userReference = new DocumentReference("xwiki", "XWiki", "User");
         when(this.context.getUserReference()).thenReturn(userReference);
-        when(xwiki.getSpacePreference(FileUploadPlugin.UPLOAD_MAXSIZE_PARAMETER, spaceReference, this.context))
-            .thenReturn("42");
+        SpaceReference lastSpaceReference = documentReference.getLastSpaceReference();
+        when(xwiki.getSpacePreference(UPLOAD_MAXSIZE_PARAMETER, lastSpaceReference, this.context)).thenReturn("42");
         when(part.getSize()).thenReturn(41L);
 
         doAnswer(invocationOnMock -> {
             TemporaryAttachmentSession temporaryAttachmentSession = invocationOnMock.getArgument(1);
             assertEquals(sessionId, temporaryAttachmentSession.getSessionId());
             return null;
-        }).when(httpSession).setAttribute(eq(ATTRIBUTE_KEY), any(TemporaryAttachmentSession.class));
+        }).when(this.httpSession).setAttribute(eq(ATTRIBUTE_KEY), any(TemporaryAttachmentSession.class));
 
         XWikiAttachment attachment = this.attachmentManager.uploadAttachment(documentReference, part);
         assertNotNull(attachment);
         assertEquals(filename, attachment.getFilename());
         assertEquals(userReference, attachment.getAuthorReference());
+        assertEquals(documentReference, attachment.getDoc().getDocumentReference());
 
-        verify(httpSession).setAttribute(eq(ATTRIBUTE_KEY), any(TemporaryAttachmentSession.class));
+        verify(this.httpSession).setAttribute(eq(ATTRIBUTE_KEY), any(TemporaryAttachmentSession.class));
     }
 
     @Test

@@ -0,0 +1,169 @@
+/*
+ * See the NOTICE file distributed with this work for additional
+ * information regarding copyright ownership.
+ *
+ * This is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This software is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+package org.xwiki.store.script;
+
+import java.io.IOException;
+import java.util.stream.Stream;
+
+import javax.inject.Provider;
+import javax.servlet.ServletException;
+import javax.servlet.http.Part;
+
+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.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.mockito.Mock;
+import org.xwiki.model.reference.DocumentReference;
+import org.xwiki.store.TemporaryAttachmentException;
+import org.xwiki.store.TemporaryAttachmentSessionsManager;
+import org.xwiki.test.LogLevel;
+import org.xwiki.test.junit5.LogCaptureExtension;
+import org.xwiki.test.junit5.mockito.ComponentTest;
+import org.xwiki.test.junit5.mockito.InjectMockComponents;
+import org.xwiki.test.junit5.mockito.MockComponent;
+
+import com.xpn.xwiki.XWiki;
+import com.xpn.xwiki.XWikiContext;
+import com.xpn.xwiki.api.Attachment;
+import com.xpn.xwiki.doc.XWikiAttachment;
+import com.xpn.xwiki.doc.XWikiDocument;
+import com.xpn.xwiki.user.api.XWikiRightService;
+import com.xpn.xwiki.web.XWikiRequest;
+
+import ch.qos.logback.classic.Level;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+/**
+ * Test of {@link TemporaryAttachmentsScriptService}.
+ *
+ * @version $Id$
+ * @since 14.9RC1
+ */
+@ComponentTest
+class TemporaryAttachmentsScriptServiceTest
+{
+    private static final DocumentReference DOCUMENT_REFERENCE = new DocumentReference("xwiki", "XWiki", "Doc");
+
+    @InjectMockComponents
+    private TemporaryAttachmentsScriptService temporaryAttachmentsScriptService;
+
+    @MockComponent
+    private Provider<XWikiContext> contextProvider;
+
+    @MockComponent
+    private TemporaryAttachmentSessionsManager temporaryAttachmentSessionsManager;
+
+    @Mock
+    private XWikiContext context;
+
+    @Mock
+    private XWikiRequest request;
+
+    @Mock
+    private Part part;
+
+    @Mock
+    private XWiki wiki;
+
+    @Mock
+    private XWikiDocument xWikiDocument;
+
+    @RegisterExtension
+    LogCaptureExtension logCapture = new LogCaptureExtension(LogLevel.WARN);
+
+    @BeforeEach
+    void setUp() throws Exception
+    {
+        when(this.contextProvider.get()).thenReturn(this.context);
+        when(this.context.getRequest()).thenReturn(this.request);
+        when(this.context.getWiki()).thenReturn(this.wiki);
+        when(this.wiki.getDocument(DOCUMENT_REFERENCE, this.context)).thenReturn(this.xWikiDocument);
+        XWikiRightService xWikiRightService = mock(XWikiRightService.class);
+        when(this.wiki.getRightService()).thenReturn(xWikiRightService);
+        when(xWikiRightService.hasProgrammingRights(this.context)).thenReturn(true);
+    }
+
+    @Test
+    void uploadTemporaryAttachment() throws Exception
+    {
+        XWikiAttachment xWikiAttachment = mock(XWikiAttachment.class);
+
+        when(this.request.getPart("upload")).thenReturn(this.part);
+        when(this.temporaryAttachmentSessionsManager.uploadAttachment(DOCUMENT_REFERENCE, this.part))
+            .thenReturn(xWikiAttachment);
+
+        Attachment temporaryAttachment =
+            this.temporaryAttachmentsScriptService.uploadTemporaryAttachment(DOCUMENT_REFERENCE, "upload");
+
+        assertSame(xWikiAttachment, temporaryAttachment.getAttachment());
+
+        verify(this.temporaryAttachmentSessionsManager).uploadAttachment(DOCUMENT_REFERENCE, this.part);
+    }
+
+    @ParameterizedTest
+    @MethodSource("provideUploadTemporaryAttachmentWithException")
+    void uploadTemporaryAttachmentPartWithException(Class<? extends Throwable> exceptionType, String expectedMessage)
+        throws Exception
+    {
+        when(this.request.getPart("upload")).thenThrow(exceptionType);
+
+        assertNull(this.temporaryAttachmentsScriptService.uploadTemporaryAttachment(DOCUMENT_REFERENCE,
+            "upload"));
+
+        verify(this.temporaryAttachmentSessionsManager, never()).uploadAttachment(DOCUMENT_REFERENCE, this.part);
+        assertEquals(expectedMessage, this.logCapture.getMessage(0));
+        assertEquals(Level.WARN, this.logCapture.getLogEvent(0).getLevel());
+    }
+
+    public static Stream<Arguments> provideUploadTemporaryAttachmentWithException()
+    {
+        return Stream.of(
+            Arguments.of(IOException.class, "Error while reading the request content part: [IOException: ]"),
+            Arguments.of(ServletException.class, "Error while reading the request content part: [ServletException: ]")
+        );
+    }
+
+    @Test
+    void uploadTemporaryAttachmentWithException()
+        throws Exception
+    {
+        when(this.request.getPart("upload")).thenReturn(this.part);
+        when(this.temporaryAttachmentSessionsManager.uploadAttachment(DOCUMENT_REFERENCE, this.part))
+            .thenThrow(TemporaryAttachmentException.class);
+
+        assertNull(this.temporaryAttachmentsScriptService.uploadTemporaryAttachment(DOCUMENT_REFERENCE,
+            "upload"));
+
+        assertEquals("Error while uploading the attachment: [TemporaryAttachmentException: ]",
+            this.logCapture.getMessage(0));
+        assertEquals(Level.WARN, this.logCapture.getLogEvent(0).getLevel());
+    }
+}
+