Bug 1372595 - Return single cookie for GetNamedCookie; r=jgraham draft
authorAndreas Tolfsen <ato@sny.no>
Fri, 16 Jun 2017 16:46:34 +0100
changeset 602405 cdd936ecddb1ce15f8f8cbf128cdc005369574f9
parent 602404 0df947287b5950e35b4aa4a5c22c0ef9648886aa
child 635592 03f6a7ac312fb99d5161486aa0bbeaa5eef56203
push id66423
push userbmo:ato@sny.no
push dateThu, 29 Jun 2017 23:57:19 +0000
reviewersjgraham
bugs1372595
milestone56.0a1
Bug 1372595 - Return single cookie for GetNamedCookie; r=jgraham The GetNamedCookie command currently returns a JSON Array of one, retained cookie, after it has removed the cookies that don't match by name. This is in violation of the WebDriver specification, which says it must return the cookie serialisation directly. MozReview-Commit-ID: 9yEiarEGBez
testing/geckodriver/src/marionette.rs
testing/web-platform/meta/MANIFEST.json
testing/web-platform/tests/webdriver/cookies.py
--- a/testing/geckodriver/src/marionette.rs
+++ b/testing/geckodriver/src/marionette.rs
@@ -35,21 +35,20 @@ use webdriver::command::WebDriverCommand
     DeleteCookies, DeleteCookie, GetTimeouts, SetTimeouts, DismissAlert,
     AcceptAlert, GetAlertText, SendAlertText, TakeScreenshot, TakeElementScreenshot,
     Extension, PerformActions, ReleaseActions};
 use webdriver::command::{
     NewSessionParameters, GetParameters, WindowRectParameters, SwitchToWindowParameters,
     SwitchToFrameParameters, LocatorParameters, JavascriptCommandParameters,
     GetNamedCookieParameters, AddCookieParameters, TimeoutsParameters,
     ActionsParameters, TakeScreenshotParameters};
-use webdriver::response::{CloseWindowResponse, Cookie, CookiesResponse,
-                          RectResponse, NewSessionResponse, TimeoutsResponse, ValueResponse,
+use webdriver::response::{CloseWindowResponse, Cookie, CookieResponse, CookiesResponse,
+                          NewSessionResponse, RectResponse, TimeoutsResponse, ValueResponse,
                           WebDriverResponse};
-use webdriver::common::{
-    Date, Nullable, WebElement, FrameId, ELEMENT_KEY};
+use webdriver::common::{Date, ELEMENT_KEY, FrameId, Nullable, WebElement};
 use webdriver::error::{ErrorStatus, WebDriverError, WebDriverResult};
 use webdriver::server::{WebDriverHandler, Session};
 use webdriver::httpapi::{WebDriverExtensionRoute};
 
 use capabilities::{FirefoxCapabilities, FirefoxOptions};
 use prefs;
 
 const DEFAULT_HOST: &'static str = "localhost";
@@ -765,22 +764,25 @@ impl MarionetteSession {
                              "Failed to find y field").as_f64(),
                     ErrorStatus::UnknownError,
                     "Failed to interpret y as float");
 
                 WebDriverResponse::WindowRect(RectResponse::new(x, y, width, height))
             },
             GetCookies => {
                 let cookies = try!(self.process_cookies(&resp.result));
-                WebDriverResponse::Cookies(CookiesResponse {value: cookies})
+                WebDriverResponse::Cookies(CookiesResponse { value: cookies })
             },
             GetNamedCookie(ref name) => {
                 let mut cookies = try!(self.process_cookies(&resp.result));
                 cookies.retain(|x| x.name == *name);
-                WebDriverResponse::Cookies(CookiesResponse { value : cookies })
+                let cookie = try_opt!(cookies.pop(),
+                                      ErrorStatus::NoSuchCookie,
+                                      format!("No cookie with name {}", name));
+                WebDriverResponse::Cookie(CookieResponse { value: cookie })
             }
             FindElement(_) | FindElementElement(_, _) => {
                 let element = try!(self.to_web_element(
                     try_opt!(resp.result.find("value"),
                              ErrorStatus::UnknownError,
                              "Failed to find value field")));
                 WebDriverResponse::Generic(ValueResponse::new(element.to_json()))
             },
@@ -864,57 +866,66 @@ impl MarionetteSession {
     fn process_cookies(&self, json_data: &Json) -> WebDriverResult<Vec<Cookie>> {
         let value = try_opt!(json_data.as_array(),
                              ErrorStatus::UnknownError,
                              "Failed to interpret value as array");
         value.iter().map(|x| {
             let name = try_opt!(
                 try_opt!(x.find("name"),
                          ErrorStatus::UnknownError,
-                         "Failed to find name field").as_string(),
+                         "Cookie must have a name field").as_string(),
                 ErrorStatus::UnknownError,
-                "Failed to interpret name as string").to_string();
+                "Cookie must have string name").to_string();
             let value = try_opt!(
                 try_opt!(x.find("value"),
                          ErrorStatus::UnknownError,
-                         "Failed to find value field").as_string(),
+                         "Cookie must have a value field").as_string(),
                 ErrorStatus::UnknownError,
-                "Failed to interpret value as string").to_string();
+                "Cookie must have a string value").to_string();
             let path = try!(
                 Nullable::from_json(x.find("path").unwrap_or(&Json::Null),
                                     |x| {
                                         Ok((try_opt!(x.as_string(),
                                                      ErrorStatus::UnknownError,
-                                                     "Failed to interpret path as String")).to_string())
+                                                     "Cookie path must be string")).to_string())
                                     }));
             let domain = try!(
                 Nullable::from_json(x.find("domain").unwrap_or(&Json::Null),
                                     |x| {
                                         Ok((try_opt!(x.as_string(),
                                                      ErrorStatus::UnknownError,
-                                                     "Failed to interpret domain as String")).to_string())
+                                                     "Cookie domain must be string")).to_string())
                                     }));
             let expiry = try!(
                 Nullable::from_json(x.find("expiry").unwrap_or(&Json::Null),
                                     |x| {
                                         Ok(Date::new((try_opt!(
                                             x.as_u64(),
                                             ErrorStatus::UnknownError,
-                                            "Failed to interpret expiry as u64"))))
+                                            "Cookie expiry must be a positive integer"))))
                                     }));
             let secure = try_opt!(
                 x.find("secure").map_or(Some(false), |x| x.as_boolean()),
                 ErrorStatus::UnknownError,
-                "Failed to interpret secure as boolean");
+                "Cookie secure flag must be boolean");
             let http_only = try_opt!(
                 x.find("httpOnly").map_or(Some(false), |x| x.as_boolean()),
                 ErrorStatus::UnknownError,
-                "Failed to interpret httpOnly as boolean");
-            Ok(Cookie {name: name , value: value, path: path, domain: domain,
-                      expiry: expiry, secure: secure , httpOnly: http_only})
+                "Cookie httpOnly flag must be boolean");
+
+            let new_cookie = Cookie {
+                name: name,
+                value: value,
+                path: path,
+                domain: domain,
+                expiry: expiry,
+                secure: secure,
+                httpOnly: http_only,
+            };
+            Ok(new_cookie)
         }).collect::<Result<Vec<_>, _>>()
     }
 
     pub fn error_from_string(&self, error_code: &str) -> ErrorStatus {
         match error_code {
             "element click intercepted" => ErrorStatus::ElementClickIntercepted,
             "element not interactable" | "element not visible" => ErrorStatus::ElementNotInteractable,
             "element not selectable" => ErrorStatus::ElementNotSelectable,
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -138528,16 +138528,22 @@
     ]
    ],
    "webdriver/contexts.py": [
     [
      "/webdriver/contexts.py",
      {}
     ]
    ],
+   "webdriver/cookies.py": [
+    [
+     "/webdriver/cookies.py",
+     {}
+    ]
+   ],
    "webdriver/navigation.py": [
     [
      "/webdriver/navigation.py",
      {}
     ]
    ],
    "webdriver/window_maximizing.py": [
     [
@@ -220619,16 +220625,20 @@
   "webdriver/conftest.py": [
    "6217bd14a1ec72f00d5a6a9014a9dc991e4289db",
    "support"
   ],
   "webdriver/contexts.py": [
    "ae157de034d7887d20f2d14fe880779bb1cc1b41",
    "wdspec"
   ],
+  "webdriver/cookies.py": [
+   "2435c4f34a9c5c8296ca775163381d472ca581b9",
+   "wdspec"
+  ],
   "webdriver/interface.html": [
    "d783d0dd370f58b264ef238d8da5cd8601dc3c7f",
    "testharness"
   ],
   "webdriver/navigation.py": [
    "dd2f52a3a77497ce8785e698f9ab462390ed0d57",
    "wdspec"
   ],
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/webdriver/cookies.py
@@ -0,0 +1,28 @@
+def test_get_named_cookie(session, url):
+    session.url = url("/common/blank.html")
+    session.execute_script("document.cookie = 'foo=bar'")
+
+    result = session.transport.send("GET", "session/%s/cookie/foo" % session.session_id)
+    assert result.status == 200
+    assert isinstance(result.body["value"], dict)
+
+    # table for cookie conversion
+    # https://w3c.github.io/webdriver/webdriver-spec.html#dfn-table-for-cookie-conversion
+    cookie = result.body["value"]
+    assert "name" in cookie
+    assert isinstance(cookie["name"], basestring)
+    assert "value" in cookie
+    assert isinstance(cookie["value"], basestring)
+    assert "path" in cookie
+    assert isinstance(cookie["path"], basestring)
+    assert "domain" in cookie
+    assert isinstance(cookie["domain"], basestring)
+    assert "secure" in cookie
+    assert isinstance(cookie["secure"], bool)
+    assert "httpOnly" in cookie
+    assert isinstance(cookie["httpOnly"], bool)
+    assert "expiry" in cookie
+    assert isinstance(cookie["expiry"], (int, long))
+
+    assert cookie["name"] == "foo"
+    assert cookie["value"] == "bar"