810031 - [email] Switch Gmail to use IMAP instead of ActiveSync

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect, P1)

Description

[Jim Porter (:squib)]

I think we should drop support for Gmail-over-ActiveSync and instead connect to Gmail via IMAP. There are a bunch of reasons for this, which I'll enumerate below, but briefly, the issue is Gmail's ActiveSync support is inferior to Hotmail's, which means either not taking full advantage of ActiveSync for Hotmail or adding extra branches to work about Gmail's shortcomings.

Here's a more detailed list of Gmail's ActiveSync problems, listed in roughly ascending order of importance:

* Gmail doesn't support Autodiscovery, which requires us to work around this during authentication by running a dummy command.

* GetItemEstimate doesn't work properly, and never tells us that a folder has more than 25 messages. This makes it harder for us to choose a good filter range for folders (see bug 808711)

* It's (seemingly) impossible to determine if a message is plain text or HTML over Gmail. This costs us a little bit more bandwidth, and makes editing replies more difficult, since plain text is generally easier to edit than HTML.

* Gmail also munges HTML messages, and returns the same HTML that they insert into the web interface. This is especially bad for embedded images, since they rewrite cid: urls to be queries relative to the Gmail web interface's URL. We're forced to demunge these URLs, which adds extra complexity to our app.

* Apparently, Gmail is talking about discontinuing their ActiveSync support anyway, which means that at some point, people will have to delete their Gmail accounts and start over with IMAP. We might as well do this now. (This is also probably a secret, which is why this bug is marked confidential. This also might never actually happen, since it hasn't been announced publicly, and even then, Google publicly said they'd drop H264, and we all know how that went.)

* Most importantly, IMAP is a better option most of the time anyway. It's a more flexible protocol that lets us do snazzy things like "infinite" scrolling, and Gmail's IMAP support is far better than their ActiveSync support.

For completeness, I'll mention the one major downside to making this switch: Gmail automatically enables ActiveSync support, but requires you to flip a pref to enable IMAP support. This is moderately painful if you're not near a computer, but I assume you could still do it via the mobile browser.

Comment 1

[Dylan Oliver [:doliver]]

Chris, we'd like to get your thoughts on this one. The technical implementation of IMAP would give a better experience for gmail users but there's the possible hurdle of needing to enable IMAP via the gmail web app.

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

After speaking with Asuth, my vote for triage is to +

Comment 3

[Dylan Oliver [:doliver]]

Confirmed with Chris Lee that we should make the switch. blocking+/P1

Comment 4

[Dietrich Ayala (:dietrich)]

Why is this bug marked MoCo Confidential?

Comment 5

[Jim Porter (:squib)]

(In reply to Dietrich Ayala (:dietrich) from comment #4)
> Why is this bug marked MoCo Confidential?

My understanding is that the rumors about Gmail dropping ActiveSync is not public information, but it was also an important part of why I think we should do this.

Comment 6

[Chris Lee [:clee]]

Agree this is the right path forward.

Comment 7

[Jim Porter (:squib)]

(In reply to Dylan Oliver [:doliver] from comment #1)
> Chris, we'd like to get your thoughts on this one. The technical
> implementation of IMAP would give a better experience for gmail users but
> there's the possible hurdle of needing to enable IMAP via the gmail web app.

Should we look into solutions for enabling IMAP in this bug, or should we file a followup?

Comment 8

[Jim Porter (:squib)]

Oh, and I believe we'd discussed detecting whether two-factor auth is enabled, and giving the user some guidance for why their usual password is failing.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Jim Porter (:squib) from comment #8)
> Oh, and I believe we'd discussed detecting whether two-factor auth is
> enabled, and giving the user some guidance for why their usual password is
> failing.

Yes, let's do that.  Assigning to Cody to do those bits

Jim's r=asuth patch to remove the autoconfig uwhere I have notes on the moving parts for the extra bits:
https://github.com/mozilla-b2g/gaia/pull/6381

Comment 10

[Jim Porter (:squib)]

I've merged my portion of the work: https://github.com/mozilla-b2g/gaia/pull/6381

Leaving this open, as we're not done here yet.

Comment 11

[Jim Porter (:squib)]

Message sent to dev-gaia discussing the switch to IMAP: http://groups.google.com/group/mozilla.dev.gaia/browse_thread/thread/11b35d94e42eca9f

Comment 12

[Cody Brocious [:Daeken]]

Attachment: MailAPI patch for two-factor handling in Gmail IMAP accounts

This patch handles the MailAPI side of the two-factor work.  It passes errors back up to the Gaia email app properly to differentiate between two-factor and bad password cases.

Comment 13

[Cody Brocious [:Daeken]]

Attachment: Adds the English two-factor string

Gaia patch for the two-factor application specific password string.  Obviously needs to be properly written to include details on how to remedy the issue.

Comment 14

[Cody Brocious [:Daeken]]

The detection of two-factor is all functional with this, but there are two things I notice: 1) we don't seem to have any support for prompting the user for a new password in the case of either a changed password, or two-factor being turned on, once the account is initially set up.  2) We should probably change the way errors show up when setting up accounts, since it's not terribly easy to read as it stands.

Comment 15

[Jim Porter (:squib)]

Comment on attachment 682343 [details] [diff] [review]
MailAPI patch for two-factor handling in Gmail IMAP accounts

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

::: data/lib/mailapi/imap/account.js
@@ +543,5 @@
>            case 'NO':
>            case 'no':
> +            // XXX: Should we check if it's GMail first?
> +            if (err.serverResponse.indexOf('[ALERT] Application-specific password required') != -1)
> +              errName = 'needs-application-password';

I'm guessing this should be 'needs-app-password', as in probe.js (or vice versa).

Comment 16

[Cody Brocious [:Daeken]]

(In reply to Jim Porter (:squib) from comment #15)
> I'm guessing this should be 'needs-app-password', as in probe.js (or vice
> versa).

Good catch, thanks.  That code doesn't currently do anything (we need messaging on login failures beyond setup), but hopefully before this lands we can get that all straightened out regardless.

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Cody Brocious [:Daeken] from comment #14)
> 1) we don't seem to have any support for prompting the user
> for a new password in the case of either a changed password, or two-factor
> being turned on, once the account is initially set up.

We do have this.  The 'NO' case calls MailUniverse.__reportAccountProblem, which then calls MailUniverse.__notifyBadLogin when it sees 'bad-password' which then calls MailBridge.notifyBadLogin for all known bridges.  The MailBridge then sends a 'badLogin' message which calls mailAPIInstance.onbadlogin which then displays a 'setup-fix-password' dialog.

We will need to widen the check from just 'bad-password' to include the error code case you are adding to take advantage of that.  Probably just a switch statement at the spot where we filter and plumb the error code through.

Please also figure out what google says when you try and login to a gmail account that has IMAP disabled so we can do that too.  The manual way to do this is:

- run: openssl s_client -connect imap.gmail.com:993
- type: a01 LOGIN [e-mail] password
- type control-d to get out

For the IMAP-is-disabled case, it's a bit awkward, but we might as well still use the fix-password UI since it accomplishes the goal of making the user fix the problem and then hit a button to tell us they fixed the problem.

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

For the IMAP prober, since we're basically switching to a node callback style and we have adopted that throughout anyways, let's change the field "accountGood" to "error" and have null be success rather than 'success'.  It appears I was trying to make debugging easier by having accountGood effectively be a tri-state (null: don't know yet, true: good, false: bad), but whether onresult is null or not already captures that, so there's no loss in doing that.

Comment 19

[Jim Porter (:squib)]

(In reply to Andrew Sutherland (:asuth) from comment #17)
> Please also figure out what google says when you try and login to a gmail
> account that has IMAP disabled so we can do that too. 

I believe, though I'm not 100% certain, that Google lets you log in no matter what. I created a new test account recently and never turned on IMAP, but I can still log in through the app.

Comment 20

[Cody Brocious [:Daeken]]

I'm implementing support for this now, but it seems that IMAP is enabled by default.  The message if you don't have it enabled (which comes after the two-factor message, oddly enough -- wasn't until I gave it an app-specific password that I got the error) is: [ALERT] Your account is not enabled for IMAP use. Please visit your Gmail settings page and enable your account for IMAP access.

Wrapping up a patch incorporating that and the other changes mentioned shortly.

Comment 21

[Cody Brocious [:Daeken]]

Attachment: Updated MailAPI patch

This patch builds on the previous MailAPI patch, providing support for handling disabled IMAP and bubbling errors up in the right places.

Comment 22

[Cody Brocious [:Daeken]]

Attachment: Updated Email app patch

This patch enables cards for alerting the user to a newly enabled two-factor setup and IMAP being disabled.

The messages all need some love, and the IMAP error card is just a stub.  I'm a bit stuck on a strange bug, though:
1) Add an account with no two-factor auth
2) Enable two-factor auth on the account
3) At the 'You must enter an application specific password' card, enter the non-app-specific password again
4) It simply dumps the error string to the console and spins, instead of showing the password card again

I believe I'm missing something in terms of clearing errors, but I can't put my finger on where.

Comment 23

[Cody Brocious [:Daeken]]

Attachment: Updated Email app patch

Woops, that was the old patch.  Previous comment applies to this one instead.

Comment 24

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 682640 [details] [diff] [review]
Updated MailAPI patch

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

I just checked the code for _cmd_clearAccountProblems in mailbridge.js, which is what actually services the call to clearAccountProblems.  The reason you are experiencing a problem is that since the error you are returning is not 'bad-user-or-pass', it thinks that we were actually successful in establishing the connection, or successful enough.

Although the comment is a little nebulous (and dubiously phrased about "not an authentication"), the rationale is that there are a number of situations where a connection can fail, and it's really annoying to present those to the user as a bad password.  So I was trying to avoid being tricked by simple connection failures or by more complicated situations like hitting the server's connection limit.

Please explicitly set the feedback or review flags for the next round, as it makes it a lot easier for me to keep on top of reviews.  Also, especially for gaia-email-libs-and-more patches, a pull request on github is preferred (using https://addons.mozilla.org/en-US/firefox/addon/github-tweaks-for-bugzilla/ to then mirror the pull request over to here), or failing that, be sure to include 8 lines of context in the diffs.  Thanks!

::: data/lib/mailapi/accountcommon.js
@@ +352,4 @@
>        ['imap', 'smtp'],
>        function probesDone(results) {
>          // -- both good?
> +        if (results.imap[0] == null && results.smtp) {

The node callback idiom is to treat falsy as success for 'error', which is what [0] is, although it's sadly not as clear as a variable named err...  So this would be !results.imap[0].

@@ +363,4 @@
>          // -- either/both bad
>          else {
>            // clean up the imap connection if it was okay but smtp failed
> +          if (results.imap[0] == null) {

same check change here

@@ +367,2 @@
>              results.imap[1].close();
> +            callback('smtp-unknown', null); // Failure was caused by SMTP, but who knows why

If you are adding a new error code, then you want to document it in mailapi.js's documentation block for tryToCreateAccount and add a "setup-error-smtp-unknown" string in email.en-US.proeprties.

::: data/lib/mailapi/imap/account.js
@@ +544,5 @@
>            case 'no':
> +            // XXX: Should we check if it's GMail first?
> +            if (err.serverResponse.indexOf('[ALERT] Application-specific password required') != -1)
> +              errName = 'needs-app-pass';
> +            else if(err.serverResponse.indexOf('[ALERT] Your account is not enabled for IMAP use.') != -1)

type coercion isn't an issue on these, but please use !== -1 for consistency

Also, these errors should be added to the mailapi.js tryToCreateAccount docs as well, although technically for the addition of the identical logic to imap/probe.js.  And I guess they want strings too?  (Maybe the strings you've already created?)

Comment 25

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 682643 [details] [diff] [review]
Updated Email app patch

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

This looks reasonable, but obviously the fix-IMAP card needs to have a button hooked up or some other means to call clearProblems and make the card go away.  And also, per my comments on the other patch, you will ideally want to add strings explaining the error for during the signup process.  If we were doing something fancier than just putting up a string for the you-need-to-enable-IMAP case, we might try and reuse that card, but I don't see a major advantage to that right now.

Comment 26

[Cody Brocious [:Daeken]]

Attachment: Fixed remaining issues

This patch fixes all the issues discussed here, with the exception of the IMAP error.  When IMAP is disabled /after/ initial setup, the message pops up and then disappears upon retry, never to come back again.  Everything else seems to be fine.  Any ideas there?

Comment 27

[Cody Brocious [:Daeken]]

Attachment: Fixed IMAP error card

This patch to the Email app fixes the format and function of the IMAP error card, and that's about it.  Everything seems to be done -- outside of some textual polish -- on the app side of things.

Comment 28

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Cody Brocious [:Daeken] from comment #26)
> When IMAP is disabled /after/ initial setup, the message pops
> up and then disappears upon retry, never to come back again.  Everything
> else seems to be fine.  Any ideas there?

Nothing comes to mind.  Given the current time pressure, I think it makes sense to land these patches now and file a follow-up bug to address that.

The general strategy for landing changes that involve gaia-email-libs-and-more is to first land your gaia-email-libs-and-more changes on master.  Then check that out, and install the changes to your gaia repo, then land your gaia changes with those changes.  There is no need to use rebase -i to squash changes for gaia-email-libs-and-more, but please do squash for gaia.

Comment 29

[Jim Porter (:squib)]

Removed Gmail-over-ActiveSync workarounds:
  https://github.com/mozilla-b2g/gaia/pull/6514
  https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/83

Comment 30

[Cody Brocious [:Daeken]]

Okay, both patches are landed in their respective repos.

Comment 31

[Andrew Sutherland [:asuth] (he/him)]

Google just announced disabling ActiveSync access for new devices for non-paying users starting January 30th, 2013.  So this was a very good call!

http://googleblog.blogspot.com/2012/12/winter-cleaning.html
http://support.google.com/a/bin/answer.py?hl=en&answer=2716936

Comment 32

[Andrew Sutherland [:asuth] (he/him)]

Clearing MoCo confidential since I'd like to name-check this bug and our rumor turned out to e true and anything secret about it is now long since moot.

Comment 33

[Andrew Sutherland [:asuth] (he/him)]

For posterity, Cody's patch landed as:

https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/84
https://github.com/daeken/gaia-email-libs-and-more/commit/eebea0825e5e03e8a1ad1231079e696866040547

https://github.com/mozilla-b2g/gaia/pull/6517
https://github.com/mozilla-b2g/gaia/commit/92996949706346a5fbdd25a3b0b084654b76f141