Servlet filter which detects if the authenticated user is a service account

Question

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?

Answer 1

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;
    }
}

Answer 2

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);
}