1305421 - Implement chrome.identity api

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[merijn.de.jonge]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.116 Safari/537.36




Expected results:

The api is described at:

https://developer.chrome.com/extensions/identity

Types
* AccountInfo

Methods
* getAccounts − chrome.identity.getAccounts(function callback)
* getAuthToken − chrome.identity.getAuthToken(object details, function callback)
* getProfileUserInfo − chrome.identity.getProfileUserInfo(function callback)
* removeCachedAuthToken − chrome.identity.removeCachedAuthToken(object details, function callback)
* launchWebAuthFlow − chrome.identity.launchWebAuthFlow(object details, function callback)
* getRedirectURL − string chrome.identity.getRedirectURL(string path)

Events
* onSignInChanged

Comment 1

[Shane Caraveo (:mixedpuppy)]

Only launchWebAuthFlow and getRedirectURL will be implemented, the rest are specific to Google.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: WIP identity

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1305421 - Implement chrome.identity,

Review commit: https://reviewboard.mozilla.org/r/85358/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85358/

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

https://reviewboard.mozilla.org/r/85358/#review84580

I'm quite interested in this, but I think Kris would be better qualified to do the final review.
I'll just list some of the issues I see though:
- I'm concerned about using `*.addons.mozilla.org` for the redirect urls, I think those URLs should be ones that are very clearly only ever used to terminate that oauth flow and aren't actually loadable
- In the interactive case, you end up launching the flow twice (once via XHR to test if it can be completed without interaction, then again to actually display an interactive window).  At the least, this is inefficient.
- I'm not sure XmlHttpRequest is the right thing here.  These requests will be made with the extension as the origin so they will be cross-origin, without thinking it through in detail it doesn't seem like that will have the desired behavior.

I'll pass the review to Kris, he can amend my list of questions.  But beyond that, regular bug comments (or even a higher bandwidth IRC or vidyo talk) might be more efficient to get through these.

Comment 5

[Shane Caraveo (:mixedpuppy)]

(In reply to Andrew Swan [:aswan] from comment #4)
> Comment on attachment 8800458 [details]
> Bug 1305421 - Implement chrome.identity,
> 
> https://reviewboard.mozilla.org/r/85358/#review84580
> 
> I'm quite interested in this, but I think Kris would be better qualified to
> do the final review.
> I'll just list some of the issues I see though:
> - I'm concerned about using `*.addons.mozilla.org` for the redirect urls, I
> think those URLs should be ones that are very clearly only ever used to
> terminate that oauth flow and aren't actually loadable

Yeah, actually we need to decide a reasonable domain here, I just choose one and forgot to loop around about it.   Chrome uses {extension-id}.chromiumapps.org.  The redirect uri can really be anything so long as webprogressilistener can catch it in onLocationChange, it doesn't matter if it 404s or if it is a real page.  The oauth app configuration an addon uses will need to configure the url used for the redirect in its settings.  The get API here is really just a convenience API for extension authors.

I did just realize I should cancel the request when I catch that in the listener.

> - In the interactive case, you end up launching the flow twice (once via XHR
> to test if it can be completed without interaction, then again to actually
> display an interactive window).  At the least, this is inefficient.

The behavior in Chrome is they keep the window hidden until the page is loaded.  Thus if the page automatically redirects the window is never shown.  AFAIK there is no way to open a hidden window in Firefox.  The XHR simply does a HEAD request to see if a redirect happens, it is very light weight.  We could get tricky and use an iframe then swapdocs to a window after it is opened, but it also increases the complexity.

> - I'm not sure XmlHttpRequest is the right thing here.  These requests will
> be made with the extension as the origin so they will be cross-origin,
> without thinking it through in detail it doesn't seem like that will have
> the desired behavior.

oauth is cross-origin.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Also, I have a working facebook sample here (turn off tp): 

https://github.com/mixedpuppy/facebook-identity-sample

Am going to add google into the mix as a second sample.

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

https://reviewboard.mozilla.org/r/85358/#review86306

::: toolkit/components/extensions/ext-identity.js:5
(Diff revision 1)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

Please also include `Cr` for sanity's sake.

::: toolkit/components/extensions/ext-identity.js:13
(Diff revision 1)
> +                                  "resource://gre/modules/Services.jsm");
> +Cu.importGlobalProperties(["URL", "XMLHttpRequest"]);
> +
> +function checkRedirected(details, redirectURI) {
> +  return new Promise((resolve, reject) => {
> +    let xhr = new XMLHttpRequest();

Can we use `fetch` for this instead?

::: toolkit/components/extensions/ext-identity.js:14
(Diff revision 1)
> +Cu.importGlobalProperties(["URL", "XMLHttpRequest"]);
> +
> +function checkRedirected(details, redirectURI) {
> +  return new Promise((resolve, reject) => {
> +    let xhr = new XMLHttpRequest();
> +    xhr.open("HEAD", details.url);

XHR converts `HEAD` requests to `GET` when it sees a redirect, which requires special-purpose `notificationCallbacks` to avoid. `fetch` has `redirect: "manual"` and `redirect: "error"` which should make that much easier to handle.

::: toolkit/components/extensions/ext-identity.js:19
(Diff revision 1)
> +      if (xhr.status === 0 && xhr.responseURL.startsWith(redirectURI))
> +        resolve(xhr.responseURL);
> +      else
> +        reject(xhr.status + " " + xhr.responseText);

Braces please.

::: toolkit/components/extensions/ext-identity.js:28
(Diff revision 1)
> +    };
> +    xhr.send();
> +  });
> +}
> +
> +function openOAuthWindow(details, redirectURI) {

Does this really need to be a window? It seems like a tab in an existing window would be preferable, when possible...

::: toolkit/components/extensions/ext-identity.js:29
(Diff revision 1)
> +  let windowArguments = Cc["@mozilla.org/supports-array;1"]
> +                          .createInstance(Ci.nsISupportsArray);
> +  let supportsStringPrefURL = Cc["@mozilla.org/supports-string;1"]
> +                                .createInstance(Ci.nsISupportsString);
> +  supportsStringPrefURL.data = details.url;
> +  windowArguments.AppendElement(supportsStringPrefURL);

This is going to need to switch to `nsIMutableArray` thanks to bug 1311223.

::: toolkit/components/extensions/ext-identity.js:39
(Diff revision 1)
> +  windowArguments.AppendElement(supportsStringPrefURL);
> +
> +  let window = Services.ww.openWindow(null,
> +                                      Services.prefs.getCharPref("browser.chromeURL"),
> +                                      "launchWebAuthFlow_dialog",
> +                                      "location=yes,centerscreen,dialog=no,resizable=yes",

"chrome,..."

::: toolkit/components/extensions/ext-identity.js:44
(Diff revision 1)
> +                                      "location=yes,centerscreen,dialog=no,resizable=yes",
> +                                      windowArguments);
> +
> +  return new Promise((resolve, reject) => {
> +    let wpl = {
> +      onLocationChange(aBrowser, aWebProgress, aRequest, aLocationURI) {

No aArgs, please.

::: toolkit/components/extensions/ext-identity.js:64
(Diff revision 1)
> +      window.removeEventListener("unload", unloadlistener);
> +      window.gBrowser.removeTabsProgressListener(wpl);
> +      reject();
> +    }
> +
> +    window.addEventListener("load", function listener() {

There's `promiseDocumentLoaded` and `promiseEvent` in `ExtensionUtils` that should handle this.

::: toolkit/components/extensions/ext-identity.js:85
(Diff revision 1)
> +          return Promise.reject({ message: "redirect_uri is missing" });
> +        }
> +
> +        // If the request is automatically redirected the user has already
> +        // authorized and we do not want to show the window.
> +        return new Promise((resolve, reject) => {

No need for `new Promise`. We already have a promise chain.

::: toolkit/components/extensions/ext-identity.js:93
(Diff revision 1)
> +                        reject({ message: "user cancelled or denied access" });
> +                      });
> +            } else {
> +              reject({ message: "requires user interaction " + error });
> +            }

Please capitalize error messages.

::: toolkit/components/extensions/ext-identity.js:96
(Diff revision 1)
> +                .then(location => { resolve(location); },
> +                      () => {
> +                        reject({ message: "user cancelled or denied access" });
> +                      });
> +            } else {
> +              reject({ message: "requires user interaction " + error });

Template string, and colon before the upstream error message, please.

Also, we need to be careful not to convert chrome exceptions into extension-visible error messages here.

::: toolkit/components/extensions/ext-identity.js:101
(Diff revision 1)
> +              reject({ message: "requires user interaction " + error });
> +            }
> +          });
> +        });
> +      },
> +      getRedirectURL : function(path) {

Add blank line.

::: toolkit/components/extensions/ext-identity.js:102
(Diff revision 1)
> +        // The redirect URL needs to be persistent since it is given to the
> +        // OAuth provider.
> +        // Temporary addons may have a generated id which does not help with
> +        // having a persistent redirect url.  We'll split that off and just
> +        // use temporary-addon.addons.mozilla.org.

This doesn't seem safe. It means that temporary add-ons will share redirect URLs, and be susceptable to all kinds of interference and security issues.

The IDs that we generate are persistent as long as the file stays in the same place, and add-ons that the generated IDs cause trouble for can always specify their own IDs. So I don't think we should give this case any kind of special treatment.

::: toolkit/components/extensions/ext-identity.js:108
(Diff revision 1)
> +        // OAuth provider.
> +        // Temporary addons may have a generated id which does not help with
> +        // having a persistent redirect url.  We'll split that off and just
> +        // use temporary-addon.addons.mozilla.org.
> +        let id;
> +        if (extension.id.indexOf("temporary-addon") > -1)

`extension.id.includes(...)`, but this isn't a safe test. If we need to do this, we should actually check that the extension is temporary.

::: toolkit/components/extensions/ext-identity.js:108
(Diff revision 1)
> +        if (extension.id.indexOf("temporary-addon") > -1)
> +          id = "temporary-addon";
> +        else
> +          id = extension.id.replace("@", ".");

Braces, please.

::: toolkit/components/extensions/ext-identity.js:111
(Diff revision 1)
> +        // use temporary-addon.addons.mozilla.org.
> +        let id;
> +        if (extension.id.indexOf("temporary-addon") > -1)
> +          id = "temporary-addon";
> +        else
> +          id = extension.id.replace("@", ".");

This is going to allow for clashes. We should probably just hash the ID, since there's no particular need for it to be readable.

::: toolkit/components/extensions/ext-identity.js:112
(Diff revision 1)
> +        let id;
> +        if (extension.id.indexOf("temporary-addon") > -1)
> +          id = "temporary-addon";
> +        else
> +          id = extension.id.replace("@", ".");
> +        return "https://" + id + ".addons.mozilla.org" + (path || "");

We should use a specialized domain for this. `addons.mozilla.org` has all kinds of special privileges, and giving add-ons any kind of special access to it seems like asking for trouble.

Also, template strings, please.

::: toolkit/components/extensions/test/mochitest/chrome.ini:30
(Diff revision 1)
> +support-files =
> +  redirect_auto.sjs
> +  oauth.html

Please list all support files in the DEFAULT section.

::: toolkit/components/extensions/test/mochitest/redirect_auto.sjs:5
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +function handleRequest(aRequest, aResponse) {

No aArgs, please.

::: toolkit/components/extensions/test/mochitest/redirect_auto.sjs:6
(Diff revision 1)
> + aResponse.setStatusLine(aRequest.httpVersion, 302, "Moved Temporarily");
> + var match = /redirect_uri=([^&]*)/.exec(aRequest.queryString);
> + var redirect_uri = decodeURIComponent(match[1]);
> + aResponse.setHeader("Location", redirect_uri);

Weird indentation.

::: toolkit/components/extensions/test/mochitest/redirect_auto.sjs:7
(Diff revision 1)
> + var match = /redirect_uri=([^&]*)/.exec(aRequest.queryString);
> + var redirect_uri = decodeURIComponent(match[1]);

`new URLSearchParams(request.queryString).get("redirect_uri");`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_identity.html:35
(Diff revision 1)
> +  yield extension.unload();
> +});
> +
> +function background_launchWebAuthFlow(interactive, path) {
> +  let base_uri = "https://example.com/chrome/toolkit/components/extensions/test/mochitest/";
> +  let redirect_uri = chrome.identity.getRedirectURL("/identity_cb");

s/chrome/browser/g

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_identity.html:37
(Diff revision 1)
> +
> +function background_launchWebAuthFlow(interactive, path) {
> +  let base_uri = "https://example.com/chrome/toolkit/components/extensions/test/mochitest/";
> +  let redirect_uri = chrome.identity.getRedirectURL("/identity_cb");
> +  let options = {
> +    interactive: interactive,

Nit: `    interactive,`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_identity.html:38
(Diff revision 1)
> +function background_launchWebAuthFlow(interactive, path) {
> +  let base_uri = "https://example.com/chrome/toolkit/components/extensions/test/mochitest/";
> +  let redirect_uri = chrome.identity.getRedirectURL("/identity_cb");
> +  let options = {
> +    interactive: interactive,
> +    url: base_uri + path + "?redirect_uri=" + encodeURIComponent(redirect_uri)

Nits: Template string, trailing comma.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_identity.html:40
(Diff revision 1)
> +  let redirect_uri = chrome.identity.getRedirectURL("/identity_cb");
> +  let options = {
> +    interactive: interactive,
> +    url: base_uri + path + "?redirect_uri=" + encodeURIComponent(redirect_uri)
> +  };
> +  chrome.identity.launchWebAuthFlow(options, function(redirectURL) {

Please use Promise-based API.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_identity.html:48
(Diff revision 1)
> +// Tests the situation where the oauth provider has already granted access and
> +// simply redirects the oauth client to provide the access key or code.
> +add_task(function* test_autoRedirect() {

We should also test with `interactive: false` where the provider does not redirect.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

https://reviewboard.mozilla.org/r/85358/#review86306

> Does this really need to be a window? It seems like a tab in an existing window would be preferable, when possible...

I'm inclined to say yes, it should be a window.  Primarily because if there is any current or future api that would allow an addon to initiate oauth in a non-browser window[1], it would be fairly awkward to switch windows to do the oauth, then switch back.  A dialog avoids that particular issue.

[1]e.g. a button in the browser toolbox that connects the user to some external dev tool, perhaps a bug tracker, or something.

> There's `promiseDocumentLoaded` and `promiseEvent` in `ExtensionUtils` that should handle this.

promiseEvent is not in ExtensionUtils, promiseDocumentLoaded is

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85358/diff/1-2/

Comment 10

[Shane Caraveo (:mixedpuppy)]

@kmag I'm open to moving to fetch, but it is having issues following through with redirects, or it is doing something otherwise different in requests.  I left those issues open on reviewboard.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

https://reviewboard.mozilla.org/r/85358/#review86306

> I'm inclined to say yes, it should be a window.  Primarily because if there is any current or future api that would allow an addon to initiate oauth in a non-browser window[1], it would be fairly awkward to switch windows to do the oauth, then switch back.  A dialog avoids that particular issue.
> 
> [1]e.g. a button in the browser toolbox that connects the user to some external dev tool, perhaps a bug tracker, or something.

If a non-browser window is focused, I'd be OK with either opening a special-purpose popup or focusing the last browser window. When a browser window is focused, though, opening a tab seems like the obviously correct behavior.

> promiseEvent is not in ExtensionUtils, promiseDocumentLoaded is

The patch that added it got backed out.

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

https://reviewboard.mozilla.org/r/85358/#review86744

::: toolkit/components/extensions/ext-identity.js:38
(Diff revisions 1 - 2)
> -  let windowArguments = Cc["@mozilla.org/supports-array;1"]
> +  let args = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
> -                          .createInstance(Ci.nsISupportsArray);
>    let supportsStringPrefURL = Cc["@mozilla.org/supports-string;1"]
>                                  .createInstance(Ci.nsISupportsString);
>    supportsStringPrefURL.data = details.url;
> -  windowArguments.AppendElement(supportsStringPrefURL);
> +  args.appendElement(supportsStringPrefURL, /*weak =*/ false);

Nit: ESLint requires at least one space inside comment markers.

::: toolkit/components/extensions/ext-identity.js:65
(Diff revisions 1 - 2)
>  
>      // If the user just closes the window we need to reject
>      function unloadlistener() {
>        window.removeEventListener("unload", unloadlistener);
>        window.gBrowser.removeTabsProgressListener(wpl);
> -      reject();
> +      reject({ message: "User cancelled or denied access." });

Nit: No whitespace inside braces. You might want to enable your editor's ESLint integration: https://wiki.mozilla.org/WebExtensions/Hacking#Integrating_ESLint_with_your_editor

::: toolkit/components/extensions/ext-identity.js:88
(Diff revisions 1 - 2)
> -          checkRedirected(details, redirectURI).then(location => {
> -            resolve(location);
> +          if (!details.interactive)
> +            return Promise.reject({ message: `Requires user interaction ${requestError}` });

Nit: Add braces, remove whitespace inside braces.

::: toolkit/components/extensions/ext-identity.js:89
(Diff revisions 1 - 2)
>  
>          // If the request is automatically redirected the user has already
>          // authorized and we do not want to show the window.
> -        return new Promise((resolve, reject) => {
> -          checkRedirected(details, redirectURI).then(location => {
> -            resolve(location);
> +        return checkRedirected(details, redirectURI).catch((requestError) => {
> +          if (!details.interactive)
> +            return Promise.reject({ message: `Requires user interaction ${requestError}` });

This turns into a pretty arcane error message when the status code is appended to the base message... And it still exposes stringified chrome exceptions to extension code if something accidentally throws.

::: toolkit/components/extensions/ext-identity.js:95
(Diff revisions 1 - 2)
> -            }
> -          });
>          });
>        },
> -      getRedirectURL : function(path) {
> -        // The redirect URL needs to be persistent since it is given to the
> +
> +      getRedirectURL : function(path = "") {

Nit: No space before ':'

::: toolkit/components/extensions/ext-identity.js:96
(Diff revisions 1 - 2)
> -          });
>          });
>        },
> -      getRedirectURL : function(path) {
> -        // The redirect URL needs to be persistent since it is given to the
> -        // OAuth provider.
> +
> +      getRedirectURL : function(path = "") {
> +        return `https://${extension.id.replace("@", ".")}${path}`;

This still allows for extension IDs to clash. E.g., "[e-mail]" and "foo.bar@com".

We should still use sub-domains of a common domain, just not under `mozilla.org`.

Also, we should add a slash after the hostname, and I guess strip any leading slashes off the provided `path`.

::: toolkit/components/extensions/test/mochitest/redirect_auto.sjs:6
(Diff revisions 1 - 2)
>  /* Any copyright is dedicated to the Public Domain.
>   * http://creativecommons.org/publicdomain/zero/1.0/
>   */
>  
> -function handleRequest(aRequest, aResponse) {
> - aResponse.setStatusLine(aRequest.httpVersion, 302, "Moved Temporarily");
> +function handleRequest(request, response) {
> +  Components.utils.importGlobalProperties(["URLSearchParams"]);

This usually goes at the top-level.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_identity.html:19
(Diff revisions 1 - 2)
>  <script type="text/javascript">
>  "use strict";
>  
>  add_task(function* test_noPermission() {
>    function background() {
> -    browser.test.assertEq(chrome.identity, undefined, "no identity api without permission");
> +    browser.test.assertEq(browser.identity, undefined, "no identity api without permission");

Nit: The expected value should be the first argument in `assertEq` calls, and the assertion message should be capitalized.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_identity.html:37
(Diff revisions 1 - 2)
> -    interactive: interactive,
> -    url: base_uri + path + "?redirect_uri=" + encodeURIComponent(redirect_uri)
> +  if (!redirect)
> +    url = `${url}&no_redirect=1`;

Nit: Add braces.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_identity.html:40
(Diff revisions 1 - 2)
> -  let redirect_uri = chrome.identity.getRedirectURL("/identity_cb");
> -  let options = {
> -    interactive: interactive,
> -    url: base_uri + path + "?redirect_uri=" + encodeURIComponent(redirect_uri)
> -  };
> -  chrome.identity.launchWebAuthFlow(options, function(redirectURL) {
> +  let redirect_uri = browser.identity.getRedirectURL("/identity_cb");
> +  let url = `${base_uri}${path}?redirect_uri=${encodeURIComponent(redirect_uri)}`;
> +  if (!redirect)
> +    url = `${url}&no_redirect=1`;
> +
> +  browser.identity.launchWebAuthFlow({ interactive, url }).then((redirectURL) => {

Nit: Remove space inside braces.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_identity.html:44
(Diff revisions 1 - 2)
> +    if (redirect)
> +      browser.test.notifyFail(error.message);
> +    else
> +      browser.test.assertEq("Requires user interaction 200", error.message);

Nit: Braces.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_identity.html:45
(Diff revisions 1 - 2)
> -      browser.test.notifyFail(chrome.runtime.lastError.message);
>      browser.test.assertEq(redirect_uri, redirectURL, "handled auto redirection");
>      browser.test.sendMessage("done");
> +  }).catch((error) => {
> +    if (redirect)
> +      browser.test.notifyFail(error.message);

Should be `browser.test.fail`. `notifyFail` is for cases when the outer test is waiting on `awaitFinish`.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_identity.html:47
(Diff revisions 1 - 2)
>      browser.test.sendMessage("done");
> +  }).catch((error) => {
> +    if (redirect)
> +      browser.test.notifyFail(error.message);
> +    else
> +      browser.test.assertEq("Requires user interaction 200", error.message);

As noted above, this message is pretty arcane. Also, please include an assertion message for all assertions.

Comment 13

[Kris Maglione [:kmag]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> @kmag I'm open to moving to fetch, but it is having issues following through
> with redirects, or it is doing something otherwise different in requests.  I
> left those issues open on reviewboard.

We should probably be handling the redirects manually, anyway. The current XHR code has its own set of problems, anyway. Request chains are converted to GET after the first request, and the detection of the final redirect URL only works because the final request fails (which, incidentally, isn't guaranteed to happen, especially with the current ID mangling, and definitely not with ISP DNS servers that are designed to intercept mistyped URLs).

Comment 14

[Shane Caraveo (:mixedpuppy)]

(In reply to Kris Maglione [:kmag] from comment #13)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> > @kmag I'm open to moving to fetch, but it is having issues following through
> > with redirects, or it is doing something otherwise different in requests.  I
> > left those issues open on reviewboard.
> 
> We should probably be handling the redirects manually, anyway. The current
> XHR code has its own set of problems, anyway. Request chains are converted
> to GET after the first request, and the detection of the final redirect URL
> only works because the final request fails (which, incidentally, isn't
> guaranteed to happen, especially with the current ID mangling, and
> definitely not with ISP DNS servers that are designed to intercept mistyped
> URLs).


I still don't see any way to do this with fetch.  With redirect:manual, I get an opaqueredirect response that does not contain url that is being redirected to (headers are also not available).  With redirect:follow, it "works" but results in a NetError and the url redirected to is not available.  

With XHR, the redirect is still a HEAD request, at least as shown by the browser console.  I can also use onreadystatechange to detect the responseURL has changed and abort the request, so we never hit any chance of a dns intercept.

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85358/diff/2-3/

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

https://reviewboard.mozilla.org/r/85358/#review86744

> This still allows for extension IDs to clash. E.g., "[e-mail]" and "foo.bar@com".
> 
> We should still use sub-domains of a common domain, just not under `mozilla.org`.
> 
> Also, we should add a slash after the hostname, and I guess strip any leading slashes off the provided `path`.

I've fixed this issue, but we'll still need to choose a proper domain.  For now, addons.firefox.com can be a placeholder to finish review/testing.

Comment 17

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

https://reviewboard.mozilla.org/r/85358/#review86306

> XHR converts `HEAD` requests to `GET` when it sees a redirect, which requires special-purpose `notificationCallbacks` to avoid. `fetch` has `redirect: "manual"` and `redirect: "error"` which should make that much easier to handle.

taking this issue back to the bug

Comment 18

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85358/diff/3-4/

Comment 19

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

https://reviewboard.mozilla.org/r/85358/#review86306

> If a non-browser window is focused, I'd be OK with either opening a special-purpose popup or focusing the last browser window. When a browser window is focused, though, opening a tab seems like the obviously correct behavior.

I'd like to move this to a followup and get ux involved.  That way we can land the new api and adjust ui later.

Comment 20

[Shane Caraveo (:mixedpuppy)]

Kris, how should we go about deciding on a base domain for this?

Comment 21

[Kris Maglione [:kmag]]

It should probably be a new second-level domain, which we'd have to register. Maybe something like "mozilla-extensions.com".

Andy is probably the person to deal with that.

Comment 22

[Andy McKay]

(In reply to Kris Maglione [:kmag] from comment #21)
> It should probably be a new second-level domain, which we'd have to
> register. Maybe something like "mozilla-extensions.com".
> 
> Andy is probably the person to deal with that.

How about something like *.extensions.allizom.org. It's off the top level mozilla.com and .org for security reasons. We maintain the allizom.org domains for this sort of thing and this should be easy for IT to do (i.e. they shouldn't have to do anything).

A user should never see or interact with this domain, but as Shane pointed out there might be the odd edge case where something goes wrong and they might hit something. It would be nice if that did happen if it was something in our control.

It's worth nothing that the chromiumapps.org is not registered and someone could take that over.

Comment 23

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85358/diff/4-5/

Comment 24

[Kris Maglione [:kmag]]

(In reply to Andy McKay [:andym] from comment #22)
> How about something like *.extensions.allizom.org. It's off the top level
> mozilla.com and .org for security reasons. We maintain the allizom.org
> domains for this sort of thing and this should be easy for IT to do (i.e.
> they shouldn't have to do anything).

Yeah, I guess that's fine for now. We probably shouldn't advertise it as stable, though.

> It's worth nothing that the chromiumapps.org is not registered and someone
> could take that over.

Ick.

Comment 25

[Kris Maglione [:kmag]]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

https://reviewboard.mozilla.org/r/85358/#review92060

For the most part, this looks good, but it still has most of the issues I commented on in previous reviews.

::: toolkit/components/extensions/ext-c-identity.js:28
(Diff revision 5)
> +    };
> +    // An unexpected error happened, log for extension authors.
> +    xhr.onerror = () => {
> +      reject(xhr.status);
> +    };
> +    // Catch redirect to our redirect_uri before a new request is made.

Sorry, but this still doesn't do what the comment claims. It currently only works because the final URL is unresolvable. `onreadystatechange` is oblivious to redirects. We need to use actual `notificationCallbacks` on the channel.

For examples, see:

http://searchfox.org/mozilla-central/rev/846adaea6ccd1428780ed47d0d94da18478acdbb/toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm#584
http://searchfox.org/mozilla-central/rev/846adaea6ccd1428780ed47d0d94da18478acdbb/toolkit/modules/CertUtils.jsm#194

::: toolkit/components/extensions/ext-c-identity.js:89
(Diff revision 5)
> +  let {extension} = context;
> +  return {
> +    identity: {
> +      launchWebAuthFlow: function(details) {
> +        // In OAuth2 the url should have a redirect_uri param, parse the url and grab it
> +        let redirectURI = new URL(details.url).searchParams.get("redirect_uri");

I know I didn't complain about this before... but we should probably check that this is a sane URL, and probably that it's for the expected domain.

::: toolkit/components/extensions/ext-c-identity.js:111
(Diff revision 5)
> +          return openOAuthWindow(details, redirectURI);
> +        });
> +      },
> +
> +      getRedirectURL: function(path = "") {
> +        let redirectDomain = Services.prefs.getCharPref("webextensions.identity.redirectDomain");

I think "extensions.webextensions." is our most common pref branch at this point. In any case, please use `XPCOMUtils.defineLazyPreferenceGetter` to cache this.

::: toolkit/components/extensions/ext-c-identity.js:112
(Diff revision 5)
> +        });
> +      },
> +
> +      getRedirectURL: function(path = "") {
> +        let redirectDomain = Services.prefs.getCharPref("webextensions.identity.redirectDomain");
> +        let url = new URL(`https://${extension.id.replace("@", "-")}.${redirectDomain}/`);

This still allows for clashes between IDs based on the location of the `@`. We really need something deterministic, and the easiest thing would be to just use a hash.

Also, UUID IDs have `{` and `}` in them, which aren't valid in domains.

::: toolkit/components/extensions/test/mochitest/oauth.html:8
(Diff revision 5)
> +<head>
> +  <script>
> +  "use strict";
> +
> +  var url = new URL(location);
> +  location.href = decodeURIComponent(url.searchParams.get("redirect_uri"));

The `decodeURIComponent` should not be necessary. The `URI` code should take care of that.

Also, maybe add a query param just to make sure that works sanely?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_identity.html:25
(Diff revision 5)
> +    browser.test.sendMessage("done");
> +  }
> +
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {},
> +    background: `(${background})()`,

No need for the template string. Just `background,` will do. Or, better, just put the background function inline as a method: `background() { ... }`

Comment 26

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85358/diff/5-6/

Comment 27

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

https://reviewboard.mozilla.org/r/85358/#review92060

> I know I didn't complain about this before... but we should probably check that this is a sane URL, and probably that it's for the expected domain.

There's nothing that requires the extension to use our helper to get a redirect url or the suggested redirect url, and there may be reason to use something else.  I did add some validation on the urls.

Comment 28

[Kris Maglione [:kmag]]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

https://reviewboard.mozilla.org/r/85358/#review92552

Looks good. Just a few nits at this point. Thanks!

::: toolkit/components/extensions/ext-c-identity.js:12
(Diff revision 6)
> +XPCOMUtils.defineLazyGetter(this, "CryptoHash", function() {
> +  return CC("@mozilla.org/security/hash;1", "nsICryptoHash", "initWithString");
> +});

No need for a lazy getter here. `Components.Constructor` is relatively cheap, and it doesn't actually instantiate any XPCOM components.

::: toolkit/components/extensions/ext-c-identity.js:15
(Diff revision 6)
> +XPCOMUtils.defineLazyGetter(this, "UnicodeConverter", function() {
> +  let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
> +                    .createInstance(Ci.nsIScriptableUnicodeConverter);
> +  converter.charset = "utf8";
> +  return converter;
> +});

Please don't use nsIScriptableUnicode converter. Just import `TextEncoder` and then use `new TextEncoder().encode()`. You can pass the resulting typed array directly to the nsICryptoHash instance.

::: toolkit/components/extensions/ext-c-identity.js:29
(Diff revision 6)
> +const {
> +  promiseDocumentLoaded,
> +} = ExtensionUtils;
> +
> +function computeHash(str) {
> +  let byteArr = UnicodeConverter.convertToByteArray(str);

Per above: Please use `TextEncoder` rather than `nsIScriptableUnicodeConverter`.

::: toolkit/components/extensions/ext-c-identity.js:37
(Diff revision 6)
> +  return hash.finish(true);
> +}
> +
> +function checkRedirected(url, redirectURI) {
> +  return new Promise((resolve, reject) => {
> +    let xhr = new XMLHttpRequest();

It occurs to me that we should probably take the caller's private browsing context or user context into account here... But that's probably better left to a follow-up bug. Can you file one?

::: toolkit/components/extensions/ext-c-identity.js:49
(Diff revision 6)
> +    xhr.onerror = () => {
> +      reject(xhr.status);
> +    };
> +    // Catch redirect to our redirect_uri before a new request is made.
> +    xhr.channel.notificationCallbacks = {
> +      QueryInterface(iid) {

`QueryInterface: XPCOMUtils.generateQI([Ci.nsIInterfaceRequestor, Ci.nsIChannelEventSync])`

::: toolkit/components/extensions/ext-c-identity.js:58
(Diff revision 6)
> +      getInterface(iid) {
> +        if (iid.equals(Ci.nsIChannelEventSink)) {
> +          return this;
> +        }
> +        throw Cr.NS_ERROR_NO_INTERFACE;
> +      },

`return this.QueryInterface(iid)`

::: toolkit/components/extensions/ext-c-identity.js:70
(Diff revision 6)
> +        // Cancel the redirect.
> +        callback.onRedirectVerifyCallback(Components.results.NS_BINDING_ABORTED);

We should only cancel the redirect if it's a redirect to our response URL, I think, in case it shows up later in the redirect chain.

::: toolkit/components/extensions/ext-c-identity.js:135
(Diff revision 6)
> +          url = new URL(details.url);
> +        } catch (e) {
> +          return Promise.reject({message: "details.url is invalid"});
> +        }
> +        try {
> +          redirectURI = new URL(url.searchParams.get("redirect_uri"));

I still think we should at least warn if this doesn't start with the expected prefix.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_identity.html:19
(Diff revision 6)
> +<script type="text/javascript">
> +"use strict";
> +
> +add_task(function* test_noPermission() {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {},

Nit: Not necessary.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_identity.html:40
(Diff revision 6)
> +        "identity",
> +        "https://example.com/",
> +      ],
> +    },
> +    background() {
> +      browser.identity.launchWebAuthFlow({interactive: true, url: "foobar"}).then(() => {

Nit: Please use `browser.test.assertRejects`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_identity.html:67
(Diff revision 6)
> +      ],
> +    },
> +    background() {
> +      let base_uri = "https://example.com/chrome/toolkit/components/extensions/test/mochitest/";
> +      let url = `${base_uri}?redirect_uri=badrobot}`;
> +      browser.identity.launchWebAuthFlow({interactive: true, url}).then(() => {

`assertRejects`

Comment 29

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85358/diff/6-7/

Comment 30

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

https://reviewboard.mozilla.org/r/85358/#review92552

> `return this.QueryInterface(iid)`

For some reason, our callback is not called with this change, so I'm dropping this change.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7054d08b878
Implement chrome.identity, r=kmag

Comment 32

[Kris Maglione [:kmag]]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

https://reviewboard.mozilla.org/r/85358/#review92592

::: toolkit/components/extensions/ext-c-identity.js:25
(Diff revision 7)
> +
> +function computeHash(str) {
> +  let byteArr = new TextEncoder().encode(str);
> +  let hash = new CryptoHash("sha1");
> +  hash.update(byteArr, byteArr.length);
> +  return hash.finish(true);

Hm. Sorry, this only occurred to me as an afterthought, but this is probably going to cause problems. This returns a base64 string, which will generally contain characters that aren't valid in a domain name. So we need to convert it to hex, instead. There's a utility for this that we already use in storage.sync, so it shouldn't be a big change:

https://reviewboard-hg.mozilla.org/gecko/file/196d52ea899c/toolkit/components/extensions/ExtensionStorageSync.jsm#l338

::: toolkit/components/extensions/ext-c-identity.js:56
(Diff revision 7)
> +      asyncOnChannelRedirect(oldChannel, newChannel, flags, callback) {
> +        let responseURL = newChannel.URI.spec;
> +        if (responseURL.startsWith(redirectURI)) {
> +          resolve(responseURL);
> +          // Cancel the redirect.
> +          callback.onRedirectVerifyCallback(Components.results.NS_BINDING_ABORTED);

We need to call the callback in the else branch, too, just with `NS_OK` rather than `NS_BINDING_ABORTED`, otherwise the request will just pause indefinitely.

Comment 33

[Kris Maglione [:kmag]]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

https://reviewboard.mozilla.org/r/85358/#review92552

> For some reason, our callback is not called with this change, so I'm dropping this change.

That's interesting. I'd be curious to know why. But the other option, which should work just as well, is to go with `getInterface: XPCOMUtils.generateQI([Ci.nsIChannelEventSink])`

Comment 34

[Iris Hsiao [:ihsiao]]

Sorry had to back out for test_chrome_ext_identity.html timed out, i.e., https://treeherder.mozilla.org/logviewer.html#?job_id=6558509&repo=autoland#L3784

Comment 35

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9dc1fd87542c
Backed out changeset c7054d08b878 for test_chrome_ext_identity.html Timed out

Comment 36

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8800458 [details]
Bug 1305421 - Implement chrome.identity,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85358/diff/7-8/

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee65eebbbef9
Implement chrome.identity, r=kmag

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ee65eebbbef9

Comment 39

[Shane Caraveo (:mixedpuppy)]

Notes for dev-doc:

I've updated my test sample at https://github.com/mixedpuppy/facebook-identity-sample/ which now has both facebook and google authorization.  The part of that which is pertinent to the identity documentation is the authorize function in background.js.  The rest of the project is just stuff necessary to make a fully working example.

Comment 40

[Andy McKay]

Maybe we could move that sample over to https://github.com/mdn/webextensions-examples.

Comment 41

[Sébastien Règne]

When you call getRedirectURL with no parameter, the method adds null : "https://a5917d103ab137a38a3e67f6c3c5435961044755.extensions.allizom.org/null"
In Chrome, there is no suffix : "https://jjllokfabehlkafdlglkjeafncpbhahg.chromiumapp.org/"

Comment 42

[merijn.de.jonge]

The launchWebAuthFlow method from the Identity API opens a popup that executes the authentication flow. However, when this popup is opened, the main extension's popup window is closed because it losses focus. This is undesired behavior (and also not in line with the behavior of the implementation of Chrome (and Opera)), because during authentication the main extension's popup should be visible and it should get the focus back when the authentication flow is completed. With the current implementation, a user cannot continue after he has logged in because the main popup window disappeared.

See also bug 1292701.

Comment 43

[Kris Maglione [:kmag]]

That is a separate bug, which we haven't decided how to handle yet. As a workaround, I suggest that you launch the auth flow from your background page or from a tab.

Comment 44

[Will Bamberg [:wbamberg]]

I've written some documentation here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/identity

Please let me know if it's OK.

Comment 45

[Shane Caraveo (:mixedpuppy)]

(In reply to Will Bamberg [:wbamberg] from comment #44)
> I've written some documentation here:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/identity
> 
> Please let me know if it's OK.

Looks good.

The mention of setting the applications id is easy to miss (could just be me, the way I scan when I read), any chance that can be highlighted somehow?

The "edge incompatibilities" section is a little weird since edge doesn't support the api.

Comment 46

[Will Bamberg [:wbamberg]]

Thanks Shane.

(In reply to Shane Caraveo (:mixedpuppy) from comment #45)
> (In reply to Will Bamberg [:wbamberg] from comment #44)
> 
> The mention of setting the applications id is easy to miss (could just be
> me, the way I scan when I read), any chance that can be highlighted somehow?

I'm not really keen on artificially highlighting particular bits of information. I think it adds noise, and it's subjective what's important and what isn't. This is mentioned in 3 places (the overview page, the page for getRedirectURL itself and the page on the add-on ID (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/WebExtensions_and_the_Add-on_ID) so I think it should be OK.

> The "edge incompatibilities" section is a little weird since edge doesn't
> support the api.

Good catch, thank you. I've removed this.

Comment 47

[Rémy Hubscher (:natim)]

> it should be a window.  Primarily because if there is any current or future api that would allow an addon to initiate oauth in a non-browser window[1], it would be fairly awkward to switch windows to do the oauth, then switch back.

It is exactly what currently happens with the current implementation of the Firefox Notes sidebar add-on.

It opens a new window, then it send an email where you need to click on a link to validate the login flow.
Then the click open a new tab with the sidebar on the side.

So you get a sidebar, a tab and a popup. You have to close the tab manually and then to wait for the popup to close itself eventually. It is really confusing.

For a sidebar add-on I wish we could start the login flow in a tab next to the sidebar which will make it much more intuitive IMO

Shane do you think you could have a look at the flow in https://github.com/Natim/notes and give me some advice of how we could improve things?

Comment 48

[Rémy Hubscher (:natim)]

Moving this discussion in a new issue: https://github.com/Natim/notes/issues/3
Sorry for the noise;

Comment 49

[ra1991k]

Are there any plans to implement this on android?