1594178 - Add username (and host) to password dialog's title

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[ns-bugmozilla-19580]

Attachment: Example.png

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

Start Thunderbird, then be asked for the password.

Actual results:

Password dialog pops up

Expected results:

The password dialog should have a less generic title to help users of password managers to use the AutoType feature.

I wrote a XUL extension that overlays the password dialog and changes the title (https://addons.thunderbird.net/en-US/developers/addon/password-dialog-title), but this is not possible with MailExtensions.
This simple change (simply put more text into the title) would help all users of external password managers.

Comment 1

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Thanks for the report! Magnus, can you someone from the team mentor Jonny on writing a patch for bug? It seems to me this would be a simple addition with a clear use case. It is possible that this should be fixed in Toolkit instead, so we might have to ask there.

Comment 2

[ns-bugmozilla-19580]

Attachment: diff.patch

Here comes the patch. Compiling thunderbird myself was too complicated, though (my disk has not enough space to clone mozilla-central for mach :o)

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9107672 [details] [diff] [review]
diff.patch

Review of attachment 9107672 [details] [diff] [review]:
-----------------------------------------------------------------

Seems to be the risk that the title gets super long (and titles can't wrap). 
The information is already in the body of the dialog. What are such password managers looking for, and where?

From the technical side: we need to bump the localization key, when changing the value. But, seems you already have the suitable value available as a key (like pop3EnterPasswordPrompt)

Comment 4

[Magnus Melin [:mkmelin]]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #1)
Toolkit doesn't use this dialog anywhere anymore, I think.

Comment 5

[ns-bugmozilla-19580]

Seems to be the risk that the title gets super long (and titles can't wrap).

That's true. For me and probably the most users we could also leave out the hostname, as probably everyone is using the email address or something similar unique as username.

The information is already in the body of the dialog. What are such password managers looking for, and where?

The password manager I (and many others) are using is KeePass or KeePassXC. Both only use the dialog's title to match the entry in the database:
https://keepass.info/help/base/autotype.html#autowindows
https://github.com/keepassxreboot/keepassxc/blob/d007ee97e3eb794a384776e75192126c857bf74f/src/autotype/AutoType.cpp#L285

From the technical side: we need to bump the localization key, when changing the value. But, seems you already have the suitable value available as a key (like pop3EnterPasswordPrompt)

Ah, ok. I can create a new patch if you want me to. With or without hostname? :)

---

For Firefox, there are also addons that connect to KeePassXC, e.g.:
https://github.com/keepassxreboot/keepassxc-browser
But as far as I found out (using Google) there is no possibility to do this for Thunderbird with MailExtensions. Is this correct?

Comment 6

[Magnus Melin [:mkmelin]]

I'd go without the hostname

Comment 7

[ns-bugmozilla-19580]

Attachment: diff2.patch

I have created a new patch with two changes:

• The localization strings XXXTitle are replaced by XXXTitleWithUsername
• The localization strings do not contain the hostname anymore

Hopefully "normal" indexing ([0]) works on AutoTArrays :)

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9114991 [details] [diff] [review]
diff2.patch

Review of attachment 9114991 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +360,4 @@
>  ## words %1$S and %2$S. Place the word %1$S where the host name should appear,
>  ## and %2$S where the user name should appear.
>  smtpEnterPasswordPromptWithUsername=Enter your password for %2$S on %1$S:
> +smtpEnterPasswordPromptTitleWithUsername=Outgoing server (SMTP) Password Required for %1$S

Can you please add a localization note explaining what the argument is. (See example a few rows above).
Let's also take the opportunity to slightly reword/improve this. Can we make it 

smtpEnterPasswordPromptTitleWithUsername=Password Required for Outgoing (SMTP) Server %1$S

Comment 9

[ns-bugmozilla-19580]

Attachment: diff3.patch

As requested :)

Comment 10

[Jorg K (CEST = GMT+2)]

Assignee is the guy/girl fixing the bug.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9118714 [details] [diff] [review]
diff3.patch

Review of attachment 9118714 [details] [diff] [review]:
-----------------------------------------------------------------

Please setup your hg username to be the full email (and name if you like), and also remember to run clang-format before submitting,
./mach clang-format -p comm/mailnews

Doesn't compile atm.

mozilla/comm/mailnews/imap/src/nsImapIncomingServer.cpp:1998:51: error: no viable conversion from 'nsTArray_Impl<nsTString<char16_t>, nsTArrayInfallibleAllocator>::elem_type' (aka 'nsTString<char16_t>') to 'const nsTArray<nsString>' (aka 'const nsTArray<nsTString<char16_t> >')
 1:10.12       "imapEnterPasswordPromptTitleWithUsername", formatStrings[0],
 1:10.12                                                   ^~~~~~~~~~~~~~~~

::: mail/locales/en-US/chrome/messenger/imapMsgs.properties
@@ +135,5 @@
>  imapDone=
>  
> +# LOCALIZATION NOTE (imapEnterPasswordPromptTitleWithUsername): Do not translate the
> +# word %1$S. Place the word %1$S where the user name should appear.
> +imapEnterPasswordPromptTitleWithUsername=Mail Server Password Required for %1$S

Maybe we should change this too to

Enter your password for %1$S

Comment 12

[ns-bugmozilla-19580]

Attachment: diff4.patch

Unfortunately I do not have enough disk space to compile mach, not even to check out the complete archive.

So can you run clang-format for me? I will happily apply your patch afterwards to fix further things if necessary.

I fixed the compilation error by creating another array with the username as only element, and change the imap title as you suggested.

How about changing the smtp title again, so that all 3 of them are the same?

Comment 13

[Magnus Melin [:mkmelin]]

The SMTP is different since you can use that for multiple usernames, so I think that should stay.

Comment 14

[Magnus Melin [:mkmelin]]

Attachment: diff4-compiling.patch

Made it compile. Also using CopyUTF8toUTF16 instead.

Comment 15

[Magnus Melin [:mkmelin]]

Attachment: diff4-fixed.patch

Comment 16

[Magnus Melin [:mkmelin]]

Ah, I notice now it's wrong for smtpEnterPasswordPromptTitleWithUsername

Comment 17

[Magnus Melin [:mkmelin]]

Attachment: diff4-fixed2.patch

Fixed that too

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/807fb40cb098
Add username (and host) to password dialog's title. r=mkmelin

Comment 19

[ns-bugmozilla-19580]

Great, thanks!

But smtpEnterPasswordPromptTitleWithUsername would have been better, with your rationale: you can use that for multiple usernames. Each username, even on the same server, should have a different password and thus be distinguishable by the title for the password database.