Skip to content

Commit

Permalink
XWIKI-22787: Pinned Pages are lost on space move for non admin user (#…
Browse files Browse the repository at this point in the history 
…3886)

  * Start work to have specific handling of WebPreferences in case of
    move/rename/copy of a full space
  * Fix unused import
  * Provide integration test
  * Fix wrong conditions
  * Add unit tests
  • Loading branch information
@surli
surli authored Feb 26, 2025
1 parent c3b7dbb commit 594fd60
Show file tree
Hide file tree
Showing 8 changed files with 328 additions and 46 deletions.
45 changes: 41 additions & 4 deletions ...tform-flamingo-skin-test-docker/src/test/it/org/xwiki/flamingo/test/docker/ViewersIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,13 @@
import org.xwiki.flamingo.skin.test.po.SiblingsPage;
import org.xwiki.livedata.test.po.TableLayoutElement;
import org.xwiki.model.reference.DocumentReference;
import org.xwiki.repository.test.SolrTestUtils;
import org.xwiki.test.docker.junit5.TestConfiguration;
import org.xwiki.test.docker.junit5.TestReference;
import org.xwiki.test.docker.junit5.UITest;
import org.xwiki.test.ui.TestUtils;
import org.xwiki.test.ui.po.CopyOrRenameOrDeleteStatusPage;
import org.xwiki.test.ui.po.RenamePage;

import static org.hamcrest.CoreMatchers.hasItem;
import static org.junit.jupiter.api.Assertions.assertEquals;
Expand Down Expand Up @@ -106,7 +110,8 @@ void siblings(TestUtils testUtils, TestReference testReference)
*/
@Test
@Order(2)
void children(TestUtils testUtils, TestReference testReference)
void children(TestUtils testUtils, TestReference testReference, TestConfiguration testConfiguration)
throws Exception
{
DocumentReference childADocumentReference =
new DocumentReference("ChildA", testReference.getLastSpaceReference());
Expand All @@ -120,7 +125,7 @@ void children(TestUtils testUtils, TestReference testReference)
testUtils.createPage(childBDocumentReference, "", "ChildB");
testUtils.createUser(EDIT_USER, EDIT_USER, null);
testUtils.loginAsSuperAdmin();
testUtils.setRights(testReference, null, EDIT_USER, "edit", true);
testUtils.setRightsOnSpace(testReference.getLastSpaceReference(), null, EDIT_USER, "edit,delete", true);

testUtils.forceGuestUser();
testUtils.gotoPage(testReference);
Expand All @@ -147,7 +152,8 @@ void children(TestUtils testUtils, TestReference testReference)

TableLayoutElement adminTableLayoutElement =
ChildrenPage.goToPage(testReference).getLiveData().getTableLayout();
assertEquals(2, adminTableLayoutElement.countRows());
// WebPreferences is visible by superadmin
assertEquals(3, adminTableLayoutElement.countRows());
adminTableLayoutElement
.assertCellWithLink(ChildrenPage.LIVE_DATA_TITLE, "ChildA", testUtils.getURL(childADocumentReference));
adminTableLayoutElement
Expand All @@ -166,13 +172,15 @@ void children(TestUtils testUtils, TestReference testReference)
assertTrue(childrenPage.hasTabs());

PinnedChildPagesTab pinnedChildPagesTab = childrenPage.openPinnedChildPagesTab();
assertEquals(List.of("ChildA", "ChildB"), pinnedChildPagesTab.getNavigationTree().getTopLevelPages());
assertEquals(List.of("ChildA", "ChildB"),
pinnedChildPagesTab.getNavigationTree().getTopLevelPages());
assertFalse(pinnedChildPagesTab.isPinned("ChildA"));
assertFalse(pinnedChildPagesTab.isPinned("ChildB"));

pinnedChildPagesTab.dragBefore("ChildB", "ChildA");
assertTrue(pinnedChildPagesTab.isPinned("ChildB"));
pinnedChildPagesTab.save();

childrenPage = ChildrenPage.goToPage(testReference);
pinnedChildPagesTab = childrenPage.openPinnedChildPagesTab();
assertEquals(List.of("ChildB", "ChildA"), pinnedChildPagesTab.getNavigationTree().getTopLevelPages());
Expand All @@ -187,5 +195,34 @@ void children(TestUtils testUtils, TestReference testReference)
assertEquals(List.of("ChildA", "ChildB"), pinnedChildPagesTab.getNavigationTree().getTopLevelPages());
assertFalse(pinnedChildPagesTab.isPinned("ChildA"));
assertFalse(pinnedChildPagesTab.isPinned("ChildB"));

pinnedChildPagesTab.pinPage("ChildB");
assertTrue(pinnedChildPagesTab.isPinned("ChildA"));
assertTrue(pinnedChildPagesTab.isPinned("ChildB"));
pinnedChildPagesTab.save();

testUtils.loginAsSuperAdmin();
new SolrTestUtils(testUtils, testConfiguration.getServletEngine()).waitEmptyQueue();
childrenPage = ChildrenPage.goToPage(testReference);
pinnedChildPagesTab = childrenPage.openPinnedChildPagesTab();
assertEquals(List.of("ChildA", "ChildB", "WebPreferences"),
pinnedChildPagesTab.getNavigationTree().getTopLevelPages());
assertTrue(pinnedChildPagesTab.isPinned("ChildA"));
assertTrue(pinnedChildPagesTab.isPinned("ChildB"));
assertFalse(pinnedChildPagesTab.isPinned("WebPreferences"));

testUtils.login(EDIT_USER, EDIT_USER);
// Check that pinned page are preserved after a move
RenamePage renamePage = testUtils.gotoPage(testReference).rename();
renamePage.setPreserveChildren(true);
renamePage.getDocumentPicker().setTitle("ChildrenRenamed");
CopyOrRenameOrDeleteStatusPage statusPage = renamePage.clickRenameButton().waitUntilFinished();
statusPage.gotoNewPage();
childrenPage = ChildrenPage.clickOnChildrenMenu();

pinnedChildPagesTab = childrenPage.openPinnedChildPagesTab();
assertEquals(List.of("ChildA", "ChildB"), pinnedChildPagesTab.getNavigationTree().getTopLevelPages());
assertTrue(pinnedChildPagesTab.isPinned("ChildA"));
assertTrue(pinnedChildPagesTab.isPinned("ChildB"));
}
}
69 changes: 37 additions & 32 deletions ...factoring-api/src/main/java/org/xwiki/refactoring/internal/job/AbstractCopyOrMoveJob.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,7 @@

import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.function.BiConsumer;
import java.util.stream.Collectors;

import org.xwiki.model.EntityType;
import org.xwiki.model.reference.DocumentReference;
Expand Down Expand Up @@ -309,16 +307,36 @@ protected void process(final SpaceReference source, final SpaceReference destina

protected boolean checkAllRights(DocumentReference oldReference, DocumentReference newReference) throws Exception
{
if (!hasAccess(Right.VIEW, oldReference)) {
this.logger.error("You don't have sufficient permissions over the source document [{}].", oldReference);
return false;
} else if (!hasAccess(Right.EDIT, newReference)
|| (this.modelBridge.exists(newReference) && !hasAccess(Right.DELETE, newReference))) {
this.logger.error("You don't have sufficient permissions over the destination document [{}].",
newReference);
return false;
boolean isWebPreferences =
isSpacePreferencesReference(oldReference) && isSpacePreferencesReference(newReference);
DocumentReference oldHomeReference = getSpaceHomeReference(oldReference);
boolean result = true;
// if we're trying to move a WebPreferences not as part of moving a WebHome we treat it with standard rights
// like we do for other pages.
if (!isWebPreferences || !this.concernedEntities.containsKey(oldHomeReference)) {
if (!hasAccess(Right.VIEW, oldReference)) {
this.logger.error("You don't have sufficient permissions over the source document [{}].", oldReference);
result = false;
} else if (!hasAccess(Right.EDIT, newReference)
|| (this.modelBridge.exists(newReference) && !hasAccess(Right.DELETE, newReference))) {
this.logger.error("You don't have sufficient permissions over the destination document [{}].",
newReference);
result = false;
}
// else if we're moving a WebHome as part of moving its WebHome we check rights on the space itself.
// Note that we don't perform other checks regarding overwriting because WebPreferences is checked last:
// - if there's only a WebPreferences document alone in the destination then it's ok to overwrite it as
// we're creating a new space (since we move WebHome)
// - if we're overwriting also WebHome then we already performed the check if it's allowed to overwrite it
} else {
if (!hasAccess(Right.EDIT, oldHomeReference)) {
this.logger.error("You don't have sufficient permissions over the home document of WebPreferences "
+ "[{}].",
newReference);
result = false;
}
}
return true;
return result;
}

protected void maybePerformRefactoring(DocumentReference oldReference, DocumentReference newReference)
Expand Down Expand Up @@ -352,14 +370,14 @@ protected boolean copyOrMove(DocumentReference oldReference, DocumentReference n

// Step 2: Delete the destination document if needed.
this.progressManager.startStep(this);
if (this.modelBridge.exists(newReference)) {
if (this.request.isInteractive() && !this.modelBridge.canOverwriteSilently(newReference)
&& !confirmOverwrite(oldReference, newReference)) {
this.logger.warn(
"Skipping [{}] because [{}] already exists and the user doesn't want to overwrite it.",
oldReference, newReference);
return false;
}
if (this.modelBridge.exists(newReference)
&& this.request.isInteractive()
&& !this.modelBridge.canOverwriteSilently(newReference)
&& !confirmOverwrite(oldReference, newReference)) {
this.logger.warn(
"Skipping [{}] because [{}] already exists and the user doesn't want to overwrite it.",
oldReference, newReference);
return false;
}
this.progressManager.endStep(this);

Expand Down Expand Up @@ -416,19 +434,6 @@ protected EntityReference getCommonParent()
return getCommonParent(entityReferences);
}

/**
* @return the list of references that have been selected to be refactored.
* @since 16.10.0RC1
*/
public Map<EntityReference, EntityReference> getSelectedEntities()
{
return this.concernedEntities.values().stream()
.filter(EntitySelection::isSelected)
.filter(entity -> entity.getTargetEntityReference().isPresent())
.collect(Collectors.toMap(EntitySelection::getEntityReference,
entity -> entity.getTargetEntityReference().get()));
}

/**
* Atomic operation to perform: should be a rename for Rename/Move and copy for Copy.
* @param source the source reference
Expand Down
12 changes: 9 additions & 3 deletions ...m-refactoring-api/src/main/java/org/xwiki/refactoring/internal/job/AbstractEntityJob.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,9 @@ public interface Visitor<T>
void visit(T node);
}

private static final JobGroupPath ROOT_GROUP = new JobGroupPath(RefactoringJobs.GROUP, null);
protected static final String PREFERENCES_DOCUMENT_NAME = "WebPreferences";

private static final String PREFERENCES_DOCUMENT_NAME = "WebPreferences";
private static final JobGroupPath ROOT_GROUP = new JobGroupPath(RefactoringJobs.GROUP, null);

/**
* The component used to access the XWiki model and to perform low level operations on it.
Expand Down Expand Up @@ -261,7 +261,13 @@ protected boolean isSpaceHomeReference(DocumentReference documentReference)
.equals(this.defaultEntityReferenceProvider.getDefaultReference(documentReference.getType()).getName());
}

private boolean isSpacePreferencesReference(EntityReference entityReference)
protected DocumentReference getSpaceHomeReference(DocumentReference reference)
{
String referenceName = this.defaultEntityReferenceProvider.getDefaultReference(EntityType.DOCUMENT).getName();
return new DocumentReference(referenceName, reference.getLastSpaceReference());
}

protected boolean isSpacePreferencesReference(EntityReference entityReference)
{
return entityReference.getType() == EntityType.DOCUMENT
&& PREFERENCES_DOCUMENT_NAME.equals(entityReference.getName());
Expand Down
18 changes: 18 additions & 0 deletions ...ing-api/src/main/java/org/xwiki/refactoring/internal/job/AbstractEntityJobWithChecks.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.HashMap;
import java.util.Locale;
import java.util.Map;
import java.util.stream.Collectors;

import org.xwiki.bridge.event.DocumentsDeletingEvent;
import org.xwiki.model.reference.DocumentReference;
Expand Down Expand Up @@ -63,13 +64,30 @@ protected void runInternal() throws Exception
// Process
progressManager.startStep(this);
setContextUser();

// FIXME: this should probably be the selected entities only...
process(entityReferences);
}
} finally {
progressManager.popLevelProgress(this);
}
}

/**
* @return the list of references that have been selected to be refactored.
* @since 17.2.0RC1
* @since 16.10.5
* @since 16.4.7
*/
public Map<EntityReference, EntityReference> getSelectedEntities()
{
return this.concernedEntities.values().stream()
.filter(EntitySelection::isSelected)
.filter(entity -> entity.getTargetEntityReference().isPresent())
.collect(Collectors.toMap(EntitySelection::getEntityReference,
entity -> entity.getTargetEntityReference().get()));
}

protected void getEntities(Collection<EntityReference> entityReferences)
{
this.progressManager.pushLevelProgress(entityReferences.size(), this);
Expand Down
15 changes: 14 additions & 1 deletion ...ki-platform-refactoring-api/src/main/java/org/xwiki/refactoring/internal/job/MoveJob.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,20 @@ protected void getEntities(Collection<EntityReference> entityReferences)
@Override
protected boolean checkAllRights(DocumentReference oldReference, DocumentReference newReference) throws Exception
{
if (!hasAccess(Right.DELETE, oldReference)) {
boolean isWebPreferences =
isSpacePreferencesReference(oldReference) && isSpacePreferencesReference(newReference);
DocumentReference oldHomeReference = getSpaceHomeReference(oldReference);
// if it's a WebPreferences document and the operation concerns also its WebHome then we check the rights of the
// WebHome only.
if (isWebPreferences && this.concernedEntities.containsKey(oldHomeReference)) {
if (!hasAccess(Right.DELETE, oldHomeReference)) {
this.logger.error("You don't have sufficient permissions over the home document of WebPreferences "
+ "[{}].",
newReference);
return false;
}
return super.checkAllRights(oldReference, newReference);
} else if (!hasAccess(Right.DELETE, oldReference)) {
// The move operation is implemented as Copy + Delete.
this.logger.error("You are not allowed to delete [{}].", oldReference);
return false;
Expand Down
Loading

0 comments on commit 594fd60

Please sign in to comment.