1206232 - Add support for the 'Block Temporarily' permission state

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P2)

Comment 1

[[e-mail]]

Marco, could you elaborate whats the specific new module required here?

Comment 2

[Marco Mucci [:MarcoM]]

(In reply to Fred Lin [:gasolin] from comment #1)
> Marco, could you elaborate whats the specific new module required here?

Forwarding to Javaun.

Comment 3

[:Paolo Amadini]

Florian, I think this separate bug may not be needed because we now only show device usage for camera and microphone, and you've already implemented it, and for the permissions on the tab we have a call in SitePermissions.jsm to query by URL. Is it correct?

Comment 4

[Florian Quèze [:florian]]

(In reply to :Paolo Amadini from comment #3)
> Florian, I think this separate bug may not be needed because we now only
> show device usage for camera and microphone, and you've already implemented
> it

If we only show usage for camera/microphone/screensharing, then my work is enough. At some point I heard mentions of doing the same treatment for geolocation usage, but nobody has mentioned it recently, so maybe we can drop it.

> for the permissions on the tab we have a call in SitePermissions.jsm
> to query by URL. Is it correct?

It's my understanding that SitePermissions.jsm is mostly a proxy to Services.perms, and doesn't keep track of the 'allowed temporarily' / 'blocked temporarily' tab-specific states. If this assumption is correct, we could repurpose this bug to cover handling of this case.

Comment 5

[:Paolo Amadini]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> It's my understanding that SitePermissions.jsm is mostly a proxy to
> Services.perms, and doesn't keep track of the 'allowed temporarily' /
> 'blocked temporarily' tab-specific states. If this assumption is correct, we
> could repurpose this bug to cover handling of this case.

Do we have tab-specific "allowed temporarily" cases now, or are they planned? Since the Services.perms API works only by origin it cannot distinguish them currently, and we surely don't display them in the Control Center now.

Comment 6

[Florian Quèze [:florian]]

(In reply to :Paolo Amadini from comment #5)
> (In reply to Florian Quèze [:florian] [:flo] from comment #4)
> > It's my understanding that SitePermissions.jsm is mostly a proxy to
> > Services.perms, and doesn't keep track of the 'allowed temporarily' /
> > 'blocked temporarily' tab-specific states. If this assumption is correct, we
> > could repurpose this bug to cover handling of this case.
> 
> Do we have tab-specific "allowed temporarily" cases now, or are they
> planned?

It's planned, eg. for the 'blocked temporarily' state for geolocation on this mockup:
https://mozilla.invisionapp.com/share/AF71R266U#/screens/142999153

We really need this for camera/microphone, so that we can put the sharing indicator with the (i) icon (https://mozilla.invisionapp.com/share/AF71R266U#/screens/143001431 ) instead of on a separate popupnotification. This separate popupnotification currently holds the 'Stop sharing' feature, which should move to the [X] next to the 'allowed temporarily' wording in the control center panel.

By the way, I'm not saying we need a specific JS module to handle this. Keeping the "allowed/blocked temporarily" information in a JS object somehow attached to the control center area of the tab may be all we need; as long as we preserve this information when tearing off tabs.

Comment 7

[:Paolo Amadini]

(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> We really need this for camera/microphone, so that we can put the sharing
> indicator with the (i) icon
> (https://mozilla.invisionapp.com/share/AF71R266U#/screens/143001431 )
> instead of on a separate popupnotification. This separate popupnotification
> currently holds the 'Stop sharing' feature, which should move to the [X]
> next to the 'allowed temporarily' wording in the control center panel.

Doesn't this represent the "in use" case? Anyways, if I understand correctly this work is not in scope for the permissions MVP we originally estimated, but forms part of the WebRTC work?

We can keep this bug around, but flagging for triage if we want to repurpose it.

Comment 8

[Florian Quèze [:florian]]

(In reply to :Paolo Amadini from comment #7)
> (In reply to Florian Quèze [:florian] [:flo] from comment #6)
> > We really need this for camera/microphone, so that we can put the sharing
> > indicator with the (i) icon
> > (https://mozilla.invisionapp.com/share/AF71R266U#/screens/143001431 )
> > instead of on a separate popupnotification. This separate popupnotification
> > currently holds the 'Stop sharing' feature, which should move to the [X]
> > next to the 'allowed temporarily' wording in the control center panel.
> 
> Doesn't this represent the "in use" case?

Yes, for the "Allowed temporarily" case. But "Blocked temporarily" isn't covered anywhere (I think).

Comment 9

[Johann Hofmann [:johannh]]

Attachment: Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review commit: https://reviewboard.mozilla.org/r/80706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80706/

Comment 10

[Johann Hofmann [:johannh]]

So, a first WIP patch from me. All features around temporary permissions should be working for Geolocation and WebRTC permissions. This patch is based on central but we should land it on Elm after bug 1282768 has landed there. There are too many changes on the API consumers right now and we should merge this in last. Alternatively we could only land the API code on central, as we did in bug 1291642 and make a new bug for integrating.

My idea was that if we don't want to do this on platform side (which is probably ok since it's more of a client feature), we should avoid putting too much work on the API-consumers. Especially since these consumers are heavily changing right now. So I went for enhancing SitePermissions with extra powers that come into play once you pass a browser object (and switching all Services.perms users to SitePermissions). This also allows us to get rid of the hack-ish version of "Temporarily Allowed" we had implemented in bug 1206233.

This is only superficially tested and I probably missed lots of edge cases. Also none of the existing tests were modified and as you can see there are no new tests yet. Note that I had a lot of crashes while testing but after rebasing they seem to be gone.

Feel free to criticize, tear apart or shed completely if the general approach seems wrong to you ;)

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80706/diff/1-2/

Comment 12

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

https://reviewboard.mozilla.org/r/80706/#review79766

::: browser/modules/SitePermissions.jsm:166
(Diff revision 2)
> -      this.remove(aURI, aPermissionID);
> +      this.remove(aURI, aPermissionID, aBrowser);
> +      return;
> +    }
> +
> +    if (aState == this.SESSION) {
> +      Services.perms.add(aURI, aPermissionID, this.ALLOW, Services.perms.EXPIRE_SESSION);

This is a big issue with this patch. The problem is that SitePermissions.jsm doesn't have the concept of "Expiry" of permissions and so I've been using this hack to get around that. There are several downsides of using this hack, e.g. this looks like a permanent permission when retrieving it for usage on labels.

We should really try to convert SitePermissions.jsm to work with permission expire times instead somehow.

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

https://reviewboard.mozilla.org/r/80706/#review80294

The approach seems reasonable.

::: browser/base/content/browser.js:1067
(Diff revision 2)
>      gBrowser.addEventListener("InsecureLoginFormsStateChange", function() {
>        gIdentityHandler.refreshForInsecureLoginForms();
>      });
>  
> +    gBrowser.addEventListener("PermissionStateChange", function() {
> +      gIdentityHandler.refreshIdentityBlock();

I think we should only do this if the browser that caused the event is currently selected.
In practice this event is likely a consequence of user action on a permission prompt, so it's likely that the browser is indeed selected.

::: browser/base/content/browser.js:3173
(Diff revision 2)
>      // URL again.
>      gBrowser.loadURIWithFlags(url, reloadFlags);
>      return;
>    }
>  
> +  gBrowser.getTemporaryPermissionsMap(gBrowser.selectedBrowser).clear();

The _temporaryPermissions map is created lazily on the first call to getTemporaryPermissionsMap, so it's disappointing that you call getTemporaryPermissionsMap to clear it when it may not exist yet.

::: browser/base/content/browser.js
(Diff revision 2)
>  
> -    let permissions = SitePermissions.getPermissionDetailsByURI(uri);
> -    if (this._sharingState) {
> -      // If WebRTC device or screen permissions are in use, we need to find
> +    let permissions = SitePermissions
> +      .getPermissionDetailsForBrowser(gBrowser.selectedBrowser);
> +
> -      // the associated permission item to set the inUse field to true.
> -      for (let id of ["camera", "microphone", "screen"]) {

Removing this harcoded list is cleaner, but I have a feeling looping over the list of permissions instead of the list of active share will cause issues. Eg. is the patch saving a temporary allowed permission state for a screen share?

::: browser/base/content/tabbrowser.xml:1395
(Diff revision 2)
> +          let tab = this.getTabForBrowser(aBrowser);
> +          if (!tab)
> +            return;
> +
> +          if (!tab._temporaryPermissions) {
> +            tab._temporaryPermissions = new Map();

It's not obvious to me why this map is stored on the tab rather than on the browser. It seems to me that storing it on the browser would save us the browser -> tabbrowser -> tab dance for each caller.

::: browser/modules/SitePermissions.jsm:18
(Diff revision 2)
>  
>    UNKNOWN: Services.perms.UNKNOWN_ACTION,
>    ALLOW: Services.perms.ALLOW_ACTION,
>    BLOCK: Services.perms.DENY_ACTION,
> +  TEMP_ALLOW: 4,
> +  TEMP_BLOCK: 5,

ALLOW is 1, DENY is 2, SESSION is 8. How would you feel about making TEMP_ALLOW be ALLOW+SESSION = 9 and TEMP_DENY be DENY+SESSION = 10, so that we could let callers that only want to know if we allowed or blocked do 's & ALLOW' rather than 's == ALLOW || s == TEMP_ALLOW'?

::: browser/modules/SitePermissions.jsm:165
(Diff revision 2)
>      if (aState == this.UNKNOWN) {
> -      this.remove(aURI, aPermissionID);
> +      this.remove(aURI, aPermissionID, aBrowser);
> +      return;
> +    }
> +
> +    if (aState == this.SESSION) {

When is this case actually used? If it's only for webNotifications, I think it's fine to keep a hack.

::: browser/modules/webrtcUI.jsm:390
(Diff revision 2)
>          return false;
>  
>        // DENY_ACTION is handled immediately by MediaManager, but handling
>        // of ALLOW_ACTION is delayed until the popupshowing event
>        // to avoid granting permissions automatically to background tabs.
> -      if (aRequest.secure) {
> +      let micPerm = SitePermissions.get(uri, "microphone", this.browser);

Why is this if (aRequest.secure) test removed?

Comment 14

[Florian Quèze [:florian]]

Attachment: Patch v3

I addressed my own comments, tested/debugged, and got the webrtc tests to pass locally. I think this is ready for review (but needs more tests).

Comment 15

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=636c5502ce9a

Comment 16

[Florian Quèze [:florian]]

Answering my own review comments:

(In reply to Florian Quèze [:florian] [:flo] from comment #13)

> ::: browser/base/content/browser.js:1067

> > +    gBrowser.addEventListener("PermissionStateChange", function() {
> > +      gIdentityHandler.refreshIdentityBlock();
> 
> I think we should only do this if the browser that caused the event is
> currently selected.

Given that we already call refreshIdentityBlock for each perm-changed notification regardless of which tab caused it, it doesn't make sense to filter here; forget this comment.

> ::: browser/base/content/tabbrowser.xml:1395
> (Diff revision 2)
> > +          let tab = this.getTabForBrowser(aBrowser);
> > +          if (!tab)
> > +            return;
> > +
> > +          if (!tab._temporaryPermissions) {
> > +            tab._temporaryPermissions = new Map();
> 
> It's not obvious to me why this map is stored on the tab rather than on the
> browser. It seems to me that storing it on the browser would save us the
> browser -> tabbrowser -> tab dance for each caller.

Actually, storing it on the tab was fine, it's really the getTemporaryPermissionsMap abstraction that annoyed me.

Comment 17

[Johann Hofmann [:johannh]]

(In reply to Florian Quèze [:florian] [:flo] from comment #13)
> Comment on attachment 8794123 [details]
> Bug 1206232 - Add temporary permission states to SitePermissions.jsm.
> 
> https://reviewboard.mozilla.org/r/80706/#review80294
> 
> The approach seems reasonable.
> 
> ::: browser/base/content/browser.js:1067
> (Diff revision 2)
> >      gBrowser.addEventListener("InsecureLoginFormsStateChange", function() {
> >        gIdentityHandler.refreshForInsecureLoginForms();
> >      });
> >  
> > +    gBrowser.addEventListener("PermissionStateChange", function() {
> > +      gIdentityHandler.refreshIdentityBlock();
> 
> I think we should only do this if the browser that caused the event is
> currently selected.
> In practice this event is likely a consequence of user action on a
> permission prompt, so it's likely that the browser is indeed selected.
> 
> ::: browser/base/content/browser.js:3173
> (Diff revision 2)
> >      // URL again.
> >      gBrowser.loadURIWithFlags(url, reloadFlags);
> >      return;
> >    }
> >  
> > +  gBrowser.getTemporaryPermissionsMap(gBrowser.selectedBrowser).clear();
> 
> The _temporaryPermissions map is created lazily on the first call to
> getTemporaryPermissionsMap, so it's disappointing that you call
> getTemporaryPermissionsMap to clear it when it may not exist yet.

Sorry to disappoint, you're obviously right :P

> 
> ::: browser/base/content/browser.js
> (Diff revision 2)
> >  
> > -    let permissions = SitePermissions.getPermissionDetailsByURI(uri);
> > -    if (this._sharingState) {
> > -      // If WebRTC device or screen permissions are in use, we need to find
> > +    let permissions = SitePermissions
> > +      .getPermissionDetailsForBrowser(gBrowser.selectedBrowser);
> > +
> > -      // the associated permission item to set the inUse field to true.
> > -      for (let id of ["camera", "microphone", "screen"]) {
> 
> Removing this harcoded list is cleaner, but I have a feeling looping over
> the list of permissions instead of the list of active share will cause
> issues. Eg. is the patch saving a temporary allowed permission state for a
> screen share?

Mmh, did you check this?

> 
> ::: browser/base/content/tabbrowser.xml:1395
> (Diff revision 2)
> > +          let tab = this.getTabForBrowser(aBrowser);
> > +          if (!tab)
> > +            return;
> > +
> > +          if (!tab._temporaryPermissions) {
> > +            tab._temporaryPermissions = new Map();
> 
> It's not obvious to me why this map is stored on the tab rather than on the
> browser. It seems to me that storing it on the browser would save us the
> browser -> tabbrowser -> tab dance for each caller.
> 

I like your solution and I agree, getTemporaryPermissionsMap wasn't a great abstraction. It originated from me having methods like .addTemporaryPermission, .removeTemporaryPermission etc. in tabbrowser until I realized I'm just providing an abstraction to all JS Map methods and added getTemporaryPermissionsMap instead. But yeah, this is easier.

> ::: browser/modules/SitePermissions.jsm:18
> (Diff revision 2)
> >  
> >    UNKNOWN: Services.perms.UNKNOWN_ACTION,
> >    ALLOW: Services.perms.ALLOW_ACTION,
> >    BLOCK: Services.perms.DENY_ACTION,
> > +  TEMP_ALLOW: 4,
> > +  TEMP_BLOCK: 5,
> 
> ALLOW is 1, DENY is 2, SESSION is 8. How would you feel about making
> TEMP_ALLOW be ALLOW+SESSION = 9 and TEMP_DENY be DENY+SESSION = 10, so that
> we could let callers that only want to know if we allowed or blocked do 's &
> ALLOW' rather than 's == ALLOW || s == TEMP_ALLOW'?
> 

Well it makes things more concise but it's also an additional cognitive overhead reading the codebase and can make changes a bit harder to reason about. It's not really the style of code I usually write, but I really don't mind either. I assume the constants won't ever change, so there's no strong point either way.

> ::: browser/modules/SitePermissions.jsm:165
> (Diff revision 2)
> >      if (aState == this.UNKNOWN) {
> > -      this.remove(aURI, aPermissionID);
> > +      this.remove(aURI, aPermissionID, aBrowser);
> > +      return;
> > +    }
> > +
> > +    if (aState == this.SESSION) {
> 
> When is this case actually used? If it's only for webNotifications, I think
> it's fine to keep a hack.

The problem isn't really that I prefer a clean solution, the problem is that in this hack we set the ALLOW state to the perms database, with no way to differentiate between ALLOW and SESSION when querying it later. We should modify our query code to check for SESSION expiry and set the state to this.SESSION accordingly. Still a hack, but I agree it's fine.

> 
> ::: browser/modules/webrtcUI.jsm:390
> (Diff revision 2)
> >          return false;
> >  
> >        // DENY_ACTION is handled immediately by MediaManager, but handling
> >        // of ALLOW_ACTION is delayed until the popupshowing event
> >        // to avoid granting permissions automatically to background tabs.
> > -      if (aRequest.secure) {
> > +      let micPerm = SitePermissions.get(uri, "microphone", this.browser);
> 
> Why is this if (aRequest.secure) test removed?

Because I assumed we also want to TEMP_ALLOW or TEMP_BLOCK for WebRTC, which would have to work over HTTP as well. I notice that you have implemented only TEMP_BLOCK, are you sure you want to only do this for HTTPS pages? :)

Comment 18

[Johann Hofmann [:johannh]]

Comment on attachment 8796582 [details] [diff] [review]
Patch v3

Review of attachment 8796582 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/webrtcUI.jsm
@@ +312,5 @@
> +  // deny again without even shown the notification icon.
> +  if ((audioDevices.length &&
> +       SitePermissions.get(null, "microphone", aBrowser) == SitePermissions.TEMP_BLOCK) ||
> +      (videoDevices.length &&
> +       SitePermissions.get(null, sharingScreen ? "scren" : "camera", aBrowser) == SitePermissions.TEMP_BLOCK)) {

typo: screen

Comment 19

[Florian Quèze [:florian]]

(In reply to Johann Hofmann [:johannh] - partially unresponsive until 11/14 from comment #17)

> > ::: browser/base/content/browser.js
> > (Diff revision 2)
> > >  
> > > -    let permissions = SitePermissions.getPermissionDetailsByURI(uri);
> > > -    if (this._sharingState) {
> > > -      // If WebRTC device or screen permissions are in use, we need to find
> > > +    let permissions = SitePermissions
> > > +      .getPermissionDetailsForBrowser(gBrowser.selectedBrowser);
> > > +
> > > -      // the associated permission item to set the inUse field to true.
> > > -      for (let id of ["camera", "microphone", "screen"]) {
> > 
> > Removing this harcoded list is cleaner, but I have a feeling looping over
> > the list of permissions instead of the list of active share will cause
> > issues. Eg. is the patch saving a temporary allowed permission state for a
> > screen share?
> 
> Mmh, did you check this?

Yes, I did; the previous WIP broke the screen sharing indicator.

> > ::: browser/modules/SitePermissions.jsm:165
> > (Diff revision 2)
> > >      if (aState == this.UNKNOWN) {
> > > -      this.remove(aURI, aPermissionID);
> > > +      this.remove(aURI, aPermissionID, aBrowser);
> > > +      return;
> > > +    }
> > > +
> > > +    if (aState == this.SESSION) {
> > 
> > When is this case actually used? If it's only for webNotifications, I think
> > it's fine to keep a hack.
> 
> The problem isn't really that I prefer a clean solution, the problem is that
> in this hack we set the ALLOW state to the perms database, with no way to
> differentiate between ALLOW and SESSION when querying it later. We should
> modify our query code to check for SESSION expiry and set the state to
> this.SESSION accordingly. Still a hack, but I agree it's fine.

I don't think I understand the problem; but maybe it'll suddenly make sense when looking at the code again.

> > ::: browser/modules/webrtcUI.jsm:390
> > (Diff revision 2)
> > >          return false;
> > >  
> > >        // DENY_ACTION is handled immediately by MediaManager, but handling
> > >        // of ALLOW_ACTION is delayed until the popupshowing event
> > >        // to avoid granting permissions automatically to background tabs.
> > > -      if (aRequest.secure) {
> > > +      let micPerm = SitePermissions.get(uri, "microphone", this.browser);
> > 
> > Why is this if (aRequest.secure) test removed?
> 
> Because I assumed we also want to TEMP_ALLOW or TEMP_BLOCK for WebRTC, which
> would have to work over HTTP as well. I notice that you have implemented
> only TEMP_BLOCK, are you sure you want to only do this for HTTPS pages? :)

We don't want TEMP_ALLOW for WebRTC, or if we wanted it, it would be different (ie. the TEMP_ALLOW permission would be removed as soon as the stream is stopped), so if we want it I think it deserves a separate bug with a discussion about it with WebRTC platform people. We don't want this bug to be blocked by such a discussion, so I made my patch preserve the status quo.

I don't think my implementation of TEMP_BLOCK is restricted to https pages, but I'll double check.

Comment 20

[Florian Quèze [:florian]]

(In reply to Johann Hofmann [:johannh] from comment #18)

> ::: browser/modules/webrtcUI.jsm
> @@ +312,5 @@
> > +  // deny again without even shown the notification icon.
> > +  if ((audioDevices.length &&
> > +       SitePermissions.get(null, "microphone", aBrowser) == SitePermissions.TEMP_BLOCK) ||
> > +      (videoDevices.length &&
> > +       SitePermissions.get(null, sharingScreen ? "scren" : "camera", aBrowser) == SitePermissions.TEMP_BLOCK)) {
> 
> typo: screen

Thanks! The screensharing UI desperately needs tests.

Comment 21

[:Paolo Amadini]

I only took a very quick look at the patch for the moment.

It seems to me that TEMP_ALLOW and TEMP_BLOCK should really be called something lime ALLOW_IN_TAB and BLOCK_IN_TAB, because this is what they are doing.

About the choice of values, it's a bit confusing because we're kind of mixing flags and enumerated values. Apparently the SESSION value is specific to cookies, but it's a value we can theoretically find associated to any permission in the database, because the permission manager would store any arbitrary integer we tell it. Reusing this value for other purposes doesn't seem correct, and apparently ALLOW+SESSION actually maps to ACCESS_ALLOW_FIRST_PARTY_ONLY in the cookies interface:

https://dxr.mozilla.org/mozilla-central/rev/7c576fe3279d87543f0a03b844eba7bc215e17f1/netwerk/cookie/nsICookiePermission.idl#32-34

Considering that the new temporary states are only available through SitePermissions.jsm, in other words we don't store them in the database, we are free to define the interface we prefer to deal with them on the module. Since this is JavaScript, we're not limited to an integer and we could just return a more expressive object like { state: ALLOW, scope: THIS_TAB }.

We can't keep backwards compatibility anyways, even if we return a flags integer from JavaScript, because existing callers expect an enumerated value and you would need to update them anyways to recognize it as a flag, like you've actually done with all the callers in the patch.

Comment 22

[:Paolo Amadini]

Another improvements to SitePermissions could be that, instead of providing aBrowser as an argument to all the functions, you could create an object linked to the browser you needed using a syntax like SitePermissions.forBrowser(browser).method(...). But I'm fine with leaving this refactoring out, and maybe have a follow-up bug to do it in the future.

Comment 23

[:Paolo Amadini]

I think the current association of the state is simple enough, but I wonder if there is any better object we can associate the state with in the parent process, without having to copy the object around when we swap docshells?

Also, while not strictly required, I strongly recommend to refactor this code to use a WeakMap in SitePermissions rather than an expando property on the tab. This would also help to encapsulate the code better. For example, browser.js would have to call a method like resetTabPermissions() instead of doing it manually, resulting in better readability.

Comment 24

[Florian Quèze [:florian]]

Attachment: Patch v4

Addressed comment 18 and fixed the leak found by the try push.

Comment 25

[Florian Quèze [:florian]]

(In reply to :Paolo Amadini from comment #23)
> I think the current association of the state is simple enough, but I wonder
> if there is any better object we can associate the state with in the parent
> process, without having to copy the object around when we swap docshells?

I don't see any object we could use that wouldn't require an explicit action to update our data when there's a docshell swap.

> Also, while not strictly required, I strongly recommend to refactor this
> code to use a WeakMap in SitePermissions rather than an expando property on
> the tab. This would also help to encapsulate the code better. For example,
> browser.js would have to call a method like resetTabPermissions() instead of
> doing it manually, resulting in better readability.

One of the reason I chose to use an expando property on the tab is that this way we get the cleanup done automatically when a tab is closed or when a window is closed. If we store the information in SitePermissions instead, we will need explicit cleanup code for these cases, and I think it'll turn out to not be cleaner.

Another reason is that it's consistent with _sharingState, that's also stored on the tab.

If we want the temporarily blocked state to be preserved with the bfcache, I should move it to the browser object; but I don't think this really matters. We want prompts to be shown again after the user reloads the page, so I think it makes sense to prompt again after navigating back.

Comment 26

[:Paolo Amadini]

(In reply to Florian Quèze [:florian] [:flo] from comment #25)
> One of the reason I chose to use an expando property on the tab is that this
> way we get the cleanup done automatically when a tab is closed or when a
> window is closed. If we store the information in SitePermissions instead, we
> will need explicit cleanup code for these cases, and I think it'll turn out
> to not be cleaner.

Uh, that's not true, a WeakMap is effectively analogous to an expando property with regard to lifetime, but it is scoped so you need a reference to the WeakMap too. You don't need any cleanup code:

https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/WeakMap

Comment 27

[:Paolo Amadini]

Example usage:

https://dxr.mozilla.org/mozilla-central/rev/42c95d88aaaa7c2eca1d278399421d437441ac4d/toolkit/components/passwordmgr/LoginManagerParent.jsm#445-458

(The comment about not handling swapDocShells simply means that we didn't add the call you have in this patch to swap the references. It isn't an effect of using a WeakMap.)

(In reply to Florian Quèze [:florian] [:flo] from comment #25)
> I don't see any object we could use that wouldn't require an explicit action
> to update our data when there's a docshell swap.

By the way, I've been thinking for a while that we may want a generic module to hold these objects, but it's low priority and not strictly necessary so I didn't file a bug.

Comment 28

[Florian Quèze [:florian]]

(In reply to :Paolo Amadini from comment #26)
> (In reply to Florian Quèze [:florian] [:flo] from comment #25)
> > One of the reason I chose to use an expando property on the tab is that this
> > way we get the cleanup done automatically when a tab is closed or when a
> > window is closed. If we store the information in SitePermissions instead, we
> > will need explicit cleanup code for these cases, and I think it'll turn out
> > to not be cleaner.
> 
> Uh, that's not true, a WeakMap is effectively analogous to an expando
> property with regard to lifetime, but it is scoped so you need a reference
> to the WeakMap too. You don't need any cleanup code:
> 
> https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/
> Global_Objects/WeakMap

Ah, the first time I read that page I assumed the weak references meant the weakmap wouldn't prevent garbage collection of the object used as a key, but I didn't realize the values in the map also get GC'ed. Thanks for persisting! :-)

Comment 29

[Florian Quèze [:florian]]

Attachment: Patch v5

I like the WeakMap idea, but I think it would really make sense to do it for the _sharingState values at the same time (with a WeakMap in webrtcUI.jsm), and the patch is already big enough, so I would prefer doing it either as another patch here, or more likely in a separate bug.

I still have to modify the browser_PermissionUI.js test to take into account the change of API and cover the new features, but I would prefer us to be in agreement on the API before I work on getting it covered by tests.

Comment 30

[:Paolo Amadini]

Comment on attachment 8798610 [details] [diff] [review]
Patch v5

Review of attachment 8798610 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +4903,5 @@
>      }
>  
> +    // We clear temporary tab permissions on navigation only if the
> +    // hostname is different.
> +    let host = aLocationURI.asciiHost;

At least this should check the full origin (host and port) like the permission manager does. The logic there is actually more complex, I'm not sure how it applies here:

https://dxr.mozilla.org/mozilla-central/rev/da986c9f1f723af1e0c44f4ccd4cddd5fb6084e8/extensions/cookie/nsPermissionManager.cpp#106-131

The attack scenario could be to have the user grant a permission to a trustworthy site, then make them navigate to another site and having the permission propagated incorrectly.

We'll need a specific test to ensure this does not happen, at least for the different port case.

I'm not sure we need all the rest of the logic because I guess that origin attributes won't change in same-tab navigation, but I'll ask someone from platform security for a code review to ensure that we're not propagating permissions when we shouldn't.

@@ +7087,5 @@
>  
>      // show permission icons
> +    let permissions =
> +      SitePermissions.getPermissionDetailsForTab(gBrowser.selectedTab);
> +    for (let permission of permissions) {

The difference between getAllByURI and getPermissionDetailsByURI is that the former does not fetch the labels, giving us the performance improvement we need when handling the default UI. We should probably keep this distinction in some way to avoid Talos regressions.

@@ +7519,5 @@
> +      state = SitePermissions.ALLOW;
> +      scope = SitePermissions.REQUEST;
> +    }
> +    stateLabel.textContent =
> +      SitePermissions.getStateLabel(aPermission.id, state, scope);

Looks like what the UI needs is an API like:

stateLabel.textContent = getStateLabel(aPermission);

Or even better, have aPermission.stateLabel already populated by the function that gets the permission details.

::: browser/base/content/tabbrowser.xml
@@ +2740,5 @@
>                webrtcUI.swapBrowserForNotification(otherBrowser, ourBrowser);
>              }
>  
> +            if (aOtherTab._temporaryPermissions)
> +              aOurTab._temporaryPermissions = aOtherTab._temporaryPermissions;

By not propagating _temporaryPermissionsHost, the permissions will be lost on the next navigation after the tab swap.

I actually recommend to define _temporaryPermissions as an object, and have two properties on it like { map, origin }.

::: browser/modules/SitePermissions.jsm
@@ +19,5 @@
> +  // Scope of the permission
> +  REQUEST: 0,
> +  TAB: 1,
> +  BROWSER_SESSION: 2,
> +  PERSISTENT: 3,

Hm, I've seen you depend on the numeric values using a greater than comparison. I'm not too fond of this approach, but if used it should be documented here at least. This also means that if you add another scope that is logically in-between (WINDOW?) you need to add fractional values or change the values.

In some other modules we use strings instead of numbers for enumerated values.

@@ +187,5 @@
> +    // Save temporary permissions on the tab.
> +    if (!aBrowser)
> +      return;
> +
> +    let tab = this._getTabForBrowser(aBrowser);

nit: Since you're handling the null case inside _getTabForBrowser, you don't need the null check before it.

Comment 31

[:Paolo Amadini]

Comment on attachment 8798610 [details] [diff] [review]
Patch v5

Tanvi, can you take a look at the security question from comment 30?

I'm not sure if a host+port check for the top-level window is enough or we need to check origin attributes as well. Also, I've not really thought about how this affects permissions granted to sub-frames, maybe Florian can clarify here.

Comment 32

[Florian Quèze [:florian]]

Attachment: Patch v6

(In reply to :Paolo Amadini from comment #30)

Thanks for the review.

> ::: browser/base/content/browser.js
> @@ +4903,5 @@
> >      }
> >
> > +    // We clear temporary tab permissions on navigation only if the
> > +    // hostname is different.
> > +    let host = aLocationURI.asciiHost;
>
> At least this should check the full origin (host and port)

URI.prePath seems to contain what we want.


> @@ +7087,5 @@
> >
> >      // show permission icons
> > +    let permissions =
> > +      SitePermissions.getPermissionDetailsForTab(gBrowser.selectedTab);
> > +    for (let permission of permissions) {
>
> The difference between getAllByURI and getPermissionDetailsByURI is that the
> former does not fetch the labels, giving us the performance improvement we
> need when handling the default UI. We should probably keep this distinction
> in some way to avoid Talos regressions.

I kept the distinction like you suggested... but this whole thing feels like a hack. I think a more correct solution would be to create a prototype shared for all permission items, with getters for the localized strings. For each permission item we return, we could create an instance of this object, and all the operations related to the strings or the states would be done lazily.

> @@ +7519,5 @@
> > +      state = SitePermissions.ALLOW;
> > +      scope = SitePermissions.REQUEST;
> > +    }
> > +    stateLabel.textContent =
> > +      SitePermissions.getStateLabel(aPermission.id, state, scope);
>
> Looks like what the UI needs is an API like:
>
> stateLabel.textContent = getStateLabel(aPermission);
>
> Or even better, have aPermission.stateLabel already populated by the
> function that gets the permission details.

I couldn't find an easy way to do this. What makes it difficult is that the browser.js code messes with the behavior due to the .inUse case.
I think this could also be fixed cleanly with the change I suggested above (as the browser.js code could set the .inUse flag on the object, and the stateLabel getter could take it into account, as it would be called later).

> ::: browser/modules/SitePermissions.jsm
> @@ +19,5 @@
> > +  // Scope of the permission
> > +  REQUEST: 0,
> > +  TAB: 1,
> > +  BROWSER_SESSION: 2,
> > +  PERSISTENT: 3,
>
> Hm, I've seen you depend on the numeric values using a greater than
> comparison. I'm not too fond of this approach, but if used it should be
> documented here at least. This also means that if you add another scope that
> is logically in-between (WINDOW?) you need to add fractional values or
> change the values.

We define these as constants, so callers shouldn't hardcode the values. It's fine to change all the values when adding one in the middle.

Comment 33

[Florian Quèze [:florian]]

(In reply to :Paolo Amadini from comment #31)
> Comment on attachment 8798610 [details] [diff] [review]
> Patch v5
> 
> Tanvi, can you take a look at the security question from comment 30?
> 
> I'm not sure if a host+port check for the top-level window is enough or we
> need to check origin attributes as well. Also, I've not really thought about
> how this affects permissions granted to sub-frames, maybe Florian can
> clarify here.

I think this was meant to be a needinfo.

Comment 34

[:Paolo Amadini]

(In reply to Florian Quèze [:florian] [:flo] from comment #33)
> I think this was meant to be a needinfo.

The code is also relevant, for example if a prePath check is enough and if we should use the principal or the location.

Comment 35

[Tanvi Vyas[:tanvi]]

(In reply to :Paolo Amadini from comment #31)
> Comment on attachment 8798610 [details] [diff] [review]
> Patch v5
> 
> Tanvi, can you take a look at the security question from comment 30?
> 
> I'm not sure if a host+port check for the top-level window is enough or we
> need to check origin attributes as well. Also, I've not really thought about
> how this affects permissions granted to sub-frames, maybe Florian can
> clarify here.

Some Origin Attributes are important to check when determining whether a site has a permission.  

For example, consider the first party domain Origin Attribute.
* User visits https://siteA.com.  siteA.com asks for geolocation permission.
* The permission should be granted for {scheme: https, host:siteA.com, port: 443, firstPartyDomain=siteA.com}. 
* Now assume the user navigates that same tab to https://siteB.com.  
* https://siteB.com has https://siteA.com as a subresource.
* The subresource https://siteA.com does not have the permission, since the origin for the subresource is now {scheme: https, host:siteA.com, port: 443, firstPartyDomain=siteB.com}.  This is not a match for the permission granted when https://siteA.com was a first party.

For Containers on the other hand (at least for now), we do not consider userContextId when granting a permission.  If a permission is granted to a site in the Work context, the same site will have the permission in the default and Personal context.

Private Browsing Mode is also moving to an Origin Attribute.  I don't remember exactly how permissions are carried over from normal to private mode (or how they aren't), but I'm sure that Origin Attribute is also handled a little differently.

In the future, there may be other Origin Attributes.  But the permissionsManager should properly handle which Origin Attributes require separation and which don't.  Maybe you aren't using the permissionsManager for temporary permissions though?

You mentioned something about "same-tab".  Once in a tab, the userContextId and the PrivateMode OriginAttribute shouldn't be able to change.  But the firstPartyDomain OriginAttribute certainly can.  And for future Origin Attributes, we'll have to just wait and see.

I'm not sure I've answered the question here, so please feel free to needinfo me again with more information.  Or we can also discuss over vidyo if that is easier.

Thanks!

Comment 36

[Florian Quèze [:florian]]

We discussed this during the meeting today. Here are the main takeaways:

- the temporarily allowed permissions introduced here for a whole tab are scary, because they are easy to abuse by frames (eg. site A has frames B and C. When frame B gets permission from the user to do something, if we store it on the whole tab, then A and C also get permission).

- given that the main user problem we are trying to solve here is permission spamming, we should reduce the scope of this bug and cover only the Block Temporarily case.

- The way this is currently implemented is easy to abuse because a site A could navigate to a domain B it controls that would redirect back to A, thus clearing the temporarily blocked permission on the tab, and letting the site prompt the user again. To remedy this, we propose to no longer clear the temporary permission on navigation, but instead to associate a timestamp with each blocked permission we store. When checking for these permissions, if the stored timestamp is more than a couple minutes old, we'll drop that temporary permission and just prompt the user. An added benefit is that if a site requests something and the user says no, it's legitimate for the site to be able to ask again an hour later.

- The current usage of prePath is probably fine now, but not future proof, so we should use the principal instead.

Comment 37

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #36)

> - The way this is currently implemented is easy to abuse because a site A
> could navigate to a domain B it controls that would redirect back to A, thus
> clearing the temporarily blocked permission on the tab, and letting the site
> prompt the user again. To remedy this, we propose to no longer clear the
> temporary permission on navigation, but instead to associate a timestamp
> with each blocked permission we store. When checking for these permissions,
> if the stored timestamp is more than a couple minutes old, we'll drop that
> temporary permission and just prompt the user.

During the meeting today we decided to make this time 1 hour.

It has been suggested (not sure if we decided on these points) :
- that we may put the time in an about:config pref so that it could be changed with a hotfix.
- that for easier discoverability, we may want to animate the 'blocked' icon in the urlbar (like we do for the download icon) when a new request has been blocked automatically, if the request has been started from a user-action on the site.

Comment 38

[:Paolo Amadini]

(In reply to Florian Quèze [:florian] [:flo] from comment #37)
> - that for easier discoverability, we may want to animate the 'blocked' icon
> in the urlbar (like we do for the download icon) when a new request has been
> blocked automatically, if the request has been started from a user-action on
> the site.

I really liked Adrian's suggestion, we could file a separate to implement it.

Comment 39

[Florian Quèze [:florian]]

Attachment: Patch v7

This updated version removes the bitrot and addresses what we discussed in bug comments and in meetings a while ago.

What's left to do:
- cleanup the handling of SitePermissions.SESSION (currently the test_SitePermissions.js xpcshell test fails due to this)
- add tests covering tab permissions specifically.
- At this point I expect this will land after the merge of elm to central, so there'll be more unbitrotting to do.

Given the 3 above points, I'm not expecting a final r+ yet, but I'm seeking a bit more than just feedback on the patch.

Comment 40

[:Paolo Amadini]

Comment on attachment 8810939 [details] [diff] [review]
Patch v7

Review of attachment 8810939 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should set expectations right: this is a major piece of work that will likely take most of release 53 to review and complete, and will define valuable infrastructure and behavior that I don't see going away for many years, just like nsIPermissionManager is still with us today.

This is also security-sensitive so we should take extra care to ensure that we handle all the cases that we can think of, and we must have tests in place to ensure that future changes don't introduce security bugs accidentally. I think writing most of the tests is the next step, and this will highlight possible bugs in the implementation much better than pure code inspection.

A good way of knowing which tests are missing is to use previous review comments on this bug. I'm also reviewing this patch with tests in mind.

And as you've noted, we need general tests for new functionality, for example the timed expiration for tab permissions. We also need tests for the subframes case, in other words multiple permissions granted for different origins on the same tab. Note that these can be granted or denied through popup notifications, but cannot be reverted from the Control Center, since the latter doesn't display subframe permissions yet.

This review is mostly about the SitePermissions API, and less about the WebRTC and front-end changes, although I also looked at them.

::: browser/base/content/browser.js
@@ +3164,5 @@
>      gBrowser.loadURIWithFlags(url, reloadFlags);
>      return;
>    }
>  
> +  gBrowser.selectedTab._temporaryPermissions = null;

Add a detailed comment specifying why this is done just here, and not implemented for example inside the browser object itself when the location changes.

Tests should cover both reloads triggered from UI, and reloads triggered by other means, and check that they have different behavior.

@@ +4611,5 @@
>      if (this._state == aState &&
> +        this._lastLocation == spec) {
> +      // Switching to a tab of the same URL doesn't change most security
> +      // information, but tab specific permissions may be different.
> +      gIdentityHandler.refreshIdentityBlock();

Ensure we have a test for this case.

@@ +7468,5 @@
>      let stateLabel = document.createElement("label");
>      stateLabel.setAttribute("flex", "1");
>      stateLabel.setAttribute("class", "identity-popup-permission-state-label");
> +    let {state, scope} = aPermission;
> +    if (!(state == SitePermissions.ALLOW) && aPermission.inUse) {

!= :-)

;-)

@@ +7471,5 @@
> +    let {state, scope} = aPermission;
> +    if (!(state == SitePermissions.ALLOW) && aPermission.inUse) {
> +      state = SitePermissions.ALLOW;
> +      scope = SitePermissions.REQUEST;
> +    }

...anyways, add a comment on why this is needed.

::: browser/modules/PermissionUI.jsm
@@ +269,5 @@
>      if (this.permissionKey) {
>        // If we're reading and setting permissions, then we need
>        // to check to see if we already have a permission setting
>        // for this particular principal.
> +      let {state} = SitePermissions.get(this.principal.URI,

I don't think all principals have an URI... we should ensure that we still handle those that don't correctly.

@@ +291,5 @@
>        // Don't offer action in PB mode if the action remembers permission
>        // for more than a session.
>        if (PrivateBrowsingUtils.isWindowPrivate(chromeWin) &&
> +          promptAction.action &&
> +          (!("scope" in promptAction) || promptAction.scope == SitePermissions.PERSISTENT)) {

In general, use "!object.property" instead of "property in object" to consider the case where the property is present but its value is undefined. This doesn't cause any console warnings anymore. This may happen for example when propagating parameters in object literals.

Anyways, this code will likely change after the rebase.

::: browser/modules/SitePermissions.jsm
@@ +9,5 @@
>  var gStringBundle =
>    Services.strings.createBundle("chrome://browser/locale/sitePermissions.properties");
>  
>  this.SitePermissions = {
> +  TAB_PERMISSIONS_VALIDITY: 3600 * 1000, // 1h in ms

nit: TAB_PERMISSIONS_EXPIRE_TIME_MS

@@ +21,5 @@
> +  // higher the value, the longer the permission will last.
> +  REQUEST: 0,
> +  TAB: 1,
> +  BROWSER_SESSION: 2,
> +  PERSISTENT: 3,

I don't think the comment exactly describes what we can expect from this progression, but while thinking about this I envisioned a test case that we can run. In pseudo-code:

  for (combinations of combinatoryTestCase) {
    SitePermission.set(uri, browser, scope, allowOrBlock);
    SitePermission.set(uri, browser, scope2, allowOrBlock2);
    if (maybe) {
      SitePermission.remove(uri, browser, scope2);
    }
    result = SitePermission.get(uri, browser, scope);
  }

Separately for the cases where "scope > scope2" and "scope < scope2", test that "result" is the same if all the other variables are the same and we only change "scope" and "scope2", ranging from "tab" to "persistent".

Here is an example of what one iteration of this test would do:

Set a "blocked for tab" permission and an "allowed for session" permission, clear the permission for the tab, and check the permission for the tab. Note whether the result is allow or deny.

Then set a "blocked for session" permission and an "allowed persistently" permission, clear the permission for the session, and check the persistent permission. Check whether the result is the same as the previous one.

I think writing a test loop that does this is valuable, because it would highlight anything that we special-case or don't handle correctly. (We may already have inconsistencies due to the nsPermissionManager::AddInternal implementation.)

@@ +254,4 @@
>     */
> +  remove: function (aURI, aPermissionID, aBrowser) {
> +    if (this.isSupportedURI(aURI))
> +      Services.perms.remove(aURI, aPermissionID);

The removal here is always unscoped, meaning that aBrowser is substantially ignored, and persistent permissions as well as tab permissions are reset. I guess this is what we need because of how the user interface works today.

Also, this seems to work on the assumption that when there is a persistent permission for an URI it always wins over a previously set tab permission for that URI, even if the permission internally is still in the tab data structure. This seems correct to me.

In other words, permissions with greater scope override permissions with lesser scope, and not the other way around.

@@ +258,5 @@
> +
> +    let tab = this._getTabForBrowser(aBrowser);
> +    if (!tab)
> +      return;
> +    let prePath = tab.linkedBrowser.currentURI.prePath;

If I remember correctly, it turned out that using prePath is wrong. We need to add tests checking that URIs with embedded usernames and passwords behave like same origin for the purposes of this code, even if the credentials are different.

Comment 41

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80706/diff/2-3/

Comment 42

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80706/diff/3-4/

Comment 43

[Johann Hofmann [:johannh]]

Very WIP, this adds a couple of tests and starts to deal with SESSION hairyness.

Comment 44

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80706/diff/4-5/

Comment 45

[Johann Hofmann [:johannh]]

jdm, can you review the changes in nsPermissionManager.cpp and nsIPermissionManager.idl? Thanks! :)

Comment 46

[Josh Matthews [:jdm]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

https://reviewboard.mozilla.org/r/80706/#review96028

The changes to nsIPermissionManager.idl and nsPermissionManager.cpp look fine to me.

::: extensions/cookie/nsPermissionManager.cpp:2012
(Diff revision 5)
>  
>  NS_IMETHODIMP
> +nsPermissionManager::GetPermissionObjectForURI(nsIURI* aURI,
> +                                         const char* aType,
> +                                         bool aExactHostMatch,
> +                                         nsIPermission** aResult)

nit: indentation.

Comment 47

[Johann Hofmann [:johannh]]

Paolo, this should cover all of the things that were left open.

Regarding the preference name, I just made something up and I'm happy to take suggestions.

Feel free to suggest additional test cases if you can come up with more :)

As discussed on IRC, we might want to change the key from prePath to be closer to the nsPermissionManager implementation. Though I don't think it would make a big difference. Anyways, you can check out the currently matched paths in the test case.

Thanks!

Comment 48

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80706/diff/5-6/

Comment 49

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17aeb6f2d938

Comment 50

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80706/diff/6-7/

Comment 51

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23f724ee87af

Comment 52

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80706/diff/7-8/

Comment 53

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=433db65b4f47

Comment 54

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80706/diff/8-9/

Comment 55

[:Paolo Amadini]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Clearing review while waiting for some updates we discussed recently.

Comment 56

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80706/diff/9-10/

Comment 57

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8560d86136cf

Comment 58

[Johann Hofmann [:johannh]]

Paolo, this hopefully implements the things we talked about. I cleaned up the confusion between (cookies) SESSION and BROWSER_SESSION (some of it was already there prior to this patch) and added a helper object to SitePermissions that deals with tab permissions. Let me know what you think.

I realize this will probably not get an r+ this time around but I wanted to request more than feedback. All tests I ran locally are passing (I'll fix the likely failures from try asap). Please let me know if you can think of any other tests/test cases to implement.

Thanks again for being so thorough :)

Comment 59

[:Paolo Amadini]

(In reply to Johann Hofmann [:johannh] from comment #58)
> I realize this will probably not get an r+ this time around but I wanted to
> request more than feedback. All tests I ran locally are passing (I'll fix
> the likely failures from try asap). Please let me know if you can think of
> any other tests/test cases to implement.

Thanks for the updated patch! I'll review this version in multiple parts so you can see the review comments earlier.

One test case I can think of right away is for the propagation of temporary permissions when a tab is moved to a new window, similarly to what browser_popupNotification_4.js does with gBrowser.replaceTabWithWindow.

Comment 60

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80706/diff/10-11/

Comment 61

[Johann Hofmann [:johannh]]

Thanks! I fixed the try failure and added a new test for the window moving case.

Comment 62

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9d85d01e404

Comment 63

[:Paolo Amadini]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

https://reviewboard.mozilla.org/r/80706/#review103312

Here is the first part of the review. I've not reviewed SitePermissions.jsm itself and everything after it yet.

::: browser/base/content/browser.js:7141
(Diff revision 11)
> -      } else if (permission.state === SitePermissions.ALLOW ||
> -                 permission.state === SitePermissions.SESSION) {
> +      } else if (permission.state == SitePermissions.ALLOW ||
> +                 permission.state == SitePermissions.SESSION) {

A better check is permission.state != SitePermissions.UNKNOWN, considering that in theory any integer value can be stored as a permission state.

::: browser/base/content/browser.js:7590
(Diff revision 11)
> -                  SitePermissions.get(uri, id) == SitePermissions.ALLOW)
> +                let perm = SitePermissions.get(uri, id);
> +                if (perm.state == SitePermissions.ALLOW &&
> +                    perm.scope == SitePermissions.PERSISTENT)
> -                SitePermissions.remove(uri, id);
> +                  SitePermissions.remove(uri, id);
> -            }
> +              }

This potentially doesn't clear session permissions in subframes anymore, but it may still be correct, I don't know enough about this special logic for subframes in WebRTC code. Probably you or Florian can comment on whether it's correct.

nit: brace single-line clauses.

::: browser/base/content/browser.js:7598
(Diff revision 11)
> +        let mm = browser.messageManager;
>          mm.sendAsyncMessage("webrtc:StopSharing", windowId);

nit: you can inline the variable.

::: browser/base/content/test/general/browser_temporary_permissions.js:10
(Diff revision 11)
> +
> +Cu.import("resource:///modules/SitePermissions.jsm", this);
> +
> +// Test that temporary permissions are removed on user initiated reload only.
> +add_task(function* testTempPermissionOnReload() {
> +  let uri = Services.io.newURI("https://example.com", null, null);

nit: I believe NetUtil.newURI should be available.

::: browser/base/content/test/general/browser_temporary_permissions.js:14
(Diff revision 11)
> +  let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, uri.spec);
> +  let browser = tab.linkedBrowser;

nit: BrowserTestUtils.withNewTab is also available.

::: browser/base/content/test/general/browser_temporary_permissions.js:30
(Diff revision 11)
> +  yield BrowserTestUtils.waitForCondition(() => {
> +    return reloadButton.disabled == false;
> +  });

Is there a better way to wait for the correct state here rather than observing the button? If not, use waitForAttribute to avoid polling if possible.

::: browser/base/content/test/webrtc/browser_devices_get_user_media.js:127
(Diff revision 11)
> +    let {state, scope} = SitePermissions.get(null, "camera", browser);
> +    is(state, SitePermissions.BLOCK, "camera requests are blocked");
> +    is(scope, SitePermissions.TAB, "the block is only temporary");

nit: I find the Assert.deepEqual style more readable, and I don't see a need for the descriptive strings.

::: browser/modules/PermissionUI.jsm:219
(Diff revision 11)
>     *
>     *  label (string):
>     *    The label that will be displayed for this choice.
>     *  accessKey (string):
>     *    The access key character that will be used for this choice.
> +   *  TODO: UPDATE THIS

...update this :-)

::: browser/modules/PermissionUI.jsm:314
(Diff revision 11)
> +            } else if (remember) {
> +              // Permanently store permission.
> +              SitePermissions.set(this.principal.URI,
> +                                  this.permissionKey,
> +                                  promptAction.action);

Worth calling out that with this change we're now storing permanent allow permissions in Private Browsing Mode if the user opts in. This stores a permanent indication that the user visited the site.

Even if this is intentional, maybe we should file a different bug instead of burying this change in a big patch?

I also don't see why the same logic shouldn't apply to a permanent block if the checkbox is selected.

Comment 64

[:Paolo Amadini]

Speaking of missing tests, we should test that we can set tab permissions for the block case using a popup notification invoked by a subframe of a different origin. This should limit the permission for the subframe, but should not affect the ability to ask for the permission from the top level document, because the origins are different.

We also need to test the multiple navigation case. Set a tab permission, navigate forward to a different origin, then navigate forward to the previous origin and check that the permission is still enforced. This is to make sure future implementations don't just reset the permissions when navigating to a different URI, which would invalidate our design choice of keying by URI instead and keeping an expiration time.

Do we have a test for verifying that tab permissions don't affect other tabs for the same origin that are open at the same time?

Comment 65

[:Paolo Amadini]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

https://reviewboard.mozilla.org/r/80706/#review103384

With the changes below, there's enough that I feel I can clear the review request. I haven't looked at the existing tests in detail yet, I may still do that on the current version or wait for the next one.

::: browser/modules/SitePermissions.jsm:12
(Diff revision 11)
> +/*
> + * A helper module to manage temporary tab permissions.
> + */
> +var TabPermissions = {

nit: "/**" style comment, and maybe "const"?

::: browser/modules/SitePermissions.jsm:16
(Diff revision 11)
>  
> +/*
> + * A helper module to manage temporary tab permissions.
> + */
> +var TabPermissions = {
> +  _tabs: new WeakMap(),

I think we really need a major change to this for sanity. In this version, some methods want a browser object, others want a tab object. The internal data structures use the tab object, and we use linkedBrowser or getTabForBrowser a lot to convert between them.

We should always key by browser instead, this module should never see the tab object. The browser is the element that's closer to the platform and it's linked to the content process by the Message Manager. The tab is just a front-end concept used to display the tab bar.

TabPermissions is fine as a module name, but methods or variables that take a browser should be renamed. This map could be named _stateByBrowser, to follow the convention used here:

https://dxr.mozilla.org/mozilla-central/rev/b548da4e16f067e5b69349376e37b2db97983cf7/toolkit/components/passwordmgr/LoginManagerParent.jsm#450

We also need a JSDoc-style comment that describes the data structure.

::: browser/modules/SitePermissions.jsm:18
(Diff revision 11)
> + * A helper module to manage temporary tab permissions.
> + */
> +var TabPermissions = {
> +  _tabs: new WeakMap(),
> +
> +  set(aTab, aURI, aId, aState) {

Don't use the "a" prefix for arguments in new code.

I also think the API will be much less error-prone if all methods took a single object argument, for example here "set({ browser, uri, id, state })".

This would really benefit the main SitePermissions API as well, although I understand that you may want to keep backwards compatibility to some extent. You may still consider doing this for some methods that are used only by the user interface, like getStateLabel.

::: browser/modules/SitePermissions.jsm:64
(Diff revision 11)
> +  },
> +
> +  getAllForTab(aTab) {
> +    let permissions = [];
> +    let tabEntry = this._tabs.get(aTab);
> +    let prePath = aTab.linkedBrowser.currentURI.prePath;

You should likely have an URI argument here like the other methods.

::: browser/modules/SitePermissions.jsm:74
(Diff revision 11)
> +  clearTab(aTab) {
> +    this._tabs.delete(aTab);
> +  },
> +
> +  copyTab(aTab, aNewTab) {

nit: Maybe just "clear" and "copy".

::: browser/modules/SitePermissions.jsm:83
(Diff revision 11)
> +  copyTab(aTab, aNewTab) {
> +    let tabEntry = this._tabs.get(aTab);
> +    if (tabEntry) {
> +      this._tabs.set(aNewTab, tabEntry);
> +    }
> +  }

nit: comma at the end of multi-line initializers

::: browser/modules/SitePermissions.jsm:92
(Diff revision 11)
> +  TAB_PERMISSIONS_EXPIRE_TIME_MS: Services.prefs.getIntPref("privacy.tab_permission_expire_time_ms", 3600 * 1000),
>  
>    UNKNOWN: Services.perms.UNKNOWN_ACTION,
>    ALLOW: Services.perms.ALLOW_ACTION,
>    BLOCK: Services.perms.DENY_ACTION,
>    SESSION: Components.interfaces.nsICookiePermission.ACCESS_SESSION,

Rename this to ALLOW_COOKIES_FOR_SESSION for clarity.

I suggest adding a check to "set" methods that would only accept ALLOW and BLOCK as valid, unless this is the cookie permission.

::: browser/modules/SitePermissions.jsm:94
(Diff revision 11)
> +  // Scope of the permission. These integer values are ordered so that the
> +  // higher the value, the longer the permission will last.
> +  REQUEST: 0,
> +  TAB: 1,
> +  BROWSER_SESSION: 2,
> +  PERSISTENT: 3,

Make the value of these constants a unique string, see the Downloads.jsm example again. This makes them readable in test output, and unable to be used accidentally in a { state: SitePermissions.PERSISTENT } manner.

Optionally, we could make the names look like SCOPE_REQUEST, which is useful to remove ambiguity, since we don't have strong typing in JavaScript. Alternatively, still optionally, we could add the type check in the methods, like Downloads.jsm does:

https://dxr.mozilla.org/mozilla-central/rev/b548da4e16f067e5b69349376e37b2db97983cf7/toolkit/components/jsdownloads/src/Downloads.jsm#234

This is all to make life easier for programmers coding with this API in the future.

::: browser/modules/SitePermissions.jsm:370
(Diff revision 11)
>    },
>  
>    /* Returns the localized label for the given permission state, to be used in
>     * a UI for managing permissions.
>     */
> -  getStateLabel(aPermissionID, aState, aInUse = false) {
> +  getStateLabel(aPermissionID, aState, aScope) {

Looks like { state, scope } is enough, as I suggested above.

::: browser/modules/SitePermissions.jsm:375
(Diff revision 11)
>        case this.UNKNOWN:
> -        if (aInUse)
> -          return gStringBundle.GetStringFromName("allowTemporarily");
>          return gStringBundle.GetStringFromName("alwaysAsk");
>        case this.ALLOW:
> +        if (typeof aScope == "number" && aScope < this.PERSISTENT)

scope != this.PERSISTENT

Comment 66

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #65)
> Make the value of these constants a unique string, see the Downloads.jsm
> example again.

This was meant to point to:

https://dxr.mozilla.org/mozilla-central/rev/b548da4e16f067e5b69349376e37b2db97983cf7/toolkit/components/jsdownloads/src/Downloads.jsm#52

Comment 67

[:Paolo Amadini]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

https://reviewboard.mozilla.org/r/80706/#review103830

As I said, adding temporary permissions isn't an easy task, and the more I look at this patch and the more I notice things that we aren't testing or potential pitfalls with the API.

This is normal for a patch of this size and impact. I understand that it isn't ideal to see these issues show up gradually, I would have preferred to be able to notice them all in advance a month ago, but reviewing isn't an easy task as well.

If adding all tests is too much work we can talk about prioritizing the most important first, on the other hand the tests help with the review as they increase the confidence that the code we're landing is correct.

::: browser/base/content/browser.js:1149
(Diff revision 11)
> +    gBrowser.addEventListener("PermissionStateChange", function() {
> +      gIdentityHandler.refreshIdentityBlock();
> +    });

Do we have tests that rely on the PermissionStateChange event already? For example, these would check the identity block after a tab permission is set. Maybe browser_PermissionUI.jsm does this already, if not we need to add a test.

Separately, we may need a simple new test in browser_PermissionUI.jsm where we use the checkbox to set a permanent permission, and see the difference compared to a tab permission.

::: browser/modules/SitePermissions.jsm:12
(Diff revision 11)
> +/*
> + * A helper module to manage temporary tab permissions.
> + */
> +var TabPermissions = {

The TabPermission object assumes that the URIs passed to its methods are supported by the permission manager. These happen to be URIs for which the prePath property is present and doesn't throw.

We need to clarify this in a comment. I suggest using the comment for the entire object, because most of the methods take similar arguments and there would be some redundancy if we put a comment on each method.

::: browser/modules/SitePermissions.jsm:87
(Diff revision 11)
> +    }
> +  }
> +};
> +
>  this.SitePermissions = {
> +  TAB_PERMISSIONS_EXPIRE_TIME_MS: Services.prefs.getIntPref("privacy.tab_permission_expire_time_ms", 3600 * 1000),

nit: I've the impression this could become a lazy getter. Also, the uppercase naming convention likely isn't needed now that this is read from a preference.

::: browser/modules/SitePermissions.jsm:125
(Diff revision 11)
>        if (gPermissionObject[permission.type]) {
>          // XXX Bug 1303108 - Control Center should only show non-default permissions
>          if (permission.type == "install") {
>            continue;
>          }
> +

nit: unnecessary whitepace change.

::: browser/modules/SitePermissions.jsm:249
(Diff revision 11)
> +
> +  /* Returns the state and scope of a particular permission for a given URI.
> +   */
> +  get(aURI, aPermissionID, aBrowser) {
> +    let result = { state: this.UNKNOWN, scope: this.PERSISTENT };
> +    if (this.isSupportedURI(aURI)) {

Actually, we don't support setting tab permissions on URIs that are not supported for session or permanent permissions, so you can return early from most functions if isSupportedURI is false.

I'm a bit undecided on whether to support passing null as the URI to various methods, meaning that the URI will be taken from the current URI of the browser. I lean towards not allowing it. Requiring the URI makes it more difficult to write code that allows attacks based on the browser navigating between the time a permission request is shown and the time it's written to the tab.

::: browser/modules/SitePermissions.jsm:274
(Diff revision 11)
> +      let tab = this._getTabForBrowser(aBrowser);
> +      if (!tab)
> +        return result;
> +
> +      let uri = tab.linkedBrowser.currentURI;
> +      let value = TabPermissions.get(tab, uri, aPermissionID);

Worth specifying in the comment that, even if an expired permission is deleted, when this happens while calling the "get" method then the "PermissionStateChange" event isn't fired.

::: browser/modules/SitePermissions.jsm:285
(Diff revision 11)
> -  /* Sets the state of a particular permission for a given URI.
> +  /* Sets the state of a particular permission for a given URI and/or browser.
> +   * Providing a value for aBrowser only sets the permission for the tab.

I guess all the comments of SitePermissions methods are still intentionally not updated, since we're still defining the exact API.

Anyways, we'll need to update the comments to JSDoc-style because there is now enough complexity to make those quite useful.

::: browser/modules/SitePermissions.jsm:297
(Diff revision 11)
> +      // We do not support setting temp ALLOW on a tab for security reasons.
> +      // In its current state, this permission could be exploited by subframes
> +      // on the same page.
> +      //
> +      // If you ever consider removing this line, make sure you have implemented
> +      // the necessary precautions to protect user privacy and security first.

Maybe we could specify precisely what these precautions are, so we don't wonder when we return on this code in the future.

::: browser/modules/test/browser_SitePermissions.js:1
(Diff revision 11)
> +"use strict";

nit: License or public domain dedication header.

::: browser/modules/test/browser_SitePermissions.js:3
(Diff revision 11)
> +// Prevent showing a dialog for https://name:[e-mail]
> +Services.prefs.setIntPref("network.http.phishy-userpass-length", 2048);

Should use pushPrefEnv to ensure that the preference is cleared.

::: browser/modules/test/browser_SitePermissions.js:18
(Diff revision 11)
> +
> +  let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, uri.spec);
> +
> +  Assert.throws(function() {
> +    SitePermissions.set(uri, id, SitePermissions.ALLOW, SitePermissions.TAB, tab.linkedBrowser);
> +  }, "'Block' is the only permission we can save temporarily on a tab")

nit: semicolon

::: browser/modules/test/browser_SitePermissions.js:81
(Diff revision 11)
> +// This test applies combinations of different permissions and
> +// checks how they override each other.
> +add_task(function* testPermissionState() {

I tend to prefer tests that explain the particular characteristic of the API that is being checked. This combinatory test does its job, but the logic with which the test cases have been created isn't explained in the test code, and the result is that it actually misses some cases.

Here is a real test I wrote for checking that the wider scope always overrides the narrower scope, regardless of the order of the calls. Since this isn't always true, there is an explained exception. This results in five test cases with two permissions set in each, some but not all of them are tested in the current combinatory test case.

  let wideAndNarrowScopes = [
    [SitePermissions.PERSISTENT, SitePermissions.BROWSER_SESSION],
    [SitePermissions.PERSISTENT, SitePermissions.TAB],
    [SitePermissions.BROWSER_SESSION, SitePermissions.TAB],
  ];

  // Use BLOCK for the narrow permission because ALLOW cannot be set on a tab.
  let wideState = SitePermissions.ALLOW;
  let narrowState = SitePermissions.BLOCK;

  for (let [wideScope, narrowScope] of wideAndNarrowScopes) {
    // Set the narrower permission first and the widest later.
    SitePermissions.set(uri, id, narrowState, narrowScope, browser);
    SitePermissions.set(uri, id, wideState, wideScope, browser);
    Assert.deepEqual(SitePermissions.get(uri, id, browser), {
      state: wideState,
      scope: wideScope,
    });
    SitePermissions.remove(uri, id, browser);

    // The behavior of BROWSER_SESSION is not in line with the general behavior
    // because of the legacy nsIPermissionManager implementation.
    if (narrowScope == SitePermissions.BROWSER_SESSION) {
      continue;
    }

    // Set the wider permission first and the narrower later.
    SitePermissions.set(uri, id, wideState, wideScope, browser);
    SitePermissions.set(uri, id, narrowState, narrowScope, browser);
    Assert.deepEqual(SitePermissions.get(uri, id, browser), {
      state: wideState,
      scope: wideScope,
    });
    SitePermissions.remove(uri, id, browser);
  }

(The code can likely be optimized and comments made clearer.)

We can add many separate tests to check for other characteristics of the API, for example one test can check that "null" and PERSISTENT scopes are equivalent when setting, despite PERSISTENT is always what is returned when reading.

Comment 68

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80706/diff/11-12/

Comment 69

[Johann Hofmann [:johannh]]

The new patch addresses many of the comments, which is a lot of stuff. Sorry. If you know of a good way to split the commit up or anything else I can do to help you with the review, let me know.

Give me a moment to write up replies to the comments that weren't fully addressed. I don't consider this patch WIP anymore, if something isn't in here it was intentionally not implemented or forgotten.

Comment 70

[Johann Hofmann [:johannh]]

(In reply to :Paolo Amadini from comment #64)
> Speaking of missing tests, we should test that we can set tab permissions
> for the block case using a popup notification invoked by a subframe of a
> different origin. This should limit the permission for the subframe, but
> should not affect the ability to ask for the permission from the top level
> document, because the origins are different.

That's not how we specified the temporary tab permissions to behave, at least for the block case. For the block case, a single request on a page, e.g. from a subframe, will disallow all future requests for the current top level URI (what the user perceives as the page) for the TAB scope. (That's also the reason why temporary allow is tricky to do with the same interface). I added a test that tests this behavior instead.

This plays into some other parts of the review, see comments inline. Most importantly this simplifies how we can handle URIs in TabPermissions (by always using the currentURI of the browser) and that URIs are ignored when getting or setting TAB scoped permissions through the SitePermissions API.

Comment 71

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

https://reviewboard.mozilla.org/r/80706/#review103830

> Do we have tests that rely on the PermissionStateChange event already? For example, these would check the identity block after a tab permission is set. Maybe browser_PermissionUI.jsm does this already, if not we need to add a test.
> 
> Separately, we may need a simple new test in browser_PermissionUI.jsm where we use the checkbox to set a permanent permission, and see the difference compared to a tab permission.

> Separately, we may need a simple new test in browser_PermissionUI.jsm where we use the checkbox to set a permanent permission, and see the difference compared to a tab permission.

I didn't do this one because I don't really get what "difference compared to" means :)

> The TabPermission object assumes that the URIs passed to its methods are supported by the permission manager. These happen to be URIs for which the prePath property is present and doesn't throw.
> 
> We need to clarify this in a comment. I suggest using the comment for the entire object, because most of the methods take similar arguments and there would be some redundancy if we put a comment on each method.

This was hopefully made redundant by not accepting URIs anymore.

> Actually, we don't support setting tab permissions on URIs that are not supported for session or permanent permissions, so you can return early from most functions if isSupportedURI is false.
> 
> I'm a bit undecided on whether to support passing null as the URI to various methods, meaning that the URI will be taken from the current URI of the browser. I lean towards not allowing it. Requiring the URI makes it more difficult to write code that allows attacks based on the browser navigating between the time a permission request is shown and the time it's written to the tab.

> I'm a bit undecided on whether to support passing null as the URI to various methods, meaning that the URI will be taken from the current URI of the browser. I lean towards not allowing it. Requiring the URI makes it more difficult to write code that allows attacks based on the browser navigating between the time a permission request is shown and the time it's written to the tab.

I don't see how requiring the URI would make an attack harder. That would literally only mean that browser.currentURI would be called two levels down the stack and then passed to our function instead of null.

Another point is that at least SitePermissions.get() does not require the URI because, as stated, temporary blocked permissions from subframes are supposed to apply for the whole page, so when calling .get() we ignore URI anyway and only look at the browser if the scope is TAB.

Maybe there's some aspect I don't perceive, but for now I made all URIs optional.

> Maybe we could specify precisely what these precautions are, so we don't wonder when we return on this code in the future.

Added more text.

> I tend to prefer tests that explain the particular characteristic of the API that is being checked. This combinatory test does its job, but the logic with which the test cases have been created isn't explained in the test code, and the result is that it actually misses some cases.
> 
> Here is a real test I wrote for checking that the wider scope always overrides the narrower scope, regardless of the order of the calls. Since this isn't always true, there is an explained exception. This results in five test cases with two permissions set in each, some but not all of them are tested in the current combinatory test case.
> 
>   let wideAndNarrowScopes = [
>     [SitePermissions.PERSISTENT, SitePermissions.BROWSER_SESSION],
>     [SitePermissions.PERSISTENT, SitePermissions.TAB],
>     [SitePermissions.BROWSER_SESSION, SitePermissions.TAB],
>   ];
> 
>   // Use BLOCK for the narrow permission because ALLOW cannot be set on a tab.
>   let wideState = SitePermissions.ALLOW;
>   let narrowState = SitePermissions.BLOCK;
> 
>   for (let [wideScope, narrowScope] of wideAndNarrowScopes) {
>     // Set the narrower permission first and the widest later.
>     SitePermissions.set(uri, id, narrowState, narrowScope, browser);
>     SitePermissions.set(uri, id, wideState, wideScope, browser);
>     Assert.deepEqual(SitePermissions.get(uri, id, browser), {
>       state: wideState,
>       scope: wideScope,
>     });
>     SitePermissions.remove(uri, id, browser);
> 
>     // The behavior of BROWSER_SESSION is not in line with the general behavior
>     // because of the legacy nsIPermissionManager implementation.
>     if (narrowScope == SitePermissions.BROWSER_SESSION) {
>       continue;
>     }
> 
>     // Set the wider permission first and the narrower later.
>     SitePermissions.set(uri, id, wideState, wideScope, browser);
>     SitePermissions.set(uri, id, narrowState, narrowScope, browser);
>     Assert.deepEqual(SitePermissions.get(uri, id, browser), {
>       state: wideState,
>       scope: wideScope,
>     });
>     SitePermissions.remove(uri, id, browser);
>   }
> 
> (The code can likely be optimized and comments made clearer.)
> 
> We can add many separate tests to check for other characteristics of the API, for example one test can check that "null" and PERSISTENT scopes are equivalent when setting, despite PERSISTENT is always what is returned when reading.

Your test seems good but the main difference to my test is that you're also reversing if the scope is not SESSION. I added a reverse flag to my tests and split my tests up into several tasks. I also added a test that specifically covers the cases you covered here.

You will note my test configurations are a bit more verbose, but they allow for more flexible testing of different scenarios.

Comment 72

[Johann Hofmann [:johannh]]

> Another point is that at least SitePermissions.get() does not require the URI because, as stated, temporary blocked permissions from subframes are supposed to apply for the whole page, so when calling .get() we ignore URI anyway and only look at the browser if the scope is TAB.

To clarify, we don't outright ignore the URI, we first check the URI and then (if there's no result) we check if the browser contains something, ignoring the URI in that check. If the URI is null or otherwise invalid we only look at the browser.

Comment 73

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80706/diff/12-13/

Comment 74

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=14b540d94b8d

Comment 75

[Johann Hofmann [:johannh]]

Rebased and added missing file, thanks for catching that :)

Comment 76

[:Paolo Amadini]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

https://reviewboard.mozilla.org/r/80706/#review104536

First pass, before moving on to the new version.

::: browser/base/content/browser.js:3239
(Diff revision 12)
>      return;
>    }
>  
> +  // Reset temporary permissions on the current tab. This is done here
> +  // because we only want to reset permissions on user reload.
> +  SitePermissions.clearTemporaryPermissions(gBrowser.selectedTab.linkedBrowser);

Just use gBrowser.selectedBrowser here and in other places.

::: browser/base/content/browser.js:7477
(Diff revision 12)
> -    stateLabel.textContent = SitePermissions.getStateLabel(
> -      aPermission.id, aPermission.state, aPermission.inUse || false);
> +    let {state, scope} = aPermission;
> +    // If the user did not permanently allow this device but it is currently
> +    // used, set the variables to display a "temporarily allowed" info.
> +    if (state != SitePermissions.ALLOW && aPermission.inUse) {
> +      state = SitePermissions.ALLOW;
> +      scope = SitePermissions.REQUEST;

.SCOPE_REQUEST

::: browser/base/content/browser.js:7479
(Diff revision 12)
> +    stateLabel.textContent =
> +      SitePermissions.getStateLabel(state, scope);

nit: no need to wrap

::: browser/base/content/test/general/browser_temporary_permissions.js:55
(Diff revision 12)
> +    yield BrowserTestUtils.waitForCondition(() => {
> +      return reloadButton.disabled == false;
> +    });

You may have forgotten this. If possible, use waitForAttribute or another solution.

::: browser/base/content/test/general/browser_temporary_permissions.js:185
(Diff revision 12)
> +// Test that temp blocked permissions requested by subframes (with a different URI) affect the whole page.
> +add_task(function* testTempPermissionSubframes() {
> +  let uri = NetUtil.newURI("https://example.com");
> +  let id = "geo";
> +
> +  yield BrowserTestUtils.withNewTab(SUBFRAME_PAGE, function*(browser) {

I'd add an assertion to check that the origins of the page and subframe are different as expected. This will be more robust to possible future changes of the URIs we use in the test.

::: browser/base/content/test/webrtc/browser_devices_get_user_media.js:127
(Diff revision 12)
> +    let {state, scope} = SitePermissions.get(null, "camera", browser);
> +    Assert.equal(state, SitePermissions.BLOCK);
> +    Assert.equal(scope, SitePermissions.SCOPE_TAB);

nit: Assert.deepEqual, unless you used this style intentionally.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js:541
(Diff revision 12)
> +    is(permission.state, SitePermissions.BLOCK,
> +       "screen sharing is blocked");
> +    is(permission.scope, SitePermissions.SCOPE_PERSISTENT,
>         "screen sharing is persistently blocked");

nit: same here

::: browser/modules/PermissionUI.jsm:269
(Diff revision 12)
> -      let result =
> -        Services.perms.testExactPermissionFromPrincipal(this.principal,
> -                                                        this.permissionKey);
> +      let {state} = SitePermissions.get(this.principal.URI,
> +                                        this.permissionKey,
> +                                        this.browser);

There are two main changes here, which might be fine, but whose effect should be thought about.

The first is that we always used the exact host match, and now we're using the match type that depends on the permission type.

The second is that we used the full principal for the check, and now we're using the URI. I believe the instanceof Ci.nsIStandardURL check above might already exclude system principals and expanded principals, but it's worth checking.

::: browser/modules/PermissionUI.jsm:300
(Diff revision 12)
>            if (this.permissionKey) {
> -            // Remember permissions.
> -            if (state && state.checkboxChecked && promptAction.action) {
> -              Services.perms.addFromPrincipal(this.principal,
> +
> +            let remember = state && state.checkboxChecked;
> +
> +            // Only remember permission for session if in PB mode.
> +            if (remember && PrivateBrowsingUtils.isBrowserPrivate(this.browser)) {

nit: you can have just one "if (remember)" clause and choose the scope based on isBrowserPrivate.

::: browser/modules/SitePermissions.jsm:197
(Diff revision 12)
> +      permission.scope = this.SCOPE_TAB;
> +      permissions[permission.id] = permission;
>      }
>  
> -    return permissions;
> +    for (let permission of this.getAllByURI(browser.currentURI)) {
> +      if (permission.expireType == Services.perms.EXPIRE_SESSION) {

It seems to me the expireType isn't set, so this won't work. Looks like the calling code ignores the scope anyways, but we should add a regression test just for the API if we want it to return the right scope.

Comment 77

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

https://reviewboard.mozilla.org/r/80706/#review103312

> Is there a better way to wait for the correct state here rather than observing the button? If not, use waitForAttribute to avoid polling if possible.

I was hitting a condition where the button wasn't clickable yet. Unfortunately the mechanics of waitForAttribute make it impossible to wait for an attribute stopping to exist. We could make a new bug to fix that but right now I think this suffices.

> Worth calling out that with this change we're now storing permanent allow permissions in Private Browsing Mode if the user opts in. This stores a permanent indication that the user visited the site.
> 
> Even if this is intentional, maybe we should file a different bug instead of burying this change in a big patch?
> 
> I also don't see why the same logic shouldn't apply to a permanent block if the checkbox is selected.

I think you're right, we were just replicating the original behavior but to me it doesn't make sense to permanently store block in private mode, especially since the checkbox explicitly says "Remember for session". I changed it.

Comment 78

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

https://reviewboard.mozilla.org/r/80706/#review103384

> Don't use the "a" prefix for arguments in new code.
> 
> I also think the API will be much less error-prone if all methods took a single object argument, for example here "set({ browser, uri, id, state })".
> 
> This would really benefit the main SitePermissions API as well, although I understand that you may want to keep backwards compatibility to some extent. You may still consider doing this for some methods that are used only by the user interface, like getStateLabel.

> I also think the API will be much less error-prone if all methods took a single object argument, for example here "set({ browser, uri, id, state })".

I disagree on that, the options pattern should be reserved for functions taking variations of optional parameters, and that is how it's used relatively consistently in the code, AFAUI. Especially with the latest changes there are very few parameters in the methods of TabPermissions, and all of them are mandatory.

> You should likely have an URI argument here like the other methods.

I switched the other methods to not take any URI parameter, instead. The URI should always be determined by the browser object. This
- is the only way we've been using this API anyway
- simplifies the API, internal as well as caller code
- prevents future (accidental) misuse by people forwarding the request URI e.g. from subframes instead of using the currentURI

See my general comment on how subframe permissions should apply for the entire page.

Comment 79

[Johann Hofmann [:johannh]]

Paolo, sorry, some of my comments were not published. See the last two comments for more details :/

Comment 80

[:Paolo Amadini]

(In reply to Johann Hofmann [:johannh] from comment #70)
> That's not how we specified the temporary tab permissions to behave

You're right, I got confused here, we decided the "block" case applies to the whole tab. If I understand correctly that is also because it's designed to solve the spamming problem, and it's better for the block to be broader.

Thinking about it, a block with "remember my decision" checked may now be narrower than a block without it.

(In reply to Johann Hofmann [:johannh] from comment #71)
> I don't see how requiring the URI would make an attack harder. That would
> literally only mean that browser.currentURI would be called two levels down
> the stack and then passed to our function instead of null.

It makes writing code that potentially allows an attack harder.

In the current patch, the race condition I talked about actually exists, and a permission may be associated to an origin that is different from the one that requested the permission in the first place and is shown to the user. For the "block" case, I actually don't see anything bad happening because of this, so it might be just fine.

Thinking about this some more, the tab block case and the current permissions model for block and allow are really different. While consumers are interested in getting the current state and scope of the permission for the "get" case, the current "set" method is probably overloaded with two use cases that are too different from each other. This can be seen in the fact that the value of some parameters change the meaning of others, specifically the URI is ignored for tab permissions and you cannot specify an allow state.

All the concerns might be solved if we had two different "set" methods, one for ordinary permissions one for tab block.

The PermissionUI module would call both methods for the block case when the checkbox is checked. This way, a block with "remember my decision" checked will not be narrower than a block without it, although the "remember my decision" part will only apply to the exact origin that requested the permission.

Comment 81

[:Paolo Amadini]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

https://reviewboard.mozilla.org/r/80706/#review104546

Apart from the comments above, the rest of the patch makes sense to me in the light of the different expectations for tab block permission.

I think the architecture of the main tests is good too, although I may still have missed something as I didn't check the actual combinations yet.

::: browser/modules/SitePermissions.jsm:17
(Diff revision 13)
>  
> -this.SitePermissions = {
> +/**
> + * A helper module to manage temporary tab permissions.
> + */
> +const TabPermissions = {
> +  _stateByBrowser: new WeakMap(),

Missing JSDoc.

::: browser/modules/SitePermissions.jsm:65
(Diff revision 13)
> +      return null;
> +    }
> +    let tabEntry = this._stateByBrowser.get(browser);
> +    let prePath = browser.currentURI.prePath;
> +    if (tabEntry && tabEntry[prePath]) {
> +      let permission = tabEntry[prePath][id];

nit: let entry =

::: browser/modules/SitePermissions.jsm:71
(Diff revision 13)
> +      return this._get(tabEntry, prePath, id, permission);
> +    }
> +    return null;
> +  },
> +
> +  getAllForBrowser(browser) {

nit: just getAll

::: browser/modules/SitePermissions.jsm:77
(Diff revision 13)
> +    let permissions = [];
> +    let tabEntry = this._stateByBrowser.get(browser);
> +    let prePath = browser.currentURI.prePath;
> +    if (tabEntry && tabEntry[prePath]) {
> +      let entries = tabEntry[prePath];
> +      for (let id in entries) {

Avoid for..in, use for..of Object.keys instead.

::: browser/modules/SitePermissions.jsm:165
(Diff revision 13)
> +   *           - availableStates: an array of all available states for that permission,
> +   *             represented as objects with the keys:
> +   *             - id: the state constant
> +   *             - label: the translated label of that state
> +   */
> +  getPermissionItem(id, scope, state = this.getDefault(id)) {

Rename to getPermissionDetails for consistent naming.

::: browser/modules/SitePermissions.jsm:217
(Diff revision 13)
> +   * @param {Browser} browser
> +   *        The browser to fetch permission for.
> +   *
> +   * @return {Array} a list of objects. See getPermissionItem for the content of each object.
> +   */
> +  getPermissionDetailsForBrowser(browser) {

getAllPermissionDetailsForBrowser

::: browser/modules/test/browser_SitePermissions.js:291
(Diff revision 13)
> +    Assert.deepEqual(SitePermissions.get(uri, id, browser), {
> +      state: SitePermissions.BLOCK,
> +      scope: SitePermissions.SCOPE_TAB,
> +    });
> +
> +    yield new Promise((c) => setTimeout(c, 2000));

This can probably work with a shorter timeout, for example use a 100ms value and wait 200ms.

I suggest moving just this test to a new file so we better categorize possible intermittent failures caused by this, and is more self-contained if we have to whitelist the use of unsafe timeouts.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/ControlCenter.jsm:96
(Diff revision 13)
>        }),
>      },
>  
>      allPermissions: {
>        applyConfig: Task.async(function* () {
> -        // there are 3 possible non-default permission states, so we alternate between them
> +        // TODO: Rewrite this to consider temporary (TAB) permission states.

Good catch, if you want you can file a bug for this and put the bug number here replacing the TODO line. This is going to be relevant for testing localizations too.

::: modules/libpref/init/all.js:1224
(Diff revision 13)
>  pref("privacy.trackingprotection.annotate_channels",  false);
>  // Lower the priority of network loads for resources on the tracking protection list.
>  // Note that this requires the privacy.trackingprotection.annotate_channels pref to be on in order to have any effect.
>  pref("privacy.trackingprotection.lower_network_priority",  false);
>  
> +// Time temporary until temporary permissions on tabs expire, in ms

Just noticed a repetition here.

Comment 82

[:Paolo Amadini]

(In reply to Johann Hofmann [:johannh] from comment #71)
> > Separately, we may need a simple new test in browser_PermissionUI.jsm where we use the checkbox to set a permanent permission, and see the difference compared to a tab permission.
> 
> I didn't do this one because I don't really get what "difference compared
> to" means :)

The difference is that one sets a temporary permission on the tab, and the other sets a permanent permission on the origin. It's a different case, and I'm not sure how we test it right now.

Comment 83

[:Paolo Amadini]

We just discussed the permanent block case in subframes in more detail. We considered the case where we ask for a permission from a subframe that has a different origin than the top level page, when the user selects the checkbox that usually says "Remember this decision" and clicks the "Block" button.

Any permission set for an origin that is not the one of the top level page is never shown in the Control Center. While this may change in the future, we considered the current state.

My proposal in comment 80 was that in this case we would set a permanent block permission for the subframe origin, and also a temporary block permission for the top level page. This has the issue that we would confusingly show "Blocked temporarily" in the Control Center, since that is what is associated to the top level page. This temporary permission would also be cleared if the user manually reloads the page, but the invisible permanent block for the subframe origin would remain.

What is implemented in the work in progress patch from comment 73 is that we only set a permanent block permission for the subframe origin. This has the issue that the permanent block is not visible in the Control Center, whereas a temporary block would be visible. The block is also less effective because it doesn't prevent other subframes or the top level page from requesting the same permission again, which is counter-intuitive for an option that should extend the lifetime of the permission.

In both cases, the issue with a temporary block from a subframe is that the user might accidentally block a site with no easy way to revert their decision. The user would need to use the general application preferences.

Thus, we decided to disallow permanent blocks from subframes of a different origin. We'll achieve this by disabling the "Block" button when the checkbox is selected. We will keep the ability to allow the origin of a subframe permanently.

For permissions that cannot be allowed permanently, like screen sharing or some other permissions over HTTP, the "Remember this decision" checkbox will not be available in subframes of a different origin, because otherwise selecting it would result in disabling both action buttons.

As a clarification, if the subframe is from the same origin as the top level page, the issues above don't apply and we can still block permanently.

Comment 84

[Johann Hofmann [:johannh]]

Tanvi, do you have any concerns regarding the planned changes described in comment 83 (disallowing subframe permission requests to be blocked permanently)?

It's a tricky problem and there's no great solution, but I think this is the best compromise.

Comment 85

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to :Paolo Amadini from comment #83)
> We just discussed the permanent block case in subframes in more detail. We
> considered the case where we ask for a permission from a subframe that has a
> different origin than the top level page, when the user selects the checkbox
> that usually says "Remember this decision" and clicks the "Block" button.
> 
> Any permission set for an origin that is not the one of the top level page
> is never shown in the Control Center. While this may change in the future,
> we considered the current state.

This is bug 1224453.

> Thus, we decided to disallow permanent blocks from subframes of a different
> origin. We'll achieve this by disabling the "Block" button when the checkbox
> is selected. We will keep the ability to allow the origin of a subframe
> permanently.

Sites would just need an iframe to subvert blocking? Terrible idea.

What exactly in this bug requires solving bug 1224453?

Comment 86

[Tanvi Vyas[:tanvi]]

(In reply to Johann Hofmann [:johannh] from comment #84)
> Tanvi, do you have any concerns regarding the planned changes described in
> comment 83 (disallowing subframe permission requests to be blocked
> permanently)?
> 
> It's a tricky problem and there's no great solution, but I think this is the
> best compromise.

I see that it is tricky.  Why don't we just block the domain permanently in all contexts?

So a.com embeds b.com.  b.com requests access to geolocation.  User checks the remember my decision button and clicks the Deny button.  Now geolocation requests from b.com will be denied in any situation -
b.com as top level page
b.com embedded in a.com
b.com embedded in c.com
etc

If the user wants to allow the permission, they will have to go to b.com as a top level page to allow it.

Do you think that would cause any significant breakage?  Would that breakage be better than the alternative - an annoying and often embedded b.com?  (Ex: a popular maps site that constantly asks for your location everytime its embedded.)

We could make this more complicated, but I'm not sure if its worth the effort: Don't allow block permanently for subframes.  If b.com is blocked temporarily 3 times as a subframe, add the "remember my decision" checkbox the 4th time the subframe asks for permission, and block b.com as a top level or subframe permanently if the user checks that box.

Comment 87

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Tanvi Vyas - behind on bugmail [:tanvi] from comment #86)
> If the user wants to allow the permission, they will have to go to b.com as
> a top level page to allow it.

Note that this won't work as a long-term solution once bug 1224453 is fixed. See also workaround in bug 1224453.

> Do you think that would cause any significant breakage?  Would that breakage
> be better than the alternative - an annoying and often embedded b.com?

What breakage? Bug 1224453 is not a regression. Also, shouldn't we take this discussion to that bug?

Comment 88

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #87)
> Note that this won't work as a long-term solution once bug 1224453 is fixed.

Sorry I meant bug 1299577 there.

Comment 89

[Florian Quèze [:florian]]

The plan in comment 83 was discussed in the context of needing a short term solution we can implement in time for 53. Bug 1224453 and bug 1299577 (and more generally better handling of subframes of different origins for permission prompts and the control center) are on our radar, but are out of scope for 53.

Comment 90

[Johann Hofmann [:johannh]]

So after seeing that this is apparently not quickly resolved I made bug 1330559 to deal with the permanent block issue separately.

The problems around permanently blocked subframe permissions are not caused by this bug, it's merely getting more exposure now. I don't think this bug has to solve these issues and discussing them here (and with the impression of "let's get this bug done") might not lead to the best solution (and diverts from the original target, temporary permissions). We should still solve bug 1330559 in a timely manner, but the resulting changes are likely to be easily upliftable.

This bug, however, has grown quite a lot and deserves to be finished.

Comment 91

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80706/diff/13-14/

Comment 92

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65fbace2adca

Comment 93

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

https://reviewboard.mozilla.org/r/80706/#review104536

> nit: Assert.deepEqual, unless you used this style intentionally.

Yeah that was intentional, I don't think we need deepEqual here.

> There are two main changes here, which might be fine, but whose effect should be thought about.
> 
> The first is that we always used the exact host match, and now we're using the match type that depends on the permission type.
> 
> The second is that we used the full principal for the check, and now we're using the URI. I believe the instanceof Ci.nsIStandardURL check above might already exclude system principals and expanded principals, but it's worth checking.

Yeah, good points. For the first point, PermissionUI.jsm is only used for permissions that have the exact match type right now. So I think that's ok.

Second point, uh, I'm not sure. What's the best way to check this?

> It seems to me the expireType isn't set, so this won't work. Looks like the calling code ignores the scope anyways, but we should add a regression test just for the API if we want it to return the right scope.

Great catch, thank you!

Comment 94

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

https://reviewboard.mozilla.org/r/80706/#review104546

> Missing JSDoc.

Added comments to all functions and a more detailed description to the module. I don't think we need formal JSDoc for this internal module.

Comment 95

[:Paolo Amadini]

(In reply to Johann Hofmann [:johannh] from comment #93)
> > The second is that we used the full principal for the check, and now we're using the URI. I believe the instanceof Ci.nsIStandardURL check above might already exclude system principals and expanded principals, but it's worth checking.
> 
> Second point, uh, I'm not sure. What's the best way to check this?

We can look at the calling code to see which paths, if any, may result in a system principal. I think the idea here is that permissions from privileged pages are granted automatically by nsIPermissionManager, but I'm not sure if this is something we rely upon to avoid showing the prompts.

In other words it could be that, while it's important for nsIPermissionManager to allow system principals for other callers, this is not relevant for the current visible permissions in Firefox.

I don't know what is the use case for expanded principals in the context of permissions, and why the permission manager treats them specially instead of just ignoring them or always denying the permission.

(In reply to Johann Hofmann [:johannh] from comment #94)
> > Missing JSDoc.
> 
> Added comments to all functions and a more detailed description to the
> module. I don't think we need formal JSDoc for this internal module.

I was referring specifically to the documentation of _stateByBrowser. We need something to illustrate the data structure so we don't need to guess by looking at the code.

Comment 96

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80706/diff/14-15/

Comment 97

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fc8e26951b2

Comment 98

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

If we're going to add this feature, we need an [x] button in the doorhanger IMHO.

To show what I mean, try to edit this fiddle: http://jsfiddle.net/srn9db4h/ in Nightly.

In Firefox release, this is no problem. But in Nightly, the doorhanger won't go away, and has to be dismissed with a choice.

People are going to instinctively chose "Don't Allow", as that's closest to "close". Once this bug lands, it sounds like this'll add more obstacles in the way, by having the fiddle stop working until the page is refreshed. Since refreshing a jsfiddle loses any edits, that's quite unfortunate! Not liking this.

Comment 99

[Johann Hofmann [:johannh]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #98)
> If we're going to add this feature, we need an [x] button in the doorhanger
> IMHO.
> 
> To show what I mean, try to edit this fiddle: http://jsfiddle.net/srn9db4h/
> in Nightly.
> 
> In Firefox release, this is no problem. But in Nightly, the doorhanger won't
> go away, and has to be dismissed with a choice.
> 
> People are going to instinctively chose "Don't Allow", as that's closest to
> "close". Once this bug lands, it sounds like this'll add more obstacles in
> the way, by having the fiddle stop working until the page is refreshed.
> Since refreshing a jsfiddle loses any edits, that's quite unfortunate! Not
> liking this.

There will be a "blocked permission" icon as indicator in the identity block, if you click on it the control center opens and lets you remove the temporary block. No reload required (although the hint will state the opposite, ugh).

I don't think anyone who is capable of finding the anchor icon after dismissing a notification right now will fail to notice the blocked icon in the exact same place in the future. There are still plans to animate the icon slightly if a permission request was blocked, maybe that could help discovery as well.

Sorry for making your workflow harder, but our assumption is that most people don't want to be repeatedly annoyed by permission requests.

Comment 100

[Dão Gottwald [:dao]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

https://reviewboard.mozilla.org/r/80706/#review105434

::: browser/base/content/tabbrowser.xml:74
(Diff revision 15)
>                     .getService(Components.interfaces.mozIPlacesAutoComplete);
>        </field>
>        <field name="AppConstants" readonly="true">
>          (Components.utils.import("resource://gre/modules/AppConstants.jsm", {})).AppConstants;
>        </field>
> +      <field name="SitePermissions" readonly="true">

SitePermissions is already available here from browser.js.

::: browser/modules/SitePermissions.jsm:23
(Diff revision 15)
> + *
> + * This uses a WeakMap to key browsers, so that entries are
> + * automatically cleared once the browser stops existing
> + * (once there are no other references to the browser object);
> + */
> +const TabPermissions = {

SCOPE_TAB and TabPermissions are confusing since all methods seem to take browsers (as they should) rather than tabs. s/tab/browser/ throughout SitePermissions.jsm please.

Comment 101

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80706/diff/15-16/

Comment 102

[Johann Hofmann [:johannh]]

So I called it SCOPE_TEMPORARY for now, it's independent of implementation and signifies the intent a little better than BROWSER, IMO. I don't feel strongly about this though, so if you still think SCOPE_BROWSER is better, I'm happy to change it.

I also changed the internal implementation to only consider BLOCK state as per my conversation with Paolo (implementing temp allow, if it ever happens, would likely require drastic changes to that internal module anyway).

Comment 103

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=638d1916b5b0

Comment 104

[:Paolo Amadini]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

https://reviewboard.mozilla.org/r/80706/#review105450

(In reply to Johann Hofmann [:johannh] from comment #102)
> So I called it SCOPE_TEMPORARY for now, it's independent of implementation
> and signifies the intent a little better than BROWSER, IMO.

I agree, also considering that they expire after a definite time.

This should be good to go, assuming that nothing breaks because of comment 95. Anyways, if we have regressions with system principals, we can easily fix those in a follow-up.

Thanks so much Johann and Florian for working on this!

::: browser/modules/SitePermissions.jsm:27
(Diff revision 16)
> +  //   prePath: {
> +  //     permissionID: {Number} timeStamp

nit: putting <prePath> and <permissionId> in angular brackets might make it clearer that these are not the actual property names.

::: browser/modules/SitePermissions.jsm:411
(Diff revision 16)
>  
> -    Services.perms.add(aURI, aPermissionID, aState);
> +      if (!browser) {
> +        throw "TEMPORARY scoped permissions require a browser object";
> +      }
> +
> +      TemporaryBlockedPermissions.set(browser, permissionID, state);

No "state" argument needed anymore.

Comment 105

[:Paolo Amadini]

There's probably a little bit of work left because browser_SitePermissions_tab_urls.js still times out, but I don't expect these test adjustments to require another review.

Comment 106

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Johann Hofmann [:johannh] from comment #99)
> There will be a "blocked permission" icon as indicator in the identity
> block, if you click on it the control center opens and lets you remove the
> temporary block. No reload required (although the hint will state the
> opposite, ugh).

Ok, so the new workflow to edit my fiddle on page load will be:
 1. Choose "Don't Allow",
 2. Locate and click "blocked permission" icon, and
 3. Remove temporary block.

Do I have that right?

> I don't think anyone who is capable of finding the anchor icon after
> dismissing a notification right now will fail to notice the blocked icon in
> the exact same place in the future. There are still plans to animate the
> icon slightly if a permission request was blocked, maybe that could help
> discovery as well.

No need to find the anchor icon. I edit the fiddle, hit the jsfiddle "Run" button (which reloads the subframe) and I get a new permission prompt. Works well today in release.

> Sorry for making your workflow harder, but our assumption is that most
> people don't want to be repeatedly annoyed by permission requests.

That problem is new with the new UX, introduced by you guys. Firefox release (50) does not have that problem.

I already proposed an alternative approach that doesn't have these problems. See bug 1302137. My assumption is that many people, me included, prefer control over when a web-site can see or hear us. This means being asked each time, and sometimes we'll say no. Not every time, it'll depend on context. Our new model doesn't seems designed for that, instead taking rejection quite hard.

Comment 107

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80706/diff/16-17/

Comment 108

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=10710f0cfe8a

Comment 109

[Florian Quèze [:florian]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

https://reviewboard.mozilla.org/r/80706/#review105646

::: modules/libpref/init/all.js:1225
(Diff revision 17)
>  // Lower the priority of network loads for resources on the tracking protection list.
>  // Note that this requires the privacy.trackingprotection.annotate_channels pref to be on in order to have any effect.
>  pref("privacy.trackingprotection.lower_network_priority",  false);
>  
> +// Time until temporary permissions expire, in ms
> +pref("privacy.temporary_permission_expire_time_ms",  3600000);

If this pref is only used in browser/, its default value should be in browser/app/profile/firefox.js

Comment 110

[Florian Quèze [:florian]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #106)

> Our new model doesn't seems designed for that, instead taking rejection
> quite hard.

Our new model makes Firefox the user's agent. If the user says no, it means no. I can't see a good reason to have the default configuration let a website ask again immediately after the user said no.

How long this 'no' applies is up to you (the user), as there's a pref in about:config that lets you control how long this 'no' will be remembered: privacy.temporary_permission_expire_time_ms

Comment 111

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80706/diff/17-18/

Comment 112

[Johann Hofmann [:johannh]]

Comment on attachment 8794123 [details]
Bug 1206232 - Add temporary permission states to SitePermissions.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80706/diff/18-19/

Comment 113

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a836d3535f45

Comment 114

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9eba4bb9cb62
Add temporary permission states to SitePermissions.jsm. r=jdm,Paolo

Comment 115

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9eba4bb9cb62

Comment 116

[Panos Astithas (he/him) [:past] (please ni?)]

Jan-Ivar, let's try the new implementation in Nightly and file followup bugs for any problems we may find. The approach in this bug has been debated to death, both here and in-person and I think it is a valid approach, in line with our general design.

Comment 117

[Johann Hofmann [:johannh]]

(In reply to Panos Astithas [:past] from comment #116)
> Jan-Ivar, let's try the new implementation in Nightly and file followup bugs
> for any problems we may find. The approach in this bug has been debated to
> death, both here and in-person and I think it is a valid approach, in line
> with our general design.

As Florian mentioned the temporary block shouldn't be a big concern for anyone, since you can simply turn it off by setting privacy.temporary_permission_expire_time_ms to 0. I think the whole issue is more about modal permission doorhangers in general, but yeah, that has been debated to death countless times.

Comment 118

[Hani Yacoub, Desktop QA]

Build ID: 20170131030205
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

Verified as fixed on on Windows 10 x 64, Mac OS X 10.11 and Ubuntu 16.04 x64 on Firefox Nightly 54.0a1, Firefox Nightly 53.0a1 and Aurora 53.0a2.