Closed Bug 1413780 Opened 7 years ago Closed 6 years ago

Make canvas permission a first-class permission

Categories

(Firefox :: Site Identity, enhancement, P1)

Product:
Firefox
Component:
Site Identity
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: cfu, Assigned: cfu)

References

Details

(Whiteboard: [fingerprinting])

Attachments

(7 files, 1 obsolete file)

Screen Shot 2017-11-02 at 4.39.44 PM.png
103.82 KB, image/png
Details
Bug 1413780 - Make canvas permission visible in site information panel.
59 bytes, text/x-review-board-request
johannh
: review+
Details
Bug 1413780 - Hide canvas permission from site information panel when privacy.resistFingerprinting is false.
59 bytes, text/x-review-board-request
johannh
: review+
Details
Icon - SVGs.zip
1.32 KB, application/zip
Details
Bug 1413780 - Update icon design and add blocked effect.
59 bytes, text/x-review-board-request
johannh
: review+
Details
Bug 1413780 - Add a test for canvas permission to test_SitePermissions.js
59 bytes, text/x-review-board-request
johannh
: review+
Details
Bug 1413780 - Change permission name from canvas/extractData to canvas.
59 bytes, text/x-review-board-request
johannh
: review+
Details 
We introduced a site permission prompt for canvas data extraction in bug 967895.  However, the permission is not listed in the site information panel so users have no chance to change it, as mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1382111#c22

    Another issue is that there is no easy way to figure out whether this
    particular site has permission to access the canvas. Normally, I would expect
    to see this information after clicking the "i" icon in the URL bar, but it
    only says that "you have not granted this site any special permissions", which
    is not true because I granted it the permanent permission to access the
    canvas. How can I deny this website from accessing my canvas in the future?
The patch simply fixes the problem, and this image is how it looks.  Do we have to hide it when privacy.resistFingerprinting is false?
Flags: needinfo?(tom)
Flags: needinfo?(arthuredelstein)
Yes, we should not show this if p.rP is false.  

However, it may be the case that it doesn't appear unless it is set.  Please test with the following configurations:

1. Never having turned on p.RP, do we ever see mention of this permission?
2. If I turn on p.rP, I should see the permission and be able to set or clear it like anything else
3. If I turn off p.rP, does mention of the permission go away?
Flags: needinfo?(tom)
Flags: needinfo?(arthuredelstein)
Component: Notifications and Alerts → Site Identity and Permission Panels
Product: Toolkit → Firefox
Assignee: nobody → cfu
Priority: -- → P1
Attachment #8924464 - Attachment is obsolete: true
Added another patch that hides canvas permission from the site information panel when privacy.resistFingerprinting is false.
So

(In reply to Tom Ritter [:tjr] from comment #3)
> Yes, we should not show this if p.rP is false.  
> 
> However, it may be the case that it doesn't appear unless it is set.  Please
> test with the following configurations:
> 
> 1. Never having turned on p.RP, do we ever see mention of this permission?

No, because the permission can't be decided.

> 2. If I turn on p.rP, I should see the permission and be able to set or
> clear it like anything else

Yes.

> 3. If I turn off p.rP, does mention of the permission go away?

Yes, it will not be listed in the site information panel.
Blocks: 1419938
Attachment #8930845 - Flags: review?(jhofmann)
Attachment #8930846 - Flags: review?(jhofmann)
Comment on attachment 8930845 [details]
Bug 1413780 - Make canvas permission visible in site information panel.

https://reviewboard.mozilla.org/r/201944/#review207854

Please filter canvas permissions in the Page Info window as well (https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/browser/base/content/pageinfo/permissions.js#20)

Thanks!
Attachment #8930845 - Flags: review?(jhofmann) → review-
Comment on attachment 8930846 [details]
Bug 1413780 - Hide canvas permission from site information panel when privacy.resistFingerprinting is false.

https://reviewboard.mozilla.org/r/201946/#review207864

Please also ignore canvas permissions here: https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/browser/base/content/browser.js#7609

to avoid showing the little dot indicator on the (i) icon to indicate ALLOW permissions or the blocked icon for the BLOCK state.
Attachment #8930846 - Flags: review?(jhofmann) → review-
(In reply to Johann Hofmann [:johannh] from comment #9)
> Comment on attachment 8930846 [details]
> Bug 1413780 - Hide canvas permission from site information panel when
> privacy.resistFingerprinting is false.
> 
> https://reviewboard.mozilla.org/r/201946/#review207864
> 
> Please also ignore canvas permissions here:
> https://searchfox.org/mozilla-central/rev/
> 33c90c196bc405e628bc868a4f4ba29b992478c0/browser/base/content/browser.js#7609
> 
> to avoid showing the little dot indicator on the (i) icon to indicate ALLOW
> permissions or the blocked icon for the BLOCK state.

That reminds me: there's no permission icon for canvas in the identity popup and no "blocked" icon in the identity block. (Try blocking and allowing geolocation or notifications for a comparison).

For the ALLOW case (to display it in the identity popup), you can just add a CSS rule here (similar to the other permissions):

https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/browser/themes/shared/notification-icons.inc.css#29

For the BLOCK case you'd need another canvas icon with a strikethrough (we used to embed the strikethrough as a separate path in the SVGs but I think it's flattened now).

Maybe Bryan can get us one quickly? :)

Thanks!
Flags: needinfo?(bbell)
(In reply to Johann Hofmann [:johannh] from comment #10)
> That reminds me: there's no permission icon for canvas in the identity popup
> and no "blocked" icon in the identity block. (Try blocking and allowing
> geolocation or notifications for a comparison).
> 
> For the ALLOW case (to display it in the identity popup), you can just add a
> CSS rule here (similar to the other permissions):
> 
> https://searchfox.org/mozilla-central/rev/
> 33c90c196bc405e628bc868a4f4ba29b992478c0/browser/themes/shared/notification-
> icons.inc.css#29
> 
> For the BLOCK case you'd need another canvas icon with a strikethrough (we
> used to embed the strikethrough as a separate path in the SVGs but I think
> it's flattened now).
> 
> Maybe Bryan can get us one quickly? :)
> 
> Thanks!

Besides this is our current icon
https://searchfox.org/mozilla-central/source/browser/themes/shared/notification-icons/canvas.svg

Thanks for help!
Whiteboard: [fingerprinting]
Our UX helper suggests that we should ask Eric Pang for the block icon, who provided the current icon for us in bug 1382111.


Hi Eric,

Could you please help with the block icon of canvas permission? Thanks!
Flags: needinfo?(bbell) → needinfo?(epang)
Attached file Icon - SVGs.zip — 
Hi Chung, I've attached svgs of the icons. Note that I've updated both states.  The palette didn't work well in the disabled state. ni me if anything else is needed!  Thanks!
Flags: needinfo?(epang) → needinfo?(cfu)
Thank you very much! I will update them.
Flags: needinfo?(cfu)
Attachment #8930845 - Flags: review?(jhofmann)
Attachment #8930846 - Flags: review?(jhofmann)
Attachment #8934144 - Flags: review?(jhofmann)
Attachment #8930845 - Flags: review?(jhofmann)
Attachment #8930846 - Flags: review?(jhofmann)
I applied the newly designed icons with some minor modifications. The differences from the original ones (attachment 8933469 [details]) are:
1. change the dimensions to 16x16
2. set fill style as an attribute of <svg> instead of CSS.
3. simplify the code by removing transform (using SVGO)

And I also made the blocked icon shown on the left of the URL bar (like denying camera/microphone).

Please take a look again, thanks!
Comment on attachment 8930845 [details]
Bug 1413780 - Make canvas permission visible in site information panel.

https://reviewboard.mozilla.org/r/201944/#review212776

You might want to change this test to add the new permission, similarly to what persistent-storage did when it was still prefed-off (though I can we can remove that condition now) https://searchfox.org/mozilla-central/source/browser/modules/test/unit/test_SitePermissions.js#18,109
Attachment #8930845 - Flags: review?(jhofmann) → review+
Comment on attachment 8930845 [details]
Bug 1413780 - Make canvas permission visible in site information panel.

https://reviewboard.mozilla.org/r/201944/#review212778

::: browser/locales/en-US/chrome/browser/sitePermissions.properties:41
(Diff revision 2)
>  permission.popup.label = Open Pop-up Windows
>  permission.geo.label = Access Your Location
>  permission.shortcuts.label = Override Keyboard Shortcuts
>  permission.focus-tab-by-prompt.label = Switch to this Tab
>  permission.persistent-storage.label = Store Data in Persistent Storage
> +permission.canvas.label = Extract canvas data

Oh, also, looking at the other labels, I think this this should be:

Extract Canvas Data
Comment on attachment 8930846 [details]
Bug 1413780 - Hide canvas permission from site information panel when privacy.resistFingerprinting is false.

https://reviewboard.mozilla.org/r/201946/#review212712

::: browser/modules/SitePermissions.jsm:225
(Diff revision 2)
>      for (let permission of this.getAllByURI(browser.currentURI)) {
>        permissions[permission.id] = permission;
>      }
>  
> +    // Hide canvas permission when privacy.resistFingerprinting is false.
> +    if (!Services.prefs.getBoolPref("privacy.resistFingerprinting")) {

getAllForBrowser calls getAllByURI to get the permissions, so I'd say the better place to put this is where we already filter out the "install" permission, currently in line 178.
Attachment #8930846 - Flags: review?(jhofmann) → review-
Comment on attachment 8934144 [details]
Bug 1413780 - Update icon design and add blocked effect.

https://reviewboard.mozilla.org/r/205094/#review212710

Looks good but it would be great if we could get rid of the slash in the class name.

::: browser/themes/shared/notification-icons.inc.css:118
(Diff revision 1)
>    list-style-image: url(chrome://browser/skin/notification-icons/screen-blocked.svg);
>  }
>  
>  #canvas-notification-icon,
> -.popup-notification-icon[popupid="canvas-permissions-prompt"] {
> +.popup-notification-icon[popupid="canvas-permissions-prompt"],
> +.canvas\/extractData-icon {

Is there any reason you can't simplify this classname to .canvas-icon on the image element? Please do, otherwise.
Attachment #8934144 - Flags: review?(jhofmann) → review+
Attachment #8930846 - Flags: review?(jhofmann)
Attachment #8936942 - Flags: review?(jhofmann)
Attachment #8936943 - Flags: review?(jhofmann)
Attachment #8930846 - Flags: review?(jhofmann)
(In reply to Johann Hofmann [:johannh] from comment #19)
> Comment on attachment 8930845 [details]
> Bug 1413780 - Make canvas permission visible in site information panel.
> 
> https://reviewboard.mozilla.org/r/201944/#review212776
> 
> You might want to change this test to add the new permission, similarly to
> what persistent-storage did when it was still prefed-off (though I can we
> can remove that condition now)
> https://searchfox.org/mozilla-central/source/browser/modules/test/unit/
> test_SitePermissions.js#18,109

Please review Attachment #8936942 [details]

(In reply to Johann Hofmann [:johannh] from comment #22)
> Comment on attachment 8934144 [details]
> Bug 1413780 - Update icon design and add blocked effect.
> 
> https://reviewboard.mozilla.org/r/205094/#review212710
> 
> Looks good but it would be great if we could get rid of the slash in the
> class name.
> 
> ::: browser/themes/shared/notification-icons.inc.css:118
> (Diff revision 1)
> >    list-style-image: url(chrome://browser/skin/notification-icons/screen-blocked.svg);
> >  }
> >  
> >  #canvas-notification-icon,
> > -.popup-notification-icon[popupid="canvas-permissions-prompt"] {
> > +.popup-notification-icon[popupid="canvas-permissions-prompt"],
> > +.canvas\/extractData-icon {
> 
> Is there any reason you can't simplify this classname to .canvas-icon on the
> image element? Please do, otherwise.

It is related to the permission name, so I added a patch (Attachment #8936943 [details]) that changes the permission name from "canvas/extractData" to "canvas". Please take a look.

Thanks!
Comment on attachment 8930846 [details]
Bug 1413780 - Hide canvas permission from site information panel when privacy.resistFingerprinting is false.

https://reviewboard.mozilla.org/r/201946/#review214124

::: browser/modules/SitePermissions.jsm:183
(Diff revision 3)
>          if (permission.type == "install") {
>            continue;
>          }
> +
> +        // Hide canvas permission when privacy.resistFingerprinting is false.
> +        if ((permission.type == "canvas/extractData") &&

Nit: you don't need the parens around this.
Attachment #8930846 - Flags: review?(jhofmann) → review+
Comment on attachment 8936942 [details]
Bug 1413780 - Add a test for canvas permission to test_SitePermissions.js

https://reviewboard.mozilla.org/r/207672/#review214126

That test should work but what I was mostly concerned about is that test_SitePermissions.js will fail in testPermissionsListing() and testCanvasPermission() in any build that has privacy.resistFingerprinting prefed on. It might be worth adding a condition to these tests like persistent-storage does it.
Attachment #8936942 - Flags: review?(jhofmann)
Comment on attachment 8936943 [details]
Bug 1413780 - Change permission name from canvas/extractData to canvas.

https://reviewboard.mozilla.org/r/207674/#review214132

r+ because this should technically work, but think carefully about the consequences. Everyone who has already set any canvas/extractData permission will lose those permissions and get prompted again. Since this is a prefed-off feature I wouldn't assume that this impacts a lot of users, I just wanted to make sure we're aware of it.

I should have really required a different permission name in my original review, so this is probably a good change.
Attachment #8936943 - Flags: review?(jhofmann) → review+
(In reply to Johann Hofmann [:johannh] from comment #30)
> Comment on attachment 8936942 [details]
> Bug 1413780 - Add a test for canvas permission to test_SitePermissions.js
> 
> https://reviewboard.mozilla.org/r/207672/#review214126
> 
> That test should work but what I was mostly concerned about is that
> test_SitePermissions.js will fail in testPermissionsListing() and
> testCanvasPermission() in any build that has privacy.resistFingerprinting
> prefed on. It might be worth adding a condition to these tests like
> persistent-storage does it.

Sorry I misunderstood it. I fixed testPermissionsListing(), and I guess the new testCanvasPermission() doesn't need update because privacy.resistFingerprinting is explicitly set in it.

(In reply to Johann Hofmann [:johannh] from comment #31)
> Comment on attachment 8936943 [details]
> Bug 1413780 - Change permission name from canvas/extractData to canvas.
> 
> https://reviewboard.mozilla.org/r/207674/#review214132
> 
> r+ because this should technically work, but think carefully about the
> consequences. Everyone who has already set any canvas/extractData permission
> will lose those permissions and get prompted again. Since this is a
> prefed-off feature I wouldn't assume that this impacts a lot of users, I
> just wanted to make sure we're aware of it.
> 
> I should have really required a different permission name in my original
> review, so this is probably a good change.

The name "canvas/extractData" looked good to me because it was more unique, but I didn't know it related to the classname. Sorry for that.
Comment on attachment 8936942 [details]
Bug 1413780 - Add a test for canvas permission to test_SitePermissions.js

https://reviewboard.mozilla.org/r/207672/#review214854

Ah, sorry, that was a typo on my side. I meant testExactHostMatch() instead of testCanvasPermission(). I've marked the function below. Otherwise this seems good to me...

::: browser/modules/test/unit/test_SitePermissions.js:109
(Diff revision 2)
>  add_task(async function testExactHostMatch() {
>    let uri = Services.io.newURI("https://example.com");
>    let subUri = Services.io.newURI("https://test1.example.com");
>  
>    let exactHostMatched = ["desktop-notification", "focus-tab-by-prompt", "camera",
>                            "microphone", "screen", "geo"];

Please add the same condition here again :)
Attachment #8936942 - Flags: review?(jhofmann) → review+
I added it to testExactHostMatch() and made it look like the persistent-storage one, using a global const RESIST_FINGERPRINTING = Services.prefs.getBoolPref("privacy.resistFingerprinting").

I really appreciate your help!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ce3ba591b61a
Make canvas permission visible in site information panel. r=johannh
https://hg.mozilla.org/integration/autoland/rev/0731f1b72c49
Hide canvas permission from site information panel when privacy.resistFingerprinting is false. r=johannh
https://hg.mozilla.org/integration/autoland/rev/577004586b2d
Update icon design and add blocked effect. r=johannh
https://hg.mozilla.org/integration/autoland/rev/d09dd833d6f0
Add a test for canvas permission to test_SitePermissions.js r=johannh
https://hg.mozilla.org/integration/autoland/rev/acc307373696
Change permission name from canvas/extractData to canvas. r=johannh
Keywords: checkin-needed
I don't see this on Firefox for Android ?
running latest nightly
Tom, is there a plan to bring the permission prompt for canvas to Android? If so, we should probably file a bug on it.
Flags: needinfo?(tom)
(In reply to Johann Hofmann [:johannh] from comment #49)
> Tom, is there a plan to bring the permission prompt for canvas to Android?
> If so, we should probably file a bug on it.

There is no plan, but we should do that, yes. Bug 1432506
Flags: needinfo?(tom)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: