Data leak through deleted documents

@@ -22,18 +22,23 @@
 import java.util.Calendar;
 import java.util.Date;
 import java.util.Locale;
-import java.util.Objects;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.xwiki.component.util.DefaultParameterizedType;
 import org.xwiki.model.reference.DocumentReference;
 import org.xwiki.security.authorization.Right;
+import org.xwiki.stability.Unstable;
+import org.xwiki.user.UserReference;
+import org.xwiki.user.UserReferenceResolver;
 
 import com.xpn.xwiki.XWikiContext;
 import com.xpn.xwiki.XWikiException;
 import com.xpn.xwiki.doc.XWikiDeletedDocument;
 import com.xpn.xwiki.doc.XWikiDocument;
+import com.xpn.xwiki.store.XWikiRecycleBinStoreInterface;
 import com.xpn.xwiki.util.Programming;
+import com.xpn.xwiki.web.Utils;
 
 /**
  * Information about a deleted document in the recycle bin.
@@ -52,6 +57,8 @@ public class DeletedDocument extends Api
      */
     private final XWikiDeletedDocument deletedDoc;
 
+    private UserReferenceResolver<DocumentReference> userReferenceResolver;
+
     /**
      * Simple constructor, initializes a new API object with the current {@link com.xpn.xwiki.XWikiContext context} and
      * the specified protected {@link com.xpn.xwiki.doc.XWikiDeletedDocument deleted document} object.
@@ -134,22 +141,46 @@ public String getBatchId()
         return this.deletedDoc.getBatchId();
     }
 
+    private UserReferenceResolver<DocumentReference> getUserReferenceResolver()
+    {
+        if (this.userReferenceResolver == null) {
+            this.userReferenceResolver = Utils.getComponent(
+                new DefaultParameterizedType(null, UserReferenceResolver.class, DocumentReference.class), "document");
+        }
+        return this.userReferenceResolver;
+    }
+
+    private boolean hasAccess(Right right)
+    {
+        UserReference userReference = getUserReferenceResolver().resolve(this.context.getUserReference());
+        XWikiRecycleBinStoreInterface recycleBinStore = this.context.getWiki().getRecycleBinStore();
+        return recycleBinStore.hasAccess(right, userReference, this.deletedDoc);
+    }
+
     /**
      * Check if the current user has the right to restore the document.
      *
      * @return {@code true} if the current user can restore this document, {@code false} otherwise
      */
     public boolean canUndelete()
     {
-        try {
-            return hasAccessLevel(ADMIN_RIGHT, getFullName()) || hasAccessLevel("undelete", getFullName())
-                || (Objects.equals(this.context.getUserReference(), getDeleterReference())
-                    && hasAccess(Right.EDIT, getDocumentReference()));
-        } catch (XWikiException ex) {
-            // Public APIs should not throw exceptions
-            LOGGER.warn("Exception while checking if entry [{}] can be restored from the recycle bin", getId(), ex);
-            return false;
-        }
+        return hasAccess(Right.EDIT);
+    }
+
+    /**
+     * Check if the current user has the right to view the deleted document.
+     * This is allowed either if the user has admin right on the original reference of the doc, or if they were the
+     * original user who deleted it.
+     *
+     * @return {code true} if the current user is allowed to view the deleted document.
+     * @since 14.10RC1
+     * @since 14.4.7
+     * @since 13.10.11
+     */
+    @Unstable
+    public boolean canView()
+    {
+        return hasAccess(Right.VIEW);
     }
 
     /**

@@ -21,6 +21,11 @@
 
 import org.xwiki.component.annotation.Role;
 import org.xwiki.model.reference.DocumentReference;
+import org.xwiki.security.authorization.AccessDeniedException;
+import org.xwiki.security.authorization.AuthorizationException;
+import org.xwiki.security.authorization.Right;
+import org.xwiki.stability.Unstable;
+import org.xwiki.user.UserReference;
 
 import com.xpn.xwiki.XWikiException;
 
@@ -56,4 +61,25 @@ public interface DocumentRevisionProvider
      * @throws XWikiException when failing to load the document revision
      */
     XWikiDocument getRevision(XWikiDocument document, String revision) throws XWikiException;
+
+    /**
+     * Check if access is granted on the given document revision, for the given user and right: if the access is not
+     * granted this method will throw an {@link AccessDeniedException}.
+     * This method allows each revision provider to have their own check depending on the type of revision.
+     *
+     * @param right the right for which to check if access is granted
+     * @param userReference the user for whom to check access
+     * @param documentReference the reference of the document
+     * @param revision the revision of the document
+     * @throws AuthorizationException if the access is denied
+     * @throws XWikiException in case of problem when loading the revision
+     * @since 14.10RC1
+     * @since 14.4.7
+     * @since 13.10.11
+     */
+    @Unstable
+    default void checkAccess(Right right, UserReference userReference, DocumentReference documentReference,
+        String revision) throws AuthorizationException, XWikiException
+    {
+    }
 }

@@ -24,6 +24,7 @@
 import java.util.Locale;
 
 import org.apache.commons.lang3.StringUtils;
+import org.apache.commons.lang3.builder.ToStringBuilder;
 import org.xwiki.localization.LocaleUtils;
 import org.xwiki.model.reference.DocumentReference;
 import org.xwiki.model.reference.DocumentReferenceResolver;
@@ -394,4 +395,14 @@ public String getBatchId()
     {
         return batchId;
     }
+
+    @Override
+    public String toString()
+    {
+        return new ToStringBuilder(this)
+            .append("id", id)
+            .append("fullName", fullName)
+            .append("locale", locale)
+            .toString();
+    }
 }

@@ -24,10 +24,14 @@
 import javax.inject.Provider;
 import javax.inject.Singleton;
 
+import org.apache.commons.lang3.tuple.Pair;
 import org.xwiki.component.annotation.Component;
 import org.xwiki.component.manager.ComponentLookupException;
 import org.xwiki.component.manager.ComponentManager;
 import org.xwiki.model.reference.DocumentReference;
+import org.xwiki.security.authorization.AuthorizationException;
+import org.xwiki.security.authorization.Right;
+import org.xwiki.user.UserReference;
 
 import com.xpn.xwiki.XWikiException;
 import com.xpn.xwiki.doc.DocumentRevisionProvider;
@@ -54,10 +58,8 @@ public class DefaultDocumentRevisionProvider extends AbstractDocumentRevisionPro
     @Named("database")
     private DocumentRevisionProvider databaseDocumentRevisionProvider;
 
-    @Override
-    public XWikiDocument getRevision(DocumentReference reference, String revision) throws XWikiException
+    private Pair<String, String> parseRevision(String revision)
     {
-        // Parse the version
         String revisionPrefix = null;
         if (revision != null) {
             int revisionPrefixIndex = revision.indexOf(':');
@@ -71,7 +73,11 @@ public XWikiDocument getRevision(DocumentReference reference, String revision) t
         } else {
             shortRevision = revision;
         }
+        return Pair.of(revisionPrefix, shortRevision);
+    }
 
+    private DocumentRevisionProvider getProvider(String revisionPrefix) throws XWikiException
+    {
         // Find the provider
         DocumentRevisionProvider provider = this.databaseDocumentRevisionProvider;
         if (revisionPrefix != null) {
@@ -80,12 +86,30 @@ public XWikiDocument getRevision(DocumentReference reference, String revision) t
                 try {
                     provider = componentManager.getInstance(DocumentRevisionProvider.class, revisionPrefix);
                 } catch (ComponentLookupException e) {
-                    throw new XWikiException("Failed to get revision provider for revision [" + revision + "]", e);
+                    throw new XWikiException("Failed to get revision provider for revision [" + revisionPrefix + "]",
+                        e);
                 }
             }
         }
+        return provider;
+    }
+
+    @Override
+    public XWikiDocument getRevision(DocumentReference reference, String revision) throws XWikiException
+    {
+        Pair<String, String> parsedRevision = parseRevision(revision);
 
         // Load the document revision
-        return provider.getRevision(reference, shortRevision);
+        return getProvider(parsedRevision.getLeft()).getRevision(reference, parsedRevision.getRight());
+    }
+
+    @Override
+    public void checkAccess(Right right, UserReference userReference, DocumentReference documentReference,
+        String revision) throws AuthorizationException, XWikiException
+    {
+        Pair<String, String> parsedRevision = parseRevision(revision);
+
+        getProvider(parsedRevision.getLeft())
+            .checkAccess(right, userReference, documentReference, parsedRevision.getRight());
     }
 }

@@ -26,6 +26,9 @@
 
 import org.xwiki.component.annotation.Component;
 import org.xwiki.model.reference.DocumentReference;
+import org.xwiki.security.authorization.AuthorizationException;
+import org.xwiki.security.authorization.Right;
+import org.xwiki.user.UserReference;
 
 import com.xpn.xwiki.XWikiContext;
 import com.xpn.xwiki.XWikiException;
@@ -61,12 +64,23 @@ public XWikiDocument getRevision(DocumentReference reference, String revision) t
         }
 
         return null;
-
     }
 
     @Override
     public XWikiDocument getRevision(XWikiDocument document, String revision) throws XWikiException
     {
         return getRevision(document != null ? document.getDocumentReferenceWithLocale() : null, revision);
     }
+
+    @Override
+    public void checkAccess(Right right, UserReference userReference, DocumentReference documentReference,
+        String revision) throws AuthorizationException, XWikiException
+    {
+        XWikiContext xcontext = this.xcontextProvider.get();
+
+        XWikiDeletedDocument deletedDocument = xcontext.getWiki().getDeletedDocument(Long.valueOf(revision), xcontext);
+        if (deletedDocument != null) {
+            xcontext.getWiki().getRecycleBinStore().checkAccess(right, userReference, deletedDocument);
+        }
+    }
 }

@@ -22,6 +22,7 @@
 import java.util.Date;
 import java.util.List;
 import java.util.Locale;
+import java.util.Objects;
 
 import javax.inject.Inject;
 import javax.inject.Named;
@@ -39,6 +40,12 @@
 import org.xwiki.component.annotation.Component;
 import org.xwiki.component.manager.ComponentLookupException;
 import org.xwiki.component.manager.ComponentManager;
+import org.xwiki.model.reference.DocumentReference;
+import org.xwiki.security.authorization.AuthorizationException;
+import org.xwiki.security.authorization.AuthorizationManager;
+import org.xwiki.security.authorization.Right;
+import org.xwiki.user.UserReference;
+import org.xwiki.user.UserReferenceSerializer;
 
 import com.xpn.xwiki.XWikiContext;
 import com.xpn.xwiki.XWikiException;
@@ -58,6 +65,13 @@
 @Singleton
 public class XWikiHibernateRecycleBinStore extends XWikiHibernateBaseStore implements XWikiRecycleBinStoreInterface
 {
+    @Inject
+    private AuthorizationManager authorizationManager;
+
+    @Inject
+    @Named("document")
+    private UserReferenceSerializer<DocumentReference> userReferenceSerializer;
+
     /**
      * {@link HibernateCallback} used to retrieve from the recycle bin store the deleted versions of a document.
      */
@@ -430,4 +444,31 @@ public void deleteFromRecycleBin(final long index, XWikiContext context, boolean
             return null;
         });
     }
+
+    @Override
+    public void checkAccess(Right right, UserReference userReference, XWikiDeletedDocument deletedDocument)
+        throws AuthorizationException
+    {
+        if (!this.hasAccess(right, userReference, deletedDocument)) {
+            throw new AuthorizationException(
+                String.format("[%s] cannot access deleted document [%s] for right [%s]: "
+                    + "only admin or deleter of the document are authorized",
+                    userReference, deletedDocument, right));
+        }
+    }
+
+    @Override
+    public boolean hasAccess(Right right, UserReference userReference, XWikiDeletedDocument deletedDocument)
+    {
+        DocumentReference documentReference = deletedDocument.getDocumentReference();
+        DocumentReference userDocReference = this.userReferenceSerializer.serialize(userReference);
+
+        boolean result = false;
+        if (this.authorizationManager.hasAccess(Right.ADMIN, userDocReference, documentReference)
+            || (Objects.equals(deletedDocument.getDeleterReference(), userDocReference)
+            && this.authorizationManager.hasAccess(right, userDocReference, documentReference))) {
+            result = true;
+        }
+        return result;
+    }
 }

@@ -22,6 +22,10 @@
 import java.util.Date;
 
 import org.xwiki.component.annotation.Role;
+import org.xwiki.security.authorization.AuthorizationException;
+import org.xwiki.security.authorization.Right;
+import org.xwiki.stability.Unstable;
+import org.xwiki.user.UserReference;
 
 import com.xpn.xwiki.XWikiContext;
 import com.xpn.xwiki.XWikiException;
@@ -232,4 +236,39 @@ default void deleteFromRecycleBin(long index, XWikiContext context, boolean bTra
         // unpredictable.
         deleteFromRecycleBin(new XWikiDocument(), index, context, bTransaction);
     }
+
+    /**
+     * Check if the given deleted document can be accessed for the given right by the given user.
+     * This method only throw the {@link AuthorizationException} if the right is not granted.
+     *
+     * @param right the right to check access for
+     * @param userReference the user for whom to check access
+     * @param deletedDocument the document to be accessed
+     * @throws AuthorizationException if the user doesn't have appropriate right
+     * @since 14.10RC1
+     * @since 14.4.7
+     * @since 13.10.11
+     */
+    @Unstable
+    default void checkAccess(Right right, UserReference userReference, XWikiDeletedDocument deletedDocument) throws
+        AuthorizationException
+    {
+    }
+
+    /**
+     * Check if the given deleted document can be accessed for the given right by the given user.
+     *
+     * @param right the right to check access for
+     * @param userReference the user for whom to check access
+     * @param deletedDocument the document to be accessed
+     * @return {@code true} if the user have appropriate right
+     * @since 14.10RC1
+     * @since 14.4.7
+     * @since 13.10.11
+     */
+    @Unstable
+    default boolean hasAccess(Right right, UserReference userReference, XWikiDeletedDocument deletedDocument)
+    {
+        return false;
+    }
 }

@@ -82,16 +82,20 @@
 import org.xwiki.resource.entity.EntityResourceReference;
 import org.xwiki.resource.internal.DefaultResourceReferenceHandlerChain;
 import org.xwiki.script.ScriptContextManager;
+import org.xwiki.security.authorization.AuthorizationException;
 import org.xwiki.security.authorization.ContextualAuthorizationManager;
 import org.xwiki.security.authorization.Right;
 import org.xwiki.stability.Unstable;
 import org.xwiki.template.TemplateManager;
+import org.xwiki.user.UserReference;
+import org.xwiki.user.UserReferenceResolver;
 import org.xwiki.velocity.VelocityManager;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.xpn.xwiki.XWiki;
 import com.xpn.xwiki.XWikiContext;
 import com.xpn.xwiki.XWikiException;
+import com.xpn.xwiki.doc.DocumentRevisionProvider;
 import com.xpn.xwiki.doc.XWikiDocument;
 import com.xpn.xwiki.internal.web.LegacyAction;
 import com.xpn.xwiki.monitor.api.MonitorPlugin;
@@ -185,6 +189,13 @@ public abstract class XWikiAction implements LegacyAction
 
     private EntityReferenceSerializer<String> localSerializer;
 
+    @Inject
+    private DocumentRevisionProvider documentRevisionProvider;
+
+    @Inject
+    @Named("document")
+    private UserReferenceResolver<DocumentReference> userReferenceResolver;
+
     /**
      * @return the class of the XWikiForm in charge of parsing the request
      * @since 13.0
@@ -892,6 +903,11 @@ protected boolean supportRedirections()
         return false;
     }
 
+    private UserReference getCurrentUserReference(XWikiContext context)
+    {
+        return this.userReferenceResolver.resolve(context.getUserReference());
+    }
+
     protected void handleRevision(XWikiContext context) throws XWikiException
     {
         String rev = context.getRequest().getParameter("rev");
@@ -906,11 +922,26 @@ protected void handleRevision(XWikiContext context) throws XWikiException
                 Locale locale = LocaleUtils.toLocale(context.getRequest().getParameter("language"), Locale.ROOT);
                 tdoc = new XWikiDocument(tdoc.getDocumentReference(), locale);
             }
-            XWikiDocument rdoc =
-                (!doc.getLocale().equals(tdoc.getLocale())) ? doc : context.getWiki().getDocument(doc, rev, context);
 
-            XWikiDocument rtdoc =
-                (doc.getLocale().equals(tdoc.getLocale())) ? rdoc : context.getWiki().getDocument(tdoc, rev, context);
+            DocumentReference documentReference = doc.getDocumentReference();
+            try {
+                documentRevisionProvider
+                    .checkAccess(Right.VIEW, getCurrentUserReference(context), documentReference, rev);
+            } catch (AuthorizationException e) {
+                Object[] args = { documentReference, rev, context.getUserReference() };
+                throw new XWikiException(XWikiException.MODULE_XWIKI_ACCESS, XWikiException.ERROR_XWIKI_ACCESS_DENIED,
+                    "Access to document {0} with revision {1} has been denied to user {2}", e, args);
+            }
+
+            XWikiDocument rdoc;
+            XWikiDocument rtdoc;
+            if (doc.getLocale().equals(tdoc.getLocale())) {
+                rdoc = this.documentRevisionProvider.getRevision(doc.getDocumentReferenceWithLocale(), rev);
+                rtdoc = rdoc;
+            } else {
+                rdoc = doc;
+                rtdoc = this.documentRevisionProvider.getRevision(tdoc.getDocumentReferenceWithLocale(), rev);
+            }
 
             context.put("tdoc", rtdoc);
             context.put("cdoc", rdoc);

@@ -43,6 +43,7 @@
 import com.xpn.xwiki.XWikiContext;
 import com.xpn.xwiki.doc.XWikiDeletedDocument;
 import com.xpn.xwiki.doc.XWikiDocument;
+import com.xpn.xwiki.store.XWikiHibernateRecycleBinStore;
 import com.xpn.xwiki.test.MockitoOldcore;
 import com.xpn.xwiki.test.junit5.mockito.InjectMockitoOldcore;
 import com.xpn.xwiki.test.junit5.mockito.OldcoreTest;
@@ -104,6 +105,9 @@ class UndeleteActionTest
     @Mock
     private XWikiDeletedDocument deletedDocument;
 
+    @Mock
+    private XWikiHibernateRecycleBinStore recycleBinStore;
+
     /**
      * The object being tested.
      */
@@ -113,7 +117,7 @@ class UndeleteActionTest
     void beforeEach() throws Exception
     {
         this.oldcore.getXWikiContext().setRequest(this.request);
-
+        this.oldcore.getSpyXWiki().setRecycleBinStore(this.recycleBinStore);
         XWikiDocument contextDocument = mock(XWikiDocument.class);
         when(contextDocument.getDocumentReference()).thenReturn(DELETED_REFERENCE);
         this.oldcore.getXWikiContext().setDoc(contextDocument);
@@ -143,8 +147,8 @@ void restoreSingleDocument() throws Exception
     {
         when(this.csrfToken.isTokenValid(null)).thenReturn(true);
 
-        when(this.oldcore.getMockRightService().hasAccessLevel(any(), any(), any(), any())).thenReturn(true);
-
+        when(this.recycleBinStore.hasAccess(any(), any(), any()))
+            .thenReturn(true);
         assertFalse(this.undeleteAction.action(this.oldcore.getXWikiContext()));
 
         verify(this.requestFactory).createRestoreRequest(Arrays.asList(ID));
@@ -157,7 +161,7 @@ void restoreSingleDocumentWhenDeleter() throws Exception
     {
         when(this.csrfToken.isTokenValid(null)).thenReturn(true);
 
-        when(this.oldcore.getMockAuthorizationManager().hasAccess(Right.EDIT, null, DELETED_REFERENCE))
+        when(this.recycleBinStore.hasAccess(any(), any(), any()))
             .thenReturn(true);
 
         assertFalse(this.undeleteAction.action(this.oldcore.getXWikiContext()));
@@ -205,8 +209,8 @@ void showBatch() throws Exception
     {
         when(this.request.getParameter("showBatch")).thenReturn("true");
 
-        when(this.oldcore.getMockRightService().hasAccessLevel(any(), any(), any(), any())).thenReturn(true);
-
+        when(this.recycleBinStore.hasAccess(any(), any(), any()))
+            .thenReturn(true);
         assertTrue(this.undeleteAction.action(this.oldcore.getXWikiContext()));
         // Render the "restore" template.
         assertEquals("restore", undeleteAction.render(this.oldcore.getXWikiContext()));
@@ -236,7 +240,8 @@ void restoreBatch() throws Exception
         // Confirmation button pressed.
         when(this.request.getParameter("confirm")).thenReturn("true");
 
-        when(this.oldcore.getMockRightService().hasAccessLevel(any(), any(), any(), any())).thenReturn(true);
+        when(this.recycleBinStore.hasAccess(any(), any(), any()))
+            .thenReturn(true);
 
         assertFalse(this.undeleteAction.action(this.oldcore.getXWikiContext()));
 

@@ -43,6 +43,7 @@
   #if($list && $list.size() > 0)
     #set ($canDelete = $list[0].canDelete())
     #set ($canRestore = $list[0].canUndelete())
+    #set ($canView = $list[0].canView())
     <hr />
     <div class="centered $!className">
       <p class="recyclebin-message">$escapetool.xml($services.localization.render($message))</p>
@@ -62,9 +63,13 @@
         <tr>
          <td>$xwiki.getUserName($dd.getDeleter())</td>
          <td>
+         #if ($canView)
            <a class="link-view" href="$doc.getURL('view', $escapetool.url({'rev' : "deleted:${dd.getId()}"}))">
              $xwiki.formatDate($dd.getDate())
            </a>
+         #else
+           $xwiki.formatDate($dd.getDate())
+         #end
          </td>
          #if ($canRestore)
            <td><a href="$xwiki.getURL($dd.fullName, 'undelete', "id=${dd.id}&amp;showBatch=true")">$!{dd.batchId}</a></td>