1155491 - Support autoconfig and manual config of gmail IMAP OAuth2 authentication

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, enhancement)

Description

[Kent James (:rkent)]

+++ This bug was initially created as a clone of Bug #849540 +++

Support autoconfig and Manual Configuration of OAuth2 for GMail IMAP.

Comment 1

[Kent James (:rkent)]

Attachment: gmail.com.xml

This is the modified version of the gmail conf file, similar to https://bugzilla.mozilla.org/attachment.cgi?id=8565317 in bug 849540 but with one variable renamed. Save the file to $objdir/dist/bin/isp/gmail.com.xml after compiling to test autoconfig.

(The change here needs to be reconciled with the feedback in bug 849540.)

Comment 2

[Kent James (:rkent)]

Attachment: Remainder of patch after bug 849540 checked in

This is what remains of the patches in bug 849540 after we checked in the backend part for Thunderbird 38 beta 2. We'll continue that work in this bug.

Comment 3

[Kent James (:rkent)]

Attachment: Support manual, auto, and advanced config

See what you think. As we discussed on IRC, openId config does nothing in the current version, so I removed it, and standardized our hard-coded configuration in a single file.

Comment 4

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8594399 [details] [diff] [review]
Support manual, auto, and advanced config

Review of attachment 8594399 [details] [diff] [review]:
-----------------------------------------------------------------

I've not fully tested the patch to make sure it works, but there is one glaring question in verifyConfig.js that I think needs answering.

::: mailnews/base/prefs/content/accountcreation/emailWizard.js
@@ +796,5 @@
>            { 1 : "resultNoEncryption", 2 : "resultSSL", 3 : "resultSTARTTLS" }),
>            unknownString);
>        let certStatus = gStringsBundle.getString(server.badCert ?
>            "resultSSLCertWeak" : "resultSSLCertOK");
> +      // ToDo: we should really also display authentication method here.

Nit: TODO (some tools have a tendency to search case-sensitively, I think).

::: mailnews/base/prefs/content/accountcreation/util.js
@@ +250,5 @@
>  
> +if (typeof gEmailWizardLogger == "undefined") {
> +  Cu.import("resource:///modules/gloda/log4moz.js");
> +  var gEmailWizardLogger = Log4Moz.getConfiguredLogger("mail.wizard");
> +}

(Side note: ugh... if only JavaScript had some sort of sane module system)

::: mailnews/base/prefs/content/accountcreation/verifyConfig.js
@@ +104,1 @@
>      }

You're not calling the successCallback in case of no password being provided? Why?

@@ +105,3 @@
>    }
> +  catch (e) {
> +    gEmailWizardLogger.error("ERROR: verify logon shouldn't have failed");

I think BenB would say you need to rethrow the error here?

::: mailnews/base/util/OAuth2Providers.jsm
@@ +34,5 @@
> +var OAuth2Providers = {
> +  // lookup [issuer, scope] from hostname
> +  hostnameDetails: function (aHostname) { return kHostnames[aHostname]},
> +  // lookup [appKey, appSecret, authURI, tokenURI] from issuer
> +  issuerDetails: function (aIssuer) { return kIssuers[aIssuer]; }

I feel like these functions are better named getHostnameDetails/getIssuerDetails, respectively. Also, Doxygen-style comments would work better, although this file will almost certainly change drastically post-dynamic client registration.

Comment 5

[Kent James (:rkent)]

(In reply to Joshua Cranmer [:jcranmer] from comment #4)
> Comment on attachment 8594399 [details] [diff] [review]
> ::: mailnews/base/prefs/content/accountcreation/verifyConfig.js
> @@ +104,1 @@
> >      }
> 
> You're not calling the successCallback in case of no password being
> provided? Why?

You' right, I'll change this. We should call successCallback() then return.

> 
> @@ +105,3 @@
> >    }
> > +  catch (e) {
> > +    gEmailWizardLogger.error("ERROR: verify logon shouldn't have failed");
> 
> I think BenB would say you need to rethrow the error here?

We should not both call the errorCallback AND throw an error, otherwise the caller has no idea
whether the method processed the error or not. When you include an errorCallback, that means that
you want errors handled normally through the errorCallback rather than through throwing.

So we could throw and not user error callback, or user error callback and no throw, but not both. IMHO of course!

So what do you want to do now? Do you want an updated patch from me, or are you going to test it?

Comment 6

[Wayne Mery (:wsmwk)]

When this is done, email me what sets of steps should be manually tested and I'll add it to moztrap - to augment whatever automated tests you have

Comment 7

[Kent James (:rkent)]

Attachment: Including 1st round of jcranmer comments.

I wonder if there is some way that we could ship gmail.com.xml so that we could test automatic config? I suppose we could just modify ispdb instead, but that has some risk to it. Th existing implementation should ignore oauth2 if it does not recognize it, but still there ought to be a way to test it.

Comment 8

[Joshua Cranmer [:jcranmer]]

When testing this patch, I've confirmed that this causes test_sendMessageLater2.js to fail:
ERROR Error console says [stackFrame ReferenceError: reference to undefined property kHostnames[aHostname]]
../../../resources/logHelper.js:_errorConsoleTunnel.observe:95
resource://testing-common/mailnews/maild.js:nsMailServer.prototype.performTest:246
/src/build/trunk/mail/_tests/xpcshell/mailnews/compose/test/unit/test_sendMessageLater2.js:sendUnsentMessages:173
../../../resources/asyncTestUtils.js:async_run:107
/src/build/trunk/mail/_tests/xpcshell/mailnews/compose/test/unit/test_sendMessageLater2.js:actually_run_test:192
self-hosted:next:675
../../../resources/asyncTestUtils.js:_async_driver:156
/src/trunk/comm-central/mozilla/testing/xpcshell/head.js:do_execute_soon/<.run:653
/src/trunk/comm-central/mozilla/testing/xpcshell/head.js:_do_main:207
/src/trunk/comm-central/mozilla/testing/xpcshell/head.js:_execute_test:513
-e:null:1
null:null:0

Comment 9

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8595599 [details] [diff] [review]
Including 1st round of jcranmer comments.

Review of attachment 8595599 [details] [diff] [review]:
-----------------------------------------------------------------

This isn't a true r+, but an r+ conditional on changes to OAuth2Providers.jsm to avoid the xpcshell test failure. I personally prefer moving to a Map instead of a JS object (since Map.prototype.get returns undefined on absent entries without any strict warnings issues), and this code is logically a map anyways.

::: mailnews/base/prefs/content/accountcreation/verifyConfig.js
@@ +110,5 @@
> +  catch (e) {
> +    gEmailWizardLogger.error("ERROR: verify logon shouldn't have failed");
> +  }
> +  // Avoid pref pollution, clear out server prefs.
> +  MailServices.accounts.removeIncomingServer(inServer, true);

I've looked through ::RemoveIncomingServer a bit, and it seems like it's unlikely that this method will throw an error.

Still, it's evidence that promisifying/Task.jsm-ifying this code would be an improvement worth investigating later.

::: mailnews/base/util/OAuth2Providers.jsm
@@ +12,5 @@
> +// map of hostnames to [issuer, scope]
> +const kHostnames = {
> +  "imap.googlemail.com" : ["accounts.google.com", "http://mail.google.com/"],
> +  "smtp.googlemail.com" : ["accounts.google.com", "http://mail.google.com/"],
> +}

Given the issue of the failed test due to strict warnings, it might make more sense to have this be a Map:
const kHostnames = new Map([
  ["imap.googlemail.com", ["accounts.google.com", "http://mail.google.com/"]],
  ["smtp.googlemail.com", ["accounts.google.com", "http://mail.google.com/"]]
]);

Comment 10

[Kent James (:rkent)]

Pushed after suggested changes (using Map in OAuth2Providers):

http://hg.mozilla.org/comm-central/rev/3f1827133f9d

Comment 11

[Kent James (:rkent)]

Attachment: Package msgOAuth2Module.jsm

We missed the packaging.

Comment 12

[Kent James (:rkent)]

Comment on attachment 8596849 [details] [diff] [review]
Package msgOAuth2Module.jsm

[Triage Comment]

[Triage Comment]

Release packages fail without this.

http://hg.mozilla.org/comm-central/rev/aedb0872c0f4
http://hg.mozilla.org/releases/comm-aurora/rev/409c4c0fd0dd
http://hg.mozilla.org/releases/comm-beta/rev/5b100d15ec3a

Comment 13

[Kent James (:rkent)]

Comment on attachment 8595599 [details] [diff] [review]
Including 1st round of jcranmer comments.

http://hg.mozilla.org/releases/comm-aurora/rev/8f1e54ffa545
http://hg.mozilla.org/releases/comm-beta/rev/1da3f1730469

Comment 14

[Philip Chee]

Attachment: Package msgOAuth2Module.jsm for SeaMonkey rs=Ratty a=Ratty CLOSED TREE a=Ratty comm-aurora comm-beta

packaging fix rs=Ratty a=Ratty CLOSED TREE a=Ratty comm-aurora comm-beta

Comment 15

[Philip Chee]

Comment on attachment 8597265 [details] [diff] [review]
Package msgOAuth2Module.jsm for SeaMonkey rs=Ratty a=Ratty CLOSED TREE a=Ratty comm-aurora comm-beta

http://hg.mozilla.org/comm-central/rev/dd3c46171f55
http://hg.mozilla.org/releases/comm-aurora/rev/60320b7d5e6c
http://hg.mozilla.org/releases/comm-beta/rev/472eb0392550