1440574 - Policy: Disable commands to send feedback

Status: VERIFIED FIXED

(Firefox :: Enterprise Policies, defect)

Description

[YUKI "Piro" Hiroshi]

Some system administrators want to disable feedback commands for safe browsing features, to prevent data breach.

Comment 1

[YUKI "Piro" Hiroshi]

Related menu items:

 * Help
   * Submit Feedback
   * Report Deceptive Site
   * This isn’t a deceptive site"

Comment 2

[YUKI "Piro" Hiroshi]

Attachment: Patch to implement policy "DisableFeedbackCommands"

I've tried to implement a policy "DisableFeedbackCommands". How about this?

Comment 3

[YUKI "Piro" Hiroshi]

Attachment: Patch to implement policy "DisableFeedbackCommands"

Updated patch for recent changes.

Comment 4

[YUKI "Piro" Hiroshi]

Attachment: Patch to implement policy "DisableFeedbackCommands"

Fix broken test

Comment 5

[YUKI "Piro" Hiroshi]

Attachment: Patch to implement policy "DisableFeedbackCommands"

Remove needless changes. Sorry to update again and again...

Comment 6

[YUKI "Piro" Hiroshi]

Attachment: Patch to implement policy "DisableFeedbackCommands"

Fix mismatched trigger: onBeforeAddons => onBeforeUIStartup

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8956000 [details] [diff] [review]
Patch to implement policy "DisableFeedbackCommands"

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

Thanks Yuki for working on policies! This is in the right direction, but there are a few changes needed, and you'll also need to update the test to accomodate these new changes that I have requested.

::: browser/base/content/browser-safebrowsing.js
@@ +15,5 @@
>      // will point to the internal error page we loaded instead.
>      var docURI = gBrowser.selectedBrowser.documentURI;
>      var isPhishingPage =
>        docURI && docURI.spec.startsWith("about:blocked?e=deceptiveBlocked");
> +    var disabledByPolicy = Services.policies.isAllowed("DisableFeedbackCommands");

This is incorrect, and will always mean that the policy is active. isAllowed should use the same string that was given to .disallowFeature. Then, in this case,  isAllowed("feedbackCommands") will return false for when the policy is active, and true if it's not active.

However, the UX direction that we have right now is not to hide any disabled command, but to just disable them (and display them as disabled).

So you can simply just never enable the broadcaster.

::: browser/base/content/browser.js
@@ +1354,5 @@
> +        let element = document.getElementById(feedbackItem);
> +        if (element)
> +          element.setAttribute("hidden", "true");
> +      }
> +    }

This works but it's unecessary to run this code on the window startup code. It's only necessary to do this when the help menu is being opened. So you could add this (although disabling instead of hidden) in the function buildHelpMenu().

But I have a question: is it necessary to disable this function too? It does not cause any data leakage, it's just a normal website being visited, where users can type whatever they want.

::: browser/components/enterprisepolicies/schemas/policies-schema.json
@@ +115,5 @@
>        "type": "boolean"
>      },
>  
> +    "DisableFeedbackCommands": {
> +      "description": "Prevents ability to send feedback.",

Make this description a bit more detailed by also mentioning the report phishing site

Comment 8

[YUKI "Piro" Hiroshi]

Thank you for reviewing!

(In reply to :Felipe Gomes (needinfo me!) from comment #7)
> But I have a question: is it necessary to disable this function too? It does
> not cause any data leakage, it's just a normal website being visited, where
> users can type whatever they want.

On some companies' security policy, any feedback from inside the company is restricted, because people may post secret information as a part of feedback accidentally, regardless of the note in page like "please don't post sensitive data". (Of course such troubles should be avoided in various way including training, but this is also one of such provisions.)

Comment 9

[YUKI "Piro" Hiroshi]

Attachment: Patch to implement policy "DisableFeedbackCommands" v2

Updated patch based on the review result. Codes around updating of broadcasters were totally rewritten to update disabled status of both "report phishing" and "report error" broadcasters.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8956316 [details] [diff] [review]
Patch to implement policy "DisableFeedbackCommands" v2

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

Perfect, thanks

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7afca6992b72
Policy: Disable commands to send feedback. r=felipc

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a87db9282d8
Policy: Disable commands to send feedback: Fix eslint errors by adding semicolons. r=eslint-fix

Comment 13

[Andreea Pavel [:apavel]]

Backed out 2 changesets (bug 1440574) for failing browser chrome at browser/components/enterprisepolicies/tests/browser/browser_policy_disable_feedback_commands.js

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7afca6992b724c6bd29b6021cf7f54b56b36b028

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=166491629&repo=mozilla-inbound&lineNumber=6913

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e7c0ffb21cd4779261f63825865a2deb2cce97

Comment 14

[YUKI "Piro" Hiroshi]

Hmm, the log says that a request to http://example.com/ is timed out but the test is based on an existing another test using same URL:
https://dxr.mozilla.org/mozilla-central/source/browser/components/safebrowsing/content/test/browser_bug415846.js
I don't know why browser_bug415846.js successes and browser_policy_disable_feedback_commands.js fails...

Comment 15

[:Felipe Gomes (needinfo for replies!)]

Can you pass the desired URL directly to withNewTab and see if it works? If the default call doesn't work because of the missing load event (as the comment indicates), you can pass a { waitForLoad: false, waitForStateStop: true } option to withNewTab  (see the documentation on BrowserTestUtils.openNewForegroundTab). 

BTW, the check for the "Submit Feedback..." entry could be outside of the check_all_feedback_commands_hidden function, because it doesn't depend on what page is loaded.

Comment 16

[YUKI "Piro" Hiroshi]

Attachment: Patch to implement policy "DisableFeedbackCommands" v3

Updated patch:

 * Test for "Submit Feedback..." is separated.
 * Use "waitForLoad: false, waitForStateStop: true" options.
 * Fixed eslint errors.

I've confirmed that the test passes on my environment by:
----
$ ./mach test browser/components/enterprisepolicies/tests/br
owser/browser_policy_disable_feedback_commands.js
----

Comment 17

[Bogdan Tara[:bogdan_tara | bogdant]]

This failed to apply:

applying DisableFeedbackCommand.diff
patching file browser/components/enterprisepolicies/tests/browser/browser.ini
Hunk #1 FAILED at 17
1 out of 1 hunks FAILED -- saving rejects to file browser/components/enterprisepolicies/tests/browser/browser.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh DisableFeedbackCommand.diff

Comment 18

[YUKI "Piro" Hiroshi]

Attachment: Patch to implement policy "DisableFeedbackCommands" v3.1

Updated patch, based on the latest mozilla-central.

Comment 19

[YUKI "Piro" Hiroshi]

The attachment 8958068 [details] [diff] [review] is a rebased version for the latest mozilla-central, so I've canceled  re-reviewing.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1962477d8dc7
"Policy: Disable commands to send feedback" r=Felipe

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f003e2175222
Policy: Disable commands to send feedback: Replace tab character with whitespaces. r=eslint-fix on a CLOSED TREE

Comment 22

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3bf314ae495
Backed out 2 changesets for failing browser/components/enterprisepolicies/tests/browser/browser_policy_disable_feedback_commands.js on OS X a=backout on a CLOSED TREE

Comment 23

[Andrei Ciure[:aciure]]

Backed out 2 changesets (bug 1440574) for failing browser/components/enterprisepolicies/tests/browser/browser_policy_disable_feedback_commands.js on OS X a=backout on a CLOSED TREE

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1962477d8dc711aee713e1a203bac45aef8f1b3b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=167410221&filter-searchStr=OS+X+10.10+debug+test-macosx64%2Fdebug-test-verify-e10s+%28TV%29

failure: 
t2: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1962477d8dc711aee713e1a203bac45aef8f1b3b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=167410221&filter-searchStr=OS+X+10.10+debug+test-macosx64%2Fdebug-test-verify-e10s+%28TV%29
log: https://treeherder.mozilla.org/logviewer.html#?job_id=167410221&repo=mozilla-inbound

t1: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&fromchange=a0014a24f1e72bb1746cb9975068953d1f3451e2&selectedJob=167423481&filter-searchStr=OS+X+10.10+debug+Mochitests+with+e10s+test-macosx64%2Fdebug-mochitest-browser-chrome-e10s-4+M-e10s%28bc4%29
log: https://treeherder.mozilla.org/logviewer.html#?job_id=167423481&repo=mozilla-inbound

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/b3bf314ae4951b775285d4b33559368ba36e1e4a

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Hi Yuki, do you have access to tryserver? It would be good to send these patches to try before landing them, to make sure they won't be backed out again.

If you don't have access let me know and I can vouch for you. To use it, there's this guide here: https://wiki.mozilla.org/ReleaseEngineering/TryServer#Scheduling_jobs_with_Try_Syntax

But I find that guide unecessarily long. Basically I just access this page:
https://mozilla-releng.net/trychooser/

Choose Windows, Linux and Mac as platforms, All unit tests, and use the command it gives me. One example is:

./mach try -b o -p linux,macosx64,win64 -u all -t none

Comment 25

[YUKI "Piro" Hiroshi]

(In reply to :Felipe Gomes (needinfo me!) from comment #24)
> If you don't have access let me know and I can vouch for you. To use it,
> there's this guide here:

Thanks, I've filed the bug 1445124 to request access right to the try server.

Comment 26

[YUKI "Piro" Hiroshi]

Attachment: Patch to implement policy "DisableFeedbackCommands" v3.2

Updated patch.

 * Don't wait until STATE_STOP for each test.

Running test on try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73de6cc54f3df5b8752a85b3856f36da31143665

Comment 27

[:Felipe Gomes (needinfo for replies!)]

So it appears that the openPopup menu function is being unreliable on Mac. I made a small test modification that just calls buildHelpMenu() directly, and I'll land the patch

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/305a2cbfdd02
Policy: Disable commands to send feedback. r=felipe

Comment 29

[YUKI "Piro" Hiroshi]

(In reply to :Felipe Gomes (needinfo me!) from comment #27)
> So it appears that the openPopup menu function is being unreliable on Mac.

Hmm, I also think so, I've just found that another test skips assertions around menu bar on macOS:
https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/browser/base/content/test/permissions/browser_reservedkey.js#43

Anyway, thank you for the modification and landing!

Comment 30

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/305a2cbfdd02

Comment 31

[YUKI "Piro" Hiroshi]

Attachment: Patch for Firefox 60 beta

Patch based on the commit on mozilla-central https://hg.mozilla.org/mozilla-central/rev/305a2cbfdd02 for mozilla-beta. "r+" is carried over the original commit on mozilla-central.

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: Missing enterprise policy around disabling feedback commands.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Not risky.
[Why is the change risky/not risky?]: This just controls availability of feedback commands, and it is well tested by an automated test.
[String changes made/needed]: None.

Comment 32

[YUKI "Piro" Hiroshi]

[Tracking Requested - why for this release]:
Enterprise policy for ESR.

Comment 33

[:Felipe Gomes (needinfo for replies!)]

Hey Yuki, during testing we discovered that the "Report Deceptive Site" item is permanently disabled now. It appears that setting disabled=false is not enough to enable it, it's necessary to do a removeAttribute("disabled"), as it was in the original code.  Could you write a patch to fix that?

Comment 34

[YUKI "Piro" Hiroshi]

Attachment: Clear "disabled" attribute for enabled items and broadcasters

Sorry, I didn't looked what disabled=false actually produces. After this patch "disabled" attribute will be cleared completely.

Test results on the try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=240d21869048dfbefc916017066ecdda1453b1e3

Comment 35

[YUKI "Piro" Hiroshi]

Attachment: Patch for Firefox 60 beta v2

Updated patch based including fix for the problem described at the comment #33.

Comment 36

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8960539 [details] [diff] [review]
Clear "disabled" attribute for enabled items and broadcasters

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

So the change in utilityOverlay.js is not necessary (which means the change on the beta patch for bug 1440573 is also uneeded).

The .disabled property works fine if there isn't a disabled="true" attribute. It's only when that attribute exists that the .disabled doesn't override it. So it's only necessary for the "Report Deceptive Site" broadcasters.

Comment 37

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8960542 [details] [diff] [review]
Patch for Firefox 60 beta v2

feedback+ to get a patch version that doesn't include the changes in utilityOverlay.

BTW, Yuki, could you configure your username and e-mail on mercurial, so that your generated patches include this information?

Comment 38

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8960539 [details] [diff] [review]
Clear "disabled" attribute for enabled items and broadcasters

I'll land this a version of this patch that only includes the changes to browser-safebrowsing.js

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/366fd2db317e
Follow-up: fix command broadcasters. r=felipe

Comment 40

[:Felipe Gomes (needinfo for replies!)]

Attachment: Rolled-up patch for beta. r=felipe (author=yuki)

Approval Request Comment
[Feature/Bug causing the regression]: Disable Feedback Commands ("Submit Feedback" and "Report Deceptive Site" from the help menu)
[User impact if declined]: Missing policy
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: already verified
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: there was a small follow-up to make this work properly in the help menu, which has landed. This roll-up patch contains the original patch and the follow-up
[String changes made/needed]: none

Comment 41

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/366fd2db317e

Comment 42

[Julien Cristau [:jcristau]]

Comment on attachment 8961628 [details] [diff] [review]
Rolled-up patch for beta. r=felipe (author=yuki)

adding an enterprise policy, beta60+

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2285a7015600

Comment 44

[Abe - QA (:Abe_LV)]

We tested this on latest Nightly(61.0a1) and beta (60.0b7) using JSON and ADM policy formats and it is verified as Fixed.
When this policy is active, "Submit Feedback...", and "This isn't a deceptive site..." options in the help menu become inactive (greyed-out).

Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760