Closed Bug 265780 Opened 20 years ago Closed 8 years ago

Want nsIAuthPrompt2 and nsIPrompt2

Categories

(Core :: Networking, enhancement, P1)

enhancement

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: Biesinger, Unassigned)

References

(Blocks 7 open bugs)

Details

(Keywords: arch)

Attachments

(3 files, 5 obsolete files)

nsIAuthPrompt is about to be frozen (bug 265513), but has issues.

Most especially, it forces a stringbundle dependency onto channels (which is the
main reason why I dislike it), but there are others, see bugs this blocks
(although this won't necessarily fix all of them)
Attached file suggested interface (obsolete) —
suggested interface. comments are very welcome.

should I provide a way to only ask for a username? (sorta like the old prompt
method, which this interface no longer provides)

should I rename the method, so that both nsIAuthPrompt and nsIAuthPrompt2 can
be easily implemented in languages w/o overloading?

Especially, do people think that any datapiece is missing for implementors?
hmm, this will probably also require nsIAuthPrompt2Wrapper (or
nsIAuthPromptWrapper2)

getNewAuthPrompter can maybe stay the same and make callers QI
I've been looking for a way to integrate GNOME Keyring support with mozilla, and
nsIAuthPrompt doesn't fit my needs.
1) The 'realm' passed to nsIAuthPrompt is hardcoded to what wallet wants. It
should instead pass the URI, the real realm, and whether this is for proxy auth,
to the auth prompt implementation
2) The API is synchronous: while we're being prompted, progress in all browser
windows is stalled.

The proposed nsIAuthPrompt2 interface doesn't address these points; but 2) may
be beyond this bug.
chpe: Can you elaborate on #2?  Why do you want an asynchronous API?
Let me elaborate on why I thought async API is/would be preferable:
* GNOME Keyring API is async: you ask for the data, and get called back when
it's ready. However, it also provides sync API, so it's not mandatory on this
account.
* Currently, the gtk embed prompts are modal. While it shows the prompt,
networking is stalled in all browser windows (tested in Epiphany and
TestGtkEmbed). When I modify the embed prompter so that it's not app-modal, but
only window modal (or not modal at all), I can use other browser windows, but
loads will not progress, and clicking a link in an already-loaded page will not
load the link -- until the prompt has been dismissed. 
Further testing reveals that this does NOT happen in mozilla-the-browser... so
I'm not sure if this reason is valid, either.
chpe:
>Further testing reveals that this does NOT happen in mozilla-the-browser... so
>I'm not sure if this reason is valid, either.

that's strange... because bug 74331 is about mozilla-the-browser and that
behaviour (modal dialog blocks all networking)..
Priority: -- → P1
Priority: P1 → P2
I put this on the wiki for easier discussion:
http://wiki.mozilla.org/Necko:nsIAuthPrompt2
Blocks: 223636
Blocks: 338549
I think we also need a way to do asynchronous prompts.  Should I file a bug on nsIPrompt2 or would you like to morph this bug into covering both nsIAuthPrompt2 and nsIPrompt2?
There's bug 228207 for nsIPrompt[Service]2, which is aimed at addressing the embedder's i18n problem, but I'd love to have an nsIPrompt*2 that also addresses the sync problem :)
I can do nsIPrompt2 w/ asynchronicity here. (hm... there's also http://lxr.mozilla.org/seamonkey/source/embedding/components/windowwatcher/public/nsINonBlockingAlertService.idl)
Summary: Want nsIAuthPrompt2 → Want nsIAuthPrompt2 and nsIPrompt2
although... maybe it would be better to wait with nsIPrompt2 until that nsIPromptService rewrite?
This bug blocks bug 338549, which is a pretty serious regression. Is this bug still on someone's radar or To-Do list? The latest comment is 2 months old.
Flags: blocking1.9a2?
Flags: blocking1.9a1?
it absolutely is, I am indeed working on it at the moment.
Priority: P2 → P1
Attached patch patch (obsolete) — Splinter Review
part I

Needs followup patches for:
- NTLM (I mishandle this in a few places)
- Channels other than HTTP
- Asynchronicity. Sorry, not part of this patch...
- Making the level parameter useful
Attachment #163161 - Attachment is obsolete: true
Attachment #231853 - Flags: review?(darin)
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9alpha
Attached patch patch v1.1 (obsolete) — Splinter Review
better unit test for authpromptwrapper, and fixed domain handling in it
Attachment #231853 - Attachment is obsolete: true
Attachment #232239 - Flags: review?(darin)
Attachment #231853 - Flags: review?(darin)
Question:

What do you think about re-defining nsIAuthPrompt2Factory as:

  [scriptable, uuid(...)]
  interface nsIPromptFactory : nsISupports {
    void getPrompt(in nsIDOMWindow parent, in nsIIDRef iid,
                   [iid_is(iid),retval] out nsQIResult prompt);
  };

So, instead of this being a factory specific to nsIAuthPrompt2, it becomes a factory interface suitable for use with any sort of prompt-type interface.  It could be used with nsIPrompt, nsIAuthPrompt, or nsIAuthPrompt2.

It might even be good to have the class objects for NS_DEFAULTPROMPT_CONTRACTID and NS_DEFAULTAUTHPROMPT_CONTRACTID QI to nsIPromptFactory.

Thoughts?
That sounds like a great idea, I'll definitely make that change.
Attached patch patch v1.2 (obsolete) — Splinter Review
with that change (except the class object part, which is maybe more effort than it's worth)
Attachment #232239 - Attachment is obsolete: true
Attachment #234071 - Flags: review?(darin)
Attachment #232239 - Flags: review?(darin)
Flags: blocking1.9a2?
Flags: blocking1.9a1?
Flags: blocking1.9+
Comment on attachment 234071 [details] [diff] [review]
patch v1.2

>Index: embedding/components/windowwatcher/public/nsIPromptService2.idl
...
>+  boolean promptUsernameAndPassword2(in nsIDOMWindow aParent,
>+  nsICancelable promptPasswordAsync(in nsIDOMWindow aParent,

nit: as suggested over IM, perhaps promptAuth and asyncPromptAuth
would be better names for these methods.


>Index: embedding/components/windowwatcher/src/nsPrompt.cpp
...
>+AuthPromptWrapper::PromptUsernameAndPassword(nsIChannel* aChannel,
...
>+      if (flags & nsIAuthInformation::NEED_DOMAIN) {
>+        // Domain is separated from username by a backslash
>+        PRInt32 idx = user.FindChar(PRUnichar('\\'));
>+        if (idx == kNotFound) {
>+          aAuthInfo->SetUserName(user);
>+        } else {
>+          aAuthInfo->SetDomain(Substring(user, 0, idx));
>+          aAuthInfo->SetUserName(Substring(user, idx + 1));
>+        }
>+      } else {
>+        aAuthInfo->SetUserName(user);
>+      }
>+      aAuthInfo->SetPassword(password);

It feels like there is some duplication of code going on here.
Can things be re-factored in such a way as to share more code?


>Index: netwerk/base/public/nsIAuthInformation.idl
...
>+  attribute AString userName;

nit: nsIURI has "username", so perhaps this interface should as well
for consistency.


>Index: netwerk/base/public/nsIAuthPrompt2.idl
...
>+  boolean promptUsernameAndPassword(in nsIChannel aChannel,
>+  nsICancelable promptPasswordAsync(in nsIChannel aChannel,

As I suggested over IM, perhaps better names for these methods
would be promptAuth and asyncPromptAuth.


>Index: netwerk/base/public/nsIAuthPromptAdapterFactory.idl
...
>+[scriptable, uuid(60e46383-bb9a-4860-8962-80d9c5c05ddc)]
>+interface nsIAuthPromptAdapterFactory : nsISupports
>+{
>+  /**
>+   * Wrap an object implementing nsIAuthPrompt so that it's usable via
>+   * nsIAuthPrompt2.
>+   */
>+  nsIAuthPrompt2 makePromptWrapper(in nsIAuthPrompt aPrompt);
>+};

How about naming this method: createAdapter or createInstance?


>Index: netwerk/base/public/nsIAuthPromptCallback.idl
...
>+[scriptable, uuid(bdc387d7-2d29-4cac-92f1-dd75d786631d)]
>+interface nsIAuthPromptCallback : nsISupports

Should this be named nsIAuthPrompt2Callback?  Sigh, maybe not.
You know, you could probably arrange to have the old nsIAuthPrompt
renamed to nsIAuthPromptObsolete.  That would only impact old JS
code, which anyways should be protected by Firefox's version
system.


>Index: netwerk/base/public/nsIAuthPromptProvider.idl
...
>+[scriptable, uuid(bd9dc0fa-68ce-47d0-8859-6418c2ae8576)]
> interface nsIAuthPromptProvider : nsISupports

As with nsIPromptFactory, perhaps nsIAuthPromptProvider
should take an IID parameter to select the auth prompt
you wish to receive.


>Index: netwerk/protocol/http/src/nsHttpChannel.cpp
...
>+class nsAuthInformationHolder : public nsIAuthInformation {
>+    public:
>+        // aAuthType must be ASCII
>+        nsAuthInformationHolder(PRUint32 aFlags, const nsString& aRealm,
>+                                const nsCString& aAuthType)
>+            : mFlags(aFlags), mRealm(aRealm), mAuthType(aAuthType) {}
>+

nit: fix indentation here to be consistent with other HTTP code.


>Index: toolkit/components/passwordmgr/base/nsSingleSignonPrompt.cpp
...
>+NS_IMETHODIMP
>+nsSingleSignonPrompt2::PromptUsernameAndPassword(nsIChannel* aChannel,

Can you implement nsSingleSignonPrompt::PromptUsernameAndPassword in
terms of nsSingleSignonPrompt2?  Or, is there at least some way in
which the two can share code?


r=darin with suggested fixes
Attachment #234071 - Flags: review?(darin) → review+
Blocks: 227632
I would rather not rename an interface that's de facto frozen...
>Can you implement nsSingleSignonPrompt::PromptUsernameAndPassword in
>terms of nsSingleSignonPrompt2?  Or, is there at least some way in
>which the two can share code?

Hm, there is already such code duplication in that file (in PromptPassword). And I can't implement either function in terms of the other. Helper functions would, I think, not help very much, so... I'd like to leave this as is (or, as it will be in the patch I'll attach tomorrow, which shares more code between nsPrompt and the password managers).
Works in Firefox, SeaMonkey and TestGtkEmbed. Checked in on trunk.
Attachment #234071 - Attachment is obsolete: true
Attached patch part II (obsolete) — Splinter Review
make the security level parameter useful
Attachment #236157 - Flags: review?(darin)
Comment on attachment 236157 [details] [diff] [review]
part II

>+     * How secure the credentials will be transmitted. This should be either
>+     * nsIAuthPrompt2::LEVEL_NONE or nsIAuthPrompt2::LEVEL_PW_ENCRYPTED and
>+     * can be dependent on the current state.
>+     */
>+    readonly attribute unsigned long securityLevel;

Can this be done by extending the authFlags?  That would be nice
because it would avoid any interface changes.  If someone created
an Auth plug-in, it would be nice if it would continue to work.
Maybe the default security level could be "unknown"

The downside to this approach is that it requires new flags to
be defined, and then you need to map them to nsIAuthPrompt2 security
levels.  I think that might be worth it.. not sure.


>Index: netwerk/protocol/http/src/nsHttpChannel.cpp

>+            if (scheme.EqualsLiteral("https"))
>+                level = nsIAuthPrompt2::LEVEL_SECURE;

Should we care if the certificate is valid?


By the way, negotiate auth is actually a bit hard to evaluate for
security level.  Technically, it's just a wrapper protocol for any
authentication scheme.  In fact, you could even use negotiate auth
to do something weak like basic auth.  In that case, is it really
secure?  Maybe we can get away with assuming that negotiate auth
will never be used like that... hmm...
The code has no way to check if the cert is valid, does it? Aren't those interfaces all in PSM, inaccessible to necko?

Hrm, there is currently no "unknown" level, should there be?

As for negotiate, it never shows an authentication prompt, does it? Meaning... it doesn't matter all that much what its securityLevel is.

As for flags vs this new attribute... I was thinking that for things like NTLM, the value of this attribute might change over the lifetime of the object while the flags should stay constant. The securityLevel would indicate the level of encryption the server provides. Hm... since there only is  "encrypted" and "not encrypted" maybe that doesn't make that much sense...
> The code has no way to check if the cert is valid, does it? Aren't those
> interfaces all in PSM, inaccessible to necko?

Yeah they are.  Maybe something should be done about that.  Necko is afterall a consumer of PSM ;-)


> Hrm, there is currently no "unknown" level, should there be?

Maybe.  We won't know in some cases perhaps.


> As for negotiate, it never shows an authentication prompt, does it? Meaning...
> it doesn't matter all that much what its securityLevel is.

Good point.  We should make it clear in the nsIHttpAuthenticator documentation that the security level is only meaningful if the authenticator requests an identity from the user.


> As for flags vs this new attribute... I was thinking that for things like NTLM,
> the value of this attribute might change over the lifetime of the object while
> the flags should stay constant.

Hmm, in that case shouldn't the security level be a property of the response generated by the authenticator?  Or, shouldn't it somehow be more tightly associated with a particular authentication request made of the authenticator?  I'm not sure how that would work or even what the use case is for that.
This just adds an IDENTITY_ENCRYPTED flag. If that's not set that's just treated as unencrypted. I think that's fine.
Attachment #236157 - Attachment is obsolete: true
Attachment #236483 - Flags: review?(darin)
Attachment #236157 - Flags: review?(darin)
Attachment #236483 - Flags: review?(darin) → review+
Comment on attachment 236483 [details] [diff] [review]
part II v1.1: simpler is better (checked in)

part II fixed on trunk
Attachment #236483 - Attachment description: part II v1.1: simpler is better → part II v1.1: simpler is better (checked in)
oh, maybe a short summary of the patch:
- Moves nsAuthInformationHolder into netwerk/base for reuse by non-HTTP channels
- Makes FTP use that
- Adds an FTP test to test_authpromptwrapper

I patched wallet but not satchel here because apparently satchel doesn't have this particular strange behaviour of wallet in PromptPassword, so I didn't imitate it in PromptAuth.
Comment on attachment 239459 [details] [diff] [review]
part II v1: Make FTP use nsIAuthPrompt2 (checked in)

>Index: extensions/wallet/src/singsign.cpp

>+  PRUint32 flags;
>+  aAuthInfo->GetFlags(&flags);
>+
>   if (checked) {
>     NS_SetAuthInfo(aAuthInfo, username, password);
>+    // If we were only asked for a password, return immediately
>+    // (to match SINGSIGN_PromptPassword)
>+    if (flags & nsIAuthInformation::ONLY_PASSWORD)

Maybe |flags| should be initialized.


r=darin
Attachment #239459 - Flags: review?(darin) → review+
Comment on attachment 239459 [details] [diff] [review]
part II v1: Make FTP use nsIAuthPrompt2 (checked in)

FTP patch checked in, with that change
Attachment #239459 - Attachment description: part II v1: Make FTP use nsIAuthPrompt2 → part II v1: Make FTP use nsIAuthPrompt2 (checked in)
I'm confused about which specific things the check-ins are supposed to have addressed. Per comments 21 and 22, something was checked in regarding password prompts (I think), but I'm still getting repeated prompts in Seamonkey mail for master password the first time it starts up. I'm using today's win32 trunk build. Or was that check in for something else? Thanks for helping me understand.
hm, I'm actually not sure that master passwords will be fixed at all by this bug, but if it is, then not until the next patch or so.
>+    nsCOMPtr<nsIProxyInfo> info;
>+    proxied->GetProxyInfo(getter_AddRefs(info));
>+    NS_ASSERTION(proxied, "proxy auth needs nsIProxyInfo");


s/proxied/info/ ?
I think so, yes.  I've taken the liberty of checking in that change.
oops. thanks for fixing this.
Flags: in-testsuite+
The crash in bug 368325 is thought to be related to this change. 
I ask one of the developers/reviewers of *this* bug to have a look to see 
if these are truly related.  

Please see the stack trace in attachment 262476 [details].  There, we see 
nsThread::ProcessNextEvent invokes nsStreamListenerTee::OnDataAvailable 
which calls some MIME code which creates some objects, and then ultimately 
calls nsPrompt::PromptPassword to do a modal dialog, prompting for the 
"master password".  

nsXULWindow::ShowModal() calls nsThread::ProcessNextEvent (again, a second
time on the same stack), which invokes nsStreamListenerTee::OnStopRequest,
which destroys the objects being used by the MIME code farther down in the
same stack.  The call stack in attachment 262476 [details] shows the point where the
MIME objects are being destroyed while still in use farther down the stack.
When the stack unwinds back into that MIME code, it crashes because the 
object pointers in the stack now point to freed memory.

This destructive recursion didn't happen until about the time that the 
problem with non-serial password prompts was noticed.  Could they have the
same root cause?
Blocks: 368325
Nelson, the problem you're describing sounds more like a consequence of bug 326273, not of this bug...

And yes, serial prompts and that problem are the same issue, really.
Just chatting with biesi on this one.  Should this really be blocking 1.9?  Stuart, comments?
Flags: blocking1.9+ → blocking1.9-
It looks like this bug -doesn't- need to block 1.9 anymore, if the pieces of async auth that bug 338549 depends on are actually done.  Thoughts?  Should this bug actually be closed out (with bugs in the async auth stuff dealt with in smaller subsequent bugs?)
No longer blocks: 338549
Assignee: cbiesinger → nobody
Target Milestone: mozilla1.9alpha1 → ---
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.