Closed Bug 1503893 Opened Last year Closed 11 months ago

"DisableFirefoxScreenshots" policy is not being successfully applied

Categories

(Firefox :: Enterprise Policies, defect, P1, critical)

defect

Tracking

()

VERIFIED FIXED
Firefox 65
Tracking Status
firefox-esr60 --- unaffected
firefox63 --- unaffected
firefox64 --- verified
firefox65 --- verified

People

(Reporter: ciprian_georgiu, Assigned: _6a68)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached file policies.json
[Affected versions]:
- latest Nightly 65.0a1
- Beta 64.0b5

[Affected platforms]:
- Windows 10 x64
- macOS 10.13
- Ubuntu 16.04 x64

[Preconditions]:
- enable “DisableFirefoxScreenshots” policy via polices.json or GPO

[Steps to reproduce]:
1. Launch Firefox.
2. Access any webpage and take a screenshot via context menu.

[Expected result]:
- Firefox screenshots is disabled.

[Actual result]:
- Firefox screenshots is enabled.

[Regression range]:
- Last good revision: e18f1acd0782f6333ee892c7ebc9711d9a5e550a
  First bad revision: 70a5ef5872f895009bfdf2958616ca0ecece807f
  Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e18f1acd0782f6333ee892c7ebc9711d9a5e550a&tochange=70a5ef5872f895009bfdf2958616ca0ecece807f

[Additional notes]:
- In order to reproduce this issue more easily I have attached the policies.json file
This shouldn't block bug 1445943 since this issue is also present with JSON policies. Bug 1445943 is simply adding support for policies via `defaults write` and configuration profiles but does not change the way policies are being applied by Firefox.
No longer blocks: 1445943
Severity: normal → major
Summary: Firefox Screenshots is not disabled via polices.json → "DisableFirefoxScreenshots" policy is not being successfully applied
I think bug 1488971 + bug 1498410 combined caused this bug.

IIUC, in bug 1488971, monitor of pref change was moved outside of extension (and delayed to window restored), but the check on startup was still in bootstrap.js [1], and that check was then removed as part of bug 1498410.

[1]: https://hg.mozilla.org/mozilla-central/rev/3113a83b3455#l3.48
Blocks: 1488971
Severity: major → critical
Priority: -- → P1
Jared, can you take a look at this please?

Note that pocket moved to a WebExtension but was still able to honor the pref. Not sure how.
Flags: needinfo?(jhirsch)
The addons team asked that we move pref-based management into the addons code, and out of the webextension. Maybe aswan has ideas.
Flags: needinfo?(jhirsch) → needinfo?(aswan)
To clarify, I can take a look as well, but aswan may have better ideas
> Note that pocket moved to a WebExtension but was still able to honor the pref. Not sure how.

I don't see pocket in the list of webextensions in browser/extensions[1], so maybe their code was just merged into the tree.

Are there any known issues with webcompat-reporter (assuming it's exposed via an enterprise policy)? It seems to be using almost the same nsBrowserGlue check as screenshots[2].


[1] https://searchfox.org/mozilla-central/source/browser/extensions
[2] https://searchfox.org/mozilla-central/source/browser/components/nsBrowserGlue.js#1457-1469
pocket is not a webextension.
The code that monitors that pref is apparently only checking for runtime changes to the pref, if the value flips before the observer is attached (and I assume policies are applied early during startup?) then nothing will ever notice.
Flags: needinfo?(aswan)
Do we want to get this patch uplifted to 64?
Flags: needinfo?(mozilla)
Comment on attachment 9023033 [details]
Bug 1503893 - Ensure Screenshots disable pref is checked at startup; r?aswan

Hey, could you take a look at this patch? It's marked critical, and I suspect will be targeting uplift into 64
Flags: needinfo?(aswan)
Yes, please.
Flags: needinfo?(mozilla)
I think the needinfo was just to remind me that this was also on Phabricator?
If I misunderstood and there's something else please re-flag me
Flags: needinfo?(aswan)
Pushed by jhirsch@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ecf239647a4
Ensure Screenshots disable pref is checked at startup; r=aswan
I wonder why the test at https://searchfox.org/mozilla-central/source/browser/components/enterprisepolicies/tests/browser/disable_fxscreenshots/ never failed. Is it the case that if this pref was set on a fresh profile (before the first run of Firefox Screenshots), it would work correctly? (because that's how this test behaves)
https://hg.mozilla.org/mozilla-central/rev/0ecf239647a4
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Flags: qe-verify+
Assignee: nobody → jhirsch
Comment on attachment 9023033 [details]
Bug 1503893 - Ensure Screenshots disable pref is checked at startup; r?aswan

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1488971

User impact if declined: Firefox Screenshots will not be disabled by the enterprise group policy

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: Set the 'DisableFirefoxScreenshots' policy, start / restart Firefox, verify none of the Screenshots UI is visible (Library menu item, page action item, context menu item). Visit screenshots.firefox.com/shots and verify no shots are shown.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The change is small and only affects Screenshots by checking a pref's state at startup, rather than only listening for the pref to be changed.

String changes made/needed: None
Attachment #9023033 - Flags: approval-mozilla-beta?
This bug is no longer reproducible on latest Nightly 65.0a1 (2018-11-15) across platforms: Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x64.

Firefox Screenshots is successfully disabled (from Library, Context Menu and Page Actions) after the policy is applied.

Needinfo myself as a reminder, to verify this on Beta once the patch is approved and uplifted.
Status: RESOLVED → VERIFIED
Flags: needinfo?(ciprian.georgiu)
Comment on attachment 9023033 [details]
Bug 1503893 - Ensure Screenshots disable pref is checked at startup; r?aswan

fix regression with screenshots and enterprise policies, approved for 64.0b10
Attachment #9023033 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This issue is also verified fixed on 64.0b10 running Windows 10 x64, macOS 10.12 and Ubuntu 16.04 x64.
Flags: qe-verify+
Flags: needinfo?(ciprian.georgiu)
You need to log in before you can comment on or make changes to this bug.