Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XWIKI-21763: Disable the server-side image resizing while exporting to PDF #2834

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
/**
* Composite Configuration Source that looks in the following sources in that order:
* <ul>
* <li>execution context</li>
* <li>user preferences wiki page</li>
* <li>space preferences wiki page</li>
* <li>wiki preferences wiki page</li>
Expand Down Expand Up @@ -58,10 +59,15 @@ public class AllConfigurationSource extends CompositeConfigurationSource impleme
@Named("user")
private ConfigurationSource userPreferencesSource;

@Inject
@Named("executionContext")
private ConfigurationSource executionContextSource;

@Override
public void initialize() throws InitializationException
{
// First source is searched first when a property value is requested.
addConfigurationSource(this.executionContextSource);
addConfigurationSource(this.userPreferencesSource);
addConfigurationSource(this.documentsPreferencesSource);
addConfigurationSource(this.xwikiPropertiesSource);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
/**
* Composite Configuration Source that looks in the following sources in that order:
* <ul>
* <li>execution context</li>
* <li>documents sources (space first and then wiki preferences wiki pages)</li>
* <li>xwiki properties file (xwiki.properties)</li>
* </ul>
Expand All @@ -52,10 +53,15 @@ public class DefaultConfigurationSource extends CompositeConfigurationSource imp
@Named("documents")
private ConfigurationSource documentsSource;

@Inject
@Named("executionContext")
private ConfigurationSource executionContextSource;

@Override
public void initialize() throws InitializationException
{
// First source is looked up first when a property value is requested.
addConfigurationSource(this.executionContextSource);
addConfigurationSource(this.documentsSource);
addConfigurationSource(this.xwikiPropertiesSource);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@
*/
package org.xwiki.configuration.internal;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.mockito.Mockito.when;

import javax.inject.Named;

import org.junit.jupiter.api.Test;
Expand All @@ -27,10 +31,6 @@
import org.xwiki.test.junit5.mockito.InjectMockComponents;
import org.xwiki.test.junit5.mockito.MockComponent;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.mockito.Mockito.when;

/**
* Validate {@link AllConfigurationSource}.
*
Expand All @@ -51,6 +51,10 @@ class AllConfigurationSourceTest
@Named("user")
private ConfigurationSource userPreferencesSource;

@MockComponent
@Named("executionContext")
private ConfigurationSource executionContextSource;

@InjectMockComponents
private AllConfigurationSource configuration;

Expand All @@ -73,5 +77,10 @@ void getProperty()
when(this.userPreferencesSource.getProperty("key")).thenReturn("userPreferencesSource");

assertEquals("userPreferencesSource", this.configuration.getProperty("key"));

when(this.executionContextSource.containsKey("key")).thenReturn(true);
when(this.executionContextSource.getProperty("key")).thenReturn("executionContextSource");

assertEquals("executionContextSource", this.configuration.getProperty("key"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@
*/
package org.xwiki.configuration.internal;

import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.inOrder;
import static org.mockito.Mockito.when;

import javax.inject.Named;

import org.junit.jupiter.api.Test;
Expand All @@ -28,9 +32,6 @@
import org.xwiki.test.junit5.mockito.InjectMockComponents;
import org.xwiki.test.junit5.mockito.MockComponent;

import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.*;

/**
* Unit tests for {@link DefaultConfigurationSource}.
*
Expand All @@ -51,6 +52,10 @@ class DefaultConfigurationSourceTest
@Named("xwikiproperties")
private ConfigurationSource xwikiPropertiesSource;

@MockComponent
@Named("executionContext")
private ConfigurationSource executionContextSource;

@Test
void containsKey()
{
Expand All @@ -60,10 +65,12 @@ void containsKey()
assertTrue(this.source.containsKey("key"));

// Verify that the call order is correct
InOrder inOrder = inOrder(xwikiPropertiesSource, this.documentsSource);
InOrder inOrder = inOrder(xwikiPropertiesSource, this.documentsSource, this.executionContextSource);
// First call
inOrder.verify(this.documentsSource).containsKey("key");
inOrder.verify(this.executionContextSource).containsKey("key");
// Second call
inOrder.verify(this.documentsSource).containsKey("key");
// Third call
inOrder.verify(xwikiPropertiesSource).containsKey("key");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/*
* 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.export.pdf.internal.job;

import java.util.Arrays;

import javax.inject.Inject;

import org.xwiki.export.pdf.job.PDFExportJobRequest;
import org.xwiki.export.pdf.job.PDFExportJobStatus;
import org.xwiki.job.AbstractJob;
import org.xwiki.job.GroupedJob;
import org.xwiki.job.JobGroupPath;
import org.xwiki.model.reference.EntityReference;
import org.xwiki.security.authorization.AuthorizationManager;
import org.xwiki.security.authorization.Right;

/**
* Base class for PDF export job.
*
* @version $Id$
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing @since?

*/
public abstract class AbstractPDFExportJob extends AbstractJob<PDFExportJobRequest, PDFExportJobStatus>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think moving the rendering part of the Job to an internal component that you can then inject is a better way to fix the ClassFanOutcheck than moving to an abstract class.

implements GroupedJob
{
/**
* The PDF export job type.
*/
public static final String JOB_TYPE = "export/pdf";

/**
* Used to check access permissions.
*
* @see #hasAccess(Right, EntityReference)
*/
@Inject
private AuthorizationManager authorization;

@Override
public String getType()
{
return JOB_TYPE;
}

@Override
public JobGroupPath getGroupPath()
{
return new JobGroupPath(Arrays.asList("export", "pdf"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want a shorter version (and based on Java standard APIs).

Suggested change
return new JobGroupPath(Arrays.asList("export", "pdf"));
return new JobGroupPath(List.of("export", "pdf"));

}

@Override
protected PDFExportJobStatus createNewStatus(PDFExportJobRequest request)
{
return new PDFExportJobStatus(getType(), request, this.observationManager, this.loggerManager);
}

/**
* Check access rights taking into account the job request.
*
* @param right the access right to check
* @param reference the target entity reference
* @return return {@code true} if the current user or the entity author have the specified access right on the
* specified entity, depending on the job request
*/
protected boolean hasAccess(Right right, EntityReference reference)
{
return ((!this.request.isCheckRights()
|| this.authorization.hasAccess(right, this.request.getUserReference(), reference))
&& (!this.request.isCheckAuthorRights()
|| this.authorization.hasAccess(right, this.request.getAuthorReference(), reference)));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,29 +22,24 @@
import java.io.IOException;
import java.io.InputStream;
import java.net.URL;
import java.util.Arrays;
import java.util.List;
import java.util.Map;

import javax.inject.Inject;
import javax.inject.Named;
import javax.inject.Provider;

import org.xwiki.component.annotation.Component;
import org.xwiki.configuration.TemporaryConfigurationExecutor;
import org.xwiki.export.pdf.PDFExportConfiguration;
import org.xwiki.export.pdf.PDFPrinter;
import org.xwiki.export.pdf.internal.RequiredSkinExtensionsRecorder;
import org.xwiki.export.pdf.job.PDFExportJobRequest;
import org.xwiki.export.pdf.job.PDFExportJobStatus;
import org.xwiki.export.pdf.job.PDFExportJobStatus.DocumentRenderingResult;
import org.xwiki.job.AbstractJob;
import org.xwiki.job.GroupedJob;
import org.xwiki.job.JobGroupPath;
import org.xwiki.model.reference.DocumentReference;
import org.xwiki.model.reference.EntityReference;
import org.xwiki.model.reference.ObjectPropertyReference;
import org.xwiki.model.reference.ObjectReference;
import org.xwiki.resource.temporary.TemporaryResourceStore;
import org.xwiki.security.authorization.AuthorizationManager;
import org.xwiki.security.authorization.Right;

/**
Expand All @@ -56,21 +51,8 @@
*/
@Component
@Named(PDFExportJob.JOB_TYPE)
public class PDFExportJob extends AbstractJob<PDFExportJobRequest, PDFExportJobStatus> implements GroupedJob
public class PDFExportJob extends AbstractPDFExportJob
{
/**
* The PDF export job type.
*/
public static final String JOB_TYPE = "export/pdf";

/**
* Used to check access permissions.
*
* @see #hasAccess(Right, EntityReference)
*/
@Inject
private AuthorizationManager authorization;

@Inject
private DocumentRenderer documentRenderer;

Expand All @@ -94,30 +76,19 @@ public class PDFExportJob extends AbstractJob<PDFExportJobRequest, PDFExportJobS
@Inject
private PrintPreviewURLBuilder printPreviewURLBuilder;

@Override
public String getType()
{
return JOB_TYPE;
}

@Override
public JobGroupPath getGroupPath()
{
return new JobGroupPath(Arrays.asList("export", "pdf"));
}

@Override
protected PDFExportJobStatus createNewStatus(PDFExportJobRequest request)
{
return new PDFExportJobStatus(getType(), request, this.observationManager, this.loggerManager);
}
@Inject
private TemporaryConfigurationExecutor temporaryConfigurationExecutor;

@Override
protected void runInternal() throws Exception
{
if (!this.request.getDocuments().isEmpty()) {
this.requiredSkinExtensionsRecorder.start();
render(this.request.getDocuments());
this.temporaryConfigurationExecutor.call("executionContext",
Map.of("rendering.imageDimensionsIncludedInImageURL", false), () -> {
render(this.request.getDocuments());
return null;
});
if (!this.status.isCanceled()) {
this.status.setRequiredSkinExtensions(this.requiredSkinExtensionsRecorder.stop());
}
Expand Down Expand Up @@ -195,22 +166,6 @@ private int render(DocumentReference documentReference, DocumentRendererParamete
return renderingResult.getHTML().length();
}

/**
* Check access rights taking into account the job request.
*
* @param right the access right to check
* @param reference the target entity reference
* @return return {@code true} if the current user or the entity author have the specified access right on the
* specified entity, depending on the job request
*/
private boolean hasAccess(Right right, EntityReference reference)
{
return ((!this.request.isCheckRights()
|| this.authorization.hasAccess(right, this.request.getUserReference(), reference))
&& (!this.request.isCheckAuthorRights()
|| this.authorization.hasAccess(right, this.request.getAuthorReference(), reference)));
}

private void saveAsPDF() throws IOException
{
URL printPreviewURL = this.printPreviewURLBuilder.getPrintPreviewURL(this.request);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.fail;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
Expand All @@ -35,12 +36,16 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.Callable;

import javax.inject.Inject;
import javax.inject.Named;

import org.apache.commons.lang3.StringUtils;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.xwiki.configuration.TemporaryConfigurationExecutor;
import org.xwiki.export.pdf.PDFExportConfiguration;
import org.xwiki.export.pdf.PDFPrinter;
import org.xwiki.export.pdf.internal.RequiredSkinExtensionsRecorder;
Expand Down Expand Up @@ -93,6 +98,9 @@ class PDFExportJobTest
@MockComponent
private PrintPreviewURLBuilder printPreviewURLBuilder;

@MockComponent
private TemporaryConfigurationExecutor temporaryConfigurationExecutor;

private DocumentReference firstPageReference = new DocumentReference("test", "First", "Page");

private DocumentRenderingResult firstPageRendering = new DocumentRenderingResult(this.firstPageReference,
Expand Down Expand Up @@ -129,6 +137,13 @@ void configure() throws Exception
this.request.setDocuments(
Arrays.asList(this.firstPageReference, this.secondPageReference, thirdPageReference, fourthPageReference));

when(this.temporaryConfigurationExecutor.call(eq("executionContext"),
eq(Map.of("rendering.imageDimensionsIncludedInImageURL", false)), any())).thenAnswer(invocation -> {
@SuppressWarnings("unchecked")
Callable<Void> callable = (Callable<Void>) invocation.getArgument(2);
return callable.call();
});

when(this.authorization.hasAccess(Right.VIEW, this.aliceReference, this.firstPageReference)).thenReturn(true);
when(this.authorization.hasAccess(Right.VIEW, this.aliceReference, this.secondPageReference)).thenReturn(true);

Expand Down
Loading