[GitHub] [sling-org-apache-sling-resourceresolver] rombert commented on a diff in pull request #78: Various improvements for the webconsole plugin
rombert commented on code in PR #78: URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/78#discussion_r941399874 ## src/main/java/org/apache/sling/resourceresolver/impl/console/ResourceResolverWebConsolePlugin.java: ## @@ -255,19 +278,42 @@ protected void doPost(HttpServletRequest request, // finally redirect final String path = request.getContextPath() + request.getServletPath() + request.getPathInfo(); -final String redirectTo; +String redirectTo; if (msg == null) { redirectTo = path; } else { redirectTo = path + '?' + PAR_MSG + '=' + encodeParam(msg) + '&' + PAR_TEST + '=' + encodeParam(test); +if ( user != null && user.length() > 0 ) { +redirectTo += '&' + PAR_USER + '=' + encodeParam(user); +} } response.sendRedirect(redirectTo); } +private ResourceResolver getImpersonatedResourceResolver(HttpServletRequest request, final String user) +throws LoginException { + +// resolver is set by the auth.core bundle in case of successful authentication, so it should +// always be there +Object resolverAttribute = request.getAttribute(AuthenticationSupport.REQUEST_ATTRIBUTE_RESOLVER); +if ( !(resolverAttribute instanceof ResourceResolver) ) { +throw new IllegalArgumentException("No " + ResourceResolver.class.getSimpleName() + " found in request, unable to proceed with impersonation"); Review Comment: @kwin suggested that we use an admin resolver instead (and include the bundle in the allow list ). If we would stop looking up the ResourceResolver in the request attribute, would it solve this issue? https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/78#discussion_r941353233 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [sling-org-apache-sling-resourceresolver] rombert commented on a diff in pull request #78: Various improvements for the webconsole plugin
rombert commented on code in PR #78: URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/78#discussion_r941330770 ## src/main/java/org/apache/sling/resourceresolver/impl/console/ResourceResolverWebConsolePlugin.java: ## @@ -255,19 +278,42 @@ protected void doPost(HttpServletRequest request, // finally redirect final String path = request.getContextPath() + request.getServletPath() + request.getPathInfo(); -final String redirectTo; +String redirectTo; if (msg == null) { redirectTo = path; } else { redirectTo = path + '?' + PAR_MSG + '=' + encodeParam(msg) + '&' + PAR_TEST + '=' + encodeParam(test); +if ( user != null && user.length() > 0 ) { +redirectTo += '&' + PAR_USER + '=' + encodeParam(user); +} } response.sendRedirect(redirectTo); } +private ResourceResolver getImpersonatedResourceResolver(HttpServletRequest request, final String user) +throws LoginException { + +// resolver is set by the auth.core bundle in case of successful authentication, so it should +// always be there +Object resolverAttribute = request.getAttribute(AuthenticationSupport.REQUEST_ATTRIBUTE_RESOLVER); Review Comment: Looking at the Oak implementation, I see that impersonation works if either: - the impersonator is an admin - the impersonator is included in the `rep:impersonators` property of the impersonated user https://github.com/apache/jackrabbit-oak/blob/a90566744551246535f65c2aefc5a44fd5275490/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/ImpersonationImpl.java#L125-L146 I am not sure if either of these is possible or desireable for a service user. Do you see another way? ## src/main/java/org/apache/sling/resourceresolver/impl/console/ResourceResolverWebConsolePlugin.java: ## @@ -255,19 +278,42 @@ protected void doPost(HttpServletRequest request, // finally redirect final String path = request.getContextPath() + request.getServletPath() + request.getPathInfo(); -final String redirectTo; +String redirectTo; if (msg == null) { redirectTo = path; } else { redirectTo = path + '?' + PAR_MSG + '=' + encodeParam(msg) + '&' + PAR_TEST + '=' + encodeParam(test); +if ( user != null && user.length() > 0 ) { +redirectTo += '&' + PAR_USER + '=' + encodeParam(user); +} } response.sendRedirect(redirectTo); } +private ResourceResolver getImpersonatedResourceResolver(HttpServletRequest request, final String user) +throws LoginException { + +// resolver is set by the auth.core bundle in case of successful authentication, so it should +// always be there +Object resolverAttribute = request.getAttribute(AuthenticationSupport.REQUEST_ATTRIBUTE_RESOLVER); Review Comment: Looking at the Oak implementation, I see that impersonation works if either: - the impersonator is an admin - the impersonator is included in the `rep:impersonators` property of the impersonated user https://github.com/apache/jackrabbit-oak/blob/a90566744551246535f65c2aefc5a44fd5275490/oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/ImpersonationImpl.java#L125-L146 I am not sure if either of these is possible or desirable for a service user. Do you see another way? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [sling-org-apache-sling-resourceresolver] rombert commented on a diff in pull request #78: Various improvements for the webconsole plugin
rombert commented on code in PR #78: URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/78#discussion_r941063449 ## src/main/java/org/apache/sling/resourceresolver/impl/console/ResourceResolverWebConsolePlugin.java: ## @@ -255,19 +278,42 @@ protected void doPost(HttpServletRequest request, // finally redirect final String path = request.getContextPath() + request.getServletPath() + request.getPathInfo(); -final String redirectTo; +String redirectTo; if (msg == null) { redirectTo = path; } else { redirectTo = path + '?' + PAR_MSG + '=' + encodeParam(msg) + '&' + PAR_TEST + '=' + encodeParam(test); +if ( user != null && user.length() > 0 ) { +redirectTo += '&' + PAR_USER + '=' + encodeParam(user); +} } response.sendRedirect(redirectTo); } +private ResourceResolver getImpersonatedResourceResolver(HttpServletRequest request, final String user) +throws LoginException { + +// resolver is set by the auth.core bundle in case of successful authentication, so it should +// always be there +Object resolverAttribute = request.getAttribute(AuthenticationSupport.REQUEST_ATTRIBUTE_RESOLVER); Review Comment: If I try to set up impersonation based on the existing resolver ( https://github.com/apache/sling-org-apache-sling-resourceresolver/blob/11f26ad706a350269e27ca42a2cbcf22a4724ce1/src/main/java/org/apache/sling/resourceresolver/impl/console/ResourceResolverWebConsolePlugin.java#L246 ) I get back > Test Failure: org.apache.sling.api.resource.LoginException: Impersonation not allowed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [sling-org-apache-sling-resourceresolver] rombert commented on a diff in pull request #78: Various improvements for the webconsole plugin
rombert commented on code in PR #78: URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/78#discussion_r940259595 ## src/main/java/org/apache/sling/resourceresolver/impl/console/ResourceResolverWebConsolePlugin.java: ## @@ -255,19 +278,42 @@ protected void doPost(HttpServletRequest request, // finally redirect final String path = request.getContextPath() + request.getServletPath() + request.getPathInfo(); -final String redirectTo; +String redirectTo; if (msg == null) { redirectTo = path; } else { redirectTo = path + '?' + PAR_MSG + '=' + encodeParam(msg) + '&' + PAR_TEST + '=' + encodeParam(test); +if ( user != null && user.length() > 0 ) { +redirectTo += '&' + PAR_USER + '=' + encodeParam(user); +} } response.sendRedirect(redirectTo); } +private ResourceResolver getImpersonatedResourceResolver(HttpServletRequest request, final String user) +throws LoginException { + +// resolver is set by the auth.core bundle in case of successful authentication, so it should +// always be there +Object resolverAttribute = request.getAttribute(AuthenticationSupport.REQUEST_ATTRIBUTE_RESOLVER); Review Comment: Do you mean that we should try to create an additional mapping for the console using a service user that has impersonation rights? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [sling-org-apache-sling-resourceresolver] rombert commented on a diff in pull request #78: Various improvements for the webconsole plugin
rombert commented on code in PR #78: URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/78#discussion_r940255957 ## src/main/java/org/apache/sling/resourceresolver/impl/console/ResourceResolverWebConsolePlugin.java: ## @@ -255,19 +278,42 @@ protected void doPost(HttpServletRequest request, // finally redirect final String path = request.getContextPath() + request.getServletPath() + request.getPathInfo(); -final String redirectTo; +String redirectTo; if (msg == null) { redirectTo = path; } else { redirectTo = path + '?' + PAR_MSG + '=' + encodeParam(msg) + '&' + PAR_TEST + '=' + encodeParam(test); +if ( user != null && user.length() > 0 ) { +redirectTo += '&' + PAR_USER + '=' + encodeParam(user); +} } response.sendRedirect(redirectTo); } +private ResourceResolver getImpersonatedResourceResolver(HttpServletRequest request, final String user) +throws LoginException { + +// resolver is set by the auth.core bundle in case of successful authentication, so it should +// always be there +Object resolverAttribute = request.getAttribute(AuthenticationSupport.REQUEST_ATTRIBUTE_RESOLVER); +if ( !(resolverAttribute instanceof ResourceResolver) ) { +throw new IllegalArgumentException("No " + ResourceResolver.class.getSimpleName() + " found in request, unable to proceed with impersonation"); +} + +@SuppressWarnings("resource") // not a leak, we don't own this resolver +ResourceResolver currentResolver = (ResourceResolver) resolverAttribute; + +Map authenticationInfo = new HashMap<>(); +authenticationInfo.put(ResourceResolverFactory.USER_IMPERSONATION, user); + authenticationInfo.put(JcrResourceConstants.AUTHENTICATION_INFO_SESSION, currentResolver.adaptTo(Session.class)); Review Comment: There are two issues to discuss here 1. The javax.jcr import is already optional - https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/78/files#diff-6d1033f3cc4642b36cd395cf203fdc2d963f51e46cb1949508765a9756743c45R2 . I _think_ that a `NoClassDefFoundError` will be thrown from the method, then caught at https://github.com/apache/sling-org-apache-sling-resourceresolver/blob/11f26ad706a350269e27ca42a2cbcf22a4724ce1/src/main/java/org/apache/sling/resourceresolver/impl/console/ResourceResolverWebConsolePlugin.java#L265 . This should not affect the path where impersonation is not used. Do we need to handle this in some other way, e.g. using the bundle class loader to dynamically load the class definition? 2. The `adaptTo` call can return null nonetheless and I will take it into acount ## src/main/java/org/apache/sling/resourceresolver/impl/console/ResourceResolverWebConsolePlugin.java: ## @@ -160,14 +169,22 @@ protected void doGet(final HttpServletRequest request, + "clearly marked, and the others listed for completeness."); pw.println(""); -pw.println("Test"); pw.print(""); -pw.print(""); +pw.println(""); +pw.print("Test "); pw.print(""); +pw.println("' class='input' size='20'>"); +pw.print("User (optional)"); +