XWiki Platform's Mail.MailConfig can be edited by any user with edit rights

@@ -20,12 +20,12 @@
 package org.xwiki.security.internal;
 
 import java.util.List;
-import java.util.stream.Collectors;
 
 import javax.inject.Inject;
 import javax.inject.Provider;
 import javax.inject.Singleton;
 
+import org.apache.commons.lang3.StringUtils;
 import org.slf4j.Logger;
 import org.xwiki.component.annotation.Component;
 import org.xwiki.security.authorization.Right;
@@ -36,6 +36,7 @@
 import com.xpn.xwiki.doc.XWikiDocument;
 import com.xpn.xwiki.objects.BaseObject;
 
+import static java.util.stream.Collectors.joining;
 import static org.xwiki.security.authorization.Right.DELETE;
 import static org.xwiki.security.authorization.Right.EDIT;
 import static org.xwiki.security.authorization.Right.VIEW;
@@ -76,28 +77,28 @@ public class DocumentInitializerRightsManager
     public boolean restrictToAdmin(XWikiDocument document)
     {
         boolean updated = false;
+        List<Right> rights = List.of(VIEW, EDIT, DELETE);
+
+        if (fixBadlyInitializedRights(document, rights)) {
+            updated = true;
+        }
+
         // If some rights have already been set on the document, we consider that it has already been protected 
         // manually.
         if (document.getXObjects(LOCAL_CLASS_REFERENCE).isEmpty()) {
-            updated = initializeRights(document, XWIKI_ADMIN_GROUP_DOCUMENT_REFERENCE, List.of(VIEW, EDIT, DELETE));
+            updated = updated || initializeRights(document, rights);
         }
 
         return updated;
     }
 
-    private boolean initializeRights(XWikiDocument document, String xwikiAdminGroupDocumentReference,
-        List<Right> rights)
+    private boolean initializeRights(XWikiDocument document, List<Right> rights)
     {
         boolean updated = false;
 
         try {
-            XWikiContext xwikiContext = this.xcontextProvider.get();
-            BaseObject object = document.newXObject(LOCAL_CLASS_REFERENCE, xwikiContext);
-            XWikiContext xWikiContext = this.xcontextProvider.get();
-            object.set(GROUPS_FIELD_NAME, xwikiAdminGroupDocumentReference, xWikiContext);
-            object.set(LEVELS_FIELD_NAME, rights.stream().map(Right::getName).collect(Collectors.toList()),
-                xWikiContext);
-            object.set(ALLOW_FIELD_NAME, 1, xWikiContext);
+            BaseObject object = document.newXObject(LOCAL_CLASS_REFERENCE, this.xcontextProvider.get());
+            setRights(object, rights);
             updated = true;
         } catch (XWikiException e) {
             this.logger.error(String.format("Error adding a [%s] object to the document [%s]", LOCAL_CLASS_REFERENCE,
@@ -106,4 +107,33 @@ private boolean initializeRights(XWikiDocument document, String xwikiAdminGroupD
 
         return updated;
     }
+
+    /**
+     * Because of XWIKI-20519, the levels of the rights of {@code XWiki.XWikiAdminGroup} can be badly initialized, and
+     * needs to be fixed. This is the case when upgrading from instances running exactly version 14.10.5 or 15.1-rc-1.
+     *
+     * @param document the document to fix
+     * @param rights the rights to set to the {@code XWiki.XWikiAdminGroup} group
+     */
+    private boolean fixBadlyInitializedRights(XWikiDocument document, List<Right> rights)
+    {
+        boolean updated = false;
+        if (document.getXObjects(LOCAL_CLASS_REFERENCE).size() == 1) {
+            BaseObject object = document.getXObject(LOCAL_CLASS_REFERENCE);
+            if (StringUtils.isEmpty(object.getStringValue(LEVELS_FIELD_NAME))
+                && StringUtils.isEmpty(object.getLargeStringValue(GROUPS_FIELD_NAME)))
+            {
+                setRights(object, rights);
+                updated = true;
+            }
+        }
+        return updated;
+    }
+
+    private void setRights(BaseObject object, List<Right> rights)
+    {
+        object.setLargeStringValue(GROUPS_FIELD_NAME, XWIKI_ADMIN_GROUP_DOCUMENT_REFERENCE);
+        object.setStringValue(LEVELS_FIELD_NAME, rights.stream().map(Right::getName).collect(joining(",")));
+        object.setIntValue(ALLOW_FIELD_NAME, 1);
+    }
 }

@@ -92,18 +92,33 @@ void setUp(MockitoComponentManager componentManager) throws Exception
     @Test
     void restrictToAdminSkipWhenAlreadyHasRights() throws Exception
     {
-        this.document.newXObject(LOCAL_CLASS_REFERENCE, this.xWikiContext);
+        BaseObject baseObject = this.document.newXObject(LOCAL_CLASS_REFERENCE, this.xWikiContext);
+        baseObject.setLargeStringValue(LEVELS_FIELD_NAME, "edit");
         assertFalse(this.rightsManager.restrictToAdmin(this.document));
     }
 
+    @Test
+    void restrictToAdminBadlyInitialized() throws Exception
+    {
+        BaseObject object = this.document.newXObject(LOCAL_CLASS_REFERENCE, this.xWikiContext);
+        object.setLargeStringValue(GROUPS_FIELD_NAME, "");
+        object.setLargeStringValue(LEVELS_FIELD_NAME, "");
+        object.setIntValue(ALLOW_FIELD_NAME, 1);
+        assertTrue(this.rightsManager.restrictToAdmin(this.document));
+        BaseObject xObject = this.document.getXObject(LOCAL_CLASS_REFERENCE);
+        assertEquals("XWiki.XWikiAdminGroup", xObject.getStringValue(GROUPS_FIELD_NAME));
+        assertEquals("view,edit,delete", xObject.getStringValue(LEVELS_FIELD_NAME));
+        assertEquals("1", xObject.getStringValue(ALLOW_FIELD_NAME));
+    }
+
     @Test
     void restrictToAdmin()
     {
         assertTrue(this.rightsManager.restrictToAdmin(this.document));
         assertEquals(1, this.document.getXObjects(LOCAL_CLASS_REFERENCE).size());
         BaseObject xObject = this.document.getXObject(LOCAL_CLASS_REFERENCE);
         assertEquals("XWiki.XWikiAdminGroup", xObject.getStringValue(GROUPS_FIELD_NAME));
-        assertEquals("view, edit, delete", xObject.getStringValue(LEVELS_FIELD_NAME));
+        assertEquals("view,edit,delete", xObject.getStringValue(LEVELS_FIELD_NAME));
         assertEquals("1", xObject.getStringValue(ALLOW_FIELD_NAME));
     }
 

@@ -113,5 +113,10 @@
       <version>${commons.version}</version>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.xwiki.platform</groupId>
+      <artifactId>xwiki-platform-security-authorization-bridge</artifactId>
+      <version>${project.version}</version>
+    </dependency>
   </dependencies>
 </project>

@@ -32,6 +32,7 @@
 import org.xwiki.model.reference.DocumentReference;
 import org.xwiki.model.reference.EntityReference;
 import org.xwiki.model.reference.LocalDocumentReference;
+import org.xwiki.security.internal.DocumentInitializerRightsManager;
 import org.xwiki.wiki.descriptor.WikiDescriptorManager;
 
 import javax.inject.Inject;
@@ -62,6 +63,9 @@ public class MailConfigMandatoryDocumentInitializer implements MandatoryDocument
     @Inject
     private Provider<XWikiContext> xcontextProvider;
 
+    @Inject
+    private DocumentInitializerRightsManager documentInitializerRightsManager;
+
     @Override
     public EntityReference getDocumentReference()
     {
@@ -107,6 +111,10 @@ public boolean updateDocument(XWikiDocument document)
             needsUpdate = true;
         }
 
+        if (this.documentInitializerRightsManager.restrictToAdmin(document)) {
+            needsUpdate = true;
+        }
+
         return needsUpdate;
     }
 

@@ -19,6 +19,16 @@
  */
 package org.xwiki.mail.internal;
 
+import javax.inject.Provider;
+
+import org.junit.jupiter.api.Test;
+import org.xwiki.model.reference.DocumentReference;
+import org.xwiki.model.reference.LocalDocumentReference;
+import org.xwiki.security.internal.DocumentInitializerRightsManager;
+import org.xwiki.test.junit5.mockito.InjectMockComponents;
+import org.xwiki.test.junit5.mockito.MockComponent;
+import org.xwiki.wiki.descriptor.WikiDescriptorManager;
+
 import com.xpn.xwiki.XWiki;
 import com.xpn.xwiki.XWikiContext;
 import com.xpn.xwiki.doc.XWikiDocument;
@@ -27,17 +37,13 @@
 import com.xpn.xwiki.test.junit5.mockito.InjectMockitoOldcore;
 import com.xpn.xwiki.test.junit5.mockito.OldcoreTest;
 import com.xpn.xwiki.test.reference.ReferenceComponentList;
-import org.junit.jupiter.api.Test;
-import org.xwiki.model.reference.DocumentReference;
-import org.xwiki.model.reference.LocalDocumentReference;
-import org.xwiki.test.junit5.mockito.InjectMockComponents;
-import org.xwiki.test.junit5.mockito.MockComponent;
-import org.xwiki.wiki.descriptor.WikiDescriptorManager;
 
-import javax.inject.Provider;
-
-import static org.junit.jupiter.api.Assertions.*;
-import static org.mockito.Mockito.*;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 /**
  * Unit tests for {@link MailConfigMandatoryDocumentInitializer}.
@@ -60,6 +66,9 @@ public class MailConfigMandatoryDocumentInitializerTest
     @InjectMockitoOldcore
     private MockitoOldcore oldcore;
 
+    @MockComponent
+    private DocumentInitializerRightsManager documentInitializerRightsManager;
+
     @Test
     public void updateDocument()
     {
@@ -81,5 +90,6 @@ public void updateDocument()
             document.getXObject(new LocalDocumentReference("Mail", "SendMailConfigClass"));
         assertNotNull(sendMailConfigObject);
         assertNotNull(document.getXObject(new LocalDocumentReference("Mail", "GeneralMailConfigClass")));
+        verify(this.documentInitializerRightsManager).restrictToAdmin(document);
     }
 }

@@ -31,7 +31,7 @@
   <name>XWiki Platform - Security - Authorization - Bridge</name>
   <description>Authorization API implementation using oldcore</description>
   <properties>
-    <xwiki.jacoco.instructionRatio>0.70</xwiki.jacoco.instructionRatio>
+    <xwiki.jacoco.instructionRatio>0.71</xwiki.jacoco.instructionRatio>
     <!-- TODO: Remove once the tests have been fixed to not output anything to the console! -->
     <xwiki.surefire.captureconsole.skip>true</xwiki.surefire.captureconsole.skip>
     <!-- Name to display by the Extension Manager -->

@@ -0,0 +1,110 @@
+/*
+ * 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.security.internal;
+
+import java.util.List;
+import java.util.stream.Collectors;
+
+import javax.inject.Inject;
+import javax.inject.Provider;
+import javax.inject.Singleton;
+
+import org.slf4j.Logger;
+import org.xwiki.component.annotation.Component;
+import org.xwiki.security.authorization.Right;
+
+import com.xpn.xwiki.XWikiContext;
+import com.xpn.xwiki.XWikiException;
+import com.xpn.xwiki.doc.XWikiDocument;
+import com.xpn.xwiki.objects.BaseObject;
+
+import static org.xwiki.security.authorization.Right.DELETE;
+import static org.xwiki.security.authorization.Right.EDIT;
+import static org.xwiki.security.authorization.Right.VIEW;
+import static org.xwiki.security.internal.XWikiConstants.ALLOW_FIELD_NAME;
+import static org.xwiki.security.internal.XWikiConstants.GROUPS_FIELD_NAME;
+import static org.xwiki.security.internal.XWikiConstants.LEVELS_FIELD_NAME;
+import static org.xwiki.security.internal.XWikiConstants.LOCAL_CLASS_REFERENCE;
+
+/**
+ * Service provided to {@link com.xpn.xwiki.doc.MandatoryDocumentInitializer}s to initialize documents with the correct
+ * rights.
+ *
+ * @version $Id$
+ * @since 14.4.8
+ * @since 14.10.5
+ * @since 15.1RC1
+ */
+@Component(roles = DocumentInitializerRightsManager.class)
+@Singleton
+public class DocumentInitializerRightsManager
+{
+    private static final String XWIKI_ADMIN_GROUP_DOCUMENT_REFERENCE = "XWiki.XWikiAdminGroup";
+
+    @Inject
+    private Provider<XWikiContext> xcontextProvider;
+
+    @Inject
+    private Logger logger;
+
+    /**
+     * Restrict the rights of the provided document so that it can only be viewed, edited and deleted by the
+     * {@code XWiki.XWikiAdminGroup} group. Note that this restriction is only applied if no rights are already applied
+     * on the current document.
+     * <br>
+     *
+     * @param document the document to updated rights on
+     * @return {@code true} if the document has been modified, {@code false} otherwise (including if the document hasn't
+     *     been modified because of an error during the modification)
+     */
+    public boolean restrictToAdmin(XWikiDocument document)
+    {
+        boolean updated = false;
+        // If some rights have already been set on the document, we consider that it has already been protected 
+        // manually.
+        if (document.getXObjects(LOCAL_CLASS_REFERENCE).isEmpty()) {
+            updated = initializeRights(document, XWIKI_ADMIN_GROUP_DOCUMENT_REFERENCE, List.of(VIEW, EDIT, DELETE));
+        }
+
+        return updated;
+    }
+
+    private boolean initializeRights(XWikiDocument document, String xwikiAdminGroupDocumentReference,
+        List<Right> rights)
+    {
+        boolean updated = false;
+
+        try {
+            XWikiContext xwikiContext = this.xcontextProvider.get();
+            BaseObject object = document.newXObject(LOCAL_CLASS_REFERENCE, xwikiContext);
+            XWikiContext xWikiContext = this.xcontextProvider.get();
+            object.set(GROUPS_FIELD_NAME, xwikiAdminGroupDocumentReference, xWikiContext);
+            object.set(LEVELS_FIELD_NAME, rights.stream().map(Right::getName).collect(Collectors.toList()),
+                xWikiContext);
+            object.set(ALLOW_FIELD_NAME, 1, xWikiContext);
+            updated = true;
+        } catch (XWikiException e) {
+            this.logger.error(String.format("Error adding a [%s] object to the document [%s]", LOCAL_CLASS_REFERENCE,
+                document.getDocumentReference()));
+        }
+
+        return updated;
+    }
+}

@@ -6,3 +6,4 @@ org.xwiki.security.authorization.internal.DefaultSecurityEntryReader
 org.xwiki.security.authorization.internal.RightsFilterListener
 org.xwiki.security.internal.DefaultUserBridge
 org.xwiki.security.internal.DefaultXWikiBridge
+org.xwiki.security.internal.DocumentInitializerRightsManager

@@ -0,0 +1,122 @@
+/*
+ * 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.security.internal;
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.xwiki.model.reference.DocumentReference;
+import org.xwiki.model.reference.EntityReference;
+import org.xwiki.test.LogLevel;
+import org.xwiki.test.annotation.AllComponents;
+import org.xwiki.test.junit5.LogCaptureExtension;
+import org.xwiki.test.junit5.mockito.InjectMockComponents;
+import org.xwiki.test.mockito.MockitoComponentManager;
+
+import com.xpn.xwiki.XWikiContext;
+import com.xpn.xwiki.XWikiException;
+import com.xpn.xwiki.doc.MandatoryDocumentInitializer;
+import com.xpn.xwiki.doc.XWikiDocument;
+import com.xpn.xwiki.internal.mandatory.XWikiRightsDocumentInitializer;
+import com.xpn.xwiki.objects.BaseObject;
+import com.xpn.xwiki.test.MockitoOldcore;
+import com.xpn.xwiki.test.junit5.mockito.InjectMockitoOldcore;
+import com.xpn.xwiki.test.junit5.mockito.OldcoreTest;
+
+import ch.qos.logback.classic.Level;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.spy;
+import static org.xwiki.security.internal.XWikiConstants.ALLOW_FIELD_NAME;
+import static org.xwiki.security.internal.XWikiConstants.GROUPS_FIELD_NAME;
+import static org.xwiki.security.internal.XWikiConstants.LEVELS_FIELD_NAME;
+import static org.xwiki.security.internal.XWikiConstants.LOCAL_CLASS_REFERENCE;
+
+/**
+ * Test of {@link DocumentInitializerRightsManager}.
+ *
+ * @version $Id$
+ */
+@OldcoreTest
+@AllComponents
+class DocumentInitializerRightsManagerTest
+{
+    @InjectMockitoOldcore
+    private MockitoOldcore oldcore;
+
+    @InjectMockComponents
+    private DocumentInitializerRightsManager rightsManager;
+
+    @RegisterExtension
+    LogCaptureExtension logCapture = new LogCaptureExtension(LogLevel.WARN);
+
+    private XWikiDocument document;
+
+    private XWikiContext xWikiContext;
+
+    @BeforeEach
+    void setUp(MockitoComponentManager componentManager) throws Exception
+    {
+        this.xWikiContext = this.oldcore.getXWikiContext();
+        String wikiId = this.xWikiContext.getWikiId();
+        this.document = this.oldcore.getSpyXWiki()
+            .getDocument(new DocumentReference(wikiId, "Space", "Page"), this.xWikiContext);
+        XWikiDocument xWikiRigths = this.oldcore.getSpyXWiki()
+            .getDocument(new DocumentReference(wikiId, "XWiki", "XWikiRights"), this.xWikiContext);
+        componentManager.<MandatoryDocumentInitializer>getInstance(MandatoryDocumentInitializer.class,
+            XWikiRightsDocumentInitializer.CLASS_REFERENCE_STRING).updateDocument(xWikiRigths);
+        this.oldcore.getSpyXWiki().saveDocument(xWikiRigths, this.xWikiContext);
+    }
+
+    @Test
+    void restrictToAdminSkipWhenAlreadyHasRights() throws Exception
+    {
+        this.document.newXObject(LOCAL_CLASS_REFERENCE, this.xWikiContext);
+        assertFalse(this.rightsManager.restrictToAdmin(this.document));
+    }
+
+    @Test
+    void restrictToAdmin()
+    {
+        assertTrue(this.rightsManager.restrictToAdmin(this.document));
+        assertEquals(1, this.document.getXObjects(LOCAL_CLASS_REFERENCE).size());
+        BaseObject xObject = this.document.getXObject(LOCAL_CLASS_REFERENCE);
+        assertEquals("XWiki.XWikiAdminGroup", xObject.getStringValue(GROUPS_FIELD_NAME));
+        assertEquals("view, edit, delete", xObject.getStringValue(LEVELS_FIELD_NAME));
+        assertEquals("1", xObject.getStringValue(ALLOW_FIELD_NAME));
+    }
+
+    @Test
+    void restrictToAdminWithException() throws XWikiException
+    {
+        this.document = spy(this.document);
+        doThrow(XWikiException.class).when(this.document)
+            .newXObject(any(EntityReference.class), any(XWikiContext.class));
+        assertFalse(this.rightsManager.restrictToAdmin(this.document));
+        assertEquals(0, this.document.getXObjects(LOCAL_CLASS_REFERENCE).size());
+        assertEquals("Error adding a [XWiki.XWikiRights] object to the document [xwiki:Space.Page]",
+            this.logCapture.getMessage(0));
+        assertEquals(Level.ERROR, this.logCapture.getLogEvent(0).getLevel());
+    }
+}