Closed Bug 534722 (ispconfig) Opened 15 years ago Closed 15 years ago

Fetch mail server config directly from ISP

Categories

(Thunderbird :: Account Manager, enhancement, P1)

Product:
Thunderbird
Component:
Account Manager
Type:
enhancement

Tracking

(blocking-thunderbird3.1 beta2+, thunderbird3.1 beta2-fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking-thunderbird3.1 --- beta2+
thunderbird3.1 --- beta2-fixed

People

(Reporter: BenB, Assigned: BenB)

References

Details

Attachments

(3 files, 3 obsolete files)

Test, v1 - not working, abandonned
3.46 KB, patch
Details | Diff | Splinter Review
Fix, v4 - only UI text changed
13.04 KB, patch
clarkbw
: ui-review+
Details | Diff | Splinter Review
Fix brandname string
3.66 KB, patch
bwinton
: review+
Details | Diff | Splinter Review
Implement step 2.1 from <https://wiki.mozilla.org/Thunderbird:Autoconfiguration#Implementation>: Try to get the configuration from the email provider 1. Try to contact https://autoconfig.emailaddressdomain/mail/mozilla.xml?emailaddress=emailaddress and see whether that host/URL exists. The returned file must have the ConfigFileFormat as below.
Blocks: 490234
Can we make sure to only do that on SSL protected links ? And I'd rather have us proxy things.
> Can we make sure to only do that on SSL protected links ? That's was the plan. It may not be feasible, though, because a domain hoster can't get a cert and IP address for each domain they host. SSL is very problematic wrt to that. We discussed that in the security review, I'll post the results later. > I'd rather have us proxy things. No, this is a thing between the user and his ISP. This was intentionally designed in a decentralized way, like email is. Central authorities which control everything are against the idea of the Internet and always end up being a problem for one reason or another - reliability, version problems, implementation delays or control.
(After all, this is just a better alternative to the guess heuristics, which are also based on the ISP mail server. And the mail server incl. CAPABILITY is also controlled by the ISP.)
> Central authorities which control everything are against the idea of the Internet and always end up being a problem So, why does step 3 even exist in https://wiki.mozilla.org/Thunderbird:Autoconfiguration#Implementation
> this is just a better alternative to the guess heuristics I agree with you there. I just tried pointing an A record for imap./domain/ at our IMAP server, but TB3 doesn't like it (presumably due to the cert mismatch.)
> So, why does step 3 even exist For the big ISPs - including hotmail.com - which don't set up a config server.
Will method 2 at https://wiki.mozilla.org/Thunderbird:Autoconfiguration#Implementation be implemented? This is my preferred solution as a very small provider with many domains. Method 3 doesn't apply to me, method 4 will probably fail as the certificate on my IMAP server matches secure.ispdomain.com, not imap.customeremaildomain.com. So here's a vote for method 2 from me.
>> Can we make sure to only do that on SSL protected links ? >> > That's was the plan. It may not be feasible, though, because a domain hoster > can't get a cert and IP address for each domain they host. SSL is very > problematic wrt to that. We discussed that in the security review, I'll post > the results later. I agree about the domain hoster limitation in obtaining certs for hosted domains. That is the problem we have. However, there is also the problem that domain owners are not willing to shell out for the additional cert, and/or they are not willing/capable of running a virtual host on their web server specifically for this purpose. Would this idea work? If https://autoconfig.emailaddressdomain/mail/mozilla.xml?emailaddress=emailaddress doesn't respond, then try https://emailaddressdomain/autoconfig/mail/mozilla.xml?emailaddress=emailaddress This might be more tolerable to those domains that don't want to shell out for an additional certificate. Additionally, how feasible would it be to support a redirect? i.e. so that https://emailaddressdomain/autoconfig/mail/mozilla.xml?emailaddress=emailaddress redirects (after the TLS negotiation) to fetch the config from https://hostingproviderdomain/autoconfig/mail/mozilla.xml?emailaddress=emailaddress&domain=domain. Or something like that.
>> So, why does step 3 even exist > > For the big ISPs - including hotmail.com - which don't set up a config server. How is "big ISP" defined? How do I know that my domains will not qualify as a "big ISP" and hence be susceptible to configuration poisoning?
>> I'd rather have us proxy things. > > No, this is a thing between the user and his ISP. I'm torn on this idea. Strictly speaking, that is the correct position to take. However, the "big ISPs" have an advantage by being listed in the ISPDB, while all the rest of us have to wait until this lookup mechanism is implemented in the client. Don't get me wrong, I don't want our domains to be listed in the ISPDB since I don't think that they have a secure verification process. However, if mozilla is trustworthy enough to provide a client that connects securely to the ISP's mail server, I think that it (via the ISPDB lookup) can be trusted to proxy the configuration data from the ISP as long as it does not overrule the results with data from the central ISPDB database. The benefit to this is that it might push adoption of the protocol while the logistics of implementation in the client are worked out.
> Will method 2 ... be implemented? So here's a vote for method 2 from me. Yes, that's what this bug is about, see initial description (comment 0). Please don't use this bug for discussion. We have already discussed this on the newsgroup, and the above documents and its subpages are the result. I'll post the results of the security review later.
discussed on Thunderbird weekly call that a solution along these lines needed at Lehigh before we can deploy version 3, i.e. 3.1 - we must skip 3.0. More to come. Nominating blocking 3.1 benb in comment #11) > I'll post the results of the security review later. we'll need to consider that information very soon if an implementation to be driven into 3.1
blocking-thunderbird3.1: --- → ?
The security review was a constructive and fruitful discussion Central db or ISP fetch has precedence? I think ISP should have precedence because I dislike central octopussy authorities, because they always fire back in the long term. bwinton preferred MozillaMessaging to control things. His suggestion was to add a "redirect" element feature to the config file format. The central database uses this to refer to the ISP for all domains for which we have no entry (or even for those for which we have an entry). If we dislike the ISP, we remove the redirect. Advantages (central): - able to change URL (but unlikely once deployed) - able to override in case it goes wrong for one ISP Disadvantages: - central authority, power question - downtime case - bug 537649 - implementation: redirects are recursive and not trivial in combination with async network fetches. Is SSL critical for config lookup? DNS MX: Mail delivery between domains happens via DNS MX lookups, which are insecure. In other words, an attacker can already re-route and intercept new mails. The risk of interception during account setup is not larger than that. More importantly: Guessed configs: An attacker can easily *prevent* connections, making the ispdb lookup (via https), isp fetch (via https) and heuristics for IMAP SSL servers and IMAP servers with secure auth mechs all fail, making us fall back to plaintext auth, and that's it, game over. Therefore, lookups via http don't add risk. Domain hosters and SSL Domain hosters and mail service providers which allow customers to buy their down domain and use that for email. Example: 1und1.de, fastmail.fm, Dreamhost, schlund.de etc. They want to set up an autoconfig server for their customers (incl. customers with their own domain). If we require SSL, we'd need to require that the cert domain matches the email domain, otherwise we'd rely on insecure steps like DNS MX lookups and can just as well drop the SSL requirement. However, it's not feasible for a domain hoster to get a cert for each customer domain. (Not feasible 1. because of verification processes and costs and 2. because of the SSL requirement to have one IP address per cert. Neither domain-wildcard certs nor certs listing several domains are applicable here.) So, we don't see a way to both support domain hosters and enforce the SSL requirement (and it being meaningful). Given that we think that domain hosters are an important case, and attackers can circumvent SSL anyways (see above), we'll drop SSL and allow http for ISP config looksup. I don't like it at all, but I don't see an alternative. (Note that trying SSL first and then falling back to http is snake oil / false sense of security, because an attacker can always prevent the https-connection, and there's no sensitive information in the config file either.) However, dropping the SSL requirement allows us to include other mechanisms like DNS SRV or MX records which could prove very useful (but not part of this bug). E.g. if a domain has a DNS SRV entry for _imap, we use that. Or if a MX for bucksch.com points to mx.fastmail.fm, we could restart config lookup with fastmail.fm as domain. References: Bug 342242, <http://tools.ietf.org/html/draft-daboo-srv-email-02> Blake, please add/correct stuff I forgot or misrepresented.
I forgot: re http[s]://autconfig.<domain>/ and philor's concern that somebody at a subdomain hoster (e.g. dreamhost handing out benbucksch.dreamhost.com to me) might grab autoconfig.<domain>: I already argued that we have the same problem with imap.<domain>. philor responded that imap. is a well-known name and is hopefully blocked for registration. I think that autoconfig can just as well become such a well-known block, but philor didn't buy that. However, Microsoft Outlook does exactly the same, they use http://autodiscover.<domain>/... . I am not aware of any incidents. Blake suggested that we use autodiscover.<domain>, given that this should be a well-known domain for hosters now, too. I think that's a good idea. (But I still don't think the original problem philor mentioned is real. If a domain hoster gives out autoconfig.dreamhost.com, that's their problem, and also easily rectified.)
Dan (our mail admin) in reference to our meeting and this bug writes "I don't particularly like 2.1 (of [1]) ( I would just use https://domain/mail/mozilla.xml rather than https://autoconfig.domain/....). 2.2 looks like what I was thinking - particularly the part about - https://wiki.mozilla.org/Thunderbird:Autoconfiguration:DNSBasedLookup. I don't really like 2.3, but could make it work. I think part 1 would meet Brad's idea of pre-configuring. Part 3 is for the larger isp's ( yahoo.com/hotmail.com/live.com and possibly gmail.com ). Part 4 needs some implementation corrections - it just doesn't work correctly/verify it's settings. Perhaps it should ask the user for their password, so it can test the config and actually see if it's guesses works. It should also be documented what it will guess, so corp's can pick reasonable settings and expect a high chance of it guessing it correctly." [1] https://wiki.mozilla.org/Thunderbird:Autoconfiguration
> Is SSL critical for config lookup? > DNS MX: Mail delivery between domains happens via DNS MX lookups, > which are insecure. In other words, an attacker can already > re-route and intercept new mails. The risk of interception during > account setup is not larger than that. That assumes that messages are more sensitive than account credentials. The risk of exposing some messages from one source is smaller than the risk of exposing credentials which gives someone unauthorized access to all messages in the account, not to mention that it gives spammers a platform to send spam using the ISP's trustworthy mail servers. > However, it's not feasible for a domain hoster to get a cert for each customer domain. I agree. This prevents Thunderbird from enforcing that the cert presented by the mail server match the domain. > I don't particularly like 2.1 (of [1]) ( I would just use > https://domain/mail/mozilla.xml rather than https://autoconfig.domain/....). I agree. Moreover, our hosted domain customers will not like the idea of purchasing a cert just for this one purpose. > Domain hosters and SSL I propose that you mandate https://domain/ (the https cert must match the email domain) and support redirecting. The hosted customer probably already has, or could easily set up, a web server with https support for their domain. Then, all they would need to do is create a mozilla.xml file that tells Thunderbird to use the configuration from a URL that is controlled by the hosting provider. > Part 4 needs some implementation corrections - it just doesn't work > correctly/verify it's settings. Perhaps it should ask the user for their > password, so it can test the config and actually see if it's guesses works. That would risk submitting credentials to an unknown party. I don't think that any credentials should be submitted unless Thunderbird *knows* that the server is correct. There is also a usability problem with domains that are split hosted. For instance, many EDU domains split their users across two mail systems (Google Apps and a local service.) This guessing approach makes too many incorrect assumptions about how mail services are set up in reality.
Jesse, I was merely posting the results of the security review. Please discuss this on the newsgroup. This is purely an implementation bug. (First word of initial description states that.)
Along those lines, here are the notes I took about the future directions of the Autoconfig. https://wiki.mozilla.org/Thunderbird:Autoconfiguration:NextSteps Later, Blake.
Jesse, I was merely posting the results of the security review. To make discussion easier, I posted comment 13 on the newsgroup, thread "Autoconfig ISP fetch security review". Please discuss this on the newsgroup. This is purely an implementation bug. (First word of initial description states that.) Please do not discuss here.
Since this is likely to be a pretty big win for organizations upgrading from Tb2 and my understanding is that it's a very small amount of effort, I think we should block on this. Since this is a feature and we'll need at least a little time for people to deploy this so they can give us feedback, aiming at beta 2.
blocking-thunderbird3.1: ? → beta2+
Which URLs to try: 1. Should we try https before http? Feels like the right thing to do, but as explained in comment 13, it's actually snake-oil: Any attacker could easily just block/suppress the SSL connection, and then give a bad response to our http request. The latter is harder than the former. So, it's completely pointless to try SSL, because in any interesting case, it wouldn't work. It only gives a false sense of security. I feel really bad not using SSL for this, but I see no other reasonable option. 2. autoconfig.domain or autodiscover.domain? Microsoft uses autodiscover.domain. This means it has technical advantages and marketing disadvantages: + The host may already be set up, the ISP just needs to place/serve another file on it. + The subdomain should already be well-known and blocked for registration by customers (if the ISP allows customers to register subdomains). - People may confuse the Microsoft autodiscover with our autoconfig. They are similar, but the formats are of course different (Microsoft's is a mess). - People may think we stole the idea from Microsoft, although I had the idea entirely independently. bwinton thinks we should use autodiscover, and I accept that. So, http://autodiscover.<domain>/mail/mozilla.xml is it. 3. Should we try http://domain/... before http://autodiscover.domain/... ? - Hard to change main webserver In some big companies I worked at, it's much easier to set up a new host than to get access to the webserver. + Easy to change main webserver In other companies, it's easier - sometimes considerably easier - to add a document to the existing webserver. - Many people have access Many non-technical people may have access to the webserver, e.g. marketeers and documentation writers. If they have a trojan/virus on their computer... - domain != host != web I hate it to assume that <http://domain> works. domain is a domain, not a host. The proper webserver is <http://www.domain>. - Trying random URLs is rude I hate to just invent URLs and blindly try them, causing errors in the web server's error log files. I think it's rude. I also absolutely hated favicon.ico for that reason, and I was not the only one (one friend of mine even served 500MB files on as favicon.ico, just to protest against MSIE's rudeness). There's <http://tools.ietf.org/html/draft-nottingham-site-meta-04>, but I don't think it's much better, and it's just a draft / proposal, not anyhow acccepted. - No security advantage Trying http://domain first doesn't help security-wise: If an ISP is aware of our autoconfig, they can register/block/revoke the autodiscover subdomain, and there's no problem. If they are not aware of it, they won't publish <http://domain/.../mozilla.xml> either. - Speed Any additional URLs we try increase the time that autoconfig takes. In particular, assuming the second-level domain (e.g. freenet.de) is resolved, it takes ~200ms to try the non-existing http://autodiscover.domain, but ~600ms to try the non-existing http://domain/... (i.e. 3 times as long, and that's the total time, incl. DNS, http, etc.). So, I think we should not try <http://domain>, but I'm not sure. (I'll attach patches for both with and without.)
So, this is the version where I try http://autodiscover.<domain>/mail/mozilla.xml and then http://<domain>/.well-known/autodiscover/mail/mozilla.xml . I also changed the strings so that the status messages say what we currently try. If there are strong objections to that, I can remove the string changes again.
Attachment #427578 - Flags: review?(bwinton)
(I hope I created the patches correctly, as I have other changes in the tree)
Attachment #427579 - Flags: review?(bwinton)
> Trying random URLs is rude If http://domain/ is a random URL, then how is http://autowhatever.domain/ not a random URL?
Failed DNS lookups don't cause log entries. 404s do cause error log entries.
> 2. autoconfig.domain or autodiscover.domain? > - People may confuse the Microsoft autodiscover with our autoconfig Sometimes, my worst fears gets surpassed by reality. An admin told me he doesn't want to set up that autodiscover. host for their domain "because they don't use Outlook anyways".
Comment on attachment 427578 [details] [diff] [review] Fix, v1 - with http://domain and string changes >+++ b/mail/locales/en-US/chrome/messenger/accountCreation.properties >@@ -7,20 +7,31 @@ selfsigned_warning=%1$S does not use a t > # LOCALIZATION NOTE(looking_up_settings): %1$S will be the brandShortName. >-looking_up_settings=%1$S is looking up the settings for your email account. >+looking_up_settings_disk=%1$S is looking up the settings for your email account on your harddisk. I think that if you change the name of the string, you need to also change it in the LOCALIZATION NOTE parameter above. >+# LOCALIZATION NOTE(looking_up_settings): %1$S will be the brandShortName. >+looking_up_settings_isp=%1$S is looking up the settings for your email account at your mail provider. >+# LOCALIZATION NOTE(looking_up_settings): %1$S will be the brandShortName. >+looking_up_settings_db=%1$S is looking up the settings for your email account at the Mozilla database. I might change these to to be "from your mail provider/from the Mozilla database". >+# LOCALIZATION NOTE(looking_up_settings): %1$S will be the brandShortName. >+looking_up_settings_guess=%1$S is trying to guess the settings for your email account. I always thought of it as more probing than guessing… Could we find some wording that makes it sound more like we've got a plan, and less like we're trying stuff at random? ;) > manually_edit_config=Editing Config > # LOCALIZATION NOTE(found_settings): %1$S will be the brandShortName. >-found_settings=%1$S has found the settings for your email account. >+found_settings_disk=%1$S has found the settings for your email account on your harddisk. >+# LOCALIZATION NOTE(found_settings): %1$S will be the brandShortName. >+found_settings_db=%1$S has found the settings for your email account in the Mozilla database. >+found_settings_isp=Your mail provider has published the settings for your email account. To be consistent with the looking_up_settings, the above two items should be switched. And we probably want to mention somewhere in the _isp text that those are the settings we'll be using. >+# LOCALIZATION NOTE(found_settings): %1$S will be the brandShortName. >+found_settings_guess=%1$S has guessed the settings for your email account. Ditto with the not-using-the-word-"guess". >+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.js >@@ -411,55 +411,71 @@ EmailConfigWizard.prototype = > findConfig : function(domain, email) > { > gEmailWizardLogger.info("findConfig()"); >- this.startSpinner("all", "looking_up_settings"); > if (this._probeAbortable) > { > gEmailWizardLogger.info("aborting existing config search"); > this._probeAbortable.cancel(); > } >+ this.startSpinner("all", "looking_up_settings_disk"); > var me = this; > this._probeAbortable = > fetchConfigFromDisk( > domain, > function(config) // success > { > me.foundConfig(config); >- me.stopSpinner("found_settings"); >+ me.stopSpinner("found_settings_disk"); > me._probeAbortable = null; > }, > function(e) // fetchConfigFromDisk failed > { > gEmailWizardLogger.info("fetchConfigFromDisk failed: " + e); >- me.startSpinner("all", "looking_up_settings"); >+ me.startSpinner("all", "looking_up_settings_isp"); > me._probeAbortable = >- fetchConfigFromDB( >+ fetchConfigFromISP( > domain, >+ email, > function(config) // success > { > me.foundConfig(config); >- me.stopSpinner("found_settings"); >+ me.stopSpinner("found_settings_isp"); > me.showEditButton(); > me._probeAbortable = null; > }, >- function(e) // fetchConfigFromDB failed >+ function(e) // fetchConfigFromISP failed > { >- gEmailWizardLogger.info("fetchConfigFromDB failed: " + e); >- var initialConfig = new AccountConfig(); >- me._prefillConfig(initialConfig); >- me.startSpinner("all", "looking_up_settings") >- me._guessConfig(domain, initialConfig, 'both'); >+ gEmailWizardLogger.info("fetchConfigFromISP failed: " + e); >+ me.startSpinner("all", "looking_up_settings_db"); >+ me._probeAbortable = And there's a trailing space at the end of this line. ;) >+ fetchConfigFromDB( >+ domain, >+ function(config) // success >+ { >+ me.foundConfig(config); >+ me.stopSpinner("found_settings_db"); >+ me.showEditButton(); >+ me._probeAbortable = null; >+ }, >+ function(e) // fetchConfigFromDB failed >+ { >+ gEmailWizardLogger.info("fetchConfigFromDB failed: " + e); >+ var initialConfig = new AccountConfig(); >+ me._prefillConfig(initialConfig); >+ me.startSpinner("all", "looking_up_settings_guess") >+ me._guessConfig(domain, initialConfig, 'both'); Please change the 's to "s. Also, this four-levels-of-indentation thing is starting to annoy me. What do you think about making some of those into named local functions? >+++ b/mailnews/base/prefs/content/accountcreation/fetchConfig.js >@@ -65,30 +65,54 @@ function fetchConfigFromDisk(domain, suc > function fetchConfigFromISP(domain, emailAddress, successCallback, > errorCallback) > { >- let url = "https://autoconfig." + sanitize.hostname(domain) + >+ let url1 = "http://autodiscover." + sanitize.hostname(domain) + > "/mail/mozilla.xml"; So, I kind of hope that, while we are the first people to use this xml file, the format is generic enough to be useful to other organizations, and so naming it something other than "mozilla.xml" would be a good idea. (And if we end up adding some mozilla-specific things inside of it, we can put them in a namespace, or in a custom <mozillaextensions> tag. And after chatting about it, I think I prefer autoconfig to autodiscover, so if you want to make that change, too, I'ld appreciate it. >- let fetch = new FetchHTTP(url, { emailaddress: emailAddress }, false, >- function(result) >- { >- successCallback(readFromXML(result)); >- }, >- errorCallback); >- fetch.start(); >- return fetch; >+ // .well-known/ <http://tools.ietf.org/html/draft-nottingham-site-meta-04> >+ let url2 = "http://" + sanitize.hostname(domain) + >+ "/.well-known/autodiscover/mail/mozilla.xml"; So, I was wondering whether we should change this to www.domain, but I think not, since we're really getting mail configuration, albeit over http. But my thought here is that the stuff we're getting isn't really part of the World Wide Web, so we should request it from just domain, not www.domain. >+ let combinedAbortable = new CombinedAbortable(); >+ var time = Date.now(); >+ let fetch1 = new FetchHTTP(url1, { emailaddress: emailAddress }, false, >+ function(result) >+ { >+ successCallback(readFromXML(result)); >+ }, >+ function(e) // fetch1 failed >+ { >+ ddump("fetchisp 1 took " + (Date.now() - time) + "ms"); >+ time = Date.now(); I think I might move this line down to directly above the "let fetch2 = …" line, for slightly more accurate timing. Or remove the ddump statements entirely. >+ ddump("isp fetch for <" + url1 + "> failed: " + e); >+ ddump("isp fetch for <" + url2 + "> now being tried"); >+ combinedAbortable.removeAbortable(fetch1); >+ let fetch2 = new FetchHTTP(url2, { emailaddress: emailAddress }, false, >+ function(result) >+ { >+ successCallback(readFromXML(result)); >+ }, >+ function(e) >+ { >+ ddump("fetchisp 2 took " + (Date.now() - time) + "ms"); >+ errorCallback(e); >+ }); I think I also prefer the {'s to be on the same line as the "function(e)", but I'm not going to insist on it. >+ combinedAbortable.addAbortable(fetch2); >+ fetch2.start(); >+ }); >+ combinedAbortable.addAbortable(fetch1); >+ fetch1.start(); >+ return combinedAbortable; > } >+++ b/mailnews/base/prefs/content/accountcreation/util.js >@@ -232,16 +232,69 @@ IntervalAbortable.prototype = >+// Allows you to make several network calls, but return only one Abortable object. >+function CombinedAbortable() >+{ >+ this._calls = new Array(); >+} >+CombinedAbortable.prototype = >+{ >+ addAbortable : function(abortable) >+ { >+ assert(abortable instanceof Abortable, "need an Abortable object"); >+ this._calls.push(abortable); >+ }, >+ removeAbortable : function(abortable) >+ { >+ assert(abortable instanceof Abortable, "need an Abortable object"); >+ ArrayRemoveAll(this._calls, abortable); >+ }, >+ cancel : function() >+ { >+ for (var i = 0; i < this._calls.length; i++) >+ this._calls[i].cancel(); >+ }, >+} >+extend(CombinedAbortable, Abortable); So, your current code only ever has one thing in the array. I think we should make a simpler subclass that's more optimized for the do-one-thing-then-another pattern. Perhaps we could just store a single abortable, instead of an array? >+function ArrayRemoveFirst(array, element) >+{ >+ var pos = array.indexOf(element); >+ if (pos == -1) >+ return false; >+ array.splice(pos, 1); >+ return true; >+} You don't seem to use this function, so I think we should remove it. >+function ArrayRemoveAll(array, element) >+{ >+ var found = 0; >+ var pos = 0; >+ while (pos != -1) >+ { >+ pos = array.indexOf(element, pos); >+ if (pos != -1) >+ { >+ array.splice(pos, 1); >+ found++; >+ } >+ } >+ return found; >+} I can't see any reason we shouldn't add tests for this function. :) Except, isn't it basically the same as: array = array.filter(function(item) {return item != element}); or better, array = [i for each (i in array) if (i != element)]; ? But, of course, if you don't use an array in the Combined Abortable, then we don't need this function at all. And I'm going to give it an r-, just because I've asked for a lot of stuff, and want to see what you come up with before giving my final stamp of approval. Later, Blake.
Attachment #427578 - Flags: review?(bwinton) → review-
> "from your mail provider/from the Mozilla database". You mean it's "look up from <source>" instead of "look up at <source>"? (I can't be sure which one is right, I'm not a native English speaker.) > I always thought of it as more probing than guessing… Could we find some > wording that makes it sound more like we've got a plan, and less like we're > trying stuff at random? ;) The SSL and authentication methods is probing, yes. The hostnames are pure guessing. When we try pop3., pop., mail. and so on, we really do poke around blindly. We have 'good guess', but we don't really know. I think the word "guess" captures exactly what we do there. Even if we find something, it may still not be the best config: some ISPs use "secureimap" or similar nonsense for the SSL-enabled hosts. I don't see any reason to pretty it up and make the user an illusion about the certainty of what we found. > And there's a trailing space at the end of this line. > Please change the 's to "s. Please don't blame me for other people's code that I just indented :). But yeah, I hate those two things as well, with a passion :). I'll fix them. > Also, this four-levels-of-indentation thing is starting to annoy me. > What do you think about making some of those into named local functions? In this case, however, I think that this is essentially a sequential flow which just happens to use async calls. Just like normal functions are written and read, this should also be read straight from top to bottom. I think making many functions out of that would be harder to read. (Esp. given that you never know (unless you search or there's a comment) that there's only one caller for the function.) > naming it something other than "mozilla.xml" would be a good idea. OK, I'll call it benbucksch.xml . (Yeah, I'm kidding. People wouldn't find my homepage on Google anymore.) I'll find a nice name which avoids confusion with Microsoft's scheme. > I think I also prefer the {'s to be on the same line as the "function(e)", > but I'm not going to insist on it. The existing code uses the "new line" style, so I think I should keep it. > So, your current code only ever has one thing in the array. Yup, but I had somebody - who was that again??? - proposing that I make calls in parallel, and then I'd need a new class again, so I'd rather keep it generic. I don't think the array will cost us much. :) > array = array.filter(function(item) {return item != element}); > array = [i for each (i in array) if (i != element)]; Both of them create new arrays. But so does slice(), I just looked up. So, yeah, I'll use one of them. Thanks for the tip. I'm OK with the other stuff. I'll put up a new patch. Thanks, Ben
(In reply to comment #28) > > "from your mail provider/from the Mozilla database". > You mean it's "look up from <source>" instead of "look up at <source>"? > (I can't be sure which one is right, I'm not a native English speaker.) It could go either way, but "look up from" sounds better to me. > > I always thought of it as more probing than guessing… Could we find some > > wording that makes it sound more like we've got a plan, and less like we're > > trying stuff at random? ;) > The SSL and authentication methods is probing, yes. The hostnames are pure > guessing. When we try pop3., pop., mail. and so on, we really do poke around > blindly. We have 'good guess', but we don't really know. I think the word > "guess" captures exactly what we do there. Even if we find something, it may > still not be the best config: some ISPs use "secureimap" or similar nonsense > for the SSL-enabled hosts. > I don't see any reason to pretty it up and make the user an illusion about the > certainty of what we found. Yeah, but guess has an element of randomness to it that I don't think we have. We may not get the correct results (although I suspect we will in most cases), but the things that we try and the order we try them are very well specified. It's like, if we were running an experiment. The process isn't a guess, because it's well defined, even though we don't know exactly what the results are going to be. (Along those lines, what do you think about saying "testing"?) Having said all that, I wouldn't block the patch if you insisted on using "guess". > > Also, this four-levels-of-indentation thing is starting to annoy me. > > What do you think about making some of those into named local functions? > In this case, however, I think that this is essentially a sequential flow which > just happens to use async calls. Just like normal functions are written and > read, this should also be read straight from top to bottom. I think making many > functions out of that would be harder to read. > (Esp. given that you never know (unless you search or there's a comment) that > there's only one caller for the function.) What about not indenting them, then? Sure, it's not exactly according to spec, but if they're meant to be read linearly, then having them be more in line can only help readability, right? > > I think I also prefer the {'s to be on the same line as the "function(e)", > > but I'm not going to insist on it. > The existing code uses the "new line" style, so I think I should keep it. Yeah, I would accept that. > > So, your current code only ever has one thing in the array. > Yup, but I had somebody - who was that again??? - proposing that I make calls > in parallel, and then I'd need a new class again, so I'd rather keep it > generic. I don't think the array will cost us much. :) Have you heard the term YAGNI? :) I'm not going to insist on the change, but it is a place we could make the code simpler, and if it needs to be more complicated later, we can always make it more complicated then. > > array = array.filter(function(item) {return item != element}); > > array = [i for each (i in array) if (i != element)]; > Both of them create new arrays. But so does slice(), I just looked up. > So, yeah, I'll use one of them. Thanks for the tip. The second one is also very Pythonic, which usually means that I like it. :) (It's also faster, but that's not a great reason to choose it.) > I'm OK with the other stuff. I'll put up a new patch. > Thanks, > Ben I'm looking forward to it. And you're very welcome. :)
Status: NEW → ASSIGNED
> It could go either way, but "look up from" sounds better to me. OK. > "guess" What's a good English verb for "educated guess"? You think you have the right answer, but you're not entirely certain. You're 70-80% sure. What's the verb? Here're some synonyms which may fit: take a stab at, work out, speculate, survey, reckon, suggest I like "work out", possibly "reckon", but I like "suggest" best. > Have you heard the term YAGNI? :) I heard of extremeprogramming, and I strongly disagree with the idea. I like to build a cathedral, one pillar at a time (and use it in the meantime), instead of a wodden shack and try to fix it when it grows too big. Experience shows that we can't fix it. I present as evidence A: Our POP code. ;-)
> YAGNI That said, I'll use a simpler implementation without array, as you suggest.
(In reply to comment #30) > What's a good English verb for "educated guess"? You think you have the right > answer, but you're not entirely certain. You're 70-80% sure. What's the verb? > > Here're some synonyms which may fit: > take a stab at, work out, speculate, survey, reckon, suggest > > I like "work out", possibly "reckon", but I like "suggest" best. "Suggest" is more along the lines of you giving the other end information rather the other end informing you. Other possible wordings that may fit both your definitions of what's being done: sample (this strikes me as being in the middle between "probe" and "guess") test try inspect assess kick the tires (in the humorous vernacular)
Comment on attachment 427579 [details] [diff] [review] Fix, v2 - only http://autodiscover.domain, with string changes (v1 is superior, so let's go with it.)
Attachment #427579 - Flags: review?(bwinton)
OK, so I wrote a test which just checks the URLs that we try. I think that's silly, but I wrote it nevertheless. (I did it by creating a fake FetchHTTP object.) Unfortunately, I can't even load the function that I want to test (fetchConfigFromISP()) into the xpcshell test, because it's a chrome file and xpcshell tests are not allowed to access chrome! I can see the idea, but it's wrong in cases like this where there's UI-independent logic in JS files. Not all of these should be JS modules! So, the only other option would be a mozmill test. But it feels wrong, given that it's a pure logic function. What would make sense would be an integration test where we hit a real ISP and see whether we get a config. Esp. given that this bug is all about integration with ISPs. Given that that just checkins the URLs we try is silly (they are clearly in the source), I'll just create a "litmus test" as Standard8 put it, which is just a nice work for manual, scripted test. Here it is (_tsk_, please put it on litmus): "Start Thunderbird, Go to File | New | Mail Account..., enter something as real name and password, and enter "thunder@bucksch.name" as email address. Click Continue. PASS = It should take only about 1 second until you see "Your email provider has published the settings for your email account" and a proper config below. Currently, it is mail.bucksch.name, IMAP, port 143, no SSL and mail.bucksch.name, SMTP, port 587, no SSL, with orange dots each. Given that you have no password, you cannot create the account. For a test pass, the "Your email provider has published" message must appear. FAIL = "Shredder is trying to find settings for your email account by checking common server names.", then "Shredder suggests the following settings for your email account." Even if the server config is the same as with PASS, the message means that the test failed. Check that <http://autoconfig.bucksch.name/mail/config-v1.xml> exists and returns an XML file. If so, it's probably a bug. If the URL returns a 404, it may be a problem with the server.
Attached patch Fix, v3 (obsolete) — — Splinter Review
Changes: - Review fixes, including: - removing CombinedAbortable for a simpler one. - changing indention of async functions. Maybe you like this better. If not, I can still change the indention. - <http://autoconfig.example.com/mail/config-v1.xml> and <http://example.com/mail/config-v1.xml>
Attachment #427578 - Attachment is obsolete: true
Attachment #427579 - Attachment is obsolete: true
Attachment #428551 - Flags: review?(bwinton)
Attachment #428551 - Flags: ui-review?(clarkbw)
clarkbw, can I have UI-review on the string changes, please?
Comment on attachment 428551 [details] [diff] [review] Fix, v3 >+++ b/mail/locales/en-US/chrome/messenger/accountCreation.properties >@@ -6,21 +6,32 @@ cleartext_warning=%1$S does not use encr >+# LOCALIZATION NOTE(looking_up_settings_db): Do not translate or replace Mozilla. It stands for the project infrastructure. The database is a generic facility. Note 2: %1$S will be the brandShortName. >+looking_up_settings_db=%1$S is looking up the settings for your email account from the Mozilla ISP database. I wonder if this should be "the Mozilla Messaging ISP database" (and down below)? >+# LOCALIZATION NOTE(looking_up_settings_guess): %1$S will be the brandShortName. Note 2: We are checking common server names like pop., pop3., smtp., mail., without knowing whether they exist or really serve this email account. If a server responds, we try to talk to it via POP/IMAP/SMTP protocols and query its capabilities. If that succeeds, we assume we found a configuration. Of course, it may still be wrong, but it often works. That's not wordy at all. :) All in all, I think I like it. Later, Blake.
Attachment #428551 - Flags: review?(bwinton) → review+
Whiteboard: [needs ui-review]
Testing this out now...
Whiteboard: [needs ui-review]
Comment on attachment 428551 [details] [diff] [review] Fix, v3 There is too much text changing. When it's working fast you can't read anything at all but see a bunch of text flash by. However when I slowed down my net connection it is helpful to know what stage it's in if something were blocking. I'd suggest we use a less wordy setup by starting with some common phrase and using filling in the specific location as needed. Something like this: Looking up configuration: $LOCATION Where $LOCATION looks like these: Looking up configuration: Thunderbird Looking up configuration: ISP ($domain) Looking up configuration: Mozilla Messaging ISP Database Looking up configuration: Guessing... Then I think we want to persist how the information was found because if you didn't happen to see which of these strings it was that the config wizard worked on you'd never know it came from your ISP. The following settings were found from: $LOCATION And we'd probably have to change "Guessing..." to Discovered or Guessed for this past tense version. How does that sound?
Attachment #428551 - Flags: ui-review?(clarkbw) → ui-review-
Bryan, that makes much sense, I will change it.
Hey Bryan, this does pretty much what you proposed above. I had to change some of the strings slightly, for various reasons: - I think "Thunderbird" for harddrive is too generic, it could also mean the ISP database, so I used "Your harddrive". Note that almost no user will see this text, because the check is very very fast (unless your filesystem is broken, the msg is there just for completeness), and we don't ship any configs. It's only there for admins who make a custom TB install for their company. - While putting the domain in the "ISP $domain" is a nice idea, I could unfortunately not implement it, because of how the code is structured (emailWizard.js startSpinner, _showStatusTitle, wasn't me!). I used "Email provider" instead. - Please see the LOCALISATION_NOTE wrt "Mozilla" vs. "Mozilla Messaging". The database was started by me and gerv, not MoMo. It just happens to be hosted on a Mozilla Messaging server. - While I agree with the word "guessing", bwinton does not (see discussion above). I therefore propose "Trying common server names", which conveys the same idea, and is understandable for normal users. I hope you'll like it. I made 2 screenshots for you. http://www.bucksch.org/xfer/tb-autoconfig-lookup-isp.png http://www.bucksch.org/xfer/tb-autoconfig-guessfound.png (BTW, offtopic: the latter baffled me: why do we guess imap.aol.com for netscape.net? Where do we get aol.com from, if not from DNS MX? Answer: SSL cert! Cool. Code from David A.)
Attachment #428551 - Attachment is obsolete: true
Attachment #432187 - Flags: ui-review?(clarkbw)
Correction: "Your harddisk", not "Your harddrive", nor "Harddisk" (as in patch)
Comment on attachment 432187 [details] [diff] [review] Fix, v4 - only UI text changed This looks good with the discussed change on IRC wrt "Thunderbird installation" instead of "Harddisk". Thanks!
Attachment #432187 - Flags: ui-review?(clarkbw) → ui-review+
Commited http://hg.mozilla.org/comm-central/rev/a03eae584548 Thanks!
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
FYI, you can (for now) test with mozilla@bucksch.name , there should be a config published. (No, you can't log in, it's just about the config fetch from ISP)
The app name is hardcoded in the commited entities. IMHO they should be replaced by something like brandShortName.
You mean for > "Thunderbird installation" instead of "Harddisk". ? Yes, my bad. Will fix.
as mentioned above
Attachment #434106 - Flags: review?(bwinton)
Comment on attachment 434106 [details] [diff] [review] Fix brandname string I believe this fixes the previously mentioned bug. r=me.
Attachment #434106 - Flags: review?(bwinton) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: