Fix loading success.html at the end of chrome://chrome-signin flow

extensions/common/api/_manifest_features.json

Index: extensions/common/api/_manifest_features.json
diff --git a/extensions/common/api/_manifest_features.json b/extensions/common/api/_manifest_features.json
index 8b10f3df75b4a0533bd4a39c1f63de0dd8789c3b..e8bd4caf151b64975b852c5e99cdfcd33f6daa04 100644
--- a/extensions/common/api/_manifest_features.json
+++ b/extensions/common/api/_manifest_features.json
@@ -332,9 +332,18 @@
       "location": "component"
     }
   ],
-  "webview": {
-    "channel": "stable",
-    "extension_types": ["platform_app"],
-    "min_manifest_version": 2
-  }
+  "webview": [

[Devlin, 2017/04/26 01:59:37]

If we accept the fact that the signin extension is special and we need to
special-case it in the permission checks already, maybe it just makes more sense
to directly check everything there?  (i.e., check the extension id + resource
path).  That way, if/when we remove it, there's only one place we need to
change.  And this isn't really buying us much if we have to go through the
effort of treating it differently elsewhere anyway.

[msarda, 2017/04/26 11:14:35]

I think the 2 things are different, so I prefer the current approach:
* the whitelist in here just allows the signin extension to load success.html in
a webview by making it accessible.
* the hack in
https://codereview.chromium.org/2831183003/diff/60001/extensions/browser/url_...
is used bacause the webview that is loading success.html does not belong to the
extension (this is my understanding at least).
Note that this approach was proposed by Lucas in CL
https://codereview.chromium.org/2838013003/

I am not an expert on extension and storage partition, so I will defer to your
decision. I would just ask you to be as explicit as possible if you prefer the
other option.

[Devlin, 2017/04/26 16:02:30]

I understand they affect different bits, but my point was more that in either
case, we need to have super-special-hacks in place for the signin extension in
the permission check, so it might make sense to only have one super-special-hack
instead of one super-special-hack and one moderately-special-hack. :)

Right now, the logic in url_request_util.cc is:

// The only exception is the sign-in extension loaded by
// chrome://chrome-signin, which is allowed to load web-accessible resources
// even if the webview that is loading the resource does not belong to the
// extension.
bool is_signin_extension =
    extension && extension->id() == "mfffpogegjflfpflabcdkioaeobkgjik";
if (owner_extension != extension && !is_signin_extension) {
  *allowed = false;
  return true;
}

*allowed = WebviewInfo::IsResourceWebviewAccessible(extension, partition_id,
resource_path);
return true;

This already requires knowledge of the signin extension and the exception it
causes.  We could alternatively do:

bool is_signin_extension =
    extension && extension->id() == "mfffpogegjflfpflabcdkioaeobkgjik";
if (is_signin_extension && resource_path == "success.html") {
  *allowed = true;
  return true;
}

if (owner_extension != extension) {
...

I don't really see it as being any less hacky than what we currently have, and
it isolates a TODO to remove these in a single place.

[Charlie Reis, 2017/04/26 20:10:25]

I like this proposal-- thanks for elaborating on what it would look like.

[msarda, 2017/04/27 12:28:45]

Done.

+    {
+      "channel": "stable",
+      "extension_types": [ "platform_app" ],
+      "min_manifest_version": 2
+    },
+    {
+      "channel": "stable",
+      "extension_types": [ "extension" ],
+      "whitelist": [
+        "mfffpogegjflfpflabcdkioaeobkgjik"
+      ]
+    }
+  ]
 }