1348257 - Checking "Remember this decision" for WebRTC prompts in PBM should store for session only

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect, P1)

Description

[Johann Hofmann [:johannh]]

Right now I can persist this setting permanently, even if set in private mode. We should probably get it in line with the other prompts.

[Tracking Requested - why for this release]:
Violating the PBM promise of not storing user data except bookmarks etc.

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1348257 - Hide the "always remember" checkbox for webrtc prompts in PBM.

It's important to not remember any user choices made in private mode
(except the ones we specifically mention in the private browsing landing
page). The original idea was to change the checkbox behavior to store
the permission for session duration if in PBM, but this would actually be
inconsistent with the other permission prompts which hide it completely.

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

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8849490 [details]
Bug 1348257 - Hide the "always remember" checkbox for webrtc prompts in PBM.

https://reviewboard.mozilla.org/r/122276/#review124724

LGTM, but I think we should have a test for this.

I wonder if we can prevent this sort of bug by having a PBM check at a lower level (like in SitePermissions.jsm maybe).

Also I wish MozReview would highlight matching braces, haha.

Comment 3

[Johann Hofmann [:johannh]]

Yeah, I thought I'd try if I can sneak this in without a test :D

Technically I'm not even sure if this needs one. Since we test the API behavior for correctness there's not much that can go wrong assuming nobody outright deletes the option.

Considering this, would you be ok with adding a test in a new bug (for all permission prompts, not just WebRTC)? I'd like to uplift this ASAP and there's bug 1349513 which would have to be resolved first to avoid littering b/b/c/test/general.

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8849490 [details]
Bug 1348257 - Hide the "always remember" checkbox for webrtc prompts in PBM.

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

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8849490 [details]
Bug 1348257 - Hide the "always remember" checkbox for webrtc prompts in PBM.

https://reviewboard.mozilla.org/r/122276/#review124890

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8849490 [details]
Bug 1348257 - Hide the "always remember" checkbox for webrtc prompts in PBM.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0837622d7105
Hide the "always remember" checkbox for webrtc prompts in PBM. r=Nihanth

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0837622d7105

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8849490 [details]
Bug 1348257 - Hide the "always remember" checkbox for webrtc prompts in PBM.

Approval Request Comment
[Feature/Bug causing the regression]: Was originally implemented in bug 1282768
[User impact if declined]: Users may accidentally persist the fact that they've visited a website if they permanently deny a camera prompt in private mode.
[Is this code covered by automated tests?]: Kind of. The API for the checkbox is tested, but hiding it in PBM will receive tests once bug 1349513 is solved.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: Go to https://permission.site in private mode. Click on "notifications". Check if you can persist your choice permanently.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Very simple frontend change that is trivial to manually verify.
[String changes made/needed]: None

Comment 10

[Gerry Chang [:gchang]]

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 11

[Simona Badau, Desktop QA]

I can confirm that in PBM the checkbox to "Remember this decision" for WebRTC prompts is not available anymore and the permissions can be given only temporarily for: Camera, Microphone, Camera and Microphone, Notifications and Screen sharing. Tested using the latest Nightly 55.0a1 (Build ID: 20170326030204) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12 on the following websites: 
https://permission.site/
https://people-mozilla.org/~fqueze2/webrtc/

I do have a question related to the way the temporary notifications are given. For example, if I give a temporary permission for the camera and I refresh the page, then the permission is requested again. But, if I give a temporary permission for the notifications and I refresh the page, the permission is still granted. 
Johann, is this behavior intended?

Comment 12

[Johann Hofmann [:johannh]]

Yes, the allow button for notifications is intended to permanently allow, see bug 1341742. :)

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8849490 [details]
Bug 1348257 - Hide the "always remember" checkbox for webrtc prompts in PBM.

Verified fix for a privacy issue, seems very minor/low risk, let's take it on beta.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora uplift.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

nvm, was a trivial rebase

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/93a70cd28ef9

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5d3641819ecc

Comment 18

[Simona Badau, Desktop QA]

(In reply to Johann Hofmann [:johannh] from comment #12)
> Yes, the allow button for notifications is intended to permanently allow,
> see bug 1341742. :)

Thanks Johann, based on this and on Comment 11, setting the tracking flag for firefox55 to verified.

Comment 19

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox Aurora 54.0a2 and on Firefox Beta 53.0b8 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.
I can confirm that in PBM the "Remember this decision" checkbox for WebRTC prompts isn't available anymore and the permissions can be given only temporarily for: Camera, Microphone, Camera and Microphone, Location and Screen/Window sharing.

However, in PBM the "Remember this decision" checkbox for Notifications is still available due to bug 1341742 which hasn't been uplifted.
Shouldn't this be uplifted?

Comment 20

[Johann Hofmann [:johannh]]

(In reply to Hani Yacoub from comment #19)
> Verified as fixed on Firefox Aurora 54.0a2 and on Firefox Beta 53.0b8 on
> Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.
> I can confirm that in PBM the "Remember this decision" checkbox for WebRTC
> prompts isn't available anymore and the permissions can be given only
> temporarily for: Camera, Microphone, Camera and Microphone, Location and
> Screen/Window sharing.
> 
> However, in PBM the "Remember this decision" checkbox for Notifications is
> still available due to bug 1341742 which hasn't been uplifted.
> Shouldn't this be uplifted?

No, the patch for bug 1341742 contains strings and can't be uplifted. The checkbox there is fine, it will only save for session duration in PBM.