Closed Bug 1598861 (Aalto-missing-IMAP) Opened 5 years ago Closed 5 years ago

Can't select IMAP/POP options during account creation - only exchange server option is offered for cases where guessing common names would have given IMAP/POP option

Categories

(Thunderbird :: Account Manager, defect)

defect
Not set
normal

Tracking

(thunderbird_esr68+ fixed, thunderbird68 wontfix, thunderbird72 fixed, thunderbird73 fixed)

RESOLVED FIXED
Thunderbird 73.0
Tracking Status
thunderbird_esr68 + fixed
thunderbird68 --- wontfix
thunderbird72 --- fixed
thunderbird73 --- fixed

People

(Reporter: mkmelin, Assigned: mkmelin)

References

(Regression)

Details

(Keywords: regression)

Attachments

(4 files, 5 obsolete files)

Attached image setup-broken-tb68.2.2.png (deleted) —

For account setup,in the case where no explicit configuration file can be found, and the server used is exchange, only the exchange account option is presented to the user.

To test this you need to provide working creds since fetchConfigFromExchange doesn't work without the right creds.

Looks like fetchConfigFromExchange should be part of guessing the config, or the guessing of the config should move to not being the fallback.

I suspect this is a fairly common scenario: just by giving the mail server a name like mail.example.com or imap.example.com you get all the configuration automatically, with no need to bother with a config file.

AFAIKT, this is broken ever since bug 1500105.

If you present incorrect creds, you'll get the IMAP and POP options.

If the AutoDiscover mechanism is available (and no config is found in ISPDB etc.), we show what the server itself responded to us about the available protocols. Could you please share the AutoDiscover response XML file that came back from the server? I need this to investigate what happened.

@Magnus: Please CC me on such bugs.

If this indeed is a bug and doesn't work as discussed and intended in bug 1500105, I'll fix this.

Assignee: mkmelin+mozilla → ben.bucksch
Attached file AutoDiscover.xml (deleted) —

AutoDiscover.xml response.

Thanks, Magnus. This helps.

This isn't a bug. The AutoDiscover protocol can return IMAP, POP3 and SMTP configurations, and we do have code to parse this out and return it and show it to the user. See: https://searchfox.org/comm-central/source/mail/components/accountcreation/content/exchangeAutoDiscover.js#283

else if (type == "POP3" || type == "IMAP" || type == "SMTP") {

However, this server does not advertise IMAP or SMTP. This is a choice by the admin and/or by Microsoft, not by us. We are simply offering the protocols that the server tells us to use. And that is only the Exchange protocols. We follow the server policy.

You keep saying that users should advocate their admins to enable IMAP. I would encourage you to do that in this particular case. You will see the reaction you get. Keep in mind that many of our users are on large domains with thousands of users, sometimes 100000s. Given that this is a University, you have a higher chance of success than e.g. changing the policy of TomTom. Even more so, because you say IMAP is actually enabled in this case and just the advertisement of it needs to be changed. You keep saying that users should talk with the admins to change server configuration, so please try it yourself and see what happens.

Maybe more constructively, there's something we could do: Instead of going into full guess config and trying to guess host names, which is gross and ugly and desperate, we can simply take the hostname from AutoDiscover of the other protocols and check whether IMAP and SMTP is available on that hostname. In this case, the hostname is mail.aalto.fi , so we could try IMAP on that.

But we would go directly against the published policy of the server, if we do that. Rather, the correct solution would be to fix the AutoDiscover of that server.

Currently, we just honor whatever the server tells us. We show all protocols that the server advertizes and that we can speak. I think this is the correct behavior.

Interesting. Ben, can you also please have a look at attachment 9111031 [details]. That's what happens if you go in there with an incorrect user and password. First it opens the box to enter your domain credentials, and after a few seconds it offers IMAP and POP3. At least opening up the extra password box looks somewhat sub-optimal. You can try this yourself without having credentials on the server. Is there room for improvement?

I assume without Exchange support, this particular server would have just fallen back to the "guess config" which luckily works, right? That's just the same as we get with an invalid or no password.

We could add an ISPDB file or does AutoDiscover override that?

I have no idea what the autodicover.xml is supposed to list. It is however, pretty clear that IMAP/SMTP is supported (as I'm using it myself).
So of course it's a bug: IMAP is there, working and available in every sense of the word - yet we don't allow the user to select it.

Please realize there are large populations of university users that are not using Windows, so supporting IMAP is not a "nice to have" for them.

(In reply to Jorg K (GMT+1) (PTO to 1st Dec 2019, not reading bugmail) from comment #9)

We could add an ISPDB file or does AutoDiscover override that?

ISPDB only contains the truly large entries. While universities are large, I don't think most/any qualify at least under current policy.

I have no idea what the autodicover.xml is supposed to list.

A quick Google search will tell you, I'm picking a semi random search result:
https://portal.smartertools.com/community/a89198/autodiscover-for-outlook-2013-2016-on-windows-for-imap-smtp-accounts.aspx

if you go in there with an incorrect user and password.

You need to enter the correct password, otherwise the protocol doesn't work. Unfortunately, the AutoDiscover protocol requires a valid login. That's my biggest grief with it. Esp. because the username is often the Windows login name, not the email address. That's what the username field is for.

We could add an ISPDB file or does AutoDiscover override that?

ISPDB overrides AutoDiscover. But what Magnus said in the last comment. We normally ask smaller domains to host the autoconfig file themselves (which then overrides ISPDB). https://developer.mozilla.org/en-US/docs/Mozilla/Thunderbird/Autoconfiguration#Configuration_server_at_ISP

it's a bug

Sure, but not of Thunderbird, but of your University, for not including IMAP in the published and automatically advertised config.

You keep saying that users should contact their admins and advocate IMAP. I'd like to hear what happens after you tried. Please do go through the experience before you ask others to do it.

If IMAP is available, and Thunderbird can find it but choses not to, then that is clearly a bug.
Like I wrote in comment 0, I'd imagine it's fairly common not to set up a configuration file since just using a common dns name for for the server would already get you set up by guessing.

not to set up a configuration file

But here, there is one and it's telling us not to use IMAP.

I've written to them:

To: servicedesk@aalto.fi
Subject: AutoDiscover of protocols available for Aalto.fi mail services

Dear Madam / Sir,
I'm writing to you from the team of Mozilla Thunderbird.
As we can see at https://www.aalto.fi/en/services/adding-an-account-in-thunderbird, you promote Microsoft Outlook, but you also allow other clients like Thunderbird to use the IMAP or POP3 protocol.
A recent version of Thunderbird interprets the AutoDiscover.xml file your server transmits (see https://bug1598861.bmoattachments.org/attachment.cgi?id=9111040) and only offers to use the Exchange protocol since your server actually does not advertise IMAP or POP3 support in the AutoDiscover.xml file.
That's is somewhat unfortunate, since the Thunderbird account setup now doesn't offer IMAP/POP3 any more. We're tracking this issue in https://bugzilla.mozilla.org/show_bug.cgi?id=1598861.
I have some related questions:
Is there a particular reason why you don't advertise IMAP/POP3 in your AutoDiscover.xml file? Is this done deliberately or is this an omission? In the latter case, how do you feel about configuring your mail service so the IMAP/POP3 (missing word: protocols) are advertised as well?
With kind regards,

Eh, of course things are fixable on a per provider basis. There's no reason not to fix it for the general case.

Looks like exchange detection just needs to be moved to being called from guessing failed. The downside is that it is slower (for the exchange case), but since we'd need to wait for the guesses to go through anyhow, seems there's not much we can do about that.

Assignee: ben.bucksch → mkmelin+mozilla
Attached patch bug1598861_probe_then_exchange.patch (obsolete) (deleted) — Splinter Review

Rouge proof of concept. Works but may need some adjustments.

Fixes bug 1598945 too.

Comment on attachment 9111081 [details] [diff] [review] bug1598861_probe_then_exchange.patch This is wrong. This is wildly guessing host names, even though we have specific instructions from the mail server itself what to do. a) This violates the published wish of the server. We correctly honor the server wishes (via network protocols, not support docs). The server is just misconfigured. b) If we want to ignore the server wish, then comment 7 is the right fix here.
Attachment #9111081 - Flags: review-

I am awaiting the admin response.

Magnus, which server names does this patch guess? imap.aalto.fi and smtp.aalto.fi as you get with an invalid password, see attachment 9111031 [details]?

The funny thing is that https://www.aalto.fi/en/services/adding-an-account-in-thunderbird for some reason says that the outgoing server should be mail.aalto.fi. imap.aalto.fi and mail.aalto.fi are the same server at 130.233.0.9, smtp.aalto.fi is different at 130.233.228.120.

Is there anything wrong with Ben's approach to use the server from the AutoDiscover? I think it's really unfortunate that they have this misconfigured. We also have no statistics how frequent that sort of misconfiguration occurs.

We try mail.example.com and imap.example.com - https://searchfox.org/comm-central/rev/a7d599e6422ad3053384502bf1555e64a41c517e/mail/components/accountcreation/content/guessConfig.js#757

Is there anything wrong with Ben's approach to use the server from the AutoDiscover?

We can use the autodiscover results if they are correct and indeed list what the server supports. As per evidence in this bug, we can't trust that result.

It's like a car manufacturer writing "only use original spare parts". Using non-original parts will work just as fine. In this case, it's even from the original manufacturer, only they forgot to put it in the ad.

Kindly answer my question from comment #24:

which server names does this patch guess? imap.aalto.fi and smtp.aalto.fi as you get with an invalid password?

Looking at the code at
https://searchfox.org/comm-central/rev/a7d599e6422ad3053384502bf1555e64a41c517e/mail/components/accountcreation/content/guessConfig.js#783
it would try smtp first.

I think since we're dealing with Exchange servers also handling "legacy" protocols, it's likely that the same server that will handle those. So it would be less guessing using "original spare parts" in this case and "using non-original parts will" ... NOT ... "work just as fine".

(In reply to Jorg K (GMT+1) (PTO to 1st Dec 2019, not reading bugmail) from comment #26)

Kindly answer my question from comment #24:

I didn't want to sidestep the discussion since it's irrelevant. But here you go:
I think(?) it's a race condition of the probes which of the servers will be found/detected first. I got mail.aalto.fi as smtp when I tried now.

The backstory is that (as the url you found) says below #5, that smtp.aalto.fi will work for some cases. This is the traditional ISP setup where the SMTP will work only inside the ISP network, which is less useful nowadays with laptops that move around. But it's still used by some, so it's still there. mail.aalto.fi is the modern smtp which will work for users not inside the local network too.

I think since we're dealing with Exchange servers also handling "legacy" protocols, it's likely that the same server that will handle those.

Not necessarily like in this case for SMTP.

So it would be less guessing using "original spare parts" in this case and "using non-original parts will" ... NOT ... "work just as fine".

Less guessing, but since it wouldn't let you drive the car (use the core features of Thunderbird) that's of little comfort.

I think since we're dealing with Exchange servers also handling "legacy" protocols, it's likely that the same server that will handle those.

Not necessarily like in this case for SMTP.

I don't get it. You're telling me you should be finding smtp.aalto.fi although it's a different server to mail.aalto.fi and their site states:
When you create a new account, the installation wizard incorrectly suggests smtp.aalto.fi as the SMTP server. Type the correct SMTP server as mail.aalto.fi, so the mail server also operates outside of Aalto’s domain.

Preferably it should find mail.aalto.fi as smtp server (and it does so for me), but whether you get that or not depends on which of the guessed servers answers first. It's not a situation that we can easily resolve by automatic means.

Well, yes, we can. We can use the AutoDiscover server. I don't understand the line of argument. You're claiming that there are Exchange(!!) setups where the advertised exchange server does NOT handle IMAP/SMTP (quote: Not necessarily like in this case for SMTP) and give the Aalto site as example, when this very site encourages the use of the Exchange server.

What's not to understand? The autodiscover file lies, as it doesn't list the standard protocols it does support, and the standard protocols we do want to use.

OK, I repeat it then.

The AutoDiscover file is incomplete, it doesn't list the protocols we want to offer. We agree on that. But it does nominate an Exchange-capable server. Unless proven otherwise, in most cases, like for Aalto.fi, this server will also handle IMAP/POP3/(EDIT):SMTP. So the preference is to probe this server for the protocols and not any other server.

Unless I've misunderstood, you've been arguing that in the case of Aalto.fi there is another SMTP server we could detect with, what Ben calls, "wild guessing". However, in the case of this site, that result isn't preferred.

So I'm still looking for proof that guessing will get a better result than just probing the Exchange server.

(In reply to Jorg K (GMT+1) (PTO to 1st Dec 2019, not reading bugmail) from comment #32)

The AutoDiscover file is incomplete, it doesn't list the protocols we want to offer. We agree on that. But it does nominate an Exchange-capable server. Unless proven otherwise, in most cases, like for Aalto.fi, this server will also handle IMAP/POP3/(EDIT):SMTP. So the preference is to probe this server for the protocols and not any other server.

Well we could to that, in addition, though it's significantly more complicated.

Unless I've misunderstood, you've been arguing that in the case of Aalto.fi there is another SMTP server we could detect with, what Ben calls, "wild guessing". However, in the case of this site, that result isn't preferred.

Yes since there's the network-internal server too, depending on which answers first, you can get the wrong config guessed. You can also get the right config guessed.

So I'm still looking for proof that guessing will get a better result than just probing the Exchange server.

In this specific case, just probing the server found from autodiscover would give you the right results, correct. I would agree, for many/most servers that could be the case.

And more: It's possible/likely that the site has one server, not multiple. If that server doesn't follow the assumed naming scheme of "imap", "mail", "pop3", "smtp", guessing won't find it, but using the AutoDiscover result will.

I would agree, for many/most servers that could be the case.

So we all agree than that the AutoDiscover-advertised server should be probed and nothing else? In that case the patch isn't right.

Let me add a personal comment: Ben is the original author of the account setup module, inventor of the ISBDB and also the guy who added the Exchange support which has proven to give undesired results, like in this case for "lying" (your words) servers. So why would Ben not be the best candidate to submit patches for us to review? Instead of the other way round: You do the work and have trouble to find a good reviewer. That should have been the agreement with Ben in the first place: TB features his add-on where appropriate and he commits to fixing issues that arise from that.

Since he has a vetted commercial interest in seeing patches in this direction delayed/rejected, excuse me for being sceptical that a timely, acceptable patch will emerge, or be reviewed should someone else submit one.

But alright, I can take stab at probing the found exchange server.

We have plenty to do, so can split up the work. Like bug 1599029 where we don't have to argue about whether a policy is right or not? The original patch in this bug fixes that too, as bug 1598945.

TB features his add-on where appropriate and he commits to fixing issues that arise from that.

See comment 4.

This is the traditional ISP setup where the SMTP will work only inside the ISP network, which is less useful nowadays with laptops that move around.
But it's still used by some, so it's still there. mail.aalto.fi is the modern smtp which will work for users not inside the local network too.

This is a very good example of how complex the matter of autoconfig is.

If you think about this a little, you will see why patches with wide-ranging effects (like here and in other bugs recently) are dangerous and cause hard-to-detect regressions. We would never know that the SMTP server doesn't work in all cases, unless end users report it to us. Even for users, it's difficult to know what happens. They test their setup, it works, they go on the road, and they need it, but it doesn't work. Awful. That's what we try to avoid. That is why the "poking around" is the absolute last resort.

That's also we should not override the email server.

This is simply a case where the email server response is misconfigured. It should be handled on this level, instead of potentially breaking lots of users that would work otherwise.

The autodiscover file lies

Then just fix it? This is your University. You keep saying users should advocate their users. Please go ahead and do yourself what you ask our users to do.

I might have them fix it, but I want to have it as a test case until it's fixed for the general scenario.

(In reply to Magnus Melin [:mkmelin] from comment #35)

Since he has a vetted commercial interest in seeing patches in this direction delayed/rejected, excuse me for being sceptical that a timely, acceptable patch will emerge, or be reviewed should someone else submit one.

Hmm, I think we should stop seeing bad intentions everywhere. Ben offered to address the issue, so we should give him at least 48 hours.

But alright, I can take stab at probing the found exchange server.

I don't think it's alright to quarrel over who will fix it. By the looks of it, we'll need to do a TB 68.3.1 to clean up the entire area. Too late fro TB 68.3 now :-(

We have plenty to do, so can split up the work. Like bug 1599029 where we don't have to argue about whether a policy is right or not? The original patch in this bug fixes that too, as bug 1598945.

Great, while I'm on PTO, why don't you guys agree on who fixes what. Here we need to probe the AutoDiscover server and in bug 1599029 we need to make sure we don't go off the rail when the server only advertises IMAP and SMTP in its own config file. I think bug 1598945 needs a subtler fix. Sure, not going down the Exchange path with fix the issue.

I might have them fix it, but I want to have it as a test case until it's fixed for the general scenario.

They listen to you? That's good news. In case the react to my ticket and fix their server, you can still fake it in the code to force it down another code path, or hardcode the wrong XML file we have.

I truly think we should invest into an O365 subscription so we'll have a (virtual) server available we can tweak to our hearts content.

Attached patch bug1598861_exchange_lies.patch (obsolete) (deleted) — Splinter Review

Implements probing when Autodiscovery lies using the server name found.
My test case is very fast for this, I barely have time to click ok before the (working) result is in.

I do think this code needs a serious refactoring to use async + promises. It's callback hell at its finest :(

Jörg, please take a look at your convenience.

Attachment #9111081 - Attachment is obsolete: true
Attachment #9111560 - Flags: feedback?(ben.bucksch)
Status: NEW → ASSIGNED

Hey Magnus, this is at least better than the previous patch. I have a few review comments, but I'm not at the office. Could this please wait a bit until next week? This code is a year old, so a week shouldn't make a difference.

That said, I still think we should honor the server, per my above comments.

Jörg, please take a look at your convenience.

Hmm, what would you like me to look at? I don't have an account at aalto.fi. Using invalid credentials I first get the domain login box, and then after a while it offers the imap. and smtp. servers. So that hasn't changed. I guess it's better with the right credentials. A little annoying that the manual config button stays disabled.

Alias: Aalto-missing-IMAP

You can test the other cases, and look at what the code is doing.

So can we move on with the review and get this landed so not all the issues are left lingering? Ben?

Compare bug 1218062, where guessing leads to a non-working config.

The patch here ignores the configs returned by AutoDiscover, hides Exchange, and shows only the guessed IMAP server. Even though that works for aalto.fi, that is dangerous in the general case and can often lead to non-working configs.

I'll take a closer look at this next week.

As you can see from bug 1218062 comment 2 even there he did manage to get it working (with IMAP).

Comment on attachment 9111560 [details] [diff] [review] bug1598861_exchange_lies.patch Review of attachment 9111560 [details] [diff] [review]: ----------------------------------------------------------------- Purely technical code review: If we want to do this, this is generally the right approach. But the code is in the wrong place. Technical code comments below. They should be fairly straight-forward to fix. r-, simply because some code changes are necessary. ::: mail/components/accountcreation/content/emailWizard.js @@ +693,5 @@ > domain, > emailAddress, > this._exchangeUsername, > this._password, > + config => { All this code below should not be here, but in one of the config method implementation files. In this case, it should be in exchangeAutoDiscover.js . Also, make this a separate function, e.g. `guessIMAPbasedOnAutoDiscover(config, successCallback)` or something descriptive, so that it's self-contained and the reader can quickly ignore all of it, if it's not interesting to him. @@ +699,5 @@ > + let alts = [config.incoming, ...config.incomingAlternatives]; > + if (alts.find(alt => alt.type == "imap" || alt.type == "pop3")) { > + // Autodiscover found an exchange server with advertized IMAP and/or > + // POP3 support. We're done then. > + call.successCallback()(config); This becomes just `successConfig(config)` after being moved into the impl file. Same for several other instances below. @@ +705,5 @@ > + } > + > + // Autodiscover is known to lie in its advertising. Let the probing > + // check see if there really isn't any IMAP/POP3 support by probing > + // the Exchange server. Use the server hostname already found. Excellent comment, but drop the "lie". It can be a deliberate policy question to let users use Exchange protocols. @@ +706,5 @@ > + > + // Autodiscover is known to lie in its advertising. Let the probing > + // check see if there really isn't any IMAP/POP3 support by probing > + // the Exchange server. Use the server hostname already found. > + let config2 = new AccountConfig(); Don't open a new config, but call `this.createNewIncoming()` (and outgoing), then make them default as appropriate. The design of the config is to capture all possible configs, not just the one that we want to be used. We capture everything that is possible. Also, create the config only after `guessConfig()` being successful. Otherwise, there's no point, given that `guessConfig()` doesn't need it as input. ::: mail/components/accountcreation/content/fetchhttp.js @@ +301,5 @@ > errorCode = -2; > errorStr = getStringBundle( > "chrome://messenger/locale/accountCreationUtil.properties" > ).GetStringFromName("cannot_contact_server.error"); > + ddump(errorStr + ": " + this._url); Make this `ddump(errorStr + " on <" + this._url + ">"); ::: mail/components/accountcreation/content/guessConfig.js @@ +134,5 @@ > > var errorInCallback = function(e) { > + // The caller's errorCallback threw. Hopefully shouldn't happen for users. > + console.trace(); > + // XXX rework this :( You should not end up here. If you did, this shows that the other code is either in the wrong place or desperately needs a try/catch to handle the errors properly.
Attachment #9111560 - Flags: review-
Comment on attachment 9111560 [details] [diff] [review] bug1598861_exchange_lies.patch In the case of aalto.fi, this might be fine, but we cannot know that for all servers. `guessConfig()` only checks whether the IMAP port is open, not whether the login actually works. Feedback-, because we would be going directly against the wish of the authoritative server.
Attachment #9111560 - Flags: feedback?(ben.bucksch) → feedback-

I ran a survey of over 22000 Exchange servers, and only 1% of the servers had an IMAP port (143 or 993) open. (And even if it's open, we cannot know whether it will be allowed for a given user.) Even in terms of users, the vast majority cannot use IMAP.

(In reply to Ben Bucksch (:BenB) from comment #45)

Also, create the config only after guessConfig() being successful.
Otherwise, there's no point, given that guessConfig() doesn't need it as
input.

No, passing in an appropriate config this way is how we pass on what server name(s) to probe specifically.

You should not end up here.
If you did, this shows that the other code is either in the wrong place or
desperately needs a try/catch to handle the errors properly.

Well that was pre debugging. It's utterly useless trying to debug out what went wrong without seeing a thing.

Attached patch bug1598861_exchange_lies.patch (obsolete) (deleted) — Splinter Review

Moved into the exchange file. Jörg, do you want to do the review, or should I ask Alex?

Attachment #9111560 - Attachment is obsolete: true
Comment on attachment 9113436 [details] [diff] [review] bug1598861_exchange_lies.patch Well, I understand that Ben is the initial author of the whole module, so I don't see why he can't do a first round of review here. After all, you're addressing his comments, no?
Attachment #9113436 - Flags: review?(ben.bucksch)

Yes, but I already addressed them to the extent reasonable

Obviously I don't have an aalto.fi account, so I could only try this with an invalid password. In this case the Exchange "Your login" box pops up and then after a few seconds the system claims that it found IMAP/POP3 by trying common server names. Well, that's not quite right, since it probed the AutoDiscover server. Personally I'm not a big fan of completely ignoring the server reply as far as the offered protocols are concerned. This is also not configurable should we decide later that we want to show the Exchange option.

Comment on attachment 9113436 [details] [diff] [review] bug1598861_exchange_lies.patch Review of attachment 9113436 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the review fixes. A few issues remain. ::: mail/components/accountcreation/content/emailWizard.js @@ +749,5 @@ > Services.io.offline > ? "guessed_settings_offline" > : "found_settings_guess" > ); > + self.resizeDialog(); BTW: Thanks for finding this. (This would be fixed with ES6 style arrow functions. I'm so happy they exist now!) ::: mail/components/accountcreation/content/exchangeAutoDiscover.js @@ +94,5 @@ > let call; > let fetch; > let fetch3; > > + let autodiscoverSuccessCallback = config => { Don't put this inline. That makes it hard to skip and read the primary code flow. Please make a new proper top-level function called `guessIMAPbasedOnAutoDiscover(config, successCallback)`, and place it at the end of the file. @@ +133,5 @@ > + // Probing didn't find any open protocols. > + // Let's use the exchange (only) config that was listed then. > + successCallback(config); > + }, > + config2, > passing in an appropriate config this way is how we pass on what server name(s) to probe specifically. Yes, you're right. Sorry, I missed the last parameter here. What you want to do is: ``` let config2 = config.copy(); config2.incomingAlternatives.unshift(config.incoming); // type=exchange ``` (unshift instead of push, because alternatives are ordered and config.incoming is the most preferred one, so it should become the first alternative.) @@ +136,5 @@ > + }, > + config2, > + "both" > + ); > + }; Your function needs a try/catch. If anything fails here, you don't want to error out, but use the config that we got. Just call `console.error(ex)` and then `successCallback(config)`. (I presume here that you moved this into a proper function.) That should solve your issue below with the error callback throwing. ::: mail/components/accountcreation/content/guessConfig.js @@ +134,5 @@ > > var errorInCallback = function(e) { > + // The caller's errorCallback threw. Hopefully shouldn't happen for users. > + console.trace(); > + // XXX rework this :( If you end up here, you're in an error callback, and you tried to run some complex code that tries to recover from the error. Which means you should try/catch that code, and explicitly deal with the fact that your recover failed. Probably, you want to pass the original error back to the caller. The fact that you get a end up here shows that your error handling has a bug.
Attachment #9113436 - Flags: review?(ben.bucksch)
Attachment #9113436 - Flags: review-
Attachment #9113436 - Flags: feedback-

In this case the Exchange "Your login" box pops up and then after a few seconds the system claims that it found IMAP/POP3 by trying common server names. Well, that's not quite right

That was exactly bug 1593122 and should be fixed now.

Personally I'm not a big fan of completely ignoring the server reply as far as the offered protocols are concerned. This is also not configurable should we decide later that we want to show the Exchange option.

Ditto.

That was exactly bug 1593122 and should be fixed now.

Sorry, since I'm on PTO. I haven't rebuilt. I'll do that now.

OK, I've rebuilt now. With invalid credentials at aalto@fi I get the "your login" prompt and nothing else. So I guess this will be hard to test without an actual login. Ben, have you tried the patch?

(In reply to Jorg K (GMT+1) (PTO to 15th Dec 2019, sporadically reading bugmail) from comment #52)

Well, that's not quite right, since it probed the AutoDiscover server.

But didn't find anything (since finding needs proper credentials).

Personally I'm not a big fan of completely ignoring the server reply as far as the offered protocols are concerned. This is also not configurable should we decide later that we want to show the Exchange option.

Well, this patch doesn't even remove the exchange option. It just adds the other protocols if/when found, so that's no problem.

As I said, impossible to test this patch for Magnus' "private" server. All I can say is that without credentials and without the patch from bug 1593122, I got the login box, and after a few seconds, IMAP and POP3, no exchange. With bug 1593122 included, it now "hangs" on the first screen with the added login box. How would you like anyone but you do a qualified review and test? At the very least you could submit a screenshot.

I've just sent a screen recording to you and Ben. Check your email.

With wrong creds, on trunk and with this patch too, I get the login box appearing - nothing else. Yes, that's why we need to get a proper message showing there to explain what happened (so bug 1593122 is still open). That without patches you got probed results coming in late showing up is for another bug.

Well, then the way forward would be to finish off bug 1593122 and address the issues from comment #53.

(In reply to Ben Bucksch (:BenB) from comment #53)

  • let autodiscoverSuccessCallback = config => {

Don't put this inline.

It's an internal functionality, so I don't agree. But sure, I can move it out.

What you want to do is:

let config2 = config.copy();
config2.incomingAlternatives.unshift(config.incoming); // type=exchange

(unshift instead of push, because alternatives are ordered and
config.incoming is the most preferred one, so it should become the first
alternative.)

I specifically won't do that. If we find IMAP, that will be the preferred option and we should not then list exchange first.

Your function needs a try/catch. If anything fails here, you don't want to
error out, but use the config that we got.

There's really nothing to put into a try-catch. But it will do what you suggest anyway.

The fact that you get a end up here shows that your error handling has a bug.

I don't end up there anymore. I did during debugging, and it was not useful not to get any trace of what was the cause. So I added the trace there. It doesn't change the functionality or error handling in any way.

Attached patch bug1598861_exchange_lies.patch (obsolete) (deleted) — Splinter Review

Updated to review comments

Attachment #9113436 - Attachment is obsolete: true

(In reply to Jorg K (GMT+1) (PTO to 15th Dec 2019, sporadically reading bugmail) from comment #60)

Well, then the way forward would be to finish off bug 1593122 and address the issues from comment #53.

That's just improving handling of the error case. This one is improving the success case. So I don't see a connection to that bug.

Comment on attachment 9113748 [details] [diff] [review] bug1598861_exchange_lies.patch So you want this reviewed/approved based on a video without any testing? How hard can it be to cover at least the testable case even if the error message is still disputed? Also given that the other bug already had code for it. I'd be happy with a dummy message for now.
Attachment #9113748 - Flags: review?(ben.bucksch)

let config2 = config.copy();
config2.incomingAlternatives.unshift(config.incoming); // type=exchange

I specifically won't do that. If we find IMAP, that will be the preferred option and we should not then list exchange first.

I understood that, and that's what I thought my proposal does. Maybe this is a misunderstand by either you or me.

The config.incoming is the preferred one that will be selected by default. The config.incomingAlternatives[*] are alternatives configs. I.e. the first entry in config.incomingAlternatives[] is not the preferred option, but the alternative. That's why I proposed this.

I don't remember exactly what guessConfig does, but I thought it would put the config it found into the config.incoming. I might be, wrong though.

There's really nothing to put into a try-catch. But it will do what you suggest anyway

Thanks. Yes, given that you're trying to enhance the result, if that fails, then just our attempts to find IMAP failed and that's not an error for the overall process, so all you can do is console.error(ex) and otherwise ignore the error.

So I added the trace there. It doesn't change the functionality or error handling in any way.

Ah, I see. If you want to add a console.error(ex) at that point, that's fine with me.

Comment on attachment 9113748 [details] [diff] [review] bug1598861_exchange_lies.patch Review of attachment 9113748 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/accountcreation/content/exchangeAutoDiscover.js @@ +102,5 @@ > + // Autodiscover found an exchange server with advertized IMAP and/or > + // POP3 support. We're done then. > + successCallback(config); > + return; > + } If you can move the above part into the function as well (it's a classic "nothing to do here" check at the beginning of a function, so that's a normal patter to put it inside, and it's tightly linked to the purpose of workings of that function), then we can drop this entire block here and just call `config => detectStandardProtocols(config, domain, successCallback)` instead of `autodiscoverSuccessCallback`. @@ +531,5 @@ > + * will be called when we could retrieve a configuration. > + * The AccountConfig object will be passed in as first parameter. > + */ > +function detectStandardProtocols(config, domain, successCallback) { > + let config2 = new AccountConfig(); I think my suggestion will do what you want here. ::: mail/components/accountcreation/content/guessConfig.js @@ +134,5 @@ > > var errorInCallback = function(e) { > + // The caller's errorCallback threw. Hopefully shouldn't happen for users. > + console.trace(); > + // XXX rework this :( Just an `console.error(ex)` should do. Drop the XXX, because there's nothing wrong here.
Attachment #9113748 - Flags: review?(ben.bucksch)
Attachment #9113748 - Flags: review-
Attachment #9113748 - Flags: feedback-

(In reply to Ben Bucksch (:BenB) from comment #65)

Thanks. Yes, given that you're trying to enhance the result, if that fails, then just our attempts to find IMAP failed and that's not an error for the overall process, so all you can do is console.error(ex) and otherwise ignore the error.

There's no such case to be found. We always call the success, only with different parameters.

Attached patch bug1598861_exchange_lies.patch (obsolete) (deleted) — Splinter Review
Attachment #9113748 - Attachment is obsolete: true
Attachment #9113913 - Flags: feedback?(ben.bucksch)

So any final nits? I want this landed today or tomorrow at the latest. If you wont review by then, let me know so I can find someone else.

Comment on attachment 9113913 [details] [diff] [review] bug1598861_exchange_lies.patch As said, please use `let config2 = config.copy()`, or explain why it's technically impossible. Please fix that. Then I need to try the new patch out and see how well it works before this can land. > I want this landed today or tomorrow at the latest. Independent from the code quality, this must not land unless explicitly discussed and approved by TB Council. This patch is **going directly against the instructions that the authoritative origin server** gave us. Your aalto.fi server is a big exception in that it offers IMAP.
Attachment #9113913 - Flags: review-
Attachment #9113913 - Flags: feedback?(ben.bucksch)
Attachment #9113913 - Flags: feedback-

Using copy() makes no sense at all. I'd have to then put 10 lines to explicitly wipe out the properties I don't want. Only the hostname should remain when we go into the probing.

But I see it's pointless to bring forwards what's technically correct. If you think some approval is needed to "allow" Thunderbird use IMAP, think again.

Can we stop wasting time here.
Jörg, do you want to review today, or should I find someone else?

I'm not an expert in account setup and I'm also trying to be on PTO, without much success :-( - So please find an appropriate reviewer.

Comment on attachment 9113913 [details] [diff] [review] bug1598861_exchange_lies.patch Review of attachment 9113913 [details] [diff] [review]: ----------------------------------------------------------------- Not easy to test. Alex, I'll send you a screen capture.
Attachment #9113913 - Flags: review- → review?(alessandro)
Comment on attachment 9113913 [details] [diff] [review] bug1598861_exchange_lies.patch Patch is still technically buggy, because you're dropping existing alternatives. In addition to being a disregard of the authorative server.
Attachment #9113913 - Flags: review-

Right now, in the success case, you're dropping all the alternative configs in config.

There might also be other non-mail related properties on there. Why can you not use .copy() and .createNewIncoming()?

(In reply to Ben Bucksch (:BenB) from comment #74)

Patch is still technically buggy, because you're dropping existing alternatives.

Only in theory. We'd never end up there if there were alternatives in the first place. But sure, I can add the the empty list there if you prefer.

There might also be other non-mail related properties on there. Why can you not use .copy() and .createNewIncoming()?

Which exactly would that be, that we'd want to keep? This is what exchange autodiscover sets up: https://searchfox.org/comm-central/rev/065eb16ca361300b4b116f6c69ebd10ea1e6c1d0/mail/components/accountcreation/content/exchangeAutoDiscover.js#231
I'll set them all up. It's still easier than switching around objects and properties on a copy. (BTW, oauthSettings seems misplaced and should be put on a server, but that's for later.)

The AccountConfig constructor already calls createNewIncoming() - https://searchfox.org/comm-central/rev/065eb16ca361300b4b116f6c69ebd10ea1e6c1d0/mail/components/accountcreation/content/accountConfig.js#25

Attached patch bug1598861_exchange_lies.patch (deleted) — Splinter Review
Attachment #9113913 - Attachment is obsolete: true
Attachment #9113913 - Flags: review?(alessandro)
Attachment #9115153 - Flags: review?(alessandro)

Ben, if you think there are technical details in Magnus' patch that should be done differently, please submit a patch and let Magnus test it.

Flags: needinfo?(ben.bucksch)

I tested this on 68 we with the 2 Office365 (IMAP enabled and disabled) accounts Ben provided.

In both cases the dialog pre-selects Exchange, even if the credentials are correct and IMAP is available.

The 3 options are all visible and selectable, but of course if I select IMAP with the account where IMAP is disabled, the final config process doesn't continue and the dialog remains open and stuck.

Is this the outcome you wanted Magnus?

Hi Alessandro, you cannot test this with Office365, because this goes through ISPDB and not the AutoDiscover process, while this patch is specifically for the AutoDiscover process.

You need to test this with aalto.fi or another on-premise Exchange server which has IMAP ports enabled. You do not need credentials to test this bug.

Flags: needinfo?(ben.bucksch)

Great we have everybody confused here :-(

You can only test this bug with proper credentials, so in fact, no one but Magnus can test it. He sent a video around showing it working.
If you enter MM@aalto.fi with password MM, you get the new messages from bug 1593122, see comment #58 and below.

Ignore Ben from comment #80 "You do not need credentials to test this bug". That's 100% wrong.

Ben, don't cancel NI without answering the question, see comment #78.

Flags: needinfo?(ben.bucksch)

(In reply to Alessandro Castellani (:aleca) from comment #79)

In both cases the dialog pre-selects Exchange, even if the credentials are correct and IMAP is available.

Not wanted but expected.
This bug only adds the IMAP/POP options for the case where the server lied. It doesn't handle any reordering or hiding.

You can only test this bug with proper credentials, so in fact, no one but Magnus can test it

Sorry for the confusion I caused. I've updated my comment above.

Flags: needinfo?(ben.bucksch)

You're continuing the confusion :-( - Alex tested with the O365 accounts. They use the ISPDB and this file which lists Exchange first:
https://github.com/thundernest/autoconfig/blob/master/ispdb/office365.com
So this bug DOEST NOT add anything, this bug is about on-premise Exchange servers which don't advertise IMAP/POP3 although it's working.

This bug has the alias Aalto-missing-IMAP. You should test it on the Aalto.fi server, but please read comment #81.

I'm in a Kindergarten here. Ben, please answer comment #78 unless you're now accepting (EDIT: the technical details of) Magnus' solution.

Flags: needinfo?(ben.bucksch)

Alex tested with the O365 accounts. They use the ISPDB ... So this bug DOEST NOT add anything, this bug is about
on-premise Exchange servers which don't advertise IMAP/POP3 although its working.

Yes, that's what I wrote in comment 81. Thanks for pointing out my mistake in the last phrase.

Ben, please answer comment #78 unless you're now accepting Magnus' solution

I've already answered that, 3-4 times, in my reviews above (search for copy()), and gave specific and helpful code instructions -- even though I think this whole bug is wrong. I nonetheless made swift reviews, and stayed with technical comments only. I don't think a reviewer should be required to make a new patch.

I don't think a reviewer should be required to make a new patch.

It could help to show how you think it should be done and let Magnus test it. That's also common procedure: A reviewer takes a patch, adjusts it slightly and then asks for feedback from the original author. I could show you many cases of that.

Comment on attachment 9115153 [details] [diff] [review] bug1598861_exchange_lies.patch Review of attachment 9115153 [details] [diff] [review]: ----------------------------------------------------------------- Thank you all for the detailed info. This is a r+ for me as it enables the missing options if available.
Attachment #9115153 - Flags: review?(alessandro) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/a0109bb8169e
make sure to probe a found exchange server whether it supports IMAP/POP3. r=aleca

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 73.0

Just for completeness. Even O365 is "misconfigured" in this way. This is a sample of the autodiscover.xml when IMAP is enabled. No IMAP listed there.

Attachment #9115153 - Flags: approval-comm-beta+

Umm, is there going to be a TB 72 beta 3 with these bugs https://mzl.la/2QiqTAL? I doubt it.

I think that's the plan.

Target Milestone: Thunderbird 73.0 → Thunderbird 72.0
Target Milestone: Thunderbird 72.0 → Thunderbird 73.0
Flags: needinfo?(ben.bucksch)

Safe and ready for ESR? Any prerequisites?

Flags: needinfo?(mkmelin+mozilla)
Comment on attachment 9115153 [details] [diff] [review] bug1598861_exchange_lies.patch Ready to go.
Flags: needinfo?(mkmelin+mozilla)
Attachment #9115153 - Flags: approval-comm-esr68?
Comment on attachment 9115153 [details] [diff] [review] bug1598861_exchange_lies.patch Approved for ESR
Attachment #9115153 - Flags: approval-comm-esr68? → approval-comm-esr68+

(In reply to Magnus Melin [:mkmelin] from comment #97)

Comment on attachment 9115153 [details] [diff] [review]
bug1598861_exchange_lies.patch

Ready to go.

This did not apply cleanly on esr68.

Flags: needinfo?(mkmelin+mozilla)

I think I got it.

Flags: needinfo?(mkmelin+mozilla)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: