1413780 - Make canvas permission a first-class permission

Status: RESOLVED FIXED

(Firefox :: Site Identity, enhancement, P1)

Description

[Chung-Sheng Fu [:cfu]]

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?

Comment 1

[Chung-Sheng Fu [:cfu]]

Attachment: 0001-Make-canvas-permission-visible-in-site-information-p.patch

Comment 2

[Chung-Sheng Fu [:cfu]]

Attachment: Screen Shot 2017-11-02 at 4.39.44 PM.png

The patch simply fixes the problem, and this image is how it looks.  Do we have to hide it when privacy.resistFingerprinting is false?

Comment 3

[Tom Ritter [:tjr]]

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?

Comment 5

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1413780 - Make canvas permission visible in site information panel.

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

Comment 6

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1413780 - Hide canvas permission from site information panel when privacy.resistFingerprinting is false.

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

Comment 7

[Chung-Sheng Fu [:cfu]]

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.

Comment 8

[Johann Hofmann [:johannh]]

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!

Comment 9

[Johann Hofmann [:johannh]]

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.

Comment 10

[Johann Hofmann [:johannh]]

(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!

Comment 11

[Chung-Sheng Fu [:cfu]]

(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!

Comment 12

[Chung-Sheng Fu [:cfu]]

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!

Comment 13

[Eric Pang [:epang] UX]

Attachment: 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!

Comment 14

[Chung-Sheng Fu [:cfu]]

Thank you very much! I will update them.

Comment 15

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1413780 - Update icon design and add blocked effect.

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

Comment 16

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8930845 [details]
Bug 1413780 - Make canvas permission visible in site information panel.

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

Comment 17

[Chung-Sheng Fu [:cfu]]

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

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

Comment 18

[Chung-Sheng Fu [:cfu]]

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 19

[Johann Hofmann [:johannh]]

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

Comment 20

[Johann Hofmann [:johannh]]

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 21

[Johann Hofmann [:johannh]]

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.

Comment 22

[Johann Hofmann [:johannh]]

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.

Comment 23

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1413780 - Add a test for canvas permission to test_SitePermissions.js

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

Comment 24

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1413780 - Change permission name from canvas/extractData to canvas.

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

Comment 25

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8930845 [details]
Bug 1413780 - Make canvas permission visible in site information panel.

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

Comment 26

[Chung-Sheng Fu [:cfu]]

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

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

Comment 27

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8934144 [details]
Bug 1413780 - Update icon design and add blocked effect.

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

Comment 28

[Chung-Sheng Fu [:cfu]]

(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 29

[Johann Hofmann [:johannh]]

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.

Comment 30

[Johann Hofmann [:johannh]]

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.

Comment 31

[Johann Hofmann [:johannh]]

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.

Comment 32

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8930845 [details]
Bug 1413780 - Make canvas permission visible in site information panel.

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

Comment 33

[Chung-Sheng Fu [:cfu]]

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

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

Comment 34

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8934144 [details]
Bug 1413780 - Update icon design and add blocked effect.

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

Comment 35

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8936942 [details]
Bug 1413780 - Add a test for canvas permission to test_SitePermissions.js

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

Comment 36

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8936943 [details]
Bug 1413780 - Change permission name from canvas/extractData to canvas.

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

Comment 37

[Chung-Sheng Fu [:cfu]]

(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 38

[Johann Hofmann [:johannh]]

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 :)

Comment 39

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8930845 [details]
Bug 1413780 - Make canvas permission visible in site information panel.

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

Comment 40

[Chung-Sheng Fu [:cfu]]

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

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

Comment 41

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8934144 [details]
Bug 1413780 - Update icon design and add blocked effect.

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

Comment 42

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8936942 [details]
Bug 1413780 - Add a test for canvas permission to test_SitePermissions.js

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

Comment 43

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8936943 [details]
Bug 1413780 - Change permission name from canvas/extractData to canvas.

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

Comment 44

[Chung-Sheng Fu [:cfu]]

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!

Comment 45

[Pulsebot]

Pushed by [e-mail]:
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

Comment 46

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/ce3ba591b61a
https://hg.mozilla.org/mozilla-central/rev/0731f1b72c49
https://hg.mozilla.org/mozilla-central/rev/577004586b2d
https://hg.mozilla.org/mozilla-central/rev/d09dd833d6f0
https://hg.mozilla.org/mozilla-central/rev/acc307373696

Comment 47

[jwms]

I don't see this on Firefox for Android ?

Comment 48

[jwms]

running latest nightly

Comment 49

[Johann Hofmann [:johannh]]

Tom, is there a plan to bring the permission prompt for canvas to Android? If so, we should probably file a bug on it.

Comment 50

[Tom Ritter [:tjr]]

(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