0
\$\begingroup\$

My concern with this code is that I am forcing execution repeatedly through the same pathway for the sake of a readability that I'm not convinced by.

It's a servlet filter which detects if the authenticated user is a service account (non-human) and then does some checks on them within the if clause.

import org.springframework.security.core.Authentication;    
import javax.servlet.Filter;
import javax.servlet.FilterChain;
import javax.servlet.FilterConfig;

public class ServiceAccountFilter implements Filter {

    @Override
    public void doFilter(ServletRequest request,
                         ServletResponse response,
                         FilterChain chain)
            throws IOException, ServletException {
        if (request instanceof HttpServletRequest
            && ((HttpServletRequest) request).getUserPrincipal() != null
            && ((HttpServletRequest) request).getUserPrincipal()
                    instanceof Authentication
            && ((Authentication)
                ((HttpServletRequest) request).getUserPrincipal())
                .getPrincipal() != null
            && ((Authentication)
                ((HttpServletRequest) request).getUserPrincipal())
                .getPrincipal() instanceof MyUser
            && ((MyUser) ((Authentication)
                ((HttpServletRequest) request).getUserPrincipal())
                .getPrincipal()).getUsername() != null
            && ((MyUser) ((Authentication)
                ((HttpServletRequest) request).getUserPrincipal())
                .getPrincipal()).getUsername().length() > 6) {
            MyUser myUser = (MyUser) ((Authentication)
                    ((HttpServletRequest) request).getUserPrincipal())
                    .getPrincipal();
            // app-specific tests
        }
        chain.doFilter(request, response);
    }

Would it be better to instantiate all of these objects and create a deeply nested set of simplified if clauses?

My opinion is that it might be marginally more performant but it wouldn't be any more readable - or am I already blind to how unreadable it is?

\$\endgroup\$
  • \$\begingroup\$ The site standard is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ – BCdotWEB Feb 4 at 12:22
  • \$\begingroup\$ The nastiness probably has to do with your class hierarchy. I suggest that you include your Authentication and MyUser classes for review. \$\endgroup\$ – 200_success Feb 4 at 17:13
1
\$\begingroup\$

One thing that is sure is that it will be more readable to move those tests in one method :

public void doFilter(ServletRequest request,
                     ServletResponse response,
                     FilterChain chain)
        throws IOException, ServletException {
    if ( isAuthenticated(request) ) {
        MyUser myUser = getAuthenticatedUser(request);
        // app-specific tests
    }
}

Then it depends but most of the time your code is more readable when extracting intermediate steps in variables or methods.

private boolean isAuthenticated(ServletRequest request) {
    return (request instanceof HttpServletRequest) &&
           hasValidPrincipal((HttpServletRequest) request);
} 

private boolean hasValidPrincipal(HttpServletRequest request) {
    MyUser user = getAuthenticatedUser(request);
    return user!=null && user.getUsername().length()>6;
}

private MyUser getAuthenticatedUser(HttpServletRequest request) {
    Object principal = request.getUserPrincipal();
    Authentication authentication = null;
    if ( !(principal instanceof Authentication) ) {
        return null;
    }
    authentication = (Authentication) request.getUserPrincipal();
    Object user = authentication.getPrincipal();

    return (user instanceof MyUser)
        ?(MyUser) user
        :null;
    }
}
\$\endgroup\$
0
\$\begingroup\$

I wanted to point out that:

(null instanceof C) == false

Hence a rewrite would be:

public void doFilter(ServletRequest request,
                     ServletResponse response,
                     FilterChain chain)
        throws IOException, ServletException {
    if (request instanceof HttpServletRequest) {
        HttpServletRequest req = (HttpServletRequest) request;
        if (req.getUserPrincipal() instanceof Authentication) {
            Authentication authentication = (Authentication) req.getUserPrincipal();
            if (authentication.getPrincipal() instanceof MyUser) {
                MyUser myUser = (MyUser) authentication.getPrincipal();
                String userName = myUser.getUsername();
                if (userName != null && userName.length() > 6) {
                    // app-specific tests
                }
            }
        }
    }
    chain.doFilter(request, response);
}

And this code I find digestible. If repeated at other locations maybe a lambda might be used to turn a ServletRequest to an optional operation on a MyUser.

public Optional<MyUser> retrieveMyUser(ServletRequest request)
        throws IOException, ServletException {
    if (request instanceof HttpServletRequest) {
        HttpServletRequest req = (HttpServletRequest) request;
        if (req.getUserPrincipal() instanceof Authentication) {
            Authentication authentication = (Authentication) req.getUserPrincipal();
            if (authentication.getPrincipal() instanceof MyUser) {
                MyUser myUser = (MyUser) authentication.getPrincipal();
                return Optional.of(myUser);
            }
        }
    }
    return Optional.empty();
}


public void doFilter(ServletRequest request,
                     ServletResponse response,
                     FilterChain chain)
        throws IOException, ServletException {
    retrieveMyUser(request).ifPresent(myUser -> {
        String userName = myUser.getUsername();
        if (userName != null && userName.length() > 6) {
            // app-specific tests
        }
    });
    chain.doFilter(request, response);
}
\$\endgroup\$

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.