Validations in filter class

Question

For my web application I have written AuthenticationFilter which validates each request to check against CSRF attacks, Browser Checks, Session Validity etc. Instead of putting multiple if else blocks I thought it can be implemented as following.

Please let me know if I can improve this.

public interface IValidator {
 public void validate(Context context) throws ValidatorException;
 public void handleFailure(Context context) throws Exception;
}
public class CSRFValidator implements IValidator, CWSConstants {
 private final Logger LOGGER = LoggerFactory.getLogger(CSRFValidator.class);
 @Override
 public void validate(Context context) throws ValidatorException {
 //some logic to validate request if it contains CSRF token
 }
 @Override
 public void handleFailure(Context context) throws Exception {
 context.getHttpServletResponse().sendError(HttpServletResponse.SC_NON_AUTHORITATIVE_INFORMATION);
 }
}
public class BrowserCheckValidator implements IValidator, CWSConstants {
 private final Logger LOGGER = LoggerFactory.getLogger(CSRFValidator.class);
 @Override
 public void validate(Context context) throws ValidatorException {
 //if request is not coming from suported browser then throw exception
 }
 @Override
 public void handleFailure(Context context) throws Exception {
 context.getHttpServletResponse().sendError(HttpServletResponse.SC_NON_AUTHORITATIVE_INFORMATION);
 }
}
public class AuthenticationFilter implements Filter{
 private List<IValidator> requestValidators = new ArrayList<>();
 public void doFilter(ServletRequest request, ServletResponse response, FilterChain fc)
 throws IOException, ServletException {
 Context context = new Context(httpRequest, httpResponse, session);
 for (IValidator validator : requestValidators) {
 try {
 LOGGER.debug("Validating " + validator.getClass().getName());
 validator.validate(context);
 } catch (ValidatorException e) {
 LOGGER.error("Error while validating " + validator.getClass().getName(), e);
 validator.handleFailure(context);
 return;
 }
 }
 fc.doFilter(request, response);
 }
 @Override
 public void init(FilterConfig filterConfig) throws ServletException {
 LOGGER.debug("Initializing validators");
 requestValidators.add(new CSRFValidator());
 }
}
public class ValidatorException extends Exception {
 private static final long serialVersionUID = 8990146478624920065L;
 public ValidatorException() {
 super();
 }
 public ValidatorException(String message, Throwable cause, boolean enableSuppression, boolean writableStackTrace) {
 super(message, cause, enableSuppression, writableStackTrace);
 }
 public ValidatorException(String message, Throwable cause) {
 super(message, cause);
 }
 public ValidatorException(String message) {
 super(message);
 }
 public ValidatorException(Throwable cause) {
 super(cause);
 }
}
public final class Context {
 private HttpServletRequest httpServletRequest;
 private HttpServletResponse httpServletResponse;
 private HttpSession httpSession;
 public Context(HttpServletRequest httpServletRequest, HttpServletResponse httpServletResponse,
 HttpSession httpSession) {
 this.httpServletRequest = httpServletRequest;
 this.httpServletResponse = httpServletResponse;
 this.httpSession = httpSession;
 }
 public HttpServletRequest getHttpServletRequest() {
 return httpServletRequest;
 }
 public HttpServletResponse getHttpServletResponse() {
 return httpServletResponse;
 }
 public HttpSession getHttpSession() {
 return httpSession;
 }
}

• Tolani: I wouldn't say "Please let me know if I can go ahead with this" as code review is about improving a written code not asking if you code is syntactically correct. Use words like "Can this be Improved"
• Jude Niroshan: Except that C++ style IValidator interface name, it looks fine for me :)

Answer 1

Instead of putting multiple if else blocks I thought it can be implemented as following.

Looks like the Strategy Pattern with polymorphic dispatch. A sound plan!

Make fields final when possible

The private fields of Context can all be final. It's good to make them final so you won't change them by accident. final fields are easier to read, as you know they will only ever have the same value.

Avoid throwing Exception

IValidator.handleFailure is defined with throws Exception. It's recommended to avoid using the generic Exception class, it's better to use specific exceptions. Code using this interface is forced to catch Exception, which creates the risk that other exception types may be caught by mistake.

Implement just what you need

Do you need all the constructors in ValidatorException? In the posted code, there isn't a single use case using of any of those. I suggest to implement only the functionality you actually need, and avoid to implement in advance something "you might need someday".

Conventions

In Java it's not a convention to prefix interface names with a capital I. I suggest to rename IValidator to Validator.

The constant interface antipattern

You haven't shared the code of CWSConstants, but it sounds like it might simply contain constants, and your classes implement this interface only to have easy access to those constants.

This is considered a bad practice. Interfaces are intended for contracts of behaviors, using them for easy access of constants is an abuse of the language. The alternative is to put the constants in a class instead of an interface. You may use static imports to avoid polluting the code with the name of the class everywhere.