397403 - NS_NewAuthPrompter needs to check if the password manager prompt factory can handle password prompts.

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Mark Banner (:standard8)]

Attachment: Allow NS_NewAuthPrompter to check the password manager prompt factory.

NS_NewAuthPrompter2 first checks to see if a NS_PWMGR_AUTHPROMPTFACTORY exists to handle an auth prompt. If it doesn't it hands it off to nsPrompt.

NS_NewAuthPrompter hands it off to nsPrompt straight away, without checking for a NS_PWMGR_AUTHPROMPTFACTORY:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/embedding/components/windowwatcher/src/nsPrompt.cpp&rev=1.30&mark=80-143#80

As part of bug 382437 we're extending toolkit's password manager to implement nsIAuthPrompt, so it makes sense to match NS_NewAuthPrompter to NS_NewAuthPrompter2.

The patch attached does the required change to check for the password manager. I've extended the failure case in this instance to check for NS_NOINTERFACE as the wallet password manager doesn't have all the required and it doesn't really make sense for us to implement the interfaces when we're hoping to drop wallet pm real soon by implementing bug 382437.

Also I'm not dropping the nsIAuthPromptWrapper bit in NS_NewAuthPrompter, as again wallet is still using it, and also Camino.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 282166 [details] [diff] [review]
Allow NS_NewAuthPrompter to check the password manager prompt factory.

looks good, but a unit test would be nice (the unit test could register an authpromptfactory, call nsIWindowWatcher.getNewAuthPrompter with some made-up window, call the functions on nsIAuthPrompt and make sure that they end up in the object that the authpromptfactory returned)

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 282166 [details] [diff] [review]
Allow NS_NewAuthPrompter to check the password manager prompt factory.

biesi, would you be happy to sr this as well?

I've had a look at the unit test, and the problem I'm getting at the moment is that the auth prompt factory that I'm trying to implement isn't getting picked up - the Firefox Password Manager version is taking priority.

Any ideas on how I could get around that? or a different way of doing the test.

We could check for a nsPrompt being returned, but all the variations do that anyway...

Comment 3

[Mark Banner (:standard8)]

Attachment: Unit test

Unit test for this bug. Implements a prompt factory and checks that GetPrompt is correctly called for both nsIAuthPrompt and nsIAuthPrompt2 cases.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 283600 [details] [diff] [review]
Unit test

get it right this time...

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 283600 [details] [diff] [review]
Unit test

+  createInstance: function eventsink_ci(outer, iid) {

I suggest s/eventsink/tPF/ for the function names here :)

+        aIID.equals(Ci.nsIAuthPrompt2))
+      authPromptRequestReceived = true;

hm... I'd return an object here. return {} should work, I think.

and for unknown IIDs, you should throw Components.results.NS_ERROR_NO_INTERFACE

+  ww.nsIPromptFactory.getPrompt(null, Ci.nsIAuthPrompt);

maybe also check the result to be not null

Comment 6

[Mark Banner (:standard8)]

Attachment: Combined patch - Allow NS_NewAuthPrompter to check the password manager prompt factory + test case

Combined patch addressing biesi's comments on the test case.

Requesting approval 1.9. SeaMonkey and Thunderbird need this (and bug 382437 which is WIP) to be able to pick up toolkit's password manager. 

Has been designed to not affect existing apps where the password manager doesn't implement nsIAuthPrompt, so should be low risk. Also includes a unit test.

Comment 7

[Mark Banner (:standard8)]

Patch checked in -> fixed.