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)
Tracking
(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)
(deleted),
image/png
|
Details | |
(deleted),
text/xml
|
Details | |
(deleted),
patch
|
aleca
:
review+
mkmelin
:
approval-comm-beta+
wsmwk
:
approval-comm-esr68+
|
Details | Diff | Splinter Review |
(deleted),
text/xml
|
Details |
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.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
If you present incorrect creds, you'll get the IMAP and POP options.
Comment 2•5 years ago
|
||
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.
Comment 3•5 years ago
|
||
@Magnus: Please CC me on such bugs.
Comment 4•5 years ago
|
||
If this indeed is a bug and doesn't work as discussed and intended in bug 1500105, I'll fix this.
Assignee | ||
Comment 5•5 years ago
|
||
AutoDiscover.xml response.
Comment 6•5 years ago
|
||
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.
Comment 7•5 years ago
|
||
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.
Comment 8•5 years ago
|
||
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.
Comment 9•5 years ago
|
||
We could add an ISPDB file or does AutoDiscover override that?
Assignee | ||
Comment 10•5 years ago
|
||
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.
Assignee | ||
Comment 11•5 years ago
|
||
(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.
Comment 12•5 years ago
|
||
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
Comment 13•5 years ago
|
||
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.
Assignee | ||
Comment 14•5 years ago
|
||
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.
Comment 15•5 years ago
|
||
not to set up a configuration file
But here, there is one and it's telling us not to use IMAP.
Comment 16•5 years ago
|
||
Comment 17•5 years ago
|
||
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,
Assignee | ||
Comment 18•5 years ago
|
||
Eh, of course things are fixable on a per provider basis. There's no reason not to fix it for the general case.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 19•5 years ago
|
||
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 | ||
Comment 20•5 years ago
|
||
Rouge proof of concept. Works but may need some adjustments.
Assignee | ||
Comment 21•5 years ago
|
||
Fixes bug 1598945 too.
Comment 22•5 years ago
|
||
Comment 23•5 years ago
|
||
I am awaiting the admin response.
Comment 24•5 years ago
|
||
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.
Assignee | ||
Comment 25•5 years ago
|
||
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.
Comment 26•5 years ago
|
||
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".
Assignee | ||
Comment 27•5 years ago
|
||
(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.
Comment 28•5 years ago
|
||
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.
Assignee | ||
Comment 29•5 years ago
|
||
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.
Comment 30•5 years ago
|
||
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.
Assignee | ||
Comment 31•5 years ago
|
||
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.
Comment 32•5 years ago
|
||
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.
Assignee | ||
Comment 33•5 years ago
|
||
(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.
Comment 34•5 years ago
|
||
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.
Assignee | ||
Comment 35•5 years ago
|
||
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.
Comment 36•5 years ago
|
||
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.
Assignee | ||
Comment 37•5 years ago
|
||
I might have them fix it, but I want to have it as a test case until it's fixed for the general scenario.
Comment 38•5 years ago
|
||
(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.
Assignee | ||
Comment 39•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Comment 40•5 years ago
|
||
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.
Comment 41•5 years ago
|
||
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.
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 42•5 years ago
|
||
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?
Comment 43•5 years ago
|
||
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.
Assignee | ||
Comment 44•5 years ago
|
||
As you can see from bug 1218062 comment 2 even there he did manage to get it working (with IMAP).
Comment 45•5 years ago
|
||
Comment 46•5 years ago
|
||
Comment 47•5 years ago
|
||
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.
Assignee | ||
Comment 48•5 years ago
|
||
(In reply to Ben Bucksch (:BenB) from comment #45)
Also, create the config only after
guessConfig()
being successful.
Otherwise, there's no point, given thatguessConfig()
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.
Assignee | ||
Comment 49•5 years ago
|
||
Moved into the exchange file. Jörg, do you want to do the review, or should I ask Alex?
Comment 50•5 years ago
|
||
Assignee | ||
Comment 51•5 years ago
|
||
Yes, but I already addressed them to the extent reasonable
Comment 52•5 years ago
|
||
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 53•5 years ago
|
||
Comment 54•5 years ago
|
||
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.
Comment 55•5 years ago
|
||
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.
Comment 56•5 years ago
|
||
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?
Assignee | ||
Comment 57•5 years ago
|
||
(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.
Comment 58•5 years ago
|
||
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.
Assignee | ||
Comment 59•5 years ago
|
||
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.
Comment 60•5 years ago
|
||
Well, then the way forward would be to finish off bug 1593122 and address the issues from comment #53.
Assignee | ||
Comment 61•5 years ago
|
||
(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.
Assignee | ||
Comment 62•5 years ago
|
||
Updated to review comments
Assignee | ||
Comment 63•5 years ago
|
||
(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 64•5 years ago
|
||
Comment 65•5 years ago
|
||
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 66•5 years ago
|
||
Assignee | ||
Comment 67•5 years ago
|
||
(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.
Assignee | ||
Comment 68•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 69•5 years ago
|
||
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 70•5 years ago
|
||
Assignee | ||
Comment 71•5 years ago
|
||
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?
Comment 72•5 years ago
|
||
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.
Assignee | ||
Comment 73•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Comment 74•5 years ago
|
||
Comment 75•5 years ago
|
||
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()
?
Assignee | ||
Comment 76•5 years ago
|
||
(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
Assignee | ||
Comment 77•5 years ago
|
||
Comment 78•5 years ago
|
||
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.
Comment 79•5 years ago
|
||
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?
Comment 80•5 years ago
|
||
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.
Comment 81•5 years ago
|
||
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.
Assignee | ||
Comment 82•5 years ago
|
||
(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.
Comment 83•5 years ago
|
||
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.
Comment 84•5 years ago
|
||
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.
Comment 85•5 years ago
|
||
I'm in a Kindergarten here. Ben, please answer comment #78 unless you're now accepting (EDIT: the technical details of) Magnus' solution.
Comment 86•5 years ago
|
||
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.
Comment 87•5 years ago
|
||
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.
Comment 88•5 years ago
|
||
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 89•5 years ago
|
||
Comment 90•5 years ago
|
||
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
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 91•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Comment 92•5 years ago
|
||
Umm, is there going to be a TB 72 beta 3 with these bugs https://mzl.la/2QiqTAL? I doubt it.
Assignee | ||
Comment 93•5 years ago
|
||
I think that's the plan.
Comment 94•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 96•5 years ago
|
||
Safe and ready for ESR? Any prerequisites?
Assignee | ||
Comment 97•5 years ago
|
||
Comment 98•5 years ago
|
||
Comment 99•5 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #97)
Comment on attachment 9115153 [details] [diff] [review]
bug1598861_exchange_lies.patchReady to go.
This did not apply cleanly on esr68.
Assignee | ||
Comment 101•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Description
•