You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2020/03/23 10:02:46 UTC

[GitHub] [drill] ihuzenko opened a new pull request #2037: DRILL-7648: Scrypt j_security_check works without security headers

ihuzenko opened a new pull request #2037: DRILL-7648: Scrypt j_security_check works without security headers
URL: https://github.com/apache/drill/pull/2037
 
 
   # [DRILL-7648](https://issues.apache.org/jira/browse/DRILL-7648): Scrypt j_security_check works without security headers
   
   ## Description
   
   1. Added callback for setting headers in DrillHttpSecurityHandlerProvider,
      since ResponseHeadersSettingFilter doesn't cover this flow.
   
   ## Documentation
   
   No need to document the bugfix.
   
   ## Testing
   
   Tested manually since the security configuration for using form-based authentication is hard to do in unit tests.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [drill] vvysotskyi commented on a change in pull request #2037: DRILL-7648: Scrypt j_security_check works without security headers

Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2037: DRILL-7648: Scrypt j_security_check works without security headers
URL: https://github.com/apache/drill/pull/2037#discussion_r396494612
 
 

 ##########
 File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/DrillHttpSecurityHandlerProvider.java
 ##########
 @@ -54,11 +55,14 @@
   private final Map<String, DrillHttpConstraintSecurityHandler> securityHandlers =
       CaseInsensitiveMap.newHashMapWithExpectedSize(2);
 
+  private final BiConsumer<HttpServletRequest, HttpServletResponse> preHandleCallback;
+
   @SuppressWarnings("unchecked")
-  public DrillHttpSecurityHandlerProvider(DrillConfig config, DrillbitContext drillContext)
+  public DrillHttpSecurityHandlerProvider(DrillConfig config, DrillbitContext drillContext,
+                                          BiConsumer<HttpServletRequest, HttpServletResponse> preHandleCallback)
 
 Review comment:
   Passing `BiConsumer` looks slightly complicated. Is it possible either to pass map with response headers or obtain headers from `DrillConfig` again and use it where needed? 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [drill] arina-ielchiieva merged pull request #2037: DRILL-7648: Scrypt j_security_check works without security headers

Posted by GitBox <gi...@apache.org>.
arina-ielchiieva merged pull request #2037: DRILL-7648: Scrypt j_security_check works without security headers
URL: https://github.com/apache/drill/pull/2037
 
 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [drill] ihuzenko commented on issue #2037: DRILL-7648: Scrypt j_security_check works without security headers

Posted by GitBox <gi...@apache.org>.
ihuzenko commented on issue #2037: DRILL-7648: Scrypt j_security_check works without security headers
URL: https://github.com/apache/drill/pull/2037#issuecomment-602691335
 
 
   Thanks @vvysotskyi  for suggestions, looks much cleaner now. Please take a look. 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [drill] vvysotskyi commented on a change in pull request #2037: DRILL-7648: Scrypt j_security_check works without security headers

Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2037: DRILL-7648: Scrypt j_security_check works without security headers
URL: https://github.com/apache/drill/pull/2037#discussion_r396491824
 
 

 ##########
 File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java
 ##########
 @@ -219,9 +220,11 @@ private ServletContextHandler createServletContextHandler(final boolean authEnab
       servletContextHandler.addServlet(dynamicHolder, "/dynamic/*");
     }
 
+    final Map<String, String> responseHeaders = ResponseHeadersSettingFilter.retrieveResponseHeaders(config);
     if (authEnabled) {
       //DrillSecurityHandler is used to support SPNEGO and FORM authentication together
-      servletContextHandler.setSecurityHandler(new DrillHttpSecurityHandlerProvider(config, workManager.getContext()));
+      servletContextHandler.setSecurityHandler(new DrillHttpSecurityHandlerProvider(config, workManager.getContext(),
+          (req, resp) -> responseHeaders.forEach(resp::setHeader)));
 
 Review comment:
   Looks like the response is used here only. Is it possible to use and pass `Consumer` instead of `BiConsumer`?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services