[GitHub] incubator-hawq pull request #1379: HAWQ-1622. Cache PXF proxy UGI so that cl...

[benchristel]

Github user benchristel commented on a diff in the pull request:

    https://github.com/apache/incubator-hawq/pull/1379#discussion_r200797423
  
    --- Diff: 
pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java
 ---
    @@ -64,21 +75,28 @@ public void init(FilterConfig filterConfig) throws 
ServletException {
          * @param chain filter chain
          */
         @Override
    -    public void doFilter(final ServletRequest request, final 
ServletResponse response, final FilterChain chain) throws IOException, 
ServletException {
    +    public void doFilter(final ServletRequest request, final 
ServletResponse response, final FilterChain chain)
    +            throws IOException, ServletException {
     
             if (SecureLogin.isUserImpersonationEnabled()) {
     
                 // retrieve user header and make sure header is present and is 
not empty
    -            final String user = ((HttpServletRequest) 
request).getHeader(USER_HEADER);
    -            if (user == null) {
    -                throw new IllegalArgumentException(MISSING_HEADER_ERROR);
    -            } else if (user.trim().isEmpty()) {
    -                throw new IllegalArgumentException(EMPTY_HEADER_ERROR);
    +            final String user = getHeaderValue(request, USER_HEADER);
    +            String transactionId = getHeaderValue(request, 
TRANSACTION_ID_HEADER);
    --- End diff --
    
    We are not passing `required=true` to `getHeaderValue()` here—is it 
really valid for `transactionId` to be null?
    
    If `transactionId` can be null, that means we're going to create 
`SegmentTransactionId`s with nothing but a segment ID in them, and we'll 
potentially get the wrong UGI from the cache if another request with no 
`transactionId` was recently served.


---