Open Bug 1391093 Opened 7 years ago Updated 2 years ago

Enable some UI to be used in an onAuthRequired callback

Categories

(WebExtensions :: Frontend, enhancement, P3)

Product:
WebExtensions
Component:
Frontend
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: markh, Unassigned)

References

Details

Attachments

(2 obsolete files) 

Bug 1341126 has added the ability to open a popup or sidebar programatically, which offers a potential solution to webextensions which want to offer some UI during an onAuthRequired callback, so the user can select the credentials to use with http-basic authentication.

However, these new functions are only available when the browser is processing user input. It would be ideal if we would also allow these functions to be called by an extension which has an outstanding onAutoRequired callback promise pending.

Shane, do you have any thoughts on the best way to actually implement this? Would it be OK to reuse/abuse the requireUserInput flag (ie, to somehow arrange for the block at http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionChild.jsm#643 to also check this and allow the call?)
Flags: needinfo?(mixedpuppy)
I'm OK with doing this, but we should make sure that we don't grant the activeTab permission in those cases.
I'm not aware of a way to set isHandlingUserInput but I'm sure we could add some kind of internal bypass flag that could be handled in callAsyncFunction.  aswan knows the userinput code, lets see what he thinks.
Flags: needinfo?(mixedpuppy) → needinfo?(aswan)
It is possible to set isHandlingUserInput, but we definitely *should not* set it for onAuthRequired callbacks. This needs to be treated as a special case.
Flags: needinfo?(aswan)
Woudln't this negate the HandlingUserInput requirement for open() methods?  A web extension could issue a request that triggers the onAuthRequired whenever it wants to bug the user with a popup...
An extension with webRequestBlocking permission could, yes. But I'm not too worried about that as long as we don't also grant activeTab permissions in that case.

And, of course, any extension that we found doing that would be blocklisted with extreme prejudice.
The code that checks an individual api call is here:
http://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/toolkit/components/extensions/ExtensionChild.jsm#642-649
It sounds like that check needs to be expanded.

Since you don't have an actual event to refer to here, you should think ahead of time about which top-level window you want the popup to open in (if some non-active tab in a non-active window generates onAuthRequired do I get the popup in my foreground window?).  It looks like bug 1341126 unconditionally opens the popup in the top window.
(In reply to Kris Maglione [:kmag] from comment #3)
> It is possible to set isHandlingUserInput, but we definitely *should not*
> set it for onAuthRequired callbacks. This needs to be treated as a special
> case.

Do you have thoughts on how that should be expressed? For this specific case the addon needs "webRequestBlocking" permissions, so it might be OK to express this as webext permissions that override the isHandlingUserInput flag (and for this specific case, it would be just ["webRequestBlocking"]). OTOH, that might be too broad - thoughts?

(In reply to Andrew Swan [:aswan] from comment #6)
> Since you don't have an actual event to refer to here, you should think
> ahead of time about which top-level window you want the popup to open in (if
> some non-active tab in a non-active window generates onAuthRequired do I get
> the popup in my foreground window?).  It looks like bug 1341126
> unconditionally opens the popup in the top window.

Yeah, that's a good point. I've got an example which uses a pageAction item, so doesn't suffer from that problem. A pageAction seems to make more sense for this specific requirement anyway, but I bet some password manager authors might prefer to use their existing browserAction. I'm new to much of this, so I'm just taking one step at a time :)
Flags: needinfo?(kmaglione+bmo)
Priority: -- → P5
Product: Toolkit → WebExtensions
Moving this back on the radar due to https://github.com/bitwarden/browser/issues/116

While a browser/page action may be an easy path, there are other issues per comment 6.  We may want to consider some other mechanism, but am unsure what right now.
Priority: P5 → P2
Summary: Allow API to open browser/page/sidebarAction to be used in an onAuthRequired callback → Enable some UI to be used in an onAuthRequired callback
Assignee: nobody → mixedpuppy
Priority: P2 → P1
Flags: needinfo?(kmaglione+bmo)
(In reply to Andrew Swan [:aswan] from comment #6)

> Since you don't have an actual event to refer to here, you should think
> ahead of time about which top-level window you want the popup to open in (if
> some non-active tab in a non-active window generates onAuthRequired do I get
> the popup in my foreground window?).  It looks like bug 1341126
> unconditionally opens the popup in the top window.

We can allow extensions to determine that behavior, and if we want to tighten the screws later we can.  The patch in phab now contains an illustration of dealing with authentication in background windows.
Attachment #9011006 - Attachment is obsolete: true
(In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> We can allow extensions to determine that behavior, and if we want to
> tighten the screws later we can.  The patch in phab now contains an
> illustration of dealing with authentication in background windows.

I don't see this in the patch on phabricator, the test loads the page that needs authentication in a foreground tab.

In any case, I'm not sure how I feel about the idea of just-let-the-extension-handle-it.  Extension authors that don't consider this case are going to create a very confusing experience..
(In reply to Andrew Swan [:aswan] from comment #14)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> > We can allow extensions to determine that behavior, and if we want to
> > tighten the screws later we can.  The patch in phab now contains an
> > illustration of dealing with authentication in background windows.
> 
> I don't see this in the patch on phabricator, the test loads the page that
> needs authentication in a foreground tab.

foreground within its own window.  But I'll restructure the test a bit to prove that.

> In any case, I'm not sure how I feel about the idea of
> just-let-the-extension-handle-it.  Extension authors that don't consider
> this case are going to create a very confusing experience..

I think it's an acceptable solution.  We cannot know how an extension will choose to handle an auth call (maybe they already have the login data and will just return it, maybe not), so we cannot make the decision to force-focus a tab that needs it.
(In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> I think it's an acceptable solution.  We cannot know how an extension will
> choose to handle an auth call (maybe they already have the login data and
> will just return it, maybe not), so we cannot make the decision to
> force-focus a tab that needs it.

I don't understand this comment, this bug is explicitly about the case where the extension does not have data available to handle the callback without user interaction.
(In reply to Andrew Swan [:aswan] from comment #16)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> > I think it's an acceptable solution.  We cannot know how an extension will
> > choose to handle an auth call (maybe they already have the login data and
> > will just return it, maybe not), so we cannot make the decision to
> > force-focus a tab that needs it.
> 
> I don't understand this comment, this bug is explicitly about the case where
> the extension does not have data available to handle the callback without
> user interaction.

And how do you know if the extension is going to need user interaction?
(In reply to Shane Caraveo (:mixedpuppy) from comment #17)
> And how do you know if the extension is going to need user interaction?

I have no idea, my only point is that I think that letting extensions open a popup in the foreground window based on something in a background window is a bad idea.  I don't have a better idea off the top of my head but if we land this right now we're making an implicit promise to continue to support it, I don't think that's something we should do hastily.  Lets talk about it in the Tuesday morning meeting.
We discussed this at the UX meeting this morning and decided to let the patch land as-is. The use of http auth is pretty low these days, the extension needs to have a reason to show a UI (like a selector for multiple potential credentials), and the user needs to have multiple windows open, all of which makes this feel like an edge case. There was no clear consensus on what the "right" thing to do would be, and a feeling that any guess as to what the right thing is might not be any better than letting the extension handle it (from a UX perspective).
(In reply to Kris Maglione [:kmag] from comment #1)
> I'm OK with doing this, but we should make sure that we don't grant the
> activeTab permission in those cases.

I don't see any code in this patch to actually block ActiveTab usage. Did we implement this part yet?
What's the status here Andrew? Are you still working on this? I see a bit of movement in the discussion doc, but I don't know what the outcome or decision was about the approach to tackle this?
Er, by Andrew, I mean Shane, of course.
(In reply to Paul Theriault [:pauljt] from comment #22)
> Er, by Andrew, I mean Shane, of course.

I'm thinking that the patch here is not the likely approach, but will wait more on discussion with product/etc. before settling on what we do.
Attachment #9009676 - Attachment is obsolete: true
Priority: P1 → P2

I think this no longer block sec-proxy.

No longer blocks: secure-proxy
Priority: P2 → P3
Assignee: mixedpuppy → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: