422814 - (autoconfig) Make account configuration quick, easy, and more secure

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, enhancement, P1)

Description

[Brian Kirsch]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12
Build Identifier: 

Current Thunderbird versions do not allow the user to select a port when
using the Account Wizard. More POP3 and IMAP4 servers are running on 
secure ports (993, 995). 

This extension checks based on the host name whether the account is 
IMAP4 or POP3 and detects the Port and Security settings (TLS, SSL).

The extension is a proof of concept and would be a good starting
point for a real implementation.

-Brian

Reproducible: Always

Steps to Reproduce:
1. Launch the Account Wizard
2. Enter an IMAP4 or POP3 server that only runs on secure ports.
3. Complete the Account Wizard
4. Try to download mail

.
Actual Results:  
An error will be raised unable to connect to server.

The user must then manually edit the Account Settings to
set the correct security and port information.

Comment 1

[Brian Kirsch]

Attachment: XPI that ties in to the Account Wizard to add port / security discovery

Here is the source code for the example Account Wizard Auto Discovery XPI.

Comment 2

[David Ascher (:davida)]

David, this is the extension we were talking about.

Comment 3

[David :Bienvenu]

My first inclination was to add these capabilities to the core protocol handlers, who know how to parse capabilities, etc, but I'm warming to the approach in this bug of just doing it from js, since it doesn't touch the core code.  I'll try to make a patch where this is integrated into the account wizard, and test it out.

Comment 4

[David :Bienvenu]

Hi Brian K, didn't notice it was you at first :-) 

I've tried the xpi, and it seems to install and work, though I haven't tried it with a real server. It adds an "auto discover" button to the page where you give the server type (imap or pop3) and name. I would think we would do auto-discovery automatically (perhaps when the user clicks the next button?) but cc'ing Bryan for his UE input.

Comment 5

[Nikolay Shopik]

related to bug 342242?

Comment 6

[David :Bienvenu]

related but not the same, since this xpi, at least, does port probing and capability testing, not auto-config via dns 

Comment 7

[David :Bienvenu]

Attachment: first stab at moving this into core account manager

This moves the autoconfigure stuff into the core account wizard. It works like the extension, in that you click an autoconfigure button to make us do port and capability probing for imap/pop3. I've got it so it works in TB, but I have a little work to do to get it working for SM (mostly string changes).

Hacking the account wizard is never fun, and I may not be putting things in all the right places.  I'm going to make a stab at making a patch that will work with SM and then run it past Neil.

Comment 8

[David :Bienvenu]

Attachment: take a stab at working w/ suite

Neil, I've basically taken the xpi code and moved it directly into the account manager. I've tried to put things in reasonable places. I didn't create a whole new dtd for the AutoConfigureDialog code (I'm not sure that should even be a separate dialog, but it does allow for "Cancel" and "Apply"). The patch seems to work and is a start on doing more autoconfig stuff.

It should probably be extended for SMTP as well.

Comment 9

[David Ascher (:davida)]

Not having reviewed the UI or the cod, I think this is extremely valuable work, and setting flags to indicate such.

Comment 10

[[e-mail]]

Comment on attachment 328483 [details] [diff] [review]
take a stab at working w/ suite

OK, so I took this patch for a spin, and while I like the idea, I think it needs some serious UI love, so I'm disappointed that Bryan hasn't commented yet ;-)

The first thing I noticed was that autoconfigure doesn't work while you're offline, but there's no feedback for that. It also doesn't work when there's no hostname, instead presenting an alert; I suggest both conditions should simply disable the button (which by the way needs an accesskey).

I don't like the way that autoconfigure results are static, but there's work to allow manual entry of the port and socket type, so we should coordinate.

I don't like the design of the autoconfigure dialog, but maybe we can redesign or eliminate it.

I don't really understand the networking code.

Comment 11

[David :Bienvenu]

(In reply to comment #10)
> (From update of attachment 328483 [details] [diff] [review])
> OK, so I took this patch for a spin, and while I like the idea, I think it
> needs some serious UI love, so I'm disappointed that Bryan hasn't commented yet
> ;-)
Yes, I'm a little reluctant to spend a lot more time on it w/o getting that UI love :-)
> 
> The first thing I noticed was that autoconfigure doesn't work while you're
> offline, but there's no feedback for that. It also doesn't work when there's no
> hostname, instead presenting an alert; I suggest both conditions should simply
> disable the button (which by the way needs an accesskey).

Those I can do.
> 
> I don't like the way that autoconfigure results are static, but there's work to
> allow manual entry of the port and socket type, so we should coordinate.
> 
> I don't like the design of the autoconfigure dialog, but maybe we can redesign
> or eliminate it.
That's something I'm interested in Bryan's opinion on. I'm torn about making it a dialog or not (i.e., automatic or explicit). I believe mail.app does it automatically. It would be a bit more work to make it happen automatically in the account wizard...we'd want to have a way for the user to cancel the process, for example, which makes doing it on "Next" a little odd.
> 
> I don't really understand the networking code.
> 

Conceptually, all the network code does is connect to different ports on the server, and if successful, sends the appropriate command to get the server to respond with capabilities, to see if STARTTLS is enabled, and then quits/logs out. But looking at it a bit more, I could clean it up a little...

Comment 12

[Bryan Clark (DevTools PM) [:clarkbw]]

I'm going to try this extension out to see how it works, we do have a wiki page for improvements to the account wizard.  See page 3 especially: http://wiki.mozilla.org/MailNews:Account_Wizard:Email#Page_3

Comment 13

[Nikolay Shopik]

We should probe 587 as SMTP port too if not doing so already.

Comment 14

[David :Bienvenu]

Yes, see #c8 - but I am thinking of taking a quick whack at that.

Comment 15

[Bryan Clark (DevTools PM) [:clarkbw]]

I've updated my account wizard mockups to match the wiki page, take a look at that in general and let me know what you think.  This may need to be broken into other bugs, but I think it is all connected to get the user experience right.

I've made the auto-detect more inline/automatic about checking the hostname, but we should discuss issues related to network timeouts or other problems.  It seems like we should be able to keep checking (on input) for valid hostnames and then probe the hostname.

Note the spinner in there so the person can have some idea of network activity going on.  There's a possibility of a person losing entered information when the probe results return (perhaps taking a while).  One option could be to make the entries insensitive, but that would likely require us to let the person cancel the probing.  Instead I think we can just create an undo-scheme where probed results return and we show an undo link besides the spinner for getting back whatever was in the entries before we auto-filled them.  (still need to work on this - not in mockups)

Weave uses a system very similar to this for password checking in their wizard code.
http://hg.mozilla.org/labs/weave/index.cgi/file/c8bce0724360/chrome/content/

Mockups are in my account wizard folder here, with relevant links below; feedback appreciated.
http://clarkbw.net/designs/account-wizard/auto-config/

IMAP w/ auto-detection
http://clarkbw.net/designs/account-wizard/auto-config/accont-wizard-p3-imap-pre-auto-detection.png
http://clarkbw.net/designs/account-wizard/auto-config/accont-wizard-p3-imap-auto-detected.png

POP w/ auto-detection
http://clarkbw.net/designs/account-wizard/auto-config/accont-wizard-p3-pop-pre-auto-detection.png
http://clarkbw.net/designs/account-wizard/auto-config/accont-wizard-p3-pop-auto-detected.png

Comment 16

[Bryan Clark (DevTools PM) [:clarkbw]]

Note, I seemed to have gotten my spinners mixed up in the pre vs. post detection and that's fixed now. 

Also added some prototype mockups with 'undo'.  This would revert the settings back to whatever they were before the auto-detect changed them.
http://clarkbw.net/designs/account-wizard/auto-config/accont-wizard-p3-imap-auto-detected%20(with%20undo).png

As well as one with a 'try again'.  This action would appear after any attempt to auto-detect failed (like offline) or after an undo.  We could try to have both actions available as there is a choice, but I'd like to be a bit minimal and the actions will appear in the same location so you could click twice for undo, try again.
http://clarkbw.net/designs/account-wizard/auto-config/accont-wizard-p3-imap-auto-detected%20(with%20try%20again).png

Comment 17

[Bryan Clark (DevTools PM) [:clarkbw]]

Oh, and bug 326076 and bug 221030 have some code for using a menu list or radio group, though not exactly what is specified on the wiki page.

Comment 18

[David :Bienvenu]

Attachment: add support for smtp port probing and STARTTLS detection (untested)

I took a stab at extending the auto discovery code to handle smtp. I haven't hooked it up to the account wizard, so it's completely untested. I won't be able to test it too much, since my ISP blocks SMTP servers other than its own, and doesn't support STARTTLS.

Comment 19

[David :Bienvenu]

Attachment: untested, and the wrong file :-)

Comment 20

[David :Bienvenu]

Brian, what would trigger the initial autoconfigure in the proposed UI? I assume the try again link is for subsequent autoconfigure attempts. Would it start off as a "configure" link or button? Apolgies if it's already in a screen shot...

I'm not sure undo is going to be that useful

Comment 21

[Bryan Clark (DevTools PM) [:clarkbw]]

I was suggesting we watch the text input box for a valid hostname and when the person has paused/finished typing.  Though there can be lots of valid looking hostnames unintentionally created, for instance when a person starts entering the initial imap of imap.gmail.com; imap could be considered valid.  So there might be some spinning during typing.

The 'try again' would only appear after failure to successfully return information and if the person hadn't typed anymore information.  (for a network down case)

Comment 22

[David :Bienvenu]

I don't think we easily know when the user has finished typing (Neil can correct me if I'm wrong). I suppose we could use timers and check if the text field value has not changed in a few seconds...We know when the text widget loses focus, but it's very likely that it will lose focus because the user has clicked the "Next" button. I worry about the spinning when typing, because I know when I'm adding a new account, I'm usually referring back and forth between the account wizard and an e-mail or web page that tells me the host name, which means it looks like I've stopped typing.

Completely unrelated comment - we also want to check for secure auth capabilities, and set that for the user if the server supports a secure authentication mechanism.

Comment 23

[Nikolay Shopik]

I suggest start detecting after user press next button and show of page of what we detected (if any) and in case user misspells name of server it can always come back at retype name of server press next again.

Comment 24

[Bryan Clark (DevTools PM) [:clarkbw]]

I think it would have to be based on a timer a second or two after typing.  Likely the real value can only be determined with some testing.

Not that user correction are going to be that likely since we should be detecting for the most secure possible connection and defaulting to that.  (as a side note I wondered if we should be keeping all possible valid connections for the person to choose)  However we want to show that we've found the right setup before they leave the page.

I think bienvenu's comment about how people enter this information is completely correct, and what we're targeting.  A person is probably looking at something else for the information and so after typing in the hostname has to read the port / security connection type from the same place.  During the time they are reading t-bird should have auto-detected the settings.

Comment 25

[David :Bienvenu]

Over irc, we discussed taking advantage of autoconfig to simplify the process - Bryan posted this for a first page - http://pastebin.mozilla.org/498008

The idea is that we'd do auto-config when the user presses next from this screen, and figure out if the server is imap or pop3, supports tls or ssl, and whether it supports secure authentication. If authentication fails, we tell the user and allow them to correct the password or server name. Bryan also suggested on the final page, we give the user the opportunity to remember the password, with a check box, turned on by default.

One issue is that we need the user name to logon with. We could try both the full e-mail address and the part before the @ as user names, but guessing about user name isn't ideal, when we'll be sending a password along with it. And there are situations where the username is neither.

If we ask for the username explicitly, this page starts to overflow. We could make the account wizard a little bigger.

An other possibility is to break auto config into two steps - in the first, the user enters their identity name, e.g., John Smith, along with the incoming server name and outgoing server name. Then we autoconfig the servers, making sure we can connect to them, figuring out a port and corresponding server type (IMAP vs. POP3), and connection type (SSL/TLS, etc). Then, on the next screen, the user enters their email address, user name, and password, and we verify that all works, and figure out if we should use secure auth. Then, the user would go to the verification page.

There might be edge cases, e.g., we can connect to the server on the imap port, but the user is only enabled for pop3 access, and we've preferred imap over pop3. If we have a way for the user to go into edit settings despite errors, that would be one work around. Or we could detect that both protocols work and let the user pick. What we need to enable is account setup even when autoconfig fails. Or a way to bypass autoconfig, perhaps to go straight to account settings - power users have asked for that, so we could solve two problems at once.

Comment 26

[Nikolay Shopik]

Can we also detect identity name of current logged in user (John Doe) on Windows platform too? I know we had this functionality on Linux at least but never seen such thing on Windows platform.

Comment 27

[David :Bienvenu]

Attachment: don't use a separate dialog for autoconfig...

This starts in the direction Bryan has proposed - ask for person's name and e-mail address, and try to figure out mail server name, type (IMAP or POP3) and connection type (TLS, SSL, plain). It shows a progress button, and tells you which host its trying.

The dialog is messy, and I left in the autoconfigure button for ease of testing.

I'm going to hand this off to DavidA, and go work on the second part of the backend for this; once we've discovered a server, type, and port, verify that the user can logon to the server with the password they give us, and determine the most secure authentication mechanism that will work with that server. I'll probably add a method to nsIMsgIncomingServer that does this, so the front end code would create an incoming server object and call the logon method. In order to make it easier to communicate results back to the front end, I may make the calling js poke the use secure auth property on the server object directly, try that logon, if it fails, fall back to insecure auth. The logon method will take a url listener, so the calling code will get called back with success or failure, once we've tried to logon.

That's all speculation at this point; I need to start implementing it to see what's going to work in the backend...

Comment 28

[David Ascher (:davida)]

Attachment: WIP of an extension to do the auto-detection.

Attaching this for sharing w/ ben bucksch and bienvenu, for discussion.  Not ready for review.

Comment 29

[Ben Bucksch (:BenB)]

Attachment: AccountConfig JS class to hold the data

As promised, here's the JS class I propose to hold the account data, within the dialog. One instance would hold the values shown in the UI, and various autoconfig functions would return instances, and there'll be an createAccount function which takes that and creates the account in the backend.

Comment 30

[David :Bienvenu]

Attachment: wip to verify imap logon

this doesn't quite work yet, and I had to actually create the imap server object w/ the account manager in order to get the logon verification url to run (it doesn't create the account, but we will need a way to clean up the server object after we're done with it - I'll probably need to add a method to the account manager to remove the server object, and we will need to be very dilligent about cleaning up after ourselves).

I have to write similar code for POP3, and SMTP, as well as getting this to work correctly for IMAP (I think the problem is in my test harness, so converting over to David's code would probably help)

Comment 31

[David Ascher (:davida)]

Cool, I'll try to merge my front-end code with the back-end part of this patch, hopefully tomorrow.

I might even try it with an hg clone of comm-central, which is probably where we need to end up anyway.

Comment 33

[David Ascher (:davida)]

TODO (to myself) from that last bug: figure out where port 587 fits in.

Comment 34

[David :Bienvenu]

Attachment: pop3 autodetection (completely untested)

Comment 35

[Chris Newman]

Port 587, as described in RFC 4409, is the standard port for mail submission.
Traditionally, port 25 was used for mail submission, but it is now good
practice for ISPs to block outbound port 25 at their firewalls as a spam
prevention mechanism, so 25 just isn't viable for traveling users or users
on a random WiFi network.  There is also use of port 465 for SSL
submission although this port isn't even registered for that purpose.

RFC 5068, an Internet Best Current Practice states:
  MUAs SHOULD use the SUBMISSION port for message submission.

So my advice would be to use 587 if it works, and fall back to port 25 if it
doesn't.  Also, if you ever see STARTTLS advertised in response to EHLO,
attempt to negotiate it and if it succeeds, require STARTTLS for all future
connections.

I don't think it's worth probing 465 since it's non-standard (and could be
something else).

Comment 36

[David Ascher (:davida)]

FYI, I've created a c-c clone at http://hg.mozilla.org/users/dascher_mozilla.com/autoconfig/, and committed the backend parts of davidb's last two patches to it.  I'll be doing my front-end work there, but most likely won't have anything committed until early next week.

(I don't care where we do this, but I wanted to learn a bit about hg clones, so this seemed a good practice area)

Also FYI, rough front-end code is currently in an extension at http://hg.mozilla.org/users/dascher_mozilla.com/emailconfig/ but it's not worth looking at yet.

Comment 37

[rsx11m]

(In reply to comment #35)
> Traditionally, port 25 was used for mail submission, but it is now good
> practice for ISPs to block outbound port 25 at their firewalls as a spam
> prevention mechanism

I wouldn't call blocking port 25 by providers "good practice" as it prevents use cases for people running their own legitimate mail servers, which may be forced to go with (higher priced, but otherwise identical) business accounts then to avoid that block. Anyway, that's beyond the scope of this RFE...

> I don't think it's worth probing 465 since it's non-standard (and could be
> something else).

While port 587 is now used by many if not most providers, some providers offer 465 for SSL. Thus, it should be probed if 587 and 25 fail or if neither of them offers TLS for encryption, and 465 with SSL tested as a fallback otherwise.

Comment 38

[David :Bienvenu]

Attachment: aw-autoDiscover.js that calls verifyLogon

this shows how to create a server object, init it, and call verifyLogon on it.

Comment 39

[David :Bienvenu]

Attachment: smtp logon verification (wip, untested)

I will land this in your/our repository, David, once I've figured out a way of testing it a little. Normal message sending still works. I've mostly restrained myself from cleaning up the code around the stuff I added, but I did change the password stuff to use (ns)ACString, and I cleaned up whitespace in places

Comment 40

[David :Bienvenu]

I should also mention that I did land an additional change to the imap code in the repository to suppress error messages during logon verification.

Comment 41

[Ben Bucksch (:BenB)]

- I refactored the wizard dialog's JS code a lot
- Added AccountConfig
- Rewrote most of emailWizard.js
- Wrapped guessConfig to return an AccountConfig and be UI-less.
- Rewrote most of verifyLogon() (now in verifyLogin.js) to
be UI-less and take an AccountConfig.
- Moved accountCreation.xul/js to emailWizard.xul/js and autoconfig to guessConfig.js

I left the old files in place, so you can just change mailnews/base/prefs/resources/content/accountUtils.js
-    window.openDialog("chrome://messenger/content/accountcreation/emailWizard.xul",
+    window.openDialog("chrome://messenger/content/accountcreation/accountCreation.xul",
to get the "old" dialog and code back.

My changes are mostly dry-coded and not much tested, DOES NOT FULLY WORK YET.
Outstanding:
- Test, fix
- onblur is called twice
- Re-add gussing of username and secure auth
(see verifyLogin.js, move to guessConfig.js)
- fetchConfig.js implementations

I hope to get the code fixed and working soon. In the meantime, David A., can you take a look at it and see whether you like it?

bienvenu, if you want to continue work on autoConfig.js / guessConfig.js, go ahead, either file will be fine, as I didn't touch the autoConfig.js code, just copied it and added a wrapper at the top, so merging should be trivial.

Note:
- log4moz.js is not built by default and breaks the whole dialog when it's not there
- I get verifyLogon is not a function(). I guess I need to update and rebuild.

Comment 42

[David :Bienvenu]

setting target milestone to b1

Comment 43

[David Ascher (:davida)]

Switching for b1 flags to target milestones, to avoid flag churn.

Comment 45

[David :Bienvenu]

Attachment: backend changes for logon verification

These are the set of backend changes we've made for auto config. They fall into three main areas:

1. Adding verifyLogon methods for imap,pop3, and smtp. These methods verify that with the passed in password, the user can logon to the server. The process should not alert the user or failure, nor put up a password prompt.

2. Don't put up a password if the url is running without a msgWindow, i.e., is a biff url. (I may need to make sure we don't forget passwords when biff urls are run, but I think that's going to be part of a rework of how we forget passwords).

3. Add the ability to remove a server from the account manager, since the new account wizard has to create temporary server objects.

Apologies in advance if I've messed this patch up - I had to edit the diff by hand quite a bit to get a diff between the two repositories.

Comment 46

[[e-mail]]

Comment on attachment 337116 [details] [diff] [review]
backend changes for logon verification

>+  /* dead code. */
>   readonly attribute boolean isSecureServer;
Is there a bug to remove it then?

>+  void verifyLogon(in ACString aPassword, in nsIUrlListener aUrlListener);
All the implementations of verifyLogon simply set the password immediately.
IMHO it makes more sense for the caller to set the password separately.

>+  // invalidate the FindServer() cache if we are removing the cached server
>+  if (m_lastFindServerResult) {
>+    nsCString cachedServerKey;
>+    rv = m_lastFindServerResult->GetKey(cachedServerKey);
>+    NS_ENSURE_SUCCESS(rv,rv);
>+
>+    if (serverKey.Equals(cachedServerKey)) {
Is there any reason why we can't compare the pointers?

>-      // clear out all identity information if no other account uses it.
>-      if (!identityStillUsed)
>-        identity->ClearAllValues();
>+        identity->ClearAllValues(); // clear out all identity information.
I think there are people who tweak their prefs to share identities...

>+  void verifyLogon(in ACString aPassword, in nsIUrlListener aUrlListener);
(Same as above)

>       if (incomingServerToUse)
>       {
>         nsCString tmpPassword;
>         nsresult rv = incomingServerToUse->GetPassword(tmpPassword);
>-        *aPassword = ToNewCString(tmpPassword);
>+        aPassword = tmpPassword;
>         return rv;
>       }
Will this simplify to return incomingServerToUse->GetPassword(aPassword); ?

>+      rv = NS_MsgBuildSmtpUrl(aFilePath, smtpServer,
>                               aRecipients, aSenderIdentity, aUrlListener, aStatusFeedback, 
>                               aNotificationCallbacks, &urlToRun, aRequestDSN); // this ref counts urlToRun
>       if (NS_SUCCEEDED(rv) && urlToRun)	
>       {
>         nsCOMPtr<nsISmtpUrl> smtpUrl = do_QueryInterface(urlToRun, &rv);
>         if (NS_SUCCEEDED(rv))
>             smtpUrl->SetSmtpServer(smtpServer);
Now you're passing the smtpServer to NS_MsgBuildSmtpUrl, it should set it directly on the created smtpUrl so that the callers don't have to.

>-	if (aRecipientsList)
>-		*aRecipientsList = ToNewCString(m_toPart);
>-	return NS_OK;
>+  if (aRecipientsList)
>+    *aRecipientsList = ToNewCString(m_toPart);
>+  return NS_OK;
You forgot to diff -w ;-)

>+    else if (!PL_strcasecmp(m_urlidSubString, "verifyLogon"))
Nit: you appended it as "verifylogon"

>+  nsCString escapedUsername;
>+  *((char**)getter_Copies(escapedUsername)) = nsEscape(popUser.get(), url_XAlphas);
Aren't we supposed to use the extenal escape API? Anyway, you mean .Adopt

>+  char * urlSpec = PR_smprintf("pop3://%s@%s:%d/?verifyLogon", 
>+                              escapedUsername.get(), popHost.get(), popPort);
>+  if (!urlSpec)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  nsCOMPtr<nsIURI> url;
>+  rv = BuildPop3Url(urlSpec, nsnull, popServer, aUrlListener, 
>+                    getter_AddRefs(url), nsnull);
>+  PR_Free(urlSpec);
Nit: PR_smprintf_free (or whatever it's called)

Comment 47

[David :Bienvenu]

I've filed Bug 453979 to remove isSecureServer, which BenB noted as dead code in the idl, and I verified earlier that it is indeed not used.

>All the implementations of verifyLogon simply set the password immediately.
>IMHO it makes more sense for the caller to set the password separately.

I was trying to maintain a bit of flexibility about how the password gets set but if we need that, I can always put it back the way it is.

>>-      // clear out all identity information if no other account uses it.
>>-      if (!identityStillUsed)
>>-        identity->ClearAllValues();
>>+        identity->ClearAllValues(); // clear out all identity information.
>I think there are people who tweak their prefs to share identities...

This is a bad diff on my part. I should have diffed against a completely clean tree...the code that looks like its getting removed is actually getting added, so that people who tweak their prefs to share identities won't have them cleaned out :-)

I'll attach a new, I hope, clean, patch in a bit...

Comment 48

[David :Bienvenu]

Attachment: logon verification backend changes addressing Neil's comments

This attempts to address Neil's comments. I also made nsMsgAccountManager::SetLastServerFound void, since it never returned an error, and that let me cleanup a little bit of code.

Comment 49

[Mark Banner (:standard8)]

Comment on attachment 337493 [details] [diff] [review]
logon verification backend changes addressing Neil's comments

>+NS_IMETHODIMP
>+nsMsgAccountManager::RemoveIncomingServer(nsIMsgIncomingServer *aServer, 
>+                                          PRBool aCleanupFiles)
>+{
>+  nsCString serverKey;
>+  nsresult rv = aServer->GetKey(serverKey);
>+  NS_ENSURE_SUCCESS(rv,rv);

nit: space missing (there are several of these throughout the patch).

>+  m_incomingServers.Remove(serverKey);
>+
>+  nsCOMPtr<nsIMsgFolder> rootFolder;
>+  aServer->GetRootFolder(getter_AddRefs(rootFolder));
>+  nsCOMPtr<nsISupportsArray> allDescendents;
>+  NS_NewISupportsArray(getter_AddRefs(allDescendents));
>+  rootFolder->ListDescendents(allDescendents);

Should we be checking these for failure / null pointers?

>+  PRUint32 cnt =0;

nit: space missing

>+  rv = allDescendents->Count(&cnt);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  for (PRUint32 i = 0; i < cnt;i++)

nit: space missing.

>+  {
>+    nsCOMPtr<nsIMsgFolder> folder = do_QueryElementAt(allDescendents, i, &rv);
>+    folder->ForceDBClosed();

You've got the value of rv, but you're not checking it...

> NS_IMETHODIMP
>diff -uwr ./base/util/nsMsgMailNewsUrl.cpp /autoclone/mailnews/base/util/nsMsgMailNewsUrl.cpp
>--- ./base/util/nsMsgMailNewsUrl.cpp	Fri Aug 29 08:26:35 2008
>+++ /autoclone/mailnews/base/util/nsMsgMailNewsUrl.cpp	Thu Aug 28 12:50:27 2008
>@@ -202,11 +202,9 @@
>   NS_ENSURE_ARG_POINTER(aMsgWindow);
>   *aMsgWindow = nsnull;
>   
>-  // note: it is okay to return a null msg window and not return an error
>-  // it's possible the url really doesn't have msg window
>   nsCOMPtr<nsIMsgWindow> msgWindow(do_QueryReferent(m_msgWindowWeak));
>   msgWindow.swap(*aMsgWindow);
>-  return NS_OK;
>+  return *aMsgWindow ? NS_OK : NS_ERROR_FAILURE;

NS_ERROR_NULL_POINTER would seem slightly more appropriate.

>@@ -72,7 +73,7 @@
>    * It can be specified/saved here to avoid prompting the user constantly for
>    * the sending password.
>    */
>-  attribute string password;
>+  attribute ACString password;

For the record, I was going to suggest changing this (and the others) to AUTF8String to help with using non-ASCII characters in passwords. However I looked into things a bit more, and came across items like base 64 encoding, and the specs for SMTP and decided we need to resolve this elsewhere with more thought.

>   nsIURI GetNewMail(in nsIMsgWindow aMsgWindow, in nsIUrlListener aUrlListener,
>-          in nsIMsgFolder aInbox,
>-                    in nsIPop3IncomingServer popServer);
>+                  in nsIMsgFolder aInbox, in nsIPop3IncomingServer popServer);

Its a bit hard to tell with -w but this looks the wrong indentation.

>   nsIURI CheckForNewMail(in nsIMsgWindow aMsgWindow, in nsIUrlListener aUrlListener,
>-             in nsIMsgFolder inbox,
>-                         in nsIPop3IncomingServer popServer);
>+                    in nsIMsgFolder inbox, in nsIPop3IncomingServer popServer);

ditto wrt the indentation

-        nsCString escapedUsername;
-        *((char **)getter_Copies(escapedUsername)) =
-            nsEscape(username.get(), url_XAlphas);
+        nsCString escapedUsername.Adopt(nsEscape(username.get(), url_XAlphas);

This doesn't compile (nsSmtpServer), and please could you use MsgEscapeString version so we're ready for frozen API.

nsPop3Service.cpp seems to be missing an #include "nsINetUtil.h"

(In reply to comment #45)
> 2. Don't put up a password if the url is running without a msgWindow, i.e., is
> a biff url. (I may need to make sure we don't forget passwords when biff urls
> are run, but I think that's going to be part of a rework of how we forget
> passwords).

I think checking this would be a good idea before submitting it. We don't want our forget password cases to get even worse.


r=me with the comments addressed, bustages fixed and the not forgetting passwords on biff checked.

Also, I think we could really do with some unit tests for this. Certainly we have fake servers for both pop and smtp and I think writing some tests around those would be a good idea. However, don't wait on writing them to get this code in, as it is probably going to conflict with some of the password manager stuff I am working on.

Comment 50

[David :Bienvenu]

Attachment: patch addressing Standard8's comments - checked in

carrying forward standard8's r+, requesting sr from Neil.

I've made it so we only forget imap passwords if there's a msg window (we were already doing this for pop3). I fixed the spaces in NS_ENSURE_SUCCESS(rv, rv) if they were lines I added, but in the interest of keeping this patch from being even bigger, I left the other ones alone. I addressed all the other comments, I believe.

Comment 51

[Mark Banner (:standard8)]

3.0b1 flag is going away in favour of 3.0 flag and milestone combination.

Comment 52

[[e-mail]]

Comment on attachment 337672 [details] [diff] [review]
patch addressing Standard8's comments - checked in

>+    else if (!PL_strcasecmp(m_urlidSubString, "verifyLogon"))
Bah, that wasn't my idea when I made the comment... it looked inconsistent with all the other imap url flags although I admit that I hadn't seen the POP3 code's consistent use of verifyLogon by then.

Comment 53

[Mark Banner (:standard8)]

This isn't ready for beta 1, so we're moving it out to beta 2.

Comment 54

[jwq]

 In recent comments to Bug 112356, Bug 231541 and Bug 312431 Bryan Clark claims that this patch will alter the default "leave on server" behaviour (which is rather contentious). I can't see any discussion above regarding a change to the default "leave on server" behaviour and Bryan's comments in the other bugs (particularly Bug 231451 comment #91) are somewhat vague.

 Could someone please explain just exactly what is the intention in this patch with regard to the default leave on server behaviour?

Comment 55

[Bryan Clark (DevTools PM) [:clarkbw]]

bug 231541 comment 91 is where I was describing an alternate method for managing mails on a POP server post account creation.  I also created bug 455681 to track the inclusion of this new way of warning and managing server space for POP3 users.

Comment 56

[Nomis101]

Today I created an email account with Apples Mail (out of curiosity). This was easy, because Apple Mail sets the encryption to SSL per default (this is good for the normal, inexperienced user). You can alter this after the account creation is complete (but I don't find a TLS option). 

In my first experiences with e-mail clients, the hardest thing in configuration was to find the configuration informations (name of the POP3/IMAP and SMTP server, Ports, best encryption). So I think a sophisticated method and helpful for inexperienced users would be a list in the Account Wizard that lists the popularest webmail providers. So you can choose your provider and the best settings will be done automatically. After that you can edit it (for the experts) or use this default settings. This would make the account creation extremely easy for everyone. But I don't know if this idea is feasible. 

One thing I think is very important is not to get rid of terms like "SSL" or "TLS" for simplification reasons. Because every webmail provider uses this terms on there websites. And this terms are established. And one thing I like on Thunderbird is the potential to have the full control of your account settings.

Comment 57

[Tony Mechelynck [:tonymec]]

(In reply to comment #56)
> [...] So I think a sophisticated method and helpful
> for inexperienced users would be a list in the Account Wizard that lists the
> popularest webmail providers. So you can choose your provider and the best
> settings will be done automatically. [...]

Most popular where? In my country, the most popular email providers are @belgacom.net and @skynet.be, but there are other national servers, and some international servers like @hotmail.com and @gmail.com, and some servers in neighbouring countries like @chello.fr or @yahoo.fr, are also popular. (My own accounts are with @belgacom.net, @skynet.be, @yahoo.co.uk and @gmail.com.) I'll concede that most of my country's citizens will prefer French or Dutch interfaces for their browsers and mailers, but I'll also bet that there are people all over the world (even in non-English-speaking countries) who, like me, prefer en-US menus and messages. I think that if you want to list all servers with a non-negligible number of users you'll get such a long list that it won't be practical, especially for an inexperienced user.

I believe it would be better to have the user type the desired server name (as mentioned on whatever document he got from his server when signing up for an account), perhaps as part of the desired email address. This would also allow maintaining a single list of servers with their respective settings, as opposed with a different list of "popular servers" for each localized version of Thunderbird.

Comment 58

[Ben Bucksch (:BenB)]

> I think a sophisticated method and helpful for inexperienced users would
> be a list in the Account Wizard that lists the popularest webmail
> providers.

That's already implemented on the autoconfig branch.
See https://wiki.mozilla.org/Thunderbird:Autoconfiguration .

Comment 59

[Ben Bucksch (:BenB)]

(More precisely, it fetches the config, depending on the email address domain.)

Comment 60

[Nomis101]

(In reply to comment #57)
I think that if you want to list all
> servers with a non-negligible number of users you'll get such a long list that
> it won't be practical, especially for an inexperienced user.
Yes, I'm aware that this is an complex to realize idea. But Thunderbird would be the first program with this feature. :D
Hm, maybe you can handle this, that you only type your email adress in and than an automated mechanism will compare the part after the "@" with an internal list of all providers to find the settings. 



(In reply to comment #58)
> That's already implemented on the autoconfig branch.
> See https://wiki.mozilla.org/Thunderbird:Autoconfiguration .
OH, wow, I didn't know about that bevor. Nice to know. I think this is that what I'm talking about, but I have to read it exactly first. :)

Comment 61

[David :Bienvenu]

In order to stop loading from BenB's web server, I've made the url we fetch http config info configurable by setting "mailnews.auto_config_url". If we get this set up, we'd set this pref in mailnews.js (or all-thunderbird.js, if it's TB-only)

Comment 62

[David :Bienvenu]

moving to b2

Comment 64

[David :Bienvenu]

I've made a pass through the code in the autoconfig repo to make the strings localizable, updated it to use the newer log4moz in gloda, and moved the duplicate account detection to before we verify the password, since there's no sense verifying the password for a duplicate account. I've updated the TODO list somewhat as well.

I'll come up with a plan of attack for the remaining work to get the code into some sort of reviewable shape so we can get it into the tree sooner rather than later, and the localizers can start translating all the strings, and people can start banging on it, and I hope help out with some of the issues.

Comment 65

[Ben Bucksch (:BenB)]

David, great changes, thanks. Also thanks for doing the localization pass.

I can help with getting this finished. Let's talk about it. Please ping me when you start, if I haven't contacted you before.

Comment 66

[David :Bienvenu]

Attachment: wip

this is a cumulative wip patch - I've made a pass cleaning up the js formatting, got rid of the dump statements, etc, and got things working in a comm-central tree, among other things. I'm going to try to start getting reviews on parts of this, while I continue to work on some remaining issues.

Comment 67

[David :Bienvenu]

Attachment: a lot more code cleanup, remove unused files

Comment 68

[David :Bienvenu]

Attachment: more wip

Phil, do you might taking a jaundiced look at this? I realize it's an insanely large patch but I'm trying to get it into the tree so that we can get it in the hands of users.

Right now, it adds a new file menu, File | New | Mail Account (Quick Setup), which brings up the autoconfig wizard that davida and benb worked on. The wizard seems to be working relatively well. I haven't hooked up any pre-canned xml files yet; Ideally they'd be on our momo server somewhere. Right now some are on BenB's server, but we don't want to check in with that in the code :-)

The windows theme is pretty far behind the mac theme since Davida works on the mac. I haven't dove into that yet. Picking the smtp server isn't fully implemented yet either.

Comment 69

[Ben Bucksch (:BenB)]

> The wizard seems to be working relatively well

It doesn't for me. It breaks easily in lots of cases for me.
That's why I'd want to work on it still, but I don't want to hold anybody up.

Comment 70

[David :Bienvenu]

Comment on attachment 356817 [details] [diff] [review]
more wip

Perhaps Magnus has some time to review some or all of this...

Comment 71

[Phil Ringnalda (:philor)]

My after-work reviewing didn't get past wondering whether you were going to attach another patch that didn't have the "blah blah blah, XXX" strings in /locales/, or you were going to attach another patch that put the strings in content until you got l10n-ready strings.

Comment 72

[David :Bienvenu]

There are a few strings that the code passes around that afaik are not actually displayed to the user. The ones that are I put in a/mail/locales/en-US/chrome/messenger/accountCreation.properties though I may have missed some...

Comment 73

[Phil Ringnalda (:philor)]

Lest I forget, or don't get the whole thing done:

+  const CC = Components.classes;
+  const CI = Components.interfaces;

r-, CC should only be Components.Constructor (CI doesn't have that confusion problem, but *will* needlessly lead to someone trying to use Ci, currently at 270 uses in the tree, instead of CI, currently at 9).

Comment 74

[Phil Ringnalda (:philor)]

+    try {
+      let prefs = CC["@mozilla.org/preferences-service;1"]
+                     .getService(CI.nsIPrefBranch);
+      prefs.setBoolPref(leaveOnServerPref,
+                        config.incoming.leaveMessagesOnServer);
+      prefs.setBoolPref(deleteFromServerPref,
+                        config.incoming.deleteOnServerWhenLocalDelete);
+    } catch (ex) {dump(ex);}

What error are we expecting, why aren't we catching it and re-throwing anything else, and why do we think we can continue creating an account when we can't save prefs?

Comment 75

[Phil Ringnalda (:philor)]

Comment on attachment 356817 [details] [diff] [review]
more wip

String nitting:

>+++ b/mail/locales/en-US/chrome/messenger/accountCreation.dtd
>+<!ENTITY none.label                      "None">
>+
>+<!ENTITY gotoadvanced.label               "Go to advanced settings">

One extra space starting here

>+<!ENTITY accountcreation.title                     "Account Setup">

Except here it's a bit more than one

>+<!ENTITY insecureCleartext.description
>+'Warning! This is an insecure server.  Email is sent in
>+clear-text, so your email could be read by attackers, etc.  Thunderbird
>+will let you get to your mail, but you should really get your email
>+provider to configure the server with a secure connection.
>+More details are available on this ,
>+blah blah blah'>

Single quotes look odd; if I've seen multiline entities wrapped in column zero like that I don't remember it (and if I really haven't, then l10n tool authors maybe haven't, and may break); blah blah blah

>+<!ENTITY incoming_settings.label       "Incoming settings:">
>+<!ENTITY outgoing_settings.label       "Outgoing settings:">
>+<!ENTITY incoming_is_checked.label       "I've double-checked the incoming settings.">

Are those aligned -2 from 0, or -2 from the ones that were +1?

>+<!ENTITY understood.label       "I know what I'm doing.">
>+<!ENTITY getmeout.label       "This is scary, let me out.">
>+
>+<!ENTITY getcertinfo.label       "Get information on securing email servers.">

Then the alignment wheels fell clear off. Personally, I don't actually care much between align and just one space after the entity name, but the variety of alignments is distracting.

>+<!ENTITY certinfo.url            "http://www.mozillamessaging.com/XXX/about/certs">

I very strongly doubt that you want that URL in a localized entity rather than in a pref, but if you do (you don't), you don't want it XXXed

>+<!ENTITY insecureSelfsigned.description
>+"Warning! This is an insecure server.  The server uses
>+a certificate that we can't trust, so we can't be sure that someone
>+isn't intercepting the traffic between Thunderbird and your server.
>+Thunderbird will let you get to your mail, but you should really get your
>+email provider to configure the server with a trusted certificate.
>+More details are available on this URL...,
>+blah blah blah">
>+
>+<!ENTITY secureServer.description
>+"Congratulations! This is a secure server, blah blah blah">

Double quotes, but again the wrapping looks surprising, and blah blah blah.

>diff --git a/mail/locales/en-US/chrome/messenger/accountCreation.properties b/mail/locales/en-US/chrome/messenger/accountCreation.properties
>new file mode 100644
>--- /dev/null
>+++ b/mail/locales/en-US/chrome/messenger/accountCreation.properties
>@@ -0,0 +1,39 @@
>+# accountCreation.properties
>+
>+cleartext_incoming=This is an insecure incoming server.  Data is sent in clear-text, so your password and email you receive could be read by third parties. 

For all of these, don't need two spaces between sentences, don't need the trailing space that's after several of them.

>+selfsigned_incoming=This is an mis-configured incoming server.  The certificate used by the server can't be trusted, so while the connection is encrypted, we can't be sure that the server is controlled by who it claims to be.

Either "misconfigured" (go go gadget spellchecker, saying that's wrong but misconfiguration is right), or perhaps better "incorrectly configured" or "incorrectly set up" - all of which still fail to adequately get across "You didn't do anything wrong, but your mail server was set up by someone who didn't know what they were doing."

>+cleartexturl=http://www.mozillamessaging.com/help/about/certs

And you certainly don't want to have that URL in both a localized entity without any guidance about how to localize it and also in this localized string, slightly different (help does look better than XXX) but still unlocalized and unnoted. You're much more likely to want to have it, once, in a pref like mailnews.start_page.welcome_url, including the probably-want of having the version as well as the locale in the URL, depending on what content's going to be there.

>+check_preconfig=Checking for preconfiguration...
>+checking_config=Checking for configuration of your account...
>+checking_mozilla_config=Checking for configuration provided by Mozilla community...
>+probing_config=Probing configuration...
>+checking_password=Checking password...

Real ellipses, rather than three dots, please…

>+incoming_server_exists=Incoming server already exists!
>+outgoing_server_exists=Outgoing server already exists!

Exciting, but not exciting enough for an exclamation point.

I didn't (yet) check for which strings are actually unused.

Comment 76

[Phil Ringnalda (:philor)]

+function RememberPassword(server, password)
+{
+  CC["@mozilla.org/passwordmanager;1"]
+    .getService(Components.interfaces.nsIPasswordManager)
+    .addUser(server.serverURI, "", password);
+}

Should be rememberPassword, CC is undefined (as it should be, for a single-use at two spaces indent where we don't really need the space-savings), and I doubt we want to save the password in wallet anymore :)

Comment 77

[Phil Ringnalda (:philor)]

+function sslLabel(val)
+{
+  switch(val)
+  {
+    case 1:
+      return gStringsBundle.getString("no_encryption");
+    case 2:
+      return "SSL"; // SSL and TLS shouldn't need to be translated
+    case 3:
+      return "TLS";
+    default:
+      gEmailWizardLogger.error("Asked to create an sslLabel for: " + val + '\n');
+      return "Unknown";
+  }
+}

It puts the START before the TLS, or it gets the hose again. But if they don't need to be translated, then we shouldn't already have them in multiple localized files, none of which have l10n notes saying not to localize them, some of which are nothing but "SSL" or "TLS" or "STARTTLS", and some of which *are* localized (ССЛ in sr, எஸ்எஸ்எல் in ta). Localize 'em.

Comment 78

[Ben Bucksch (:BenB)]

philor wrote:
> >+<!ENTITY certinfo.url            "http://www.mozillamessaging.com/XXX/about/certs">
> I very strongly doubt that you want that URL in a localized entity rather than
> in a pref

This is just a "more info" URL. It contains text that would have been too much to put in the dialog, but otherwise is just a long explanation of the dialog text. As such, it's inherently part of the localization (or help).
It's not a pref in that it would ever be changed by a user, does not affect how the system works etc..

I agree that it should appear only in one place, though.

Comment 79

[Phil Ringnalda (:philor)]

No, it should be a pref, because that's how partner builds and corporate deployments decide to have things like updates, or addons, or crash reports, or explanations of just how badly their certs suck, go to their server rather than ours.

And while it should be localized in the sense of http://www.mozillamessaging.com/ja-JP/about/certs (or certs?locale=ja-JP, depending mostly on what gozer likes), that shouldn't be done by localizers in a localized string file, it should be done by code fetching a pref and using the URLFormatterService to replace %LOCALE% and probably %VERSION%, if it now or later will say different things for different versions.

Comment 80

[Ben Bucksch (:BenB)]

philor, I once was a redistributor. I wouldn't want to change such a URL.
If so, I'd want it to be part of help or online help and use the same base URL.

Comment 81

[Philippe M. Chiasson (:gozer)]

(In reply to comment #79)
> [...]
> And while it should be localized in the sense of
> http://www.mozillamessaging.com/ja-JP/about/certs (or certs?locale=ja-JP,
> depending mostly on what gozer likes), that shouldn't be done by localizers in
> a localized string file, it should be done by code fetching a pref and using
> the URLFormatterService to replace %LOCALE% and probably %VERSION%, if it now
> or later will say different things for different versions.

The only general rule I care about is to separate all URLs that the client will be talking to in their own separate URL-space:

http://live.mozillamessaging.com/[...]

And whatever is there can redirect things all over the place, it doesn't matter,
but the idea is to keep all URLs that will be baked into the client in the same root, so we can treat it extra carefully.

Apart from that, how content is organized under there, not that important to me. Right now, we have:

http://live.mozillamessaging.com/thundebird/start
http://live.mozillamessaging.com/thunderbird/firstfun
http://live.mozillamessaging.com/thunderbird/whatsnew

Each get passed in a bunch of url arguments, i.e. ?locale=[xx]&version=[xx]&platform=[xx]

Comment 82

[Phil Ringnalda (:philor)]

(In reply to comment #80)
> philor, I once was a redistributor. I wouldn't want to change such a URL.

That's fine, as long as you don't confuse your own desires with those of other people. Corporate deployments don't _have_ to run their own AUS, but they can. Partner builds don't _have_ to provide their own help site, but they can.

> If so, I'd want it to be part of help or online help and use the same base URL.

Fine by me, if you can figure out a non-horrid system for making it clear that anyone who uses a different pref for app.support.baseURL is then required to support URLs x (y and z as time goes on), and can work out the website details with the way the MoMo support.baseURL currently sends everything off to mozilla.org. Not sure whether that would be twice as hard as just putting it in its own pref, or ten times, though.

Comment 83

[Phil Ringnalda (:philor)]

(In reply to comment #75)
> I didn't (yet) check for which strings are actually unused.

Only unused ones are

+thisurl=on this web page.
+cleartexturl=http://www.mozillamessaging.com/help/about/certs

Comment 84

[Phil Ringnalda (:philor)]

+var emailRE = /^(("[\w-\s]+")|([\w-]+(?:\.[\w-]+)*)|("[\w-\s]+")([\w-]+(?:\.[\w-]+)*))(@((?:[\w-]+\.)*\w[\w-]{0,66})\.([a-z]{2,6}(?:\.[a-z]{2})?)$)|(@\[?((25[0-5]\.|2[0-4][0-9]\.|1[0-9]{2}\.|[0-9]{1,2}\.))((25[0-5]|2[0-4][0-9]|1[0-9]{2}|[0-9]{1,2})\.){2}(25[0-5]|2[0-4][0-9]|1[0-9]{2}|[0-9]{1,2})\]?$)/i;

If we're going to go with the current very frustrating UI of just silently sitting there with no feedback or guidance when we don't like an email address, we need The Ultimate Email Regex. This one failed on the second address I tried, [e-mail].

Comment 85

[David :Bienvenu]

> 
> If we're going to go with the current very frustrating UI of just silently
> sitting there with no feedback or guidance when we don't like an email address,

we should give some sort of feedback when we don't like the e-mail address

> we need The Ultimate Email Regex. This one failed on the second address I
> tried, [e-mail].

Is there such a regex? A quick Google search didn't turn up anything. I think we want to err on the side of forgiveness since we're basically just using the e-mail address to get the host domain and the user name, and the user can override both.

Comment 86

[Ashley Bischoff (blog at handcoding.com)]

(In reply to comment #85)
> > we need The Ultimate Email Regex. This one failed on the second address I
> > tried, [e-mail].
> 
> Is there such a regex? A quick Google search didn't turn up anything. I think
> we want to err on the side of forgiveness since we're basically just using the
> e-mail address to get the host domain and the user name, and the user can
> override both.

The fellow who authored this page compared 11 different e-mail-based regexes and put each of them up against a test suite of 17 hypothetically-valid and 18 invalid e-mail addresses:

http://fightingforalostcause.net/misc/2006/compare-email-regex.php

In the end, he did find a "winner" that triggered a couple false positives within the test suite but was able to avoid triggering any false negatives. That regex is listed first on the page and, for convenience, I've also pasted it below:

/^[-_a-z0-9\'+*$^&%=~!?{}]++(?:\.[-_a-z0-9\'+*$^&%=~!?{}]+)*+@(?:(?![-.])[-a-z0-9.]+(?<![-.])\.[a-z]{2,6}|\d{1,3}(?:\.\d{1,3}){3})(?::\d++)?$/iD

Comment 87

[Phil Ringnalda (:philor)]

davida, clarkbw: how committed are we to keeping this "guess when the user is done typing their email address instead of having a [Set me up] button" UI? Even assuming someone rewrites Alex's regex to not need look-behind assertions, we're still absolutely certain to get false positives on the user being done, and go out and hit the network for something other than what they intend.

Having that actually do evil _probably_ requires a pretty contrived thing, like you're setting up your [e-mail] CNet account, get as far as entering your password, remember it's now evil.cnet.com, backspace through the last .com and then an alert in another program blurs us, and we wind up sending your password to evil.com while you're dealing with that, but having it surprise the user by going off to do something they didn't ask for or want is nearly certain.

Comment 88

[Ben Bucksch (:BenB)]

In case somebody doesn't know: I implemented this together with davida and bienvenu.
Given that you seem to be progressing to review, and I still considered this unfinished, I am looking into it a bit. The state is still scary, code quality is still "experimental". There are dozens of bugs still, on all levels, starting from design and logic bugs. Whatever I try falls on its nose. About half of these are logged in TODO, but there's much more. I don't think this is ready for review.
There are also UI changes that I discussed with Bryan in Barcelona.

I'd like to fix this, but it'll probably take me a week of concentrated work.

Comment 89

[David :Bienvenu]

BenB, thx for your offer of help. I don't think this feature has a chance of making it into the product unless we we get it reviewed and landed in the next week or two, so that the strings can be localized, and we can get feedback from users. The current patch makes it trivial to "remove" this feature from the UI since there's only a single menu item point of access.

I would like to fix the worst of the bugs without reworking the whole thing and putting it back into an unreviewable state. I'd also like to get a directory setup on the mozilla message site with .xml files for the most popular e-mail accounts so that we don't have to do the guessing at all for most users.

Comment 90

[David :Bienvenu]

> The only general rule I care about is to separate all URLs that the client will
> be talking to in their own separate URL-space:
> 
> http://live.mozillamessaging.com/[...]
> 
> And whatever is there can redirect things all over the place, it doesn't
> matter,
> but the idea is to keep all URLs that will be baked into the client in the same
> root, so we can treat it extra carefully.
> 
> Apart from that, how content is organized under there, not that important to
> me. Right now, we have:
> 
> http://live.mozillamessaging.com/thundebird/start
> http://live.mozillamessaging.com/thunderbird/firstfun
> http://live.mozillamessaging.com/thunderbird/whatsnew
> 
> Each get passed in a bunch of url arguments, i.e.
> ?locale=[xx]&version=[xx]&platform=[xx]

Gozer, I assume this also goes for the xml config files that we will be fetching for the common isp's like gmail, at&t, yahoo, etc? Eventually, we'd like to be able to populate these files from contributors or something like gerv's database, but it will take time to set up a process for that.  I'd like to get started, however, with some of the basic xml files - do you have any preference for what directory we put them in? I'm thinking something like http://live.mozillamessaging.com/emailconfig - I don't think these files are particularly Thunderbird-specific (e.g., SM could use them as well). I don't know if locales are important - I know gmail does things a little differently in Europe, for example, but I think they use a different host domain name, which would just be an extra file. We would probably still pass in the full url, and let redirects handling things on the server-side?

Comment 91

[Philippe M. Chiasson (:gozer)]

(In reply to comment #90)
> > [...]
> > 
> > http://live.mozillamessaging.com/thundebird/start
> > http://live.mozillamessaging.com/thunderbird/firstfun
> > http://live.mozillamessaging.com/thunderbird/whatsnew
> > 
> > Each get passed in a bunch of url arguments, i.e.
> > ?locale=[xx]&version=[xx]&platform=[xx]
> 
> Gozer, I assume this also goes for the xml config files that we will be
> fetching for the common isp's like gmail, at&t, yahoo, etc?

Yes, I'd treat them the same.

> Eventually, we'd
> like to be able to populate these files from contributors or something like
> gerv's database, but it will take time to set up a process for that.  I'd like
> to get started, however, with some of the basic xml files - do you have any
> preference for what directory we put them in? I'm thinking something like
> http://live.mozillamessaging.com/emailconfig

The idea is to have the client configured to talk to that url, even if under the
hood, that ends up redirecting somewhere else. So in this case, for simplicity,
we might setup

http://emailconfig.mozillamessaging.com/ for where the files actually live, but
keep the clients talking to http://live.mozillamessasging.com/emailconfig and blindly redirect for the time being.

> - I don't think these files are
> particularly Thunderbird-specific (e.g., SM could use them as well). I don't
> know if locales are important - I know gmail does things a little differently
> in Europe, for example, but I think they use a different host domain name,
> which would just be an extra file. We would probably still pass in the full
> url, and let redirects handling things on the server-side?

Yup, it's better to keep the smarts server-side, instead of in the client. The client passes in as much information as it can, and it lets the server decide how to deal with it.

Comment 92

[Ben Bucksch (:BenB)]

> I would like to fix the worst of the bugs

I fully understand why you'd prefer to changes to be kept small. Unfortunately, the bugs I see here are so fundamental and so severe and so many that I can't keep the changes confined, they'll need to be substantial.

That's also why I don't see any point right now to get any testing from users for this, as I just have to poke it a tiny bit for it to fall over badly, I find more and more bugs all the time, more than I can document. In fact, it's so bad that I didn't know where to start and got quite frustrated and consequently couldn't continue.

Comment 93

[David :Bienvenu]

Philor, thx very much for the review comments. I've addressed about half of them in the autoconfig repo, and will work on the other half asap. One thing - perhaps I'm doing something wrong, but on windows, TB isn't very happy with ellipses in .properties files - it complains on load that they're not utf8 characters.

Comment 94

[[e-mail]]

(In reply to comment #93)
> perhaps I'm doing something wrong, but on windows, TB isn't very happy with
> ellipses in .properties files - it complains on load that they're not utf8
> characters.
Your editor is probably not set up to edit in UTF-8... in Windows-1252, an ellipsis is 0x85 but what you want is a U+2026 which is 0xE2 0x80 0xA6 in UTF-8 or ò€¦ in Windows-1252 (ironically, no saving in bytes!)

Note that some UTF-8 editors insert a UTF-8 BOM which we don't want.

Comment 95

[David :Bienvenu]

Thx, Neil - yes, it definitely seems to be an insert time/edit issue, not a save issue per se.  I'm using dev studio, but I think I'll just use xcode for this for now.

Comment 96

[Phil Ringnalda (:philor)]

If you haven't already done rememberPassword, my working-so-it-stops-annoying-me-testing version is

function rememberPassword(server, password)
{
  if (server instanceof Components.interfaces.nsIMsgIncomingServer)
    var passwordURI = server.type + "://" + server.hostName;
  else if (server instanceof Components.interfaces.nsISmtpServer)
    var passwordURI = "smtp://" + server.hostname;
   
  if (passwordURI)
  {
    let lm = Components.classes["@mozilla.org/login-manager;1"]
                       .getService(Components.interfaces.nsILoginManager);
    let login = Components.classes["@mozilla.org/login-manager/loginInfo;1"]
                          .createInstance(Components.interfaces.nsILoginInfo);  
    login.init(passwordURI, null, passwordURI, server.username, password, "",
               "");
    lm.addLogin(login);
  }
}

(smtp's untested, since we don't verifyLogon for outgoing servers, so unless it's coming from XML we won't ever think we need auth, which is a bit of problem)

Comment 97

[David :Bienvenu]

thx, Phil, I'll put that it into the autoconfig repo, and then into the next patch I generate.

Whether things work with xml or probing seems to depend on who touched the code last :-(

Comment 98

[David :Bienvenu]

> If we're going to go with the current very frustrating UI of just silently
> sitting there with no feedback or guidance when we don't like an email address,

If I enter a bad e-mail address, I get a big red error message above the e-mail address field, when I leave the field.

Comment 99

[David :Bienvenu]

> 
> The fellow who authored this page compared 11 different e-mail-based regexes
> and put each of them up against a test suite of 17 hypothetically-valid and 18
> invalid e-mail addresses:
> 
> http://fightingforalostcause.net/misc/2006/compare-email-regex.php
> 
> In the end, he did find a "winner" that triggered a couple false positives
> within the test suite but was able to avoid triggering any false negatives.
> That regex is listed first on the page and, for convenience, I've also pasted
> it below:
> 
> /^[-_a-z0-9\'+*$^&%=~!?{}]++(?:\.[-_a-z0-9\'+*$^&%=~!?{}]+)*+@(?:(?![-.])[-a-z0-9.]+(?<![-.])\.[a-z]{2,6}|\d{1,3}(?:\.\d{1,3}){3})(?::\d++)?$/iD


JS doesn't seem to like this regexp, and my regexp foo is woefully inadequate.

Comment 100

[Ben Bucksch (:BenB)]

> > If we're going to go with the current very frustrating UI of just silently
> > sitting there with no feedback or guidance when we don't like an email
> > address,

> If I enter a bad e-mail address, I get a big red error message above the e-mail
> address field, when I leave the field.

Yes, I fixed that last night. Problem was merely a missing error msg in the string bundle.

Comment 101

[Ben Bucksch (:BenB)]

*cough* *cough*
          let sslType = sslLabel(config.incoming.socketType);
          switch (sslType) {
            case 'SSL':
            case 'TLS': ...
            case 'No encryption':
sslLabel() is the *translated*, user-visible text. The whole point of config.incoming.socketType being an int was that it's a reliably processable enum. I'll fix that.

Comment 102

[Ben Bucksch (:BenB)]

philor wrote:
> If you haven't already done rememberPassword, my
> working-so-it-stops-annoying-me-testing version is
> function rememberPassword(server, password)
>     let lm = Components.classes["@mozilla.org/login-manager;1"]

Error: Cc['@mozilla.org/login-manager;1'] is undefined

Comment 103

[[e-mail]]

(In reply to comment #102)
> (In reply to comment #96)
> > If you haven't already done rememberPassword, my
> > working-so-it-stops-annoying-me-testing version is
> > function rememberPassword(server, password)
> >     let lm = Components.classes["@mozilla.org/login-manager;1"]
> Error: Cc['@mozilla.org/login-manager;1'] is undefined
Upgrade your wallet build to a login manager build?

Comment 104

[David :Bienvenu]

I may need to re-merge with the trunk for that to work - I'll do that now.

Comment 105

[Ben Bucksch (:BenB)]

+  try {
     lm.addLogin(login);
+  } catch (e if e.message.indexOf("This login already exists") != -1) {
+    // TODO modify
   }

Comment 106

[Phil Ringnalda (:philor)]

Oops, I just wanted to see _something_ work. lm.findLogins({}, passwordURI, null, passwordURI), iterate through them looking for one with server.username == login.username and modifyLogin to change the password if it's different, else save, I think is right (and should be fairly copyable from nsLoginManagerPrompter.js).

Comment 107

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #99)
> 
> JS doesn't seem to like this regexp, and my regexp foo is woefully inadequate.

The O'Reilly regexp book is pretty good; I can lend you my copy if you want to try and use it along with MDC to debug that...

Comment 108

[Ben Bucksch (:BenB)]

- Fixed SMTP server selection (full day of work)
- Fixed manual hostname editing a bit, but still broken
- Implemented custom input fields and "enable URLs" from config XML files.
- Stopped verifyConfig() from guessing and downgrading login auth and
  username form when config comes from config files.
- Localized JS logic files (error messages).
  (The frontend - the vast majority - has been done by bienvenu earlier.)
  Localization is complete now, if I haven't overlooked anything.

Outstanding, high priority:
- State changes / transitions (user edits various fields at various points) buggy to completely broken.

Comment 109

[David :Bienvenu]

Phil, do you have stuff you want to land? I'd like to make an other pass at your review comments but I don't want to duplicate stuff you've already done.

Comment 110

[Phil Ringnalda (:philor)]

You won't - I've been doing stuff I hadn't yet mentioned, mostly whitespace and style. The only mentioned part I've got is changing the regex to

^[-_a-z0-9\'+*$^&%=~!?{}]+(?:\.[-_a-z0-9\'+*$^&%=~!?{}]+)*@(?:[-a-z0-9.]+\.[a-z]{2,6}|\d{1,3}(?:\.\d{1,3}){3})(?::\d+)?$

(which is the "will allow @-foo-.com but who cares?" version) which I've been pretending I'll test more, but probably won't.

Comment 111

[David :Bienvenu]

we're not going to hold b2 for this, but I'd really like to push to get it into b2

Comment 112

[Ben Bucksch (:BenB)]

> I've been doing stuff I hadn't yet mentioned, mostly whitespace and style

You're working off the autoconfig branch, though, are you? Because I see no checkins apart from mine in the last days, and I made a lot of changes, too.

Comment 113

[Phil Ringnalda (:philor)]

Yeah, that's because I haven't pushed anything. What I *meant* to tell bienvenu was that I would just push nits like trailing whitespace and style, rather than do enormous review comments citing 60 lines with this, and 50 lines with that, and 45 lines with the other, but I still haven't quite figured out how to do that to a moving target, other than maybe make a list and just keep grepping, or do nit after-the-fact reviews on every push that touches anything I've already done.

Comment 114

[Ben Bucksch (:BenB)]

Well, either you state here what you dislike, e.g. "no trailing whitespace" or "replace ' with " ", or you do it yourself, all at once within 2 hours, and commit it immediately. If you do, please do not mix style and whitespace changes with real code changes.

Comment 115

[David :Bienvenu]

we seem to have lost the progress widgets, at least on windows - I don't see any progress happening - didn't that used to work on Windows? I'll try the mac again...

Comment 116

[Renato S. Yamane]

I see in David Ascher blog (that is commented in Thunderbird Beta1 Preview Release) a new feature (autoconfig), but I test it with no success.

So, I submit a bug: https://bugzilla.mozilla.org/show_bug.cgi?id=477138

But Banner says that this feature is not available yet, and *maybe* spreadsheet available in https://wiki.mozilla.org/MailServerList is not used by developers to develop autoconfig.

I think that this spreadsheet have a lot of good information about mail servers.

Best regards,
Renato

Comment 117

[David :Bienvenu]

Comment on attachment 356817 [details] [diff] [review]
more wip

marking obsolete - we've moved on a bit.

Comment 118

[David :Bienvenu]

putting in m3 - Bryan, please push back if you don't think we're going to make that.

Comment 119

[David :Bienvenu]

Attachment: patch for review

Phil, do you think you could have a look at this? Bryan and I have changed the way this works a bit - now, if you need to edit settings because autodetect won't work for your account, you need to stop the autodiscovery process, make your changes, and then press go.

I've set up all my accounts on my new machine with autoconfig, so most things have worked at one point or an other, though I may have messed up generating a diff between the autoconfig repo and the trunk. I'm using your e-mail address regexp, I believe...

Comment 120

[Phil Ringnalda (:philor)]

Pushed trailing whitespace removal and __end removal to the autoconfig repo.

Comment 121

[Phil Ringnalda (:philor)]

Looks like chrome://messenger/skin/identity.png as (not) seen in the tooltip for the green button for a happy secure server didn't manage to actually land.

Comment 122

[David :Bienvenu]

do you mean in the autoconfig repo or in my patch? If the latter, I'll go make sure it gets added to my trunk repo so it'll be in the diffs...

Comment 123

[Phil Ringnalda (:philor)]

The autoconfig repo: looks like the intention was to copy http://mxr.mozilla.org/comm-central/source/mozilla/browser/themes/winstripe/browser/identity.png (and the other two themes) and then use just the middle third of it, so it should really be just that chunk, and drop the -moz-image-region.

Comment 124

[Phil Ringnalda (:philor)]

Pushed three identity.png icons, and the changes to jar.mn and accountCreation.css to go with them, to the autoconfig repo.

But, hmm, poor Gnomestripe doesn't seem to have gotten any of the other icons at all, have to do something about that once I remove the spinner.gif one, since we have perfectly usable throbbers already.

Comment 125

[Phil Ringnalda (:philor)]

Got rid of spinner.gif, and copied over the Qute icons to Gnomestripe so it'll build. Hope you've got a fairly easy way to go from the autoconfig repo to a patch :)

Comment 126

[Phil Ringnalda (:philor)]

While removing unused strings, I tripped over the customfields and enableURLs stuff. I know BenB's going to hit the roof, but I don't see any sign that that's ready to land. Our rudimentary collection of config files only includes one with one enableURL which isn't commented out, and since there appears to be no way to get past the t-online.de file to get to the t-online.de imap ssl file which uses only one of those two features, we're not only just guessing that it works, we're also expecting localizers to translate some... rather vague strings without any way to see them (and for that matter, expecting ui-r on something that Bryan's not going to even see).

Comment 127

[David :Bienvenu]

We removed the UI for the enableURLs stuff because it was ui-r, but I didn't remove the strings and code because at some point we would like to have some sort of UI for this.

I'm not sure about the t-online.de stuff...

Comment 128

[Ben Bucksch (:BenB)]

enableURL is required for gmail

Comment 129

[Ben Bucksch (:BenB)]

enableURL is required for gmail.
I'll also use it to notify people that Hotmail can't work (without extra software).
customfields are needed for many, many ISPs.

Can we get that in, please? The UI wasn't any more broken than the rest of the UI ;-).

Comment 130

[David :Bienvenu]

Attachment: patch against trunk with philor's recent changes

Comment 131

[Phil Ringnalda (:philor)]

Comment on attachment 371248 [details] [diff] [review]
patch against trunk with philor's recent changes

>--- a/mail/locales/en-US/chrome/messenger/messenger.dtd
>+++ b/mail/locales/en-US/chrome/messenger/messenger.dtd
>+<!ENTITY openMessageMenu.label "Open">

That unused entity is just *desperate* to make its way back into the tree, isn't it?

Comment 132

[David :Bienvenu]

ah, sorry, I'll fix that.

Comment 133

[Phil Ringnalda (:philor)]

(In reply to comment #129)
> enableURL is required for gmail.

For Gmail *IMAP*. For which https://bugzilla.mozilla.org/buglist.cgi?quicksearch=gmail+imap might be a better URL.

> I'll also use it to notify people that Hotmail can't work (without extra
> software).

You might have, before March 14th. Now, that's no longer the case.

> customfields are needed for many, many ISPs.

Then we need a config file which uses them, *before* we land strings, so localizers have fighting chance of seeing what they are doing.

Though the 'spec' on the wiki doesn't exactly make it sound like we need a complicated extensible system allowing for every possible type of data and up to five different things with a vaguely worded localized generalized description and then non-localized labels so much as we need to optionally allow for either one or two "username" fields, which could then have localized labels.

Comment 134

[Phil Ringnalda (:philor)]

Some things I don't currently know how to fix, offhand:

* The bad cert listener is still rather fragile (or maybe just broken): for email:[e-mail] password:goats (they deserve the suspicious-looking traffic, for exposing such broken crap even unintentionally), we seem to think we've discovered a secure POP server, at pop.harborside.com:993, but then we throw up an exception dialog with "imaps://pop.harborside.com" as the location, throw "Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.port]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://pippki/content/exceptionDialog.js :: getURI :: line 220"  data: no]" in the console, spin pretending we're thinking about it until you get bored and hit the Cancel button, do the same thing twice more, and then say it's an insecure server. Then, for real fun, hit the "Back" button under the address/password, then without changing anything hit "Next" and we'll say it's a lovely green secure server.

* Failure to log in breaks the dialog. With the goats@/goats, hit "Create account" and the failure to log in puts up a "FAIL" dialog, but also shoves the right side of the main window over a little with an "Username or password invalid" (which I bet is then a huge shove with a language where that has to be longer), and shoves the "Edit" and "Create Account" buttons halfway out of sight, because the "Configuration could not be verified -- is the username or password wrong?" string is too long and not wrapping, which probably means that things are broken for more than just that one string in more verbose locales than English.

* Since "Go to advanced settings" is actually "Create this account right now, or die trying" it really shouldn't be there acting like it will work and trying to work from the very second the window comes up, when it actually won't work more often than not.

Comment 135

[Phil Ringnalda (:philor)]

* Not sure what we need to do to make the tooltips with insecureCleartext.description or insecureSelfsigned.description wrap, but they're currently a single very long line (and while someone's in the neighborhood, MDC and I don't think "bottom" is a valid value for the align attribute).

Comment 136

[Phil Ringnalda (:philor)]

Reason to want to drop the unused enableURL strings number n: s/Thunderbird/brandShortName/ for entities was simple, but there are two Thunderbird instances in .properties. One is in findoutmore, which as the TODO comment in the code says should move to an entity, the other is in an enableURL string, so we'll have to include a branding <stringbundle> to brand an unused string.

So, if enableURL failed ui-r, what makes us think it will at some point pass ui-r, what will the changes look like, and why do we think we'll make them later rather than now?

Comment 137

[Phil Ringnalda (:philor)]

* If you don't already have an SMTP server configured, the menulist doesn't show anything at all (probably not a huge concern unless/until we decide to use autoconfig for the first-run first-account wizard, since my way of getting there by creating an RSS account to get past first-run isn't wildly common).

Comment 138

[David :Bienvenu]

Ultimately, autoconfig will be used for the first run account wizard, so we'll have to figure out the smtp server list for that case. It should be a text box at that point, and I believe it works as one now.

Re the cert error, there are a couple ways we encounter cert errors - one is during port probing, if we're able to connect via the SSL port, the other is if we can't connect via the SSL port but end up doing STARTTLS during verifyLogon. It sounds like you encountered the first one...the test case I had access to only did TLS so I've never exercised that code (I think davida wrote that...).

I don't know what the plan for using enableURL might be - I know Bryan did not like the UI but I'm not sure what the plan for getting a better UI is.

Comment 139

[Phil Ringnalda (:philor)]

Note to self: don't forget that .larger-button looks like pure evil except for pinstripe which doesn't style it, and that the style on #back_button to make it look disabled when it isn't disabled is... not entirely in line with typical UI guidelines.

Comment 140

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/users/dascher_mozilla.com/autoconfig/rev/ecac1d23a1b8 - moved one of the two .properties strings with a hardcoded "Thunderbird" out to the .dtd, but I had trouble coming up with a more accurate name than the previous inaccurate findoutmore, so it probably still needs a rename.

Comment 141

[Phil Ringnalda (:philor)]

Several notes-to-self:

* a fair share of the CSS doesn't actually apply to anything

* functions with no callers are evil; global functions with no callers are doubly-evil

* functions only called by uncalled functions aren't less evil

* semicolons at the end of lines may be optional to the interpreter, they aren't optional to the reviewer

* wrapping is good, random indents and random start-of-wrapped-line chars aren't

* let kDebug = true;

* catch (e) { alert(e.message); throw e; }

* TODO

* var gEnableURLsDialog

Comment 142

[Ben Bucksch (:BenB)]

FWIW, I had discussed and agreed the general flow for enableURL and customfields (and the other dialog screens) with Bryan in Barcelona. The current UI looks horrible, because it depended on the main dialog flow / logic to be fixed first. I'll take a look at the new code, and try to fix the review comments, in the next days (probably starting Friday).

Comment 143

[David :Bienvenu]

I recreated the harborside problem - I think at that point in the process we're supposed to be trying to do a temporary override of the cert without bringing up the UI, which is what we do for the smtp server. The reason the accept cert dialog is broken is that it got put up synchronously, which doesn't give the nss code a chance at the event loop to fetch the cert. The verifyLogon code which intentionally puts up the cert dialog has to do it on a timeout from the actual notification of the cert error.

Why the pop/imap confusion, I'm not sure. It may be that we discovered the imap port working after the pop3 port, and if we hadn't got the cert exception, we would have updated the UI to indicate the imap server. But since we prefer IMAP over POP, and secure over non-secure, I'm not sure why that would be.

Comment 144

[David :Bienvenu]

(In reply to comment #134)

> * Since "Go to advanced settings" is actually "Create this account right now,
> or die trying" it really shouldn't be there acting like it will work and trying
> to work from the very second the window comes up, when it actually won't work
> more often than not.

I've talked with Bryan about the need to fix that, but we haven't reached any UI conclusions. One thought is to change the label to something like Create and edit. We may want to only enable it if the auto probing has succeeded, or the user has stopped auto probing.

Comment 145

[Phil Ringnalda (:philor)]

For the dunno pile: the hostname fields and the email address field should have the class uri-element, to force them to be LTR even in an RTL locale, but the emptytext for the email address field shouldn't.

Comment 146

[David :Bienvenu]

The cert override stuff is weird - if I set breakpoints in venkman, I don't get the cert exception dialogs so I have a feeling this is some sort of timing issue...

Comment 147

[Dan Mosedale (:dmosedale, :dmose)]

In my experience, Venkman breakpoints aren't terribly reliable; it may be worth adding dump statements double-check.

Comment 148

[Phil Ringnalda (:philor)]

The way that using the Back button and then trying again doesn't give the dialogs again, while not proof of a race, would at least be nicely explained by one. (You are starting over from scratch after having gotten the dialogs, right? I usually go for a whole new profile to be sure nobody's caching certs or the lack thereof.)

Comment 149

[David :Bienvenu]

my issue is that the breakpoints *were* getting hit, and that changed the timing/event loop stuff in such a way that the error did not happen...

Phil, I landed some semicolon fixes, and Bryan's going to have a look at the css and remove stuff that's not used. And I'll try to figure out why Billy Goat can't read e-mail.

Comment 150

[David :Bienvenu]

(In reply to comment #148)
> The way that using the Back button and then trying again doesn't give the
> dialogs again, while not proof of a race, would at least be nicely explained by
> one. (You are starting over from scratch after having gotten the dialogs,
> right? I usually go for a whole new profile to be sure nobody's caching certs
> or the lack thereof.)

I'm going into options and deleting the cert exceptions. We're supposed to be adding temporary cert exceptions in our port probing but the exceptions seem to survive a restart, which is odd.

Comment 151

[Ben Bucksch (:BenB)]

> The way that using the Back button and then trying again doesn't give the
> dialogs again, while not proof of a race, would at least be nicely explained by
> one.

I think (cert stuff is not my code) that behaviour is intentional - once you confirmed the scary dialogs, you don't want to see them again - at least not for the same hostname (they should show again for a different hostname, but don't IIRC).

Comment 152

[Phil Ringnalda (:philor)]

Except that I've *never* confirmed them, because we've never fetched anything we would let me confirm: we throw up a dialog, with a missing port according to the console error, never enable the "confirm exception" button, and all I do is hit the "cancel" button three times.

Comment 153

[David :Bienvenu]

Phil's right - and it's not your code, Ben, I don't think. I believe it's DavidA's or mine, and I see the bug(s) now - we've hardcoded the url to imaps, among other problems. NSS gave us a perfectly good targetsite, but we forgot it along the way, so I just need to pass that along.

Comment 154

[David :Bienvenu]

Actually, I'm not sure why the cert exception is permanent, but I'll deal with that after I fix the other issues.

Comment 155

[Ben Bucksch (:BenB)]

> I'm not sure why the cert exception is permanent

You mean the "Yes, I know the cert is insecure" red dialog of the account wizard adding a permanent cert exception? That is intentional, though, for all I know: We want to remember that the user manually confirmed that this cert is fine, and not warn him about this one anymore (e.g. when checking mail).

Comment 156

[[e-mail]]

Not actually having read the patch, I was wondering whether the code was sufficiently modular to provide for a "Test manual settings" button in a) the account wizard b) the account manager.

Comment 157

[David :Bienvenu]

I think it's permanent because we never remove the override - we return before we hit the code that removes the override.

I have the cert exception dialog code working better in my tree - it puts in the right site url, and allows the user to confirm the exception. It pops up twice for the goats account, which is a small issue :-) But the larger issue is, do we want the cert exception dialog at all, or do we want to use the UI BenB was referring to, which is way downstream, i.e., the scary dialogs, which at this point are a bit redundant.

Re Neil's question, it seems like the nsIMsgIncomingServer's verifyLogon method could give you most of what you want for Test manual settings.

Comment 158

[David :Bienvenu]

OK, I've fixed up the cert exception dialog so that it has the right url. I'm still working on figuring out why we get two of them, and fixing that.

Phil, I still can't logon to that account with that user name and password - should I be able to? If so, what should my settings have been? (You can e-mail me privately if you want...)

Comment 159

[Phil Ringnalda (:philor)]

Heh, no, you shouldn't: they're actually my first-ever ISP, not something I control to create working world-accessible accounts with. Real login in the mail, go ahead and delete any spam you happen to find there ;)

Comment 160

[Ben Bucksch (:BenB)]

> the larger issue is, do we want the cert exception dialog at all,
> or do we want to use the UI BenB was referring to, which is way downstream,
> i.e., the scary dialogs, which at this point are a bit redundant.

I think our red dialog is supposed to replace the standard PSM invalid cert dialog. But Bryan and DavidA will know better :).

Comment 161

[Phil Ringnalda (:philor)]

* msgNewMailAccount() needs to either not openDialog with a window name, or use whatsit, toOpenWindowByType or some such, depending on whether we want a second click of the menuitem to open a second autoconfig window or just find the existing one - right now, a second click of the menuitem does nothing.

Comment 162

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #142)
> FWIW, I had discussed and agreed the general flow for enableURL and
> customfields (and the other dialog screens) with Bryan in Barcelona.

To be clear on this.  

We agreed that the enableURL could be helpful and that we need to figure out a way to work in custom fields.  However the enableURL as it was implemented was a "force the user to click on the URL before they can proceed" interaction which we didn't agree was correct.  That kind of system is not only annoying to us as people trying to test and develop but will be to other people who aren't stupid.  That system of interaction will not go into the final product.

What we can do with the enableURL is offer it as a helpful option when our effort to login to the IMAP server fails.  I understand there is no easy way of knowing why the login failed, essentially we have to try to help the person 'debug' what went wrong: password, username, IMAP enabled.  We haven't put much effort toward designing this part yet so it definitely fails to be helpful right now.

For the custom fields I didn't want to insert them into the current dialog, but add them as an extra dialog to the config dialog.  Nothing about the custom fields requires guessing usernames or ID codes, those are all provided by the ISP and therefore don't require space in the interactive configuration area.  We can add a button to open the dialog along with a green light / red light for indicating that the values are correct or incorrect.  Likely the dialog can open on it's own when custom configuration values are detected to be necessary.

Comment 163

[Ben Bucksch (:BenB)]

Bryan, what you describe was your stance *before* we discussed it constructively and came to an agreement that we both liked.

Part of the problem was that we cannot even reliably detect a failure. In my testing, I had many cases (for different reasons) were we'd just be stalling for 2 minutes and eventually time out, because a) DNS doesn't resolve, b) the POP server simply doesn't answer (it doesn't like the user account / our IP address) etc.. So, while I agree this "show help on error" sounds reasonable in theory, it simply does not work in practice, in my real world experience. I tried many different ISPs and accounts in my testing. It was a horrible experience (despite me knowing the correct settings), and I want to save our users from that.

We had discussed that and several problems in the logic flow (and resulting bugs) of the dialog. The agreement, as I remember it, was that we'll have a dialog with several modes/parts/pages (the left side with name, email, password stays the same, and the right part changes). enableURL and customfields were just two of those changing right parts. The other right parts are: nothing (empty), hostname guessing, manual hostname editing, final configuration/hostname display, and (possibly) cert warning. The parts are sequential and the transition happens with explicit Next buttons.

Comment 164

[Phil Ringnalda (:philor)]

Can anyone persuade me that there's a reason to keep validatePassword? Right now, if you tab into and then out of the password field, we put up a lying error message, saying "Login did not succeed...", turn the emptytext into eight password bullet characters, and leave a blinking caret there, even though it doesn't have focus. While I don't doubt that's all fixable, I don't see blur as being the right time to be complaining about a zero-length password: as the error string says, when the Login did not succeed is plenty soon enough.

Comment 165

[Ben Bucksch (:BenB)]

Phil, this and other cases - we have far worse situations - are why we should have explicit Next buttons (see my last comment) instead of relying on blur or the user knowing what he has to do next.

Comment 166

[Phil Ringnalda (:philor)]

You haven't built autoconfig for a while, have you? We have an explicit Next button. We also have emptytext instead of labels, so actually I can't just get rid of it, at the very least I have to have validatePassword turn it back into text so the emptytext shows up again when you go in-n-out.

Comment 167

[Bryan Clark (DevTools PM) [:clarkbw]]

phil: yeah that should be changed, I think the blur event was carried over from before.

Comment 168

[Phil Ringnalda (:philor)]

Apparently I've gotten so license-block blind that I didn't even notice that emailWizard.xul is missing one, and needs to have it.

Comment 169

[David :Bienvenu]

Attachment: patch to enable autoconfig

I've landed most of the autoconfig stuff on the trunk, as NPOTB - this patch is what you need to apply to turn on the autoconfig UI on the trunk.

Technically, I probably shouldn't have added the jar file changes, or messenger.dtd, but nightly users won't see the changes.

Comment 170

[Phil Ringnalda (:philor)]

Ack, or the strings, since that now means that any change of string text requires a change of entity name/key.

Comment 171

[David :Bienvenu]

(In reply to comment #170)
> Ack, or the strings, since that now means that any change of string text
> requires a change of entity name/key.

I did not add the string files to the mail jar files (I only landed the theme jar file changes). I would hope the localizers would ignore property files that weren't part of the product though that's probably too much to hope for :-(

Comment 172

[Phil Ringnalda (:philor)]

'Fraid so: even as we type, the quickest locales are looking at pages like http://l10n.mozilla.org/buildbot/compare/hg-comm-langpack/18487 and localizing away.

Comment 173

[Phil Ringnalda (:philor)]

Attachment: some better patch to enable autoconfig

We still need a UI decision on whether a second click on the menuitem should open a second autoconfig window or should find the existing one, but at least this version doesn't break SeaMonkey's accountcentral link, and points us at the HTTPS MoMo config server.

Comment 174

[Dan Mosedale (:dmosedale, :dmose)]

Ugh, I didn't think about the l10n thing when I proposed going ahead with the landing; my apologies.

So in the Firefox 3 timeframe, the way this problem was worked around was that Axel encouraged developers to put all their strings in code directories while development was in progress, and move them into the locale directory once they were more or less done and the strings weren't changed.

Since Mercurial is better at moves than CVS ever was, we could conceivably try and do that now, post facto.  I'm a little unsure whether that cure is worse than the disease, since it could be a frustrating experience for the localizers who have already started.  Adding sipaq to the CC for his thoughts here.

clarkbw: do you have a very rough feel for how much string churn we're likely to see here before we're mostly done churning?

Comment 175

[Dan Mosedale (:dmosedale, :dmose)]

s/strings weren't changed/strings weren't being changed much/

Comment 176

[Phil Ringnalda (:philor)]

Not counting things where Bryan doesn't like the wording, there's certinfo.url, depending on how it goes 10 to 16 customfields/enableURLs strings, one of the pair of cleartext strings where in one we say "email" and the other we say "email and password", the two where we have double-hyphens for an em dash, and perhaps ten or fifteen removals where we're localizing what I think are developer-targetted console error messages like "enum value not supported" that I'm inclined to say we shouldn't be making our localizers try to translate.

Comment 177

[Bryan Clark (DevTools PM) [:clarkbw]]

i haven't done a real cleanup of the strings in there at all.  many of them are left overs of the first attempt.  I would plan for a good amount of churn I guess.  Even for the strings in there that we plan to keep they don't likely have the final wording as that takes a while and really needs everything else together before it can happen.

Comment 178

[Ben Bucksch (:BenB)]

There used to be a way to put translation notes in there as message to the translator, IIRC in form of comments, e.g. "DO NOT TRANSLATE" (for "Firefox" etc.). You might just put a note "Do not translate before May" or somesuch at the top of each file.

Comment 179

[Phil Ringnalda (:philor)]

If someone's in a position to read a l10n note in the en-US file, then they haven't started translating and thus are in a position to be saved by a quick move to content. If not, they won't have any reason to be looking back at the en-US file. And from what I've heard about the lack of support for "ENTIRE FILE" l10n notes, it might not even ever be seen.

It's also cruel to say "leave your tinderbox red for four weeks, and every time you check how you're doing, dig through the details and ignore these 100+ issues" - it's exactly equivalent to landing a patch that turns the code tinderbox red, and saying "eh, leave it for four weeks, and just read the build log to see if there are any new errors."

Comment 180

[Phil Ringnalda (:philor)]

Attachment: Unrotted patch to enable

A wee bit unfortunate to be dropping an ifdef right back into mailnews/jar.mn minutes after the previous ifdef went away, but this way it's where it belongs when SM's ready to jar it.

Comment 181

[Phil Ringnalda (:philor)]

* When we give up on guessing (@hotmail.com is handy for that, since it's pop3.live.com), you can edit the hostname like we tell you to, but we have an Edit button rather than a Go button. It should either still be disabled, or the button should change to Go (or disappear, if what we really mean is "we're bored with this, fix it however you want and then Create account").

* When you change the port and security (@hotmail.com is handy for that, since you want 993 and SSL/TLS, but we leave you at 110 and none), we then ignore that and start over at 110 and STARTTLS.

Comment 182

[Simon Paquet [:sipaq]]

(In reply to comment #174)
> Ugh, I didn't think about the l10n thing when I proposed going ahead with the
> landing; my apologies.
> 
> So in the Firefox 3 timeframe, the way this problem was worked around was 
> that Axel encouraged developers to put all their strings in code directories 
> while development was in progress, and move them into the locale directory 
> once they were more or less done and the strings weren't changed much.
> 
> Since Mercurial is better at moves than CVS ever was, we could conceivably 
> try and do that now, post facto.  I'm a little unsure whether that cure is 
> worse than the disease, since it could be a frustrating experience for the 
> localizers who have already started.  Adding sipaq to the CC for his thoughts 
> here.

That would be the best solution. Localizers who live on the bleeding edge know 
what they are doing, so this would be acceptable for them. Nevertheless it 
would be polite to inform of them of this decision via a posting in mozilla.dev.l10n

Oh, and once you move the strings back from content/ into locales/ please make sure to remove all obsolete leftovers. Because we've tried hard in the last few months to eliminate all obsolete strings.

Comment 183

[Robert Kaiser]

(In reply to comment #182)
> That would be the best solution.

I don't agree 100% here, though it's probably the best of the options for this particular scenario. The best real solution is not landing things that are not ready. If you want to land it because you think it might get ready and needs more widespread nightly testing, you might want to just have the strings in a state that warrants widespread testing as well, including comments from localizers if there are problematic strings or string usages in there.
IMHO, landing experimental stuff that is known to not be ready enough for that is wrong in the first place, but it seems to have happened already in this case.

Comment 184

[Simon Paquet [:sipaq]]

Let me just add that Robert is 100% right here in a general sense, that being that things should not be landed in the general nightly builds until they are ready. If they aren't, you should use a try-server build instead.

In this particular situation however, where this was already landed (in error), the proposal from dmose' comment 174 is the best course of action.

Comment 185

[Phil Ringnalda (:philor)]

Strings moved in http://hg.mozilla.org/comm-central/rev/4e12018f2e8c

Comment 186

[Phil Ringnalda (:philor)]

Attachment: Patch to enable with content strings

Comment 187

[Phil Ringnalda (:philor)]

Attachment: Review nits, round 1 - checked in

I tried to keep to mostly whitespace and wrapping and stray commented-out stuff this round, though some other things slipped in.

Comment 188

[Phil Ringnalda (:philor)]

Attachment: Make email address and hostnames ltr in rtl locales - checked in

Turns out I was wrong in comment 145, and emptytext is smarter than me. Even if you set class="uri-element" and thus direction: ltr;, the emptytext still takes the document direction, so you have [             כתובת דוא״ל] but when you start typing you get [me@foo          ].

Comment 189

[Phil Ringnalda (:philor)]

(In reply to comment #168)
> Apparently I've gotten so license-block blind that I didn't even notice that
> emailWizard.xul is missing one, and needs to have it.

Fixed.

Comment 190

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #183)
> 
> I don't agree 100% here, though it's probably the best of the options for this
> particular scenario. The best real solution is not landing things that are not
> ready. If you want to land it because you think it might get ready and needs
> more widespread nightly testing, you might want to just have the strings in a
> state that warrants widespread testing as well, including comments from
> localizers if there are problematic strings or string usages in there.
> IMHO, landing experimental stuff that is known to not be ready enough for that
> is wrong in the first place, but it seems to have happened already in this
> case.

(In reply to comment #184)
> Let me just add that Robert is 100% right here in a general sense, that being
> that things should not be landed in the general nightly builds until they are
> ready. If they aren't, you should use a try-server build instead.

There's plenty of history at Mozilla of landing features at various stages of maturity for a variety of reasons, and it's generally a judgment call: there's no one-size-fits-all policy to be had here.  As I mentioned in comment 174, the lead localizer for Firefox (Axel) encouraged all front-end feature developers to land strings in content before they were ready to be localized; I see no evidence at all that if we had actually done that in this case that it would have somehow been "wrong".

The error here, which I take responsibility for, is not thinking through the l10n consequences of the early landing before suggesting going forward.

Comment 191

[Phil Ringnalda (:philor)]

* emailWizard.xul has a non-localized |<label class="heading">Account Information</label>| (wonder how many times I've looked at that without seeing it)

Comment 192

[David :Bienvenu]

Comment on attachment 372354 [details] [diff] [review]
Review nits, round 1 - checked in

-        errorStr = getStringBundle("chrome://messenger/content/accountCreationUtil.properties")
-                        .GetStringFromName("cannot_contact_server.error");
+        var stringBundle = getStringBundle("chrome://messenger/content/accountCreationUtil.properties")
+        errorStr = stringBundle.GetStringFromName("cannot_contact_server.error");

Is this change for formatting/readability reasons? We don't need the temp var stringBundle...but if it's for readability, maybe use let instead of var?

while you're here, can you add some parens around the == clause just to be clear?
+  if (!oX.@type == "smtp")

Everything else looks fine, thanks!

Comment 193

[Phil Ringnalda (:philor)]

It was for wrapability, and to match the usage a dozen lines up, but I can let them both, sure.

Comment 194

[Ben Bucksch (:BenB)]

-    assert( ! this.result, "Call already returned");
+    assert(!this.result, "Call already returned");

Can you leave this, please? I put spaces around the ! quite intentionally, because it's very eas to overlook otherwise.
(same thing dozens of times)

+  if (!oX.@type == "smtp")

Good catch, bienvenu. That looks like a bug ("!" takes precedence and the expression then makes no sense and is never true), and it should of course just be != . Looks like bad refactoring in a hurry.

Comment 195

[Phil Ringnalda (:philor)]

Yeah, I realized that you put the spaces there intentionally, but as you know you don't entirely get to pick your coding style while writing Mozilla code. accountcreation has 13 |( ! [a-zA-Z]|, the whole tree has 28 in \.js$, the rest in js engine tests, accountcreation has one |(! [a-zA-Z]| and the whole tree has 53, accountcreation has 37 |(![a-zA-Z]| and the whole tree has mxr's bignumber, "over 1000". If we decide tree-wide to change, or even to change only for new code, fine, but we're not going to make that another "first author of a file gets to choose" thing, much less an "author of a particular line of code gets to choose" thing.

Comment 196

[David :Bienvenu]

moving to m5 - Phil is making good progress on the reviews. Bryan has a few things he needs to cleanup.

Re the spaces, I agree with Phil - reviews and sr's always say for those to be removed.

Comment 197

[Phil Ringnalda (:philor)]

Comment on attachment 372355 [details] [diff] [review]
Make email address and hostnames ltr in rtl locales - checked in

http://hg.mozilla.org/comm-central/rev/f525b7b9b3f5

Comment 198

[Phil Ringnalda (:philor)]

Comment on attachment 372354 [details] [diff] [review]
Review nits, round 1 - checked in

http://hg.mozilla.org/comm-central/rev/3aa1bf107060

Comment 199

[Phil Ringnalda (:philor)]

Oh, no wonder it looks like we ignore your changes of port and protocol: onGo calls _startProbingIncoming, which still thinks it's responding to a blur on hostname and merrily sets protocol/port/socketType to undefined.

ui-wanted: if I change the hostname, I might have also changed one of the other bits, or I might not have because I didn't know anything other than the right hostname, or I might not have changed them because they happened to be perfect at the time I stopped probing.

I can see how to make it work with buttons that say essentially [Start probing from scratch] and [Try this and nothing else], or with the horrid UI (but perfect behavior) of blanking the port and selecting a blank item at the top of the protocol and socketType menus while entering edit mode, but I don't see any way of divining whether something unchanged was unknown, or already right. Maybe in this case the horrid way is the right way, and "left blank means you figure it out" will make sense to people?

Comment 200

[David :Bienvenu]

If the user explicitly changes the port and/or protocol, can't we lock those down and make _startProbingIncoming not set them to undefined? Then the issue becomes, what if the user got either of them wrong? I suppose if we ultimately fail, we could clear the fact that the user set the port and/or protocol and start guessing again, if something else (e.g., username/hostname) changed.

Comment 201

[Bryan Clark (DevTools PM) [:clarkbw]]

I had started on changes that I thought could fix this, but it took longer than I expected to put them together.  I was making a menu list widget that contained all the valid probed information, with an additional list item for "Custom...".  This menu list sat above the existing form entry which was just visibility:hidden so it would take up the right amount of space.

[ IMAP mail.example.com - 143 TLS  | v ]
| POP  mail.example.com - 67  TLS  |
| POP  mail.example.com - 43  None |
| Custom...                        |
'----------------------------------'

When you selected the "Custom..." it showed our current entry form with our "best guess" (top value) pre-filled in.  From there everything was manual entry without trying to stick certain values and keep others.

With the richlist box I styled the different pieces so I don't think it looked too bad. (|| == graytext)

*PROTOCOL*  HOST - |PORT| |SECURITY|

But I don't think my dev-foo is quite up to making this happen, or at least I failed at getting it to work correctly.

Comment 202

[Phil Ringnalda (:philor)]

Onchange may be the best we can do, despite not working for two of my use cases:

Smartypants doesn't like us poking around in the wrong places, can't believe we got rid of the wizard, stops probing as quickly as possible, but happens to stop us with his chosen set being probed. He has to either change everything, and then change it back, or know that "Go to advanced settings" means "Create it now."

Mr. Patient waits through the three or four minutes of probing, realizes we didn't try the right hostname for his POP/110/None account, changes the hostname, and sits through three or four minutes of probing to get back to there with the right hostname.

Sadly, my

[      ] v
| POP  |
| IMAP |
'------'

idea, with leaving it on the blank choice to mean "restart probing this" didn't survive getting home and actually looking at an autoconfig build, since I'd forgotten that there aren't any labels for those menus, so unless an a11y review winds up requiring a label, that's out - I'd do that for "Protocol: [     ] v" but not for "[     ]v".

Comment 203

[Ben Bucksch (:BenB)]

I was also very confused about the mixing of probing and manual editing (besides the fact that it doesn't work right atm). When I edit manually, I don't want any probing. I can see that a user may know the hostname, but not the port and protocol, and/or not SSL and auth capability. So, I think we should have a "manual edit" mode/page, where the hostname is empty by default, and the other values are defaulting to "auto". If the user enters a setting, we use that (and if it fails, we go back to that screen - I do *not* want other values to be used in that case). If the user leaves the default of "auto", we probe that particular setting.

Comment 204

[David :Bienvenu]

(In reply to comment #202)
>  or know that "Go to advanced settings" means "Create
> it now."
> 

We need to fix that string no matter what - in my mind, that link is how we can ship this feature w/o handling 100% of the use cases. Bryan, any thoughts on what a better label would be? Create & Edit Settings?

> Mr. Patient waits through the three or four minutes of probing, realizes we
> didn't try the right hostname for his POP/110/None account, changes the
> hostname, and sits through three or four minutes of probing to get back to
> there with the right hostname.

Not sure I follow - there's no "there" to get back to if we never tried the right hostname. Or is it that the very last settings we try happen to be the right ones for the new hostname? I think we just have to live with that - we try those settings last because we've decided they're the least desirable. We could sugar coat it by not showing those settings when we've failed, I guess. I'd be more worried about the case where the user knew those were the right settings and picked them.

Comment 205

[Bryan Clark (DevTools PM) [:clarkbw]]

sorry for the sidetracking.

(In reply to comment #204)
> (In reply to comment #202)
> >  or know that "Go to advanced settings" means "Create
> > it now."
> > 
> 
> We need to fix that string no matter what - in my mind, that link is how we can
> ship this feature w/o handling 100% of the use cases. Bryan, any thoughts on
> what a better label would be? Create & Edit Settings?

I feel like we want to get across the idea that you are actually going to create the account but we're dropping you into the advanced settings dialog to stop any auto settings we might have tried.  Something like "Create in Advanced Editor" or "Continue with Advanced Settings"?  I guess I'm not too concerned about the creation of the account as it's easily removed, but I think it makes sense to get that idea across.  I'd like to have an idea of "advanced" in there as we already allowed some reasonable settings to be editing and we're about to drop you into radio and checkbox heaven.

> > Mr. Patient waits through the three or four minutes of probing, realizes we
> > didn't try the right hostname for his POP/110/None account, changes the
> > hostname, and sits through three or four minutes of probing to get back to
> > there with the right hostname.
> 
> Not sure I follow - there's no "there" to get back to if we never tried the
> right hostname. Or is it that the very last settings we try happen to be the
> right ones for the new hostname? I think we just have to live with that - we
> try those settings last because we've decided they're the least desirable. We
> could sugar coat it by not showing those settings when we've failed, I guess.
> I'd be more worried about the case where the user knew those were the right
> settings and picked them.

Agreed.  I should have done more from the beginning to get our list of use cases that we were going to support written out.  I think we're handling a decent set right now but I keep changing between the cases we are not going to be good at and what we are going to be good at.

Comment 206

[David :Bienvenu]

I went back and looked at mail.app to see how they deal with some of those issues, and I realized that their config stuff is a lot less ambitious than ours. If we can make it a lot easier than it was for 95+% of users, and not make it harder than it was for the remaining 5%, it'll be a big win.

Comment 207

[David :Bienvenu]

Attachment: fix enabling of next and advanced settings - checked in.

This patch makes it so "Next" is only enabled with a remotely valid e-mail address (e.g., a@b), and only shows the "advanced settings" link when we have enough information to actually to go to account settings.

Still to do - make "next" and "back" localizable(!)

We also need to make the "advanced settings" link report errors creating the account (most likely, the account already exists). And of course, decide on a better label for the link.

Comment 208

[Phil Ringnalda (:philor)]

Hmm, are we unable to remember a password of ""? The comment for the soon to be rewritten validatePassword seems to think such a situation might arise, but if we refuse to remember that your password is "" (in general, not just in this patch), I sort of doubt anyone would actually use us with a blank-password account.

Comment 209

[Phil Ringnalda (:philor)]

Ah, perhaps I could just read nsLoginManager, where it throws on a .length == 0 password, couldn't I?

Comment 210

[David :Bienvenu]

(In reply to comment #209)
> Ah, perhaps I could just read nsLoginManager, where it throws on a .length == 0
> password, couldn't I?

Or perhaps I could have explained why I added that code :-)

Comment 211

[Phil Ringnalda (:philor)]

Comment on attachment 373409 [details] [diff] [review]
fix enabling of next and advanced settings - checked in.

Seems like a step in the right direction (except for the trailing whitespace on the first and last lines of the onEmailInput hunk, that's not so right direction).

Do we actually need a non-blank realname? The old wizard demands one, even though you can just go back and remove it later in the account manager. Here, we mutter softly about it only if you first focus then blur the realname field, but then blow up in varying but equally ugly ways if you try to either Create account or Go to advanced settings.

Comment 212

[Phil Ringnalda (:philor)]

Attachment: Don't set isSecureServer

Warning: inServer.isSecureServer is read-only
Source File: chrome://messenger/content/accountcreation/createInBackend.js
Line: 66

Comment 213

[Phil Ringnalda (:philor)]

Attachment: Don't beg for bitrot - checked in

Now that you mention it, yes, a sane person *would* patch on top of the one he just reviewed.

Comment 214

[Phil Ringnalda (:philor)]

Who is supposed to be checking whether an account already exists? Whoever it is, they're not terribly good at it. At one point, I did manage to get a dialog complaining that my outgoing server already existed, but most ways I've tried either just go ahead and create a duplicate account or fail by looking hung when nsIMsgAccountManager.createIncomingServer returns an uncaught NS_ERROR_FAILURE.

Comment 215

[David :Bienvenu]

Comment on attachment 373542 [details] [diff] [review]
Don't beg for bitrot - checked in

thx, I keep forgetting to remove that.

Comment 216

[David :Bienvenu]

(In reply to comment #211)

> Do we actually need a non-blank realname? The old wizard demands one, even
> though you can just go back and remove it later in the account manager. Here,
> we mutter softly about it only if you first focus then blur the realname field,
> but then blow up in varying but equally ugly ways if you try to either Create
> account or Go to advanced settings.

I don't think we really need one internally but I don't mind strongly encouraging that there be one. Perhaps I'll look into seeing why we blowup w/o one, and fix that, and we can decide about enforcing one separately.

Comment 217

[Phil Ringnalda (:philor)]

Comment on attachment 373542 [details] [diff] [review]
Don't beg for bitrot - checked in

http://hg.mozilla.org/comm-central/rev/68243590d40c

Comment 218

[David :Bienvenu]

clarkbw, do you think you're going to have a chance to fix the error message box problem that philor pointed out? And make the Next/Back button text localizable? Those are two blocking issues, I think.

Comment 219

[David :Bienvenu]

Philor, what do you think are the top things preventing this from landing so it can get more testing? I'd like to get it in and get people working on a small set of targeted config files for various isp's (the baby bells, for a start) and e-mail providers (hotmail, fastmail, etc).

Comment 220

[hansen]

Eventhough Renato already wrote this in comment #116 I just wanted to point out, that most of the big servers are listed in a spreadsheet created by Gerv.
https://wiki.mozilla.org/MailServerList - to avoid double work.

Comment 221

[David :Bienvenu]

Attachment: move next & back to dtd file - checked in.

Comment 222

[Vlado Valastiak [:wladow]]

Comment on attachment 373732 [details] [diff] [review]
move next & back to dtd file - checked in. 

>+<!ENTITY back.label                      "Back &#171;">
Are you sure? ;)

Comment 223

[David :Bienvenu]

sure about what? Is there some confusion because this dtd file isn't where localizers can see it? If so, I really can't win for losing :-)

Comment 224

[Phil Ringnalda (:philor)]

You can't, and neither can I, having thought "I'll just review this without applying it or really looking at it, what can go wrong? Other than TEARS AND DEATH that is."

The fake arrow is supposed to be before the word in the Back button.

Comment 225

[David :Bienvenu]

duh, ok, I'll fix that.

Comment 226

[Phil Ringnalda (:philor)]

Attachment: Fix password emptytext as bullets - checked in

The error we were displaying onblur with an empty password was a lie ("login failed"), and we don't need to complain about it until we actually try to use it, plus by setting the type to "password" and leaving it we were displaying eight bullets as emptytext if you tab through without entering your password or delete what you entered and then change focus.

Comment 227

[David :Bienvenu]

Attachment: require a real name before showing next

this makes us require a real name before allowing next. I also made us hide "next" if the requirements go from met to not met, and I added a little check to the e-mail validity check, for a '.' after the '@'.

Comment 228

[David :Bienvenu]

Attachment: philor points out we don't want to require '.'

Comment 229

[Ben Bucksch (:BenB)]

I don't get that "emailAddrValidish" (or better "validateEmailSemiValid") - why would you do a half-check and not the full check before allowing to continue to configuration fetching? Either we allow email addresses without fully qualified domain name or we don't.

Comment 230

[Phil Ringnalda (:philor)]

Comment on attachment 373744 [details] [diff] [review]
Fix password emptytext as bullets - checked in

http://hg.mozilla.org/comm-central/rev/754af56fdd77 once I got rid of the call in validateAndFinish, since failure to log in does just fine at blowing up if it fails to log in because of a blank password.

Comment 231

[David :Bienvenu]

(In reply to comment #229)
> I don't get that "emailAddrValidish" (or better "validateEmailSemiValid") - why
> would you do a half-check and not the full check before allowing to continue to
> configuration fetching? Either we allow email addresses without fully qualified
> domain name or we don't.

The penalty for getting it wrong at this step is the user can never create the account. The penalty for getting it wrong in the full check is that we give an invalid warning but the user can still create the account.

Comment 232

[Phil Ringnalda (:philor)]

Comment on attachment 373770 [details] [diff] [review]
philor points out we don't want to require '.'

r=me, and pushed with light denitting in http://hg.mozilla.org/comm-central/rev/1337b0933fe8

Comment 233

[Phil Ringnalda (:philor)]

Attachment: missing string - checked in

Dunno how I missed seeing the title "undefined" when we can't find either host for so long, since now that I've noticed it, I can't *not* see it. I suspect this wasn't exactly the string you had in mind, but you can always tell me what you really wanted as a review nit ;)

(Localizing the non-localized Account Information is just a ridealong, because I keep seeing it and forgetting to fix it.)

Comment 234

[[e-mail]]

Comment on attachment 373768 [details] [diff] [review]
require a real name before showing next

>+      _show("next_button");
>+    else
>+      _hide("next_button");
On a real wizard I would have expected the button to be disabled...

>                   <button id="next_button"
>                           hidden="true"
... but then maybe you're not using a real wizard...

Comment 235

[Ben Bucksch (:BenB)]

Neil, you're of course right. It should be disabled, not hidden. If disabled state had any purpose, then here.

Comment 236

[Phil Ringnalda (:philor)]

Um, why? Wizards have disabled Next buttons because that's how you know that you are on pane n of m but haven't yet leveled up enough to go to n+1, that you aren't at the end yet. Here, you are in a single window, with three inputs and a Next button once you fill in two of them. What does a disabled Next button tell you that no Next button does not?

I wouldn't *object* to it being disabled, though, since it would force addressing a review comment about how back is styled disabled when it isn't, up there a hundred comments or so.

Comment 237

[David :Bienvenu]

Attachment: don't check if existing server exists.

if the user has picked an smtp server, don't error out if it actually exists.

Comment 238

[David :Bienvenu]

The issue with duplicate incoming server detection is that it happens before user name probing happens - if the user name happens to be the kind w/o the @host, then duplicate detection works; otherwise it doesn't. My thinking is that we should just try both flavors of the user name for duplicate detection. That would cause an issue if you had two accounts on a server, one with username "fred" and one with "[e-mail]". I don't think that's particularly likely.

Comment 239

[Phil Ringnalda (:philor)]

My list of blockers for shipping in nightlies with en-US strings from content/:

* Straighten out the buttons when we give up on probing: right now, we replace the Stop button with an Edit button even though we've already enabled editing, so you edit, click Edit, it changes to Go, you click Go.

* Don't expose UI to edit things we'll ignore. Ideally, that would be "expose it all, don't ignore any of it" but at the moment we ignore everything except username and hostname, and we could probably survive if we only paid attention to username, hostname and port. That would require reworking the UI so there's a separate UI for editing to probe again and for showing the results of probing and either creating or manually editing everything and manually creating, but it still might be easier than figuring out how we'll tell what people meant to specify from what we just prefilled for them.

* Make it possible to use an existing SMTP server.

* Straighten out the SMTP server editing UI. Right now, we have UI to change the port and connection security (that I think we ignore like we do for incoming), but then if you click in the hostname menu, it disappears, which is both confusing, and making it impossible to give up on probing and just fill everything in.

* Make "Go to advanced settings" use the current values in the editing form (whether that's the existing one or a new complete editing form) - right now, when you finally get sick of trying to persuade us that smtp.live.com needs starttls, change the form one more time, and Go to advanced settings, we still create it with the result of our last probe, without starttls. Or for even more fun, if after we probe imap/pop3/mail//.hotmail.com and don't find a single incoming server you change to pop3.live.com and Go to advanced settings without another probe, we create an incoming server with the hostname "-1".

* Remove the fetchConfigFromISP step, since nobody has done anything about a security review for it.

Comment 240

[David :Bienvenu]

Attachment: check both forms of the username when looking for incoming dup detection - checked in.

this also contains the previous patch for outgoing server detection.

Basically, if the username doesn't have a '@', we also check the e-mail address for duplicate accounts. This may not be the right thing to do 100% of the time, but not doing it is wrong.

Comment 241

[Ben Bucksch (:BenB)]

> a separate UI for editing to probe again and for showing the results
> of probing and either creating or manually editing everything
> and manually creating

Yes, agreed. That's exactly what Bryan and I discussed and agreed on in
Barcelona.
I want to still implement that, if nobody else has done so.

> Remove the fetchConfigFromISP step

I did post a formal security review request, as you requested.
http://groups.google.com/group/mozilla.dev.apps.thunderbird/browse_thread/thread/e85fd8d5db0a4a6d/2b36ce3fbb7c2142?q=#2b36ce3fbb7c2142

Nobody had any problems with it.

We show the configuration to the user before we create the account.
Please also see
http://groups.google.com/group/mozilla.dev.apps.thunderbird/msg/2f1d68e425b5a14e

Comment 242

[David :Bienvenu]

(In reply to comment #239)
> My list of blockers for shipping in nightlies with en-US strings from content/:

thx for doing this list, Phil.
> 
> * Don't expose UI to edit things we'll ignore. 

There are two flavors of ignore, right? One when you say Create Account, and one when you do "Go". It's the latter that could require UI changes. The former is really the same as go to account settings. I would not want to lose the ability for the user to override things and go to create account because probing breaks them.

> * Make it possible to use an existing SMTP server.

I think my patch should fix this.

> 
> * Straighten out the SMTP server editing UI. Right now, we have UI to change
> the port and connection security (that I think we ignore like we do for
> incoming), but then if you click in the hostname menu, it disappears, which is
> both confusing, and making it impossible to give up on probing and just fill
> everything in.

I can look into this.

> 
> * Make "Go to advanced settings" use the current values in the editing form
> (whether that's the existing one or a new complete editing form) - right now,
> when you finally get sick of trying to persuade us that smtp.live.com needs
> starttls, change the form one more time, and Go to advanced settings, we still
> create it with the result of our last probe, without starttls. Or for even more
> fun, if after we probe imap/pop3/mail//.hotmail.com and don't find a single
> incoming server you change to pop3.live.com and Go to advanced settings without
> another probe, we create an incoming server with the hostname "-1".

I'll fix this.

Comment 243

[Magnus Melin [:mkmelin]]

> <!ENTITY ssltls.label                    "SSL / TLS">

This is without middle spaces elsewhere.

Comment 244

[Phil Ringnalda (:philor)]

Arrgh, "Well, that didn't go terribly well" wasn't terrible enough to force picking something reasonable? Next time I'm using lorem ipsum :)

(In reply to comment #241)
> I did post a formal security review request, as you requested.

Since I think he's done what I actually mean, dmose should be able to explain the difference, and how to go about it.

Comment 245

[Phil Ringnalda (:philor)]

Comment on attachment 373786 [details] [diff] [review]
missing string - checked in

http://hg.mozilla.org/comm-central/rev/55aeae6362c1

Comment 246

[Phil Ringnalda (:philor)]

Comment on attachment 373865 [details] [diff] [review]
check both forms of the username when looking for incoming dup detection - checked in.

Nice! One of these days, we ought to make the caller of checkOutgoingAccountIsNew a little smarter, since finding it exists really calls for using the existing one, not alert()ing, but that's not nearly as much a blocker.

>+    if (isNew && incoming.username.indexOf('@') < 0) {

Micro nit: double quotes unless you actually *have* to use single.

Comment 247

[Phil Ringnalda (:philor)]

Comment on attachment 371248 [details] [diff] [review]
patch against trunk with philor's recent changes

No real point in a review on this anymore, I've done it piecemeal and the real r? will be the "enable" patch.

Comment 248

[Phil Ringnalda (:philor)]

Attachment: Remove bogus more-info link - checked in

Over the three months plus four days since I said this URL needed to be removed from the dtd, nobody's removed it, nobody's added the pref it should be, and nobody's written the content, which rather makes me think it's not going to happen. If someone decides to make it happen, they'll have an easy enough time adding it back in (the right way).

Comment 249

[David :Bienvenu]

I have a fix for clicking on the outgoing server hostname widget hiding the port and connection type.

The reason picking the connection type doesn't work is that setSecurity is not defined:

gEmailConfigWizard.setSecurity

which I should be able to fix this morning.

Comment 250

[Phil Ringnalda (:philor)]

Comment on attachment 374023 [details] [diff] [review]
Remove bogus more-info link - checked in

http://hg.mozilla.org/comm-central/rev/af0e6d43c7f1

Comment 251

[David :Bienvenu]

Attachment: make probing respect user choices

this patch makes probing respect various user settings, and should make "advanced settings" respect the user choices as well.

It doesn't quite work w.r.t the socketType, though it works quite a bit better than without this patch, at least for a live.com account.  live.com will never quite work with my ISP since it does smtp on port 25, but I was able to enter a live.com e-mail address, do next, press stop once guessing had started, and fill in all the correct information, and press go, and wait for smtp to fail, pick an existing smtp server, and then press finish.

I'm thinking what I should do in guessConfig.js is start off with an array of all the possibilities, and remove the ones that don't match the user-specified protocol and/or port and/or socketType.

philor, if you get a chance, let me know if this patch works better for you.

Bryan/Phil, I'm thinking the Stop/Go buttons should be bigger - they're kind of subtle now - maybe it's because I'm testing the cases where Stop/Go are important, but I'm not sure users will see them. Also, <ESC> cancels out of the whole dialog, instead of doing a STOP.

Comment 252

[Phil Ringnalda (:philor)]

Doesn't your ISP foolishly fail to block 587? Don't remember whether I've tried it, but the announcement says smtp.live.com:587 should work (hmm, and we should probably be probing 587 before we try 25, for both starttls and none, since even people who aren't currently on a 25-blocked ISP might wind up on one later).

Comment 253

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #252)
> Doesn't your ISP foolishly fail to block 587? Don't remember whether I've tried
> it, but the announcement says smtp.live.com:587 should work (hmm, and we should
> probably be probing 587 before we try 25, for both starttls and none, since
> even people who aren't currently on a 25-blocked ISP might wind up on one
> later).

I object. This would make port 587 much more prominent since end users using Thunderbird will use this port by default. This will raise visiblity among ISPs and they will obviously at some point also block port 587.

Comment 254

[David :Bienvenu]

(In reply to comment #252)
> Doesn't your ISP foolishly fail to block 587? Don't remember whether I've tried
> it, but the announcement says smtp.live.com:587 should work (hmm, and we should
> probably be probing 587 before we try 25, for both starttls and none, since
> even people who aren't currently on a 25-blocked ISP might wind up on one
> later).

I was imitating a naive and/or impatient user by"stopping" and following the instructions on MS's site. We don't try 587 for smtp at all - we try 573 and 465. Should we also be trying 587, or 587 instead of 573? One piece of doc I found says 25 and 587 should be used for insecure or STARTTLS, and 465 for SSL. And 587 does work for live.com, over an insecure connection.

I don't think what Thunderbird does is really going to cause ISP's to block port 587.

Comment 255

[David :Bienvenu]

I see Ben added port 573 for SMTP - Ben, can you say why? Is it common in Europe, or is it a case of a single ISP that would probably be better handled with a config file?

Comment 256

[rsx11m]

Where is 573 coming from? /etc/services says it's registered for banyan-vip.
The registered "submission" port is 587 (e.g., Gmail uses it for SMTP).

Comment 257

[Phil Ringnalda (:philor)]

Despite the way I always say "foolishly don't block," really blocking 25 but not 587 is absolutely correct ISP behavior: ISPs block 25 because their zombie customers are contacting example.com:25 to deliver spam for @example.com users, acting as an MTA, not because they are contacting example.com:25 as an MUA, to submit spam for delivery to users at other hosts. There's no need for an ISP to block 587, and it was created explicitly to be the port that ISPs don't need to block, because the server gets to refuse any connection it doesn't like, unlike 25. (And I'm guessing that 573 is a typo for 587, since as rsx11m says that's just not any sort of an SMTP port.)

Comment 258

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #251)
> Bryan/Phil, I'm thinking the Stop/Go buttons should be bigger - they're kind of
> subtle now - maybe it's because I'm testing the cases where Stop/Go are
> important, but I'm not sure users will see them. Also, <ESC> cancels out of the
> whole dialog, instead of doing a STOP.

That's a good point we could increase their size or change position

Comment 259

[Ben Bucksch (:BenB)]

If I added port 573 and SMTP is 587, then that's surely a typo.

Note that ISPs are blocking outgoing port 25 to counter spam originating from botnet zombie PCs on their network, not to force the use of their own mail servers. (At least I'd hope so.) That's also exactly the reason why 587 was introduced, IIRC: to differentiate between server-server SMTP (25) and client-server SMTP (25 originally, now 587).

Comment 260

[rsx11m]

See comment #35 on 587, also citing that it /should/ be used if available.

Comment 261

[David :Bienvenu]

Attachment: a bunch of fixes for stopping and restarting

This patch does a few things:

replaces smtp 573 with 587
remember if the user picks any of the settings, and make probing respect those choices
don't reprobe incoming or outgoing on "Go" if we've successfully probed

The elimination of things to probe is a bit ad hoc, and I really should do the table subtraction approach I mentioned earlier, but this works for the case where you stop probing and fill in the values yourself, which I think will be relatively common for folks unlucky enough not to get a pre-canned config. Philor, I'm wondering if you can try this out and let me know what you think.

Comment 262

[Phil Ringnalda (:philor)]

Two bugs, one that's probably this, maybe in that tangle about protocol and type, one that's probably something else:

1. autoconfig your dreamhost account, either stop us while we're probing IMAP or wait until we discover the IMAP server and then edit
2. change to POP, click Go
3. curse the rediscovery of the IMAP server

and

1. autoconfig a hotmail account, stop probing and change the username to [e-mail], the incoming hostname to pop3.live.com and the outgoing hostname to smtp.live.com
2. probing should find a working incoming server, and smtp.live.com:587+STARTTLS
3. create the account and try to send mail from it
4. when you get tired of waiting, go look at the smtp server details, note that it's not using username+password, which hotmail requires, curse.

Comment 263

[Phil Ringnalda (:philor)]

Attachment: Patch to enable with just one autoconfig window at a time

I finally got tired of having the menuitem clobber an existing autoconfig window, but not focus it, when you forgot that you had one open already, so I answered my comment 161 question of which way we want to go with "find and focus an existing window."

Comment 264

[David :Bienvenu]

(In reply to comment #262)
> 1. autoconfig your dreamhost account, either stop us while we're probing IMAP
> or wait until we discover the IMAP server and then edit
> 2. change to POP, click Go
> 3. curse the rediscovery of the IMAP server

Thx, Phil. I think this one is because I don't restart probing on discovered accounts - I need to clear that state if the user changes any of the settings on a discovered account, so that we'll reprobe.
> 
> 1. autoconfig a hotmail account, stop probing and change the username to
> [e-mail], the incoming hostname to pop3.live.com and the outgoing
> hostname to smtp.live.com
> 2. probing should find a working incoming server, and
> smtp.live.com:587+STARTTLS
> 3. create the account and try to send mail from it
> 4. when you get tired of waiting, go look at the smtp server details, note that
> it's not using username+password, which hotmail requires, curse.

Hmm, I thought we would have erred in the other direction...I'll look into it.

Comment 265

[David :Bienvenu]

Attachment: fix philor's latest issues - checked in.

this fixes our detection of live.com's STARTTLS support (POP3/IMAP/SMTP commands are supposed to end with CRLF, not simply LF)

It also fixes our smtp auth handling - we now assume that a user name and password are required, which was the opposite of before, but this is a much more reasonable default.

It also fixes the incoming protocol/type issue, and reprobing after you changed something in a successful probe.

Comment 266

[Phil Ringnalda (:philor)]

Sometime, I think pretty recently, remembering passwords for POP account broke: remember checked does remember for IMAP, but not for either POP account I tried creating.

Comment 267

[Phil Ringnalda (:philor)]

Oh, because pop3 passwords are apparently supposed to be saved for "mailbox://foo.example.com" rather than "pop3://foo.example.com" so maybe it didn't ever work.

Comment 268

[Phil Ringnalda (:philor)]

Comment on attachment 374339 [details] [diff] [review]
fix philor's latest issues - checked in.

Mmm, it's getting pretty nicely behaved!

>   clickOnOutgoingServer: function()
>   {
>-    _hide('outgoing_protocol');
>-    _hide('outgoing_port');
>-    _hide('outgoing_security');
>   },

Got a future plan in mind for that shell of a function?

>+    dump ("setting outgoing auth to 1\n");

Probably don't need to dump that, but if you do a ddump we can shut off would probably be better.

Comment 269

[David :Bienvenu]

Comment on attachment 374339 [details] [diff] [review]
fix philor's latest issues - checked in.

checked in with comments addressed.

Comment 270

[David :Bienvenu]

I think I've addressed all of Philor's blockers in #c239 except the first and last (the buttons when we give up probing, and disabling the fetchConfigFromISP pending a review). Once those and any new blocking issues are addressed, we can turn on this feature with strings still in content. I'll try to fix those today. Bryan, given that the string freeze is this coming Tuesday, do you think you'll have a chance to do a review of the strings before then? I assume we need that review before we can turn this feature on with strings from locales. We really should fix the issue with the error messages messing up the column width as well. Were there any other issues that would prevent turning this on with strings in locales, so the translators can get going, Philor?

Comment 271

[David :Bienvenu]

Attachment: Philor's fix for stop | edit | go

I've been trying this out - it works much better than what we had before. I think the one place it falls down is when the user does a "back" and a "next". I'll look into that.

Comment 272

[David :Bienvenu]

Attachment: philor's patch, plus fix for handling back+next - checked in.

I'll land this...

Comment 273

[David :Bienvenu]

Attachment: disable fetch from isp pending security review

this removes the call to the code that tries to fetch the config from the isp, but leaves the fetching code in - dmose, do you think you could help Ben with what we mean in terms of a security review so that we can get on track to put this code back?

Comment 274

[Ben Bucksch (:BenB)]

I object to this being disabled. I initiated the review as requested by Phil, publicly on the cited newsgroup and on the sec-group list. Nobody came up with significant issues. In the same thread, I also gave several reasons why this is not pose a considerable problem. We had specifically designed this to be secure, and it's much more secure than status quo.

Comment 275

[Ben Bucksch (:BenB)]

Another point: If the "fetch config from isp" were a security problem, then the probing is as well, for the same reasons.

Comment 276

[David :Bienvenu]

See https://wiki.mozilla.org/Firefox3.1/Security, which Dan pointed to in that very same thread in the newsgroup.

Comment 277

[Phil Ringnalda (:philor)]

(In reply to comment #275)
> Another point: If the "fetch config from isp" were a security problem, then the
> probing is as well, for the same reasons.

Should I take that to mean that you'll say anything to land fetch from ISP, so I should discount your claims about it, or should I take that to mean that you don't see any difference between "Get access to a well-known subdomain which has long been used for mail servers, and run a mail server, or a script which adequately imitates one, on ports other than 80" and "Get access to an unknown subdomain which means nothing special to any host prior to Tb 3, and serve an XML file over HTTP on port 80," so I should discount your claims about it?

I'm sorry that you didn't understand either what I was asking for, or what dmose meant when he said that your posting was "a fine start" rather than saying it was "all that is required," but that's not a get out of jail free card. I'll never like following the robots.txt anti-pattern, but I can live with it, if and only if we have security-minded people actually commit to having an actual meeting where they will actually look at the code and say one way or the other whether they feel that the risk is adequately minimized.

Comment 278

[Ben Bucksch (:BenB)]

> not a get out of jail free card

That's precisely what I want to say: That I'm put in jail here, for no good reason. Same with the other code from me that was just removed without even communicating with me.

> if and only if we have security-minded people

You know what? I'm one of them.

Comment 279

[Ben Bucksch (:BenB)]

For the record, the problem I have is that I am not seeing this "meeting" happening anytime soon, given all the "Unscheduled" on the page, and I don't want this to block indefinitely. I feel I have done all *I* can do for this, including public discussion, 2-3 times. Anyways, I'll add this to the mentioned page.

Comment 280

[David :Bienvenu]

Attachment: this time w/o syntax errors

Comment 281

[Phil Ringnalda (:philor)]

Comment on attachment 374527 [details] [diff] [review]
this time w/o syntax errors

That one works. I'd say if we start saving POP passwords with mailbox:// instead of pop3://, we'll be ready to go.

Comment 282

[Ben Bucksch (:BenB)]

Posted the security review form.

Comment 283

[David :Bienvenu]

Attachment: fix remembering pop3 server password

this should fix it for pop3.

Comment 284

[Phil Ringnalda (:philor)]

Comment on attachment 374217 [details] [diff] [review]
Patch to enable with just one autoconfig window at a time

Most of this is yours, r=me, but there are a few bits that are mine, r? you.

Comment 285

[David :Bienvenu]

Comment on attachment 374217 [details] [diff] [review]
Patch to enable with just one autoconfig window at a time

you caught me in the middle of a full rebuild (I got tired of js_aborting) so it'll be 30 minutes or so before I can try this patch out...

Comment 286

[David :Bienvenu]

Comment on attachment 374217 [details] [diff] [review]
Patch to enable with just one autoconfig window at a time

looks good, thx!

Comment 287

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/comm-central/rev/c393002d5882 and we're done here.

New bugs for broken and not-so-good bits, please, and don't forget to mark them as blocking this bug, and nominate them for blocking Tb3 if they deserve it.

Comment 288

[David :Bienvenu]

Mini test plan

File | New | Mail Account (Quick Setup)

First, try creating accounts that we have config files on our server for, e.g., gmail. Verify that good username and password work, and bad username and/or password gives an appropriate error.

Then, try creating accounts that we don't have config files for (e.g., your own personal domain, if you have one). Verify that we prefer IMAP over POP3, and TLS, then SSL, then non-encrypted connections.

Then, try invalid domains and or servers that we won't be able to guess from the e-mail address.  Verify that we fail to find the server, and give an appropriate error. Then, try correcting the domain and verifying that discover works.

Verify that stopping a probing session and picking one or more of protocol, port, and connection type works.

Verify that picking an existing smtp server from the list works, and creates the account correctly.

In all cases, you should check that the account created works, so that you can receive incoming mail and send outgoing mail. Verify that remembering/not remembering the password works as expected.

Try connecting to a server with a self-signed cert and verify that we give you the chance to add an exception for the cert.

There are many comments in this bug that contain steps to reproduce problems that we have fixed - repeating those steps and verifying that the problems are fixed would also be very helpful.

Comment 289

[Ben Bucksch (:BenB)]

Testing:
Must fail: The guessing is known to fail for bucksch.org. If it finds an SMTP server, it's probably a false positive (which would be misleading and bad). There's no IMAP or POP server either.

fetch config: You can see the list of supported ISPs for which we know and can fetch the config - no guessing - at https://live.mozillamessaging.com/autoconfig/ . (All files with a space in the name are disabled.)

Comment 290

[Ben Bucksch (:BenB)]

Everybody: If you know the configuration for a big ISP or email provider, and it's not in the list yet, please add it to <https://wiki.mozilla.org/MailServerList>.

Comment 291

[Renato S. Yamane]

@Ben Bucksch: I get nightly build (April/28) and try config a new e-mail account (Mandic Mail), but none config is loaded automatically. Thunberbird request to me all configs, as account type, server address, etc.

Mandic Mail is listed in MailServerList (comment #290) more than 4 months.

Comment 292

[Ben Bucksch (:BenB)]

Yes, we have no process yet to automatically or semi-automatically put the settings at MailServerList live to https://live.mozillamessaging.com/autoconfig/ , so that TB3 will actually use them. I am doing that manually right now for the big ones.
If your ISP is not yet at https://live.mozillamessaging.com/autoconfig/ , you help by putting it in the MailServerList, or/and testing the settings which are there. I (or the other devs) can't test most settings, because we have no accounts, so we have to rely on your testing.

Also, it would be good to check whether there are better settings possible than those listed, specifically whether SSL and secure auth are possible.

For real ISPs (offering Internet connection), sometimes the possible settings differ based on whether you are connected to their network for Internet, or whether you are on other networks ("on the road", in the company). It would be good to test the settings in both situations, and if there are any discrepancies (some settings work only in one of the cases, note them in the "Additional Notes" field.

Comment 295

[Renato S. Yamane]

@Ben,

I send you a xml file such as available in https://live.mozillamessaging.com/autoconfig/

Can you add it on this autoconfig directory to me test it?

And I see some repeatable files, as:
yahoo.co.uk
yahoo.com
yahoo.com.br
yahoo.de
yahoo.fr
yahoo.it

All this domains is showed in <domain> xml file, so I think that is necessary only one (yahoo.com), right?

Comment 296

[Renato S. Yamane]

If, in the future, gmail (e.g.) change all server address, is possible config mail account manually?

Comment 297

[Ludovic Hirlimann [:Usul]]

(In reply to comment #296)
> If, in the future, gmail (e.g.) change all server address, is possible config
> mail account manually?

Yes you still can configure your emails manually.

Comment 298

[Ben Bucksch (:BenB)]

Re ISP fetch "security review", I wrote in comment 279:

> the problem I have is that I am not seeing this "meeting" happening
> anytime soon, given all the "Unscheduled" on the page, and I don't
> want this to block indefinitely.

It is still Unscheduled
https://wiki.mozilla.org/Firefox3.5/Security

Comment 299

[Steffen Wilberg]

Ben, that page says: The "who" is responsible for scheduling the review, and email certain people about the date. That would be you...

Comment 300

[Ben Bucksch (:BenB)]

duh. Didn't see that (probably because it's in bold ;-P ). Thanks, sent the mail to dveditz and co now.

Comment 301

[Nomis101]

I'm not sure if this is the right place for this question, but I couldn't find a better place. Is there any guidline how I will get new ISPs in the Autoconfig list: http://spreadsheets.google.com/pub?key=p49SW32nNYX0otkRc3UZUJA ??
A few month ago I added lycos.de, but now I don't find it anymore (don't know why).  I know a few persons with a lycos address. I also miss alice-dsl.de, firemail.de, freenet.de and versatel.de at the moment. 
I read somewhere you now must open a bug if you want a new ISP in the list. Is this right?

Comment 302

[Renato S. Yamane]

I already input all data about a brazilian e-mail service and send a XML file to Ben Bucksch, but autoconfig don't work yet to it.

Maybe it work only with Google.

Comment 303

[Ludovic Hirlimann [:Usul]]

(In reply to comment #301)

> I read somewhere you now must open a bug if you want a new ISP in the list. Is
> this right?

We changed this and are working on making an web-application available to the public - we gave it the name ispdb - it's being tested at the moment (as can be read on the weekly status meetings of Thunderbird). This will be announced on the status meeting notes, mozilla newgroups and a few weblogs when we are ready to accept user input. For now you can ask david access to have a look at the website and provide feedback.

Comment 307

[John]

Does the local xml file still work as advertised ?

From what I have read the autoconfig process is supposed to start with
1. Config files on hard disk, in <installdir>/isp/*.xml

I have been testing with current release:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.5) Gecko/20091204 Thunderbird/3.0 
And have found that steps 3 and 4 work, but have not been able to get the local xml file step to work.

I thought I must be doing something wrong in the xml, so I downloaded this file (have also tested with many others):
https://live.mozillamessaging.com/autoconfig/live.co.jp
and added a domain to the relevant section, eg;
<domain>mydomain.com</domain>
The wizard goes almost immediately into stage 4 heuristic testing.

I haven't seen many comments on this stage of process, so if anyone has got local isp/whatever.xml file working, please post a link to correct syntax.

Comment 308

[Ben Bucksch (:BenB)]

> 1. Config files on hard disk, in <installdir>/isp/*.xml

Yes, that's implemented.
Simply adding <domain>foo.com</domain> is not enough, the file needs to have the domain name as filename, i.e. .../isp/foo.com.xml .

---

ATTENTION for all further comments:
Please post all further "This doesn't work for me" or "How do I set up my mail server to support this?" on the newsgroup (e.g. mozilla.dev.apps.thunderbird), not here. This bug is already way too long.

Comment 309

[Ben Bucksch (:BenB)]

Specs: https://wiki.mozilla.org/Thunderbird:Autoconfiguration
(see also subpages at top, and Related Information at bottom)

Comment 310

[Ludovic Hirlimann [:Usul]]

(In reply to comment #308)

> ATTENTION for all further comments:
> Please post all further "This doesn't work for me" or "How do I set up my mail
> server to support this?" on the newsgroup (e.g. mozilla.dev.apps.thunderbird),
> not here. This bug is already way too long.

Actually the discussion happens on the ispdb google group.