Enable SecureBrowserUI for embedding apps

VERIFIED FIXED

Status

()

VERIFIED FIXED
18 years ago
16 years ago

People

(Reporter: ccarlen, Assigned: chak)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

18 years ago
Currently, an embedding app has to do some work in order to create an instance
of nsISecureBrowserUI for each top level window. Without doing so, they will not
receive any security notifications through nsIWebProgressListener. This is, to
say the least, not obvious. I suggest we create the nsISecureBrowserUI instance
in the webBrowser lib in order to hide step this from embeddors.
(Assignee)

Comment 1

18 years ago
Created attachment 35305 [details] [diff] [review]
Patch to create nsISecureBrowserUI during the creation of the webbrowser object
(Assignee)

Comment 2

18 years ago
Conrad had some corerns about exposing nsISecureBrowserUI as an attribute of 
nsIWebBrowser.

With the above patch this is what embedding apps will have to do do display 
security info. such as doing view certificate:

1. Get nsISecureBrowserUI from nsIWebBrowser (which they already have)
2. QI nsISecureBrowserUI for nsISSLStatusProvider.
3. Get the nsISSLStatus attribute from nsISSLStatusProvider.
4. Get cert info. etc. from nsISSLStatus.

I've got this scheme working in the mfcembed sample which embeddors can use as a 
starting point.

Please let me know if should be giving them some other interface than 
nsISecureBrowserUI. May be we give them access to an nsISSLStatus?
Status: NEW → ASSIGNED
(Assignee)

Updated

18 years ago
Blocks: 81808
(Assignee)

Comment 3

18 years ago
Created attachment 35587 [details] [diff] [review]
Patch to get nsISecureBrowserUI via a GetInterface rather tahn as an attribute.
(Assignee)

Comment 4

18 years ago
Hi Terry:

Could you please let me know if giving embeddors access to nsISecureBrowserUI is 
the way to go.

If yes, i need an r= and sr=
If no, please let me know the PSM interface we should be giving out

Thanks

Comment 5

18 years ago
Is it possible to create the secure UI on demand so we don't have the bloat if 
it's never used?

Also, can you update your mfcEmbed example to use the GetInterface version of 
this patch?
(Reporter)

Comment 6

18 years ago
> Is it possible to create the secure UI on demand so we don't have the bloat if 
it's never used?


I don't think so. When the component is created, it installs listeners which
have to be there initially.

Comment 7

18 years ago
Sorry to be so late with my comments:

Providing access to nsISecureBrowserUI isn't the most useful thing, since the
methods there are directly useful, and the client will need to QI to do
anything. It might be better to provide access to the nsISSLStatusProvider
interface, or directly to the nsISSLStatus attribute.

However, providing a direct attribute for nsISSLStatusProvider (say) would make
you depend on PSM interfaces in the web browser interface.  I think the
implementation already depends on PSM, so that may not be an issue.

If you don't mind adding the dependency, then I would add the
nsISSLStatusProvider to the web browser as an attribute.  If you don't like the
dependancy (in the interface) you could have your implementation implement
nsISSLProvider and allow the client to QI for it.

At some level you need the PSM interfaces to display the data, it's just a
question of whether that is assumed at the very low (browser interface) level.
Otherwise the interfaces you are using are fine. 

Comment 8

18 years ago
if we expose
http://lxr.mozilla.org/mozilla/source/security/manager/ssl/public/nsISSLStatus.idl#49
either/both of these ifaces, they'll become frozen. Terry, are you prepared for
that? Also, nsIX509Cert gets pulled in, which would inter freeze its entrails as
well.
(Reporter)

Comment 9

18 years ago
> However, providing a direct attribute for nsISSLStatusProvider (say) would make
you depend on PSM interfaces in the web browser interface.

Using a GetInterface instead of an attribute removes that concern. That was my
objection as well to using an attribute.

Jud's point is good. Should we have another layer of API which would be exosed
to embeddors to insulate us a bit from the deep internals of PSM?

Comment 10

18 years ago
(adding cc for Javi and David)

It would be a useful exercise for you to define the embedding view of the
security as a new API.  That would tell us what things are needed at that layer.
 If you end up covering most of the PSM intefaces anyway, we should just
standardize those.  However if you don't need certs, or ciphernames, or key
strength values (...) then we can just implement your frozen interfaces using
the underlying PSM ones.

Comment 11

18 years ago
heh, I was hoping you guys could provide the "what's needed" view :-). I have no
clue, and I suspect most embeddors don't have a clue either, so, you tell us
what you think the top properties should be, and we'll expose them through some
nsISecurePageInfo type iface, and modify that as true requirements surface.
(Assignee)

Comment 12

18 years ago
I think we should not be coming up with a new interface based on our assumptions 
on what embeddors may want or not. Some may be interested in the cert 
functionality while others may not care for it.

If we came up with a new intermediate interface, i'm pretty sure down the road 
there'll be someone who wants the missing functionality in the new interface(and 
those that are available in the PSM interfaces)
(Assignee)

Comment 13

18 years ago
Since it might be a while before we decide on what's the appropriate 
security info interface to expose to embeddors jud mentioned that i should at 
least go ahead and hook into the OnSecurityChange() notification for the lock 
icon updates.

I'll submit a patch with those changes shortly and would appreciate a r= and 
sr=.
(Assignee)

Comment 14

18 years ago
Created attachment 36361 [details] [diff] [review]
Patch with just the changes to hook into OnSecurityChange()
(Reporter)

Comment 15

18 years ago
Now the patch fits the summary of the bug. I'd file another one on allowing
embeddors to get page security info.

One little nit:
+   mBackgroundColor(0), mSecurityUI(nsnull)
You don't need that. The nsCOMPtr constructor initializes itself.

r=ccarlen

(Assignee)

Comment 16

18 years ago
*** Bug 63323 has been marked as a duplicate of this bug. ***
With conrad's comment fixed sr=blizzard
(Assignee)

Comment 18

18 years ago
Created attachment 36724 [details] [diff] [review]
Updated patch based on Conrad's comments i.e. no need to init mSecurityUI
(Assignee)

Comment 20

18 years ago
Fix checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 21

16 years ago
Clean up verification of dated code change bus
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.