Closed Bug 1155491 Opened 9 years ago Closed 9 years ago

Support autoconfig and manual config of gmail IMAP OAuth2 authentication

Categories

(MailNews Core :: Account Manager, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird38+ fixed, thunderbird39 fixed, thunderbird40 fixed)

RESOLVED FIXED
Thunderbird 40.0
Tracking Status
thunderbird38 + fixed
thunderbird39 --- fixed
thunderbird40 --- fixed

People

(Reporter: rkent, Assigned: rkent)

References

Details

(Keywords: feature)

Attachments

(4 files, 2 obsolete files)

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

Support autoconfig and Manual Configuration of OAuth2 for GMail IMAP.
Summary: Log in to Gmail (IMAP/SMTP) using OAuth → Support autoconfig and manual config of gmail IMAP OAuth2 authentication
Attached file 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.)
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.
Assignee: nobody → rkent
Status: NEW → ASSIGNED
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.
Attachment #8593748 - Attachment is obsolete: true
Attachment #8594399 - Flags: review?(Pidgeot18)
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.
(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?
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
Flags: in-testsuite?
Flags: in-moztrap?
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.
Attachment #8594399 - Attachment is obsolete: true
Attachment #8594399 - Flags: review?(Pidgeot18)
Attachment #8595599 - Flags: review?(Pidgeot18)
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 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/"]]
]);
Attachment #8595599 - Flags: review?(Pidgeot18) → review+
Pushed after suggested changes (using Map in OAuth2Providers):

http://hg.mozilla.org/comm-central/rev/3f1827133f9d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite? → in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
Attachment #8595599 - Flags: approval-comm-beta?
Attachment #8595599 - Flags: approval-comm-aurora?
We missed the packaging.
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
Attachment #8596849 - Flags: approval-comm-beta+
Attachment #8596849 - Flags: approval-comm-aurora+
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
Attachment #8595599 - Flags: approval-comm-beta?
Attachment #8595599 - Flags: approval-comm-beta+
Attachment #8595599 - Flags: approval-comm-aurora?
Attachment #8595599 - Flags: approval-comm-aurora+
packaging fix rs=Ratty a=Ratty CLOSED TREE a=Ratty comm-aurora comm-beta
Attachment #8597265 - Flags: review+
Depends on: 1176773
Depends on: 1178413
Depends on: 1211597
You need to log in before you can comment on or make changes to this bug.