Fix inconsistency between CHF and Servlet API cookie handling. #22

WrenSecurity · Dec 7, 2020 · 20fb6de · 20fb6de
1 parent d9b44d4
commit 20fb6de
Show file tree

Hide file tree

Showing 5 changed files with 81 additions and 2 deletions.
diff --git a/...ession-module/src/main/java/org/forgerock/jaspi/modules/session/jwt/JwtSessionModule.java b/...ession-module/src/main/java/org/forgerock/jaspi/modules/session/jwt/JwtSessionModule.java
@@ -131,7 +131,7 @@ Collection<CookieWrapper> createCookies(String value, int maxAge, String path) {
             cookies.add(new CookieWrapper(new Cookie()
                     .setName(sessionCookieName)
                     .setValue(value)
-                    .setMaxAge(maxAge)
+                    .setMaxAge(maxAge >= 0 ? maxAge : null)
                     .setPath(path)
                     .setDomain(cookieDomain)
                     .setSecure(isSecure)

diff --git a/...module/src/main/java/org/forgerock/jaspi/modules/session/jwt/ServletJwtSessionModule.java b/...module/src/main/java/org/forgerock/jaspi/modules/session/jwt/ServletJwtSessionModule.java
@@ -18,7 +18,6 @@
 
 import static org.forgerock.caf.http.Cookie.getCookies;
 import static org.forgerock.caf.http.Cookie.newCookie;
-import static org.forgerock.jaspi.modules.session.jwt.AbstractJwtSessionModule.LOGOUT_SESSION_REQUEST_ATTRIBUTE_NAME;
 
 import javax.security.auth.Subject;
 import javax.security.auth.callback.CallbackHandler;
@@ -92,6 +91,7 @@ public Jwt validateJwtSessionCookie(MessageInfo messageInfo) {
      * @param messageInfo The message info.
      * @return The cookie, or null.
      */
+    @Override
     public Cookie findJwtSessionCookie(MessageInfo messageInfo) {
         HttpServletRequest request = (HttpServletRequest) messageInfo.getRequestMessage();
         Set<Cookie> cookies = getCookies(request);

diff --git a/...on-module/src/test/java/org/forgerock/jaspi/modules/session/jwt/JwtSessionModuleTest.java b/...on-module/src/test/java/org/forgerock/jaspi/modules/session/jwt/JwtSessionModuleTest.java
@@ -0,0 +1,50 @@
+package org.forgerock.jaspi.modules.session.jwt;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertTrue;
+
+import java.util.Collection;
+import java.util.Collections;
+
+import org.forgerock.http.protocol.Cookie;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+public class JwtSessionModuleTest {
+
+    JwtSessionModule jwtSessionModule;
+
+    @BeforeMethod
+    public void setUp() {
+        jwtSessionModule = new JwtSessionModule();
+        jwtSessionModule.cookieDomains = Collections.singleton("example.com");
+    }
+
+    @Test
+    public void shouldCreateSessionCookieWithMaxAge() {
+        Collection<CookieWrapper> cookies = jwtSessionModule.createCookies("foo", 7, "/");
+        assertEquals(cookies.size(), 1);
+        Cookie cookie = cookies.iterator().next().getCookie();
+        assertEquals(cookie.getMaxAge(), Integer.valueOf(7));
+        assertNull(cookie.getExpires());
+    }
+
+    @Test
+    public void shouldCreateSessionCookieWithoutMaxAge() {
+        Collection<CookieWrapper> cookies = jwtSessionModule.createCookies("foo", -1, "/");
+        assertEquals(cookies.size(), 1);
+        Cookie cookie = cookies.iterator().next().getCookie();
+        assertNull(cookie.getMaxAge());
+        assertNull(cookie.getExpires());
+    }
+
+    @Test
+    public void shouldCreateSessionExpiredCookie() {
+        Collection<CookieWrapper> cookies = jwtSessionModule.createCookies("foo", 0, "/");
+        assertEquals(cookies.size(), 1);
+        Cookie cookie = cookies.iterator().next().getCookie();
+        assertTrue(cookie.getMaxAge() <= 0);
+    }
+
+}
diff --git a/...le/src/test/java/org/forgerock/jaspi/modules/session/jwt/ServletJwtSessionModuleTest.java b/...le/src/test/java/org/forgerock/jaspi/modules/session/jwt/ServletJwtSessionModuleTest.java
@@ -32,13 +32,15 @@
 import static org.testng.Assert.assertNotEquals;
 import static org.testng.Assert.assertNotNull;
 import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertTrue;
 
 import java.io.IOException;
 import java.io.UnsupportedEncodingException;
 import java.net.URLDecoder;
 import java.security.Key;
 import java.util.Arrays;
 import java.util.Calendar;
+import java.util.Collection;
 import java.util.Date;
 import java.util.HashMap;
 import java.util.Map;
@@ -1295,4 +1297,29 @@ public void shouldLogoutSession() throws Exception {
         assertThat(cookieCaptor.getValue().getMaxAge()).isEqualTo(0);
         assertThat(cookieCaptor.getValue().getPath()).isEqualTo("/");
     }
+
+    @Test
+    public void shouldCreateSessionCookieWithMaxAge() {
+        Collection<org.forgerock.caf.http.Cookie> cookies = jwtSessionModule.createCookies("foo", 7, "/");
+        assertEquals(cookies.size(), 1);
+        org.forgerock.caf.http.Cookie cookie = cookies.iterator().next();
+        assertEquals(cookie.getMaxAge(), 7);
+    }
+
+    @Test
+    public void shouldCreateSessionCookieWithoutMaxAge() {
+        Collection<org.forgerock.caf.http.Cookie> cookies = jwtSessionModule.createCookies("foo", -1, "/");
+        assertEquals(cookies.size(), 1);
+        org.forgerock.caf.http.Cookie cookie = cookies.iterator().next();
+        assertTrue(cookie.getMaxAge() < 0);
+    }
+
+    @Test
+    public void shouldCreateSessionExpiredCookie() {
+        Collection<org.forgerock.caf.http.Cookie> cookies = jwtSessionModule.createCookies("foo", 0, "/");
+        assertEquals(cookies.size(), 1);
+        org.forgerock.caf.http.Cookie cookie = cookies.iterator().next();
+        assertTrue(cookie.getMaxAge() == 0);
+    }
+
 }
diff --git a/pom.xml b/pom.xml
@@ -60,6 +60,8 @@
     <!-- This is enforced by wrensec-parent via the `enforce` profile, which is enabled unless -->
     <!-- -Dskip-enforce=true -->
     <maven.min.version>3.0.1</maven.min.version>
+    <!-- Override wrensec-parent version that breaks on some system due to Xstream library  -->
+    <mavenWarPluginVersion>3.3.1</mavenWarPluginVersion>
   </properties>
 
   <modules>