Flash scope improvements

* flash scope get lost between (sub)paths fix #468 * allow to customize flash cookie fix #470
jooby-project · Sep 7, 2016 · f59b177 · f59b177
1 parent b5addee
commit f59b177
Show file tree

Hide file tree

Showing 11 changed files with 186 additions and 22 deletions.
diff --git a/coverage-report/src/test/java/apps/flashscope/FlashScopeApp.java b/coverage-report/src/test/java/apps/flashscope/FlashScopeApp.java
@@ -22,8 +22,8 @@ public class FlashScopeApp extends Jooby {
 
     get("/", () -> Results.html("flash"));
 
-    post("/", req -> {
-      req.flash("success", req.param("message").value());
+    get("/send", req -> {
+      req.flash("success", req.param("message").value("It works"));
       return Results.redirect("/");
     });
 

diff --git a/coverage-report/src/test/java/org/jooby/issues/Issue397.java b/coverage-report/src/test/java/org/jooby/issues/Issue397.java
@@ -49,13 +49,13 @@ public void shouldCreateAndDestroyFlashCookie() throws Exception {
         .post("/397/flash")
         .expect(302)
         .header("Set-Cookie", setCookie -> {
-          assertEquals("flash=success=Thank+you%21;Version=1", setCookie);
+          assertEquals("jooby.flash=success=Thank+you%21;Version=1;Path=/;HttpOnly", setCookie);
           request()
               .get("/397/flash")
               .header("Cookie", setCookie)
               .expect("{success=Thank you!}")
               .header("Set-Cookie", clearCookie -> {
-                assertTrue(clearCookie.startsWith("flash=;Version=1;Max-Age=0;"));
+                assertTrue(clearCookie.startsWith("jooby.flash=;Version=1;Path=/;HttpOnly;Max-Age=0;"));
               });
         });
   }
@@ -76,19 +76,19 @@ public void shouldRecreateCookieOnReset() throws Exception {
         .post("/397/flash")
         .expect(302)
         .header("Set-Cookie", setCookie1 -> {
-          assertEquals("flash=success=Thank+you%21;Version=1", setCookie1);
+          assertEquals("jooby.flash=success=Thank+you%21;Version=1;Path=/;HttpOnly", setCookie1);
           request()
               .post("/397/reset")
               .header("Cookie", setCookie1)
               .expect(302)
               .header("Set-Cookie", setCookie2 -> {
-                assertEquals("flash=success=Thank+you%21&foo=bar;Version=1", setCookie2);
+                assertEquals("jooby.flash=success=Thank+you%21&foo=bar;Version=1;Path=/;HttpOnly", setCookie2);
                 request()
                     .get("/397/flash")
                     .header("Cookie", setCookie2)
                     .expect("{success=Thank you!, foo=bar}")
                     .header("Set-Cookie", clearCookie -> {
-                      assertTrue(clearCookie.startsWith("flash=;Version=1;Max-Age=0;"));
+                      assertTrue(clearCookie.startsWith("jooby.flash=;Version=1;Path=/;HttpOnly;Max-Age=0;"));
                     });
               });
         });
@@ -98,10 +98,10 @@ public void shouldRecreateCookieOnReset() throws Exception {
   public void shouldClearFlashCookieWhenEmpty() throws Exception {
     request()
         .get("/397/discard")
-        .header("Cookie", "flash=success=OK;Version=1")
+        .header("Cookie", "jooby.flash=success=OK;Version=1")
         .expect(200)
         .header("Set-Cookie", setCookie -> {
-          assertTrue(setCookie.startsWith("flash=;Version=1;Max-Age=0;"));
+          assertTrue(setCookie.startsWith("jooby.flash=;Version=1;Path=/;HttpOnly;Max-Age=0;"));
         });
   }
 }
diff --git a/coverage-report/src/test/java/org/jooby/issues/Issue397b.java b/coverage-report/src/test/java/org/jooby/issues/Issue397b.java
@@ -55,12 +55,12 @@ public void flashScopeOnMvc() throws Exception {
         .get("/397/flash")
         .expect("{foo=bar}")
         .header("Set-Cookie", setCookie -> {
-          assertEquals("flash=foo=bar;Version=1", setCookie);
+          assertEquals("jooby.flash=foo=bar;Version=1;Path=/;HttpOnly", setCookie);
           request()
               .get("/397/flash/attr")
               .expect("bar")
               .header("Set-Cookie", clearCookie -> {
-                assertTrue(clearCookie.startsWith("flash=;Version=1;Max-Age=0;"));
+                assertTrue(clearCookie.startsWith("jooby.flash=;Version=1;Path=/;HttpOnly;Max-Age=0;"));
               });
         });
   }

diff --git a/coverage-report/src/test/java/org/jooby/issues/Issue468.java b/coverage-report/src/test/java/org/jooby/issues/Issue468.java
@@ -0,0 +1,43 @@
+package org.jooby.issues;
+
+import org.jooby.FlashScope;
+import org.jooby.Results;
+import org.jooby.test.ServerFeature;
+import org.junit.Test;
+
+import com.typesafe.config.ConfigFactory;
+import com.typesafe.config.ConfigValueFactory;
+
+public class Issue468 extends ServerFeature {
+
+  {
+    use(ConfigFactory.empty()
+        .withValue("application.path", ConfigValueFactory.fromAnyRef("/468")));
+
+    use(new FlashScope());
+
+    get("/", req -> req.flash().get("foo"));
+
+    get("/redirect", req -> {
+      req.flash("foo", "bar");
+      return Results.redirect(req.contextPath() + "/");
+    });
+  }
+
+  @Test
+  public void flashAttributeIsPresentBetweenDiffPaths() throws Exception {
+    request()
+        .dontFollowRedirect()
+        .get("/468/redirect")
+        .execute()
+        .header("Set-Cookie", "jooby.flash=foo=bar;Version=1;Path=/468;HttpOnly");
+  }
+
+  @Test
+  public void flashAttributeIsPresentBetweenDiffPathsOnRedirect() throws Exception {
+    request()
+        .get("/468/redirect")
+        .expect("bar");
+  }
+
+}
diff --git a/coverage-report/src/test/java/org/jooby/issues/Issue468a.java b/coverage-report/src/test/java/org/jooby/issues/Issue468a.java
@@ -0,0 +1,29 @@
+package org.jooby.issues;
+
+import org.jooby.Cookie;
+import org.jooby.FlashScope;
+import org.jooby.Results;
+import org.jooby.test.ServerFeature;
+import org.junit.Test;
+
+public class Issue468a extends ServerFeature {
+
+  {
+    use(new FlashScope(new Cookie.Definition("x").httpOnly(false)));
+
+    get("/468", req -> {
+      req.flash("foo", "bar");
+      return Results.redirect(req.contextPath() + "/");
+    });
+  }
+
+  @Test
+  public void flashAttributeIsPresentBetweenDiffPaths() throws Exception {
+    request()
+        .dontFollowRedirect()
+        .get("/468")
+        .execute()
+        .header("Set-Cookie", "x=foo=bar;Version=1;Path=/");
+  }
+
+}
diff --git a/coverage-report/src/test/java/org/jooby/issues/Issue468b.java b/coverage-report/src/test/java/org/jooby/issues/Issue468b.java
@@ -0,0 +1,37 @@
+package org.jooby.issues;
+
+import org.jooby.FlashScope;
+import org.jooby.Results;
+import org.jooby.test.ServerFeature;
+import org.junit.Test;
+
+public class Issue468b extends ServerFeature {
+
+  {
+    use(new FlashScope());
+
+    get("/468", req -> req.flash().get("foo"));
+
+    get("/468/redirect", req -> {
+      req.flash("foo", "bar");
+      return Results.redirect("/468");
+    });
+  }
+
+  @Test
+  public void flashAttributeIsPresentBetweenDiffPaths() throws Exception {
+    request()
+        .dontFollowRedirect()
+        .get("/468/redirect")
+        .execute()
+        .header("Set-Cookie", "jooby.flash=foo=bar;Version=1;Path=/;HttpOnly");
+  }
+
+  @Test
+  public void flashAttributeIsPresentBetweenDiffPathsOnRedirect() throws Exception {
+    request()
+        .get("/468/redirect")
+        .expect("bar");
+  }
+
+}
diff --git a/coverage-report/src/test/resources/apps/flashscope/flash.html b/coverage-report/src/test/resources/apps/flashscope/flash.html
@@ -5,9 +5,7 @@
   {{flash.success}}!!
 {{else}}
   Welcome
-  <form action="/" method="post">
-    <input name="message">
-  </form>
+  <a href="/send">Redirect me</a>
 {{/if}}
 </body>
 </html>
diff --git a/jooby/src/main/java/org/jooby/Cookie.java b/jooby/src/main/java/org/jooby/Cookie.java
@@ -182,6 +182,15 @@ public Definition(final String name, final String value) {
       value(value);
     }
 
+    /**
+     * Creates a new {@link Definition cookie's definition}.
+     *
+     * @param name Cookie's name.
+     */
+    public Definition(final String name) {
+      name(name);
+    }
+
     /**
      * Produces a cookie from current definition.
      *

diff --git a/jooby/src/main/java/org/jooby/FlashScope.java b/jooby/src/main/java/org/jooby/FlashScope.java
@@ -18,7 +18,10 @@
  */
 package org.jooby;
 
+import static java.util.Objects.requireNonNull;
+
 import java.util.Map;
+import java.util.Optional;
 import java.util.function.Function;
 
 import org.jooby.internal.handlers.FlashScopeHandler;
@@ -101,14 +104,41 @@ public class FlashScope implements Jooby.Module {
 
   private Function<Map<String, String>, String> encoder = Cookie.URL_ENCODER;
 
-  private String cookie = "flash";
+  private Optional<Cookie.Definition> cookie = Optional.empty();
 
   private String method = "*";
 
   private String path = "*";
 
+  /**
+   * Creates a new {@link FlashScope} and customize the flash cookie.
+   *
+   * @param cookie Cookie template for flash scope.
+   */
+  public FlashScope(final Cookie.Definition cookie) {
+    this.cookie = Optional.of(requireNonNull(cookie, "Cookie required."));
+  }
+
+  /**
+   * Creates a new {@link FlashScope}.
+   */
+  public FlashScope() {
+  }
+
   @Override
   public void configure(final Env env, final Config conf, final Binder binder) {
+    Config $cookie = conf.getConfig("flash.cookie");
+    String cpath = $cookie.getString("path");
+    boolean chttp = $cookie.getBoolean("httpOnly");
+    boolean csecure = $cookie.getBoolean("secure");
+    Cookie.Definition cookie = this.cookie
+        .orElseGet(() -> new Cookie.Definition($cookie.getString("name")));
+
+    // uses user provided or fallback to defaults
+    cookie.path(cookie.path().orElse(cpath))
+        .httpOnly(cookie.httpOnly().orElse(chttp))
+        .secure(cookie.secure().orElse(csecure));
+
     env.routes().use(method, path, new FlashScopeHandler(cookie, decoder, encoder))
         .name("flash-scope");
   }

diff --git a/jooby/src/main/java/org/jooby/internal/handlers/FlashScopeHandler.java b/jooby/src/main/java/org/jooby/internal/handlers/FlashScopeHandler.java
@@ -31,23 +31,26 @@
 
 public class FlashScopeHandler implements Route.Filter {
 
-  private String name;
+  private Cookie.Definition template;
+
+  private String cname;
 
   private Function<String, Map<String, String>> decoder;
 
   private Function<Map<String, String>, String> encoder;
 
-  public FlashScopeHandler(final String cname, final Function<String, Map<String, String>> decoder,
+  public FlashScopeHandler(final Cookie.Definition cookie, final Function<String, Map<String, String>> decoder,
       final Function<Map<String, String>, String> encoder) {
-    this.name = cname;
+    this.template = cookie;
+    this.cname = cookie.name().get();
     this.decoder = decoder;
     this.encoder = encoder;
   }
 
   @Override
   public void handle(final Request req, final Response rsp, final Route.Chain chain)
       throws Throwable {
-    Optional<String> value = req.cookie(name).toOptional();
+    Optional<String> value = req.cookie(cname).toOptional();
     Map<String, String> flashScope = value.map(decoder::apply)
         .orElseGet(HashMap::new);
     Map<String, String> copy = new HashMap<>(flashScope);
@@ -67,16 +70,16 @@ private Route.After finalizeFlash(final Map<String, String> initialScope,
       if (scope.equals(initialScope)) {
         // 1.a. existing data available, discard
         if (scope.size() > 0) {
-          rsp.cookie(new Cookie.Definition(name, "").maxAge(0));
+          rsp.cookie(new Cookie.Definition(template).maxAge(0));
         }
       } else {
         // 2. change detected
         if (scope.size() == 0) {
           // 2.a everything was removed from app logic
-          rsp.cookie(new Cookie.Definition(name, "").maxAge(0));
+          rsp.cookie(new Cookie.Definition(template).maxAge(0));
         } else {
           // 2.b there is something to see in the next request
-          rsp.cookie(name, encoder.apply(scope));
+          rsp.cookie(new Cookie.Definition(template).value(encoder.apply(scope)));
         }
       }
       return result;

diff --git a/jooby/src/main/resources/org/jooby/jooby.conf b/jooby/src/main/resources/org/jooby/jooby.conf
@@ -96,6 +96,21 @@ session {
   }
 }
 
+###################################################################################################
+#! flash scope defaults
+###################################################################################################
+flash {
+  cookie {
+    name = jooby.flash
+
+    path = ${application.path}
+
+    httpOnly = true
+
+    secure = false
+  }
+}
+
 ###################################################################################################
 #! server defaults
 ###################################################################################################