SLING-9662 Use SlingUri as provided in request from auth.core #10
Conversation
| if (requestURL.indexOf(";") > -1 && !path.contains(";")) { | ||
| final String decodedURL; | ||
| try { | ||
| decodedURL = URLDecoder.decode(requestURL.toString(), "UTF-8"); |
There was a problem hiding this comment.
I didn't check, but I assume this logic when into SlingUri?
There was a problem hiding this comment.
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.
| } | ||
| path = path.concat(decodedURL.substring(decodedURL.indexOf(';'))); | ||
| // Set by o.a.s.auth.core bundle | ||
| SlingUri slingUri = (SlingUri) request.getAttribute(AuthenticationSupport.REQUEST_ATTRIBUTE_URI); |
There was a problem hiding this comment.
I think we should allow that the attribute is missing
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
ok fair enough, I'll add a fallback handling :)
| } | ||
| // ensure slingUri is bound to correct resource resolver | ||
| slingUri = slingUri.adjust(b -> b.setResourceResolver(resourceResolver)); | ||
| request.setAttribute(AuthenticationSupport.REQUEST_ATTRIBUTE_URI, slingUri); |
There was a problem hiding this comment.
I would rather remove the attribute, so its not leaking any further
|
|
||
| Resource resource = resourceResolver.resolve(request, path); | ||
| Resource resource = resourceResolver.resolve(request, slingUri.getPath()); |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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
| ContentData contentData = setContent(resource, requestPathInfo); | ||
|
|
||
| requestProgressTracker.log("Resource Path Info: {0}", requestPathInfo); | ||
| SlingUri slingUri = (SlingUri) getSlingRequest().getAttribute(AuthenticationSupport.REQUEST_ATTRIBUTE_URI); |
There was a problem hiding this comment.
I think the uri should rather be parsed in as an argument to this method than relying on request attributes
There was a problem hiding this comment.
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
API PR (can be merged separately):
apache/sling-org-apache-sling-api#25
Impl PRs that belong together:
apache/sling-org-apache-sling-resourceresolver#22
#10 (in this module)
apache/sling-org-apache-sling-auth-core#6