214402 - Keep thread pane blank until we are logged in

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Scott MacGregor]

4.x had this behavior. I'm sure this is reported in bugzilla already but I could
not find a 4xp (that means 4.x parity) bug, so I'm filing this one. 

We should provide a hidden preference which can be set to do the following:

1) If we don't already know the password and the user has not entered his
password yet, keep the thread pane blank. 

2) If remember password was turned on then by all means skip this.

This feature should be pretty cheap to add.

Comment 1

[David :Bienvenu]

I did this in 4.x - off the top of my head, I'd say the easiest way to implement
this is:

In ChangeFolderByURI, set showMessagesAfterLoading to true if this pref is set.
This works for IMAP, probably, where folder loading and password prompting are
tied together already. The tricky part is that for local/pop3 folders, we need
to specially prompt for the password.

Comment 2

[David :Bienvenu]

taking. The 4.x pref was "mail.password_protect_local_cache". In 4.x, we stored
whether the user had been authenticated for the current session in the msg
master. I think for Mozilla, we'd store this in the account manager. Also, when
prompting for a password for local folders, I think we'd use the password for
the default incoming server.

Comment 3

[David :Bienvenu]

Attachment: fix for imap folder case

this fixes this for online imap and pop3 accounts (but not the local folders
directory, or the imap offline case).

Comment 4

[David :Bienvenu]

Attachment: default pref to false

Comment 5

[(not reading, please use [e-mail] instead)]

Comment on attachment 129386 [details] [diff] [review]
fix for imap folder case

r/sr/a=sspitzer for 1.5 beta

Comment 6

[(not reading, please use [e-mail] instead)]

Comment on attachment 129399 [details] [diff] [review]
default pref to false

r/sr/a=sspitzer for 1.5 beta

Comment 7

[David :Bienvenu]

fix for imap and pop3 accounts checked in.

Comment 8

[Scott MacGregor]

David, thanks for the quick turn around on this! I just checked this in for
thunderbird as well.

Comment 9

[David :Bienvenu]

Attachment: work in progress

This patch fixes the local folder case, and removes some unused methods that
implemented this feature in 4.x. What's left is the offline imap case.
Implementing that may require rearranging some code, but maybe not...

Comment 10

[David :Bienvenu]

Attachment: more work in progress

this patch actually works :-) now, for pop3, we only store password in the
wallet when the logon succeeds. Remaining work : I need to make this true for
IMAP, and move the password prompting code into nsMsgDBFolder.cpp, to avoid the
code duplication.

Comment 11

[David :Bienvenu]

Attachment: fix for review

this patch implements the basic functionality for local,pop3, and imap folders.

Comment 12

[Scott MacGregor]

Comment on attachment 130029 [details] [diff] [review]
fix for review

sr =mscott

I noticed an out of place tab though:

+	  nsCAutoString hostFound;

thanks for doing this david.

Comment 13

[(not reading, please use [e-mail] instead)]

r/a=sspitzer for 1.5 beta

here's some comments and suggestions.

I'm rusty, so my apologies if I'm being overzealous.

1)
 
+  /* for preventing unauthenticated users from seeing header information */
+  attribute boolean userNeedsToAuthenticate;

consider JavaDoc-style comments (for the tools that parse our .idl?)

   /**
    * for preventing unauthenticated users from seeing header information
    */
2)

same here.
 
+  void storePassword(); // logon succeeded - persist password, if user chooses.
+
   /* forget the password in memory which is cached for the session */
   void forgetSessionPassword();

3)

+# LOCALIZATION NOTE (Error 5047): Do not translate the word %S below.
+# Place the word "%S" in your translation where the email address
+# or the username should appear
+passwordPrompt=Enter your password for %s on %s:

the localization note doesn't match the value.

Also, what if a localizer for language foo wanted the reverse?  

To allow for that, shouldn't this be:

+passwordPrompt=Enter your password for %1$s on %2$s:

For comparison, the other password strings:

C:\trees\trunk\mozilla\mailnews\compose\resources\locale\en-US\composeMsgs.properties(156):12543=Enter
your password for %S:
C:\trees\trunk\mozilla\mailnews\imap\resources\locale\en-US\imapMsgs.properties(241):5047=Enter
your password for %S:
C:\trees\trunk\mozilla\mailnews\local\resources\locale\en-US\localMsgs.properties(150):4017=Enter
your password for %1$s@%2$s:
C:\trees\trunk\mozilla\mailnews\mapi\resources\locale\en-US\mapi.properties(7):loginText=Please
enter your password for %S:


4)

+
+## @name ENTER_PASSWORD_PROMPT_TITLE
+## @loc None
+passwordTitle=Mail Server Password Required

Does that title make sense for "Local Folders"?  (this feature works for local
folders, right?)


5)

+NS_IMETHODIMP
+nsMsgAccountManager::GetUserNeedsToAuthenticate(PRBool *aRetval)
+{
+  NS_ENSURE_ARG_POINTER(aRetval);
+  if (!m_userAuthenticated)
+  {
+    PRBool passwordProtectLocalCache;
+
+    m_prefs->GetBoolPref("mail.password_protect_local_cache",
&passwordProtectLocalCache);
+    *aRetval = passwordProtectLocalCache;
+    return NS_OK;
+    // check if we're password protecting local cache.
+  }
+  *aRetval = !m_userAuthenticated;
+  return NS_OK;
+}

I don't think you need passwordProtectLocalCache.

in the if block, can you do:

+  if (!m_userAuthenticated)
+  {
+    // we only need to authenticate if we are password protecting the local cache 
+    return m_prefs->GetBoolPref("mail.password_protect_local_cache", aRetval);
+  }

that should be ok, assuming that pref is defaulted in mailnews.js

6)

+  server->GetRealHostName(getter_Copies(hostName));
+  server->GetRealUsername(getter_Copies(userName));
+  bundle->GetStringFromName(NS_LITERAL_STRING("passwordTitle").get(),
getter_Copies(passwordTitle));
+  bundle->GetStringFromName(NS_LITERAL_STRING("passwordPrompt").get(),
getter_Copies(passwordTemplate));

if we don't care if those fail, should it be preceeded by (void)?

7)

+  if (passwordTemplate)
+    passwordPromptString = nsTextFormatter::smprintf(passwordTemplate, (const
char *) userName, (const char *) hostName);

since you have the bundle, we should use

    NS_ConvertASCIItoUCS2 usernameStr(userName);
    NS_ConvertASCIItoUCS2 hostNameStr(hostName);

    const PRUnichar *stringParams[2] = { usernameStr.get(), hostNameStr() };

    rv = bundle->FormatStringFromName(
        NS_LITERAL_STRING("passwordPrompt").get(), stringParams, 2, 
        getter_Copies(passwordPromptString ));

saves you from calling nsTextFormatter::smprintf_free, too.

8)

+      nsCOMPtr <nsIPasswordManagerInternal> passwordMgrInt =
do_GetService(NS_PASSWORDMANAGER_CONTRACTID, &rv);
+      if(NS_SUCCEEDED(rv) && passwordMgrInt) 
+      {

you could do:

+      nsCOMPtr <nsIPasswordManagerInternal> passwordMgrInt =
do_GetService(NS_PASSWORDMANAGER_CONTRACTID);
+      if(passwordMgrInt) 
+      {

9)

what's the munged uri for, again?   can you add a comment to elaborate?

+        currServerUri.Insert('x', 0);

10)


+    nsCOMPtr<nsIMsgAccountManager> accountManager =
do_GetService(NS_MSGACCOUNTMANAGER_CONTRACTID, &rv);
+    if (NS_SUCCEEDED(rv))
+      accountManager->SetUserNeedsToAuthenticate(PR_FALSE);

or

+    nsCOMPtr<nsIMsgAccountManager> accountManager =
do_GetService(NS_MSGACCOUNTMANAGER_CONTRACTID);
+    if (accountManager)
+      accountManager->SetUserNeedsToAuthenticate(PR_FALSE);

so you don't have to worry about messing with rv.  (this should not fail)

11)

+      rv = m_prefBranch->GetBoolPref( "mail.password_protect_local_cache",
&passwordProtectLocalCache);

since we never check rv, and this pref is defaulted, how about "(void)" instead
of "rv = "

12)
 
+    // if we're password protecting the local cache, we're going to munge the
uri in the password mgr to 
+    // start with 'x', so that we can remember the password in order to
challenge the user, w/o having the
+    // password mgr automatically use the password.
+    if (PasswordProtectLocalCache())
+      serverSpec.Insert('x', 0);

ah, this explains #9.

so this would be ximap:// instead of imap://

13)

+    if (!server) return NS_ERROR_UNEXPECTED;

per our coding style, you want:

+    if (!server) 
+      return NS_ERROR_UNEXPECTED;

Comment 14

[David :Bienvenu]

>Does that title make sense for "Local Folders"?  (this feature works for local
>folders, right?)

yes, because we're asking for your mail server password - if it's a pop3 folder,
it will be the pop3 server password; if it's a local folder, it will be the
password for the default account.

thx for the r/a=, I'll attach a new patch addressing your other comments.

Comment 15

[David :Bienvenu]

Attachment: fix addressing review comments

Comment 16

[David :Bienvenu]

fix checked in, r=mscott,sr/a=sspitzer

Comment 17

[:aceman]

I think I have found bug 222927 in this.