TRUNK-6151: Session cookies should be cleared on logout

web/src/main/java/org/openmrs/web/filter/CookieClearingFilter.java

@@ -0,0 +1,100 @@
+/**
+ * This Source Code Form is subject to the terms of the Mozilla Public License,
+ * v. 2.0. If a copy of the MPL was not distributed with this file, You can
+ * obtain one at http:https://mozilla.org/MPL/2.0/. OpenMRS is also distributed under
+ * the terms of the Healthcare Disclaimer located at http:https://openmrs.org/license.
+ *
+ * Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS
+ * graphic logo is a trademark of OpenMRS Inc.
+ */
+package org.openmrs.web.filter;
+
+import javax.servlet.FilterChain;
+import javax.servlet.ServletException;
+import javax.servlet.http.Cookie;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import javax.servlet.http.HttpSession;
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Properties;
+
+import org.apache.commons.lang3.StringUtils;
+import org.openmrs.api.context.Context;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.web.filter.OncePerRequestFilter;
+
+/**
+ * This servlet filter exists to remove session cookies when a user logs out.
+ * <p/>
+ * This filter is configurable at runtime using the following runtime properties:
+ * <ul>
+ * <li><tt>cookieClearingFilter.toClear = comma separated list of cookies to clear</tt>
+ * determines the cookies we will try to clear. If unset, will default to just clearing the JSESSIONID cookie.</li>
+ * </ul>
+ */
+public class CookieClearingFilter extends OncePerRequestFilter {
+
+ private static final Logger log = LoggerFactory.getLogger(CookieClearingFilter.class);
+
+ @Override
+ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)
+ throws ServletException, IOException {
+
+ // if an earlier filter has already written a response, we cannot do anything
+ if (response.isCommitted()) {
+ filterChain.doFilter(request, response);
+ return;
+ }
+
+ String[] cookiesToClear = new String[0];
+
+ // the try-catch here is defensive; if, for whatever reason, we cannot parse this setting, this filter should not
+ // stop the request
+ try {
+ Properties properties = Context.getRuntimeProperties();
+ String cookiesToClearSetting = properties.getProperty("cookieClearingFilter.toClear", "JSESSIONID");
+
+ if (StringUtils.isNotBlank(cookiesToClearSetting)) {
+ cookiesToClear = Arrays.stream(cookiesToClearSetting.split("\\s*,\\s*")).map(String::trim).toArray(
+ String[]::new);
+ }
+ } catch (Exception e) {
+ log.warn("Caught exception while trying to determine cookies to clear", e);
+ }
+
+ boolean requestHasSession = false;
+ if (cookiesToClear.length > 0) {
+ // we need to track whether this request initially was part of a session
+ // if it was and there is no valid request at the end of the session, we clear the session cookies
+ requestHasSession = request.getRequestedSessionId() != null;
+ }
+
+ // handle the request
+ try {
+ filterChain.doFilter(request, response);
+ } finally {
+ if (cookiesToClear.length > 0 && !response.isCommitted()) {
+ HttpSession session = request.getSession(false);
+ // session was invalidated
+ if (session == null && requestHasSession) {
+ for (Cookie cookie : request.getCookies()) {
+ for (String cookieToClear : cookiesToClear) {
+ if (cookieToClear.equalsIgnoreCase(cookie.getName())) {
+ // NB This doesn't preserve the HttpOnly flag, but it seems irrelevant since Max-Age: 0 expires
+ // the cookie, i.e., a well-behaved user agent will throw it away and, in any case, we delete
+ // the value
+ Cookie clearedCookie = (Cookie) cookie.clone();
+ clearedCookie.setValue(null);
+ clearedCookie.setMaxAge(0);
+ response.addCookie(clearedCookie);
+ break;
+ }
+ }
+ }
+ }
+ }
+ }
+ }
+}

web/src/test/java/org/openmrs/web/filter/CookieClearingFilterTest.java

@@ -0,0 +1,208 @@
+package org.openmrs.web.filter;
+
+import javax.servlet.GenericServlet;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.Cookie;
+import javax.servlet.http.HttpServletRequest;
+import java.util.Properties;
+import java.util.UUID;
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.openmrs.api.context.Context;
+import org.openmrs.test.TestUtil;
+import org.openmrs.web.WebConstants;
+import org.springframework.mock.web.MockFilterChain;
+import org.springframework.mock.web.MockHttpServletRequest;
+import org.springframework.mock.web.MockHttpServletResponse;
+import org.springframework.mock.web.MockHttpSession;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+class CookieClearingFilterTest {
+
+ Properties runtimeProperties;
+
+ CookieClearingFilter filter;
+
+ MockHttpServletRequest request;
+
+ MockHttpServletResponse response;
+
+ MockHttpSession session;
+
+ MockFilterChain chain;
+
+ @BeforeEach
+ void setupRuntimeProperties() {
+ runtimeProperties = TestUtil.getRuntimeProperties(WebConstants.WEBAPP_NAME);
+ Context.setRuntimeProperties(runtimeProperties);
+ }
+
+ @BeforeEach
+ public void setup() {
+ filter = new CookieClearingFilter();
+ session = new MockHttpSession();
+ request = new MockHttpServletRequest();
+ response = new MockHttpServletResponse();
+ chain = new MockFilterChain();
+ }
+
+ @Test
+ void shouldClearCookiesIfSessionEnded() throws Exception {
+ // arrange
+ createChainThatInvalidatesSession();
+ clearJSessionIdOnLogout();
+ createSessionWithId("1234");
+
+
+ // act
+ filter.doFilter(request, response, chain);
+
+ // assert
+ Cookie[] cookies = response.getCookies();
+ assertEquals(1, cookies.length, "Expected only a single cookie");
+ assertTrue(cookies[0].getMaxAge() <= 0, "Expected Max-Age to be zero or a negative number");
+ }
+
+ @Test
+ void shouldNotClearCookiesIfSessionNotInvalidated() throws Exception {
+ // arrange
+ clearJSessionIdOnLogout();
+ createSessionWithId("1234");
+
+ // act
+ filter.doFilter(request, response, chain);
+
+ // assert
+ Cookie[] cookies = response.getCookies();
+ assertEquals(0, cookies.length, "Expected no cookies in response");
+ }
+
+ @Test
+ void shouldNotClearCookiesIfNewSessionCreatedButNotInvalidated() throws Exception {
+ // arrange
+ createChainThatInvalidatesSession();
+ clearJSessionIdOnLogout();
+ createSessionWithId("1234", true);
+
+ // act
+ filter.doFilter(request, response, chain);
+
+ // assert
+ Cookie[] cookies = response.getCookies();
+ assertEquals(1, cookies.length, "Expected no cookies in response");
+ }
+
+ @Test
+ void shouldNotClearCookiesIfNewSessionCreatedAndInvalidatedInOneRequest() throws Exception {
+ // arrange
+ clearJSessionIdOnLogout();
+ createSessionWithId("1234", true);
+
+
+ // act
+ filter.doFilter(request, response, chain);
+
+ // assert
+ Cookie[] cookies = response.getCookies();
+ assertEquals(1, cookies.length, "Expected the new session cookie in response");
+ assertTrue(cookies[0].getMaxAge() > 0, "Expected Max-Age to be set to some future value");
+ }
+
+ @Test
+ void shouldClearAllConfiguredCookies() throws Exception {
+ // arrange
+ createChainThatInvalidatesSession();
+ runtimeProperties.setProperty("cookieClearingFilter.toClear", "JSESSIONID,AnotherCookie");
+ createSessionWithId("1234");
+ // add our non-session cookie
+ {
+ Cookie myOtherCookie = new Cookie("AnotherCookie", UUID.randomUUID().toString());
+ Cookie[] requestCookies = request.getCookies();
+ Cookie[] cookies = new Cookie[requestCookies.length + 1];
+ System.arraycopy(requestCookies, 0, cookies, 0, requestCookies.length);
+ cookies[requestCookies.length] = myOtherCookie;
+ request.setCookies(cookies);
+ }
+
+ // act
+ filter.doFilter(request, response, chain);
+
+ // assert
+ Cookie[] cookies = response.getCookies();
+ assertEquals(2, cookies.length, "Expected two cookies");
+ for (Cookie cookie : cookies) {
+ assertTrue(cookie.getMaxAge() <= 0, "Expected Max-Age to be less than or equal to 0 for cookie " + cookie.getName());
+ }
+ }
+
+ @Test
+ void shouldClearAllConfiguredCookiesIgnoringWhitespace() throws Exception {
+ // arrange
+ createChainThatInvalidatesSession();
+ runtimeProperties.setProperty("cookieClearingFilter.toClear", " JSESSIONID \t, AnotherCookie ");
+ createSessionWithId("1234");
+ // add our non-session cookie
+ {
+ Cookie myOtherCookie = new Cookie("AnotherCookie", UUID.randomUUID().toString());
+ Cookie[] requestCookies = request.getCookies();
+ Cookie[] cookies = new Cookie[requestCookies.length + 1];
+ System.arraycopy(requestCookies, 0, cookies, 0, requestCookies.length);
+ cookies[requestCookies.length] = myOtherCookie;
+ request.setCookies(cookies);
+ }
+
+ // act
+ filter.doFilter(request, response, chain);
+
+ // assert
+ Cookie[] cookies = response.getCookies();
+ assertEquals(2, cookies.length, "Expected two cookies");
+ for (Cookie cookie : cookies) {
+ assertTrue(cookie.getMaxAge() <= 0, "Expected Max-Age to be less than or equal to 0 for cookie " + cookie.getName());
+ }
+ }
+
+ void clearJSessionIdOnLogout() {
+ runtimeProperties.setProperty("authentication.cookies.toClear", "JSESSIONID");
+ }
+
+ void createChainThatInvalidatesSession() {
+ chain = new MockFilterChain(new SessionInvalidationServlet());
+ }
+
+ void createSessionWithId(String id) {
+ createSessionWithId(id, false);
+ }
+
+ void createSessionWithId(String id, boolean isNew) {
+ session = new MockHttpSession(null, id);
+ session.setNew(isNew);
+ request.setSession(session);
+
+ if (!isNew) {
+ request.setRequestedSessionId(id);
+ Cookie sessionCookie = new Cookie("JSESSIONID", "1234");
+ sessionCookie.setMaxAge(60 * 60 * 2);
+ request.setCookies(sessionCookie);
+ } else {
+ Cookie sessionCookie = new Cookie("JSESSIONID", "1234");
+ sessionCookie.setMaxAge(60 * 60 * 2);
+ response.addCookie(sessionCookie);
+ }
+ }
+
+ private static final class SessionInvalidationServlet extends GenericServlet {
+
+ @Override
+ public void service(ServletRequest req, ServletResponse res) {
+ if (req instanceof HttpServletRequest) {
+ ((HttpServletRequest) req).getSession().invalidate();
+ }
+ }
+ }
+
+}

webapp/src/main/webapp/WEB-INF/web.xml

@@ -150,6 +150,17 @@
  </filter-mapping>
  <!-- End second filter -->
 
+ <!-- This filter should come before the OpenmrsFilter, i.e., before we've created
+ a session. -->
+ <filter>
+ <filter-name>CookieClearingFilter</filter-name>
+ <filter-class>org.openmrs.web.filter.CookieClearingFilter</filter-class>
+ </filter>
+ <filter-mapping>
+ <filter-name>CookieClearingFilter</filter-name>
+ <url-pattern>/*</url-pattern>
+ </filter-mapping>
+
  <filter>
  <filter-name>OpenmrsFilter</filter-name> 
  <filter-class>org.openmrs.web.filter.OpenmrsFilter</filter-class>