Merge SecurityOrigin::canAccessCheckSuborigins into canAccess

Suborigins are supposed to make script from different suborigin
namespaces inaccessible to each other. canAccess (in spec terms
same-origin domain) is supposed to do that, so just fold the two
together

BUG=336894
R=mkwst@chromium.org

Review-Url: https://codereview.chromium.org/2805683005
Cr-Commit-Position: refs/heads/master@{#462872}

third_party/WebKit/Source/SpecMapping.md

@@ -28,9 +28,9 @@ domain](https://html.spec.whatwg.org/multipage/browsers.html#same-origin-domain)
 using `SecurityOrigin::isSameSchemeHostPort`.
 
 The [Suborigins spec](https://w3c.github.io/webappsec-suborigins/) extends
-HTML's definition of origins. To check for same-origin and same-origin domain
-use `SecurityOrigin::canAccessCheckSuborigins` and
-`SecurityOrigin::isSameSchemeHostPortAndSuborigin`.
+HTML's definition of origins. To check for same-origin corresponds to
+`SecurityOrigin::isSameSchemeHostPortAndSuborigin` while the check for same-origin
+domain already takes the suborigin into account.
 
 ### [Window object](https://html.spec.whatwg.org/#window)
 

third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp

@@ -60,7 +60,7 @@ bool canAccessFrameInternal(const LocalDOMWindow* accessingWindow,
 
   const SecurityOrigin* accessingOrigin =
       accessingWindow->document()->getSecurityOrigin();
-  if (!accessingOrigin->canAccessCheckSuborigins(targetFrameOrigin))
+  if (!accessingOrigin->canAccess(targetFrameOrigin))
     return false;
 
   // Notify the loader's client if the initial document has been accessed.
@@ -241,7 +241,7 @@ bool BindingSecurity::shouldAllowNamedAccessTo(const DOMWindow* accessingWindow,
   SECURITY_CHECK(!(targetWindow && targetWindow->frame()) ||
                  targetWindow == targetWindow->frame()->domWindow());
 
-  if (!accessingOrigin->canAccessCheckSuborigins(targetOrigin))
+  if (!accessingOrigin->canAccess(targetOrigin))
     return false;
 
   // Note that there is no need to call back

third_party/WebKit/Source/core/css/CSSStyleSheet.cpp

@@ -256,10 +256,10 @@ bool CSSStyleSheet::canAccessRules() const {
     return true;
   if (document->getSecurityOrigin()->canRequestNoSuborigin(baseURL))
     return true;
-  if (m_allowRuleAccessFromOrigin &&
-      document->getSecurityOrigin()->canAccessCheckSuborigins(
-          m_allowRuleAccessFromOrigin.get()))
+  if (m_allowRuleAccessFromOrigin && document->getSecurityOrigin()->canAccess(
+                                         m_allowRuleAccessFromOrigin.get())) {
     return true;
+  }
   return false;
 }
 

third_party/WebKit/Source/core/frame/DOMWindow.cpp

@@ -137,9 +137,10 @@ bool DOMWindow::isInsecureScriptAccess(LocalDOMWindow& callingWindow,
     // FIXME: The name canAccess seems to be a roundabout way to ask "can
     // execute script".  Can we name the SecurityOrigin function better to make
     // this more clear?
-    if (callingWindow.document()->getSecurityOrigin()->canAccessCheckSuborigins(
-            frame()->securityContext()->getSecurityOrigin()))
+    if (callingWindow.document()->getSecurityOrigin()->canAccess(
+            frame()->securityContext()->getSecurityOrigin())) {
       return false;
+    }
   }
 
   callingWindow.printErrorMessage(
@@ -270,8 +271,7 @@ String DOMWindow::crossDomainAccessErrorMessage(
   // It's possible for a remote frame to be same origin with respect to a
   // local frame, but it must still be treated as a disallowed cross-domain
   // access. See https://crbug.com/601629.
-  ASSERT(frame()->isRemoteFrame() ||
-         !activeOrigin->canAccessCheckSuborigins(targetOrigin));
+  DCHECK(frame()->isRemoteFrame() || !activeOrigin->canAccess(targetOrigin));
 
   String message = "Blocked a frame with origin \"" + activeOrigin->toString() +
                    "\" from accessing a frame with origin \"" +

third_party/WebKit/Source/modules/serviceworkers/NavigatorServiceWorker.cpp

@@ -57,7 +57,7 @@ ServiceWorkerContainer* NavigatorServiceWorker::serviceWorker(
     ExceptionState& exceptionState) {
   ExecutionContext* executionContext = scriptState->getExecutionContext();
   DCHECK(!navigator.frame() ||
-         executionContext->getSecurityOrigin()->canAccessCheckSuborigins(
+         executionContext->getSecurityOrigin()->canAccess(
              navigator.frame()->securityContext()->getSecurityOrigin()));
   return NavigatorServiceWorker::from(navigator).serviceWorker(
       navigator.frame(), exceptionState);
@@ -69,7 +69,7 @@ ServiceWorkerContainer* NavigatorServiceWorker::serviceWorker(
     String& errorMessage) {
   ExecutionContext* executionContext = scriptState->getExecutionContext();
   DCHECK(!navigator.frame() ||
-         executionContext->getSecurityOrigin()->canAccessCheckSuborigins(
+         executionContext->getSecurityOrigin()->canAccess(
              navigator.frame()->securityContext()->getSecurityOrigin()));
   return NavigatorServiceWorker::from(navigator).serviceWorker(
       navigator.frame(), errorMessage);

third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp

@@ -231,6 +231,12 @@ bool SecurityOrigin::canAccess(const SecurityOrigin* other) const {
   if (isUnique() || other->isUnique())
     return false;
 
+  if (hasSuborigin() != other->hasSuborigin())
+    return false;
+
+  if (hasSuborigin() && suborigin()->name() != other->suborigin()->name())
+    return false;
+
   // document.domain handling, as per
   // https://html.spec.whatwg.org/multipage/browsers.html#dom-document-domain:
   //
@@ -257,17 +263,6 @@ bool SecurityOrigin::canAccess(const SecurityOrigin* other) const {
   return canAccess;
 }
 
-bool SecurityOrigin::canAccessCheckSuborigins(
-    const SecurityOrigin* other) const {
-  if (hasSuborigin() != other->hasSuborigin())
-    return false;
-
-  if (hasSuborigin() && suborigin()->name() != other->suborigin()->name())
-    return false;
-
-  return canAccess(other);
-}
-
 bool SecurityOrigin::passesFileCheck(const SecurityOrigin* other) const {
   ASSERT(isLocal() && other->isLocal());
 

third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h

@@ -102,19 +102,9 @@ class PLATFORM_EXPORT SecurityOrigin : public RefCounted<SecurityOrigin> {
   // SecurityOrigin. For example, call this function before allowing
   // script from one security origin to read or write objects from
   // another SecurityOrigin.
-  bool canAccess(const SecurityOrigin*) const;
-
-  // Same as canAccess, except that it adds an additional check to make sure
-  // that the SecurityOrigins have the same suborigin name. If you're not
-  // familiar with Suborigins, you probably want canAccess() for now.
-  // Suborigins is a spec in progress, and where it should be enforced is
-  // still in flux. See https://crbug.com/336894 for more details.
   //
-  // TODO(jww): Once the Suborigin spec has become more settled, and we are
-  // confident in the correctness of our implementation, canAccess should be
-  // made to check the suborigin and this should be turned into
-  // canAccessBypassSuborigin check, which should be the exceptional case.
-  bool canAccessCheckSuborigins(const SecurityOrigin*) const;
+  // This takes suborigins into account.
+  bool canAccess(const SecurityOrigin*) const;
 
   // Returns true if this SecurityOrigin can read content retrieved from
   // the given URL. For example, call this function before issuing
@@ -122,11 +112,10 @@ class PLATFORM_EXPORT SecurityOrigin : public RefCounted<SecurityOrigin> {
   bool canRequest(const KURL&) const;
 
   // Same as canRequest, except that it adds an additional check to make sure
-  // that the SecurityOrigin does not have a suborigin name. Like with
-  // canAccessCheckSuborigins() above, if you're not familiar with
-  // Suborigins, you probably want canRequest() for now. Suborigins is a spec
-  // in progress, and where it should be enforced is still in flux. See
-  // https://crbug.com/336894 for more details.
+  // that the SecurityOrigin does not have a suborigin name. If you're not
+  // familiar with Suborigins, you probably want canRequest() for now.
+  // Suborigins is a spec in progress, and where it should be enforced is still
+  // in flux. See https://crbug.com/336894 for more details.
   //
   // TODO(jww): Once the Suborigin spec has become more settled, and we are
   // confident in the correctness of our implementation, canRequest should be

third_party/WebKit/Source/platform/weborigin/SecurityOriginTest.cpp

@@ -334,17 +334,16 @@ TEST_F(SecurityOriginTest, CanAccess) {
 
   struct TestCase {
     bool canAccess;
-    bool canAccessCheckSuborigins;
     const char* origin1;
     const char* origin2;
   };
 
   TestCase tests[] = {
-      {true, true, "https://foobar.com", "https://foobar.com"},
-      {false, false, "https://foobar.com", "https://bazbar.com"},
-      {true, false, "https://foobar.com", "https-so://name.foobar.com"},
-      {true, false, "https-so://name.foobar.com", "https://foobar.com"},
-      {true, true, "https-so://name.foobar.com", "https-so://name.foobar.com"},
+      {true, "https://foobar.com", "https://foobar.com"},
+      {false, "https://foobar.com", "https://bazbar.com"},
+      {false, "https://foobar.com", "https-so://name.foobar.com"},
+      {false, "https-so://name.foobar.com", "https://foobar.com"},
+      {true, "https-so://name.foobar.com", "https-so://name.foobar.com"},
   };
 
   for (size_t i = 0; i < WTF_ARRAY_LENGTH(tests); ++i) {
@@ -353,8 +352,6 @@ TEST_F(SecurityOriginTest, CanAccess) {
     RefPtr<SecurityOrigin> origin2 =
         SecurityOrigin::createFromString(tests[i].origin2);
     EXPECT_EQ(tests[i].canAccess, origin1->canAccess(origin2.get()));
-    EXPECT_EQ(tests[i].canAccessCheckSuborigins,
-              origin1->canAccessCheckSuborigins(origin2.get()));
   }
 }