316245 - Secure news server icon does not display padlock

Status: VERIFIED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Chris Ilias [:cilias]]

Using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051112 Thunderbird/1.5 ID:2005111206

- Create a new profile.
- Create a new newsgroup account for a secure news server. (example: secnews.netscape.com )
- Restart Thunderbird, for SSL settings to take affect.

Result: The icon for that account still does not display the padlock, although the account does seem to access the server with SSL.

Expected result: The padlock should display.

Other info:
- Secure IMAP displays the padlock
- On snews://secnews.netscape.com:563/netscape.mozilla.thunderbird , a Linux user reported this, so I've set the OS to 'All', assuming this is a cross-platform bug. I don't know if it's on Mac.
- the reports on the thunderbird newsgroup are that this appears in Thunderbird 1.5 RC1, so I guess this broke before then. I'll do some back tracking...

Comment 1

[Chris Ilias [:cilias]]

http://archive.mozilla.org/pub/thunderbird/nightly/2005-08-17-09-mozilla1.8/
is the first branch build to contain this bug.

Comment 2

[Chris Ilias [:cilias]]

I should add that I've been using auto-update for a while, and the icon appears for my main profile, which was created prior to using auto-update.

Comment 3

[Magnus Melin [:mkmelin]]

Based on the regression window, looks like fallout from bug 304466, checked in 2005-08-16 10:39.

Comment 4

[Chris Ilias [:cilias]]

(In reply to comment #0)
> - On snews://secnews.netscape.com:563/netscape.mozilla.thunderbird , a Linux
> user reported this, so I've set the OS to 'All', assuming this is a
> cross-platform bug. I don't know if it's on Mac.

snews://secnews.netscape.com:563/dl7d9k$[e-mail]
Melchert Fruitema confirmed this on MacOS X/TB version 1.5 (2005102519).

Comment 5

[CatThief]

I see the missing padlock on secure newsservers as well - Linux and Windows.

Verified on a clean install of 1.5 Beta1 and Beta 2 as well as 1.5 RC1 with a new profile, default theme, no userChrome.css options, completely new mail and news setups. The code inside folderPane.css is correct and the image does exist inside the theme.

Tested on secnews.netscape.com

Comment 6

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Comment 7

[Scott MacGregor]

Comment on attachment 203026 [details] [diff] [review]
proposed fix

This isn't going to work for non news accounts that aren't secure right? For those, we'll end up calling GetIsSecure when we only want to do that for news.  

I'm still confused why existing news servers show the lock icon but creating the news server in a new profile has this problem.

Comment 8

[Chris Ilias [:cilias]]

This could very well be a core bug. Same bug appears in Seamonkey.

Comment 9

[Chris Ilias [:cilias]]

Done some backtracking on SeaMonkey. The first SM build (trunk) to have this bug is also the first one with Gecko 1.9a1. 
That build is:
http://ftp.mozilla.org/pub/mozilla.org/seamonkey/nightly/2005-08-13-06-trunk/

(ie. nightly before it still says 1.8, and the padlock appears in it)

Comment 10

[Scott MacGregor]

(In reply to comment #9)
> Done some backtracking on SeaMonkey. The first SM build (trunk) to have this
> bug is also the first one with Gecko 1.9a1. 
> That build is:
> http://ftp.mozilla.org/pub/mozilla.org/seamonkey/nightly/2005-08-13-06-trunk/
> 
> (ie. nightly before it still says 1.8, and the padlock appears in it)
> 

That looks like the same day I landed Bug 304466. Thanks Chris.

Comment 11

[Scott MacGregor]

Magnus, I think a better fix would be to take the server object and QI it to an nsINntpIncomingServer. If that QI gives back a valid value, then call server->GetIsSecure(&isSecure); 

so

if (nntpIncomingServer)
  server->GetIsSecure(&isSecure);
else
{
  check the socket server type like we currently do...
}

Comment 12

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Something like this?

Comment 13

[Scott MacGregor]

Comment on attachment 203541 [details] [diff] [review]
proposed fix, v2

almost perfect I think.

We just need to ditch the 
if (NS_FAILED(rv)) return rv; 

line.

If GetIsSecure failed to return a value, we should still fall through and set the false property on the data source. I can make that change when I check this in though.

Comment 14

[David :Bienvenu]

Comment on attachment 203541 [details] [diff] [review]
proposed fix, v2

sr=bienvenu, w/ that change

Comment 15

[Scott MacGregor]

Attachment: fix as checked in

for historical purposes in case this ends up on the branch.

Comment 16

[Scott MacGregor]

fixed on the trunk, leaving open just in case we have to respin thunderbird 1.5. If this tests out well on the trunk we might consider it. (Would be great if others could test a trunk build from tomorrow).

Comment 17

[[e-mail]]

I'm not sure that this is the right fix for this (and bug 304466) because setting the socket type isn't getting reflected in the UI. Perhaps setting the socket type also needs to set IsSecure (which would make bug 304466 redundant)?

Comment 18

[Chris Ilias [:cilias]]

Both
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051118 Thunderbird/1.6a1 ID:2005111818
and
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051119 SeaMonkey/1.5a ID:2005111909
work for me.

I set up secure NNTP, IMAP, and POP accounts. All displayed the padlock. All are functioning properly.

Comment 19

[Scott MacGregor]

(In reply to comment #17)
>  because
> setting the socket type isn't getting reflected in the UI. 

Hey Neil, I'm not sure what you mean by this. Are there still more areas where the padlock isn't showing up right?

Comment 20

[[e-mail]]

(In reply to comment #19)
>(In reply to comment #17)
>>because setting the socket type isn't getting reflected in the UI. 
>Hey Neil, I'm not sure what you mean by this. Are there still more areas where
>the padlock isn't showing up right?
No, I was just thinking aloud that an alternative way to address this and bug 304466 is to make POP3/IMAP servers return a correct isSecure attribute. Or you could always switch news servers to the socketType attribute instead, reusing the Never/SSL radiobuttons from the POP3/IMAP server security UI.

Comment 21

[Chris Ilias [:cilias]]

Why is this bug not marked as FIXED?

Comment 22

[Magnus Melin [:mkmelin]]

(In reply to comment #21)
> Why is this bug not marked as FIXED?

Works fine for me to on linux- but it's not fixed on the branch(es) yet. Maybe now would be a good time to get it in there as well? Or maybe it should be fixed another way, like Niel's comment 20. Scott?

Comment 23

[Magnus Melin [:mkmelin]]

Sorry, I see Scott checked it in to the 1_8 and 1_8_0 branches 2005-12-01 14:55 - I see no fixed1.8 keyword though. My linux build of 1.5 says 2005120113, so it missed 1.5 (en-US) by a couple of hours :(. Don't know about windows. Anyway, for current nightly branch builds the patch is working.

Comment 24

[Scott MacGregor]

I left the bug open in case we re-implement the fix based on Neil's suggestions. 

Comment 25

[Magnus Melin [:mkmelin]]

Marking this fixed, will open a new bug to implement comment 20 - make server.isSecure really work for imap/pop. (Have a patch, just need to test some more.)

Comment 26

[Chris Ilias [:cilias]]

So verified. :-)

Comment 27

[Magnus Melin [:mkmelin]]

Created bug 462045.