1315662 - deleting of SMTP-Account does not delete corresponding entry in password-manager

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Hubertus vonStreit]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20161028133011

Steps to reproduce:

- Delete mail-account IMAP
- Delete corresponding SMTP-entry


Actual results:

- Entry for deleted SMTP in Password-Manager still listed
- Password data for deleted accounts remain visible!?



Expected results:

- Like after deleting IMAP mail-account, the corresponding entry in Password-Manager should have been deleted.

Comment 1

[Jorg K (CEST = GMT+2)]

This seems to a duplicate, I think I saw something similar the other day but can't find it now.

Comment 2

[:aceman]

We attempt to do this for incoming servers (POP/IMAP) but I do not see code for it for SMTP. I'll look at this.

Comment 3

[Jorg K (CEST = GMT+2)]

Let's confirm it then.

Comment 4

[:aceman]

Attachment: WIP patch

Theoretically, this should do it. However, this only removes the password for the current smtp hostname+username combo. But a SMTP server could store more passwords if its hostname or username has changed in the past. The incoming servers specifically work against this and remove the password if hostname or username changes. So there is always only one password for the incoming server. Can I copy this logic to SMTP too?

Comment 5

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #4)
> hostname or username changes. So there is always only one password for the
> incoming server. Can I copy this logic to SMTP too?

Huh? So what about when you have multiple accounts for one server? That is probably not even uncommon.

Comment 6

[:aceman]

(In reply to Magnus Melin from comment #5)
> Huh? So what about when you have multiple accounts for one server? That is
> probably not even uncommon.

We are talking SMTP server here. Anyway, if you have multiple accounts on the same SMTP server, you have to create multiple SMTP servers in TB (nsSmtpServer). Each server can only hold one username and one password. However, if you change the username, you just get asked for new password and another hostname+username+password entry is stored in password manager. The entry for old username is not removed even though it is useless since now, you will never login with that username into the server (the object does not reference the old username anymore). This is in contrast to the incoming server objects, which just wipe the old username+password entry from PM whenever username (or hostname changes). See bug 302388 comment 11. I ask if I can implement the logic also in the SMTP server object to not leave stray passwords in the PM when no longer needed.

Comment 7

[Magnus Melin [:mkmelin]]

Yes, leaving the old password sounds like a bug to me.

Comment 8

[Wayne Mery (:wsmwk)]

aceman, you got your feedback :)

Comment 9

[:aceman]

Attachment: 1315662.patch

This should do it, removes password also on username or hostname change, similar to the incoming servers.

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9072380 [details] [diff] [review]
1315662.patch

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

Looks good to me, r=mkmelin

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 9072380 [details] [diff] [review]
1315662.patch

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

::: mailnews/compose/src/nsSmtpServer.cpp
@@ +553,5 @@
>        do_GetService(NS_LOGINMANAGER_CONTRACTID, &rv);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    // Get the current server URI without the username
> +  NS_ConvertASCIItoUTF16 serverUri(GetServerURIInternal(false));

The original code had `NS_ConvertUTF8toUTF16 currServer(serverUri);` so why switch this to ASCII now?

@@ -521,5 @@
>        do_GetService(NS_LOGINMANAGER_CONTRACTID, &rv);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    // Get the current server URI without the username
> -  nsAutoCString serverUri(NS_LITERAL_CSTRING("smtp://"));

Interesting clean-up, how did you find this duplicated code?

Comment 12

[:aceman]

(In reply to Jorg K (GMT+2) from comment #11)

// Get the current server URI without the username

• NS_ConvertASCIItoUTF16 serverUri(GetServerURIInternal(false));

The original code had NS_ConvertUTF8toUTF16 currServer(serverUri); so why
switch this to ASCII now?

All occurrences of GetServerURIInternal() do it like this when they need UTF16.
Especially the one at https://searchfox.org/comm-central/rev/9f64b3c10baf73678700dbb7b82020ce0f54ae34/mailnews/compose/src/nsSmtpServer.cpp#391 which is fetching the password too.

@@ -521,5 @@

   do_GetService(NS_LOGINMANAGER_CONTRACTID, &rv);

NS_ENSURE_SUCCESS(rv, rv);

// Get the current server URI without the username

• nsAutoCString serverUri(NS_LITERAL_CSTRING("smtp://"));
  Interesting clean-up, how did you find this duplicated code?

I don't remember, I did this a long time ago but probably forgot to upload the patch.
Maybe it is from https://searchfox.org/comm-central/rev/9f64b3c10baf73678700dbb7b82020ce0f54ae34/mailnews/compose/src/nsSmtpServer.cpp#391 which does what we need here.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/166aaa29faa1
delete SMTP server login credentials when deleting the account or if hostname/username changes. r=mkmelin

Comment 14

[Jorg K (CEST = GMT+2)]

TB 68 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/ddad01cf38846c38d01e20316f82a3eb6e22937d

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0e2572d4a88f
Follow-up: reformat. rs=reformat DONTBUILD

Comment 16

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-beta/rev/a5df96cca9633419c979a93289e4ba22c910c3d0 (reformat)