Closed Bug 1231642 Opened 8 years ago Closed 8 years ago

Log in to Mail.Ru (IMAP/SMTP) using OAuth

Categories

(MailNews Core :: Networking, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird45+ fixed, thunderbird46 fixed)

VERIFIED FIXED
Thunderbird 46.0
Tracking Status
thunderbird45 + fixed
thunderbird46 --- fixed

People

(Reporter: monolithed, Assigned: rkent)

References

(Blocks 2 open bugs)

Details

(Keywords: feature, user-doc-needed)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.73 Safari/537.36
Severity: normal → enhancement
OS: Unspecified → All
Hardware: Unspecified → All
1. Add configuration to the `comm-central` (comm-central/mailnews/base/util/OAuth2Providers.jsm)

> diff --git a/mailnews/base/util/OAuth2Providers.jsm b/mailnews/base/util/OAuth2Providers.jsm
> --- a/mailnews/base/util/OAuth2Providers.jsm
> +++ b/mailnews/base/util/OAuth2Providers.jsm
> @@ -10,32 +10,41 @@ var EXPORTED_SYMBOLS = ["OAuth2Providers
>  var {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components;
>  
>  // map of hostnames to [issuer, scope]
>  var kHostnames = new Map([
>    ["imap.googlemail.com", ["accounts.google.com", "https://mail.google.com/"]],
>    ["smtp.googlemail.com", ["accounts.google.com", "https://mail.google.com/"]],
>    ["imap.gmail.com", ["accounts.google.com", "https://mail.google.com/"]],
>    ["smtp.gmail.com", ["accounts.google.com", "https://mail.google.com/"]],
> +
> +  ["imap.mail.ru", ["o2.mail.ru", "mail.imap"]],
> +  ["smtp.mail.ru", ["o2.mail.ru", "mail.imap"]],
>  ]);
>  
>  
> var kIssuers = new Map ([
>    ["accounts.google.com", [
>      '406964657835-aq8lmia8j95dhl1a2bvharmfk3t1hgqj.apps.googleusercontent.com',
>      'kSmqreRr0qwBWJgbf5Y-PjSU',
>      'https://accounts.google.com/o/oauth2/auth',
>      'https://www.googleapis.com/oauth2/v3/token'
>    ]],
> +  ["o2.mail.ru", [
> +    'thunderbird',
> +    'I0dCAXrcaNFujaaY',
> +    'https://o2.mail.ru/login',
> +    'https://o2.mail.ru/token'
> +  ]],
>  ]);


2. Add ISP configuration to the Mozilla autoconfig

> Index: trunk/mail.ru
> ===================================================================
> --- trunk/mail.ru   (revision 148636)
> +++ trunk/mail.ru   (working copy)
> @@ -13,6 +13,7 @@
>        <port>993</port>
>        <socketType>SSL</socketType>
>        <username>%EMAILADDRESS%</username>
> +      <authentication>OAuth2</authentication>
>        <authentication>password-cleartext</authentication>
>      </incomingServer>
>      <incomingServer type="imap">
> @@ -41,6 +42,7 @@
>        <port>465</port>
>        <socketType>SSL</socketType>
>        <username>%EMAILADDRESS%</username>
> +      <authentication>OAuth2</authentication>
>        <authentication>password-cleartext</authentication>
>      </outgoingServer>
>      <outgoingServer type="smtp">
We hated hard-coding those Google OAuth settings, but at the time there was no easy alternative. I suppose we are going to have to face the situation with other providers.

What is the current status of OAuth with Mail.ru? Specifically, is it required or otherwise strongly encouraged? What is the current user experience without OAuth?

IF we were to support Mail.ru we would need to create a new app key and secret that used our thunderbird-accounts email address, not whatever you have here (which I assume you created yourself).
@Kent James

> What is the current status of OAuth with Mail.ru?

We already use OAuth as our primary protocol and it is strongly recommended to our clients.
Please see our documentation https://cloud.mail.ru/public/Mk6L/MtC8EcgBL

> What is the current user experience without OAuth?

login / password

> IF we were to support Mail.ru, we would need to create a new app key and secret that used our thunderbird-accounts email address, not whatever you have here (which I assume you created yourself).

Nope. We created these keys specifically for this application.
OK some questions.

1) Your language implies that you are officially representing Mail.ru here. Is that true? If so, how can I verify that?

2) Have you tried the changes that you suggest in a custom build of Thunderbird, and confirmed that it works?
Status: UNCONFIRMED → NEW
Component: Untriaged → Networking
Ever confirmed: true
Product: Thunderbird → MailNews Core
> 1) Your language implies that you are officially representing Mail.ru here. Is that true? If so, how can I verify that?

It's true. We can continue our communication via a.abashkin@corp.mail.ru if you see fit.

> 2) Have you tried the changes that you suggest in a custom build of Thunderbird, and confirmed that it works?

Yes, of course we have a custom build of Thunderbird. We had to make some changes in our OAuth-implementation. I will try to test and report you on next week.
OK at least from my perspective, this all sounds reasonable. But the whole OAuth implementation that we did was a little controversial internally, as there are differences of opinions about whether OAuth is a good thing to be promoting. Certainly the hard-coding of support for only GMail was not optimal but done to be pragmatic. There were valid criticisms that doing that would force other providers who offer OAuth to also require hard-coding, as we are seeing here with your request. Still, we have no intention of offering preferential support to GMail, so we should reasonably accept requests from Mail.ru to support their OAuth implementation. Given the current implementation, adding support for Mail.ru in the code seems reasonable.

Timing wise, we are getting close to our next major release, which is TB 45 due in March 2016. But actually the first code freeze for that occurs next week. But there is nothing in the proposed changes here that would be difficult to uplift during the aurora and beta cycles, but still ideally we would get this done within six weeks so that the Mail.ru OAuth support is included in the betas for TB 45 that we will start to ship around the end of January.

The changes in the core code should ideally be posted here as a patch rather than described. Could you do that, and ask for my review? For the changes to the ISP database, that requires a separate bug, but I don't see any obstacles to that moving forward quickly once the core code changes are available.

I would also want to test this implementation. Can I just go to Mail.ru and create an account that would allow me to do that, or do you need to provide an account with appropriate credentials?

Thanks for working to support Thunderbird better with Mail.ru, we do appreciate that.
Attached patch 1231642.patch — — Splinter Review
Attachment #8697982 - Flags: review?(rkent)
> The changes in the core code should ideally be posted here as a patch rather than described. Could you do that, and ask for my review??

Sure, but we had to make some changes in our OAuth-implementation to suppress "state" parameter. We have to make the release by the end of this week. I'll let you know when it will be ready. 

> I would also want to test this implementation. Can I just go to Mail.ru and create an account that would allow me to do that, or do you need to provide an account with appropriate credentials?

Test credentials:
test-thunderbird-oauth@mail.ru / m0zillA

Or you can create an account at https://e.mail.ru/signup?lang=en_US


Thanks!
@Kent James, we are ready. Could you review?
I'm mildly disappointed that we've had to hardcode credentials for gmail, and I worry about having to add more for any other site. I don't like the precedence that this is setting.

I would much rather that sites implement RFC 7591 and RFC 7628 (that's OAuth dynamic client registration and the SASL OAUTHBEARER).
(In reply to Joshua Cranmer [:jcranmer] from comment #10)
> I'm mildly disappointed that we've had to hardcode credentials for gmail,
> and I worry about having to add more for any other site. I don't like the
> precedence that this is setting.
> 
> I would much rather that sites implement RFC 7591 and RFC 7628 (that's OAuth
> dynamic client registration and the SASL OAUTHBEARER).

Yes I thought you might be skeptical, and my comment 6 was largely directed at that issue. One request per year is not much of a burden, particularly when the provider has done the majority of the work. I really think that for now we need to be pragmatic. We have no dynamic client registration to test against, so we do not really support that ourselves. It is unreasonable to preferentially support gmail over mail.ru when mail.ru has a similar request.

If and when a provider supports dynamic site registration and we implement that support, then we can start demanding of mail providers that they support it if they want OAuth2 to be included in Thunderbird for their domain. We might at some point also put some restriction on the size of provider we will hardcode, but Mail.ru would reasonably meet any requirements of sufficient scale.

And for the record, the support of GMail Oauth2 has gone quite smoothly with this implementation as far as I can tell.
I can confirm that email operations are successful using the provided changes, including both IMAP as well as SMTP, when the configuration is manually switched to OAuth2.

You'll have to admin, Joshua, that consolidating the credentials in a single file like that makes the required changed to support a new provider dead easy from our perspective!

Alexander, the login screen displays in English, but the next page is in Russian (which I can read by the way). I don't know how important support of non-Russian readers is for you, but that is less than optimal. Is there any way to fix that?

Also, you mentioned that there were some changes on your server required for this to work. Is there anything that we could do to resolve whatever that issue was, so that we don't have this issue in the future?
> Alexander, the login screen displays in English, but the next page is in Russian (which I can read by the way). I don't know how important support of non-Russian readers is for you, but that is less than optimal. Is there any way to fix that?

The reason why you are getting Russian language after username\password check is because Russian is the language of your mailbox (you may change it in mailbox settings).

Language logics is following:
1. check GET lang parameter and use it
2. if there is no lang parameter use mailbox language settings(your case)
3. if none of the first two then use browsers language



> Also, you mentioned that there were some changes on your server required for this to work. Is there anything that we could do to resolve whatever that issue was, so that we don't have this issue in the future?

Nope, everything was resolved. Thank you for your help.
Joshua, I intend to r+ this and ultimately push it for TB 45 unless you want to object. But be prepared for some strong counter-objections from me.
Flags: needinfo?(Pidgeot18)
(In reply to Kent James (:rkent) from comment #14)
> Joshua, I intend to r+ this and ultimately push it for TB 45 unless you want
> to object. But be prepared for some strong counter-objections from me.

I'm fine with this going in, but on one major caveat:

I desperately want to see this static list disappear, and the way to make this disappear is dynamic client registration. This has already achieved RFC status (see the RFCs reach above). Once I can see servers implementing this, I'm planning on ripping the list out, perhaps even if not all the providers have implemented dynamic client registration yet.

So, to Abashkin Alexander, if you're working with the mail.ru team, could you perchance help convince them to implement dynamic client registration? I would be more than happy to prototype a client implementation of the technology if that poses a significant stumbling block to implementing it on the server side.
Flags: needinfo?(Pidgeot18)
Comment on attachment 8697982 [details] [diff] [review]
1231642.patch

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

As discussed, I'll approve and push this.
Attachment #8697982 - Flags: review?(rkent) → review+
https://hg.mozilla.org/comm-central/rev/625d871a9669

Alexander, could you answer Joshua's question in comment 15 about whether you would be willing to work with him on a dynamic registration prototype?
Assignee: nobody → duthen
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(duthen)
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 46.0
Version: 45 → 46
(In reply to Joshua Cranmer [:jcranmer] from comment #15)
> 
> So, to Abashkin Alexander, if you're working with the mail.ru team, could
> you perchance help convince them to implement dynamic client registration? I
> would be more than happy to prototype a client implementation of the
> technology if that poses a significant stumbling block to implementing it on
> the server side.

(In reply to Kent James (:rkent) from comment #17)
> https://hg.mozilla.org/comm-central/rev/625d871a9669
> 
> Alexander, could you answer Joshua's question in comment 15 about whether
> you would be willing to work with him on a dynamic registration prototype?


I apologize for the late reply, I had a vacation.
Yes, we have such plans. When? I can't say right now.
Flags: needinfo?(duthen)
Comment on attachment 8697982 [details] [diff] [review]
1231642.patch

[Triage Comment]

This is a very simple change to support OAuth for Mail.ru, and should be available in TB45.
Attachment #8697982 - Flags: approval-comm-aurora+
Alexander, there is now a beta out that supports this. The autoconfig database though does not currently specify this (see https://autoconfig.thunderbird.net/v1.1/mail.ru)

Are you planning on filing a bug there to add the OAuth fields to this? Or is there an existing bug?
Flags: needinfo?(monolithed)
Blocks: 1253196
Kent, As far as I understand, we have to specify ISP configuration after the current release. Please see my patch https://bug1253196.bmoattachments.org/attachment.cgi?id=8726130
Flags: needinfo?(monolithed)
Flags: needinfo?(rkent)
Raising the discussion here instead of on bug 1253196 for changing the ISPDB:

Should/can mail.ru self-host the OAuth-declaring autoconfig file (as described at https://developer.mozilla.org/en-US/docs/Mozilla/Thunderbird/Autoconfiguration#Configuration_server_at_ISP)?

I'm not a great enforcer of the advocacy aspects of the ISPDB, but this seems like a point at which a decision should be made as to whether we should be requiring any other OAuth-adopters to stand up their own self-hosted autoconfig entries.  (Even if we also have to or might as well end up updating the ISPDB entry as well if they're already hosted in the ISPDB and because of bug 1253147 we can't actually remove the domain.)
Andrew, I see no problem to host autoconfig file at Mail.ru. Could you provide a usage example?
Flags: needinfo?(bugmail)
See https://developer.mozilla.org/en-US/docs/Mozilla/Thunderbird/Autoconfiguration#Configuration_server_at_ISP

In short, you'd want to stand up the revised ISPDB entry at one of the following:
- http://autoconfig.mail.ru/mail/config-v1.1.xml
- http://mail.ru/.well-known/autoconfig/mail/config-v1.1.xml

Ideally you would also to host with https (using a valid certificate) as well so that things don't break when Thunderbird's autoconfig process becomes secure.
Flags: needinfo?(bugmail)
Andrew, it seems we need more information. 
We have a wide variety of own and delegated hostnames. How does the ISPDB resolve hostnames (a split of mx records or emails)?
Flags: needinfo?(bugmail)
Thunderbird first tries with the mail domain the user enters including self-hosted and ISPDB-hosted.  If that's not working out, it asks the ISPDB server to perform an MX lookup.  Then it checks the ISPDB for that MX-looked-up domain.  Unfortunately, it doesn't check for self-hosting on the MX-looked-up domain at this time.  (But it's not a huge change for Thunderbird to do that, just no one is actively improving Thunderbird's autoconfig and the most recent would-be contributor seems to have given up.)

If you have other domains that have MX entries pointing at mail.ru (via any MX with the mail.ru domain suffix), then we'll need the ISPDB entry updated to mirror what you are self-hosting.  This would be the ideal path, I think.
Flags: needinfo?(bugmail)
Alexander, it seems to me like you are getting what you need from Andrew, so clearing the Need Info
Flags: needinfo?(rkent)
Andrew, Kent, thanks for your help! 
We are planning to provide the autoconfig file next week.
The self-hosted file does not include the oauth line you proposed in bug 1253196.  Is this an oversight?  Specifically, the diff against what the ISPDB hosts right now is just:

$ svn diff
Index: mail.ru
===================================================================
--- mail.ru	(revision 150392)
+++ mail.ru	(working copy)
@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="UTF-8"?>
+<?xml version="1.0"?>
 <clientConfig version="1.1">
   <emailProvider id="mail.ru">
     <domain>mail.ru</domain>
It's true, we are planning to include these lines after 45 release.
Flags: needinfo?(bugmail)
Okay, then please needinfo me on bug 1253196 when you do that and I'll be sure to update the ISPDB at the same time.
Flags: needinfo?(bugmail)
Assignee: monolithed → rkent
Status: RESOLVED → VERIFIED
Blocks: 1310389
See Also: → 1310384
Blocks: 1370186
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: