brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ahgittin <...@git.apache.org>
Subject [GitHub] incubator-brooklyn pull request: BrooklynRestApiLauncher rework
Date Fri, 25 Jul 2014 20:22:00 GMT
Github user ahgittin commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/98#discussion_r15422429
  
    --- Diff: usage/rest-server/src/main/java/brooklyn/rest/security/BrooklynPropertiesSecurityFilter.java
---
    @@ -62,69 +63,66 @@
     
         @Override
         public void doFilter(ServletRequest request, ServletResponse response, FilterChain
chain) throws IOException, ServletException {
    -        if (provider==null) {
    +        if (provider == null) {
                 log.warn("No security provider available: disallowing web access to brooklyn");
                 ((HttpServletResponse) response).sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE);
                 return;
             }
    -        
    -        if (authenticate((HttpServletRequest)request)) {
    -            String user = Strings.toString( ((HttpServletRequest)request).getSession().getAttribute(AUTHENTICATED_USER_SESSION_ATTRIBUTE)
);
    -            if (handleLogout((HttpServletRequest)request)) {
    -                log.debug("REST logging out "+user+" of session "+((HttpServletRequest)request).getSession());
    +
    +        HttpServletRequest httpRequest = (HttpServletRequest) request;
    +        if (authenticate(httpRequest)) {
    +            String user = Strings.toString(httpRequest.getSession().getAttribute(AUTHENTICATED_USER_SESSION_ATTRIBUTE));
    +            if (handleLogout(httpRequest)) {
    +                log.debug("REST logging out " + user + " of session " + httpRequest.getSession());
                     // do nothing here, fall through to below
                 } else {
                     WebEntitlementContext entitlementContext = null;
    -                String uri = ((HttpServletRequest)request).getRequestURI();
    +                String uri = httpRequest.getRequestURI();
                     try {
    -                    String uid = Integer.toHexString(hashCode());
    -                    entitlementContext = new WebEntitlementContext(user, ((HttpServletRequest)request).getRemoteAddr(),
uri, uid);
    -                    if (originalRequest.get()==null) {
    +                    String uid = Identifiers.makeRandomId(6);
    +                    entitlementContext = new WebEntitlementContext(user, httpRequest.getRemoteAddr(),
uri, uid);
    +                    if (originalRequest.get() == null) {
                             // initial filter application
                             originalRequest.set(uri);
                         } else {
                             // this filter is being applied *again*, probably due to forwarding
(e.g. from '/' to '/index.html')
    -                        log.debug("REST request {} being forwarded from {} to {}", new
Object[] { uid, originalRequest.get(), uri });
    +                        log.debug("REST request {} being forwarded from {} to {}", new
Object[]{uid, originalRequest.get(), uri});
                             // clear the entitlement context before setting to avoid warnings
                             Entitlements.clearEntitlementContext();
                         }
                         Entitlements.setEntitlementContext(entitlementContext);
    -                    
    -                    log.debug("REST starting processing request {} with {}", uri, entitlementContext);
    -                    if (!((HttpServletRequest)request).getParameterMap().isEmpty()) {
    -                        log.debug("  REST req {} parameters: {}", uid, ((HttpServletRequest)request).getParameterMap());
    -                    }
    -                    if (((HttpServletRequest)request).getContentLength()>0) {
    -                        int len = ((HttpServletRequest)request).getContentLength();
    -                        log.debug("  REST req {} upload content type {}", uid, ((HttpServletRequest)request).getContentType()+"
length "+len
    -                            // would be nice to do this, but the stream can only be read
once;
    -                            // TODO figure out how we could peek at it
    -//                            +(len>0 && len<4096 ? ""+Streams.readFullyString(((HttpServletRequest)request).getInputStream())
: "") 
    -                            );
    -                    }
    -                    
    +
    +                    log.debug("REST req {} starting processing request {} with {}", new
Object[]{uid, uri, entitlementContext});
                         chain.doFilter(request, response);
    -                    
    -                    log.debug("REST completed, code {}, processing request {} with {}",

    -                        new Object[] { ((HttpServletResponse)response).getStatus(), uri,
entitlementContext } );
    +
    +                    // This logging must NOT happen before chain.doFilter, or FormMapProvider
will not work as expected.
    +                    // Getting the parameter map consumes the request body and only resource
methods using @FormParam
    +                    // will work as expected.
    +                    if (log.isDebugEnabled()) {
    +                        log.debug("REST req {} complete, responding {} for request {}
with {}",
    +                                new Object[]{uid, ((HttpServletResponse) response).getStatus(),
uri, entitlementContext});
    +                        if (!httpRequest.getParameterMap().isEmpty()) {
    +                            log.debug("     parameters were: {}", httpRequest.getParameterMap());
    +                        }
    +                        if (httpRequest.getContentLength() > 0) {
    +                            int len = httpRequest.getContentLength();
    +                            log.debug("     upload content type was {}, length={}", httpRequest.getContentType(),
len);
    +                        }
    +                    }
                         return;
    -                    
                     } catch (Throwable e) {
                         // NB errors are typically already caught at this point
                         if (log.isDebugEnabled()) {
    -                        log.debug("REST failed processing request "+uri+" with "+entitlementContext+":
"+e, e);
    +                        log.debug("REST failed processing request " + uri + " with "
+ entitlementContext + ": " + e, e);
                         }
    -                    
                         throw Exceptions.propagate(e);
    -                    
                     } finally {
                         originalRequest.remove();
                         Entitlements.clearEntitlementContext();
                     }
                 }
             }
    -        
    -        ((HttpServletResponse) response).setHeader("WWW-Authenticate","Basic realm=\"brooklyn\"");
    +        ((HttpServletResponse) response).setHeader("WWW-Authenticate", "Basic realm=\"brooklyn\"");
    --- End diff --
    
    not needed as part of this pull request but two related thoughts while i'm here:
    * the realm is likely something we'll want configurable as part of `brooklyn.properties`
    * it has been requested we support single-sign-on across multiple brooklyn nodes, that
is something we should look at additionally.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message