Bug 1408962 - Make add cookie command webdriver spec comformant r?ato, whimboo draft
authorPeter Major <majorpetya@gmail.com>
Sat, 28 Oct 2017 11:37:40 +0100
changeset 693348 20b2962608cd328d76145994fe71f94af8cde273
parent 693347 e64846245b002c204144ac412fbd874e32e47edf
child 739001 65b2ade6c381bc55df2cda5fd583d6964d055cda
push id87767
push userbmo:majorpetya@gmail.com
push dateSun, 05 Nov 2017 22:19:11 +0000
reviewersato, whimboo
bugs1408962
milestone58.0a1
Bug 1408962 - Make add cookie command webdriver spec comformant r?ato, whimboo * Changed positiveInteger to validate upper bounds * Removed processing for the no longer existent 'session' field * Added default values for absent fields MozReview-Commit-ID: E4x6TyMwZQs
testing/marionette/assert.js
testing/marionette/cookie.js
testing/marionette/test_cookie.js
testing/web-platform/tests/webdriver/tests/cookies/add_cookie.py
--- a/testing/marionette/assert.js
+++ b/testing/marionette/assert.js
@@ -228,17 +228,17 @@ assert.callable = function(obj, msg = ""
  * @return {number}
  *     <var>obj</var> is returned unaltered.
  *
  * @throws {InvalidArgumentError}
  *     If <var>obj</var> is not an integer.
  */
 assert.integer = function(obj, msg = "") {
   msg = msg || pprint`Expected ${obj} to be an integer`;
-  return assert.that(Number.isInteger, msg)(obj);
+  return assert.that(Number.isSafeInteger, msg)(obj);
 };
 
 /**
  * Asserts that <var>obj</var> is a positive integer.
  *
  * @param {?} obj
  *     Value to test.
  * @param {string=} msg
--- a/testing/marionette/cookie.js
+++ b/testing/marionette/cookie.js
@@ -4,17 +4,20 @@
 
 "use strict";
 
 const {interfaces: Ci, utils: Cu, results: Cr} = Components;
 
 Cu.import("resource://gre/modules/Services.jsm");
 
 Cu.import("chrome://marionette/content/assert.js");
-const {InvalidCookieDomainError} = Cu.import("chrome://marionette/content/error.js", {});
+const {
+  InvalidCookieDomainError,
+  UnableToSetCookieError,
+} = Cu.import("chrome://marionette/content/error.js", {});
 const {pprint} = Cu.import("chrome://marionette/content/format.js", {});
 
 this.EXPORTED_SYMBOLS = ["cookie"];
 
 const IPV4_PORT_EXPR = /:\d+$/;
 
 /** @namespace */
 this.cookie = {
@@ -29,54 +32,50 @@ this.cookie = {
 
 /**
  * Unmarshal a JSON Object to a cookie representation.
  *
  * Effectively this will run validation checks on |json|, which will
  * produce the errors expected by WebDriver if the input is not valid.
  *
  * @param {Object.<string, (number|boolean|string)>} json
- *     Cookie to be deserialised.  <var>name</var> and <var>value</var>
- *     are required fields which must be strings.  The <var>path</var> and
+ *     Cookie to be deserialised. <var>name</var> and <var>value</var>
+ *     are required fields which must be strings. The <var>path</var> and
  *     <var>domain</var> fields are optional, but must be a string if
  *     provided.
- *     The <var>secure</var>, <var>httpOnly</var>, and
- *     <var>session</var>fields are similarly optional, but must be
- *     booleans.  Likewise, the <var>expiry</var> field is optional but
- *     must be unsigned integer.
+ *     The <var>secure</var>, and <var>httpOnly</var> are similarly
+ *     optional, but must be booleans. Likewise, the <var>expiry</var> field
+ *     is optional but must be unsigned integer.
  *
  * @return {Cookie}
  *     Valid cookie object.
  *
  * @throws {InvalidArgumentError}
  *     If any of the properties are invalid.
  */
 cookie.fromJSON = function(json) {
   let newCookie = {};
 
   assert.object(json, pprint`Expected cookie object, got ${json}`);
 
   newCookie.name = assert.string(json.name, "Cookie name must be string");
   newCookie.value = assert.string(json.value, "Cookie value must be string");
 
+  if (typeof json.path != "undefined") {
+    newCookie.path = assert.string(json.path, "Cookie path must be string");
+  }
   if (typeof json.domain != "undefined") {
     newCookie.domain = assert.string(json.domain, "Cookie domain must be string");
   }
-  if (typeof json.path != "undefined") {
-    newCookie.path = assert.string(json.path, "Cookie path must be string");
-  }
   if (typeof json.secure != "undefined") {
     newCookie.secure = assert.boolean(json.secure, "Cookie secure flag must be boolean");
   }
   if (typeof json.httpOnly != "undefined") {
     newCookie.httpOnly = assert.boolean(json.httpOnly, "Cookie httpOnly flag must be boolean");
   }
-  if (typeof json.session != "undefined") {
-    newCookie.session = assert.boolean(json.session, "Cookie session flag must be boolean");
-  }
   if (typeof json.expiry != "undefined") {
     newCookie.expiry = assert.positiveInteger(json.expiry, "Cookie expiry must be a positive integer");
   }
 
   return newCookie;
 };
 
 /**
@@ -88,38 +87,46 @@ cookie.fromJSON = function(json) {
  *     Perform test that <var>newCookie</var>'s domain matches this.
  *
  * @throws {TypeError}
  *     If <var>name</var>, <var>value</var>, or <var>domain</var> are
  *     not present and of the correct type.
  * @throws {InvalidCookieDomainError}
  *     If <var>restrictToHost</var> is set and <var>newCookie</var>'s
  *     domain does not match.
+ * @throws {UnableToSetCookieError}
+ *     If an error occurred while trying to save the cookie.
  */
 cookie.add = function(newCookie, {restrictToHost = null} = {}) {
   assert.string(newCookie.name, "Cookie name must be string");
   assert.string(newCookie.value, "Cookie value must be string");
 
+  if (typeof newCookie.path == "undefined") {
+    newCookie.path = "/";
+  }
+
   let hostOnly = false;
   if (typeof newCookie.domain == "undefined") {
     hostOnly = true;
     newCookie.domain = restrictToHost;
   }
   assert.string(newCookie.domain, "Cookie domain must be string");
 
-  if (typeof newCookie.path == "undefined") {
-    newCookie.path = "/";
+  if (typeof newCookie.secure == "undefined") {
+    newCookie.secure = false;
   }
-
+  if (typeof newCookie.httpOnly == "undefined") {
+    newCookie.httpOnly = false;
+  }
   if (typeof newCookie.expiry == "undefined") {
-    // twenty years into the future
-    let date = new Date();
-    let now = new Date(Date.now());
-    date.setYear(now.getFullYear() + 20);
-    newCookie.expiry = date.getTime() / 1000;
+    // The XPCOM interface requires the expiry field even for session cookies.
+    newCookie.expiry = Number.MAX_SAFE_INTEGER;
+    newCookie.session = true;
+  } else {
+    newCookie.session = false;
   }
 
   let isIpAddress = false;
   try {
     Services.eTLD.getPublicSuffixFromHost(newCookie.domain);
   } catch (e) {
     switch (e.result) {
       case Cr.NS_ERROR_HOST_IS_IP_ADDRESS:
@@ -145,26 +152,30 @@ cookie.add = function(newCookie, {restri
     }
   }
 
   // remove port from domain, if present.
   // unfortunately this catches IPv6 addresses by mistake
   // TODO: Bug 814416
   newCookie.domain = newCookie.domain.replace(IPV4_PORT_EXPR, "");
 
-  cookie.manager.add(
-      newCookie.domain,
-      newCookie.path,
-      newCookie.name,
-      newCookie.value,
-      newCookie.secure,
-      newCookie.httpOnly,
-      newCookie.session,
-      newCookie.expiry,
-      {} /* origin attributes */);
+  try {
+    cookie.manager.add(
+        newCookie.domain,
+        newCookie.path,
+        newCookie.name,
+        newCookie.value,
+        newCookie.secure,
+        newCookie.httpOnly,
+        newCookie.session,
+        newCookie.expiry,
+        {} /* origin attributes */);
+  } catch (e) {
+    throw new UnableToSetCookieError(e);
+  }
 };
 
 /**
  * Remove cookie from the cookie store.
  *
  * @param {Cookie} toDelete
  *     Cookie to remove.
  */
--- a/testing/marionette/test_cookie.js
+++ b/testing/marionette/test_cookie.js
@@ -5,16 +5,19 @@
 const {utils: Cu} = Components;
 
 Cu.import("chrome://marionette/content/cookie.js");
 
 cookie.manager = {
   cookies: [],
 
   add: function (domain, path, name, value, secure, httpOnly, session, expiry, originAttributes) {
+    if (name === "fail") {
+      throw new Error("An error occurred while adding cookie");
+    }
     let newCookie = {
       host: domain,
       path: path,
       name: name,
       value: value,
       isSecure: secure,
       isHttpOnly: httpOnly,
       isSession: session,
@@ -111,28 +114,18 @@ add_test(function test_fromJSON() {
     let test = {
       name: "foo",
       value: "bar",
       httpOnly: invalidType,
     };
     Assert.throws(() => cookie.fromJSON(test), /Cookie httpOnly flag must be boolean/);
   }
 
-  // session
-  for (let invalidType of ["foo", 42, [], {}, null]) {
-    let test = {
-      name: "foo",
-      value: "bar",
-      session: invalidType,
-    };
-    Assert.throws(() => cookie.fromJSON(test), /Cookie session flag must be boolean/);
-  }
-
   // expiry
-  for (let invalidType of ["foo", true, [], {}, null]) {
+  for (let invalidType of [-1, Number.MAX_SAFE_INTEGER + 1, "foo", true, [], {}, null]) {
     let test = {
       name: "foo",
       value: "bar",
       expiry: invalidType,
     };
     Assert.throws(() => cookie.fromJSON(test), /Cookie expiry must be a positive integer/);
   }
 
@@ -147,26 +140,24 @@ add_test(function test_fromJSON() {
   // everything
   let full = cookie.fromJSON({
     name: "name",
     value: "value",
     domain: ".domain",
     path: "path",
     secure: true,
     httpOnly: true,
-    session: true,
     expiry: 42,
   });
   equal("name", full.name);
   equal("value", full.value);
   equal(".domain", full.domain);
   equal("path", full.path);
   equal(true, full.secure);
   equal(true, full.httpOnly);
-  equal(true, full.session);
   equal(42, full.expiry);
 
   run_next_test();
 });
 
 add_test(function test_add() {
   cookie.manager.cookies = [];
 
@@ -216,16 +207,20 @@ add_test(function test_add() {
   cookie.add({
     name: "name5",
     value: "value5",
     domain: "domain5",
     path: "/foo/bar",
   });
   equal("/foo/bar", cookie.manager.cookies[3].path);
 
+  Assert.throws(() => {
+    cookie.add({name: "fail", value: "value6", domain: "domain6"})
+  }, /UnableToSetCookieError/);
+
   run_next_test();
 });
 
 add_test(function test_remove() {
   cookie.manager.cookies = [];
 
   let crumble = {
     name: "test_remove",
@@ -242,41 +237,38 @@ add_test(function test_remove() {
   equal(0, cookie.manager.cookies.length);
   equal(undefined, cookie.manager.cookies[0]);
 
   run_next_test();
 });
 
 add_test(function test_iter() {
   cookie.manager.cookies = [];
+  let tomorrow = new Date();
+  tomorrow.setHours(tomorrow.getHours() + 24);
 
   cookie.add({
-    session: false,
+    expiry: tomorrow,
     name: "0",
     value: "",
     domain: "foo.example.com",
   });
   cookie.add({
-    session: false,
+    expiry: tomorrow,
     name: "1",
     value: "",
     domain: "bar.example.com",
   });
 
   let fooCookies = [...cookie.iter("foo.example.com")];
   equal(1, fooCookies.length);
   equal(".foo.example.com", fooCookies[0].domain);
   equal(true, fooCookies[0].hasOwnProperty("expiry"));
 
-  // here we're explicitly setting session to true as a workaround until
-  // bug 1408962 has been fixed. when that bug has been fixed the cookie
-  // will be created as session cookie simply by leaving out the 'expiry'
-  // property.
   cookie.add({
-    session: true,
     name: "aSessionCookie",
     value: "",
     domain: "session.com",
   });
 
   let sessionCookies = [...cookie.iter("session.com")];
   equal(1, sessionCookies.length);
   equal("aSessionCookie", sessionCookies[0].name);
--- a/testing/web-platform/tests/webdriver/tests/cookies/add_cookie.py
+++ b/testing/web-platform/tests/webdriver/tests/cookies/add_cookie.py
@@ -1,10 +1,11 @@
 from tests.support.fixtures import clear_all_cookies
 from tests.support.fixtures import server_config
+from datetime import datetime, timedelta
 
 def test_add_domain_cookie(session, url):
     session.url = url("/common/blank.html")
     clear_all_cookies(session)
     create_cookie_request = {
         "cookie": {
             "name": "hello",
             "value": "world",
@@ -69,8 +70,75 @@ def test_add_cookie_for_ip(session, url,
     assert "value" in cookie
     assert isinstance(cookie["value"], basestring)
     assert "domain" in cookie
     assert isinstance(cookie["domain"], basestring)
 
     assert cookie["name"] == "hello"
     assert cookie["value"] == "world"
     assert cookie["domain"] == "127.0.0.1"
+
+def test_add_non_session_cookie(session, url):
+    session.url = url("/common/blank.html")
+    clear_all_cookies(session)
+    a_year_from_now = int((datetime.utcnow() + timedelta(days=365)).strftime("%s"))
+    create_cookie_request = {
+        "cookie": {
+            "name": "hello",
+            "value": "world",
+            "expiry": a_year_from_now
+        }
+    }
+    result = session.transport.send("POST", "session/%s/cookie" % session.session_id, create_cookie_request)
+    assert result.status == 200
+    assert "value" in result.body
+    assert isinstance(result.body["value"], dict)
+
+    result = session.transport.send("GET", "session/%s/cookie" % session.session_id)
+    assert result.status == 200
+    assert "value" in result.body
+    assert isinstance(result.body["value"], list)
+    assert len(result.body["value"]) == 1
+    assert isinstance(result.body["value"][0], dict)
+
+    cookie = result.body["value"][0]
+    assert "name" in cookie
+    assert isinstance(cookie["name"], basestring)
+    assert "value" in cookie
+    assert isinstance(cookie["value"], basestring)
+    assert "expiry" in cookie
+    assert isinstance(cookie["expiry"], int)
+
+    assert cookie["name"] == "hello"
+    assert cookie["value"] == "world"
+    assert cookie["expiry"] == a_year_from_now
+
+def test_add_session_cookie(session, url):
+    session.url = url("/common/blank.html")
+    clear_all_cookies(session)
+    create_cookie_request = {
+        "cookie": {
+            "name": "hello",
+            "value": "world"
+        }
+    }
+    result = session.transport.send("POST", "session/%s/cookie" % session.session_id, create_cookie_request)
+    assert result.status == 200
+    assert "value" in result.body
+    assert isinstance(result.body["value"], dict)
+
+    result = session.transport.send("GET", "session/%s/cookie" % session.session_id)
+    assert result.status == 200
+    assert "value" in result.body
+    assert isinstance(result.body["value"], list)
+    assert len(result.body["value"]) == 1
+    assert isinstance(result.body["value"][0], dict)
+
+    cookie = result.body["value"][0]
+    assert "name" in cookie
+    assert isinstance(cookie["name"], basestring)
+    assert "value" in cookie
+    assert isinstance(cookie["value"], basestring)
+    assert "expiry" in cookie
+    assert cookie.get("expiry") is None
+
+    assert cookie["name"] == "hello"
+    assert cookie["value"] == "world"