Closed
Bug 265780
Opened 20 years ago
Closed 8 years ago
Want nsIAuthPrompt2 and nsIPrompt2
Categories
(Core :: Networking, enhancement, P1)
Core
Networking
Tracking
()
RESOLVED
INCOMPLETE
People
(Reporter: Biesinger, Unassigned)
References
(Blocks 7 open bugs)
Details
(Keywords: arch)
Attachments
(3 files, 5 obsolete files)
130.96 KB,
patch
|
Details | Diff | Splinter Review | |
9.28 KB,
patch
|
darin.moz
:
review+
|
Details | Diff | Splinter Review |
30.54 KB,
patch
|
darin.moz
:
review+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Comment 1•20 years ago
|
||
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?
Reporter | ||
Comment 2•20 years ago
|
||
hmm, this will probably also require nsIAuthPrompt2Wrapper (or nsIAuthPromptWrapper2) getNewAuthPrompter can maybe stay the same and make callers QI
Comment 3•20 years ago
|
||
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.
Comment 4•20 years ago
|
||
chpe: Can you elaborate on #2? Why do you want an asynchronous API?
Comment 5•20 years ago
|
||
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.
Reporter | ||
Comment 6•20 years ago
|
||
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)..
Reporter | ||
Updated•19 years ago
|
Priority: -- → P1
Reporter | ||
Updated•19 years ago
|
Priority: P1 → P2
Reporter | ||
Comment 7•19 years ago
|
||
I put this on the wiki for easier discussion: http://wiki.mozilla.org/Necko:nsIAuthPrompt2
Comment 8•18 years ago
|
||
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?
Comment 9•18 years ago
|
||
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 :)
Reporter | ||
Comment 10•18 years ago
|
||
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
Reporter | ||
Comment 11•18 years ago
|
||
although... maybe it would be better to wait with nsIPrompt2 until that nsIPromptService rewrite?
Comment 12•18 years ago
|
||
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?
Reporter | ||
Comment 13•18 years ago
|
||
it absolutely is, I am indeed working on it at the moment.
Reporter | ||
Updated•18 years ago
|
Priority: P2 → P1
Reporter | ||
Comment 14•18 years ago
|
||
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)
Reporter | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9alpha
Reporter | ||
Comment 15•18 years ago
|
||
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)
Comment 16•18 years ago
|
||
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?
Reporter | ||
Comment 17•18 years ago
|
||
That sounds like a great idea, I'll definitely make that change.
Reporter | ||
Comment 18•18 years ago
|
||
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)
Updated•18 years ago
|
Flags: blocking1.9a2?
Flags: blocking1.9a1?
Flags: blocking1.9+
Comment 19•18 years ago
|
||
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+
Reporter | ||
Comment 20•18 years ago
|
||
I would rather not rename an interface that's de facto frozen...
Reporter | ||
Comment 21•18 years ago
|
||
>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).
Reporter | ||
Comment 22•18 years ago
|
||
Works in Firefox, SeaMonkey and TestGtkEmbed. Checked in on trunk.
Attachment #234071 -
Attachment is obsolete: true
Reporter | ||
Comment 23•18 years ago
|
||
make the security level parameter useful
Attachment #236157 -
Flags: review?(darin)
Comment 24•18 years ago
|
||
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...
Reporter | ||
Comment 25•18 years ago
|
||
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...
Comment 26•18 years ago
|
||
> 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.
Reporter | ||
Comment 27•18 years ago
|
||
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)
Updated•18 years ago
|
Attachment #236483 -
Flags: review?(darin) → review+
Reporter | ||
Comment 28•18 years ago
|
||
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)
Reporter | ||
Comment 29•18 years ago
|
||
Attachment #239459 -
Flags: review?(darin)
Reporter | ||
Comment 30•18 years ago
|
||
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 31•18 years ago
|
||
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+
Reporter | ||
Comment 32•18 years ago
|
||
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)
Comment 33•18 years ago
|
||
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.
Reporter | ||
Comment 34•18 years ago
|
||
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.
Comment 35•18 years ago
|
||
>+ nsCOMPtr<nsIProxyInfo> info;
>+ proxied->GetProxyInfo(getter_AddRefs(info));
>+ NS_ASSERTION(proxied, "proxy auth needs nsIProxyInfo");
s/proxied/info/ ?
Comment 36•18 years ago
|
||
I think so, yes. I've taken the liberty of checking in that change.
Reporter | ||
Comment 37•18 years ago
|
||
oops. thanks for fixing this.
Updated•17 years ago
|
Flags: in-testsuite+
Comment 38•17 years ago
|
||
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
Comment 39•17 years ago
|
||
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.
Comment 40•17 years ago
|
||
Just chatting with biesi on this one. Should this really be blocking 1.9? Stuart, comments?
Updated•17 years ago
|
Flags: blocking1.9+ → blocking1.9-
Comment 41•17 years ago
|
||
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?)
Reporter | ||
Updated•12 years ago
|
Assignee: cbiesinger → nobody
Target Milestone: mozilla1.9alpha1 → ---
Updated•8 years ago
|
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.
Description
•