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.

Sign up for GitHub

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

SLING-9662 Use SlingUri as provided in request from auth.core #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+37 −27
Open

SLING-9662 Use SlingUri as provided in request from auth.core #10

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions pom.xml
View file
Original file line number Diff line number Diff line change
@@ -111,7 +111,7 @@
<dependency>
<groupId>org.apache.sling</groupId>
<artifactId>org.apache.sling.api</artifactId>
<version>2.6.0</version>
<version>2.23.1-SNAPSHOT</version>
<scope>provided</scope>
</dependency>
<dependency>
@@ -123,7 +123,7 @@
<dependency>
<groupId>org.apache.sling</groupId>
<artifactId>org.apache.sling.auth.core</artifactId>
<version>1.0.0</version>
<version>1.5.1-SNAPSHOT</version>
<scope>provided</scope>
</dependency>
<dependency>
38 changes: 20 additions & 18 deletions src/main/java/org/apache/sling/engine/impl/request/RequestData.java
View file
Original file line number Diff line number Diff line change
@@ -23,7 +23,6 @@
import java.io.BufferedReader;
import java.io.IOException;
import java.io.UnsupportedEncodingException;
import java.net.URLDecoder;
import java.util.ArrayList;

import javax.servlet.Servlet;
@@ -46,8 +45,10 @@
import org.apache.sling.api.resource.Resource;
import org.apache.sling.api.resource.ResourceResolver;
import org.apache.sling.api.servlets.ServletResolver;
import org.apache.sling.api.uri.SlingUri;
import org.apache.sling.api.wrappers.SlingHttpServletRequestWrapper;
import org.apache.sling.api.wrappers.SlingHttpServletResponseWrapper;
import org.apache.sling.auth.core.AuthenticationSupport;
import org.apache.sling.engine.impl.SlingHttpServletRequestImpl;
import org.apache.sling.engine.impl.SlingHttpServletResponseImpl;
import org.apache.sling.engine.impl.SlingMainServlet;
@@ -213,8 +214,7 @@ public RequestData(SlingRequestProcessorImpl slingRequestProcessor,
this.servletResponse = response;

this.slingRequest = getSlingHttpServletRequestFactory().createRequest(this, this.servletRequest);
this.slingResponse = new SlingHttpServletResponseImpl(this,
servletResponse);
this.slingResponse = new SlingHttpServletResponseImpl(this, servletResponse);

// Getting the RequestProgressTracker from the request attributes like
// this should not be generally used, it's just a way to pass it from
@@ -237,19 +237,17 @@ public Resource initResource(ResourceResolver resourceResolver) {
requestProgressTracker.startTimer("ResourceResolution");
final SlingHttpServletRequest request = getSlingRequest();

StringBuffer requestURL = servletRequest.getRequestURL();
String path = request.getPathInfo();
if (requestURL.indexOf(";") > -1 && !path.contains(";")) {
final String decodedURL;
try {
decodedURL = URLDecoder.decode(requestURL.toString(), "UTF-8");
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't check, but I assume this logic when into SlingUri?

Copy link
Author

Choose a reason for hiding this comment

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

Not in SlingUri, idea is to allow SlingUri to contain both encoded and non-encoded values (non-encoded is unproblematic as long as the URI components are in separate fields, it only gets problematic when you serialize it to a String, see SLING-9777 for details.

But the idea is that PathToUriMappingServiceImpl.getSlingUriFromRequest() takes care of it, it uses the methods request.getServletPath() and request.getPathInfo() that both return already decoded values according to javadoc HttpServletRequest

But now looking at SLING-4804 and @trekawek 's commit 83acbff it looks the path parameter is not contained in getServletPath(), so the most likely current version needs a fix, however this should be done in PathToUriMappingServiceImpl.getSlingUriFromRequest() then. I'll take care of it.

} catch (UnsupportedEncodingException e) {
throw new AssertionError("UTF-8 encoding is not supported");
}
path = path.concat(decodedURL.substring(decodedURL.indexOf(';')));
// Set by o.a.s.auth.core bundle
SlingUri slingUri = (SlingUri) request.getAttribute(AuthenticationSupport.REQUEST_ATTRIBUTE_URI);
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 we should allow that the attribute is missing

Copy link
Author

Choose a reason for hiding this comment

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

So in my tests it seemed like this case is impossible (since all requests pass through auth.core), hence implementing a fallback here would produce "dead code" - can you think of a scenario when auth.core is not setting REQUEST_ATTRIBUTE_URI?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like implicit contracts, it is not clear that when calling this method, the request attribute needs to be set. This might pose a problem over time. I guess the other situation is when an old auth.core bundle is used - I would assume that the engine does not have an import auth.core as its only using constants.
We have dead code nevertheless, either the dead code is throwing an exception or creating a new SlingUri :)

Copy link
Author

Choose a reason for hiding this comment

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

ok fair enough, I'll add a fallback handling :)

if (slingUri == null) {
throw new IllegalStateException(
"SlingUri not available as attribute of request (expected to be set in bundle o.a.s.auth.core)");
}
// ensure slingUri is bound to correct resource resolver
slingUri = slingUri.adjust(b -> b.setResourceResolver(resourceResolver));
request.setAttribute(AuthenticationSupport.REQUEST_ATTRIBUTE_URI, slingUri);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather remove the attribute, so its not leaking any further

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense.


Resource resource = resourceResolver.resolve(request, path);
Resource resource = resourceResolver.resolve(request, slingUri.getPath());
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 this is where we need a new method in the resource resolver, uri mapping has already been applied.
Or we can use getResource() - as slingUri has already done that part (using a service user) - only if null is returned, we can invoke resolve()

Copy link
Author

@ghenzler ghenzler Oct 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

yes, so the version works because rr.resolve() is not yet using PathToUriMappingService (and hence potential SlingUriMapper services are not called when using rr.resolve()). Moving forward PathToUriMappingService.resolve()/map() should delegate to PathToUriMappingService and for that, I agree we'll need an additional method in ResourceResolver - this should be able to stay privately in bundle as PathToUriMappingServiceImpl and ResourceResolverImpl are in same bundle (resourceresolver). I'll work on this and update the PR

if (request.getAttribute(REQUEST_RESOURCE_PATH_ATTR) == null) {
request.setAttribute(REQUEST_RESOURCE_PATH_ATTR, resource.getPath());
}
@@ -262,10 +260,14 @@ public Resource initResource(ResourceResolver resourceResolver) {
public void initServlet(final Resource resource,
final ServletResolver sr) {
// the resource and the request path info, will never be null
RequestPathInfo requestPathInfo = new SlingRequestPathInfo(resource);
ContentData contentData = setContent(resource, requestPathInfo);

requestProgressTracker.log("Resource Path Info: {0}", requestPathInfo);
SlingUri slingUri = (SlingUri) getSlingRequest().getAttribute(AuthenticationSupport.REQUEST_ATTRIBUTE_URI);
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 the uri should rather be parsed in as an argument to this method than relying on request attributes

Copy link
Author

@ghenzler ghenzler Oct 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The problem was a bit that initResource() already returns a resource and using slingUri as member var would put a bit duplication on the object (currentContentData is already a field and slingUri contains some of the same information) - but I agree using the request attribute again is not really nice either, I'll find a solution

ContentData contentData = setContent(resource, slingUri);

requestProgressTracker.log("Resource Path Info: resourcePath={0}, selectorString={1}, extension={2}, suffix={3}",
slingUri.getResourcePath(),
slingUri.getSelectorString(),
slingUri.getExtension(),
slingUri.getSuffix());

// finally resolve the servlet for the resource
requestProgressTracker.startTimer("ServletResolution");
22 changes: 15 additions & 7 deletions src/test/java/org/apache/sling/engine/impl/request/InitResourceTest.java
View file
Original file line number Diff line number Diff line change
@@ -24,6 +24,9 @@

import org.apache.sling.api.request.RequestProgressTracker;
import org.apache.sling.api.resource.ResourceResolver;
import org.apache.sling.api.uri.SlingUri;
import org.apache.sling.api.uri.SlingUriBuilder;
import org.apache.sling.auth.core.AuthenticationSupport;
import org.jmock.Expectations;
import org.jmock.Mockery;
import org.junit.Before;
@@ -48,13 +51,13 @@ public class InitResourceTest {
@Parameters(name="URL={0} path={1}")
public static Collection<Object[]> data() {
return Arrays.asList(new Object[][] {
{ "/one;v=1.1", "one;v=1.1", "/one;v=1.1" },
{ "/two;v=1.1", "two", "/two;v=1.1" },
{ "/one;v=1.1", "one;v=1.1", "/one;v='1.1'" },
{ "/two;v=1.1", "two", "/two;v='1.1'" },
{ "/three", "three", "/three" },
{ "/four%3Bv=1.1", "four", "/four" },
{ "/five%3Bv=1.1", "five;v=1.1", "/five;v=1.1" },
{ "/six;v=1.1", "six;v=1.1", "/six;v=1.1" },
{ "/seven", "seven;v=1.1", "/seven;v=1.1" },
{ "/five%3Bv=1.1", "five;v=1.1", "/five;v='1.1'" },
{ "/six;v=1.1", "six;v=1.1", "/six;v='1.1'" },
{ "/seven", "seven;v=1.1", "/seven;v='1.1'" },
});
}

@@ -73,8 +76,13 @@ public void setup() throws Exception {
resourceResolver = context.mock(ResourceResolver.class);

context.checking(new Expectations() {{
allowing(req).getRequestURL();
will(returnValue(new StringBuffer(requestURL)));

allowing(req).getAttribute(AuthenticationSupport.REQUEST_ATTRIBUTE_URI);

SlingUri slingUri = SlingUriBuilder.create().setPath(expectedResolvePath).build();
will(returnValue(slingUri));

allowing(req).setAttribute(AuthenticationSupport.REQUEST_ATTRIBUTE_URI, slingUri);

allowing(req).getRequestURI();